fix(windows): retry atomic file renames on Access is denied / Sharing violation#641
fix(windows): retry atomic file renames on Access is denied / Sharing violation#641euxaristia wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAtomic file replacement paths in cron, sandbox, sessions, and swarm now retry Windows permission and sharing-violation rename failures. Mailbox rename behavior is injectable for tests covering retryable and non-retryable errors. ChangesWindows-safe atomic renames
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/swarm/mailbox_test.go (1)
295-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd deterministic coverage for the rename retry path.
This contention test can pass without triggering errno 32 or access denied, especially outside Windows. Add an injectable rename operation and verify retryable errors are retried while other errors stop immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/swarm/mailbox_test.go` around lines 295 - 299, The concurrent mailbox test does not deterministically exercise rename retries. Add an injectable rename operation to the mailbox implementation, then update the relevant test to return retryable errors such as errno 32 or access denied before succeeding and assert all attempts occur; also verify a non-retryable error stops immediately without further retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/swarm/mailbox_test.go`:
- Around line 295-299: The concurrent mailbox test does not deterministically
exercise rename retries. Add an injectable rename operation to the mailbox
implementation, then update the relevant test to return retryable errors such as
errno 32 or access denied before succeeding and assert all attempts occur; also
verify a non-retryable error stops immediately without further retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4295f9-0b76-4089-9f92-94d750679ac6
📒 Files selected for processing (5)
internal/cron/store.gointernal/sandbox/windows_unelevated.gointernal/sessions/store.gointernal/swarm/mailbox.gointernal/swarm/mailbox_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed #641 on a fresh worktree. Build, vet, and tests for the touched packages (cron, sandbox, sessions, swarm) are all green, and gofmt is clean.
The change looks correct and well-scoped. renameWithRetry is bounded (10 attempts), only retries the two named transient Windows errors — ERROR_SHARING_VIOLATION (32) via the explicit isWindowsSharingViolation check and ERROR_ACCESS_DENIED (5) via os.IsPermission, which is exactly how the rest of the codebase already surfaces that code (see internal/cron/lock.go:70) — and preserves the original error by return err after the loop rather than masking it. No race is introduced: src is the caller-owned temp file, nothing re-reads or re-writes it between attempts, and a failed os.Rename does not move the file, so retrying is safe. On non-Windows the runtime.GOOS == "windows" guard means it breaks after the first error, so behavior is identical to plain os.Rename there — no behavior change on Linux/macOS. It composes cleanly with the lock/reclaim work from #615/#616/#631: this retries only the final data-file rename, never the lock acquisition, so the two concerns stay orthogonal.
A few non-blocking notes:
-
The
renameWithRetry+isWindowsSharingViolationpair is now byte-identical across four packages (cron/store.go, sandbox/windows_unelevated.go, sessions/store.go, swarm/mailbox.go), and a fifth variant already lives ininternal/update/replace_windows.go. If the retry policy ever changes, four files drift in lockstep. A tiny shared internal helper would be cleaner; happy to leave that for a follow-up. -
There's no test for the retry path and no comment justifying why. I understand it's gated by
runtime.GOOS == "windows"and so can't fire on Linux CI, but the error classifier (os.IsPermission/isWindowsSharingViolation) is pure and could be unit-tested on Windows, or the loop could take an injectable classifier so the retry decision is testable cross-platform. Not a blocker — the existing concurrent mailbox test still passes and now logs send failures, which is a nice diagnostic touch. -
The 10ms delay is fixed rather than backoff (~90ms worst case), and ERROR_LOCK_VIOLATION (33) isn't retried even though
internal/securefiletreats it as transient. Both are fine for the scoped Access-denied/Sharing-violation fix but worth keeping in mind if the policy is revisited.
Verdict: approve. This is a real Windows-reliability improvement with no correctness or security concern, and it's safe on other platforms.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/swarm/mailbox.go (1)
412-417: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
TestMailboxRenameRetryon non-Windows runners
runtime.GOOS == "windows"means the retry path never runs on Linux/macOS jobs, soTestMailboxRenameRetrywill fail there unless it skips off-Windows or the retry predicate is injectable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/swarm/mailbox.go` around lines 412 - 417, Update TestMailboxRenameRetry to skip execution when runtime.GOOS is not Windows, ensuring the test only exercises the Windows-specific retry behavior. Keep the existing retry logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/swarm/mailbox_test.go`:
- Around line 316-337: Update TestMailboxRenameRetry to skip execution when
runtime.GOOS is not Windows, adding the runtime import and returning early
before setting up the retry scenario. Preserve the existing retry assertions and
behavior on Windows.
---
Outside diff comments:
In `@internal/swarm/mailbox.go`:
- Around line 412-417: Update TestMailboxRenameRetry to skip execution when
runtime.GOOS is not Windows, ensuring the test only exercises the
Windows-specific retry behavior. Keep the existing retry logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7383eb09-1868-4efe-bdfc-732c61b2f257
📒 Files selected for processing (2)
internal/swarm/mailbox.gointernal/swarm/mailbox_test.go
|
Pushed 2af6ab3 to fix the Smoke and Zero Review failures: removed a blank line at EOF in mailbox_test.go flagged by gofmt. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Apply the retry policy to the remaining sandbox state writers
internal/sandbox/windows_unelevated.go:144
This adds a retry only for the unelevated-plan marker, but the same temp-file replacement pattern remains a one-shotos.Renamein the elevated setup marker (windows_setup.go:242), capability-SID store (windows_runner.go:666), and grants store (grants.go:603). A transient Windows sharing/access-denied error therefore still rolls backzero sandbox setup, prevents restricted-command initialization, or rejects a grant/revoke operation—the exact persistence failure the PR says it fixes for sandbox. Route these writers through the same retry policy (and cover them) rather than leaving the primary sandbox paths vulnerable. -
[P1] Retry checkpoint-file replacement during rewinds
internal/sessions/rewind.go:293
RestoreToSequencestill restores workspace files throughwriteFileAtomic, which performs a bare rename. On Windows, an indexer or delayed handle can make that rename return error 5/32; the caller then only appends the file toreport.Skippedand continues. Consequently a rewind can appear successful while leaving files at their later contents, even though this PR advertises retry protection for session atomic writes. Use the retrying helper here as well, or surface this partial restore as a failure.
…elper Convert the 3 remaining sandbox marker/state writers and the sessions rewind writeFileAtomic method to use the existing package-level renameWithRetry helper instead of a bare os.Rename, so a transient Windows Access is denied / Sharing violation no longer rolls back the write.
|
Pushed e8f5b7f: routed the remaining 4 bare os.Rename call sites through the existing retry helper (windows_setup.go's setup marker write, windows_runner.go's capability-SID store write, grants.go's writeState, and sessions/rewind.go's separate writeFileAtomic method used by RestoreToSequence). All 4 reuse the retry helper already defined in their own package — no new helper, no import changes. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Please rebase and resolve conflicts.
@Vasanthdev2004 LGTM otherwise.
|
Closing this. The fix already landed on main via #660: that PR added a shared internal/fsutil.RenameWithRetry helper and wired it into the same call sites this PR touches (cron store, sandbox windows_unelevated marker, sessions store, swarm mailbox), plus equivalent tests. Rebasing this branch onto main leaves an empty diff, so there's nothing left for this PR to contribute. |
3 participants
This PR introduces a renameWithRetry helper to replace os.Rename for atomic file overwrites in sessions, sandbox, cron, and swarm. This helper retries the rename operation up to 10 times with a 10ms backoff when encountering transient Windows sharing violations (error 32) or access denied (error 5) errors, which frequently occur under concurrency on Windows (e.g. from background indexing or delayed file handle releases).
Summary by CodeRabbit
Bug Fixes
Tests