Clear away mutation run copies, and give every lock one shared mechanism - #1957
Conversation
Each isolated mutation run copied the whole checkout into .mutation-runs/<id>/work and left it there, so the folder filled up with copies of the project. A run now deletes its copy as soon as it ends, and each new run first deletes whatever earlier, stopped runs left behind. The small run.json record stays, so --list, --kill and --clean still work the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 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 (6)
📝 WalkthroughWalkthroughMutation isolation now persists run records atomically, validates and tolerates incomplete state, uses path-aware locks, removes inactive runs and work snapshots, and centralizes filesystem helpers. The mutation runner performs startup and terminal cleanup with expanded lifecycle and concurrency tests. ChangesMutation isolation lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 8dd0812d8d
ℹ️ 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".
| args, | ||
| root = projectRoot, | ||
| ) => { | ||
| await removeInactiveRuns(root); |
There was a problem hiding this comment.
When two mutation commands start together, this unconditional cleanup can classify the other run's copying record as inactive before it acquires its lock, then recursively delete that run after it has acquired the lock and begun copying. The second command can therefore remove a live snapshot from under its child; cleanup must hold or atomically acquire each run's lock across both the activity decision and removal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2e8a9b9, though not by holding the lock. Deno 2.5.6 has no non-blocking tryLock, so taking each run's lock across the decision would mean waiting for a live run to finish — a clear-up step that can block for an hour is worse than the race.
Instead, copyingRunStillActive now applies the same startup grace that runningProcessStillExists already used: a copying record is treated as busy while it is younger than RUN_STARTUP_GRACE_MS. The gap you describe is the few milliseconds between writing the record and taking the lock, so a competing run in that gap is always inside the grace and is never removed. This also tightens the identical, pre-existing hazard in --clean.
Regression test: test/scripts/mutation/isolation/commands.test.ts now has to age its mutation-stale-copying record before --clean all will remove it, which fails without the grace.
Generated by Claude Code
| /** Clears out whatever earlier runs left behind, so nothing piles up. */ | ||
| const removeInactiveRuns = async (root: string): Promise<void> => { | ||
| const { removable } = await cleanableRuns(await readRunRecords(root)); | ||
| await Promise.all(removable.map(removeRun)); |
There was a problem hiding this comment.
When a later mutation starts after any completed or stale run, this maps removeRun, which recursively deletes record.root and therefore its run.json, rather than deleting only record.workRoot. Consequently --list loses the earlier status and --clean finished can no longer target it; automatic cleanup should remove the snapshot while reserving whole-root deletion for the explicit clean command.
AGENTS.md reference: AGENTS.md:L935-L939
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deliberate, and I'd like to keep it. A run's own record survives the run that made it — it is only cleared when a later run starts — so --list and --clean finished still work for the run you just did, which is when anyone looks. Keeping every record for ever is the thing that fills the folder we are trying to keep empty, and makes --list unreadable over time.
The bytes argument is already handled separately: the snapshot is deleted the moment a run ends, so what a later run clears out is only the leftover run.json (a few hundred bytes) plus the copy of any run that was killed before it could clean up after itself.
Generated by Claude Code
| /** Clears out whatever earlier runs left behind, so nothing piles up. */ | ||
| const removeInactiveRuns = async (root: string): Promise<void> => { | ||
| const { removable } = await cleanableRuns(await readRunRecords(root)); | ||
| await Promise.all(removable.map(removeRun)); |
There was a problem hiding this comment.
When deleting an old run fails because its folder is busy, read-only, or otherwise inaccessible, removeRun resolves to { removed: false }, so this Promise.all still succeeds and its results are discarded. The new run then proceeds without any warning while the checkout copy remains and continues consuming disk space; inspect these results and report or throw on failures instead of silently treating cleanup as complete.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 2e8a9b9. removeInactiveRuns now inspects each result and reports failures through the same reportRemoveFailure helper the explicit --clean uses (Failed to remove the earlier run <id>: <reason>). It reports rather than throws: a folder we cannot delete is not a reason to refuse to run the mutation the person actually asked for.
Regression test: "reports an earlier run it cannot clear out" in test/scripts/mutation/isolation/startup-cleanup.test.ts.
Generated by Claude Code
|
|
||
| /** Clears out whatever earlier runs left behind, so nothing piles up. */ | ||
| const removeInactiveRuns = async (root: string): Promise<void> => { | ||
| const { removable } = await cleanableRuns(await readRunRecords(root)); |
There was a problem hiding this comment.
When the supervisor is hard-killed during a non-atomic writeRunRecord call, the run can retain a full snapshot with a truncated run.json; readRunRecord then returns null, so this record-based sweep never considers or removes that directory on later starts. Scan run directories as well as successfully parsed records, while using their locks to avoid live runs, so the exact interrupted-run copies this feature targets cannot remain permanently.
AGENTS.md reference: AGENTS.md:L935-L938
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 2e8a9b9. The sweep now walks the run folders (runDirectoryNames) as well as the readable records. A folder with no readable record gets a stand-in record so it goes through exactly the same activity check as any other, using its folder change time in place of the record's timestamp: a folder changed within the startup grace is left alone (a run may be writing its record right now), an older one is cleared out.
Regression tests in test/scripts/mutation/isolation/startup-cleanup.test.ts cover the old broken folder being removed, the just-written one being left, and the two cases where the folder's change time cannot be read.
Generated by Claude Code
Review follow-ups on the clear-up that runs before each mutation run: - A run that is still copying now counts as busy while it is young, the same way a just-started run already did. Without that, a second run starting at the same moment could delete the first run's folder in the short gap before it takes its lock. - Folders whose run.json cannot be read — a run killed while writing it — are now cleared out too, instead of keeping their copy of the project for ever. A folder that changed moments ago is left alone in case a run is writing it right now. - A folder that cannot be deleted is now reported instead of passing in silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e8a9b9fc3
ℹ️ 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 { removable } = await cleanableRuns(await runsToSweep(root)); | ||
| const results = await Promise.all(removable.map(removeRun)); |
There was a problem hiding this comment.
When a snapshot copy takes longer than 30 seconds and another mutation starts during the supervisor-to-child lock handoff, cleanup can read the old copying record, see its grace period has expired, then have runLockIsHeld acquire the lock after the supervisor releases it but before the mutation child does. This marks the run removable, and this later unprotected deletion can remove the working copy after the real child acquires the lock. The fresh evidence is that the new grace check still separates the activity decision and removeRun instead of acquiring the run lock across both operations.
AGENTS.md reference: AGENTS.md:L929-L935
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This particular sequence can't happen. The record is marked running inside the lock — markRunning + writeRunRecord both run in the withMutationRunLock callback, before it returns and the lock is released (isolation.ts, the child = await withMutationRunLock(...) block). So by the time the supervisor-to-child handoff window opens, the record no longer says copying; it says running with a timestamp of moments ago, and runningProcessStillExists treats it as busy through the same grace. A slow copy doesn't change that: the supervisor holds the lock for the whole copy, however long it takes, and the grace clock for the running state starts when the copy ends.
So there is no window where a live run is both outside its grace and not holding its lock. Holding the lock across the decision would still mean blocking for the whole of another run, which is worse than what it protects against here.
Generated by Claude Code
| const removeRun = (record: MutationRunRecord): Promise<RemoveRunResult> => | ||
| removeRunPath(record, record.root); |
There was a problem hiding this comment.
When any later mutation starts after a completed or stale run, removeInactiveRuns calls this whole-root remover, so the earlier run's run.json is deleted along with its snapshot. The fresh evidence is that the follow-up still defines removeRun against record.root, while the documented behavior says every run keeps its small record; as a result --list loses history and --clean finished cannot select those runs. Automatic cleanup should remove record.workRoot and reserve whole-root removal for the explicit clean command.
AGENTS.md reference: AGENTS.md:L935-L939
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same point as the earlier thread, where I explained why whole-folder removal is deliberate: the record outlives the run that made it and is only cleared when a later run starts, and keeping every record for ever is what fills the folder we are trying to keep empty.
The documentation mismatch you spotted was real, though — AGENTS.md read as if a record is kept indefinitely. Fixed in 26139fb: it now says the record lives until the next run starts.
Generated by Claude Code
| const removeWorkSnapshot = async (record: MutationRunRecord): Promise<void> => { | ||
| const result = await removeRunPath(record, record.workRoot); | ||
| if (!result.removed) reportRemoveFailure("the snapshot of ", result); |
There was a problem hiding this comment.
When the completed run's snapshot is busy, read-only, or otherwise cannot be removed, this branch only prints an error and runMutationInSnapshot still returns the child's exit code, so a passing mutation command exits successfully while leaving the checkout copy behind. Propagate the cleanup failure or convert a successful child result to failure so automation cannot accept a run whose required cleanup did not happen.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not doing this one. The exit code of deno task mutation answers one question — did the tests kill every mutant — and precommit:mutation gates on exactly that. Turning a run that killed every mutant into a failure because a folder was busy would report a passing branch as failing, which is a worse outcome than the leftover folder.
The failure is not silent: it prints Failed to remove the snapshot of <id>: <reason>, and the next run's clear-up removes the folder and reports again if it still cannot. That is the recovery path, and it does not need the exit code to carry it.
Generated by Claude Code
The old wording read as if every run keeps its record for ever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26139fb0ef
ℹ️ 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 runsToSweep = async (root: string): Promise<MutationRunRecord[]> => { |
There was a problem hiding this comment.
This change grows isolation.ts from 356 to 437 lines, crossing the repository's ~400-line target for files being refactored. Extract the new startup/snapshot cleanup responsibilities into a focused module rather than leaving the supervisor over the documented limit.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — fixed in 26e0555. The "is this run still going, and delete it if not" cluster now lives in scripts/mutation/isolation-cleanup.ts (173 lines), used by both the clear-up before a run and the list/kill/clean commands. isolation.ts is back down to 294 lines.
Generated by Claude Code
| /** When a folder last changed, or 0 when that cannot be told. */ | ||
| const folderChangedAt = async (path: string): Promise<number> => { | ||
| const info = await Deno.stat(path).catch((error: unknown) => { | ||
| rethrowUnlessNotFound(error); |
There was a problem hiding this comment.
When Deno.stat rejects with an error other than NotFound, this new line rethrows it, but the added startup-cleanup tests exercise only successful stats, a null mtime, and NotFound. The repository's coverage gate therefore reports this line as uncovered and fails precommit; add a direct test that supplies a non-NotFound stat error and asserts that the command rejects.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The test is worth having and is in 26e0555 ("gives up when a run folder cannot be asked about at all" — a PermissionDenied stat, asserting the command rejects rather than deleting blind).
The coverage claim itself was wrong, though: deno task precommit passed on both 2e8a9b9 and 26139fb. The line runs on every unreadable folder — the NotFound test executes it — and the throw/return decision lives inside rethrowUnlessNotFound, which has its own tests. So the gate was never failing here.
Generated by Claude Code
isolation.ts had grown past the 400-line target. The "is this run still going, and delete it if not" work now lives in isolation-cleanup.ts, which both the clear-up before a run and the list/kill/clean commands use. Also adds a test for a folder that cannot be asked about at all: anything other than a missing folder stops the run instead of deleting blind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26e0555870
ℹ️ 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".
| updatedAt: withinStartupGrace(changedAt) | ||
| ? new Date().toISOString() | ||
| : new Date(0).toISOString(), |
There was a problem hiding this comment.
When a snapshot copy lasts over 30 seconds and another run reads run.json while the supervisor is rewriting it to running, readRunRecords drops the partial record and this fallback uses the run directory's old mtime instead of the freshly written status. During the supervisor-to-child lock handoff, the lock probe can then succeed before the child acquires the lock, classify the synthesized record as inactive, and delete the live snapshot. Fresh evidence beyond the earlier lock thread is this unreadable-record path, which discards the timestamp written before the supervisor releases its lock; write records atomically or reread the record before removing the run.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good spot — this one is real, and the atomic-write half of your suggestion is the fix. Done in e22d816: writeRunRecord now writes to run.json.writing and renames it over run.json, so a reader always sees one whole record, old or new. A live run can no longer be mistaken for an unreadable folder, and the mtime fallback is left for what it was meant for — a folder abandoned before any record was ever written.
Regression test: "keeps the last complete record readable while writing a new one" in test/scripts/mutation/isolation-state/records.test.ts holds the swap half way and reads the record, which returns the previous complete record instead of nothing.
Generated by Claude Code
The record was written straight into run.json, so another run reading it at that moment could catch it half written. It then fell back to the folder's change time, which is older than the record, and could decide a live run was finished. The new text now goes to a spare file that is swapped into place, so a reader always sees a whole record — the old one or the new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
…up-tests-v40mo5 # Conflicts: # test/scripts/mutation/isolation-state/records.test.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 22-47: Extract a runProcessIsUp(record) helper in
scripts/mutation/isolation-cleanup.ts covering the running status, defined PID,
and processExists checks, then use it in processBelongsToRun and
runningProcessStillExists while preserving their distinct lock-check behavior.
In scripts/mutation/isolation-cleanup.ts lines 52-58, remove the redundant
missing-PID guard and map directly through processBelongsToRun, which handles
that case.
- Around line 109-114: Update reportRemoveFailure so what is treated as a plain
noun rather than requiring callers to include trailing whitespace; add the
separator in the failure message template. Adjust all callers of
reportRemoveFailure to remove trailing spaces from values such as "the snapshot
of" and "the earlier run", while preserving the empty-string case.
- Around line 52-58: Remove the record.pid === undefined early return from the
records mapping in the live-set construction, and always delegate the ownership
check to processBelongsToRun(record), retaining the existing record.id/null
result behavior and final filtering.
In `@scripts/mutation/isolation.ts`:
- Line 177: In the block containing the terminal cleanup, replace the
conditional call guarded by isTerminalRunStatus with an unconditional
removeWorkSnapshot(record) call, since all reachable records already have
terminal statuses.
In `@test/scripts/mutation/isolation-state/records.test.ts`:
- Around line 74-103: Resolve the merge conflict affecting the test containing
keeps the last complete record readable while writing a new one by rebasing or
merging the branch with main. Preserve the existing atomic-swap test logic,
including the mid-write read of run.json and write to run.json.writing.
In `@test/scripts/mutation/isolation/commands.test.ts`:
- Around line 76-82: Centralize the shared long-ago timestamp by exporting a
LONG_AGO date constant from isolation-helpers.ts, then update both
commands.test.ts and startup-cleanup.test.ts to import and reuse it instead of
defining or inlining the literal. Preserve the existing stale-record behavior.
In `@test/scripts/mutation/isolation/helpers.ts`:
- Around line 72-76: Update failWorkRemoval so its Deno.remove stub rejects only
the run snapshot path, while allowing unrelated removals such as the
removeInactiveRuns startup sweep to proceed normally. Use the removal path
argument to identify and preserve the intended snapshot case, keeping the
existing “work is busy” error for that path.
In `@test/scripts/mutation/isolation/startup-cleanup.test.ts`:
- Around line 37-48: Update the comment above stubRunsFolderStat to accurately
state that it intercepts stats for paths containing “mutation-broken” and
delegates all other paths to the original Deno.stat implementation. Keep it
concise and focused on the stub’s non-obvious behavior.
- Around line 13-20: Remove the redundant withTempDir import from
`#test/scripts/mutation/isolation-helpers.ts` and retain the existing
withSharedTempDir import from `#test-utils/files.ts`, updating any references if
needed so both call sites use the shared helper consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 59343200-7422-4102-a7a3-8d333ae813ef
📒 Files selected for processing (9)
AGENTS.mdscripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tstest/scripts/mutation/isolation-state/records.test.tstest/scripts/mutation/isolation/commands.test.tstest/scripts/mutation/isolation/helpers.tstest/scripts/mutation/isolation/snapshot-runs.test.tstest/scripts/mutation/isolation/startup-cleanup.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95665aaed0
ℹ️ 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 { removable } = await cleanableRuns(await runsToSweep(root)); | ||
| const results = await Promise.all(removable.map(removeRun)); |
There was a problem hiding this comment.
When a second deno task mutation starts after the first child exits but before its supervisor finishes lines 160–163 in isolation.ts, the stored record still says running while processExists(pid) is already false, so this cleanup marks the run removable and can delete its root concurrently with the supervisor's terminal writeRunRecord. If deletion removes the pending record or its directory before the atomic rename, that write throws and the catch path changes an otherwise passing mutation run to exit code 1. Track the supervisor as active or otherwise synchronize cleanup through completion rather than treating child exit alone as a stale run.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac4504b. The supervisor now takes the run's lock while it writes that final record:
await withMutationRunLock(record.root, () => writeRunRecord(record));A clear-up elsewhere checks the lock before deciding a run is finished, so it can no longer take the folder in the window between the child exiting and the supervisor's last write. The lock is held for the length of one small write, so nothing waits meaningfully on it.
Generated by Claude Code
| const unreadable = (await runDirectoryNames(root)).filter( | ||
| (name) => !known.has(name), | ||
| ); |
There was a problem hiding this comment.
When .mutation-runs contains any directory without a readable run.json that was not created by this runner, runDirectoryNames includes it here and this filter classifies it as an unreadable run; once it is older than the grace period, startup recursively deletes the entire directory. Generated run IDs have a known mutation-... form, so validate that form or another ownership marker before passing an unknown directory to removeRun; otherwise starting a mutation command can erase unrelated files placed under this parent directory.
AGENTS.md reference: AGENTS.md:L937-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in ac4504b. The sweep now only considers folders whose name starts with the run-id prefix this runner uses (MUTATION_RUN_ID_PREFIX, the same constant createRunId builds ids from), so anything else placed under .mutation-runs is never touched.
Regression test: "leaves alone a folder this runner did not name" in test/scripts/mutation/isolation/startup-cleanup.test.ts puts an old, differently-named folder with a file in it under .mutation-runs and checks the file survives a run.
Generated by Claude Code
- Only folders this runner named are cleared out, so anything else put under .mutation-runs is left alone. - The last record of a run is written while holding its lock, so a clear-up elsewhere cannot take the folder mid-write. - One shared helper answers "does this run look alive", used by both the copying and running checks. - Failure messages join their words, instead of each caller having to remember a trailing space. - Tests share one "long ago" time and one temp folder helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4504b6e2
ℹ️ 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".
| /** Every run folder, whether or not it still holds a readable record. */ | ||
| export const runDirectoryNames = async ( |
There was a problem hiding this comment.
This commit refactors isolation-state.ts and adds another filesystem responsibility, but grows the already oversized file from 471 to 491 lines. Move the new run-directory and atomic-record I/O into a focused module so this refactor moves the file toward the repository's documented ~400-line limit instead of farther past it.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — fixed in c15d68a. isolation-state.ts is split into three: reading and writing a run's record moved to isolation-records.ts (81 lines), and the lock a run holds over its folder moved to isolation-lock.ts (64 lines). isolation-state.ts is now 364 lines, so every file in the set is under the target:
175 isolation-cleanup.ts
64 isolation-lock.ts
81 isolation-records.ts
364 isolation-state.ts
289 isolation.ts
Generated by Claude Code
| export const MUTATION_WORK_DIR = "work"; | ||
| export const MUTATION_RECORD_FILE = "run.json"; | ||
| export const MUTATION_RUN_ID_PREFIX = "mutation-"; | ||
| export const MUTATION_RECORD_PENDING_SUFFIX = ".writing"; |
There was a problem hiding this comment.
MUTATION_RECORD_PENDING_SUFFIX is exported even though a repo-wide search finds no caller outside this module; only writeRunRecord uses it. Remove the export modifier so this implementation detail does not become an unused public API that callers can start depending on.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in c15d68a. The suffix is now a private constant in isolation-records.ts, next to the only function that uses it.
Generated by Claude Code
isolation-state.ts had grown past the 400-line target. Reading and writing a run's record now lives in isolation-records.ts, and the lock a run holds over its folder lives in isolation-lock.ts. Every file in the mutation isolation set is now under 300 lines. The name of the half-written record file is no longer exported: only the writer uses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c15d68a11f
ℹ️ 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".
| looksAlive(record) && | ||
| (runStartedRecently(record) || (await runLockIsHeld(record))); |
There was a problem hiding this comment.
When copying takes over 30 seconds and another mutation starts during the supervisor-to-child lock handoff, this check can retain the old copying record, wait for the supervisor to release the lock, then report the lock as free even though the supervisor already atomically wrote a fresh running record and the child is about to acquire it. removeInactiveRuns subsequently deletes that live snapshot, so the child loses its source and test files. Fresh evidence beyond the earlier rebuttal is that cleanableRuns keeps the pre-probe record and never rereads it before removeRun; recheck the record after a successful lock probe before classifying the run as removable.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and my earlier rebuttal missed this: the record is read before the probe, so a run that moves from copying to running while we wait on the lock is judged on stale data. Fixed in 93306be — runIsActive re-reads the record from disk when the first look says inactive, and a run counts as busy if either the record we started from or the one on disk now says so.
Regression test: "keeps a run that came to life while the lock was probed" in test/scripts/mutation/isolation/commands.test.ts hands cleanableRuns an old copying record while the record on disk says running moments ago, and the run is not removable. It fails without the re-read.
Generated by Claude Code
Waiting on a run's lock takes time, and the run can move from copying to running while we wait. The record read before the wait is by then out of date, so a run that had just come to life could be judged finished and have its folder deleted under it. A run now counts as busy if either the record we started from, or the one on disk now, says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93306be88e
ℹ️ 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".
| looksAlive(record) && | ||
| (runStartedRecently(record) || (await runLockIsHeld(record))); |
There was a problem hiding this comment.
When a second mutation starts after the first child exits but before its supervisor completes the locked final write in isolation.ts, runProcessIsUp is false, so this && short-circuits and never checks the lock the supervisor now holds. The reread can still see running until the atomic rename, allowing removeInactiveRuns to delete the run root mid-write; writeRunRecord then fails and the catch changes an otherwise passing mutation run to exit code 1. Fresh evidence beyond the earlier thread is that the final lock is ignored specifically once the child PID disappears; treat that lock as active through supervisor settlement.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right again — the liveness check short-circuits on the gone child, so the lock the supervisor takes for its final write was never consulted. Fixed in 7a125db: a held lock now marks a run busy on its own, whatever its record says.
if (await looksActive(record)) return true;
// Whoever holds the folder's lock owns it, whatever its record says — the
// supervisor takes it again to write its last record once its child has gone.
if (await runLockIsHeld(record)) return true;Regression test: "keeps a finished run whose folder is still held" in test/scripts/mutation/isolation/commands.test.ts — a run whose record already reads passed is not removable while its lock is held, and is removable once it is released.
Generated by Claude Code
After a run's child ends, its supervisor takes the folder's lock again to write the last record. The check for a live process short-circuits on the gone child and never looked at that lock, so a clear-up could delete the folder mid-write and turn a passing run into a failing one. Whoever holds the lock now owns the folder, so that alone marks a run busy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a125db2f2
ℹ️ 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 { removable } = await cleanableRuns(await runsToSweep(root)); | ||
| const results = await Promise.all(removable.map(removeRun)); |
There was a problem hiding this comment.
When a second mutation starts after the first child exits but just before the first supervisor acquires its final lock in runMutationInSnapshot, runIsActive can finish all lock probes while the folder is unlocked and place the run in removable; the supervisor can then acquire the lock before this unprotected deletion begins. Deleting the folder during writeRunRecord can make the first, otherwise successful command exit with code 1 or lose its terminal record. Fresh evidence after the prior lock fix is that the unconditional probe is still released before the decision returned here and before removeRun, so it does not synchronize deletion with a supervisor that acquires the lock immediately afterward. Keep an ownership signal active through terminal settlement or atomically reserve the folder before deleting it.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and it is now fixed at the root rather than with another guard. In 7ac103d a run's folder is deleted while its lock is held, and the record is read again under that lock before anything goes:
export const removeRun = (record) =>
withRunLockIfFree(record, async () => {
const latest = (await freshRecord(record)) ?? record;
return activeByRecord(latest) ? null : await removeRunPath(record, record.root);
});withRunLockIfFree takes the lock only if it is free within a moment and gives up otherwise, so clearing up never queues behind a run that owns its folder for an hour. The mutation child holds that same lock for its whole run (scripts/mutation.ts), so holding it is proof no live run owns the folder — the check-then-delete gap you describe no longer exists, and the separate lock probes it relied on are gone.
Regression tests: "keeps a finished run whose folder is still held" and "keeps a run that came to life while the lock was probed" in commands.test.ts, plus "gives up rather than queue behind a run that holds its folder" in isolation-state/lock.test.ts.
Generated by Claude Code
| export const withinStartupGrace = ( | ||
| at: number, | ||
| now: Date = new Date(), | ||
| graceMs: number = RUN_STARTUP_GRACE_MS, | ||
| ): boolean => | ||
| Date.parse(record.updatedAt) > 0 && | ||
| now.getTime() - Date.parse(record.updatedAt) < graceMs; | ||
| ): boolean => at > 0 && now.getTime() - at < graceMs; |
There was a problem hiding this comment.
When the system clock is corrected backward after a supervisor writes a copying record, or an orphaned run directory has a future mtime, now.getTime() - at is negative and therefore always less than graceMs. A killed run with no lock is consequently treated as recently active on every later startup until the clock catches up, which can leave its full checkout copy occupying disk for an arbitrarily long time. Bound the elapsed time on both sides, or otherwise reject implausibly future timestamps instead of considering every future date recent.
AGENTS.md reference: AGENTS.md:L935-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 7ac103d. withinStartupGrace now requires the age to be zero or more as well as under the grace, so a time in the future reads as long ago rather than as "just now".
Regression test: "treats a run stamped in the future as not recent" in test/scripts/mutation/isolation-state/records.test.ts.
Generated by Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mutation/isolation.ts (1)
133-162: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftMake active-run ownership and deletion atomic.
The runner releases its lock immediately after spawning the child, while cleanup considers an old running record active only when that lock is held. A child that outlives startup grace can therefore have its active snapshot deleted. Separately, cleanup can delete after observing an unlocked folder even if its owner acquires the lock before removal.
scripts/mutation/isolation.ts#L133-L162: retain durable run ownership through child completion and terminal-record persistence.scripts/mutation/isolation-cleanup.ts#L53-L73: keep liveness tied to that durable ownership; do not trust PID existence alone.scripts/mutation/isolation-cleanup.ts#L194-L198: acquire the run lock, reread/revalidate state, then delete while holding that ownership.test/scripts/mutation/isolation/commands.test.ts#L141-L155: add a regression for an active child beyond startup grace during a cleanup sweep.🤖 Prompt for 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. In `@scripts/mutation/isolation.ts` around lines 133 - 162, Make active-run ownership atomic across the lifecycle: in scripts/mutation/isolation.ts lines 133-162, retain the run lock from child spawning through completion and terminal-record persistence; in scripts/mutation/isolation-cleanup.ts lines 53-73, determine liveness from that durable ownership rather than PID existence alone; in scripts/mutation/isolation-cleanup.ts lines 194-198, acquire the run lock, reread and revalidate the record, then delete while holding the lock; in test/scripts/mutation/isolation/commands.test.ts lines 141-155, add a regression covering an active child that outlives startup grace during cleanup.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@scripts/mutation/isolation.ts`:
- Around line 133-162: Make active-run ownership atomic across the lifecycle: in
scripts/mutation/isolation.ts lines 133-162, retain the run lock from child
spawning through completion and terminal-record persistence; in
scripts/mutation/isolation-cleanup.ts lines 53-73, determine liveness from that
durable ownership rather than PID existence alone; in
scripts/mutation/isolation-cleanup.ts lines 194-198, acquire the run lock,
reread and revalidate the record, then delete while holding the lock; in
test/scripts/mutation/isolation/commands.test.ts lines 141-155, add a regression
covering an active child that outlives startup grace during cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2141d875-4c1e-4677-9584-cf45e420267a
📒 Files selected for processing (14)
scripts/mutation.tsscripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-lock.tsscripts/mutation/isolation-records.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tstest/scripts/mutation/isolation-helpers.tstest/scripts/mutation/isolation-state/lock.test.tstest/scripts/mutation/isolation-state/records.test.tstest/scripts/mutation/isolation/commands.test.tstest/scripts/mutation/isolation/helpers.tstest/scripts/mutation/isolation/list-and-kill.test.tstest/scripts/mutation/isolation/snapshot-runs.test.tstest/scripts/mutation/isolation/startup-cleanup.test.ts
Clearing up looked at a run, decided it was over, and then deleted it. Between those two steps the run's owner could take the folder back, and the delete would pull it away mid-write. A folder is now deleted while its lock is held, and the record is read again under that lock before anything is removed. The lock is only taken if it is free within a moment, so clearing up never queues behind a run that holds its folder for an hour. A time in the future no longer counts as "just now", so a clock put back cannot leave a dead run's copy sitting there for ever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/scripts/mutation/isolation-state/records.test.ts (1)
85-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize before reading the old record.
writingis started but not confirmed to have reached the blockedDeno.rename, so this can pass before the swap is in progress. Signal entry in the stub and await that signal before Line 96. As per coding guidelines, “Maintain complete, deterministic test coverage.”🤖 Prompt for 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. In `@test/scripts/mutation/isolation-state/records.test.ts` around lines 85 - 101, Make the isolation-state test synchronize with the stubbed Deno.rename before reading the old record: add an entry signal that the rename stub resolves immediately upon invocation, then await that signal after starting writing and before readRunRecord. Keep held.promise blocking the actual rename and resolve held only after confirming the expected copying state.Source: Coding guidelines
🤖 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-lock.ts`:
- Around line 69-78: Replace the pending file.lock(true) race in
scripts/mutation/isolation-lock.ts around the lock acquisition flow with
non-blocking tryLock(true), ensuring the file is always released in a finally
block, including rejection paths. In
test/scripts/mutation/isolation-state/lock.test.ts lines 124-126, remove the
real pause and assert the next acquisition immediately.
---
Outside diff comments:
In `@test/scripts/mutation/isolation-state/records.test.ts`:
- Around line 85-101: Make the isolation-state test synchronize with the stubbed
Deno.rename before reading the old record: add an entry signal that the rename
stub resolves immediately upon invocation, then await that signal after starting
writing and before readRunRecord. Keep held.promise blocking the actual rename
and resolve held only after confirming the expected copying state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4015768-e0b5-495a-893b-0b28c93ed155
📒 Files selected for processing (7)
scripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-lock.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tstest/scripts/mutation/isolation-state/lock.test.tstest/scripts/mutation/isolation-state/records.test.tstest/scripts/mutation/isolation/commands.test.ts
The test read the record straight after starting the write, so it could pass without ever catching the swap half way — the moment it exists to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5543372760
ℹ️ 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".
| try { | ||
| return JSON.parse(await Deno.readTextFile(path)) as MutationRunRecord; | ||
| } catch { | ||
| return null; |
There was a problem hiding this comment.
When an old run.json cannot be read because of PermissionDenied or a transient I/O error, this broad catch returns null; startup cleanup then treats the run as an orphan and can recursively delete its folder. This conflicts with the nearby Deno.stat path, which correctly stops rather than deleting when the disk cannot be inspected. Reserve null for the intended missing/malformed-record cases and propagate other read failures.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in f9b37a0. readRunRecord now only answers null for a missing file or a half-written one; any other read failure throws, so a disk that cannot be read never reads as "no record here".
const text = await nullIfNotFound(Deno.readTextFile(path));
if (text === null) return null;
try { return JSON.parse(text) as MutationRunRecord; } catch { return null; }nullIfNotFound is a new shared helper in scripts/not-found.ts, used by this and the Deno.stat path you compared it against, so the two now behave the same by construction.
Generated by Claude Code
| const unreadable = (await runDirectoryNames(root)).filter( | ||
| (name) => !known.has(name) && name.startsWith(MUTATION_RUN_ID_PREFIX), | ||
| ); |
There was a problem hiding this comment.
Fresh evidence after the earlier thread is that the final guard only uses startsWith: an old directory such as .mutation-runs/mutation-backups still passes this filter despite not matching createRunId's timestamp-and-suffix format, and an absent or unreadable run.json causes startup to recursively delete it. Validate the complete generated ID format or a reliable ownership marker so cleanup remains limited to actual earlier runs.
AGENTS.md reference: AGENTS.md:L937-L940
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — mutation-backups is a good counter-example. Fixed in f9b37a0: the sweep now checks the whole shape of a name this runner makes, not just the prefix.
export const isRunId = (name: string): boolean =>
/^mutation-\d{8}T\d{6}Z-[0-9a-f]{8}$/.test(name);Regression test: "knows which folder names are its own runs" in test/scripts/mutation/isolation-state/records.test.ts checks a real createRunId() passes while mutation-backups and a wrong-suffix name do not. The startup-cleanup tests now use a properly shaped id for their broken folder, so they exercise the real filter.
Generated by Claude Code
Two ways a folder that is not a finished run could have been deleted: - A record that could not be read *at all* — a locked-down or failing disk — read as "no record", which made the run look abandoned. Only a missing or half-written record now reads that way; anything else stops the run. - The name check only looked for the "mutation-" prefix, so a folder someone else named, like mutation-backups, counted as ours. It now has to match the full shape of a name this runner makes. Both share one small helper, `nullIfNotFound`, for "not there" versus "could not look". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d51572ec2
ℹ️ 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".
| if (!(await waitForLock(file))) { | ||
| // Closing hands back anything the abandoned wait is later granted. | ||
| file.close(); | ||
| return null; |
There was a problem hiding this comment.
When --clean targets an active run, the timer can win this race, but the file.lock(true) operation remains pending and ref'ed; closing the FsFile does not cancel the in-flight wait because the operation retains the underlying file until it completes. A standalone cleanup command can therefore stay alive until the active mutation releases its lock, despite returning null after 250 ms internally. Use a cancellable/subprocess probe or explicitly unref the abandoned operation so the timeout actually lets the process finish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You were right, and it was worse than "may": I measured it before changing anything. A wait that gave up after 300ms took 9.3 seconds to return — the process stayed alive until the other holder released, so --clean against a live run printed its answer and then sat there.
Fixed across 3e1e541, fb9b7a8, 69f6206 and 3575635. A child process does the waiting now, so giving up really is giving up: it takes the lock, says which file it got (so the "is this still the file at the path?" check survives the move), and holds it until its input closes — when we let go, or if we die first. The same give-up now returns in 0.4s.
Two details, both of which I got wrong first time round:
- The child carries its own deadline, counted from when it starts waiting. Charging process start-up to the wait made a free lock look held on a busy machine — it failed exactly that way in the grouped test suite while passing when run alone.
- Because the child ends itself, this side needs no timer, no signal to stop it, and no branch for a child that had already stopped.
Regression tests: test/scripts/held-lock-process.test.ts (five, at the module's mirror path) plus the give-up cases in test/scripts/lock-file.test.ts.
Generated by Claude Code
| .filter(({ outcome }) => outcome?.removed === true) | ||
| .map(({ record }) => record), | ||
| skipped: results | ||
| .filter(({ outcome }) => outcome === null || outcome === undefined) |
There was a problem hiding this comment.
When two --clean commands select the same finished run and the second has already opened run.lock, the first can delete the folder before releasing the lock; the second then acquires the unlinked inode, fails heldLockIsAtPath, and returns null here as though the run were active. removeMatchedRuns consequently prints both “Skipped active” and “No cleanable” and exits 1 even though the requested cleanup succeeded. Fresh evidence beyond the earlier missing-before-open case is this already-open, queued waiter; recheck whether the root disappeared before classifying a null outcome as skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 3e1e541. A null from the lock now gets one more look: if the run's folder is no longer there, another clear-up got there first, which is the outcome we wanted, so it counts as removed rather than "skipped active". A folder that is still there is a genuine skip, as before.
Generated by Claude Code
Waiting for a lock could not be called off. The wait keeps the process alive until the lock arrives however long that takes, and closing the file does not stop it, so `deno task mutation --clean` against a run that was still going printed its answer and then sat there until that run finished. Measured: a wait that gave up after 300ms took 9.3 seconds to exit; it now takes 0.4. The waiting is done by a child process instead, which can be stopped. It takes the lock, says which file it got so we can still check it is the file at the path, and holds it until its input closes — when we let go, or if we die first. Also: a clear-up that lost the race to another clear-up called the run "skipped active" and exited 1, though the run it wanted gone was gone. A folder that is no longer there now counts as removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
The wait for a lock is done by a child process, and its start-up was counted against the time we were prepared to wait. On a busy machine — the grouped test runner, or CI — starting the child could use the whole quarter second, so a lock nobody held looked held and a run that should have been cleared away was skipped instead. It failed that way in the grouped suite while passing on its own. The child now says it is about to wait, and the clock starts there. Also clears the timer it leaves behind, which the leak check catches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
The child waiting for the lock now gives itself the time we would have given it, counted from when it starts waiting, and ends itself if that runs out. So this side needs no timer, no signal to stop it, and no branch for a child that already stopped — all of which were code no test could reach. The give-up path had its own copy of "is this still the file at the path?". Both paths now share one, so both cover it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
Once the timed wait moved to a child process, the helper had a single caller and the same shape as it, which jscpd rightly called a duplicate. Taking the lock is now one loop in one place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
That arm used to be reached by chance through the deleted-folder test, but the child keeping its own deadline makes that test finish earlier, so it needs a test of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
…up-tests-v40mo5 # Conflicts: # TODO.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 482a9a3ac9
ℹ️ 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".
| if (line === null) { | ||
| await closeDown(child); | ||
| return null; |
There was a problem hiding this comment.
When the helper child fails before reporting held—for example, because Deno.open or file.lock gets PermissionDenied or an I/O error—heldLine returns null and this branch treats every child exit as an ordinary timeout. Through withFileLockOrNull, explicit mutation cleanup then reports the run as active and startup cleanup silently leaves its checkout behind instead of exposing the filesystem failure. Inspect the child's exit status and reserve null for the documented timeout or missing-folder outcomes; propagate other failures.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — a swallowed failure, and the worst kind, since the run would be called active and its copy left behind without a word. Fixed in c002fed.
The child now ends quietly only when its own time runs out, so any other ending means the lock could not be taken at all, and that throws with the child's own error text attached.
Writing the test caught a distinction worth naming: the folder going while the child is starting also kills the child, but that is the documented "nobody's lock", not a failure — so it throws only when the folder is still there. My own deleted-folder test failed until I got that right. New test: "says so loudly when the lock cannot be taken at all", which puts a folder where the lock file should be.
Generated by Claude Code
The child doing the waiting ends quietly when its own time runs out, so any other ending means the lock could not be taken at all — a folder we may not read, a disk that will not answer. That was being read as "somebody is holding it", so a clear-up called the run active and left its copy behind without a word. It now says what went wrong, in the child's own words. A folder that goes while the child is starting still counts as nobody's lock, which is what the deleted-folder test was there to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e99430166e
ℹ️ 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 removeRun = async ( | ||
| record: MutationRunRecord, | ||
| ): Promise<RemoveRunResult | null> => { | ||
| const outcome = await withRunLockOrNull(record, async () => { |
There was a problem hiding this comment.
When an old running record's PID has been reused and cleanup is recursively deleting its snapshot under this lock, a concurrent --kill sees both processExists(record.pid) and runLockIsHeld(record) as true. processBelongsToRun therefore treats the cleanup helper's lock as proof that the unrelated process owns this run and sends that reused PID SIGTERM/SIGKILL; deletion of a large checkout makes this race window substantial. Tie the lock evidence to the recorded child, or make cleanup ownership distinguishable from run ownership.
AGENTS.md reference: AGENTS.md:L938-L946
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and I've recorded it in TODO.md rather than fixing it here — "Tell a clear-up's hold on a run apart from the run's own".
The reasoning: it needs a process id that has since been given to something else and a clear-up deleting that same run at that moment, and the fix is not a patch — the lock evidence has to be tied to the run's own child, so either the holder writes down who it is, or a clear-up holds something a run never holds. That changes what holding a run's lock means, which is a poor thing to bolt onto a PR this size at this point. The entry carries the failure, why it is out of scope, and the three call sites to start from.
Generated by Claude Code
| const results = await Promise.all( | ||
| records.map(async (record) => ({ | ||
| outcome: await removeRun(record), | ||
| record, | ||
| })), |
There was a problem hiding this comment.
When the first mutation after this upgrade encounters the large backlog this change is meant to remove, this Promise.all starts one removeRun per folder at once, and each one launches a separate Deno lock-holder process through withRunLockOrNull. Hundreds of retained snapshots can therefore create hundreds of Deno processes simultaneously, exhausting memory, file descriptors, or the process limit and aborting startup before the backlog is cleared. Process the records sequentially or with a small concurrency limit.
AGENTS.md reference: AGENTS.md:L935-L941
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it lands right where it hurts: the first run after this change is exactly the one that meets the backlog the change exists to clear. Fixed in 469e343 — eight at a time now, through the repo's chunk helper, so a folder full of old runs cannot start hundreds of lock-holder processes at once and stop the run that came to tidy them up.
Generated by Claude Code
Every run being cleared away waits for its folder's lock in a process of its own, and they were all started at once. The first run after a long time without one meets exactly the backlog this change exists to clear — hundreds of folders, so hundreds of processes, enough to run a machine out of them and stop the run that was going to tidy them up. Eight at a time now. TODO.md records the one thing left here: a clear-up holding a run's lock looks the same as the run holding it, which matters only when a process id has since been given to something else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469e343899
ℹ️ 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 file = await openLockFileOrNull(path); | ||
| if (file === null) return false; | ||
| file.close(); | ||
| return (await lockProbeExitCode(path, timeoutMs)) === LOCK_HELD_EXIT_CODE; |
There was a problem hiding this comment.
When the initial open succeeds but the probe subprocess then hits PermissionDenied or another I/O error while opening or locking run.lock, LOCK_PROBE_SCRIPT exits with code 2 and this comparison reports the lock as free. As a result, --list can label a live run stale and --kill can refuse to signal it instead of exposing the filesystem failure. Fresh evidence in 587f003 is that the probe still maps both open and lock failures to exit 2 despite the earlier thread stating that this was fixed; distinguish the expected missing-lock result and propagate other probe failures.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and right to point at the earlier thread: that reply overstated it. I fixed the parent's open and left the probe mapping "could not open" and "could not lock" to the same ending as "free". Fixed properly in 732549e.
The check now has three endings — held, free, and could-not-tell — and the last one throws naming the path and how the check ended, instead of quietly answering "nobody".
While there: it no longer opens the lock file just to see whether one exists. Asking a question should not make the thing it asks about, and the file's presence is what the question was really about.
Two tests cover the loud paths (a folder that cannot be read, and a folder sitting where the lock file should be). A third came out of the coverage gate catching a gap I had just made: answering "not held" straight from the disk left nothing reaching the probe's own "free" ending, so a run that takes its lock and lets it go now checks that too.
Generated by Claude Code
| file.close(); | ||
| `; | ||
|
|
||
| export interface HeldLock { |
There was a problem hiding this comment.
A repo-wide search at this commit finds HeldLock referenced only in this module as holdLockOrNull's return contract; no production caller imports the interface itself. Remove its export modifier so this implementation detail does not become another unused public API that internal callers can start depending on.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 732549e — HeldLock is no longer exported. openLockFileOrNull went the same way in the same commit: the lock check stopped needing it, so it is now private to scripts/lock-file.ts.
Generated by Claude Code
The check that asks whether another run holds a lock ran in a child process, and that child ended the same way whether the lock was free or it could not read the file at all. So a folder we may not read made `--list` call a live run stale and `--kill` refuse to signal it. Free, held, and could-not-tell are now three different endings, and the last one says the path and how the check ended. It no longer opens the lock file just to see whether one is there — asking a question should not make the thing it asks about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
Answering "not held" straight from the disk when there is no lock file left nothing reaching the check's own "nobody is holding it" ending, so it could have started saying anything and no test would have minded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
TODO.md (2)
1690-1720: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the “every other lock” claim.
This section explicitly says
src/shared/db/migrations/lock.tsuses a separate owner/TTL protocol, so “same shape as every other lock” and “Every lock in the repo ... goes throughwithFileLock” are inaccurate. Limit the claim to filesystem locks or name the migration lock as an exception.🤖 Prompt for 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. In `@TODO.md` around lines 1690 - 1720, Revise the TODO section heading and opening claim to qualify “every other lock” as filesystem locks using withFileLock. Explicitly identify the database migration lock in src/shared/db/migrations/lock.ts as an exception, while preserving the explanation of the stripe-mock install lock’s separate protocol.
1666-1689: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this TODO focused on the remaining contract.
This addition mixes the desired behavior with PR provenance, historical behavior, implementation narration, and fixture migration details. Keep only the invariant, acceptance criteria, and concise starting point.
🤖 Prompt for 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. In `@TODO.md` around lines 1666 - 1689, Condense the TODO to the remaining contract: mutation commands must ignore folders whose names are not valid run IDs, including when reading records for explicit list, kill, and clean operations. Retain a concise starting point directing updates to readRunRecords to reuse isRunId filtering like runsToSweep, plus the affected mutation isolation fixtures; remove PR provenance, historical context, implementation narration, and migration details.Source: Coding guidelines
🤖 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/held-lock-process.ts`:
- Around line 40-52: Update heldLine to use the existing `@std/streams`
TextLineStream for line-oriented parsing, and return only a complete line
beginning with “held”; do not match an unterminated partial line. Preserve the
null result when the reader ends before a valid held line is received.
In `@scripts/mutation/isolation-cleanup.ts`:
- Line 126: Remove the narrative documentation comment immediately above
removeFinishedRuns, leaving the function and its behavior unchanged.
In `@test/scripts/held-lock-process.test.ts`:
- Around line 35-37: Remove the real-time setTimeout sleep using
LONG_ENOUGH_TO_BE_LET_IN_MS in the held-lock test, and replace it with a
deterministic lock-attempt signal or controlled/virtual backoff mechanism.
Preserve the test’s assertion that the lock remains held without relying on
scheduler timing.
- Around line 16-21: Make each held-lock setup in the test strict: assert the
result of holdLockOrNull is non-null, return early only afterward if needed for
nullable narrowing, and avoid optional chaining when accessing or releasing the
holder. Wrap each holder’s assertions and test actions in try/finally so letGo
always runs, including when an assertion fails; apply this consistently to the
referenced holder blocks.
---
Outside diff comments:
In `@TODO.md`:
- Around line 1690-1720: Revise the TODO section heading and opening claim to
qualify “every other lock” as filesystem locks using withFileLock. Explicitly
identify the database migration lock in src/shared/db/migrations/lock.ts as an
exception, while preserving the explanation of the stripe-mock install lock’s
separate protocol.
- Around line 1666-1689: Condense the TODO to the remaining contract: mutation
commands must ignore folders whose names are not valid run IDs, including when
reading records for explicit list, kill, and clean operations. Retain a concise
starting point directing updates to readRunRecords to reuse isRunId filtering
like runsToSweep, plus the affected mutation isolation fixtures; remove PR
provenance, historical context, implementation narration, and migration details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 939dda6b-40a8-4910-963f-35bc432303d2
📒 Files selected for processing (8)
TODO.mdscripts/held-lock-process.tsscripts/lock-file.tsscripts/mutation/isolation-cleanup.tsscripts/mutation/isolation-lock.tstest/scripts/held-lock-process.test.tstest/scripts/lock-file.test.tstest/scripts/mutation/isolation-lock.test.ts
"held" and the file number it carries can arrive in two pieces. Half a line taken for the whole of one lost the number, and a lock nobody could name reads as a lock on a file nothing points at — so a run holding its folder perfectly well would have been called swept. It now waits for the newline. Also: the test helper takes a lock that must be there and lets it go however the work inside ends, so a failed assertion cannot leave a holder behind; and TODO.md no longer claims every lock in the repo is a file, since the database migration lock is a row in a table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
The test helper needed a name for what a taken lock is, and importing the interface would have made it public again. It reads the type off the function that hands one back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28c53c32ec
ℹ️ 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 file = await Deno.open(path, { read: true, write: true }).catch(() => null); | ||
| if (file === null) { | ||
| clearTimeout(timeout); | ||
| Deno.exit(2); |
There was a problem hiding this comment.
When a concurrent cleanup removes the run folder after the parent’s statOrNull check but before this subprocess opens run.lock, Deno.open gets NotFound, this branch returns exit code 2, and runLockIsHeld throws. In the inspected listRuns and signalRun paths, a normal concurrent cleanup can therefore make --list or --kill abort instead of reporting that the run is no longer active. Fresh evidence after the prior probe-error fix is that the child still gives an expected missing path the same exit code as permission and I/O failures; give NotFound its own free/missing result while continuing to propagate other failures.
AGENTS.md reference: AGENTS.md:L147-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right again — fixed in 2aed858. The probe now gives a missing lock file the same answer as a free one, and keeps its own exit code for permission and I/O failures, which still stop the command.
This is the third time round the same distinction on this path — the parent's open, then the probe's failures, now the probe's expected absence — and the rule that settles all three is the same: an absence somebody could reasonably meet is an answer, anything else is worth raising. A clear-up taking the folder away mid-question is squarely the first kind.
Regression test: "reports a lock that goes while being looked at as not held", driven by a stat that claims the file is there when it is not, which is exactly what losing that race looks like from the parent's side.
Generated by Claude Code
The check that asks whether another run holds a lock looks at the file in a child process, and a clear-up can take the folder away between being asked and being looked at. That is the run no longer being anybody's, but it came back as "the check could not answer", so `--list` and `--kill` would stop rather than say the run is over. A file that is not there now answers the same as a lock nobody holds. A folder we may not read, or a disk that will not answer, still stops the command. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
Two tests told the same kind of lie about what is on disk. They now share a helper that answers the paths it is given and passes the rest to the real one, so each test says only the lie it needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1db7849ab
ℹ️ 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 folderWent = (await statOrNull(dirname(path))) === null; | ||
| if (!(stopped.success || folderWent)) { |
There was a problem hiding this comment.
When another cleanup removes the run directory after the parent check but before the helper's Deno.open, the child exits with NotFound; if the run owner recreates that directory before this later statOrNull, folderWent is false and an expected concurrent removal is misreported as a lock failure. In the mutation startup path, that rejection escapes removeInactiveRuns and prevents the new run from starting. The fresh, separate path from the fixed lock probe is holdLockOrNull's post-exit directory check; have the child report NotFound distinctly instead of inferring it from mutable state afterward.
AGENTS.md reference: AGENTS.md:L937-L941
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and the reasoning is the part worth keeping: I was deciding whether the folder had gone by looking at the disk after the child had exited — a fact read from state that can change underneath it. The owner remaking the folder in that gap turns an ordinary concurrent clear-up into "the lock could not be taken", and on the startup path that rejection stops the next run from starting at all.
Fixed in e0d6567. The child reports a missing folder with an ending of its own rather than dying of it, so the answer comes from the moment it happened. Anything else it dies of still throws, with its own error text attached.
Second time tonight this same shape has bitten on this path, so to say it plainly: when something transient has to be told apart, the party that saw it should say so, instead of somebody re-deriving it later.
Generated by Claude Code
The folder can go while the child is starting, which is nobody's lock and not a failure. That was worked out afterwards by looking at the disk again — but the run's owner can make the folder again in between, and then an ordinary clear-up came back as "the lock could not be taken", which stops the next run from starting at all. The child says so with an ending of its own now, so the answer comes from the moment it happened instead of from state that can change underneath us. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XTNZkFGEL7MjARJrbQqCu
…up-tests-v40mo5 # Conflicts: # TODO.md
2 participants
Every mutation run makes its own copy of the whole project in
.mutation-runs/<run>/work, so it can change files without touching the realones. Those copies were never removed, so the folder grew until it took up a lot
of disk space.
Clearing up after a run
was stopped.
going — including a folder left by a run that was killed before it could tidy
up after itself, whose record may be unreadable. Only folders this runner
named are touched.
silence.
starting at the same moment cannot delete each other's work.
written while holding the run's lock, so another run always sees a whole
record and can never take a folder mid-write.
--list,--kill, and--cleanwork as before.The "is this run still going, and delete it if not" code moved into its own
file,
scripts/mutation/isolation-cleanup.ts, which both the clear-up and thelist/kill/clean commands use.
One way of locking, for everything that locks
Deleting folders while other runs may be using them turned up a problem that
was never about mutation runs at all.
A lock is a file. If anything deletes that file while somebody is waiting for
it, the waiter is handed a file that nothing points at any more — it holds a
lock that keeps nobody out, and two jobs can then do the same work at once.
So the mutation runner's two hard-won rules now live in the one place every
lock in the repo already went through,
scripts/lock-file.ts:whatever now sits at its path. If they are not the same file, make the folder
and take the lock again.
hour behind one of them, and must not bring back a folder somebody removed.
That is a second way of taking a lock, which waits only as long as it is
given and otherwise says "not mine".
The precommit gate, the browser-asset build, the stripe-mock start and every
mutation run all use these, so they are all safe from the deleted-lock problem
now, not just the one that found it.
scripts/mutation/isolation-lock.tsis left with the mutation words: where arun's lock lives, and asking whether another run is holding one.
Other temporary folders
I checked every temporary folder the tests make. They all remove themselves when
the test ends, and a full test run leaves nothing behind.
Left for another day
TODO.mdrecords the one lock still doing its own thing: stripe-mock's installlock, which can break a claim whose owner walked away. Two other places answer
that same question their own way, so it is worth folding into one — but it is a
whole protocol rather than a shared helper, and did not belong in this change.
Summary by CodeRabbit