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

Activity

1. tgkim-openerd

   added a commit that references this issue on May 27, 2026

   fix(review): actually wire --background through to enqueueBackgroundT…

   ...

   a59940e
2. Glebuar commented on Jun 8, 2026

   

   Glebuar

   on Jun 8, 2026

   More actions

   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.
3. akari-musubi commented on Jul 4, 2026

   

   akari-musubi

   on Jul 4, 2026

   More actions

   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.
4. zknpr

   added 3 commits that reference this issue on Jul 16, 2026

   feat(codex): native --background dispatch for review and adversarial-…

   ...

   f7e5682

   fix(codex): progress reporter survives a broken stderr and stays dura…

   ...

   f847f0d

   test(codex): pin persist-before-spawn ordering for background jobs (o…

   ...

   6d20be4
5. zknpr commented on Jul 16, 2026

   

   zknpr

   on Jul 16, 2026

   More actions

   @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.
6. sid-valecha commented on Aug 7, 2026

   

   sid-valecha

   on Aug 7, 2026 – with ChatGPT Codex Connector

   More actions

   Independent v1.0.6 reproduction confirming Bug 1 and adding duration data:

   • I ran roughly 10 review / adversarial-review jobs in one day.
   • Short reviews—2–3 questions, roughly 2 minutes—completed.
   • Longer reviews—roughly 5+ minutes—exited 137 partway through, leaving an empty log and no recoverable entry in status --all or result.
   • handleReviewCommand parses --background and --wait but unconditionally enters runForegroundCommand, so the caller’s lifetime still owns the review despite the advertised flag.

   The duration correlation held across:

   • a 1.3 KB prompt that failed and a 2.4 KB prompt that succeeded,
   • about 45% free memory with 2.5 GB swap available,
   • a task that successfully used git log,
   • two separate worktrees.

   The SIGKILL may come from the invoking harness, but that is precisely the durability boundary --background is expected to provide.

   I also prototyped and tested a focused review-worker path locally. Regression coverage includes queued-to-completed execution and status/result retrieval for both review types, conflicting --wait --background flags, and refusal to resurrect a cancelled queued job. npm run build and all 94 tests pass.

   PR #346 already contains the broader reliability solution, including atomic worker claims and dead-worker reconciliation. This reproduction is additional evidence for prioritizing that PR—or extracting its native background-review commits into a smaller PR.