Skip to content

Retry a lock claim read that Windows refuses mid-unlink instead of failing the acquire - #2803

Merged
kriszyp merged 1 commit into
mainfrom
fix/lock-claim-read-windows-delete-pending
Sep 25, 2026
Merged

kriszyp merged 1 commit into
mainfrom
fix/lock-claim-read-windows-delete-pending

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Windows refuses to open a file whose unlink is in progress: the open fails with EPERM until the unlinking handle closes, then the name is gone. The component preparation lock's claim reader treated only ENOENT as a vanished claim, so a contender that scanned while a peer was removing its choosing claim failed the whole acquire. The five-writer race test that Publish a component's root-config entry as an effect of its activation (#2796) added hits this on the Windows Unit Test leg. It failed there on main at 30f8ff6 (run 36144493772) and on that PR's branch before merge (run 36083964824). Every path that takes this lock has the same exposure under contention: deploy_component, install_node_modules, restart, and root-config publication.

readOwner now retries an EPERM read on the file's existing 10/40/160 ms backoff until the claim either reads or is gone. A claim that is still unreadable after that fails the scan, as it did before: reading a live ticket as absent would let a second holder in.

Where main's Unit Test red stands. The red started at 8f411b3 and has three fingerprints, none caused by #2796's product change:

Fingerprint Leg Status
LMDB Segmentation fault right after databaseAliasIdentity.test.js Node.js v22 Fixed by Close a shared LMDB environment once instead of closing its dbis after another alias freed it (#2766), merged; Node.js v22/v24/v26 green at 30f8ff6
derivedIndexCoverage.test.js:184 "Missing expected rejection" any Linux leg; sixth sighting Fixed by the open draft Fix flaky derived-index coverage tests by holding the native barrier until the transaction monitor acts (#2758). Checked against 30f8ff6 with the race forced (native flush age patched to 5 ms): base test fails 3/3, #2758's test passes 5/5
rootConfigPublication.test.js:256 EPERM in readOwner Windows Node.js v24 This PR

For the human reviewer

  1. A claim that stays unreadable still fails the scan (unreadable-claim-fails-scan). After 210 ms of EPERM the caller gets a plain EPERM. Boot activation recovery records that as a durable .unsettled verdict (components/Application.ts:2347-2381); a lock timeout would only defer the component. The domain reviewer suggested treating a persistently unreadable claim as a live blocker instead, rebuilding its ticket number and token from the filename, so the wait ends in ComponentPreparationLockTimeoutError. Not done here, for two reasons:

    • Node 24's libuv (1.49 and later) deletes with POSIX semantics. The refusal lasts only until the unlinker's own handle closes, which is microseconds whoever else has the file open. Only a filesystem without POSIX delete support falls back to the legacy mode, where other handles extend it.
    • A blocker turns a persistent failure into a wait up to the caller's deadline, which defaults to 2 h for deploys.

    Base threw on the first EPERM, so this PR only narrows the window. The follow-up is recorded for triage.

  2. The retry budget is not measured on Windows (fixed-210ms-retry-budget). It reuses the 10/40/160 ms schedule ticket removal already uses, now one shared constant. If Windows still reports this EPERM, measure how long it lasts before changing the number.

  3. The tests patch fs.promises.readFile (test-injects-via-builtin-monkeypatch) rather than adding a reader parameter to scanLiveClaims, which already carries a test-only onEntriesListed. withRefusedReads calls syncBuiltinESMExports() after both the patch and the restore, so the ESM binding under TypeStrip follows it too. That keeps the production signature unchanged.

  4. Mixed versions are covered one way only. A new build tolerates claims that an older build removes; an older build still fails on this EPERM. components/DESIGN.md records this in the lock section, next to the matching note for release markers.

  5. Only EPERM is retried. Gemini suggested adding EACCES, and error?.code. Neither is adopted:

    • libuv maps the delete-pending ERROR_ACCESS_DENIED to EPERM (src/win/error.c) on every supported Node line. EACCES comes only from ERROR_ELEVATION_REQUIRED, ERROR_CANT_ACCESS_FILE and WSAEACCES, which are genuine access failures and should keep failing at once.
    • readFile and JSON.parse throw only Error objects, so there is no non-object to guard against.
  6. Planning review (codex): Framing-Verdict: chosen-approach-sound. Rejected alternatives:

    • An in-process lock for root-config publication. It misses out-of-process writers such as set_configuration from the CLI and restart, and the deploy callers of the same lock would keep the bug.
    • Renaming a claim aside before unlinking it. A process running an older build still unlinks directly, so readers need this retry anyway, and the rename would touch all three removal sites and add a tombstone state that needs its own orphan sweep.
    • Mapping EPERM to "absent". That drops a live ticket and admits two holders.

Where to look hardest: the retry loop in readOwner. It must return "absent" only for ENOENT or a parse error.

Verification

  • Fails on base, passes with the fix. The two new tests are a choosing claim refused mid-unlink resolves to the ticket it upgraded to and a live ticket whose read is refused is never dropped. The second covers a refusal that clears (the ticket is kept) and one that persists (the scan rejects and the file stays). On base both fail with the CI stack (readOwner → scanLiveClaims → EPERM); with the fix both pass.
  • npm run test:unit:main: 5942 passing, 1 failing. The failure is gitCredentials.test.js:395 and is environmental: the dispatch sandbox exports GIT_CONFIG_GLOBAL and GIT_SSH_COMMAND, and the suite passes 19/19 with them unset.
  • npm run test:unit:resources: 3048 passing, 0 failing.
  • Integration, the deploy and components suites (integrationTests/deploy/** and integrationTests/components/**, which take this lock through real deploys): 247 pass, 0 fail, 1 skipped. The rest of test:integration:all is left to CI.
  • npm run lint:required, tsc --noEmit, prettier and npm run check:design-docs: clean.
  • End-to-end route: this PR's Windows Unit Test leg runs rootConfigPublication.test.js's five-writer race, which is the real reproduction. It is probabilistic (two hits in recent Windows runs), so a green leg is evidence, not proof. Linux has no delete-pending state, which is why the unit tests inject the error.
  • TypeStrip mode was not run locally: NODE_OPTIONS=--conditions=typestrip fails in unitTests/mocha.init.js on an unrelated JSON import attribute (ERR_IMPORT_ATTRIBUTE_MISSING), on base as well.

Complexity: easy

Claude Opus 5.5

🤖 Generated with Claude Code

Origin — the dispatch brief this PR was written from

harper main is red: Unit Test at 8f411b3

main on HarperFast/harper went from green to red. Failing workflows: Unit Test at 8f411b3.

Find the merge that broke it and get main green. Walk that workflow's recent push runs on main oldest-to-newest to find the FIRST red head SHA and the PR it came from, then pull the failing test names from that run and from the current head. A failure on every matrix leg (Node version, Bun, uWS, Windows) is a regression; one leg is usually a flake.

Then either fix it or, when the culprit is a single merge and the fix is not obvious, open a revert and say so — main being green is worth more than the change being preserved. Record the fingerprint on the initiatives/ci-health board doc either way (prose row beginning with the test path, never a bare ref), and add your PR to initiatives/testing-and-deploy > "CI & build infrastructure" as a bare-ref line so it renders as a card.

Dispatch: task main-red-kriszyp_harper_8f411b369_4d7eefda · queued by automation · ran by claude/opus/xhigh · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=cursor-muse,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi; rounds=1; full=1 @ dbcbedb

Human-Review-Need: 3 (decisions: unreadable-claim-fails-scan, fixed-210ms-retry-budget, test-injects-via-builtin-monkeypatch) @ dbcbedb

…iling the acquire

On Windows, opening a file whose unlink is in progress fails with EPERM until the
unlinking handle closes, after which the name is gone. readOwner treated only ENOENT
as a vanished claim, so a contender scanning while a peer removed its choosing claim
failed the whole acquire. harper#2796's five-writer root-config race test hit it on
the Windows unit leg (main run 36144493772 and PR run 36083964824); every deploy path
that takes the component preparation lock has the same exposure under contention.

readOwner now retries an EPERM read with the file's short backoff until the claim
reads or is gone; one that stays unreadable still fails the scan, since reading a
live ticket as absent would admit a second holder.

Dispatch-Task: main-red-kriszyp_harper_8f411b369_4d7eefda
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gemini-code-assist[bot]

This comment was marked as resolved.

@kriszyp
kriszyp marked this pull request as ready for review September 25, 2026 18:10
@kriszyp
kriszyp merged commit 59bb349 into main Sep 25, 2026
55 of 56 checks passed
@kriszyp
kriszyp deleted the fix/lock-claim-read-windows-delete-pending branch September 25, 2026 18:10
@claude

claude Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

dawsontoth added a commit that referenced this pull request Sep 25, 2026
main's #2803 retries an EPERM claim read inside readOwner, which fixes the
Windows race this branch had worked around with a scan-level retry; that
retry is dropped in favour of it. The release's fallback for a record it
cannot read stays, now behind readOwner's own retry. The design note keeps
both #2803's paragraph and this branch's marker-owner sentence.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant