codex-companion.mjs: three reliability bugs in background review handling

[bradygrapentine]

Reporting three bugs in scripts/codex-companion.mjs (vendored at ~/.claude/plugins/marketplaces/openai-codex/plugins/codex/scripts/codex-companion.mjs, ~1007 lines) that have caused silent failures in production use. All three were root-caused from .codex-runs/ history across multiple projects.

Bug 1: --background flag declared but never read

handleReviewCommand declares the --background flag in its options block but never reads options.background to dispatch through the queue. As a result, reviews intended to run in the background actually run foreground.

Symptom: caller (Claude Code) expects a {jobId} JSON response to track the queued review; instead it gets the raw log stream of a foreground execution, which is unparseable.

Correlated incident: lease-analyzer wave9 — kickoff returned log stream instead of {jobId} JSON, marked as unparseable-no-jobid in audit.

Fix: wire options.background into handleReviewCommand; add enqueueBackgroundReview and a review-worker subcommand that the queue dispatches to.

Bug 2: Detached workers can die mid-execution with no liveness check

spawnDetachedTaskWorker uses detached: true, stdio: "ignore" and does not poll kill(pid, 0) afterward. If the worker dies (OOM, signal, runtime crash), the job record stays stuck in "queued" or "running" forever. Status/result handlers never detect the death.

Correlated incident: lease-analyzer wave41 — pid 13224 died mid-review before result harvest. Orchestrator showed the review as in-flight for ~1.5 hours before manual intervention.

Fix: add kill(pid, 0) liveness check in status and result handlers; if the pid is dead and no result file exists, mark the job as failed-worker-died with the last-known stderr (if captured).

Bug 3: Quota errors classified as infrastructure failures

ensureCodexReady does not distinguish quota-exhaustion errors from infrastructure errors. When the org hits its monthly Codex usage limit, the error path is the same as a hung process or unreachable runtime, so codex:rescue treats it as something to retry/recover instead of surfacing the actual blocking reason.

Correlated incident: carelog TD-84 — monthly usage limit misclassified as infra hang, escalation noise instead of "quota — wait for billing cycle reset".

Fix: add a quota-specific error class. Pattern-match the upstream error message; record quota-exhausted in the job record; have codex:rescue short-circuit and report rather than retry.

---

Happy to open separate issues per bug if that's preferred for triage. Filing combined since the diagnostic context overlaps and a single PR could plausibly address all three (auditing background/queue dispatch + worker lifecycle + error classification together).

[Glebuar]

Confirming Bug 1 from an independent root-cause pass, and adding two adjacent failure modes in the same review --background path along with a tested fix. (Can't speak to Bugs 2/3.)

Bug 1 confirmed: review --background is parsed but never dispatched

handleReviewCommand declares booleanOptions: ["json", "background", "wait"] but never reads options.background / options.wait — it unconditionally takes the foreground path. So review --background runs the review in-process, foreground, and a caller expecting a {jobId} gets the raw foreground log stream instead. The only if (options.background) branch and the spawnDetached* / enqueueBackground* machinery live in the task command. Matches your finding exactly.

Two adjacent failure modes this opens

(A) A capped output stream wedges the (still-foreground) review at running with no verdict.
Because --background is a no-op, a caller treats the output as throwaway and caps it (e.g. review --background … | head -N) — but the review is actually still streaming foreground. createProgressReporter (lib/tracked-jobs.mjs) does an unguarded synchronous process.stderr.write(...) per progress event, before it appends to the job log / updates job state. When head exits and closes the pipe, the next progress write hits a dropped reader. On a pipe-backed stderr the EPIPE surfaces asynchronously as an 'error' event (not a synchronous throw), so a try/catch around the write wouldn't even catch it — the in-process review loop is orphaned and the job sits at running forever (the broker/app-server is a separate daemon, so nothing marks it done). Reproduces reliably with … | head; completes fine whenever the stream is fully drained (foreground, or … | tail).

(B) Spawn-before-persist can orphan a background job at queued.
enqueueBackgroundTask spawns the detached worker before writeJobFile / upsertJob. The worker's readStoredJob can run before the record exists → it throws and exits → job stuck at queued. The window is tiny (the parent's two synchronous writes vs. the child's Node boot), but it's correct-by-construction wrong and is exactly the class this issue is about.

Tested fix (shape)

Working and verified end-to-end (native background review reaching completed with a retrievable result; … | head no longer wedges; an EPIPE smoke test survives with all progress/state updates intact):

• Native review dispatch. Generalize spawnDetachedTaskWorker → spawnDetachedWorker(cwd, jobId, subcommand) and enqueueBackgroundTask → enqueueBackgroundJob(…, workerSubcommand = "task-worker") (default keeps the existing task call sites byte-identical), add buildReviewRequest + handleReviewWorker + a review-worker case in the main switch, branch handleReviewCommand on options.background, and reject --background --wait. This is essentially your proposed "add enqueueBackgroundReview and a review-worker subcommand."
• Reporter hardening (A). In createProgressReporter, do the log/state updates first, then best-effort stderr via try/catch + a one-time process.stderr.on('error', …) swallow (for the async EPIPE) + a per-reporter "stop after first failure" latch. Diagnostic stderr can no longer abort the loop or block durable updates.
• Persist-before-spawn (B). Write the queued record (with its request payload) before spawning, then patch pid after spawn (the parent's writes land well before the freshly forked worker boots, so they can't clobber its running/terminal state).

Relation to #346 and a caveat

#346 already ships essentially the persist-before-spawn + --background/--wait mutual-exclusion mechanism — but scoped to task, leaving review/adversarial-review foreground. So the review-side dispatch above is best framed as extending #346's ordering to the review path. Two things should land before any review detachment ships, or a detached review worker can be orphaned-after-spawn or hang:

• dead-PID liveness reconcile (kill(pid, 0) on status reads → mark failed), as in fix(runtime): prevent tracked jobs hanging forever on broker disconnect #184 / Fix #202: zombie job blocks subsequent task calls when process crashes #216 / fix: handle zombie jobs when process crashes but status remains "running" #218 / Companion reliability overhaul: deterministic completion, cancellation-safe background jobs (task + review), and explicit thread resume #346 — none of which currently protect a review job;
• a wall-clock / turn watchdog (fix(runtime): prevent tracked jobs hanging forever on broker disconnect #184 / feat(codex): per-turn watchdog with TurnWatchdogError + exit 124 #312 / feat(adversarial-review): make self-collect path multi-turn so it can actually work #328) so a worker stalled inside captureTurn self-terminates instead of hanging at running.

Happy to open a PR stacked on #346 if that's the preferred path.

[akari-musubi]

Additional reproduction data for Bug 2 (worker death → state stuck running forever), from production use of the task subcommand (plugin 1.0.5):

Repro path we hit: dispatching with foreground task --write … from an automation harness whose shell enforces a 2-minute command timeout. The timeout SIGTERMs the companion wrapper → the detached worker chain dies before runTrackedJob can write a terminal state → the job record shows running indefinitely. In our incident the job had been dead for ~40 minutes while status kept reporting running; a poller watching only status output never noticed. Root cause matches your analysis: runTrackedJob (inside the task-worker) is the only writer of terminal states and has no SIGTERM handler, and status/result never probe the persisted job.pid.

A nuance for the fix design: worker-pid liveness alone may not be sufficient in the other direction. We separately observed a case where the wrapper/worker was killed but the per-git-root broker (app-server-broker.mjs) kept executing the in-flight turn — so a kill(pid, 0) probe on the worker would report "dead" while real work continues in the broker. A robust liveness check probably needs to consider broker/thread state, not just the worker pid.

Mitigations we use meanwhile, in case useful to others:

1. Always dispatch with the native --background flag — avoids the caller-timeout SIGTERM class entirely (and matches the advice in [BUG] codex:codex-rescue hangs in worktrees when run_in_background is used #198).
2. Treat only terminal states (completed / failed / cancelled) as trustworthy — they are written by the worker itself. A reported running is unverifiable on its own.
3. Check liveness via output signals rather than state: job log mtime, pgrep for the broker scoped to the job's --cwd, git diff in the workspace, and kill -0 <job.pid> (the pid is persisted in jobs/<id>.json even though status doesn't use it).

Happy to provide more detail if useful.

[zknpr]

@Glebuar's confirmation and failure-mode analysis both check out — implemented on #346 (which already carried the persist-before-spawn/mutual-exclusion mechanics for task):

• Bug 1 / native review dispatch (f7e5682): review and adversarial-review now honor --background through the same generalized detached-worker machinery as task (worker-subcommand parameter, review-worker), riding Companion reliability overhaul: deterministic completion, cancellation-safe background jobs (task + review), and explicit thread resume #346's locked job-store transactions, worker-side atomic claim, dispatched-sentinel rendering, --background --wait rejection, and — your first pre-detachment prerequisite — dead-PID liveness reconciliation, which now covers review jobs (status/status --wait reconcile a dead review worker to failed). --json returns the {jobId, …} dispatch payload.
• (A) reporter hardening (f847f0d): durable-first ordering (job log + state updates before stderr), stderr best-effort behind a one-time module-level 'error' guard plus a stop-after-first-failure latch — a dropped pipe reader (… | head) can no longer orphan the loop or wedge a job at running.
• (B) persist-before-spawn (6d20be4): on Companion reliability overhaul: deterministic completion, cancellation-safe background jobs (task + review), and explicit thread resume #346's current head the enqueue path already persists the queued record with its complete request in both stores before spawn; pinned with a deterministic test that inspects both stores from inside the intercepted spawn call.

Honest scope note: the wall-clock/turn watchdog (#184/#312/#328) is intentionally not part of this — dead workers now reconcile promptly, but a hung-alive worker (stalled captureTurn) remains an open problem tracked in those issues, and it affects task workers equally today.