fix: reap detached workers that die before reaching 'running', and mark in-process crashes failed immediately - #52
Conversation
…ram mismatch
npm run build (tsc -p tsconfig.app-server.json) fails on main with:
codex.mjs(1209,53): error TS2339: Property 'threadId' does not exist on type '{}'.
codex.mjs(1209,63): error TS2339: Property 'turnId' does not exist on type '{}'.
codex.mjs(1209,71): error TS2339: Property 'timeoutMs' does not exist on type '{}'.
interruptAppServerTurn's second parameter is destructured directly in the
signature (`{ threadId, turnId, timeoutMs } = {}`), while the JSDoc above
it types a parameter named `options`. TS's JSDoc-to-signature binding
matches by parameter position/name, not by shape, so the destructuring
pattern doesn't pick up the JSDoc type -- TS instead infers the parameter's
type from its `= {}` default, i.e. `{}`, and then rejects every property
access on the destructured names.
Fix: destructure inside the function body instead of the signature (same
pattern already used by CodexAppServerClient#request in app-server.mjs),
so the JSDoc-typed `options` parameter name lines up with the actual
parameter. No behavior change -- same defaulting, same property reads,
just moved one line down.
Landed via db52e28/f67a09f without a build check catching it (npm test
alone doesn't run tsc). Verified by reverting this change on a clean
main checkout and reproducing the same three errors, then re-applying to
confirm `npm run build` is clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HbkiKZR4w8hZUmNNTdb6kB
…rk in-process crashes failed immediately Investigated upstream openai#425 ("detached task worker that dies without throwing leaves job stuck 'running' forever") for portability here. Verified against this fork's own job-tracking code before porting anything. This fork already had an independent equivalent of upstream's reader-side reapDeadJobs: state.mjs's reconcileRunningJobs, called unconditionally inside listJobs() itself (not as a separate wrapper every call site has to remember to apply — architecturally tighter than upstream's approach, which wraps 5 separate call sites in job-control.mjs/codex-companion.mjs/ stop-review-gate-hook.mjs with reapDeadJobs()). That closes the "running" half of openai#425 and the whole of openai#392 (verified separately, see companion report) without any change. One real gap remained: reconcileRunningJobs only reconciled job.status === "running", not "queued". enqueueBackgroundTask records the detached worker's pid at enqueue time (status: "queued"), before that worker has run far enough to flip its own record to "running" via runTrackedJob. A worker that dies in that window — crash, immediate OOM kill — left the job stuck "queued" forever with an already-dead pid, invisible to the "running"-only check, permanently blocking --resume-last and every other gate that treats queued/running as active. Reproduced with a real dead pid recorded against a "queued" job; the --resume-last gate threw "still running" forever. Fixed by also reconciling "queued" jobs. Ported registerWorkerCrashGuard on top of that gap fix: an in-process uncaughtException/unhandledRejection handler installed in the task worker (handleTaskWorker) that marks the job failed immediately, with the actual crash reason logged, instead of waiting for the next listJobs() read to lazily reconcile it. Adapted to this fork's tracked-jobs.mjs/state.mjs helpers (readJobFile/writeJobFile/upsertJob) rather than upstream's shape. Deliberately does NOT install signal handlers (SIGTERM/SIGINT/SIGHUP): SIGKILL is uncatchable regardless, so the reader-side reconciliation must cover process death either way, and /codex:cancel's SIGTERM teardown races a "cancelled" write that this guard must not clobber back to "failed" — the guard checks for an already-terminal status before rewriting, same as upstream's guard. Tests: - tests/runtime.test.mjs: two end-to-end --resume-last tests — one proving the pre-existing "running" reconciliation already unblocks --resume-last (not just /status), one proving the "queued" gap and its fix (spawn a real dead pid, seed a stuck "queued" job, assert --resume-last succeeds instead of throwing "still running"). - tests/tracked-jobs.test.mjs (new): registerWorkerCrashGuard marks a job failed on an unhandled rejection with the reason logged; does not clobber an already-cancelled job on SIGTERM. npm test: 193/193 passing (190 baseline + 3 new). npm run build: clean (verified on top of #51, which this branch is stacked on, since main's tsc currently fails on an unrelated pre-existing error #51 fixes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbkiKZR4w8hZUmNNTdb6kB
|
@codex review PR #52 at exact head e4e4986. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production. Context: this PR closes the remaining gap from upstream openai#425 (a detached task worker that dies without throwing leaves its job stuck forever). Note it is stacked on #51 — its base is Two changes:
Specifically worth checking:
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Investigated upstream openai/codex-plugin-cc#425 ("detached task worker that dies without throwing leaves job stuck 'running' forever") for portability here. TDD-first against this fork's own job-tracking code, not the upstream diff.
Stacked on #51 (the pre-existing
mainbuild break interrupts a cleannpm run buildon any branch based onmain).Most of openai#425 was already closed independently. This fork has its own equivalent of upstream's reader-side
reapDeadJobs:state.mjs'sreconcileRunningJobs, called unconditionally insidelistJobs()itself — not a separate wrapper that 5 different call sites (job-control.mjs,codex-companion.mjs,stop-review-gate-hook.mjs) each have to remember to apply, which is how upstream wired it. Every consumer that reads jobs already goes through the reconciliation for free. This also fully closes upstream openai#392 (verified separately as part of this investigation — no change needed there, see companion report to the requester).One real gap remained:
reconcileRunningJobsonly reconciledjob.status === "running", not"queued".enqueueBackgroundTaskrecords the detached worker's pid at enqueue time withstatus: "queued", before that worker has run far enough to flip its own record to"running"viarunTrackedJob. A worker that dies in that window (crash, immediate OOM kill) left the job stuck"queued"forever with an already-dead pid — invisible to the "running"-only check — permanently blocking--resume-lastand every other gate that treats queued/running as active.Reproduced: seeded a
"queued"job with a real, verified-dead pid;task --resume-lastthrew"Task <id> is still running..."forever. Fixed by extendingreconcileRunningJobsto also reconcile"queued".Also ported:
registerWorkerCrashGuard, adapted to this fork'stracked-jobs.mjs/state.mjshelpers rather than blind-copying upstream's shape. Installed inhandleTaskWorker, it's an in-processuncaughtException/unhandledRejectionhandler that marks the job failed immediately, with the actual crash reason logged to the job's log file — instead of waiting for the nextlistJobs()read to lazily reconcile it. This closes the remaining functional gap between "eventually correct" (the reconciliation fix above) and "immediately correct with a useful error message," which is the rest of what upstream openai#425 does. Deliberately does not install signal handlers (SIGTERM/SIGINT/SIGHUP): SIGKILL is uncatchable regardless, so reader-side reconciliation must cover process death either way, and/codex:cancel's SIGTERM teardown races a"cancelled"write that this guard must not clobber back to"failed"— the guard checks for an already-terminal status before rewriting, mirroring upstream's own reasoning for the same signal exclusion.Test plan
tests/runtime.test.mjs:task --resume-last is not permanently blocked by a job stuck 'running' with a dead worker pid (upstream #392)— proves the pre-existing "running" reconciliation already unblocks--resume-lastend-to-end, not just/status/task-resume-candidate.task --resume-last is not permanently blocked by a job stuck 'queued' with a dead worker pid (upstream #425 gap)— spawns a real, verified-dead pid, seeds a stuck"queued"job, asserts--resume-lastsucceeds instead of throwing. Verified red before thestate.mjsfix (reverted the change, confirmed the exact upstream-reported throw), green after.tests/tracked-jobs.test.mjs(new):registerWorkerCrashGuardmarks a job failed on an unhandled rejection with the reason logged in the error message; does not clobber an already-"cancelled"job when SIGTERM arrives mid-teardown.npm test— 193/193 passing (190 baseline + 3 new).npm run build— clean, stacked on fix: repair the tsc build broken by interruptAppServerTurn's JSDoc/param mismatch #51.No version bump.