fix: reap ghost jobs whose worker died without recording a result#425
fix: reap ghost jobs whose worker died without recording a result#425Greenbauer wants to merge 1 commit into
Conversation
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>
There was a problem hiding this comment.
💡 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".
| phase: "failed", | ||
| errorMessage, | ||
| pid: null, | ||
| completedAt |
There was a problem hiding this comment.
Return reaped jobs with a current updatedAt
When an old running job is reaped, this returned record keeps base.updatedAt while upsertJob writes a fresh updatedAt only to disk. The current /codex:status call sorts the in-memory list immediately after reapDeadJobs, so if the ghost job has more than the default page of newer completed jobs it is omitted from that first status report and the user sees no failed job/resume hint until running status again. Include updatedAt: completedAt (or return the updated state entry) so the first reader reflects the failure it just recorded.
Useful? React with 👍 / 👎.
rajpratham1
left a comment
There was a problem hiding this comment.
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.
Problem
A detached task worker that dies without throwing — an unhandled rejection, an OOM
SIGKILL, a native crash — never reachesrunTrackedJob'scatch, so the job file is never rewritten and the job staysstatus: "running"forever./codex:statusthen reports a ghost job indefinitely.Observed in practice (2026-07-03): a real background task died about 2 minutes in, and
/codex:statuskept 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:
Worker side —
registerWorkerCrashGuard(workspaceRoot, jobId, logFile)(lib/tracked-jobs.mjs, installed inhandleTaskWorkerbeforerunTrackedJob): onuncaughtException,unhandledRejection,SIGTERM,SIGINT, orSIGHUP, appends the reason to the job log, marks the job file failed, and exits.Reader side —
reapDeadJobs(workspaceRoot, jobs)(lib/tracked-jobs.mjs, wrapping everylistJobscall site inlib/job-control.mjs): any queued/running job whose recorded pid fails aprocess.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.Testing
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 unmodifiedmainon macOS and are unrelated to this change.npm run build: clean.🤖 Generated with Claude Code