Repository navigation
lock: a guard mid-release reads as EPERM on Windows — treat it as contention - #197
Conversation
…tention One of twelve concurrent `truecopy add`s exited 1 on the windows-latest CI leg with "EPERM: operation not permitted, open …/race.lock.guard" while the other eleven pinned fine and the lock on disk was correct (run 35579991225). acquire() retried only on EEXIST and threw on anything else. On Windows, open(wx) on a guard that another process is unlinking at that instant fails with EPERM (delete-pending), not EEXIST. Measured here: six processes hammering one guard saw EPERM on ~1% of attempts. The guard is not lost, it is mid-release, so EPERM, EACCES and EBUSY from the guard open now wait and retry exactly like EEXIST. Any other code still throws. Test injects each code through the fs module the lock uses and asserts the update lands and the guard is released; an unrelated ENOSPC still fails loudly; and six real processes hammering one guard through updateLock land all 240 updates. Fails 3/5 on the old code.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: request changes — a permanent access-denied error can now loop indefinitely instead of failing.
Blocking — correctness — src/lock.mjs:105,119
const GUARD_CONTENTION = new Set(['EEXIST', 'EPERM', 'EACCES', 'EBUSY']);
if (!GUARD_CONTENTION.has(e.code)) throw e;
EACCES (and some EPERM cases) also represents a permanent ACL/parent-directory denial, not a delete-pending guard. For example, if the lock directory is not accessible, openSync(g, 'wx') throws EACCES; guardAge(g) then cannot stat the guard and returns null. The existing if (age === null) continue path is reached before the deadline check, so this newly classified error spins forever rather than reporting the permission failure (or even timing out). That can hang every CLI operation attempting to update a lock on an inaccessible path.
// Treat only a raced-away guard as retryable. Preserve stat/access failures.
try {
const age = Date.now() - fs.statSync(g).mtimeMs;
// apply stale/deadline handling to `age`
} catch (statError) {
if (statError.code === 'ENOENT') continue;
throw statError;
}
The focused injected-code coverage and the green cross-platform CI matrix are good evidence for the intended Windows contention case, but add a persistent permission-denial regression case when narrowing the retry condition.
…spin forever The gating review on #197 is right: widening GUARD_CONTENTION to EPERM/ EACCES/EBUSY made a PERMANENT denial retryable. An inaccessible lock directory answers open(wx) with EACCES, guardAge() then could not stat the guard and returned null, and 'if (age === null) continue' short- circuited BEFORE the deadline check — so every CLI operation updating a lock on an unreachable path looped without ever pinning, timing out, or reporting the permission failure. guardAge now distinguishes ENOENT (the guard genuinely raced away) from any other stat failure, which it returns as { error } instead of swallowing. acquire() keeps retrying a stat error for the normal wait window — Windows can fail the stat transiently on a delete-pending guard, which is the case this PR exists to fix — and rethrows the original access error once the deadline passes, so the operator sees EACCES rather than a misleading 'another process is holding it'. The age === null path is now deadline-bounded too; it previously had the same unbounded-spin shape if a guard kept vanishing between open and stat.
…ports Three new cases, all of which hang forever against ad0b8a1 (confirmed by running them there — the suite never terminates, which is the defect the review described): - a PERMANENT EACCES/EPERM on the guard path, where open(wx) AND stat both fail the way an unreadable lock directory behaves. Asserts the ORIGINAL access error surfaces, not a 'timed out waiting for another truecopy process' message that would send the operator hunting a holder that does not exist, and that it terminates. - a guard that loses every race (EEXIST on open, ENOENT on stat), which exercises the age === null branch that also lacked a deadline check. A timeout is correct here; it just has to arrive. - a stat that fails transiently and then recovers, pinning the Windows delete-pending case so the new bound cannot be tightened into a regression of the behaviour this PR adds. 9/9 green on the fix.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: request changes — the new EACCES/EPERM contention handling can still spin forever on a permanent access denial.
-
Blocking —
src/lock.mjs:155} else if (age > staleMs) { try { fs.unlinkSync(g); } catch {} continue; }A process can receive
EACCESfromopenSync(g, 'wx'), yet still be allowed tostatan existing stale guard (directory ACLs commonly permit metadata reads while denying deletion). Once that guard exceedsstaleMs, a failedunlinkSyncis swallowed and the unconditionalcontinuebypasses thetimedOutcheck at line 156 andsleepSyncat line 157. The result is a tight, unbounded loop rather than the promised original access error afterwaitMs. The added permanent-denial tests only model stat failing too, so they do not cover this reachable permission layout.} else if (age > staleMs) { try { fs.unlinkSync(g); continue; } catch (unlinkError) { if (timedOut) throw unlinkError; } } else if (timedOut) throw timedOutError();Also add a regression test where
openSync(..., 'wx')andunlinkSyncreturnEACCESwhilestatSyncreturns an old mtime, asserting termination and the access error.
What's good: the rework correctly distinguishes ENOENT from other stat failures, bounds the vanishing-guard path, and adds focused injected coverage. All reported CI checks, including both Windows Node versions, are green.
…forever A directory ACL can permit stat while denying unlink. openSync(wx) then returns EACCES, statSync succeeds with an old mtime, and the guard reads as stale — but the unlinkSync that would reap it also fails. The swallowed catch plus the unconditional continue skipped both the timedOut check and sleepSync(20), so that layout spun at full speed forever instead of reporting the access error after waitMs. Reap-then-continue only on a successful unlink; on failure fall through to the deadline, throwing the unlink error itself once the window closes. Refs FIX-532e9d1c
…assed The existing permanent-denial tests model stat failing too, so they never reach the stale branch. This one injects the layout a directory ACL actually produces: open(wx) EACCES/EPERM, stat SUCCEEDS with a 5-minute-old mtime (past GUARD_STALE_MS), unlink EACCES/EPERM. Asserts termination and that the access error surfaces rather than a phantom-holder timeout. Both cases hang forever against 6df966e — node --test cancels them at the event loop, which is the unbounded spin the review described. Refs FIX-532e9d1c
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: approve — no blocking issues found in the live-head rework.
The rework at 27ca620dc45f2e23e241eea704424474f2ba506e closes the prior stale-guard unlink failure: src/lock.mjs:155-166 now retains the retry window for a transient failure but throws the actual unlinkSync access error once the deadline expires. test/lock-guard-windows.test.mjs:129-175 specifically models the reachable layout where guard metadata is readable but open(wx) and unlink return EACCES/EPERM, and asserts termination with that access error.
What's good: the change is narrowly scoped to the reported unbounded-loop path and preserves the Windows delete-pending retry behavior. The complete CI matrix is green, including Windows on Node 20 and 22; I did not run the local suite.
Bumps the version to 0.10.4. On merge, auto-release.yml tags v0.10.4, cuts the GitHub release from the new CHANGELOG section and dispatches publish.yml to npm. The Windows EPERM lock-guard fix (#197) is the only change to shipped code since 0.10.3, and it reaches users only in a release. The pinned version in the docs and issue templates moves with it; docker/ stays on 0.10.3 until 0.10.4 is on npm.
Fixes the windows-latest / Node 22 CI failure in run 35579991225:
12 concurrent pins all survivefailed because onetruecopy addexited 1 withEPERM: operation not permitted, open …/race.lock.guard. The other eleven pinned and the lock was correct. (The GitHub Manager card called this a test-runner exit-code mismatch; it is a real, single failing test — 258 pass, 1 fail.)Cause.
acquire()treated onlyEEXISTas "someone else holds the guard". On Windows,open(wx)on a guard another process is unlinking at that instant returnsEPERM(delete-pending). Measured on Windows 11 / Node 22: six processes hammering one guard sawEPERMon ~1% of attempts. With twelve writers that is a near-certain hit per CI run.Fix.
EPERM,EACCES,EBUSYfrom the guard open are contention: wait and retry likeEEXIST, under the same deadline and stale-guard rules. Anything else still throws.Tests.
test/lock-guard-windows.test.mjs: injects each code through thefsmodule the lock uses and asserts the update lands and the guard is released; an unrelatedENOSPCstill fails loudly; six real processes hammer one guard throughupdateLockand all 240 updates survive. Fails 3/5 against the old code, 5/5 with the fix. Full suite 260 pass, 0 fail (4 skipped as before).