fix: stop treating a Windows sharing collision as a lost job record - #9
Conversation
TestResubmit_SpawnsNewJobAndLinksBothWays was flaky on Windows CI. The cause turned out not to be the test. Windows opens deny by default, and Go's os.ReadFile asks for FILE_SHARE_READ|FILE_SHARE_WRITE but not FILE_SHARE_DELETE. So while any reader holds a job record open, the rename that publishes a new version of it fails with ERROR_ACCESS_DENIED; while that rename is in flight, a reader fails with ERROR_SHARING_VIOLATION. Both mean "try again in a moment" -- which is what POSIX does implicitly by letting the rename proceed under an open handle -- and both were treated as permanent. Measured with one reader and one writer on a single path: 809 of 2000 renames failed with errno 5 and 857 reads failed with errno 32. Neither failure was reported. Every persist site discards its error (`_ = m.savePersistedSnapshot...`), so a terminal state that failed to write is simply lost; and decodeRecordFile turns a failed read into an UnreadableRecord, which loadPersistedJobs drops from the results entirely. A perfectly good job record therefore vanished from a reload and was described as malformed. atomicfile now waits the collision out: ten attempts, ten milliseconds apart, and only for those two errnos, so a file genuinely held open by something else still fails and says so. isShareViolation is false away from Windows, where the situation cannot arise, so the loops there run exactly once -- same syscalls, no sleep. atomicfile.ReadFile is the read half, and the job record readers use it. The regression test contends a reader and a writer on one path. With the retry it passes 5/5; with isShareViolation forced false it fails 3/3, reporting both directions. The test had a second, smaller fault of its own: it waited for Manager.Get to report a terminal state, then read the record off disk. markTerminalCause flips the in-memory state under the manager lock and persists after releasing it, so that wait did not mean what the read needed it to mean -- and reading inside the window found the successor still running, which sent the loader down its reconcile-and-rewrite path against a file the manager was writing at that moment. It now waits on the record, via the read-only reader rather than the loader that would rewrite what it polls, and asserts nothing came back unreadable. That discarded return is the reason the failure only ever said "map does not contain <id>". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21cdcc7082
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| time.Sleep(shareRetryDelay) | ||
| } | ||
| return err |
There was a problem hiding this comment.
When an editor, antivirus scanner, or another process keeps the destination share-locked beyond this retry window, renameRetrying still returns ERROR_ACCESS_DENIED; however, terminal job persistence discards that result at internal/jobs/manager.go:1339 and other save sites. The temporary file is then removed by Write, so the terminal snapshot is neither retried nor reported, and a restart can reconcile the older running record as abandoned—the same silent durability failure this change intends to fix. Preserve the failed snapshot for a later retry or propagate/report the persistence error instead of relying solely on this short retry window.
Useful? React with 👍 / 👎.
295701d to
21cdcc7
Compare
1 participant
TestResubmit_SpawnsNewJobAndLinksBothWayswas flaky on Windows CI. The cause turned out not to be the test.The bug
Windows opens deny by default, and Go's
os.ReadFileasks forFILE_SHARE_READ|FILE_SHARE_WRITEbut notFILE_SHARE_DELETE. So:ERROR_ACCESS_DENIED(5);ERROR_SHARING_VIOLATION(32).Both mean "try again in a moment" — which is exactly what POSIX does implicitly by letting the rename proceed under an open handle. Both were treated as permanent.
Measured with one reader and one writer contending on a single path:
os.Renameover an open fileos.ReadFileduring a renameWhy it was silent
Neither failure was reported anywhere:
_ = m.savePersistedSnapshot...), so a terminal state that failed to write is simply lost;decodeRecordFileturns a failed read into anUnreadableRecord, andloadPersistedJobsdrops those from its results.So a perfectly good job record vanished from a reload and was described as malformed. This is a real durability bug on Windows, not only a test problem.
The fix
atomicfilewaits the collision out — ten attempts, 10 ms apart, and only for those two errnos, so a file genuinely held open by something else (an editor, a scanner with a long lease) still fails and says so.isShareViolationis false away from Windows, so those loops run exactly once there: same syscalls, no sleep.atomicfile.ReadFileis the read half; the job record readers use it.Evidence
The regression test contends a reader and a writer on one path:
isShareViolationforced false: fails 3/3, reporting both directions (rename: Access is denied,open: The process cannot access the file because it is being used by another process)The test's own smaller fault
It waited for
Manager.Getto report terminal, then read the record off disk.markTerminalCauseflips the in-memory state under the manager lock and persists after releasing it, so that wait did not mean what the read needed it to mean. It now waits on the record itself — via the read-only reader, not the loader that would rewrite what it polls — and asserts nothing came backunreadable. That discarded return is why the failure only ever said "map does not contain ".Checks
go test ./...green;-count=30 -run TestResubmitunder 3 concurrent full-suite runs, twice, clean (~105 iterations total).go vet ./...andgolangci-lint(--max-issues-per-linter=0 --max-same-issues=0) underGOOS=linux,darwin,windows: 0 issues.🤖 Generated with Claude Code