One shared "claim that can go stale" for the install lock and mutation runs - #2042
Conversation
A claim is a file naming its owner and when the owner last checked in, copying the shape of the database migration lock: take a free claim or one whose owner walked away, keep touching it while working, release only your own. The stripe-mock install now uses the shared module instead of its hand-rolled protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
…lock The supervisor takes a claim on the run's folder before the record's first write and keeps it fresh until the snapshot is gone, using the same stale-claim module as the stripe-mock install. Liveness questions — what --list shows, what --kill may signal, what a clear-up may delete — are all answered by that one claim. This closes two gaps the old run lock had. A run no longer reads as dead in the moment between its child ending and the supervisor writing the final record, because the claim spans both. And a clear-up holding the old lock can no longer make a dead run look alive to --kill, because only the run's own supervisor writes and touches its claim — so a process id handed to an unrelated program is never signalled once the claim goes stale. The run lock, its child-process probe, the lock-holding child, and the startup grace all fall away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
The claim module now offers withClaim (take, work, release), which the install lock and each mutation run's supervisor both use. The install and start locks share binDirGuard, and LockBody moves to lock-file.ts so every hold names its body the same way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
The mutation cleanup's folder-age check was its only deterministic coverage, and that check is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
The parse quirks, the touch that outlives a release, the rearmed timer a release must cancel, and the pause between retries each get a test of their own. The one equivalent mutant — delay(0) against delay(1) after the stop flag — is recorded with its proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR replaces mutation-run locks and process-liveness checks with supervisor-held stale claims. It applies claim coordination to mutation execution, cleanup, snapshot children, and Stripe mock installation. It also updates interruption handling, tests, and equivalent-mutant records. ChangesStale claim coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MutationIsolation
participant ClaimFile
participant SnapshotChild
participant Cleanup
MutationIsolation->>ClaimFile: acquire run claim
MutationIsolation->>SnapshotChild: start with supervisor PID
SnapshotChild->>ClaimFile: refresh claim during work
Cleanup->>ClaimFile: check claim freshness
SnapshotChild->>ClaimFile: age claim when supervisor is absent
MutationIsolation->>ClaimFile: release claim after settlement
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f53473e5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function touchClaim() { | ||
| latestTouch = writeClaimTime(path, owner).then( | ||
| scheduleNextTouch, | ||
| scheduleNextTouch, | ||
| ); |
There was a problem hiding this comment.
When Deno.writeTextFile rejects for longer than staleMs—for example because the disk becomes read-only—the rejection handler silently schedules another touch and lets the protected install or mutation run continue. Another process can then treat the old timestamp as abandoned and take the same claim, so both bodies run concurrently; a later successful touch by the first holder can even overwrite the new owner's record. Surface the heartbeat failure to the holder instead of discarding it.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72f9ffb. The touch loop now tracks how long its writes have been failing: an outage spanning the whole stale window sets a sticky flag (a later successful touch cannot clear it, since the claim may already have been taken and overwritten), and release() throws a contextful error instead of letting the work end quietly. A transient blip that recovers inside the window stays quiet. Regression tests drive a fake clock through all four shapes — outage spanning the window, recovery arriving too late, an outage only the release can see, and a blip that heals (test/scripts/stale-claim.test.ts, "a holder whose touches stop landing").
Generated by Claude Code
| (await runClaimIsFresh(record)) | ||
| ? null | ||
| : await removeRunPath(record, record.root); |
There was a problem hiding this comment.
When two mutation commands start together, one cleanup can observe the other command's newly created run directory before its claim file exists, get false here, and then delete the directory after the supervisor has created its fresh claim. The supervisor creates the directory first for claim.guard, and this check and removeTree are not serialized with claim creation, so the affected run can fail during its first record write or snapshot copy. Hold the claim guard and recheck immediately before deleting, or create the initial claim somewhere cleanup cannot remove.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72f9ffb. removeRun now judges the claim and deletes the folder inside withRunClaimGuard — the same takers' guard the supervisor holds while creating its claim — so a sweep either waits out a mid-take supervisor and then sees the fresh claim, or finishes before the take begins (in which case withFileLock's make-the-folder-again loop hands the supervisor a fresh folder to claim). Regression test: "waits at the takers' door, honouring a claim made meanwhile" in test/scripts/mutation/isolation-cleanup/removing.test.ts holds the guard, writes the claim under it, and proves the sweep skips the run.
Generated by Claude Code
| requireOwnSnapshot(); | ||
| return body(); |
There was a problem hiding this comment.
When the supervisor is killed without running its signal handler—for example by SIGKILL or the OOM killer—the mutation child keeps running, but only the dead supervisor was refreshing the claim. After 30 seconds another mutation command treats the claim as stale and deletes the snapshot out from under that live child. Previously runSnapshotChild held the run lock in the child process itself, so preserve equivalent child-owned protection or arrange for the child to terminate with its supervisor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72f9ffb. runSnapshotChild now keeps the supervisor's claim fresh itself, via keepClaimFresh — it reads the owner from the claim, touches it from the first moment, and never removes it (that stays the supervisor's release). So the claim stays fresh as long as either process lives, matching the old lock's coverage (child while working, supervisor afterwards) with no unheld window. A child whose snapshot has no claim at all refuses to work, loudly. Regression tests: "keeps the supervisor's claim fresh while it works" and "refuses to work in a snapshot no claim protects" in test/scripts/mutation/snapshot-child.test.ts.
Generated by Claude Code
| export const binDirGuard = | ||
| (holdAt: (paths: StripeMockPaths) => <T>(body: LockBody<T>) => Promise<T>) => | ||
| async <T>(paths: StripeMockPaths, body: LockBody<T>): Promise<T> => { |
There was a problem hiding this comment.
Add an explicit return type to binDirGuard; it is an exported, higher-order generic function whose public contract is currently inferred from its implementation, contrary to the repository rule requiring every exported function to state its return type so signature drift fails at the definition and the checker can cache the contract.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72f9ffb — binDirGuard now returns a named BinDirGuard type, so the exported contract is stated at the definition rather than inferred.
Generated by Claude Code
Four fixes from the pull request review. A holder now hears about touches that failed for a whole stale window: the release throws instead of the run ending quietly after its claim may have been taken by somebody else. A clear-up judges and deletes a run's folder while holding the claim takers' guard, so a brand-new run whose claim is a moment from landing cannot be swept as abandoned. The mutation child keeps its supervisor's claim fresh while it works, so a supervisor killed outright no longer costs a live child its copy after the stale window. And binDirGuard states its return type. A touch whose timer fires in the moment of the release now bails on the stopped flag instead of leaning on an extra waited turn, and the timing tests drive a fake clock instead of sleeping for real. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
The real-time setTimeout comes in as a named import instead of a module-level alias, and the release-progress check awaits instead of chaining then(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31529442eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A timer can fire in the very moment of the stop; a touch that went | ||
| // ahead then could re-create the claim file after release removed it. | ||
| if (stopped) return; | ||
| latestTouch = writeClaimTime(path, owner).then(touchLanded, touchFailed); |
There was a problem hiding this comment.
When a holder is paused or blocks the event loop for longer than staleMs (for example a suspended CI worker or laptop sleep), another process can see the old timestamp, remove the claim, and take it. The resumed holder then reaches this unconditional write with its old owner and overwrites the new owner's record; the failure tracking above only covers rejected writes, so both protected bodies can keep running and the old holder may remove the new claim on release. Treat a delayed touch/owner mismatch as a lost claim instead of rewriting it.
AGENTS.md reference: AGENTS.md:L85-L85
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48beeea. Every touch now runs under the takers' guard and only writes while the claim still names this holder (touchIfStillOurs via whileStillOwned) — a resumed holder that finds somebody else's name stops touching, never overwrites the record, and its hold ends with a lost-claim error; the release performs the same ownership check before removing, so a foreign or missing claim at the end also fails the work rather than trampling or quietly succeeding. Regression tests: "a holder whose claim is taken while it works" in test/scripts/stale-claim.test.ts proves the taker's record survives byte-for-byte, that touching stops at discovery, and that a taking no touch was awake to see is still caught at release.
Generated by Claude Code
| failingSince ??= Date.now(); | ||
| if (Date.now() - failingSince >= staleMs) wentUnfreshed = true; |
There was a problem hiding this comment.
Fresh evidence: this records the first failed touch, but other takers judge staleness from the timestamp written by the last successful touch. With the default 1s touch and 30s stale windows, if every touch starts failing at t=1s, another process may steal the claim at t=30s; this holder will not set or throw wentUnfreshed until t=31s, so a body that finishes in that one-second window reports success after duplicate work may already have started. Measure the outage from the last successful write, or compare the file's actual age, so the holder fails closed as soon as the claim can be stolen.
AGENTS.md reference: AGENTS.md:L85-L85
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48beeea, by going one step past the suggestion: the failure-span bookkeeping is gone entirely. What decides the outcome now is ownership, checked under the takers' guard — at every touch and once more at release. Your t=30–31s window closes because a steal at t=30 leaves the thief's name on the file, and the release's own ownership check at t=30.5 finds it and fails the work; and the converse false-alarm (file aged past the window but nobody actually took it) now rightly succeeds, since the release finds the claim still its own. Covered by "reports at release a taking no touch was awake to see" and "carries on when every touch fails but the claim stays its own" in test/scripts/stale-claim.test.ts.
Generated by Claude Code
| try { | ||
| return await body(); | ||
| } finally { | ||
| await stopTouching(); |
There was a problem hiding this comment.
When the supervisor is killed without cleanup and the child later exits, this finally only stops refreshing; keepClaimFresh deliberately leaves the supervisor's claim file in place, so the run record still looks running with a fresh claim until RUN_CLAIM_STALE_MS passes. In that window, if the recorded child PID is reused, --kill treats the orphaned run as live and can signal an unrelated process, while --clean also skips the dead run. Clear or stale the child-maintained claim when the child stops and no supervisor can release it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48beeea. The supervisor now passes its process id to the child (TICKETS_MUTATION_SUPERVISOR_PID), and when the child ends it checks whether that supervisor is still alive: if not, it ages the claim (rewriting it, still under the owner's name, with a time from the start of the clock) so the run reads as over immediately — --kill refuses it and --clean can sweep it, with no stale window for a reused pid to be signalled in. With the supervisor alive, the claim is left fresh and releasing stays the supervisor's job — aging it then would hand a concurrent clean a window to sweep the folder mid-copy-back, which is exactly the gap this PR closes. Regression tests: "ages the claim on exit when its supervisor is gone" and the updated "keeps the supervisor's claim fresh while it works" in test/scripts/mutation/snapshot-child.test.ts.
Generated by Claude Code
Every touch and the release itself now check, under the takers' guard, that the claim still names this holder. A holder paused past the stale window can no longer overwrite a new owner's record: the touch that finds somebody else's name stops touching and the hold ends loudly, leaving the new owner's record exactly as it was. The release fails the work whenever the claim is not its own to remove — which also makes the whole failed-touches bookkeeping unnecessary, since what matters is ownership at the end, not how the file aged along the way. A work failure of the body's own stays the reported story, with a lost claim noted alongside it. The mutation child now knows its supervisor's process id: when the child ends and the supervisor is gone, it ages the claim so the run reads as over at once, instead of staying live for a whole stale window in which --kill could trust a reused process id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/isolation-cleanup.ts`:
- Around line 27-33: Rename processBelongsToRun to a claim-based name that
reflects status, PID presence, and runClaimIsFresh rather than process liveness,
then update all callers including signalRun in isolation.ts and the cleanup
tests. Preserve the existing boolean logic and behavior.
In `@scripts/stale-claim.ts`:
- Around line 237-249: Replace the duplicated withFileLock(claimGuardPath(path),
...) wrapper in the claim-taking flow with the existing withClaimGuard helper,
preserving the current callback logic and return behavior. Then remove the
now-unused withFileLock import from the lock-file module import.
- Around line 80-86: Update readClaimRecord to validate stat.mtime before
calling getTime(), throwing a named error when mtime is null while preserving
existing missing-claim handling by continuing to use Deno.stat. Add a regression
test covering a claim with no readable time and null mtime, asserting the named
error.
In `@scripts/stripe-mock/install.ts`:
- Around line 29-35: Remove the first, longer JSDoc block immediately before
BinDirGuard, leaving only the short JSDoc comment that directly documents the
declaration.
In `@test/scripts/mutation/isolation-lock.test.ts`:
- Line 105: Replace the real-time pause in the withCopyBackLock concurrency test
with withVirtualBackoff-driven retry control. Assert that the second callback
has not entered while the lock is held, then release the lock and assert that it
enters afterward, preserving deterministic verification of the retry path.
In `@test/scripts/stale-claim.test.ts`:
- Around line 737-746: Remove the wall-clock-dependent lower-bound assertion on
tries in the expectClaimRefused test. Keep the existing tries upper-bound
assertion, which verifies retry pacing without requiring more than one attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f58e5e69-bba7-4036-b545-34aa9ae197b9
📒 Files selected for processing (28)
TODO.mdscripts/held-lock-process.tsscripts/lock-file.tsscripts/mutation/equivalent-mutants/scripts.txtscripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-lock.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/mutation/snapshot-child.tsscripts/process.tsscripts/stale-claim.tsscripts/stripe-mock.tsscripts/stripe-mock/install.tstest/scripts/held-lock-process.test.tstest/scripts/lock-file.test.tstest/scripts/mutation/isolation-cleanup/removing.test.tstest/scripts/mutation/isolation-cleanup/startup.test.tstest/scripts/mutation/isolation-helpers.tstest/scripts/mutation/isolation-lock.test.tstest/scripts/mutation/isolation-state/records.test.tstest/scripts/mutation/isolation/commands.test.tstest/scripts/mutation/isolation/copy-back.test.tstest/scripts/mutation/isolation/helpers.tstest/scripts/mutation/isolation/list-and-kill.test.tstest/scripts/mutation/isolation/snapshot-runs.test.tstest/scripts/mutation/snapshot-child.test.tstest/scripts/not-found.test.tstest/scripts/stale-claim.test.ts
💤 Files with no reviewable changes (4)
- scripts/held-lock-process.ts
- TODO.md
- test/scripts/mutation/isolation-state/records.test.ts
- test/scripts/held-lock-process.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7317d3e5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const record = { root: runRoot }; | ||
| const claim = await keepRunClaimFresh(record); | ||
| try { | ||
| return await body(); |
There was a problem hiding this comment.
Fresh evidence: the real snapshot child in scripts/mutation.ts still passes main to runSnapshotChild, and main ends every normal path with Deno.exit(...), so this finally is skipped instead of running stopTouching()/ageRunClaimToStale(). When the supervisor was killed and the child then exits normally, its last touch leaves the run looking live for the full stale window; during that window --kill/--clean can skip the dead run or signal a reused child PID. Make the child body return an exit code and let the wrapper call Deno.exit after the cleanup runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ede81f3 — main now returns its exit code instead of calling Deno.exit, and the entry point does Deno.exit(await runSnapshotChild(main)), so the child's finally (stop touching, age the claim when the supervisor is gone) always runs before the process ends. test/scripts/mutation/snapshot-child.test.ts covers the claim being aged on exit when the supervisor has walked away.
Generated by Claude Code
| (await runClaimIsFresh(record)) | ||
| ? null | ||
| : await removeRunPath(record, record.root); | ||
| }); | ||
| if (outcome !== null) return outcome; | ||
| // Nothing left to leave alone means another clear-up got there first, which | ||
| // is the outcome we wanted, not a run we skipped. | ||
| return (await statOrNull(record.root)) === null | ||
| ? { record, removed: true } | ||
| : null; | ||
| }; | ||
| : await removeRunPath(record, record.root), |
There was a problem hiding this comment.
When a mutation run started by the previous implementation is still active, its folder has a run.lock and a running record but no claim. This new branch treats the missing claim as stale and removes record.root, so starting any new mutation command from an updated checkout can delete the old child’s snapshot while it is still mutating. Gate no-claim running records through the old lock/grace path or leave them alone until they are definitely from the new claim format.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deliberately not handled: the repo's rule is "no internal compatibility layers" — we own every caller, so the old run.lock probing was deleted rather than kept alongside the claim. The crossover window is one developer switching branches mid-run; the worst outcome is a loudly-reported re-run of a local mutation pass, not data loss. Keeping the old probe alive would resurrect exactly the machinery this PR removes.
Generated by Claude Code
| staleMs: number, | ||
| ): Promise<boolean> => { | ||
| const age = await nullIfNotFound(claimAgeMs(path)); | ||
| return age !== null && age < staleMs; |
There was a problem hiding this comment.
When the wall clock is moved backward after a claim is touched and the owner then dies, claimAgeMs becomes negative; this check still treats that as fresh because any negative number is < staleMs. That leaves an abandoned mutation run or stripe-mock install claim unstealable until the clock catches up to the future timestamp, so --clean/startup cleanup can skip dead runs for much longer than the stale window and install waiters just time out. Require a non-negative age before calling the claim fresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ede81f3 — claimIsFresh now requires age >= 0, so a claim stamped in the future reads as nobody's and stays stealable. Regression test: "a claim stamped in the future is nobody's to trust" in test/scripts/stale-claim/freshness.test.ts, on a frozen clock so the boundary is exact.
Generated by Claude Code
| record.status === "running" && | ||
| record.pid !== undefined && | ||
| (await runClaimIsFresh(record)); |
There was a problem hiding this comment.
When a snapshot run has files to copy back, finishChild keeps the record in running status until after copy-back finishes, but the child process has already exited; this freshness check still makes --kill trust the old child PID solely because the supervisor claim is alive. If that PID is reused while copy-back is running, a user running --kill all can signal an unrelated process. Treat the supervisor-held finishing phase separately from a signalable child, or clear/update the PID before copy-back starts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ede81f3 — markChildEnded drops the pid from the run record the moment the child's status resolves, before the record is written back, so --kill can never signal a pid the child no longer owns. Tests: "drops the pid once the child has ended" in test/scripts/mutation/isolation-state/records.test.ts, plus the settled-record assertion in snapshot-runs.test.ts.
Generated by Claude Code
| @@ -0,0 +1,763 @@ | |||
| import { join } from "node:path"; | |||
There was a problem hiding this comment.
This change adds test/scripts/stale-claim.test.ts as a 763-line suite, well past the repo's ~400-line target for test files. The guideline calls out test file size specifically because large suites slow mutation-test mapping and should be split into narrower files when introduced or refactored.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ede81f3 — the 763-line file is now a folder, test/scripts/stale-claim/, split by concern: helpers.ts, taking, freshness, touching, losing, and releasing. Every file is comfortably under the ~400-line target.
Generated by Claude Code
The snapshot child's entry now returns its exit code instead of calling Deno.exit mid-run, so the child's clean-up — waiting out its last touch and aging an orphaned claim — really runs. A run's record drops the child's process id the moment the child ends, so --kill can never trust an id that may already be somebody else's while the copy-back finishes. A claim stamped in the future reads as walked away rather than unstealable, and a claim with no readable time on a filesystem that keeps no write times fails loudly instead of crashing bare. The claim suite is split into focused files under test/scripts/stale-claim/, its timing assertions wait for the looked-for state instead of sampling fixed pauses, and the taking path reuses the one guard helper. The run liveness check is renamed runIsOwned to say what it reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
The full-suite coverage gate flagged three gaps the split left behind: the refused-work closure that must never run, the patient wait's loud give-up, and keepClaimFresh's first touch finding the claim already taken. The refusal helper now proves nothing ran by the holder's record surviving, the wait takes an attempt count so its give-up is provable quickly, and a claim-read watcher lets a test slip a stand-in record under the keeper's first look. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
lint:ci refuses void in a union, so the claim-read watcher answers with a record or null. The retry-pacing test also drops its wall-clock lower bound: the upper bound alone proves the pause between tries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebeb422381
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path: string, | ||
| settings: StaleClaimSettings, | ||
| ): Promise<{ ownedBy: string; stopTouching: StopTouching }> => { | ||
| const { owner } = await readClaimRecord(path); |
There was a problem hiding this comment.
When a snapshot child starts while its supervisor's heartbeat is rewriting the claim, this unguarded read can observe the file after Deno.writeTextFile has truncated it but before the owner line has been written. readClaimRecord then returns no owner and the child throws names no owner, aborting a live mutation run even though the supervisor still owns the claim. Read the owner inside the same claim guard used by touches, or combine the owner read with the guarded first touch, so a partial refresh cannot make the child fail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae066a2 — keepClaimFresh now reads the owner and lands its first touch under one hold of the takers' guard, so it can never catch the heartbeat mid-write and misread a half-written record (the between-read-and-touch race is gone by construction, not just narrowed). Regression test: "waits at the takers' door before reading who to keep fresh for" in test/scripts/stale-claim/losing.test.ts holds the guard and proves no read happens until it is let go.
Generated by Claude Code
| const record = { root: runRoot }; | ||
| const claim = await keepRunClaimFresh(record); | ||
| try { | ||
| return await body(); |
There was a problem hiding this comment.
This cleanup only runs if body returns or throws, but the other real snapshot entry point still exits inside the body: scripts/audit-equivalent-mutants.ts calls Deno.exit(130) when its abort signal is set. When an orphaned audit child is signalled, that exit bypasses this finally, so it stops refreshing without aging the supervisor's claim and the dead run can look live until the stale window expires. Make the audit child return 130 like scripts/mutation.ts does, so runSnapshotChild can always stop touching and age the claim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae066a2 — the audit's abort path now returns 130 through runAudit (the .catch maps an aborted failure to a sentinel and the function returns the code), so runSnapshotChild's cleanup always runs before the wrapper's final Deno.exit, the same shape scripts/mutation.ts uses.
Generated by Claude Code
| // With the supervisor gone, nobody is left to release the claim; age | ||
| // it so the run reads as over the moment this child ends, instead of | ||
| // staying "live" for a whole stale window. | ||
| if (!processExists(supervisorPid)) { |
There was a problem hiding this comment.
Fresh evidence: the current check only proves that some process still has supervisorPid, not that it is the supervisor that can release this claim. If the supervisor was SIGKILLed and is still a zombie briefly, or its PID has already been reused, this returns true and the child skips ageRunClaimToStale; after the child exits, the run keeps a fresh running claim until it ages out, so --clean skips it and --kill can trust the old child PID. Use a parent-death signal/pipe or another non-reusable parent-liveness check before deciding not to stale the claim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae066a2 — the child now judges the supervisor gone by having been reparented (Deno.ppid !== supervisorPid) instead of probing the pid. A reused pid can never become this child's parent, and a dead-but-unreaped supervisor has already had its children reparented, so both false-alive cases are closed. The supervisor spawns the child directly (new Deno.Command in isolation.ts), so the parent relationship is exact. processExists lost its last caller and is deleted.
Generated by Claude Code
| await whileStillOwned(path, owner, () => | ||
| Deno.writeTextFile(path, `${owner}\n1`), | ||
| ); |
There was a problem hiding this comment.
When a supervisor is gone and another cleanup has already removed or replaced the claim after the child's last heartbeat, this call returns without reporting anything because ageClaimToStale discards whileStillOwned's false result. The orphaned child can then exit successfully even though its claim was lost and the snapshot may already have been taken or deleted; make this path throw when it cannot age the owner it was keeping fresh.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae066a2 — ageClaimToStale now throws the lost-claim error when the claim no longer names the owner being aged, so an orphaned child whose claim was taken or removed fails loudly instead of exiting clean. Regression tests: "refuses to age a claim that is no longer that owner's" and "refuses to age a claim that has already gone" in test/scripts/stale-claim/losing.test.ts.
Generated by Claude Code
A keeper now reads the owner and lands its first touch under one hold of the takers' guard, so it cannot catch the owner's heartbeat mid-write and misread a half-written record. Aging a claim for a gone owner throws when the claim is no longer that owner's, instead of ending quietly with the work possibly re-run elsewhere. The snapshot child tells a dead supervisor by having been reparented — a reused pid can never become its parent — and the audit's interrupt returns through the child's claim cleanup instead of exiting straight past it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
A mutation run showed nothing failed when the pid check tightened from zero to one, so a test now books pid 1 as a supervisor: a real process id, just never this child's parent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
A failed touch settles on its own time, so on a slow machine the next touch can be armed a step later than a fixed tick script expects. The test now keeps stepping the clock until a healed touch lands, which only a touch after the blip can make fresh in so small a window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/isolation.ts`:
- Line 76: Update the --kill flow around runIsOwned so claim freshness cannot
authorize Deno.kill for a reused PID; use a supervisor-mediated stop path or
another race-free process-identity check before terminating the process. Add a
regression test covering the interval between child exit and writeRunRecord,
ensuring a reused PID is not killed.
In `@test/scripts/mutation/snapshot-child.test.ts`:
- Around line 144-155: Update the test around “accepts the smallest real process
id as a supervisor” so its PID-1 validation cannot depend on the current
Deno.ppid. Separate the PID-1 acceptance check from the
supervisor-death/aged-claim assertion, or inject a parent PID distinct from
Deno.ppid for that assertion, while preserving coverage that PID 1 is accepted.
In `@test/scripts/stale-claim/losing.test.ts`:
- Around line 210-216: Strengthen the missing-claim test around keepClaimFresh
by asserting the specific error type thrown when the claim file is absent,
rather than using a matcherless rejects.toThrow() presence check. Ensure the
assertion distinguishes the missing-file error from the existing “no owner”
error covered by the preceding test.
In `@test/scripts/stale-claim/taking.test.ts`:
- Around line 85-94: Update the Deno.open stub in the “surfaces a claim file
that cannot be made at all” test to reject only when the requested path is the
claim path, while allowing the takers’ guard-file open to proceed. Follow the
path-targeting pattern used by countClaimReads and failClaimTouches in the
helpers, preserving the assertion for the claim-operation failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e61704c6-2ea6-4234-81e7-1b1a24904807
📒 Files selected for processing (20)
scripts/audit-equivalent-mutants.tsscripts/mutation.tsscripts/mutation/equivalent-mutants/scripts.txtscripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/mutation/snapshot-child.tsscripts/process.tsscripts/stale-claim.tsscripts/stripe-mock/install.tstest/scripts/mutation/isolation-state/records.test.tstest/scripts/mutation/isolation/snapshot-runs.test.tstest/scripts/mutation/snapshot-child.test.tstest/scripts/process.test.tstest/scripts/stale-claim/freshness.test.tstest/scripts/stale-claim/helpers.tstest/scripts/stale-claim/losing.test.tstest/scripts/stale-claim/releasing.test.tstest/scripts/stale-claim/taking.test.tstest/scripts/stale-claim/touching.test.ts
💤 Files with no reviewable changes (3)
- scripts/process.ts
- test/scripts/process.test.ts
- scripts/stripe-mock/install.ts
The pid-1 supervisor test now proves acceptance by the work running, with no dependence on what this process's own parent is. The missing claim test pins the not-found error apart from the no-owner one. The unmakeable-claim test breaks only the claim file's open, so the takers' guard beside it is not what fails. CodeRabbit's supervisor-mediated --kill suggestion is recorded in TODO.md as its own follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18035e67bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const status = await spawned.status; | ||
| record = markChildEnded(record); | ||
| await writeRunRecord(record); |
There was a problem hiding this comment.
Fresh evidence: the current code still leaves spawned.pid in run.json from the moment spawned.status resolves until this writeRunRecord lands. In that window the child has exited, so its pid may already be reusable, but --kill will still see the old running record plus the supervisor's fresh claim and can signal an unrelated process. Make the stop path supervisor-mediated, or otherwise make the record unsignalable before the child pid can be reused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixed in this PR, by design — this is the same few-millisecond window CodeRabbit raised (child exited, record write landing), and the full fix is the supervisor-mediated stop recorded in TODO.md ("Let --kill stop a run through its supervisor, not the child's pid", added in 18035e6) with a starting point noted. markChildEnded already shrinks the exposure to a single record write on a local dev command; making the record unsignalable any earlier means changing the record shape and the kill flow, which is that follow-up's job rather than this PR's locking unification.
Generated by Claude Code
| const stopChild = (): void => { | ||
| if (interrupted) forceStopChild(child); | ||
| interrupted = true; | ||
| killChildQuietly(child); |
There was a problem hiding this comment.
When a second SIGINT/SIGTERM arrives after the first one has set interrupted, this branch calls forceStopChild, which kills the child and calls Deno.exit(130) from inside the withRunClaim body. That skips withClaim's release path, so the supervisor leaves a freshly touched claim on a run whose child it just killed; for the stale window, startup/--clean skip the dead folder and --kill can still trust the old child pid if it is reused. Let this path unwind through the claim cleanup, or stale/remove the claim before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2eba463 — the forced stop now removes the supervisor's own claim before exiting, so the run reads as over at once instead of after a stale window. The removal is synchronous and safe: the claim has been held fresh since it was taken, so it can only be this supervisor's, and no other code runs mid-removal. The "escalates repeated interrupts" test now asserts the claim file is gone after the forced stop, and that the release (which the stubbed exit lets run in the test) finds it gone.
Generated by Claude Code
A second interrupt kills the child and exits at once, skipping the claim's release — so the dead run kept reading as live for a whole stale window, and --kill could still trust its old child pid. The forced stop now removes the supervisor's own claim before exiting, and the escalation test proves the claim is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eba463bbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // It is this supervisor's own — held fresh since it was taken, and no | ||
| // other code runs mid-removal — and without it the run reads as over at | ||
| // once instead of after a whole stale window. | ||
| Deno.removeSync(runClaimPath(record)); |
There was a problem hiding this comment.
Fresh evidence in the current fix is that this removes the claim outside the takers' guard while the snapshot child may be in keepRunClaimFresh touching the same file. If a repeated SIGINT/SIGTERM lands just after the child heartbeat has read the owner but before it opens the claim for its write, stopProcessNow only sends SIGKILL and this remove can land first; the in-flight child touch can then recreate a fresh claim after the supervisor exits. That leaves the dead run looking live for RUN_CLAIM_STALE_MS, so cleanup skips it and --kill can trust the stale child pid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6238d7 — the forced removal now happens under the takers' guard, the same one every child touch holds across its read-and-write. A touch already inside the guard finishes first and the removal waits it out (the child was just SIGKILLed, so the guard frees in moments); a touch not yet inside finds the claim gone and writes nothing. The escalation test now covers the guarded takedown.
Generated by Claude Code
|
|
||
| const { runMutationTesting } = await import("./mutation/runner.ts"); | ||
| const code = await runMutationTesting({ | ||
| return await runMutationTesting({ |
There was a problem hiding this comment.
This path now depends on runMutationTesting returning so runSnapshotChild can stop touching and, when the supervisor is gone, age the claim; however runMutationTesting still has process-exit paths (Deno's default SIGTERM during withTestHarness setup, and Deno.exit(130) on a repeated signal in mutate). When an orphaned child is killed in either window, it exits without snapshot-child.ts's finally, leaving the last fresh claim on a running record for 30 seconds; during that window --clean skips the dead run and --kill can signal a reused pid. Make those signal paths return/throw through the wrapper or age the claim before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not actionable as suggested — both remaining windows are the designed fallback, not gaps in it. The first signal already unwinds through every finally, including runSnapshotChild's claim cleanup; the second-signal Deno.exit(130) in the runner exists precisely for an unwind that has stalled, so routing it back through the unwind would defeat it, and aging the claim there would mean threading the isolation layer's claim context deep into the runner. A signal during harness setup is the same story as SIGKILL, which no code can intercept: the stale window exists exactly so any untidy death — however it happens — resolves itself in a bounded 30 seconds on a local tool. The child's aging is a tidy-exit acceleration, not the safety mechanism.
Generated by Claude Code
| export const runClaimIsFresh = ( | ||
| record: Pick<MutationRunRecord, "root">, | ||
| timeoutMs = 50, | ||
| ): Promise<boolean> => { | ||
| const path = runLockPath(record); | ||
| // No lock file means no run holding one. | ||
| if ((await statOrNull(path)) === null) return false; | ||
| const stopped = await lockProbeExitCode(path, timeoutMs); | ||
| if (stopped === LOCK_HELD_EXIT_CODE) return true; | ||
| if (stopped === LOCK_FREE_EXIT_CODE) return false; | ||
| throw new Error( | ||
| `Could not tell whether the lock at ${path} is held: the check stopped with code ${stopped}`, | ||
| ); | ||
| }; | ||
| ): Promise<boolean> => claimIsFresh(runClaimPath(record), RUN_CLAIM_STALE_MS); |
There was a problem hiding this comment.
When --list or --kill calls this while the supervisor or child is refreshing the claim, the read is not protected by the same guard as the write. A partial write such as owner\n1 parses as an ancient timestamp, so a live run can be shown as stale or skipped by --kill even though its next guarded touch would prove it is still owned. Read the claim under the takers' guard, or make the freshness read tolerate mid-write records the same way the guarded callers do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not actionable as suggested: every decision that destroys or writes anything already reads under the guard — removeRun judges freshness inside withRunClaimGuard, and every touch, steal, and release goes through whileStillOwned/tryTakeClaim, which hold it too. The unguarded reads are --list's display and --kill's permission check, and a torn read there fails safe: the run shows as not-live for one look, or the kill is refused — nothing is deleted, signalled, or overwritten, and the next read heals. Guarding those reads isn't possible uniformly anyway, because runClaimIsFresh is also called from inside the guard (in removeRun), where taking it again would deadlock; a separate guarded variant for two cosmetic call sites would add surface for no protective gain.
Generated by Claude Code
The snapshot child is its own process: its guarded touch could read the claim just before the supervisor's unguarded removal and write it back after, leaving a dead run reading as live for a stale window. The forced stop now removes the claim under the same guard the touches hold, then exits. The escalation test catches the expected release failure from the start, since it can land mid-watch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
A targeted mutation run found that dropping the markChildEnded record write survived: nothing watched the on-disk record between the child ending and the settled write, which is exactly the window --kill must not trust. A new test records every record write and checks the last running one carries no pid. The moved equivalent-mutant entry for the starting exit code is re-pointed at its new line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
An unpiped mutation run showed three more survivors. Two are provably equivalent and recorded with their proofs: keepFiles' interrupted arm hands back a number nobody reads, and the copy-back fallback list is never falsy. The third was real: skipping the copy-back shortcut made a run with nothing to keep queue on the shared lock, so the basic run test now checks the lock file is never even created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/isolation.ts`:
- Around line 126-128: Update the forced-exit cleanup in forceStopChild to
retain the acquired claim handle or owner token and release the claim through
the ownership-checked API instead of directly calling
Deno.removeSync(runClaimPath(record)). If ownership has been replaced, treat the
release as lost ownership and leave the current claim untouched; add a
regression test covering claim replacement before a second termination signal.
In `@test/scripts/mutation/isolation/snapshot-runs.test.ts`:
- Around line 383-385: Update the test around getStopChild so the required
listener is captured once and narrowed after the existing defined assertion (or
an equivalent early-return narrowing), then invoke that bound handler twice
without optional chaining. Preserve the test’s two-delivery behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5cfb39aa-6a7c-48f0-9b7b-3ca692878c6d
📒 Files selected for processing (7)
TODO.mdscripts/mutation/equivalent-mutants/scripts.txtscripts/mutation/isolation.tstest/scripts/mutation/isolation/snapshot-runs.test.tstest/scripts/mutation/snapshot-child.test.tstest/scripts/stale-claim/losing.test.tstest/scripts/stale-claim/taking.test.ts
Optional chaining made the two deliveries silently skippable; the listener is now narrowed after the assertion and called directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25302f2201
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The forced stop takes the claim down — under the takers' guard, | ||
| // so a child touch mid-write cannot put it back — and then exits: | ||
| // the run reads as over at once, not after a whole stale window. | ||
| await eventually( | ||
| async () => !(await pathExists(runClaimPath({ root: record.root }))), |
There was a problem hiding this comment.
This change grows test/scripts/mutation/isolation/snapshot-runs.test.ts from 366 to 427 lines, past the repo's ~400-line target for test files. The repository calls this out because broad test files slow mutation-test mapping, so the new repeated-interrupt/claim-cleanup coverage should be moved into a narrower companion file instead of pushing this suite over the limit.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd82baa — the six signal/interrupt tests now live in their own interrupts.test.ts (169 lines), and snapshot-runs.test.ts is back down to 283.
Generated by Claude Code
The suite had grown past the ~400-line target, so the six tests about signals and interrupts now live in their own interrupts.test.ts. Both files sit well under the limit and mutation mapping stays narrow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
2 participants
Three different parts of the codebase each had their own way of saying "whoever claimed this job walked away, so take it from them": the stripe-mock install lock, the mutation runner's run records, and the database migration lock. This change gives the two file-based ones a single shared mechanism, copying the shape of the migration lock — an owner written into the claim, a check-in time the holder keeps refreshing, and a rule that a claim untouched for long enough may be removed and taken over.
What changed
scripts/stale-claim.ts, holds the whole protocol. A claim is a file naming its owner and when the owner last checked in. The holder keeps touching it while it works — and every touch, and the final release, first checks under a small guard that the claim still names this holder. A holder that was paused so long its claim was taken over can never overwrite the new owner's record: it stops touching, and its work ends with a clear "the claim was lost" error instead of a quiet false success. A claim stamped with a time from the future — a clock put back — reads as nobody's, so a dead owner's claim never becomes unstealable.--listshows, what--killmay signal, what a clear-up may delete — is answered by that one claim, and clear-ups make their decision holding the same guard takers use, so a brand-new run can never be swept in the moment before its claim lands.--killcan never signal a process id the child no longer owns.Two long-standing gaps this closes
--killused to judge a run alive when its recorded process was up and somebody held its folder — but a clear-up deleting the folder held it too, so a process id that had since been handed to an unrelated program could be signalled. Now only a run's own supervisor and child touch its claim, so a dead run cannot be mistaken for a live one.What could be deleted because of this
The run lock, the child process that probed it, the child process that held locks on our behalf, the startup grace period, the process-liveness probe, and the touch-failure bookkeeping an earlier draft carried are all gone — ownership at the end is what matters, not how the file aged along the way.
Checks
deno task precommitpasses in full. The new module's tests live in their own folder (test/scripts/stale-claim/), driven by a fake clock rather than real waiting, and the changed files hold a 100% mutation score (one provably equivalent mutant recorded with its reason). Five review rounds — three from Codex, two from CodeRabbit — are each fixed with regression tests and answered in place; the one suggestion out of scope here (a supervisor-mediated--kill) is recorded in TODO.md with a starting point. The three TODO entries this closes are removed.🤖 Generated with Claude Code
https://claude.ai/code/session_01Gx6EygisQvxkVyDrnN211P
Summary by CodeRabbit
Improvements
Bug Fixes
Summary by CodeRabbit