Repository navigation
Retry a lock claim read that Windows refuses mid-unlink instead of failing the acquire - #2803
Merged
Merged
Conversation
…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>
kriszyp
marked this pull request as ready for review
September 25, 2026 18:10
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi and
heskew
September 25, 2026 18:10
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>
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
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.
Windows refuses to open a file whose unlink is in progress: the open fails with
EPERMuntil the unlinking handle closes, then the name is gone. The component preparation lock's claim reader treated onlyENOENTas 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 onmainat 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.readOwnernow retries anEPERMread 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:Segmentation faultright afterdatabaseAliasIdentity.test.jsderivedIndexCoverage.test.js:184"Missing expected rejection"rootConfigPublication.test.js:256EPERMinreadOwnerFor the human reviewer
A claim that stays unreadable still fails the scan (
unreadable-claim-fails-scan). After 210 ms ofEPERMthe caller gets a plainEPERM. Boot activation recovery records that as a durable.unsettledverdict (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 inComponentPreparationLockTimeoutError. Not done here, for two reasons:Base threw on the first
EPERM, so this PR only narrows the window. The follow-up is recorded for triage.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 thisEPERM, measure how long it lasts before changing the number.The tests patch
fs.promises.readFile(test-injects-via-builtin-monkeypatch) rather than adding a reader parameter toscanLiveClaims, which already carries a test-onlyonEntriesListed.withRefusedReadscallssyncBuiltinESMExports()after both the patch and the restore, so the ESM binding under TypeStrip follows it too. That keeps the production signature unchanged.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.Only
EPERMis retried. Gemini suggested addingEACCES, anderror?.code. Neither is adopted:ERROR_ACCESS_DENIEDtoEPERM(src/win/error.c) on every supported Node line.EACCEScomes only fromERROR_ELEVATION_REQUIRED,ERROR_CANT_ACCESS_FILEandWSAEACCES, which are genuine access failures and should keep failing at once.readFileandJSON.parsethrow onlyErrorobjects, so there is no non-object to guard against.Planning review (codex):
Framing-Verdict: chosen-approach-sound. Rejected alternatives:set_configurationfrom the CLI and restart, and the deploy callers of the same lock would keep the bug.EPERMto "absent". That drops a live ticket and admits two holders.Where to look hardest: the retry loop in
readOwner. It must return "absent" only forENOENTor a parse error.Verification
readOwner→scanLiveClaims→EPERM); with the fix both pass.npm run test:unit:main: 5942 passing, 1 failing. The failure isgitCredentials.test.js:395and is environmental: the dispatch sandbox exportsGIT_CONFIG_GLOBALandGIT_SSH_COMMAND, and the suite passes 19/19 with them unset.npm run test:unit:resources: 3048 passing, 0 failing.integrationTests/deploy/**andintegrationTests/components/**, which take this lock through real deploys): 247 pass, 0 fail, 1 skipped. The rest oftest:integration:allis left to CI.npm run lint:required,tsc --noEmit, prettier andnpm run check:design-docs: clean.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.NODE_OPTIONS=--conditions=typestripfails inunitTests/mocha.init.json 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
mainon 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
mainoldest-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-healthboard doc either way (prose row beginning with the test path, never a bare ref), and add your PR toinitiatives/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-1Review-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