fix(npm): treat Windows cache-lock contention as retryable - #134
waterbro-8 wants to merge 3 commits into
Conversation
The per-asset cache lock is taken with `mkdir`, which reports a contended directory as EPERM or EACCES on Windows instead of EEXIST. acquireAssetLock recognised only EEXIST as contention, so a concurrent first run aborted with a hard permission error instead of waiting for the lock holder. Classify EPERM/EACCES as contention on win32 only, resolve osPlatform in the lock helpers the way install() already does so the win32 branch is provable off Windows, and leave a non-contention permission error failing closed. Fixes #133
waterbro-8
added
type:bug
The first version of this test chmodded the cache directory to 0o500 and expected a permission error. NTFS ignores the POSIX mode, so mkdir succeeded, acquireAssetLock took the lock, and the Windows job rejected the test rather than the code. The fix itself passed unchanged on real Windows. Use ENOENT from a lock mkdir under a missing parent, which both POSIX and Windows raise, and bound the elapsed time so a future change that swallows a non-contention error into the retry loop fails fast instead of hanging.
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
P1 — the Windows permission classification can enter an unbounded busy loop and hides real permission failures.
Audited head: cb9c5cfe75c29df6913a32d34432d2d1fa005451.
At npm/install.js:318-325, every Windows mkdirSync(lockPath) EPERM/EACCES is classified as contention. If inspection then returns ENOENT, reclaimStaleLock() returns true and the caller continues before both the deadline check and the delay. A persistent real permission failure therefore loops indefinitely. I reproduced this on the exact head by injecting persistent mkdir EPERM plus lstat ENOENT: even with waitTimeoutMs: 5, the process had not returned after one second. If lstat itself returns EPERM/EACCES, line 258 also treats that inspection failure as contention, replacing the original permission error with a generic lock timeout.
The current AC-003 test does not cover this path: it injects ENOENT with osPlatform: "linux", so it only proves the already-fatal non-Windows classification.
Please preserve the original Windows permission error until lock existence is positively established, never suppress an EPERM/EACCES from lstat, and ensure every retry path is delayed and deadline-bounded. Add deterministic child-process cases for persistent mkdir EPERM + lstat ENOENT, lstat EPERM/EACCES, and transient Windows contention followed by a real lock/success; assert the first two fail promptly without a busy loop.
The release-train statement also needs correction: v0.1.1 was already immutably tagged and released from cc727db0bc72655f299166de1f60756f5c686cc7 before this PR was opened. This fix can only ship in a later version; the existing tag must not move.
|
Superseded by #137 at exact head Reason: this candidate retains public non-noreply commit metadata and its current head contains a P1 retry path in which lock-disappearance races can bypass the deadline and backoff. #137 is a clean normal-history successor, includes deterministic regression coverage for both races, and has completed its exact-head required CI including node24-windows. It is awaiting an independent @Bindy-lbb CODEOWNER review. The source branch is intentionally left untouched for audit; no force-push or history rewrite is performed. Tracking Issue #133 remains open and is not closed by this PR closure. |
## Tracking record Refs #133 - Consumed revision: [#133 R1](#133 (comment)) - This PR does not close the Issue. Product acceptance remains a separate step after merge evidence. ## Why A concurrent cold-cache install on Windows can surface mkdir contention as EPERM or EACCES rather than EEXIST. The installer previously treated only EEXIST as retryable. The original remediation candidate [#134](#134) also left a P1 retry path: an EEXIST to ENOENT or stale-rename race could retry synchronously before its deadline or abort-aware delay. This is a clean successor to #134. That older public branch is left intact for audit; no history is rewritten or force-pushed. ## Scope - Classify EEXIST as lock contention on every platform and EPERM or EACCES as potential contention only on win32. - Prove a lock can be inspected before retrying a Windows permission-shaped contention error; unproven permission failures still fail closed. - Route every failed acquisition, including stale recovery and lock-disappearance races, through the existing deadline and abort-aware poll. - Add deterministic regression coverage and the Unreleased changelog entry. No lock primitive, per-asset ownership model, stale or orphan threshold, dependency, public API, tag, npm publication, or release behavior changes. ## Requirement trace | Requirement / acceptance | Implementation | Evidence | | --- | --- | --- | | REQ-001; AC-001, AC-002 | npm/install.js error classifier and acquireAssetLock | Host-independent classifier table and real-lock simulated Windows EPERM acquisition test. | | REQ-002; AC-003 | npm/install.js lock inspection path | Persistent no-lock EPERM and inspection EACCES tests reject promptly. | | REQ-003; AC-004 | npm/install.js platform guard | Linux and Darwin keep EPERM or EACCES fatal; exact-head Windows CI remains required. | | REQ-004; AC-001, AC-004 | injectable osPlatform plus npm/install.test.js | win32 branch is exercised off Windows; hosted node24-windows job is required before review. | | AC-005 | CHANGELOG.md | Unreleased user-visible fix note. | ## Failure evidence and regression proof At base cc727db, a deterministic pre-require fs injection of EEXIST followed by lstat ENOENT produced two immediate lock mkdir attempts and an EPERM sentinel: the retry bypassed its zero wait deadline. This candidate makes the same path return the expected timeout after one attempt. A separate stale rename ENOENT race has the same bounded behavior. ## Validation Local macOS, Node 24: | Command | Observed result | | --- | --- | | PATH=/Users/huyz/.nvm/versions/node/v24.14.1/bin:$PATH npm test in npm/ | 41 passed, 0 failed, 1 Windows-only shim skipped as expected on macOS | | node --test npm/install.test.js | 30 passed, 0 failed | | node --check install.js; node --check mem-mcp; node --check platforms.js | passed | | npm pack --dry-run --ignore-scripts | passed; six expected package files only | | git diff --check | passed | | focused added-diff sensitive-pattern scan | no matches | Independent preflight replayed the focused and full Node 24 suite, plus the full Node 18 suite: 41 passed, 0 failed, 1 expected Windows-only skip in each full run. It found no P0 or P1 and recorded PREFLIGHT PASS for this code candidate. ## Review and delivery gates - Exact head required CI, especially npm wrapper compatibility (node24-windows), must be green. - @Bindy-lbb must provide an independent current-head CODEOWNER approval. The author and last pusher will not self-approve. - Normal merge authorization, merge-SHA push/main verification, verification ledger, and product acceptance remain required. ## Security, compatibility, and release boundary The retry set is deliberately narrow. On Windows, EPERM or EACCES is retried only after a lock can be inspected; otherwise the original error is surfaced. Non-Windows behavior remains fail-closed. No credentials, dependencies, or public-history changes are introduced. v0.1.1 was already published from main. This candidate may only enter a later, separately authorized release; it does not authorize a tag move, npm publish, GitHub Release, merge, or Issue closure. Co-authored-by: waterbro-8 <318569545+waterbro-8@users.noreply.github.com>
2 participants
Tracking record
Fixes #133
Summary
This candidate fixes Windows cache-lock contention for the npm installer without
turning real permission failures into retry loops.
mkdircontention is still recognized asEEXISTon every platform and asEPERM/EACCESonly on Windows. The lock path is then inspected before anyretry is allowed: a retry is safe only when an
EEXISTlock disappeared beforeinspection, or when a real lock directory can be inspected and is still active.
A Windows permission error with no provable lock now fails immediately; an
inspection permission error also propagates rather than being converted to a
generic timeout.
Owner repair on this head
The prior exact-head review found that a speculative Windows
EPERMpluslstat ENOENTcouldcontinuebefore both the delay and deadline checks. Thishead preserves the original
mkdirerror until the lock is proven to exist andadds deterministic child-process regression cases for:
EPERMwhile another process owns a real lock, followed bysuccessful acquisition;
EPERMwith no lock, which fails promptly instead ofbusy-looping; and
EPERMfollowed by anEACCESlock inspection, which fails promptlywith the inspection error.
Requirement trace
npm/install.jslock acquisitionnpm/install.jserror handlingEPERMand inspectionEACCESfail promptly.npm/install.test.jsCHANGELOG.md[Unreleased]fix note remains in the candidate.Validation ledger
Local on Linux x86_64 with Node 24:
node --test install.test.jsnpm testinnpm/.cmdshim skipped on POSIXgit diff --checkHosted exact-head checks for
30e70a712aca646e8f84d7e36879bba148151363:npm wrapper compatibility (node24-windows): https://github.com/fullstack-ai-infra/mem/pull/134/checksScope and non-goals
The lock primitive, one-lock-per-asset model, stale/orphan reclaim thresholds,
and ordinary retry deadline are unchanged. No dependency, publication, runtime
API, or lock semantic is added. The repair does not suppress errors to make CI
green.
Release ledger correction
v0.1.1was already immutably tagged and released fromcc727db0bc72655f299166de1f60756f5c686cc7before this PR opened. This changecan ship only in a later release; no published tag is moved or rewritten.
Review handoff
PeterGuy326authored the owner repair on this head and therefore will notself-approve it.
@Bindy-lbbis requested for an independent CODEOWNER reviewafter this exact-head CI result.