Repository navigation
fix(auth): retry reading a session file while it is replaced on Windows - #275
Merged
Merged
Conversation
The file store replaces a session file with a rename, which retries on Windows while another process has the file open. Reading it had no retry, so a command reading the file during the rename failed with a sharing violation, as in TestAuthRefresh_ManyExpiries. Reads now retry on a sharing violation in the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔍 Full review · 4 files reviewed
Verification
readFilereturns the error unchanged once the error is not a sharing violation, soreadSessionFilestill mapsfs.ErrNotExistto (nil, nil).- The read retry has the same bounds as the rename retry in
WriteFileAtomic: 41 attempts with 25ms sleeps. sharing_windows.goandsharing_windows_test.goare limited to Windows by their_windowsfile-name suffix, andsharing_other.gohas the matching!windowstag.golang.org/x/sysis already a direct requirement in go.mod.- The test uses
sync.WaitGroup.Go, which is supported by the module'sgo 1.27.1directive.
The only coverage is the new TestReadFileWhileReplaced, which is Windows-only. Per the PR description, CI runs the Go unit tests on Linux only, so no automated check runs this test or reaches the retry branch.
Review details
- Commit: 653915a
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
The migration marker is also written by WriteFileAtomic, and read outside the migration lock. Export the retrying read as store.ReadFile and use it there too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pjcdawkins
enabled auto-merge (squash)
October 26, 2026 20:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestAuthRefresh_ManyExpiriesfailed on Windows in a CI run of #272 (it passed on a re-run):With Go auth, the file store replaces a session file with a rename, which
WriteFileAtomicretries on Windows while another process has the file open. Reads of the file, outside the lock, had no retry, so a command that read it during a rename failed with a sharing violation. Reads now retry on a sharing violation, as the rename does (store.ReadFile). The migration marker, the other file thatWriteFileAtomicwrites, is read the same way.TestReadFileWhileReplaced(Windows only) reads a file while it is replaced 500 times. Run in a Windows 11 VM, 10–19 plain reads failed in each run, and none with the retry. CI runs Go unit tests on Linux only, so it doesn't run there.🤖 Generated with Claude Code