fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix#628
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughLock lifecycle code now delegates lock-file removal and stale-lock reclamation to ChangesStale-lock restoration
Estimated code review effort: 4 (Complex) | ~45 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cron/lock.go (1)
121-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider hoisting this helper into one shared internal package.
restoreLockFileis byte-for-byte identical across cron, daemon, hooks, oauth, and swarm. Safety-critical concurrency logic duplicated 5× will drift — and the Windows fix above now has to land in all five copies. A singleinternal/lockutil.RestoreLockFilewould keep the invariant in one place.🤖 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/cron/lock.go` around lines 121 - 129, Consolidate the duplicated restoreLockFile helper into a shared internal lock utility. Add exported lockutil.RestoreLockFile containing the existing Windows rename and non-Windows link/remove behavior, then remove the local copies in cron, daemon, hooks, oauth, and swarm and update all callers to use the shared function.
🤖 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/cron/lock.go`:
- Around line 121-129: Update restoreLockFile to use a Windows restore operation
that does not replace an existing destination, ensuring it returns an error when
path already exists; preserve the existing hard-link/remove behavior on
non-Windows platforms.
---
Nitpick comments:
In `@internal/cron/lock.go`:
- Around line 121-129: Consolidate the duplicated restoreLockFile helper into a
shared internal lock utility. Add exported lockutil.RestoreLockFile containing
the existing Windows rename and non-Windows link/remove behavior, then remove
the local copies in cron, daemon, hooks, oauth, and swarm and update all callers
to use the shared function.
🪄 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: cb074b82-00ea-484a-b2f9-af563bdefb9c
📒 Files selected for processing (5)
internal/cron/lock.gointernal/daemon/lock.gointernal/hooks/lock.gointernal/oauth/lock.gointernal/swarm/mailbox.go
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] Complete CodeRabbit's no-overwrite restoration request on Windows
internal/cron/lock.go:121
The new Windows branch is stillos.Rename(reclaimed, path), but Go documents thatos.Renamereplaces an existing non-directory destination. If another process acquirespathafter it is moved aside, this restore overwrites that new lock and recreates the split-brain race the PR is intended to eliminate. Please complete CodeRabbit's request with a no-replace restoration primitive on Windows (and apply it to the four identical helpers), then add a regression that creates the competing destination before restore. -
[P1] Preserve a live sidelined lock when restoration fails for another reason
internal/cron/lock.go:112
Every changed reclaim path removesreclaimedafter anyrestoreLockFileerror. That is safe only when a newly-createdpathconclusively caused the failure; an I/O/space error, a filesystem without hard-link support, or another unrelated rename failure instead deletes the verified live holder's lock and allows a new acquirer through. Please keep the sidelined lock and fail closed for errors that do not prove a replacement lock exists, and add a failure-path regression test.
During lock reclamation, reclaiming a suspected stale lock renames the file aside and renames it back if not stale. On POSIX systems, the rename back silently overwrites any new lock file created in the gap. On Windows, it fails to rename back and leaks the stale file. Introduce a cross-platform restoreLockFile helper: - On POSIX: Use os.Link to link the file back (which atomic-fails if the target already exists), then remove the temporary link. - On Windows: Use os.Rename (which safely fails if the target exists). If restoring fails, delete the temporary file immediately to prevent leaks.
b1434e7 to
f544d18
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.
Findings
- [P1] Fix the Windows smoke failure in the no-replace restore helper
internal/lockutil/lockutil_windows.go:20
The requiredSmoke (windows-latest)check fails becausesyscall.MoveFileExis undefined, sogo test ./...cannot compileinternal/lockutilor any package that imports it on Windows. Please complete the prior Windows no-overwrite restoration fix with a supported Windows API binding or implementation, add Windows coverage, and rerun the Windows smoke check.
Addresses jatmn's follow-up review: the required Windows smoke check was failing because syscall.MoveFileEx is not exported by the standard library's syscall package on this platform, so internal/lockutil (and everything importing it: cron, daemon, hooks, oauth) failed to compile on Windows. Switched to golang.org/x/sys/windows, which does export MoveFileEx with the same signature, already a direct module dependency. Verified on a real Windows machine that the no-flags call still fails with ERROR_ALREADY_EXISTS when the destination exists, and that this satisfies errors.Is(err, os.ErrExist), which cron/daemon/hooks/oauth's reclaim paths all depend on to decide whether the sidelined lock was genuinely superseded (safe to discard) versus some other, unrelated failure (must be preserved). TestRestoreLockFile already covered both the successful-restore and fail-when-target-exists cases; it now passes on Windows.
There was a problem hiding this comment.
I traced the lock acquire/release/reclaim path across cron, daemon, hooks, oauth, and swarm, and this is a sound fix.
The mechanism is an O_EXCL-created lock file (not flock), released by remove-if-token-matches, with stale reclaim via rename-aside.
The real bug was that the restore step used os.Rename(reclaimed, lockPath). On POSIX, this atomically replaces the destination, and on Windows, it passes MOVEFILE_REPLACE_EXISTING. If a fresh holder O_EXCL-created lockPath in that tiny gap, the restore would clobber it, creating a two-writer / lost-lock risk.
The new RestoreLockFile correctly fails closed when the target exists (os.Link on POSIX, MoveFileEx with no flags on Windows). On os.ErrExist, the caller drops the sidelined file, which is correct and ensures no leaks. Failed-acquire paths still clean up via RemoveLockFile, and the Windows delete-retry loop handles delete-pending/sharing-violations without skipping the unlock path.
Build, vet, and the touched package tests all pass on Windows (the lockutil test exercises the real MoveFileEx path and confirms errors.Is(err, os.ErrExist) holds), and it cross-compiles for linux.
Minor nit only: the first commit message's Windows rationale is slightly stale (it says os.Rename safely fails), but the final code is correct.
|
Fixed the Windows build failure: internal/lockutil/lockutil_windows.go used syscall.MoveFileEx, which the standard library does not export on Windows. Switched to golang.org/x/sys/windows.MoveFileEx (same signature, already a direct dependency) and verified on real Windows hardware that its no-overwrite failure still satisfies errors.Is(err, os.ErrExist), since the four call sites that reclaim a stale lock depend on that exact behavior. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
H1 — High: non-os.ErrExist restore failure can still lose a live lock / cause split-brain
Files: internal/lockutil/lockutil_other.go:11-16 and the identical caller branches
internal/cron/lock.go:108-116, internal/hooks/lock.go:102-108,
internal/oauth/lock.go:97-103, internal/daemon/lock.go:90-98,
internal/swarm/mailbox.go:384-388.
POSIX RestoreLockFile is os.Link(reclaimed, path) then os.Remove(reclaimed).
os.Link can fail with errors other than os.ErrExist:
EXDEV— cross-device link (overlayfs/bind-mount boundaries, a sidelined temp on a
different mount, some container layouts, exFAT/some FUSE/network mounts);ENOSPC/EPERM/ENOSYS/ELOOP— space, permission, unsupported FS, symlink loop.
On such a failure the caller only cleans reclaimed when errors.Is(err, os.ErrExist),
so it does nothing: reclaimed (the live holder's lock) is left on disk and
lockPath stays missing. The caller returns false; the surrounding acquire loop
then executes its next os.OpenFile(lockPath, O_CREATE|O_EXCL|O_WRONLY, 0o600) — which
succeeds because the file is gone — and the acquirer takes the lock. The original
holder (whose lock is in reclaimed) is still inside its critical section, so two
processes now believe they hold the lock → split-brain / lost mutual exclusion
(corrupted mailbox JSON, duplicate job-metadata read-modify-write, or two
single-instance daemons).
Why this is reachable: the FRESH branch is entered when a holder "reacquired in the
gap" (the post-rename Stat shows mtime <= staleAfter). That race is exactly the
reacquire-in-gap scenario this PR targets, and on any link-incapable/under-pressure
filesystem os.Link fails there. Concrete scenario: A ages out holder B's lock, renames
it aside, re-stats and sees it FRESH, calls RestoreLockFile; os.Link fails (EXDEV);
lockPath is empty; A's loop re-acquires it; B and A both hold.
Same class on Windows: MoveFileEx(...,0) can also fail with ERROR_SHARING_VIOLATION
(32) / ERROR_ACCESS_DENIED (5) / ERROR_NOT_SAME_DEVICE / a UTF16PtrFromString encoding
error — none of which is os.ErrExist — producing the identical "preserve sidelined +
empty lockPath + re-acquire" split-brain.
Not the partial-success case: if os.Link succeeds but os.Remove(reclaimed) fails,
lockPath is correctly restored (still a hard link); only an orphan .stale.* remains
(Low, see L2). The High is specifically when os.Link itself fails with non-os.ErrExist
and lockPath ends up empty.
Recommended fix (closes it on both platforms): RestoreLockFile should not surface a
non-os.ErrExist error when the lock can still be handed back safely. After a
non-os.ErrExist link failure, fall back to an O_EXCL copy (which still fails EEXIST
if a new holder appeared, preserving the no-overwrite guarantee), and on a successful link
but failed remove, return nil so the lock is treated as restored:
func RestoreLockFile(reclaimed, path string) error {
if err := os.Link(reclaimed, path); err == nil {
return os.Remove(reclaimed)
} else if errors.Is(err, os.ErrExist) {
return err
}
return restoreByCopy(reclaimed, path) // O_CREATE|O_EXCL|O_WRONLY copy, then remove reclaimed
}
// restoreByCopy copies reclaimed -> path without overwriting an existing path.
func restoreByCopy(reclaimed, path string) error {
src, err := os.Open(reclaimed)
if err != nil {
return err
}
defer src.Close()
dst, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
if err != nil {
return err // EEXIST -> caller drops reclaimed (new holder won); other -> safe fallback
}
defer dst.Close()
if _, err := io.Copy(dst, src); err != nil {
dst.Close()
os.Remove(path)
return err
}
if err := dst.Close(); err != nil {
os.Remove(path)
return err
}
return os.Remove(reclaimed)
}(A copy resets mtime to now, which keeps the lock "fresh" — safe; just note it.) As
defense-in-depth, callers could also os.Stat(lockPath) after any RestoreLockFile
error before deciding to re-acquire.
M1 — Medium: PR description and commit messages are inaccurate about the Windows mechanism
- PR body and first commit: "On Windows, it uses os.Rename (which safely fails if
the target exists)." — false. The final code uses
windows.MoveFileEx(from, to, 0)(lockutil_windows.go:31), andos.Renameon Windows
actually overwrites (passesMOVEFILE_REPLACE_EXISTING) — the very bug this PR fixes
(correctly stated in the file's own comment atlockutil_windows.go:19-21). Doubly wrong;
confirms the prior reviewer's "stale Windows rationale" note. - PR body: "If restoring fails, the temporary file is deleted rather than leaked." —
overstated: cleanup happens only onos.ErrExist(cron/lock.go:111-116and
siblings); other errors leak (this is the H1 leak aspect). - Third commit subject is truncated in metadata ("…ndows retry loop").
lockutil_windows.go:18claims theos.ErrExistmapping is "verified against the real
Win32 call" — but there is no Windows-tagged test backing it (see M2).- Stated test plan
go test -race ./internal/cron/... ./internal/daemon/... ./internal/hooks/... ./internal/oauth/... ./internal/swarm/...omits./internal/lockutil/..., where the primaryTestRestoreLockFilelives.
M2 — Medium: test-coverage gaps leave H1 and the Windows path unverified
- The no-overwrite guarantee when a competing destination exists IS covered on POSIX
(internal/lockutil/lockutil_test.go:29-48), and the FRESH restore success path IS
covered byinternal/cron/lock_reclaim_test.go:29-39(writes a fresh lock, asserts it
is restored intact — note it does not pre-create a competing destination before
restore; that exact race is exercised only at thelockutillevel). - Windows
MoveFileExpath is entirely untested — no//go:build windowstest; the
load-bearingerrors.Is(err, os.ErrExist)coupling (ERROR_ALREADY_EXISTS) is asserted
only in a comment. Must be verified on a Windows runner / Wine cross-build. RemoveLockFile(including the Windows 15×5ms retry loop on errno 32/5) has ZERO
tests on any platform.- No test for the non-
os.ErrExistpreserve branch, no EXDEV-fallback test, no
-race/concurrency test for the reclaim→restore path. - Only
cronhas a reclaim/restore test;daemon,hooks,oauth,swarmhave none for
this logic. The earlier reviewer's requested "competing destination before restore"
regression should be added per-package (mirroring cron).
L1 — Low: RemoveLockFile contract diverges across platforms
POSIX returns fs.ErrNotExist on a missing file; Windows swallows it to nil
(lockutil_windows.go:41). daemon/lock.go:109 relies on the POSIX behavior
(!errors.Is(err, fs.ErrNotExist)). Today benign (other callers use _ =), but a shared
package should present one uniform contract — make both swallow ErrNotExist, or document
the divergence explicitly on both doc comments.
L2 — Low: orphan .stale.* leak on Windows retry exhaustion / generic non-os.ErrExist
The genuinely-stale RemoveLockFile(reclaimed) is _ =-ignored
(cron/lock.go:118, hooks/lock.go:110, oauth/lock.go:105, swarm/mailbox.go:382). On
Windows, if the 15-retry sharing-violation loop exhausts, reclaimed is left on disk
permanently (no code ever sweeps *.stale.*). Harmless to correctness but a directory
leak; at least propagate/log the error. (The POSIX partial-success orphan from H1's
link-ok/remove-fail case is the same class, Low.)
L3 — Low/Nit: predictable tokens & un-hardened lock dirs (defense-in-depth)
Tokens are pid+nanotime+seq (not crypto/rand) and lock dirs are created 0o700 but
MkdirAll leaves a pre-existing world/group-writable dir untouched
(cron/lock.go:38, hooks/lock.go:37, oauth/lock.go:30; daemon has no MkdirAll). Not
exploitable given the rename-first invariant, but swarm's ensureInboxDir tightens dirs
and does a symlink-aware confinement check — the other callers should match for
consistency. Also add a // Package lockutil doc comment (revive will flag its absence).
RestoreLockFile could fail with errors other than os.ErrExist (hardlink- incapable filesystems such as FAT or some FUSE/network mounts, ENOSPC, Windows sharing violations). Callers only clean up on os.ErrExist, so any other failure left the lock path missing with the live holder's lock stranded in the sidelined file; the next O_EXCL create then succeeded and two processes held the lock. RestoreLockFile now falls back to an O_EXCL copy in that case, which preserves the no-overwrite guarantee, and a successful restore with a failed sidelined-file cleanup reports nil instead of a failed restore. RemoveLockFile now has one cross-platform contract: removing an already missing file reports nil on POSIX too, matching Windows, and daemon release() drops its caller-side ErrNotExist filter. Tests: TestRestoreByCopy and TestRemoveLockFile run on all platforms; Windows-tagged tests pin the ERROR_ALREADY_EXISTS mapping to os.ErrExist and exercise the RemoveLockFile retry loop to success and to bounded exhaustion. The package doc comment documents the shared contract.
The stale-lock reclaim in acquireLock was inline, so its restore path (a holder rotated a fresh lock in the gap between the stale check and the rename) could not be regression-tested the way cron, daemon, hooks, and oauth test theirs. Extract it into reclaimStaleLock with the same shape as the other callers, behavior unchanged, and add the mirrored regression test.
Mirror the cron reclaim test in the two packages that lacked coverage for reclaimStaleLock: a genuinely stale lock is reclaimed, a fresh lock (reacquired in the gap) is restored intact rather than stolen, and a missing lock reports no reclaim. daemon already had equivalent tests in lock_test.go.
|
@jatmn Pushed fixes for this review: 2df309d, b59c370, 1f79311. H1 (fixed): Implemented the recommended fallback. M1 (fixed): Rewrote the PR body. It now says the Windows mechanism is M2 (fixed): Added L1 (fixed): POSIX L2 (partly addressed): After the H1 fix, the sidelined file can only be orphaned when a remove fails after the lock is already safely restored or reclaimed, for example Windows retry exhaustion. There is no logging facility at this layer to report it to, and a L3 (partly addressed): Added the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/lockutil/lockutil_windows.go (1)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named Windows error constants here.
windows.ERROR_SHARING_VIOLATIONandwindows.ERROR_ACCESS_DENIEDare already referenced in the Windows tests, and they make this retry path self-documenting.🤖 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/lockutil/lockutil_windows.go` around lines 60 - 64, Update the retry condition in the lock acquisition logic to compare errno against windows.ERROR_SHARING_VIOLATION and windows.ERROR_ACCESS_DENIED instead of numeric literals 32 and 5. Preserve the existing errors.As check, sleep duration, and retry behavior.
🤖 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/cron/lock.go`:
- Around line 111-118: Update the RestoreLockFile error handling in the lock
recovery flow to surface every non-os.ErrExist error instead of silently
ignoring it; retain the existing reclaimed-file removal only for ErrExist. Apply
the same correction to the corresponding lock recovery patterns in daemon,
hooks, and oauth, using their RestoreLockFile call sites and existing logging or
propagation conventions.
In `@internal/swarm/mailbox.go`:
- Around line 395-412: Update reclaimStaleLock and its counterparts in cron,
hooks, and oauth so every RestoreLockFile error removes the reclaimed .stale.*
file, not only errors.Is(err, os.ErrExist). Preserve the existing return
behavior while ensuring cleanup runs for both ErrExist and non-ErrExist
failures.
---
Nitpick comments:
In `@internal/lockutil/lockutil_windows.go`:
- Around line 60-64: Update the retry condition in the lock acquisition logic to
compare errno against windows.ERROR_SHARING_VIOLATION and
windows.ERROR_ACCESS_DENIED instead of numeric literals 32 and 5. Preserve the
existing errors.As check, sleep duration, and retry behavior.
🪄 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: 60a913e3-8eac-403c-a5d5-66ca7ef5700b
📒 Files selected for processing (13)
internal/cron/lock.gointernal/daemon/lock.gointernal/hooks/lock.gointernal/hooks/lock_reclaim_test.gointernal/lockutil/lockutil.gointernal/lockutil/lockutil_other.gointernal/lockutil/lockutil_test.gointernal/lockutil/lockutil_windows.gointernal/lockutil/lockutil_windows_test.gointernal/oauth/lock.gointernal/oauth/lock_reclaim_test.gointernal/swarm/mailbox.gointernal/swarm/mailbox_test.go
A non-os.ErrExist error from RestoreLockFile means both the no-replace restore and its O_EXCL copy fallback failed, so the lock path may be missing while the sidelined holder is live. Callers previously swallowed that error and their next O_EXCL create would succeed, breaking mutual exclusion. reclaimStaleLock in cron, daemon, hooks, oauth, and swarm now returns an error for that case and the acquire loops fail closed instead of re-acquiring. The sidelined .stale.* file is now removed on every restore failure, not only os.ErrExist: once the restore has failed it has no protocol function (release only consults the lock path), so keeping it only leaked files. Also replace the numeric errno literals in the Windows RemoveLockFile retry loop with windows.ERROR_SHARING_VIOLATION and windows.ERROR_ACCESS_DENIED.
|
Pushed 13b729d addressing the latest CodeRabbit review.
PR body updated to match the new caller semantics. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cron/lock_reclaim_test.go (1)
10-46: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo coverage for the new fail-closed error path.
Tests cover stale-reclaimed, fresh-restored, and missing-lock cases, but none exercise
reclaimStaleLockreturning a non-nil error (i.e.,RestoreLockFilefailing for a reason other thanos.ErrExist). That branch is the actual safety-critical addition in this PR (fail closed instead of re-acquiring), and it's currently only reachable/verifiable indirectly through lower-levellockutiltests, not through this package's integration path. Same gap applies tointernal/daemon/lock_test.go.🤖 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/cron/lock_reclaim_test.go` around lines 10 - 46, Extend TestReclaimStaleLock to force RestoreLockFile to return a non-os.ErrExist error and verify reclaimStaleLock returns that error without re-acquiring the lock. Add the equivalent fail-closed integration case to the relevant lock test in internal/daemon, preserving existing stale, fresh, and missing-lock coverage.internal/cron/lock.go (1)
109-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail-closed restore logic is correct; consider consolidating duplicated reclaim logic.
This addresses both prior review comments (Windows overwrite risk, swallowed non-
ErrExisterrors) correctly: the no-replace restore + copy fallback + fail-closed-on-error chain matches the documentedlockutilcontract.Separately:
hooks/lock.goandoauth/lock.gocarry byte-for-byte identicalreclaimStaleLockbodies (mtime-based stale check → rename-aside → restore-if-fresh → fail-closed). This triplication of critical mutual-exclusion logic means any future fix has to land in three places (as the past-review history for this exact function shows). Worth extracting a shared helper intointernal/lockutilthat takes the lock path, a reclaimed-suffix, and a "still live" predicate.♻️ Illustrative shared helper
// internal/lockutil/reclaim.go func ReclaimStaleLock(lockPath, suffix string, isLive func(reclaimedPath string) bool) (bool, error) { reclaimed := lockPath + ".stale." + suffix if err := os.Rename(lockPath, reclaimed); err != nil { return false, nil } if isLive(reclaimed) { if rerr := RestoreLockFile(reclaimed, lockPath); rerr != nil { _ = RemoveLockFile(reclaimed) if !errors.Is(rerr, os.ErrExist) { return false, rerr } } return false, nil } _ = RemoveLockFile(reclaimed) return true, nil }Each package would then supply its own
isLive(mtime check for cron/hooks/oauth, PID-aliveness for daemon).🤖 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/cron/lock.go` around lines 109 - 133, Extract the duplicated reclaimStaleLock logic from cron, hooks, and oauth into a shared internal lockutil helper that accepts the lock path, stale suffix/token, and an isLive predicate. Preserve the existing rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error behavior; update each caller to provide its package-specific liveness check, including PID-based checking where applicable.
🤖 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/cron/lock_reclaim_test.go`:
- Around line 10-46: Extend TestReclaimStaleLock to force RestoreLockFile to
return a non-os.ErrExist error and verify reclaimStaleLock returns that error
without re-acquiring the lock. Add the equivalent fail-closed integration case
to the relevant lock test in internal/daemon, preserving existing stale, fresh,
and missing-lock coverage.
In `@internal/cron/lock.go`:
- Around line 109-133: Extract the duplicated reclaimStaleLock logic from cron,
hooks, and oauth into a shared internal lockutil helper that accepts the lock
path, stale suffix/token, and an isLive predicate. Preserve the existing
rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error
behavior; update each caller to provide its package-specific liveness check,
including PID-based checking where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ada8a5d-a175-4181-bb2b-b590b2406965
📒 Files selected for processing (11)
internal/cron/lock.gointernal/cron/lock_reclaim_test.gointernal/daemon/lock.gointernal/daemon/lock_test.gointernal/hooks/lock.gointernal/hooks/lock_reclaim_test.gointernal/lockutil/lockutil_windows.gointernal/oauth/lock.gointernal/oauth/lock_reclaim_test.gointernal/swarm/mailbox.gointernal/swarm/mailbox_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/oauth/lock_reclaim_test.go
- internal/swarm/mailbox_test.go
- internal/swarm/mailbox.go
- internal/lockutil/lockutil_windows.go
- internal/hooks/lock_reclaim_test.go
Consolidate the duplicated reclaimStaleLock functions from cron, hooks, oauth, and swarm into lockutil.ReclaimStaleLock. Each caller now passes an mtime-based isLive predicate to the shared helper, which owns the rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error propagation. Daemon keeps its own reclaimStaleLock because it uses PID-based liveness (different predicate shape and no staleAfter duration). The per-package reclaim regression tests are removed since the shared helper is thoroughly tested in lockutil/reclaim_test.go (dead lock reclaimed, live lock restored, missing lock, fail-closed on restore error, sidelined file dropped when new holder wins).
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)
370-383: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate the retry on
cleared
ReclaimStaleLockreturnsfalse, nilwhen it loses the rename race or restores a live holder, so this unconditionalcontinueskips the sleep/backoff and can hot-spin under contention. Match the other lock callers by continuing only whenclearedis true.🤖 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 370 - 383, Capture the boolean result from lockutil.ReclaimStaleLock in the stale-lock branch of the mailbox acquisition loop, and only execute continue when cleared is true. When the call returns false with no error, fall through to the existing wait/sleep backoff path; preserve the current fail-closed return for errors.
🤖 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/lockutil/reclaim.go`:
- Around line 29-31: Update the os.Rename error handling in reclaim so only
os.ErrNotExist and Windows sharing-related os.ErrPermission errors return the
benign lost-race result; propagate all other errors to the caller instead of
returning false, nil.
---
Outside diff comments:
In `@internal/swarm/mailbox.go`:
- Around line 370-383: Capture the boolean result from lockutil.ReclaimStaleLock
in the stale-lock branch of the mailbox acquisition loop, and only execute
continue when cleared is true. When the call returns false with no error, fall
through to the existing wait/sleep backoff path; preserve the current
fail-closed return for errors.
🪄 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: 3917b0f3-9db7-4581-a466-5df56ed9110c
📒 Files selected for processing (7)
internal/cron/lock.gointernal/cron/lock_reclaim_test.gointernal/hooks/lock.gointernal/lockutil/reclaim.gointernal/lockutil/reclaim_test.gointernal/oauth/lock.gointernal/swarm/mailbox.go
💤 Files with no reviewable changes (1)
- internal/cron/lock_reclaim_test.go
…taleLock A rename-aside failure that is not a lost race previously mapped to the benign lost-race result, so acquirers spun until their deadline and reported a timeout instead of the real failure. Keep only a missing source and the Windows sharing-violation/access-denied contention errnos benign (neither Windows errno maps to os.ErrExist, and ERROR_SHARING_VIOLATION does not map to os.ErrPermission either, so the classification compares raw errnos) and return everything else so callers fail closed. Also convert daemon's private reclaimStaleLock to the shared helper so it picks up the same contract, and reword the caller error messages from "restore reclaimed" to "reclaim stale" since the error can now come from the rename aside as well.
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] Do not expose a partially restored live lock
internal/lockutil/lockutil.go:27
The fallback createspathwithO_EXCLbefore copying the live lock's contents. On a filesystem where the link/move fallback is used, another daemon can observe the empty or truncated PID file, classify it as stale, reclaim it, and acquire a new lock while the original daemon is still running. If the copy or close then fails,ReclaimStaleLockalso removes the sidelined lock and leaves the original holder with no protocol-visible lock for later contenders. Stage a complete copy under a separate name and publish it with a no-replace primitive, and make failed restoration keep acquisition globally fail-closed until the live holder's lock is again represented at the lock path. -
[P1] Honor the mailbox lock timeout after a failed reclaim attempt
internal/swarm/mailbox.go:373
ReclaimStaleLockdeliberately returns(false, nil)when it loses the rename race or cannot reclaim a contended Windows file, but this caller discards the boolean and alwayscontinues. When the stale file remains present, the next iteration takes the same branch before the timeout and 2 ms backoff below, so mailbox operations can hot-spin pastLockTimeout. Captureclearedand only continue immediately when it is true; otherwise fall through to the timeout and sleep path.
…box reclaims restoreByCopy created the destination lock file empty before copying into it, so a crash mid-copy could leave a live holder's lock permanently unrepresented at the lock path. Stage the copy under a private name and publish it with the platform's no-replace primitive instead. swarm's mailbox lock discarded the reclaimed boolean and always retried immediately, so a lost reclaim race (or Windows sharing-violation contention) could hot-spin past the lock timeout instead of falling through to the bounded wait like the other lock callers.
|
Pushed 01bb58a for both P1s from the 5:15pm review. Mailbox lock timeout: Partial restore exposure: Added a staged-copy cleanup assertion to the existing restoreByCopy tests to cover the new intermediate file on both the success and losing-the-race paths. |
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] Keep a live lock protected while it is being restored
internal/lockutil/reclaim.go:39
The no-overwrite restore still leaveslockPathabsent between the rename-aside and the restore. A reclaimer can observe an old lock, a new holder can acquire it before the reclaimer's rename, and the reclaimer then moves that new live lock toreclaimed. BeforeRestoreLockFilepublishes it back, another process can successfullyO_EXCL-createlockPath; the restore returnsos.ErrExist, and this branch deletesreclaimedand returns normally. The original holder and that third process then both continue through their critical sections. The POSIX copy fallback makes the same window longer and cannot publish on the hard-link-incapable filesystems it claims to support because it callsos.Linkagain. Do not treat a competing canonical path as a safe winner while the displaced live holder is still running; use a recovery protocol that prevents acquisitions for the entire restore transition, and add a regression that interleaves a third acquisition before restore.
…g rename Restoring a lock that turned out live went through the no-replace path (plus its copy fallback on some failures) first, which is slow enough that a fourth process's unrelated O_EXCL create could win lockPath while it was absent for the whole call. RestoreLockFile's no-replace contract would then report os.ErrExist and the caller silently dropped the sidelined lock, orphaning the still-running original holder. restoreLiveLock now tries a single replacing rename straight back first, falling back to the no-replace path only if that fails. It shrinks the window from a whole restore call down to roughly one syscall; it does not close it, since the fast path can silently overwrite a competing lock rather than detect it. A fully race-free reclaim would need an OS-level advisory lock checked non-destructively instead of by moving the file, which is a larger change than this.
|
Confirmed, this is real: when the moved-aside lock turns out live, RestoreLockFile's no-replace-plus-copy-fallback path can take long enough that a fourth process's O_EXCL create wins lockPath while it's absent, and the current code treats that ErrExist as a benign lost race, silently dropping the original live holder's lock. Pushed d8f2943. restoreLiveLock now tries a single replacing rename straight back first, before falling back to the no-replace path. That shrinks the window from a whole restore call (potentially including the copy fallback) down to roughly one syscall. I want to be direct about what this does and doesn't do: it does not make the race impossible, it makes it much less likely to be hit. The fast path can still silently overwrite a competing lock instead of detecting it, so a fourth process that legitimately wins the window can still run its critical section concurrently with the original holder. Closing this properly would need an OS-level advisory lock (flock/LockFileEx) held for a holder's whole critical section and checked non-destructively instead of by moving the file, across all five callers of this protocol. That's a real design change, not a fixup commit, so I've scoped this one to the pragmatic tightening rather than that redesign. Happy to open a follow-up for the advisory-lock approach if you'd rather have it fully closed. Added a test pinning the new fast-path overwrite behavior (TestRestoreLiveLockFastPathOverwritesCompetingFile). No existing tests needed changes: the seam-based tests replace restoreLockFile wholesale, so they're unaffected by what restoreLiveLock does internally. |
4 participants
Summary
Fixes a High-severity concurrency/safety issue in lock file reclamation.
When reclaiming a suspected stale lock, the lock file is renamed aside, checked, and restored if it turns out fresh (a holder reacquired it in the gap). The restore step must never overwrite a competing lock created at the original path in the meantime.
os.Renamesilently overwrites the target if it already exists, so the old restore could clobber a new holder's lock and violate mutual exclusion.os.Renamealso overwrites: it passesMOVEFILE_REPLACE_EXISTINGto match POSIX rename semantics cross-platform. The same bug, plus the sidelined.stale.*file could be leaked.This PR introduces
internal/lockutilwith a no-overwrite restore:RestoreLockFileusesos.Link, which fails withos.ErrExistif the target lock file has been recreated.MoveFileExwith no flags viagolang.org/x/sys/windows, which fails withERROR_ALREADY_EXISTS(satisfieserrors.Is(err, os.ErrExist)) instead of overwriting.RestoreLockFilefalls back to anO_EXCLcopy, so the lock path is never left missing while the live holder's lock is stranded in the sidelined file.RemoveLockFilehas one cross-platform contract: removing a missing file reports nil, and Windows retries transient sharing violations.Changes
internal/cron/lock.go,internal/daemon/lock.go,internal/hooks/lock.go,internal/oauth/lock.go, andinternal/swarm/mailbox.goto uselockutil.reclaimStaleLockhelper mirroring cron, hooks, and oauth, so its restore path is testable.lockutilrestore, copy-fallback, and remove behavior on all platforms; Windows-tagged tests pinning theERROR_ALREADY_EXISTSmapping toos.ErrExistand exercising theRemoveLockFileretry loop; per-package stale-reclaim tests for cron, daemon, hooks, oauth, and swarm.Test plan
go test ./internal/lockutil/... ./internal/cron/... ./internal/daemon/... ./internal/hooks/... ./internal/oauth/... ./internal/swarm/...lockutiltests); Smoke (ubuntu, macos, windows) in CI covers the rest.Summary by CodeRabbit