Skip to content

fix: reap zombie 'running' jobs at the resume gate via pid-liveness check (#222, #202) - #392

Open
storyarcade wants to merge 1 commit into
openai:mainfrom
storyarcade:fix/reap-zombie-running-jobs-pid-liveness
Open

fix: reap zombie 'running' jobs at the resume gate via pid-liveness check (#222, #202)#392
storyarcade wants to merge 1 commit into
openai:mainfrom
storyarcade:fix/reap-zombie-running-jobs-pid-liveness

Conversation

@storyarcade

Copy link
Copy Markdown

Problem

When a Codex task worker crashes or is killed externally, its job record stays status: "running" forever. The next task/resume call in that cwd then throws and is permanently blocked:

Task <id> is still running. Use /codex:status before continuing it.

resolveLatestTrackedTaskThread() finds any queued|running job and throws without checking whether the worker process is actually alive — there is no pid-liveness check, so a dead worker's record is indelible. (Root cause: #222; symptoms: #202, #164.)

Fix

Before the blocking throw, probe the worker pid:

  1. process.kill(pid, 0)ESRCH ⇒ dead, EPERM ⇒ alive-but-not-ours (treat as alive). Portable across macOS/Linux/Windows via libuv; no new dependency.
  2. If the worker is dead, reconcile the job to failed — writing state.json (the store the gate reads) plus a best-effort per-job-file mirror for status/result readers — then mutate the in-memory snapshot and fall through to normal resolution.
  3. Only throw the "still running" error if the worker is actually alive (live jobs are unaffected).

Why minimal (vs #216)

This touches only the gate function: ~12 lines + two small helpers. It does not add a deadline/timeout, a background reaper, or a daemon — just "don't block on a provably-dead worker." Builds on the approach in the now-conflicting #216, scoped to the surgical, conflict-free subset.

Known limitation (PID reuse)

A PID-based probe inherits the standard caveat: if the OS recycles the crashed worker's PID before the next task call and an unrelated process holds it, the probe briefly treats the zombie as alive. It self-corrects on a later call, and the dominant case (immediate crash) is handled correctly. A future hardening could store the job id in a pidfile and verify identity, not just existence — out of scope here.

Test

  • Spawn a task, kill -9 the worker, run another task in the same cwd → previously throws forever; now the dead job is reconciled to failed and the new task proceeds.
  • Live job unaffected: a running worker still correctly blocks with the original message.

node --check passes on the patched file.

…heck

resolveLatestTrackedTaskThread() throws "Task <id> is still running" on any
queued/running job without checking whether the worker process is alive. When
a task worker crashes, its record stays running forever and permanently blocks
the next task call in that cwd (openai#222 root cause; openai#202/openai#164 symptoms).

Before the blocking throw, probe the worker pid with process.kill(pid, 0)
(ESRCH => dead, EPERM => alive). If dead, reconcile the job to failed (writing
state.json, the store the gate reads, plus a best-effort per-job mirror) and
fall through to normal resolution; only throw if the worker is actually alive.

Minimal: ~12 lines + two helpers, touching only the gate. No deadline/daemon.
Known limitation: PID-based probe (a recycled pid is briefly treated as alive;
self-corrects next call). Builds on the approach in the conflicting openai#216.
@storyarcade
storyarcade requested a review from a team June 21, 2026 21:37

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99390fd4b2

ℹ️ 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".

}
// Worker pid is dead -> the "running" record is a zombie. Reconcile it so it
// never blocks the gate again, then fall through to normal resolution.
reconcileDeadJob(workspaceRoot, activeTask);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep checking for live active tasks after reaping one

When the newest active task has a dead PID but an older task in the same session is still queued/running, this reconciles only the single .find result and then falls through to findLatestResumableTaskJob. Fresh background tasks can be launched without passing through this resume gate, so multiple active task records are possible; in that state task --resume-last can resume a completed thread while the older live task is still running instead of preserving the existing "still running" block. After reaping a zombie, reload or loop over the remaining active tasks before selecting a resumable thread.

Useful? React with 👍 / 👎.

@ghbaud

ghbaud commented Jun 28, 2026

Copy link
Copy Markdown

This is a clean, minimal fix for the blocking symptom in #202, and process.kill(pid, 0) is the right portable primitive for it.

One scope note, in case the intent is for this to also close the #222 root cause and not just #202: the reconcile happens at the resume gate via resolveLatestTrackedTaskThread(), which reads state.json. That covers every job that made it into state.json. It does not cover a worker that dies before it registers there — one that crashes after writeJobFile() but before upsertJob(). That job exists only as an orphaned per-job file with a stale running/queued status and a dead pid, and it never appears in state.json at all.

That variant doesn't trigger #202's blocking failure — the gate never finds the job, so nothing gets blocked — which is exactly why it's reasonable to leave it out of a #202-focused change. But it is still part of the #222 root cause: the dead job stays invisible to /codex:status and /codex:result, because both of those also start from state.json. So if this lands and #222 is closed along with it, that never-registered case would quietly remain.

I hit this exact variant on Windows 11 and wrote it up here: #222 (comment). The patch I'm running locally adds a small sweep over the per-job files that finds entries missing from state.json with a dead pid and reconciles them to failed, run at the status/result read paths. It's complementary to this PR rather than competing, since it touches the read paths instead of the resume gate. Happy to open it as a follow-up on top of this if that's useful.

axisrow added a commit to axisrow/codex-plugin-cc that referenced this pull request Aug 3, 2026
…rk in-process crashes failed immediately (#52)

Смерджено в ручную

* fix: repair the tsc build broken by interruptAppServerTurn's JSDoc/param 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

* fix: reap detached workers that die before reaching 'running', and mark 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

---------

Co-authored-by: axisrow <axisrow@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants