fix(lock): cover native Windows teardown denials - #92
Merged
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
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
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
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
Co-authored-by: Yigtwxx <yigiterdogan023@gmail.com>
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 5:23 PM ET / 21:23 UTC. ClawSweeper reviewWhat this changesThis PR makes native Windows exclusive lock creation report Merge readinessKeep this PR open: current Priority: P2 Review scores
Verification
How this fits together
flowchart LR
A[Caller requests file lock] --> B[Sidecar lock acquisition]
B --> C[Native or JavaScript exclusive create]
C --> D{Windows EPERM names lock file?}
D -->|Yes| E[Bounded contention retry]
D -->|No| F[Propagate original error]
E --> G[Held lock handle]
G --> H[Protected file operation]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the native-path retry with the documented Do we have a high-confidence way to reproduce the issue? Yes, at source level: the PR’s Windows-only native integration test injects the binding’s bare Is this the best way to solve the issue? Yes. Attaching provenance at the native exclusive-open boundary and requiring an exact lock-file path is narrower and safer than broadening the retry loop to all Windows permission failures. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68. LabelsLabel changes:
Label justifications:
EvidenceWhat 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
|
This was referenced Aug 2, 2026
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
2 participants
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.
Builds directly on @Yigtwxx's #87 — their five commits are preserved verbatim in this branch's history, and the changelog credits them for the fix. This PR adds the native-path coverage that #87 was missing, so please land this one and close #87 as superseded rather than asking for a revision.
What #87 got right
On Windows, a lock file denied while its directory entry is still being torn down surfaces as
EPERM(errno-4048) rather thanEEXIST, soacquire()treated transient contention as a hard failure. @Yigtwxx classified that denial as contention at both touch points — the exclusive create andreadSidecarLockSnapshot()— and re-entered the existing bounded retry loop. Their evidence was 8 failures in 85 runs before, 0 in 110 after.The gap this closes
The fix only covered the JavaScript fallback.
createNativeExclusiveFile()runs first whenever the binding is loaded, and the Windows binding mappedERROR_ACCESS_DENIEDtoEACCESwith no target pathname attached — so theEPERM-plus-exact-path predicate could never match, and packaged Windows installs still hit the original bug. Every test in #87 forces native mode off, which is why this was invisible.Two changes fix it. The Rust mapper now reports
ERROR_ACCESS_DENIEDasEPERM, matching Node/libuv and the JavaScript fallback — until now the two paths reported different codes for the identical underlying condition, which was a latent inconsistency in its own right. And the TypeScript side attaches the target path only whenbinding.openBeneath()fails during exclusive creation; parent setup, payload, write, chmod/stat and unrelated native failures stay untagged, so the retry predicate stays exactly as narrow as it was.This cannot fail open. A bare access-denied with no path provenance is still unclassifiable and still propagates, persistent denials stay bounded to eight retries, and the ninth surfaces the original
EPERMrather than decaying intofile_lock_timeoutand losing the diagnosis.Compatibility
win_error()is the global Windows error mapper, so this changes access-denied reporting for every native Windows operation, not just locks. That has its own changelog entry with migration guidance. Both in-packageEACCESbranches —directory-durability.tsandsecure-temp-dir.ts— already acceptEPERMalongside it, so nothing internal changes behavior.Structure
@Yigtwxx flagged that
src/sidecar-lock.tssat at 498 lines against the 500-line budget and offered to split the module rather than raise the budget. Taking that offer: the acquisition state machine moved tosidecar-lock-acquire.ts, manager lifecycle stayed behind, and both files now sit comfortably inside the budget. The Windows denial tests split the same way. BothLINE_BUDGETSexceptions are removed rather than added to.Proof
Windows CI is the live proof surface, since this behavior is Windows-only and no Windows host was available locally: run 30766942217 — all 15 jobs green, including Node 22/24 Windows, the Windows native crate/build/integration jobs, and the Windows bundled-package smoke. The hosted Windows tests inject the binding's real bare-error shape and exercise the actual binding on the succeeding attempt, covering both transient recovery and persistent-denial preservation.
Repeated local runs of the real suites on macOS: 50 passed / 0 failed before and after.
pnpm check645 passed / 32 platform-skipped,pnpm test:security62 passed,git diff --checkclean, Codex autoreview clean.Not captured: an organic, uninstrumented occurrence of the kernel teardown race. The injected shape is the binding's real error shape, but the timing is deterministic rather than natural.