fix: reconcile stale background job workers - #728
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: 1986b7ec45
ℹ️ 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".
|
Addressed the P2 liveness-consistency feedback in c571102: the reconciled job view now reaches @codex review |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5711025d6
ℹ️ 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.
The ESRCH projection handles workers that have already claimed a PID, but queued records with pid: null are explicitly left active. Since background enqueue persists exactly that bootstrap state before the detached worker is spawned, a spawn failure can still strand a job forever and keep status/resume/Stop treating it as active. I would either terminally record spawn failure at enqueue, or add a bounded policy for unclaimed queued records so this reconciliation covers the full startup lifecycle.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8286b41191
ℹ️ 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 (error?.code === "ESRCH") { | ||
| return { ...job, status: "terminated-unknown", phase: "worker-exited" }; |
There was a problem hiding this comment.
When a detached worker exits after turn/started, the shared Codex process may still be executing the turn: the broker's socket-close handler only clears ownership in app-server-broker.mjs:225-227 and does not send turn/interrupt. Marking the job terminal solely because its wrapper PID disappeared lets the resume and Stop-hook paths start another turn or review while the orphaned turn can still edit the workspace. Reconcile or interrupt the persisted threadId/turnId before treating this job as inactive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 12a4cf5. Liveness reconciliation now distinguishes wrapper death before vs. after durable Codex turn identity exists: an ESRCH job with no threadId/turnId remains terminated-unknown, while a job with both IDs stays logically active as running with phase: worker-exited-turn-unknown, pid: null, and workerExited: true. This keeps Stop/resume fail-closed and leaves the job selectable by /codex:cancel, whose existing path can issue turn/interrupt using the persisted IDs. Deterministic RED→GREEN coverage includes unit/state consumption plus the two reviewer-critical runtime routes: task --resume-last blocks an orphaned live turn while still ignoring a stale pre-turn worker, and Stop keeps an orphaned live turn active while still ignoring a stale pre-turn worker. Fresh validation on the exact branch: job-control/render/commands 17/17 PASS; targeted runtime stale-vs-live-turn 4/4 PASS; node --check and git diff --check PASS. @codex review
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. You're on a roll.
Reviewed commit: 12a4cf5532
ℹ️ 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".
|
On the I addressed that startup failure causally in the companion enqueue path in #733: the queued record is persisted before spawn, synchronous/asynchronous spawn failures terminalize that record, and the worker/cancel/SessionEnd handoff is guarded so late startup events cannot resurrect or overwrite terminal state. This PR remains the read-time reconciliation layer for jobs that did claim a PID and later disappeared. Keeping those mechanisms separate avoids a heuristic timeout in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a4cf5532
ℹ️ 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; | ||
| } | ||
| if (error?.code === "ESRCH") { | ||
| if (job.threadId && job.turnId) { |
There was a problem hiding this comment.
When a detached worker exits after the broker accepts turn/start but before its response is processed, the persisted job has a threadId but no turnId, so this condition falls through to terminated-unknown and resume paths may start another workspace-editing turn. Fresh evidence in this revision is that captureTurn buffers all turn notifications until the response sets state.turnId (plugins/codex/scripts/lib/codex.mjs:563-591), while the broker releases request ownership as soon as the crashed worker's socket closes (plugins/codex/scripts/app-server-broker.mjs:225-232); therefore requiring both identifiers still leaves an accepted, potentially running turn classified as inactive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8de65eb. I reproduced the accepted-turn/start-response window with three RED regressions on 12a4cf5: (1) dead wrapper + persisted threadId but no turnId was projected terminal, (2) /codex:cancel could not represent the still-running/unknown-turn state safely, and (3) SessionEnd removed tracking for that potentially live turn. The fix is fail-closed: any dead wrapper with durable threadId remains logically active as worker-exited-turn-unknown; cancel refuses to claim success while turnId is unavailable; SessionEnd preserves the reconciled orphaned job instead of deleting its tracking. GREEN: 3/3 new regressions, job-control/render/commands 18/18 PASS, ordinary SessionEnd cleanup 1/1 PASS, syntax/diff checks clean. The brokered cancel runtime remains flaky on Windows; the same test also fails on untouched upstream/main in this environment. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de65eb8fb
ℹ️ 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 (job.workerExited && threadId && !turnId) { | ||
| throw new Error( | ||
| `Cannot safely cancel ${job.id}: the worker exited after turn/start was accepted, but the turn id is not yet known. The Codex turn may still be running.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
When a reconciled job has workerExited: true and a known turnId, this guard allows cancellation to continue even if connecting to the broker or turn/interrupt fails and returns interrupted: false. Reconciliation has already cleared the PID, so terminateProcessTree cannot stop anything, yet the job is recorded as cancelled and resume/Stop paths may permit new workspace-editing work while the orphaned turn continues. Fail closed for every worker-exited job unless interruption succeeds, not only when the turn ID is missing.
Useful? React with 👍 / 👎.
| if (error?.code === "ESRCH") { | ||
| if (job.threadId) { |
There was a problem hiding this comment.
A persisted threadId does not prove that turn/start was accepted: runAppServerTurn reports and persists the thread at codex.mjs:1123-1125, then only submits the turn at 1132-1143. If the worker exits in that gap, this branch projects a job with no running turn as active, clears its PID, and the new cancellation guard rejects it forever because no turnId can ever arrive. Persist a separate turn-submission marker or query the thread rather than treating every created thread as an accepted turn.
Useful? React with 👍 / 👎.
| if (reconciled.workerExited && reconciled.threadId) { | ||
| retainedJobs.set(job.id, reconciled); | ||
| continue; |
There was a problem hiding this comment.
During SessionEnd, broker shutdown is requested before this cleanup and the broker closes its app-server process, with the subsequent teardown providing a hard-stop fallback. Retaining the reconciled record as running with pid: null therefore leaves it permanently active after its turn has been terminated; later liveness checks skip PID-less jobs, and an unknown-turn record cannot be cancelled. Preserve the diagnostic record if desired, but transition it to a terminal/unknown status once broker teardown has made the turn non-live.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de65eb8fb
ℹ️ 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".
| status: "running", | ||
| phase: "worker-exited-turn-unknown", | ||
| pid: null, | ||
| workerExited: true |
There was a problem hiding this comment.
When the wrapper exits after Thread ready—including before turn/start is sent—this projection records the job as running while clearing its PID. Every subsequent reconcileJobLiveness call then returns immediately because the PID is null, and a record without a turnId cannot be cancelled, so task --resume-last and status --wait remain blocked indefinitely even if no turn was started or the accepted turn later completed. Preserve enough durable turn-start state to query or eventually terminate this transitional record instead of making it permanently active.
Useful? React with 👍 / 👎.
| const threadId = existing.threadId ?? job.threadId ?? null; | ||
| const turnId = existing.turnId ?? job.turnId ?? null; | ||
|
|
||
| if (job.workerExited && threadId && !turnId) { |
There was a problem hiding this comment.
When a dead wrapper has a persisted turnId, this guard is skipped; if interruptAppServerTurn subsequently reports interrupted: false because the broker or RPC failed, the handler still terminates the already-null PID and persists cancelled. The brokered Codex turn may therefore continue editing while resume and Stop paths treat the job as terminal and allow overlapping work. For workerExited jobs, require a successful interrupt or confirmation that the turn is terminal before recording cancellation.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #704.
queued/runningjob records against the recorded worker PID when jobs are readprocess.kill(pid, 0)ESRCHas a missing worker and project the job asterminated-unknownwith phaseworker-exitedEPERMas alive and fail open on unrecognized probe errors to avoid false terminationEPERM, terminal/pid-less records, and active-queue classificationThis fixes stale jobs already on disk without requiring a worker-side protocol change.
Validation
On Windows 11 / Node 26.3.1:
reconcileJobLivenessdid not existnode --test tests/job-control.test.mjs tests/render.test.mjs tests/commands.test.mjs-> 14 passed, 0 failed999999projects{ status: "terminated-unknown", phase: "worker-exited" }node --check plugins/codex/scripts/lib/job-control.mjsgit diff --cached --checkI also attempted the repository-wide
npm teston this Windows host. It encounters an unrelated existingbroker-endpoint.test.mjsfailure for the Unix-socket case before the new job-control coverage; this PR does not modify broker endpoint code.A Codex CLI review was also attempted but did not execute because the account had reached its code-review usage limit, so it is not claimed as validation.