fix(lock): retry Windows lock files denied mid-teardown - #87
Conversation
Windows denies access to a lock file while a just-unlinked directory entry is still being torn down, so a contended acquire observed EPERM on a name that was already gone. At the failure lstat reports ENOENT and a zero-delay retry opens the file, which makes this contention rather than a permission failure. acquire() only treated EEXIST as contention, so the transient EPERM escaped from two touch points: the exclusive create, and readSidecarLockSnapshot, which maps only ENOENT to a vanished lock. The repository already applies this Windows equivalence in replace-file.ts, json.ts, and move-path.ts; sidecar-lock.ts was the outlier. Retries are bounded so a genuine denial still surfaces as EPERM instead of degrading into a lock timeout. Locally this failed 8 times in 85 runs before the change and 0 times in 110 runs after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGksu4AaxVTcTHZntKTyAs
Both new tests held the raw mkdtemp() result and matched the injected path by string equality. Windows runners return an 8.3 short name from os.tmpdir() while acquire() realpaths the target, so the injection never matched the path the lock code used. The create case then saw zero injections and the read case fell through to the real snapshot, which read as a live lock and timed out. Follow the suite convention and realpath the root. Verified locally by pointing the same scenario at a directory junction, which reproduces the CI mismatch: the raw path records zero injections and the resolved path records one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGksu4AaxVTcTHZntKTyAs
clawsweeper
Bot
added
proof: sufficient
|
CI status on this branch: 22 pass, 1 fail, and the one red leg is not from this diff.
That test is not reachable from this change — the diff touches Worth noting what did change: the two failures this PR is about — The remaining ACL failure looks like a different defect (a 15s timeout rather than an The first run of this branch did have two genuine failures of my own, both in the new tests — they held the raw |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 5:09 PM ET / 21:09 UTC. ClawSweeper reviewWhat this changesThis PR retries a transient Windows Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain This PR is not ready to merge: its narrowed JavaScript fallback handling preserves fail-closed behavior, but the binding-enabled Windows exclusive-create path used by packaged installs cannot satisfy the new lock-file-path predicate and is neither covered by the regressions nor demonstrated by runtime proof. The existing P1 remains unresolved. Priority: P1 Review scores
Verification
How this fits togetherThe sidecar lock manager coordinates concurrent writes by creating a flowchart TD
A[Caller requests a file lock] --> B[Sidecar lock manager]
B --> C[Exclusive sidecar lock creation]
C --> D{Created, contended, or denied?}
D -->|Created| E[Return owned lock]
D -->|Contended| F[Read holder snapshot]
F --> G[Retry policy and deadline]
G --> B
D -->|Unexpected denial| H[Propagate fail-closed error]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep unrelated native errors fail-closed, but make the native exclusive-create operation distinguishable at its boundary and add binding-enabled Windows evidence that the exact teardown denial retries without reclassifying parent-directory or caller errors. Do we have a high-confidence way to reproduce the issue? Yes for the gap in this PR: source proves that binding-enabled exclusive creation reaches Is this the best way to solve the issue? No. The scoped fallback classifier is appropriately fail-closed, but it does not solve the production native exclusive-create route; the native operation needs a narrow provenance signal plus a focused Windows regression or live proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
The classification sat in the catch that wraps the whole acquisition body, which also covers options.payload(), lockRoot.create/open, the payload write, and the stat. A Windows EPERM raised by any of those was reclassified as contention, so the loop reran the caller's payload callback and, once the retry budget was spent, replaced the caller's error with file_lock_timeout. That is the opposite of the fail-closed contract. Classify at the two operations the evidence actually covers: the direct exclusive create and the holder snapshot read. Everything else propagates unchanged. The loop now only owns the retry budget. Adds a regression asserting that an EPERM thrown by the caller's payload reaches the caller as EPERM and that the callback runs exactly once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGksu4AaxVTcTHZntKTyAs
|
The P1 is correct. The classification sat in the catch wrapping the whole acquisition body, which also covers Classification now happens at the two operations the evidence does cover:
The loop only owns the budget now ( Added the regression you asked for: an Re-verified after the narrowing rather than assuming it held: On the P1 merge risk about the hosted Windows check: that failure was |
clawsweeper
Bot
added
P1
createNativeExclusiveFile() opens dirname(lockPath) before its native exclusive open, so a denial raised by that setup step was still classified as the teardown window. The outer loop then retried it and reran options.payload(), which is the same fail-closed violation as before, one level further in. Require the EPERM to name the lock file itself. The observed failures carry path set to the lock path, from fs.open(lockPath, "wx") and from the snapshot read, so the evidence maps exactly onto that condition. A parent directory denial names the directory and now propagates untouched, as does any native helper error that identifies no path. Adds a regression asserting that a denial naming the lock parent rejects with the original EPERM and runs the payload callback exactly once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGksu4AaxVTcTHZntKTyAs
|
Also correct.
The classification now requires the error to name the lock file: export function isTransientLockFileDenial(error: unknown, lockPath: string): boolean {
const denial = error as NodeJS.ErrnoException | null;
return process.platform === "win32" && denial?.code === "EPERM" && denial.path === lockPath;
}That maps onto the evidence rather than onto a code path. Every observed failure carries On covering the native-enabled case: a native New regression: a denial naming the lock parent rejects with the original Re-ran the race after the change rather than assuming the discriminator still matched: |
withinDenialBudget() only bounded the denial count, not the caller's retry and timeout limits. A lock-file EPERM still went through waitForRetry(), which throws file_lock_timeout under retries: 0 or an elapsed deadline, so the caller lost the permission diagnosis on exactly the settings that ask to fail fast. The PR claimed bounded denials preserved EPERM; they did not. Hand the original denial back when waiting cannot schedule another attempt. Any other waitForRetry() failure still propagates as itself, and both the create and snapshot-read branches route through the same helper. Adds a regression pinning EPERM with retries: 0 for the create branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGksu4AaxVTcTHZntKTyAs
|
Correct again, and this one contradicted a claim in my own PR body.
The wait now hands the original denial back when no further attempt can be scheduled: const retryOrRethrowDenial = async (denial: unknown): Promise<void> => {
try {
await waitForRetry();
} catch (waitError) {
if ((waitError as NodeJS.ErrnoException).code === "file_lock_timeout") throw denial;
throw waitError;
}
};Both the create and snapshot-read branches route through it, and any other New regression: a lock-file Re-ran the race again rather than assuming: 0 failures in 60 runs on Windows 11 / Node 22 across One thing worth your judgement rather than mine: the line-budget entries have crept across these rounds — |
|
@clawsweeper re-review The open P1 ( |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
status: 📣 needs proof
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Landed as b1132f9 via #92 — thank you, this was a good catch and the evidence was excellent. Your diagnosis was right and your five commits are preserved verbatim in the merged history, with the changelog crediting you. I closed the one remaining gap myself rather than sending it back: The Rust mapper now reports I also took you up on the module split instead of raising The 8-in-85 versus 0-in-110 measurement was the thing that made this reviewable — a single green run would have proved nothing for a race this shape. Thanks for doing it properly. |
2 participants
What Problem This Solves
Fixes an issue where consumers taking a contended file lock on Windows would see
acquireFileLock()reject withEPERMwhen the previous holder released the lock at the same moment. The affected surface is the sidecar file lock (acquireFileLock,withFileLock,createFileLockManager).Windows denies access to a lock file while a just-unlinked directory entry is still being torn down. In that window the name is already gone but a fresh exclusive create is refused with
ERROR_ACCESS_DENIED, which libuv reports asEPERM(errno-4048). The next attempt succeeds, so this is contention, not a permission failure.acquire()treated onlyEEXISTas contention, so the transientEPERMescaped from two touch points:fs.open(lockPath, "wx")readSidecarLockSnapshot(), which maps onlyENOENTto a vanished lockInstrumenting the failure showed the state directly — at the moment of the
EPERM, the lock file is already gone and a zero-delay retry opens it:Why This Change Was Made
Both touch points now classify a Windows
EPERMon the lock file as contention and re-enter the existing retry loop. The retry budget is bounded (8 denials), so a genuine permission denial still surfaces asEPERMinstead of degrading into afile_lock_timeoutand losing the diagnosis.The classification is gated to
win32, matching the equivalence this repository already applies elsewhere —isPermissionRenameErrorinsrc/replace-file.ts,renameJsonFileWithFallbackinsrc/json.ts, andwindows-rename-deniedinsrc/move-path.ts.src/sidecar-lock.tswas the outlier. No public API, error shape, or POSIX behaviour changes.src/sidecar-lock.tssat at 498 lines against the default 500-line budget, so the predicate lives insrc/sidecar-lock-policy.tsand the file gets aLINE_BUDGETSentry. Happy to split the module instead if you would rather not raise the budget.User Impact
Concurrent lock acquisition on Windows no longer fails intermittently when one holder releases while another is waiting. Consumers on POSIX are unaffected. Nothing is added to or removed from the public API.
Evidence
Two regression tests inject the transient denial at each touch point and assert the acquire still completes. Both fail on
mainand pass with this change:Repeated runs of the real suites on Windows 11 (Node 22),
test/file-lock-reentrancy.test.tsplustest/new-primitives.test.ts:The rate before the fix moves with machine load, so a single green run does not demonstrate anything here — the loop does.
Full local verification:
vitest run489 passed / 183 skipped,tsc --noEmitclean,scripts/check-file-size.mjsandscripts/check-fs-boundary-primitives.mjsboth exit 0.Note on the flake reports
This is the defect behind the
file-lock-reentrancyandnew-primitives > file locksfailures that have been rotating across Windows legs.src/sidecar-lock.tslast changed in #65, so it predates the #79 dependency refresh — that bump only shifted timing enough to expose an existing race.It does not explain
move-path-regression.test.ts > publishes a fresh inode when hardlink rejection is enabled, which is a different symptom and is still open.CHANGELOG.mdupdated when release-relevant