Fix job records lost on concurrent background task launches - #689
Fix job records lost on concurrent background task launches#689rajasekar-venkatesan wants to merge 9 commits into
Conversation
updateState() did a lockless read-modify-write of state.json: concurrent `codex-companion.mjs task --background` launches each read the same base state, appended only their own job, and clobbered siblings on write. saveState()'s prune then deleted the orphaned siblings' job files, so most concurrently launched agents vanished from /codex:status and /codex:result. Serialize the read-modify-write with a cross-process O_EXCL lockfile (withStateLock, with jittered backoff and stale-lock stealing) and make the state write atomic (temp file + rename) so lockless readers never observe a torn file. Stress test (12 concurrent upsertJob processes): 2/12 tracked before, 12/12 after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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: c76e9ec46e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Date.now() - fs.statSync(lockFile).mtimeMs > 10000) { | ||
| fs.unlinkSync(lockFile); |
There was a problem hiding this comment.
When a crashed process leaves a stale lock and multiple background launches arrive together, two contenders can both stat the old inode; after one unlinks it and acquires a fresh lock, the other can execute this unlinkSync against that fresh lock, acquire its own lock, and enter the read-modify-write concurrently. The first contender's unconditional cleanup can then remove the second lock as well, allowing further writers in and recreating the lost-job corruption this change is intended to prevent. Stale-lock removal needs an ownership-preserving atomic takeover strategy rather than deleting the shared pathname after a separate stat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in af579cf.
The stat-then-unlinkSync(lockFile) reclaim was not atomic, so two racers could both reclaim the same stale lock and one could unlink the other's freshly-acquired lock, letting two writers into the read-modify-write.
The reclaim is now atomic via rename(), which has exactly-one-winner semantics: concurrent reclaimers target the same inode, only one renameSync succeeds, and the losers get ENOENT and fall back to re-contending on open(O_EXCL). The lock pathname is never unlinked directly during reclaim. On release, each acquisition stamps a unique owner id into the lock and only unlinks it if the contents still match, so a process can never delete a lock another process now holds.
Added a regression test that seeds a stale lock (old mtime) and fires 10 concurrent writers, asserting no job records are dropped and the lock is released. Full suite: 93/93.
Addresses review feedback: the previous stale-lock cleanup did a stat-then-unlink on the lock pathname, which is not atomic. Two racing launches could both observe the same stale lock; after one unlinked it and acquired a fresh lock, the other's unconditional unlink could remove that fresh lock and let a second writer into the read-modify-write concurrently, recreating the lost-job corruption this change prevents. Reclaim the stale lock with rename() instead, which has exactly-one-winner semantics: concurrent reclaimers target the same inode and only one succeeds; losers get ENOENT and fall back to re-contending on open(O_EXCL). Stamp a unique owner id into the lock and only unlink it on release if the contents still match, so a process can never delete a lock another process now holds. Add a regression test that seeds a stale lock and fires concurrent writers, asserting no job records are dropped and the lock is released.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af579cf6db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| if (Date.now() - fs.statSync(lockFile).mtimeMs > 10000) { | ||
| const scratch = `${lockFile}.stale.${ownerId}`; | ||
| fs.renameSync(lockFile, scratch); // only one racer wins this |
There was a problem hiding this comment.
When two reclaimers have both observed the stale lock, one can rename and remove it, loop, and acquire a fresh lock before the other reaches this call; the delayed renameSync then moves and deletes that fresh lock because it operates on the pathname rather than the inode previously statted. Both processes can consequently enter the state read-modify-write and lose job records. Fresh evidence in this revision is that the replacement lock remains vulnerable at renameSync(lockFile, scratch), so the claimed exactly-one-winner behavior does not hold once the source pathname is recreated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right — the rename reclaim still had a TOCTOU: a delayed reclaimer operates on the pathname, so if another process recreated a fresh lock there in the meantime, the rename moved/removed that live lock. Any pathname-based reclaim (unlink or rename) has this window. Reworked in 7d7326a so no reclaim is ever decided against a live owner, and no lock is ever seen half-published.
- Liveness-gated reclaim: the lock stamps its holder PID; a waiter only reclaims when
process.kill(pid, 0)shows the owner is dead. A live owner (even slow or briefly suspended) is never reclaimed, so the removal cannot race a lock another process still holds. mtime is demoted to a coarse 10-minute backstop that only covers PID reuse of a dead owner. - Atomic publication: locks are now published with write-temp +
linkSyncinstead ofopenSync(O_EXCL)+ a separatewriteSync.linkSyncis atomic and failsEEXISTlikeO_EXCL, but the file carries its full owner id the instant it appears — closing the window where a concurrent reader could see an empty lock and treat a just-created live lock as abandoned (this affected both the state lock and the reclaim lock). - Reclaim is still serialized through a second lock, which is itself atomically published, PID-stamped, cleared only when its own holder is dead, and released only by its owner.
Residual: a holder suspended past the 10-minute backstop, or PID reuse of a dead owner within it — the timeout limit inherent to any pure-fs PID lock (no new deps allowed here), documented in the code. state.test.mjs (concurrency + stale-lock reclaim) passes.
Follow-up review found the rename-based reclaim still had a TOCTOU: a delayed reclaimer could act on the lock pathname after another process had recreated a fresh lock there. Reworked the lock so no reclaim decision is ever made against a live owner, and so a lock is never observed in a half-published state. - Reclaim is now gated on owner liveness: the lock stamps its holder's PID and a waiter only reclaims when process.kill(pid, 0) shows the owner is dead. A live owner (even a slow or briefly-suspended one) is never reclaimed, so the removal can never race a lock another process still holds. mtime is demoted to a coarse 10-minute backstop that only covers PID reuse of a dead owner (preserving liveness without letting a live holder be reclaimed in any realistic case). - Locks are published atomically via write-temp + linkSync instead of openSync(O_EXCL) followed by a separate writeSync. linkSync is atomic and fails EEXIST like O_EXCL, but the file carries its full owner id the instant it appears, closing the window where a concurrent reader could see an empty lock and treat a just-created live lock as abandoned. Applied to both the state lock and the serialized reclaim lock. - Reclaim is still serialized through a second lock; that lock is atomically published, PID-stamped, cleared only when its own holder is dead, and released only by its owner. The residual (a holder suspended past the 10-minute backstop, or PID reuse of a dead owner within it) is the timeout limit inherent to any pure-fs PID lock and is documented in the code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d7326a5e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function isAbandoned(id, mtimeMs) { | ||
| if (!ownerAlive(id)) return true; // dead owner -> reclaimable | ||
| return Date.now() - mtimeMs > LOCK_BACKSTOP_MS; // else only the far backstop |
There was a problem hiding this comment.
When a lock holder is suspended for more than ten minutes, this branch declares it abandoned despite ownerAlive(id) succeeding; once another process removes the lock, the suspended holder can resume its critical section concurrently with the new owner and overwrite its state. Fresh evidence in this revision is the new live-PID backstop itself, which preserves the replacement-lock race from the earlier review even though normal dead-owner reclamation is now serialized. Reclaim only after verifying that the original process instance is gone, rather than expiring a lock held by a live PID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — a live-but-suspended holder must never be expired. Removed all time-based expiry in cb7a730 and switched to process-instance identity.
- The owner id now embeds a start-time token (Linux
/proc/<pid>/statfield 22; otherwise/bin/ps -o lstartwithTZ/LC_ALLpinned so a stamper and a checker render it identically, and the two sources are never mixed).isAbandonedis true only for a dead PID, or a live PID whose current start token differs from the stamp (PID reuse). A live instance is never reclaimed regardless of how long it holds the lock, and an unverifiable ("0") stamp is never token-reclaimed. - The second "reclaim" lock (whose own stale cleanup had a TOCTOU) is gone. Reclaim now captures the abandoned lock with an atomic
rename(exactly one winner; losers get ENOENT and re-contend), confirms byte-for-byte that it captured the instance it judged, and restores rather than deletes anything it did not. A pre-rename re-read keeps the (possiblyps-backed) liveness probe out of the capture window.
I ran this by two independent adversarial reviewers; both confirmed the live-owner-reclaim, PID-reuse, and reclaim-lock issues are closed. The one residual they both land on is fundamental: a fully atomic conditional-delete needs an OS advisory lock (flock), which Node’s fs builtins do not expose. The remaining window is ~2 adjacent syscalls and its failure mode is a bounded acquire-timeout error (the 15s deadline), never silent corruption or permanent deadlock. This is documented in the code. state.test.mjs (concurrency + instance-liveness reclaim) passes.
Review flagged that the 10-minute mtime backstop could still expire a lock
whose PID is alive (a holder suspended past the backstop), letting it resume
concurrently with a new owner. Removed all time-based expiry.
- A lock is reclaimed only when the exact process *instance* that wrote it is
gone. The owner id now embeds a start-time token (Linux /proc/<pid>/stat
field 22; otherwise `/bin/ps -o lstart` with TZ/locale pinned so a stamper and
a checker render it identically, and never mixing the two sources). isAbandoned
is true only for a dead PID, or a live PID whose current start token differs
from the stamp (PID reuse). A live instance is never reclaimed, no matter how
long it holds the lock. An unverifiable ("0") stamp is never token-reclaimed.
- Reclaim no longer uses a second "reclaim" lock (whose own stale cleanup had a
TOCTOU). It captures the abandoned lock with an atomic rename (exactly one
winner; losers get ENOENT and re-contend), confirms byte-for-byte that it
captured the instance it judged, and restores rather than deletes anything it
didn't. A pre-rename re-read keeps the subprocess-backed liveness probe out of
the capture window.
Residual, documented in code: fully atomic conditional-delete needs an OS
advisory lock (flock), which Node's fs builtins don't expose; the remaining
window is ~2 adjacent syscalls and fails as a bounded acquire-timeout error, not
silent corruption or permanent deadlock.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb7a730719
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export function updateState(cwd, mutate) { | ||
| const state = loadState(cwd); | ||
| mutate(state); | ||
| return saveState(cwd, state); | ||
| return withStateLock(cwd, () => { | ||
| const state = loadState(cwd); | ||
| mutate(state); | ||
| return saveState(cwd, state); |
There was a problem hiding this comment.
When one Claude session ends while another process updates the same workspace, this lock does not protect cleanupSessionJobs in session-lifecycle-hook.mjs, which still performs loadState followed by the exported saveState directly. If an upsertJob completes between those calls, cleanup writes its stale snapshot over the newly added job, and saveState also deletes that job's JSON/log artifacts, so the lost-record corruption remains for concurrent session shutdown and task launch. Route that cleanup through the locked read-modify-write path, or make every state-saving entry point participate in the lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11ce4cc. cleanupSessionJobs now runs through the locked updateState() read-modify-write instead of loadState() + the exported saveState(), so a concurrent upsertJob can no longer be clobbered (nor its job/log files pruned). It collects the running jobs to kill inside the locked mutation and terminates them in a finally after the lock is released — so a failed state write (including the 15s lock-acquire timeout) neither leaks this session's processes nor aborts the rest of session shutdown (broker teardown). If the locked update fails, the pids are identified via a best-effort unlocked read only; no unlocked write ever happens. It was the only state-saving entry point outside the lock (grepped the tree). Two independent reviewers confirmed no reentrancy/deadlock (cleanupSessionJobs is only called from the SessionEnd hook).
| let alive; | ||
| try { | ||
| process.kill(pid, 0); | ||
| alive = true; | ||
| } catch (err) { | ||
| alive = err.code === "EPERM"; // exists but not ours (still alive); ESRCH => dead | ||
| } | ||
| if (!alive) return true; // owner process is gone |
There was a problem hiding this comment.
On Linux, if a lock-holding process exits but remains a zombie because its parent has not reaped it, process.kill(pid, 0) succeeds and /proc/<pid>/stat continues to report the original start token. This therefore classifies the dead owner as live, causing every subsequent state update to time out after 15 seconds until the parent eventually reaps the zombie, which may be indefinite for a suspended or faulty parent. Inspect the process state from /proc/<pid>/stat and reclaim when it is Z.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 11ce4cc. isAbandoned now treats a zombie owner as gone, checked right after the kill(pid,0) liveness probe: on Linux it reads the state field of /proc/<pid>/stat (the token after the last )), elsewhere it runs /bin/ps -o state= (LC_ALL=C). A real macOS zombie renders as ZN, so the check matches startsWith("Z") rather than an exact "Z". Reclaiming a zombie is always safe: it can never execute again, so it can neither use nor release the lock. All probe failures return false (treat as live), which is the conservative direction — it can only delay reclaim, never delete a live lock.
Two review follow-ups: - Session-end cleanup bypassed the lock. cleanupSessionJobs did loadState() then the exported saveState() directly, so a concurrent upsertJob (task launch) could be clobbered and its just-added job/log files pruned. It now runs through the locked updateState() read-modify-write, collecting the running jobs to kill inside the locked mutation. Process teardown runs in a finally after the lock is released, so a failed state write (including the 15s lock-acquire timeout) can neither leak this session's processes nor abort the rest of session shutdown (broker teardown); if the locked update fails, the pids are identified via a best-effort unlocked *read* (never an unlocked write). - Zombie lock owners were treated as live. A process that has exited but not been reaped still answers process.kill(pid, 0) and keeps its start token, so it looked like a live owner forever and blocked every writer until the parent reaped it. isAbandoned now treats a zombie as gone: Linux reads the state field of /proc/<pid>/stat; elsewhere it checks `ps -o state` (matches "Z"/"Z+"/"ZN"). Also corrected the reclaim residual comment: the sub-syscall capture window can, worst case, permit a brief two-writer overlap, not only an acquire timeout.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11ce4cc58e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return; // vanished -- re-contend | ||
| } | ||
| try { | ||
| fs.renameSync(lockFile, tomb); // atomic: exactly one reclaimer captures the path |
There was a problem hiding this comment.
When three contenders race to reclaim an abandoned lock, this can still rename a freshly acquired live lock: one reclaimer removes the stale file and claims the path, the delayed reclaimer executes this pathname-based rename, and a third contender claims the temporarily absent path before the restore at line 302. The first and third processes then both believe they hold the lock and can overwrite each other's state. Fresh evidence in this revision is the residual-race comment at lines 264-274, which explicitly acknowledges that this implementation still permits a two-writer overlap; stale-lock takeover needs an ownership-preserving primitive rather than an unconditional rename of the shared pathname.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rethought this at the root rather than patching the lock again. You are right that a pathname-based takeover cannot preserve ownership, and more generally a crash-safe cross-process lock is not achievable with Node fs builtins (no flock). So I removed the lock entirely in 40032c5: job state is now one file per job at jobs/<id>.json, and the list is derived by scanning that directory. Concurrent launches touch DIFFERENT files, so a stale snapshot overwriting a sibling job is impossible by construction — there is no shared read-modify-write and no lock to take over. Writes are atomic (unique-temp + rename); prune never evicts a live job; legacy state.json job arrays are migrated into per-job files. Reviewed end-to-end by two independent adversarial passes.
| toTerminate.push(job.pid ?? Number.NaN); | ||
| } | ||
| } | ||
| state.jobs = state.jobs.filter((job) => job.sessionId !== sessionId); |
There was a problem hiding this comment.
When a running worker is already waiting in upsertJob for this cleanup's state lock, filtering its record here releases the lock while the worker is still alive; the worker can acquire it immediately and re-add the deleted record before the finally block sends SIGTERM. Session shutdown then leaves a dead or incomplete job visible in state, potentially with recreated artifacts. Terminate the captured workers before the final locked deletion, or perform a second locked removal after termination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40032c5, and the underlying model changed. Session cleanup no longer does a locked read-modify-write: it terminates this session's workers first, waits for them to exit (escalating to SIGKILL), and only then deletes their per-job files — so a worker cannot re-add a record after cleanup removed it. The worker also skips a task whose record is already cancelled when it starts, closing the enqueue/cancel startup window. Because job state is one file per job with no shared array, there is no snapshot for cleanup to clobber a concurrent upsertJob with.
Repeated review rounds showed a pure-Node-fs cross-process lock cannot be made crash-safe (no flock/advisory-lock in fs builtins), so every stale-takeover variant left a residual. Remove the shared state.json jobs[] array and the lock entirely: store one record per job at jobs/<id>.json and derive the list by scanning that directory. Concurrent launches (and session cleanup) now touch DIFFERENT files, so a stale snapshot clobbering a sibling job is impossible by construction, not merely serialized. - upsertJob/writeJobFile merge a patch into jobs/<id>.json via unique-temp + rename (atomic publish; readers never see a torn file). listJobs scans the dir, skipping ENOENT/parse failures. state.json now holds only config. - Prune keeps the newest MAX_JOBS and evicts only terminal jobs, non-terminal jobs whose pid is dead, or pid-less records stuck past a 60s grace -- never a live queued/running job -- and removes the payload and both log paths. - Legacy state.json jobs[] arrays are migrated into per-job files (additive merge, existing payload wins) then state.json is rewritten config-only. - Session cleanup terminates this session's workers, waits for exit (escalating to SIGKILL), then deletes their files -- no lock, no locked read-modify-write. - Enqueue writes the queued record before spawning (fixing a "No stored job found" race) and the worker owns all later lifecycle states; the worker skips a task already cancelled during its startup window. Removed a stale-snapshot re-write in the progress updater that could resurrect a cancelled job. Job ids are validated before being used as paths. Guarded the per-job reads against prune/cleanup TOCTOU. Tests updated to the per-job-file model.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40032c5384
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const jobFile = resolveJobFile(cwd, jobId); | ||
| fs.writeFileSync(jobFile, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); | ||
| return jobFile; | ||
| const existing = readJobRecord(cwd, jobId) ?? {}; |
There was a problem hiding this comment.
When two processes update the same job, the atomic rename does not make this read-modify-write atomic: for example, the enqueueing parent can read the queued record for its PID-only patch, the worker can publish status: "running", and then the parent can replace that file with its stale queued snapshot. The job is consequently reported as queued and loses its running metadata until completion; cancellation/progress races can similarly overwrite newer lifecycle fields. Use per-job serialization or a compare-and-retry mechanism around the read and replace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right — the atomic rename doesn't serialize the read-modify-write, and a monotonic guard computed from a stale snapshot can't fix it either (still last-rename-wins). Rather than serialize the RMW, e1b6591 removes the concurrency: the record is single-writer and cancellation moves off the record. The parent no longer patches the pid after spawn (the worker publishes its own pid with the running record, and the pre-spawn queued write is an atomic wx claim), so jobs/<id>.json has one writer and mergeJobRecord is a plain additive merge. Cancellation is an immutable <id>.cancelled marker that readAllJobs overlays, so it can never be reverted by a racing worker write.
| if (!isRunning(job)) continue; | ||
| const pid = job.pid; | ||
| if (Number.isInteger(pid) && pid > 0) pids.push(pid); | ||
| try { | ||
| terminateProcessTree(job.pid ?? Number.NaN); | ||
| terminateProcessTree(pid ?? Number.NaN); |
There was a problem hiding this comment.
When SessionEnd races the startup window created by enqueueBackgroundTask—after its queued record is written with pid: null but before the PID patch—this branch has no process to terminate. If the worker already read the queued record, it can recreate the deleted record and keep running; if cleanup deletes first, the parent's later PID patch can still recreate a partial job. Fresh evidence in this revision is the new pre-spawn pid-less queued record, which this cleanup path treats as running but cannot stop; publish a cancellation tombstone or otherwise coordinate startup before deleting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in e1b6591 via that marker plus a publish-pid / check-marker handshake. Cleanup writes the immutable <id>.cancelled marker (and a session-<hash>.ended marker) before reading any pid; the worker publishes its pid, then re-checks the markers — each side writes its flag before reading the other's, so either cleanup finds the pid and kills it or the worker self-aborts. A queued pid-less job is left as record+marker (overlay => cancelled), never a plain unlink; the session marker is written before the scan, and prune evicts on the raw record and never age-evicts a pid-less job, so a booting worker can't be stranded or resurrected.
Replace the mutable-record cancellation path (which lost updates and could resurrect cancelled jobs under the lock-free per-file store) with a single-writer record plus immutable cancel/session markers and a publish-pid / check-marker handshake. - Record is single-writer: the parent no longer patches the child pid after spawn; the worker publishes its own pid with the running record. The pre-spawn queued write is an atomic wx claim (createJsonExclusive), so a duplicate id cannot spawn a second worker; mergeJobRecord is a plain additive merge again. - Cancellation is an immutable jobs/<id>.cancelled marker (atomic wx create); readAllJobs overlays it so a cancellation can never be masked by a racing worker write. A session-<hash>.ended marker closes the enqueue-after-scan window. - Handshake: cancel/cleanup create the marker BEFORE reading the pid; the worker publishes its pid BEFORE re-checking the marker, so at least one side observes the other -- the worker is killed or it self-aborts. - Session cleanup writes the session marker before its raw scan, keeps a booting/still-runnable worker's record+marker, and deletes only provably-dead or terminal jobs. prune evicts on the raw record and never age-evicts a pid-less non-terminal job. Legacy migration is create-exclusive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b659188a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const job of parsed.jobs) { | ||
| if (!job || !isValidJobId(job.id)) continue; | ||
| createJsonExclusive(resolveJobFile(cwd, job.id), job); // never overwrite a live record | ||
| } |
There was a problem hiding this comment.
When upgrading a workspace, legacy jobs normally already have <id>.json payload files, so this exclusive create discards the corresponding state.json entry instead of combining them. Any fields present only in the index—such as the final summary, thread metadata, or timing information—are then permanently lost when state.json is rewritten without jobs; the existing status shows phases, hints, and the latest finished job test demonstrates this by losing the finished job's duration and thread ID. Migrate the union of the legacy index metadata and stored payload while still preventing a live worker from being overwritten.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c8bb486. Migration folds the union of the legacy index and the stored payload ({...index, ...existing}, payload wins) so a finished job keeps its summary/threadId/timing — but only when the RAW per-job record has no live worker (isEvictable: terminal, or running with an ESRCH-dead pid). A live or still-booting record is left untouched (never a second writer racing the worker), and a missing record is still created exclusively.
| const workspaceRoot = resolveWorkspaceRoot(cwd); | ||
| const stateFile = resolveStateFile(workspaceRoot); | ||
| if (!fs.existsSync(stateFile)) { | ||
| if (!fs.existsSync(resolveStateFile(workspaceRoot)) && !fs.existsSync(resolveJobsDir(workspaceRoot))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When SessionEnd races the first background launch in a workspace, both state paths can be absent at this check, causing cleanup to return without creating the session-ended marker; the launcher can then create its queued record and run normally after the session has ended. Fresh evidence in this revision is this pre-marker early return, which bypasses the tombstone handshake intended to fix the previously reported startup race. Create the marker before this existence optimization, or remove the optimization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c8bb486. Removed the existsSync early-return; cleanupSessionJobs now publishes markSessionEnded first (it ensures the state dir), before any check or scan. So a SessionEnd racing the first launch always writes the tombstone: the worker either sees it, or (slipping both checks) it published its pid before the marker and thus before the scan, so cleanup finds and kills it. Cleanup also now exits non-zero if that load-bearing marker cannot be written.
…g, cancellation hardening Bot findings: - P1 migrateLegacyState: fold legacy state.json index-only metadata (summary, threadId, timing) into an existing per-job record instead of discarding it, but only when the RAW record has no live worker (isEvictable); create-exclusive for a missing record, leave a live/booting record untouched. - P2 cleanupSessionJobs: publish the session-ended marker BEFORE any existsSync check or scan, so a SessionEnd racing the first launch can't skip it. Fresh full-scan hardening: - /codex:result renders from the overlaid record, not a raw read, so a completed-then-cancelled job can't leak its result/rendered. - overlayJob surfaces the cancel marker's reason/at as errorMessage/cancelledAt/ completedAt (best-effort; marker existence stays authoritative), still stripping the completion payload. - handleCancel wraps terminateProcessTree; resolveCancelableJob classifies raw records so a marked job stays targetable only while running with a live pid (to retry a kill that threw) and never shadows a genuinely active job -- honoring both the cancel and session-ended markers. - Session cleanup exits non-zero if a load-bearing marker (session-ended, or a kept job's cancel marker) cannot be written.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8bb48671e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const evictable = jobs | ||
| .filter(isEvictable) | ||
| .sort((a, b) => String(a.updatedAt ?? "").localeCompare(String(b.updatedAt ?? ""))); |
There was a problem hiding this comment.
When at least 50 records are non-evictable—for example, 50 queued pid-less jobs—completing another job makes overflow positive while that newly completed job is the only evictable record. Filtering to evictable jobs before sorting therefore deletes the just-published completion, so status and result immediately lose its output. Determine the oldest records outside the newest-50 window first and evict only eligible records from that set, allowing the cap to remain soft while too many jobs are active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e73e56c. pruneJobs now protects the newest MAX_JOBS: only records OUTSIDE that window are eviction candidates, and among those it removes the evictable ones (decided on the raw record). So when the cap is saturated by non-evictable queued/pid-less jobs and one more job completes, that fresh completion (the newest record) is never the eviction target — the cap simply stays soft while too many jobs are active, exactly as suggested. (This push also folds in several fixes from a fresh full-scan: fresh per-record marker reads to close a scan/overlay race, stripping the completion summary on a cancelled overlay, fail-loud session cleanup on a genuine scan I/O error with prune isolated from the committed write, a config re-read in legacy migration, final-log-before-terminal ordering, and excluding cancelled tasks from --resume-last.)
…ce, fail-loud scans Bot P2 (prune cap): evict only records OUTSIDE the newest-MAX_JOBS window, so a fresh completion is never deleted when the cap is saturated by non-evictable queued jobs. Fresh full-scan hardening (adversarial review + independent audit): - readAllJobs reads each record's cancel/session marker FRESH (after the record), not from a stale readdir snapshot, so a marker published between the directory listing and the record read is still observed and a cancelled completion can't leak. A null/non-object marker body is treated as present (existence is authoritative). - overlayJob applies to a marked record even if its raw status is already "cancelled", and strips `summary` in addition to `result`/`rendered`. - scanRawJobs: an absent dir (ENOENT) is empty, but EACCES/EIO (on the dir or a visible record) propagate; session cleanup fails loud (non-zero exit) on a scan failure instead of reporting success. Prune is isolated from the committed upsertJob write, so a GC scan failure can never turn a committed `completed` into `failed`. - migrateLegacyState re-reads the current config before the state.json rewrite and aborts the rewrite if that read fails, so a concurrent setConfig isn't clobbered. - runTrackedJob appends the final-output log BEFORE publishing the terminal record, so the terminal write is the worker's last fs touch and cleanup can't orphan a recreated log. - Resume (--resume-last) only considers completed/failed tasks, never cancelled.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e73e56cbe8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function isEvictable(job) { | ||
| if (typeof job.id !== "string") return false; | ||
| if (TERMINAL_STATUSES.has(job.status)) return true; | ||
| if (Number.isInteger(job.pid) && job.pid > 0) return !pidAlive(job.pid); | ||
| return false; // pid-less non-terminal -> a worker may still be booting; never age-evict |
There was a problem hiding this comment.
If the launcher exits after claiming a queued record but before the worker publishes its PID—for example, the detached spawn emits an asynchronous error or the worker is killed during startup—the record remains queued with pid: null permanently. This branch makes such records ineligible for every future prune, and session cleanup also retains them behind a marker, so repeated startup failures accumulate an unbounded number of job files and increasingly expensive directory scans despite the nominal 50-job cap. Add a race-safe way to retire definitively abandoned queued records rather than treating them as non-evictable forever.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — an abandoned pid-less queued record does leak if the launcher dies before the worker publishes its pid. Worth calling out that this is in direct tension with the earlier change that STOPPED age-evicting pid-less queued records (age-eviction races a still-booting worker and can resurrect a job). Both are real; they trade off. A race-safe reclaim is doable — write the cancelled marker first (so any late-booting worker self-aborts on its post-pid re-check), then GC after a boot-window grace — but the acceptable leak-vs-resurrection balance and the grace value are a maintainer call. I left a fuller status note on the PR conversation (#issuecomment-5425821694); happy to implement whichever tradeoff you prefer.
Status note after 9 automated review roundsFirst — thanks, genuinely. This reviewer has caught real issues and materially improved the PR. I want to flag where I think things stand, with the data, because the two most recent findings now pull in opposite directions, which suggests we've moved from defects into judgment tradeoffs. Trajectory (13 inline findings across 9 rounds):
The tension in the latest finding. An earlier revision stopped age-evicting pid-less The core protocol has held up across the last several rounds — recent findings poke at GC/edge policy, not the invariants. Happy to implement whichever direction on the reclaim tradeoff you prefer; I mainly wanted to surface that a human maintainer's read would likely be more useful here than another automated round. |
1 participant
Problem
updateState()inplugins/codex/scripts/lib/state.mjsdoes a lockless read-modify-write ofstate.json. When severalcodex-companion.mjs task --backgroundjobs launch at nearly the same time, each process reads the same base state, appends only its own job, and writes back, so the last writer wins and the sibling jobs are lost.saveState()'s prune then deletes the orphaned siblings'.json/.logfiles, because they are absent from the writer's stale view.Net effect for multi-agent use: launch N background agents and only ~1 gets tracked; the rest silently vanish from
/codex:statusand/codex:result, and some never register at all.Reproduction
12 concurrent
upsertJobchild processes against one workspace:Fix
withStateLock()serializes the read-modify-write with a cross-processO_EXCLlockfile (jittered backoff, stale-lock stealing so a crashed process can't wedge the store). This is the core fix: every writer now reads the latest state, including siblings, before mutating.rename) so lockless readers never observe a tornstate.json.sleepSync()usesAtomics.waitso the busy path blocks without spinning the CPU, matching the synchronousupdateStatecontract.Tests
tests/state.test.mjs: 12 concurrentupsertJobprocesses, asserts every job is tracked.node --test tests/*.test.mjs-> 91/91.🤖 Generated with Claude Code