Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/build/maintner, x/build/maintner/maintnerd/maintapi: (*netMutSource).syncSeg occasionally fails with "Access is denied." on Windows, as caught by TestGetRef failing on windows-amd64-longtest #36163

Closed
bcmills opened this issue Dec 16, 2019 · 5 comments
Closed

x/build/maintner, x/build/maintner/maintnerd/maintapi: (*netMutSource).syncSeg occasionally fails with "Access is denied." on Windows, as caught by TestGetRef failing on windows-amd64-longtest #36163

bcmills opened this issue Dec 16, 2019 · 5 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 16, 2019 •
edited
Loading

It appears that maintner is trying to install log segments using atomic renames:
https://github.com/golang/build/blob/0d536a3af5fb211db2c73a4499672e9c77ed06cb/maintner/netsource.go#L580-L582

Note that Windows does not actually ensure the success of atomic calls to os.Rename (see also #34681).

For cmd/go we have an internal workaround in cmd/go/internal/robustio, which retries errors suspected to be spurious, although at this point we're mostly switching over to explicit file-locking rather than renaming for synchronization (see also #33974).

2019-12-13T18:18:58-fba2c51/windows-amd64-longtest

--- FAIL: TestGetRef (9.50s)
    api_test.go:191: getting corpus: syncing segment 27: rename C:\Users\gopher\.cache\golang-maintner\tempseg028062994 C:\Users\gopher\.cache\golang-maintner\0027.5c4a6891e9788c963979597b118bc64d1571078489b05b69be0f53be.mutlog: Access is denied.

CC @dmitshur @toothrot @cagedmantis @bradfitz
@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 16, 2019
@bcmills bcmills added this to the Backlog milestone Dec 16, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Dec 16, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Feb 20, 2020

https://build.golang.org/log/0420d03897485cb745280248cb0e951b8a866917

--- FAIL: TestGetRef (11.13s)
    api_test.go:191: getting corpus: syncing segment 46: rename C:\Users\gopher\.cache\golang-maintner\tempseg000044895 C:\Users\gopher\.cache\golang-maintner\0046.bb5084c76340045b19e6d06755ef9be321888b1344474d24be5fe494.mutlog: Access is denied.

@toothrot
Copy link
Contributor

Retrying, as robustio does, seems like a good solution.

@bcmills
Copy link
Contributor Author

bcmills commented Feb 20, 2020

If you happen to decide to use explicit file-locking instead, see #33974.

@dmitshur dmitshur mentioned this issue Jun 2, 2020
Open
@dmitshur dmitshur changed the title x/build/maintner: (*netMutSource).syncSeg occasionally fails with "Access is denied." on Windows x/build/maintner, x/build/maintner/maintnerd/maintapi: (*netMutSource).syncSeg occasionally fails with "Access is denied." on Windows, as caught by TestGetRef failing on windows-amd64-longtest Jun 2, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/361334 mentions this issue: maintner: add and use a copy of cmd/go/internal/robustio for atomic renames

gopherbot pushed a commit to golang/build that referenced this issue Nov 5, 2021
maintner: add and use a copy of cmd/go/internal/robustio for atomic r…
b427d95 
…enames

os.Rename is not atomic on Windows, although in practice it seems to
be mostly-atomic most of the time. At the very least, this should
substantially reduce the maintner failure rate for Windows users (if
there are any) and on the windows-amd64-longtest builder.

It would be better still to refactor netMutSource to avoid assuming
that rename is atomic and to do something else instead, like appending
to the file's final location and using the file's length or checksum
to check for completeness, or using file-locking (which does work on
Windows, at least on NTFS filesystems) instead.

(This change, in contrast, seems like the shortest path to something
that works well enough to pass on the builders.)

For golang/go#36163

Change-Id: Iad20d284d1dde798df66d1b493193e646cd70c4d
Reviewed-on: https://go-review.googlesource.com/c/build/+/361334
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Alexander Rakoczy <[email protected]>
@bcmills
Copy link
Contributor Author

bcmills commented Jan 28, 2022

I think this is basically as fixed as it's going to get, so closing.

(The implementation could stand to be refactored to use a more robust, more portable approach, but I don't think that will ever be a priority.)

@bcmills bcmills closed this as completed Jan 28, 2022
@bcmills bcmills mentioned this issue Jan 28, 2022
Closed
@golang golang locked and limited conversation to collaborators Jan 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@toothrot @bcmills @gopherbot