Skip to content

fix(lock): retry Root fallback exclusive-create denials - #218

Merged
steipete merged 1 commit into
mainfrom
fix/root-sidecar-create-denial
Sep 3, 2026
Merged

fix(lock): retry Root fallback exclusive-create denials#218
steipete merged 1 commit into
mainfrom
fix/root-sidecar-create-denial

Conversation

@steipete

@steipete steipete commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix the Windows Root-backed sidecar exclusive-create retry gap without changing Root confinement, retry limits, or the watchdog harness in #217.

Pathname lock acquisition already classifies a Windows exact-lock-path EPERM from its exclusive open and retries it within an eight-denial cap and the caller's retry/deadline budget. Root-backed creation instead called Root.create() without carrying the exclusive-open provenance back to that policy. Its fallback could therefore reject on the first such denial even when retry budget remained.

Reproduction and narrow fix

A deterministic regression forces native mode off and injects EPERM at the actual Root fallback exclusive fs.open(O_CREAT | O_EXCL) call, using real temporary files. Before this patch, three assertions fail: a one-time denial cannot recover; a caller allowing two retries gets only one attempt; and the eight-denial policy never gets beyond the first attempt. The zero-retry control passes.

The patch records a distinct exclusive-create receipt at the failing open, using the existing operation-local observation mechanism. Each Root.create() call has a fresh observation. Only a matching escaping error, exact lock path, and Windows EPERM classification enter the existing retry path. No persistent tags are added to errors.

A generic Root.create() error, ordinary read/open error, parent/other-path denial, callback error, post-create write/stat failure, or error replayed from another attempt/acquisition is not exclusive-open evidence. Retrying creates no ownership, stale-reclaim, or deletion authority; normal Root creation and exact creator admission must succeed anew.

Production delta: +14/-4, net +10 lines across four internal files. No public signatures, dependencies, native code, denial caps, stale policy, or production timeout behavior changed.

Validation

  • Baseline regression: 3 expected failures / 1 passing zero-retry control.
  • Added suite: 21 tests covering recovery, exact original errors, caller retry limits, finite deadlines, the cap shared with snapshot reads, wrapper/callback/replay isolation, and post-create failures.
  • Focused receipt/admission/budget neighborhood: 260 passed / 6 platform-specific skipped.
  • CI=1 pnpm check: 202 suites passed / 2 skipped; 7,101 tests passed / 80 skipped, including build, boundary/size gates, documentation and package contracts.
  • pnpm test:security: 84 passed.
  • git diff --check: passed.
  • Codex autoreview: no accepted/actionable P0 findings in the complete local change bundle.

Local regressions used macOS arm64, Node 24.20.0, synthetic Windows/errno selection, and real files. They are not claimed as actual Windows execution. Actual Windows CI output follows below. Combined five-pair contention proof additionally records ten successful Windows fallback/native invocations at one frozen integration head, with all failed or pre-proof-blocked attempts preserved.

Actual Windows after-fix evidence

CI run 33774729358, attempt 1 passed. Both Windows jobs checked out 9fca7dd98f64acbc0ceb95894d598ed62a1f2eaf (recorded in their checkout logs), the PR merge commit whose tree is 4ab0ed3a7294e5f798fd7f3f6f9304ed4237df8b, identical to candidate 570fa9710693b4fa209cd2441585b518e43752ef.

The Windows fallback job executed the new regression suite on the actual Windows filesystem:

test/sidecar-lock-root-create-denial.test.ts (21 tests) 376ms — passed

Those 21 passing assertions include one-time exclusive-open denial recovery, exact original-error preservation with zero retries, three attempts with two retries, nine attempts at the eight-denial cap, finite deadline exhaustion, the cap shared across create and snapshot opens, and wrapper/callback/replay/post-create failure rejection. The EPERM in these tests is deliberately injected; executing them on Windows does not turn it into a naturally observed OS error.

The same Windows x64 / Node 24.19.0 fallback job then exercised the public built package without fault injection:

nativeMode=off pathname-async 100/100 passed; counter=100; markerAbsent=true; sidecarAbsent=true
nativeMode=off pathname-sync  100/100 passed; counter=100; markerAbsent=true; sidecarAbsent=true
nativeMode=off root-async     100/100 passed; counter=100; markerAbsent=true; sidecarAbsent=true
nativeMode=off root-sync      100/100 passed; counter=100; markerAbsent=true; sidecarAbsent=true
summary: 16 processes, 400/400 acquisitions, passed-with-skip (POSIX permission case unavailable on Windows)

The native Windows job, also Node 24.19.0 x64, passed all four cases at 100/100 and the require summary at 400/400 with exact counters and marker/sidecar absence. Native-mode success is a control; the changed fallback path is exercised by the no-binding job. No watchdog failure occurred in these passing invocations.

This verifies this PR's Windows behavior but does not satisfy the separate five-run combined gate for #217. Neither one green PR run nor a synthetic error proves the earlier naturally observed EPERM has been fully explained.

Relationship to the held watchdog PR

PR #217 changes only the contention harness's liveness owner. It remains held: four of its five independent workflow runs passed, while 33767148513 failed with EPERM in fallback Root async acquisition (78/100); Root sync passed 100/100 in that same run. The failed run was not retried away.

The redacted worker report does not identify the failing syscall. This patch fixes an independently reproduced, contractually retryable create-path gap; it does not establish that this exact path caused that earlier live failure. Resolver/churn hypotheses require their own deterministic evidence before any further production changes. No broad catch around Root.create()/Root.open() and no larger magic retry cap is proposed.

The implementations stay separate. Both fixes must be exercised together in a frozen Windows candidate before the campaign's whole-proof hold is cleared. No release or publication is part of this PR.

Record exclusive-open failures within each Root create observation and route only matching Windows lock-file denials through the existing bounded retry policy. Preserve caller budgets, denial caps, original errors, and replay isolation.
@steipete
steipete requested a review from a team as a code owner September 3, 2026 15:47
@clawsweeper

clawsweeper Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 3, 2026
@clawsweeper

clawsweeper Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 3, 2026, 12:42 PM ET / 16:42 UTC.

ClawSweeper review

What this changes

The PR records Root fallback exclusive-create failures per call so Windows exact-lock-path denials can use the existing bounded sidecar-lock retry policy.

Regression provenance

Possible regression — probable (reviewed change; reproduction). No predecessor PR is attributed.

Merge readiness

Ready for maintainer review

This PR remains necessary: current main is its base and does not contain the Root-create provenance bridge. The introduced path is narrow, preserves fail-closed ownership behavior, has no correctness finding, and has sufficient after-fix Windows evidence.

Priority: P2
Reviewed head: 570fa9710693b4fa209cd2441585b518e43752ef

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, well-contained repair with strong platform-specific behavioral evidence and no actionable defect found.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): A Windows fallback job on the verified PR merge tree exercised the changed Root fallback suite after the fix and observed all 21 assertions passing; the same built package completed Root async contention with 100/100 acquisitions and absent cleanup markers. The synthetic EPERM is appropriately limited to demonstrating the exact retry branch rather than claiming a naturally observed OS failure.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): A Windows fallback job on the verified PR merge tree exercised the changed Root fallback suite after the fix and observed all 21 assertions passing; the same built package completed Root async contention with 100/100 acquisitions and absent cleanup markers. The synthetic EPERM is appropriately limited to demonstrating the exact retry branch rather than claiming a naturally observed OS failure.
Evidence reviewed 6 items Introduced retry classification: The Root-backed creation branch creates a fresh observation, runs only that Root.create call inside it, and enables retry only when the escaping error carries the matching exclusive-create receipt and exact Windows denial classification.
Fallback failure receipt: The confined missing-file fallback records provenance only around its O_CREAT|O_EXCL open; subsequent stat/write failures remain outside the receipt boundary.
Focused regression coverage: The added 21-case suite covers recovery, caller and eight-denial budgets, deadlines, replay isolation, wrong paths, ordinary opens, and post-create failures using real temporary files.
Findings None None.
Security None None.

How this fits together

Sidecar locks coordinate exclusive access to files. Lock acquisition uses either direct filesystem calls or a confined Root abstraction, then returns a verified lock handle or the original failure.

flowchart LR
  A[Lock request] --> B[Sidecar lock acquisition]
  B --> C{Root-backed lock?}
  C -->|yes| D[Confined exclusive creation]
  C -->|no| E[Direct exclusive creation]
  D --> F[Denial provenance check]
  E --> F
  F --> G[Bounded retry policy]
  G --> H[Verified lock handle or original error]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +14/-4; tests +264 The net ten-line production change is narrowly paired with focused denial, budget, and replay-isolation coverage.
Review surface 7 files affected The patch confines behavior changes to the Root fallback, sidecar acquisition policy, documentation, changelog, and one dedicated test suite.

Technical review

Best possible solution:

Land the focused provenance bridge with its regression suite, preserving the existing eight-denial and caller retry/deadline budgets.

Do we have a high-confidence way to reproduce the issue?

Yes—source establishes that the pre-patch Root branch could not set the existing create-denial retry flag, and the focused suite injects the exact fallback exclusive open on real temporary files.

Is this the best way to solve the issue?

Yes—the per-invocation receipt extends the established narrow denial policy without broadly retrying Root.create errors or granting cleanup authority.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against e9b98dff22da.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: The patch repairs a bounded Windows fallback lock-acquisition failure without evidence of a broad production outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): A Windows fallback job on the verified PR merge tree exercised the changed Root fallback suite after the fix and observed all 21 assertions passing; the same built package completed Root async contention with 100/100 acquisitions and absent cleanup markers. The synthetic EPERM is appropriately limited to demonstrating the exact retry branch rather than claiming a naturally observed OS failure.
  • proof: sufficient: Contributor real behavior proof is sufficient. A Windows fallback job on the verified PR merge tree exercised the changed Root fallback suite after the fix and observed all 21 assertions passing; the same built package completed Root async contention with 100/100 acquisitions and absent cleanup markers. The synthetic EPERM is appropriately limited to demonstrating the exact retry branch rather than claiming a naturally observed OS failure.

Evidence

What I checked:

  • Introduced retry classification: The Root-backed creation branch creates a fresh observation, runs only that Root.create call inside it, and enables retry only when the escaping error carries the matching exclusive-create receipt and exact Windows denial classification. (src/sidecar-lock-acquire.ts:161, 570fa9710693)
  • Fallback failure receipt: The confined missing-file fallback records provenance only around its O_CREAT|O_EXCL open; subsequent stat/write failures remain outside the receipt boundary. (src/root-impl.ts:1702, 570fa9710693)
  • Focused regression coverage: The added 21-case suite covers recovery, caller and eight-denial budgets, deadlines, replay isolation, wrong paths, ordinary opens, and post-create failures using real temporary files. (test/sidecar-lock-root-create-denial.test.ts:36, 570fa9710693)
  • Current-main status: The checked-out PR head has e9b98df as its recorded parent, which is also local main/origin/main; the patch is therefore not already implemented on main. (src/sidecar-lock-acquire.ts:164, e9b98dff22da)
  • After-fix Windows proof: The supplied PR evidence identifies a Windows fallback job on the verified PR merge tree that passed the new 21-test suite and a built-package Root async contention run at 100/100 acquisitions with cleanup markers absent. The injected EPERM validates the targeted retry path; the built-package run validates normal fallback operation. (test/sidecar-lock-root-create-denial.test.ts:37, 9fca7dd98f64)
  • Feature-history routing: Recent merged lock work includes retry-budget, stale-observation, and callback-isolation changes by the same contributor, establishing a strong current-main history connection to this subsystem. (src/sidecar-lock-acquire.ts:114, 90e22416aaa1)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-03T15:51:37.549Z sha 570fa97 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-03T16:12:51.615Z sha 570fa97 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 3, 2026
@steipete

steipete commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Combined Windows validation is complete for the lock changes in #217 and #218.

Frozen validation head: dab64dbb922afb5a86cd2fb782544d9d334a3052; tree: 0e27501c81d73003da4a05c48df6e7d1fbc30e3d. This combines the unchanged reviewed watchdog patch 07e65986b15398d82db7c74bb4cd683710490785 and the separate Root exclusive-create retry patch 570fa9710693b4fa209cd2441585b518e43752ef. It is a validation-only merge, not either PR's standalone head and not a merge to main.

Five independent workflow-dispatch runs, all attempt 1, each completed Windows x64 / Node 24.19.0 fallback off and native require:

Run Fallback Required native
33777333762 400/400 400/400
33777339065 400/400 400/400
33777343904 400/400 400/400
33777348487 400/400 400/400
33778308002 400/400 400/400

Each job's checkout log was verified against the frozen SHA, not merely its workflow metadata. All four cases per invocation reported 100/100, counter 100, markerAbsent=true, and sidecarAbsent=true. Each summary reported 16 launched processes and 400/400 contention acquisitions. Total: ten successful Windows proof invocations, 4,000 acquisitions. The POSIX permission case is explicitly skipped on Windows. The new 21-test Root exclusive-create regression suite also passed in each successful Windows fallback job, covering injected denial recovery, bounded exhaustion, and original-error/replay protections.

Preserved failure: six workflows were launched. 33777327817, attempt 1, failed its existing private-store stress test at 5,000ms before reaching fallback contention. It was not rerun or counted as a passing fallback proof. Its native job passed. The additional independent run obtained the missing executed proof; it does not turn the failed workflow into a pass.

The earlier watchdog-only failure 33767148513, with Root async EPERM, also remains preserved. No EPERM or watchdog failure appeared in the ten successful combined invocations. This is after-fix behavioral evidence, not proof that the original runtime syscall has been conclusively identified or that no undiscovered bug remains.

Local combined contention also passed 400/400 plus 2/2 permission recovery acquisitions in each of off and require on macOS arm64 / Node 24.20.0. Disposable harness negative controls verified that blocked sync children are killed and reaped by the unchanged 60-second watchdog, and that deliberately retained sidecars cause failure despite counters reaching 100. Both negative fixtures exited nonzero and left their scratch roots empty.

Local combined full-check failures are separately preserved: one stalled in an unbounded workflow-dispatch Bash fixture and was manually interrupted; its repeat reached an external validation watchdog. That test-lifetime issue is being handled separately; these attempts are not claimed as local full-check passes. The production-only Root repair's complete local check passed 7,101 tests / 80 skipped and its CI passed across platforms.

Neither PR is being represented as a release, and their implementation scopes remain separate.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Sep 3, 2026
@steipete
steipete merged commit ae0cc7e into main Sep 3, 2026
37 checks passed
@steipete

steipete commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Root exclusive-create denial repair landed

PR: #218
Merge: ae0cc7e
Reviewed head: 570fa97
Reviewed and merged tree: 4ab0ed3a7294e5f798fd7f3f6f9304ed4237df8b
Base: e9b98df

Root-backed sidecar creation now carries per-call exclusive-open failure provenance into the existing Windows exact-path EPERM retry policy. The eight-denial cap, caller retry/deadline limits, original errors, fresh Root admission, and ownership/reclaim boundaries are unchanged. Arbitrary Root wrappers, callback errors, earlier error receipts, ordinary opens, and post-create write/stat failures do not qualify. Production delta: net +10 lines.

The deterministic regression failed three assertions before the repair (first denial escaped instead of recovery/three attempts/nine attempts), while the zero-retry control passed. The final new suite has 21 cases. Focused neighborhood: 260 passed / 6 skipped. Complete production-branch pnpm check: 7,101 passed / 80 skipped. Security: 84 passed. Build, docs, size/boundary lint, package contracts, diff-check, and P0-scoped Codex autoreview passed. ClawSweeper's replacement review found no actionable correctness/security issue and rated proof/patch 5/6.

Exact-head CI run 33774729358, attempt 1, passed all matrix jobs. Windows checkout logs identify PR merge commit 9fca7dd; its tree matches the reviewed/landed tree. Windows Node 24.19.0 x64 fallback job 100713466558 ran all 21 new regression cases and completed the built public-API contention proof at 400/400. Native job 100713466406 also completed 400/400.

Combined watchdog + Root repair verification: frozen head dab64db, tree 0e27501c81d73003da4a05c48df6e7d1fbc30e3d. Five independent complete Windows fallback/native pairs, all attempt 1, produced ten passing proof invocations and 4,000 acquisitions. Source and proof-script equivalence to the individual PRs was verified by explicit git diffs. Evidence: root-create-denial-combined-windows-summary.json and per-job logs. Six workflows were launched: 33777327817 failed an existing private-store stress test before fallback proof; it was retained, never rerun or counted as passing. A sixth independent workflow supplied the missing executed pair.

The original watchdog-only EPERM run 33767148513 remains preserved. Its exact failing syscall was not captured, so this repair is not claimed to conclusively identify that historical operation. No EPERM or watchdog failure occurred in the ten successful combined invocations. This does not prove absence of undiscovered bugs.

Local combined contention passed 400/400 plus 2/2 POSIX permission acquisitions in each mode. Disposable negative controls proved the 60-second watchdog killed/reaped blocked sync children and retained sidecars could not produce a passing proof. Separate local combined full-check attempts stalled in the workflow-dispatch fixture and are recorded as failures; the test-only follow-up is PR219, not part of this production repair.

No version bump, tag, release, or registry publication was performed. main was synchronized and verified clean after merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant