fix: persist background jobs before worker spawn - #733
Conversation
![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: ce6563b305
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8452a16df
ℹ️ 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".
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Persist-before-spawn fixes the original child-beats-parent race, and the worker's queued-status check correctly makes cancellation successor-safe. One failure path is now exposed: if spawnDetachedTaskWorker() itself throws after writeJobFile/upsertJob, the caller gets an error but a durable queued, pid: null job remains with no worker to claim it. Please transition or remove that bootstrap record on spawn failure under the same job ownership boundary, so this does not trade a missing-record race for a permanently stranded queue entry.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f35b690dc
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7a435dbf6
ℹ️ 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".
| return { ...job, status: "removed", phase: "removed", pid: null }; | ||
| } | ||
| const currentJob = readStoredJobOrNull(job.workspaceRoot, job.id) ?? job; | ||
| if (isJobCancellationRequested(job.workspaceRoot, job.id) || currentJob.status !== "queued") { |
There was a problem hiding this comment.
When an emitted spawn error overlaps /codex:cancel or SessionEnd, this marker/status check and the subsequent failed-record writes are not atomic. If the callback passes this guard just before the other process publishes its marker and terminalizes/removes the job, it can resume afterward and overwrite cancelled with failed or recreate a job that session cleanup removed. The fresh evidence is that failTrackedJobLaunch performs its only cancellation/removal checks before the writes at lines 167-168; coordinate the transition or revalidate without allowing a check-to-write race.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0d8cb00. I reproduced both check→write races deterministically by triggering cancellation/removal from error.toString() after the initial guard but before the failed-record write. RED on d7a435d: both cases ended as failed. The fix now revalidates durable terminal markers after persisting failed: removal deletes job file/state; cancellation rewrites canonical cancelled state, so terminal authority wins even if it arrives in that window. Validation on Windows: tracked-jobs + commands + state 20/20 PASS; targeted runtime background|cancel|SessionEnd 6 relevant cases PASS with one known Windows taskkill failure. I reproduced that exact taskkill ... operation not permitted failure on untouched upstream/main (db52e28), confirming it is baseline and unrelated. node --check and git diff --check clean. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d8cb00272
ℹ️ 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".
| cancelledAt: completedAt, | ||
| errorMessage: "Cancelled by user." | ||
| }; | ||
| writeJobFile(job.workspaceRoot, job.id, cancelledRecord); |
There was a problem hiding this comment.
When /codex:cancel and SessionEnd overlap during the queued-worker handoff, the worker can pass the removal check at line 217, observe the cancellation marker, and then have session cleanup publish .removed and delete the job before this write. If process termination is delayed or fails, this branch recreates the cancelled job file and index after cleanup and never revalidates the removal marker, unlike failTrackedJobLaunch; recheck removal after these writes and delete the artifacts/state so session removal remains authoritative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d5fda68. The startup-cancellation branch now rechecks the durable removal marker after persisting cancelled; if SessionEnd won in that window, it deletes the per-job file, removes the indexed state, and returns removedExecution instead of recreating terminalized work. Added a RED structural regression on 0d8cb00 requiring removal revalidation after the cancellation writes; GREEN on this commit, plus existing behavioral coverage. Fresh validation: tracked-jobs + commands + state 21/21 PASS. Targeted runtime background|cancel|SessionEnd has 6 relevant cases PASS and one Windows taskkill failure; I reproduced that exact failure again on untouched upstream/main (db52e28), confirming baseline. node --check/git diff --check clean. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5fda68121
ℹ️ 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 cwd = resolveCommandCwd(options); | ||
| const reference = positionals[0] ?? ""; | ||
| const { workspaceRoot, job } = resolveCancelableJob(cwd, reference, { env: process.env }); | ||
| markJobCancellationRequested(workspaceRoot, job.id); |
There was a problem hiding this comment.
When /codex:cancel pauses in interruptAppServerTurn and SessionEnd runs in that window, cleanup publishes .removed and deletes the job, but this handler subsequently writes and indexes cancelled unconditionally, recreating the removed artifacts. A late worker will not repair this because it reads the recreated cancelled record and returns before runTrackedJob. The fresh evidence after the line-232 fix is that removal revalidation was added only to the worker's startup-cancellation branch; handleCancel still needs to recheck removal after its cancellation writes and delete the per-job file and indexed state when SessionEnd won.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5fda68121
ℹ️ 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 execution = await runner(); | ||
| if (isJobRemovalRequested(job.workspaceRoot, job.id)) { |
There was a problem hiding this comment.
When cleanupSessionJobs starts just after this check, the worker can write the completed job file with pid: null, after which SessionEnd reads the still-running index entry, cannot terminate the worker using that file, and deletes the job before the worker's subsequent upsertJob. The upsert then recreates the session's job index without its result file, so cleanup is undone and /codex:result cannot retrieve the advertised completed result. Revalidate the removal marker after the completion file/index writes or serialize terminalization with removal.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #620.
task-workercan never race a missing bootstrap recordpid: null; the worker claims the record with its own PID whenrunTrackedJobstartsrunningjob-file/index writes insiderunTrackedJob's existingtryso startup persistence failures enter the normal failure-bookkeeping pathspawnDetachedTaskWorkerand requires therunTrackedJobtry boundary to cover its initial writesThis keeps the background protocol unchanged while removing the measured Windows race where the child could reach
readStoredJobhundreds of milliseconds before the parent wrote the record.Validation
Windows 11 / Node 26.3.1:
upstream/main, becausespawnDetachedTaskWorkerprecedeswriteJobFile/upsertJobtests/commands.test.mjs: 9 passed, 0 failedbackground|cancel: 5 passed, 1 failed; the single failure is the existing Windowstaskkill"operation not permitted" case and reproduces identically on untouchedupstream/mainnode --check plugins/codex/scripts/codex-companion.mjsnode --check plugins/codex/scripts/lib/tracked-jobs.mjsgit diff --checkFresh collision check immediately before publication found no open PR for #620.