test(lock): let contention watchdog own liveness - #217
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. Reviewed September 3, 2026, 1:00 PM ET / 17:00 UTC. ClawSweeper reviewWhat this changesThe PR moves sidecar-lock contention proof workers to an unbounded per-acquisition wait governed by their existing 60-second parent watchdog, and adds a static contract test plus an Unreleased changelog entry. Regression provenancePossible regression — probable (reproduction; reviewed change; failure trace). No predecessor PR is attributed. Merge readiness✅ Ready for maintainer review Keep open: this is a focused, active CI-proof repair with strong after-fix Windows evidence and no introduced correctness or security defect found. Priority: P3 Review scores
Verification
How this fits togetherThe sidecar contention proof exercises the public cross-process file-lock API in child processes and is run by CI in JavaScript-fallback and required-native modes. It verifies mutual exclusion, ownership, release, and cleanup without changing the package’s production lock behavior. flowchart LR
A[CI job] --> B[Contention proof]
B --> C[Four child workers]
C --> D[Sidecar lock acquisition]
D --> E[Ownership and counter checks]
C --> F[60-second worker watchdog]
F --> G[Kill and reap on stall]
E --> H[Pass or fail result]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the narrowly scoped proof-harness change after normal maintainer review, retaining the finite permission-case budget and the 60-second kill-and-reap watchdog. Do we have a high-confidence way to reproduce the issue? Yes—retained Windows evidence shows the prior false timeout under healthy contention, and the current-main baseline in the verified PR delta has the finite worker acquisition budget that permits it. Is this the best way to solve the issue? Yes—the change uses the documented unlimited-wait mode only inside watched proof workers while preserving the finite budget where no parent watchdog exists. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ae0cc7e3f6e6. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
|
Windows validation update: this PR remains on hold; the declared five-run whole-proof gate has not passed. At candidate The failed fallback Windows job reported pathname-async 100/100, pathname-sync 100/100, root-async 78/100, and root-sync 100/100. One root-async worker returned The PR's own Windows outputs show 400/400 in both fallback and native modes on Node 24.19.0 x64, including exact counters and marker/sidecar absence. That is useful after-fix evidence, but it does not erase the independent failure. The observed |
|
Combined Windows validation is complete for the lock changes in #217 and #218. Frozen validation head: Five independent workflow-dispatch runs, all attempt 1, each completed Windows x64 / Node 24.19.0 fallback
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 Local combined contention also passed 400/400 plus 2/2 permission recovery acquisitions in each of 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. |
|
Windows proof is now in the updated PR body and combined proof comment. The standalone PR head passed 400/400 in both modes; the unchanged watchdog patch plus the separate #218 repair has five complete independent Windows fallback/native pairs (4,000 acquisitions), with checkout SHA verification, retained failed attempts, and executed watchdog negative controls. The validation-only merge is clearly distinguished from this PR's head. Please replace the earlier future-proof verdict using the current evidence. No source changes were made to this PR, and no failed workflow was erased or reclassified as passing. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Update the watchdog proof PR onto main without changing its script or guard. The resulting tree matches the frozen combined Windows validation tree byte for byte.
|
Updated this PR onto the separately landed Root denial repair, #218 ( Current PR head: The tree is byte-for-byte identical to frozen validation head Focused tests on the updated branch passed 26/26. Updated-base Codex autoreview found no accepted/actionable P0 findings. Fresh exact-head CI is running. The documented failure-latency consequence is accepted: watched children may spend up to the existing 60-second whole-worker deadline, rather than requiring each acquisition to succeed within 15 seconds. The deadline is unchanged and tested to kill/reap blocked sync children; production lock budgets and the unwatched parent permission case remain bounded as before. This is the intended separation between contention safety and harness liveness. The original Root async EPERM failure and the later pre-proof store-stress timeout remain preserved, with the latter not counted as a successful proof. |
clawsweeper
Bot
added
proof: sufficient
Sidecar watchdog proof repair landedPR: #217 Watched contention children now use an infinite per-acquisition deadline with omitted retry count. The existing 60-second parent watchdog remains the sole whole-worker liveness bound; four cases, four workers per case, 25 acquisitions per worker, ownership checks, exclusive marker, exact counters, release barrier, and kill/reap cleanup are unchanged. Unwatched POSIX permission acquisitions retain the finite 15-second default. Production LOC changed by this PR: zero. The initial Windows Root-sync failure exhausted per-acquisition budgets while other workers progressed and before the whole-worker watchdog fired. Cross-process FIFO fairness is not a lock contract. This repair removes that accidental proof requirement without changing production lock budgets or safety checks. The exact historical scheduling cause is not claimed to be proven. Initial local proof passed 400/400 plus 2/2 permission acquisitions in each mode. Initial full check passed 7,085 tests / 80 skipped; security 84; package smoke and diff checks passed. Updated-base focused tests passed 26/26, and autoreview found no accepted P0 findings. Exact final-head CI run 33781536626 passed; ClawSweeper reviewed head 6199a22 with no correctness/security findings and sufficient Windows logs. Five complete independent Windows x64 / Node 24.19.0 fallback/native pairs, all attempt 1, passed on frozen integration head dab64db. Its entire tree is identical to the landed tree. Each of ten invocations completed 400/400 with counter and marker/sidecar absence assertions: 4,000 successful acquisitions. Every checkout SHA was verified from job logs. Evidence: root-create-denial-combined-windows-summary.json and per-job logs. Six workflows were launched. Run 33777327817 failed its existing private-store stress test before fallback proof; it was preserved, not rerun or counted as passing. The supplemental independent run provided the missing executed pair. The earlier watchdog-only run 33767148513 also remains preserved: Root async EPERM was separate from this harness concern and led to the independently reproduced Root-create fix in #218. No claim that all workflows passed or that the original EPERM syscall was conclusively identified. Disposable negative controls executed the actual watchdog: four deliberately blocked sync children were SIGKILLed after the unchanged 60-second deadline and reaped before scratch cleanup. Deliberately retained sidecars caused all contention cases to fail the absence check despite counters reaching 100. Both fault fixtures exited nonzero and left scratch empty. After merging, main was synchronized and verified clean at 9edca2c. A fresh build, the 26 relevant regressions, and complete built-package off/require proofs were rerun from actual merged main on macOS arm64 / Node 24.20.0. Both modes passed 400/400 plus the POSIX permission recovery case. Outputs: sidecar-landed-main-{regressions.log,off.jsonl,require.jsonl}. Separate local combined full-check attempts stalled or timed out in other fixtures and remain failed evidence. The dispatch subprocess lifetime follow-up is PR219 and is not part of this merge. No release, tag, version bump, or registry publication was performed. |
1 participant
Summary
Root cause
The contention proof launches four processes and requires each to complete 25 acquisitions. Every acquisition previously had a 15-second timeout and a finite retry count, while the parent independently enforced a 60-second whole-worker watchdog.
Actual Windows native proof reproduced the mismatch:
file_lock_timeoutin acquisition;Cross-process locks promise exclusion, exact ownership, fail-closed stale behavior, and caller-selected waiting budgets—not FIFO fairness. A releasing worker can reacquire while another process sleeps or inspects the old record. Requiring every contender to win within 15 seconds can therefore false-fail while other workers are safely progressing.
Repair
Contention children now use
lockOptions(Infinity)with no retry-count limit. The existing 60-second parent watchdog owns liveness for their whole workload. It starts at child launch, kills a stuck child, resolves failed barriers on child closure, and reaps every sibling before scratch cleanup.The finite 15-second default remains in
lockOptions()and is still used by the POSIX permission/release case, which has no child watchdog. Backoff/jitter, stale timeout, and fail-closed recovery are unchanged.The proof still requires:
No production source, lock API/default, fairness behavior, denial cap, test timeout, workflow timeout, workload size, native code, exports, or types changed. Production LOC delta: zero.
Regression guard
A new static contract test reads the standalone proof without importing or launching it and pins:
lockOptions()default;Infinityand omitted retry count;SIGKILL, child-close settlement, sibling kill, andPromise.allSettledreaping.Local validation
off: 400/400 contention plus 2/2 permission acquisitions;require: 400/400 contention plus 2/2 permission acquisitions;CI=1 pnpm check: 202 test files passed / 2 skipped, 7,085 tests passed / 80 skipped;git diff --check: passed;Neither local proof hit the watchdog or failed an exclusion, identity, counter, release, or cleanup assertion.
Windows validation results
The standalone PR run passed 400/400 in both Windows fallback and Windows required-native modes. Four of the original five whole workflow runs passed. The fifth, 33767148513, failed with a separate Root async
EPERM; it remains preserved and was not retried away. That discovery prompted the separate production repair in #218, rather than broadening this harness-only PR.Complete combined after-fix proof now records five independent Windows x64 / Node 24.19.0 fallback/native pairs at frozen integration head
dab64dbb922afb5a86cd2fb782544d9d334a3052, tree0e27501c81d73003da4a05c48df6e7d1fbc30e3d: ten successful 400/400 invocations, 4,000 acquisitions, exact counters, marker/sidecar absence, and no watchdog expiry. Checkout SHAs were verified from job logs. This is the combined validation tree, not this PR's standalone head; this PR's script and guard are byte-identical in that tree.Six combined workflows were launched: five completed both proofs, while 33777327817 failed an existing private-store stress test before its fallback proof. The additional independent run supplied the missing executed pair; the failed workflow is retained, not counted as passing. No dedicated physical Windows provider is claimed: these are actual Windows GitHub Actions executions.
Disposable local negative controls also verified that four deliberately blocked sync children were killed and reaped at the existing 60-second watchdog, and that retained sidecars fail the proof despite complete counters. Both exited nonzero and left their scratch roots empty. Local combined full-check attempts separately stalled in a workflow-dispatch fixture and are preserved as failures, not represented as successful checks.