Skip to content

fix: reap ghost jobs whose worker died without recording a result#425

Open
Greenbauer wants to merge 2 commits into
openai:mainfrom
Greenbauer:fix/reap-dead-tracked-jobs
Open

fix: reap ghost jobs whose worker died without recording a result#425
Greenbauer wants to merge 2 commits into
openai:mainfrom
Greenbauer:fix/reap-dead-tracked-jobs

Conversation

@Greenbauer

Copy link
Copy Markdown

Problem

A detached task worker that dies without throwing — an unhandled rejection, an OOM SIGKILL, a native crash — never reaches runTrackedJob's catch, so the job file is never rewritten and the job stays status: "running" forever. /codex:status then reports a ghost job indefinitely.

Observed in practice (2026-07-03): a real background task died about 2 minutes in, and /codex:status kept reporting it as running for 28+ minutes until a human noticed the log file's mtime had stopped advancing and found no live worker process.

Fix

Two complementary guards, so the job record converges to the truth from either side:

  1. Worker side — registerWorkerCrashGuard(workspaceRoot, jobId, logFile) (lib/tracked-jobs.mjs, installed in handleTaskWorker before runTrackedJob): on uncaughtException, unhandledRejection, SIGTERM, SIGINT, or SIGHUP, appends the reason to the job log, marks the job file failed, and exits.

  2. Reader side — reapDeadJobs(workspaceRoot, jobs) (lib/tracked-jobs.mjs, wrapping every listJobs call site in lib/job-control.mjs): any queued/running job whose recorded pid fails a process.kill(pid, 0) liveness probe is rewritten as failed with a resume hint (task --resume-last). This catches the cases no in-process handler can (SIGKILL, hard native crashes). Details:

    • ESRCH ⇒ dead; EPERM ⇒ process exists but isn't ours, treated as alive.
    • The job file is re-read before rewriting, so a job that finished between the caller's read and the probe keeps its real result.
    • Jobs with no recorded pid are left untouched.

Testing

  • New tests/tracked-jobs.test.mjs: dead-pid job is reaped to failed (job file + state index), live-pid job untouched, pid-less job untouched, finished-during-probe race keeps the stored result, and a spawned worker with the crash guard that hits an unhandled rejection exits 1 with the job file marked failed.
  • npm test: the 5 new tests pass; 91/95 overall — the 4 failures (status shows phases…, status preserves adversarial…, result returns the stored output…, resolveStateDir uses a temp-backed…) fail identically on unmodified main on macOS and are unrelated to this change.
  • npm run build: clean.

🤖 Generated with Claude Code

A detached task worker that dies without throwing (unhandled rejection,
OOM kill, native crash) never reaches runTrackedJob's catch, so its job
file stays status:"running" forever and /codex:status reports a ghost
job indefinitely.

Two complementary guards:

- registerWorkerCrashGuard (worker side): installed in handleTaskWorker
  before runTrackedJob; marks the job failed on uncaughtException,
  unhandledRejection, SIGTERM, SIGINT, or SIGHUP, then exits.
- reapDeadJobs (reader side): wraps every listJobs call in job-control
  so status/result/cancel probe each active job's recorded pid with
  process.kill(pid, 0); ESRCH means the worker is gone and the job is
  rewritten as failed with a resume hint. The job file is re-read first
  so a job that finished between the read and the probe keeps its real
  result, and EPERM (alive but not ours) is treated as alive.

Test run: node --test — 5 new tests pass; the 4 pre-existing failures on
macOS (tmpdir symlink) are unchanged from main.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Greenbauer Greenbauer requested a review from a team July 4, 2026 01:52

@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: 8a7d9bf376

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

Comment thread plugins/codex/scripts/lib/tracked-jobs.mjs Outdated

@rajpratham1 rajpratham1 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.

This fixes a real problem and has solid test coverage, but I agree with the reviewer's concern and found one issue that should be addressed before merge.

Blocking issue

updatedAt is stale after reaping

markJobDead() creates:

const completedAt = nowIso();
const record = {
...
completedAt
};

Then upsertJob() updates the persisted state with a fresh updatedAt, but the returned record still contains the old updatedAt (or none at all).

Immediately afterwards, callers do:

sortJobsNewestFirst(reapDeadJobs(...))

So the in-memory list is sorted using stale timestamps, meaning a freshly reaped failed job may still appear far down the list until the next /codex:status.

The returned object should include:

updatedAt: completedAt

or return the fully updated state after upsertJob().

What's good
Fixes "ghost" jobs that stay in running forever after crashes.
Uses process.kill(pid, 0) correctly to probe process existence.
Treats EPERM as "alive", avoiding false failures.
Handles race conditions where another process finishes the job before it is reaped.
Crash guard covers:
uncaughtException
unhandledRejection
SIGTERM
SIGINT
SIGHUP
Comprehensive regression tests for:
dead PID
live PID
missing PID
race with completed job
unhandled rejection
Minor suggestion

reapDeadJobs(workspaceRoot, listJobs(workspaceRoot)) is invoked in several places. If job counts grow, consider reaping once per command and reusing the result instead of repeatedly reading and rewriting job state.

markJobDead built the returned record by spreading base, which kept the
old updatedAt. upsertJob wrote a fresh updatedAt only to the state index,
so the in-memory record (and the per-job file) still carried the stale
timestamp. Callers sort the reaped list with sortJobsNewestFirst (keyed
on updatedAt), so a ghost job with more than a page of newer completed
jobs ahead of it could be paged out of the first /codex:status report —
the user would see no failed job or resume hint until running status
again.

Set updatedAt: completedAt on the record so the first reader reflects the
failure it just recorded. Adds a regression test.

Addresses review feedback from @chatgpt-codex-connector and @rajpratham1.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Greenbauer

Copy link
Copy Markdown
Author

Thanks @rajpratham1 — good catch, both fixed/addressed in dc005e8.

Blocking issue — stale updatedAt after reaping: fixed. markJobDead now sets updatedAt: completedAt on the returned record, so the reaped job carries a current timestamp in both the in-memory list sortJobsNewestFirst sorts on and the per-job file written by writeJobFile. A freshly reaped failed job now sorts newest-first in the same read that recorded it. Added a regression test asserting the reaped job's updatedAt is refreshed and matches completedAt.

Minor suggestion — reap once per command: each of the four reapDeadJobs(workspaceRoot, listJobs(...)) sites lives in a separate command entry point (buildStatusSnapshot, buildSingleJobSnapshot, resolveResultJob, resolveCancelableJob) — they don't call one another, so a single /codex:status, /codex:result, or /codex:cancel invocation reaps exactly once. The liveness probe is a bare process.kill(pid, 0) per active job (no I/O), and a rewrite only happens for jobs actually found dead, so the steady-state cost is one syscall per active job. Happy to hoist it to a single shared reap if you'd prefer, but as-is there's no repeated reaping within one command.

@Greenbauer

Copy link
Copy Markdown
Author

@codex review

@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: dc005e8815

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

Comment on lines +286 to +287
for (const sig of ["SIGTERM", "SIGINT", "SIGHUP"]) {
process.on(sig, mark(`received ${sig}`));

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 Preserve cancelled jobs when SIGTERM is delivered

When cancelling a background task, handleCancel sends SIGTERM via terminateProcessTree and then writes the job as cancelled; with this new handler installed, the worker can process that same SIGTERM after the cancel command writes its update and markJobDead will rewrite the job back to failed. This makes normal /codex:cancel operations race into a failed status/result instead of cancelled, so intentional teardown signals should not use the crash-failure path or should detect an existing cancellation before writing.

Useful? React with 👍 / 👎.

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