fix: let /codex:cancel reach turns orphaned by a SIGKILLed companion - #49
Merged
Conversation
Owner
Author
🔍 Local review (cycle 1) — round af8f360a-c4ba-4e0f-8f96-2f99cd9240d1Reviewed locally (
|
axisrow
added a commit
that referenced
this pull request
Aug 2, 2026
…ncellation /codex:cancel was finalizing an orphaned job (pid already nulled by reconciliation) as "cancelled" even when interruptAppServerTurn failed to reach the remote broker/turn. settleCancellationAfterTermination treats a null pid as "stopped" unconditionally, so a failed or never-attempted interrupt still produced exit 0 and rewrote the job to cancelled — masking the fact that the detached Codex turn may still be running with no way to reach it again, since a "cancelled" job no longer matches isOrphanedTurn's retry gate. For the orphan path specifically, require interrupt.interrupted to be true before persisting "cancelled". On failure, restore the exact orphan marker (status: "failed" + the reconciliation error message) so a retried /codex:cancel can select and re-attempt the same job via isOrphanedTurn, and surface the interrupt failure via a thrown error instead of a false success. The existing pid-based termination path for live foreground jobs is unaffected. Found by Codex adversarial review during cycle-review triage of #49. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ph4oGsw1WyeGmrckaDoGkb
Owner
Author
🔍 Local review (cycle 2) — round 0619fdf0-341a-4a0a-85cd-00566f3060fbReviewed locally (
|
axisrow
added a commit
that referenced
this pull request
Aug 2, 2026
…ter it succeeds Round-2 review found that the orphan-cancellation gate added in the previous commit still had a crash window: persistCancellation() wrote "cancelled" to disk BEFORE the (unbounded) await on interruptAppServerTurn(), and the rollback only ran once that await resolved. A hung broker/transport, or the companion process being killed while still awaiting the interrupt, left the job permanently marked "cancelled" with the rollback never executing — the exact false-success class of bug this PR exists to fix, now surfaced in /codex:cancel itself. Two changes: - interruptAppServerTurn() now bounds its turn/interrupt RPC with a 15s default timeout (CODEX_INTERRUPT_TIMEOUT_MS override), since it's a control-plane request, not agent work, and must not hang the way a turn can. - handleCancel()'s orphan path is reordered: attempt the interrupt first, and only call persistCancellation() if it actually succeeds. No job file is touched at all while the interrupt is in flight, so there is nothing to roll back — a failed or timed-out interrupt simply leaves the existing orphan-failed state untouched. The non-orphan (live pid) path is unchanged. Added a stalled-interrupt fake-codex behavior (mirrors the existing stalled-initialize pattern) and a regression test proving cancel does not hang and does not persist "cancelled" when the remote interrupt never responds. Found by Codex adversarial review during cycle-review round 2 of #49. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ph4oGsw1WyeGmrckaDoGkb
Owner
Author
🔍 Local review (cycle 3, final) — round 681d9dc6-b596-4472-a6c0-d8938d1a33cfReviewed locally (
Why HALLUCINATION: Empirically disproven: net.Socket defaults to allowHalfOpen:false (unchanged by this codebase), so a local end() call unilaterally tears the connection down regardless of peer behavior — verified both by a standalone socket test (closes in ~1ms against a peer that ignores writes and never responds) and by the existing stalled-interrupt regression test, which drives this exact path end-to-end against a real never-responding fake broker and completes in a few seconds, not indefinitely.
No FIX or UNVERIFIED verdicts this round — this is the final review cycle for this PR. |
Owner
Author
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 1 HALLUCINATION (refuted), 0 SKIP, 0 UNVERIFIED. Local review ( |
A foreground run killed by Claude Code's Bash tool leaves the underlying Codex turn running inside the detached broker. The job is lazily reconciled from "running" to "failed" on the next listJobs call, and resolveCancelableJob only considered "queued"/"running" jobs cancelable — so the orphaned turn became permanently unreachable through /codex:cancel even though interruptAppServerTurn already supports attaching to a busy broker via threadId/turnId. Widen the cancel eligibility filter to also accept jobs reconciled to "failed" with the unreported-process-exit message when they still carry a threadId and turnId, since those ids mean the underlying app-server turn may still be live and reachable for interrupt. No change to reconciliation or status display: the job record is still accurately "failed", cancel just gets a second path to reach it. Closes #42. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ph4oGsw1WyeGmrckaDoGkb
…ncellation /codex:cancel was finalizing an orphaned job (pid already nulled by reconciliation) as "cancelled" even when interruptAppServerTurn failed to reach the remote broker/turn. settleCancellationAfterTermination treats a null pid as "stopped" unconditionally, so a failed or never-attempted interrupt still produced exit 0 and rewrote the job to cancelled — masking the fact that the detached Codex turn may still be running with no way to reach it again, since a "cancelled" job no longer matches isOrphanedTurn's retry gate. For the orphan path specifically, require interrupt.interrupted to be true before persisting "cancelled". On failure, restore the exact orphan marker (status: "failed" + the reconciliation error message) so a retried /codex:cancel can select and re-attempt the same job via isOrphanedTurn, and surface the interrupt failure via a thrown error instead of a false success. The existing pid-based termination path for live foreground jobs is unaffected. Found by Codex adversarial review during cycle-review triage of #49. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ph4oGsw1WyeGmrckaDoGkb
…ter it succeeds Round-2 review found that the orphan-cancellation gate added in the previous commit still had a crash window: persistCancellation() wrote "cancelled" to disk BEFORE the (unbounded) await on interruptAppServerTurn(), and the rollback only ran once that await resolved. A hung broker/transport, or the companion process being killed while still awaiting the interrupt, left the job permanently marked "cancelled" with the rollback never executing — the exact false-success class of bug this PR exists to fix, now surfaced in /codex:cancel itself. Two changes: - interruptAppServerTurn() now bounds its turn/interrupt RPC with a 15s default timeout (CODEX_INTERRUPT_TIMEOUT_MS override), since it's a control-plane request, not agent work, and must not hang the way a turn can. - handleCancel()'s orphan path is reordered: attempt the interrupt first, and only call persistCancellation() if it actually succeeds. No job file is touched at all while the interrupt is in flight, so there is nothing to roll back — a failed or timed-out interrupt simply leaves the existing orphan-failed state untouched. The non-orphan (live pid) path is unchanged. Added a stalled-interrupt fake-codex behavior (mirrors the existing stalled-initialize pattern) and a regression test proving cancel does not hang and does not persist "cancelled" when the remote interrupt never responds. Found by Codex adversarial review during cycle-review round 2 of #49. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ph4oGsw1WyeGmrckaDoGkb
axisrow
force-pushed
the
fix/issue-42-cancel-orphaned-turn
branch
from
August 3, 2026 01:27
debc110 to
f67a09f
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reconcileRunningJobslazily flips the job fromrunningtofailed, and/codex:cancel's eligibility filter only acceptedqueued/runningjobs — so the orphaned turn became permanently unreachable through the plugin.resolveCancelableJob(plugins/codex/scripts/lib/job-control.mjs) now also treats a job as cancelable when it was reconciled tofailedwith theUNREPORTED_PROCESS_EXIT_MESSAGEand still carries athreadId/turnId— those ids mean the underlying app-server turn may still be alive, andinterruptAppServerTurnalready supports attaching to a busy broker to interrupt it.UNREPORTED_PROCESS_EXIT_MESSAGEfromstate.mjssojob-control.mjscan recognize the reconciliation marker without re-deriving it./cancelcan't reach the orphan). The broker-side (turn/interrupton socket close) and companion-side (SIGTERM/SIGINThandlers) mitigations from the issue are independent and not included here.Test plan
cancel reaches a job orphaned by a SIGKILLed companion process (closes #42)intests/runtime.test.mjs, which reproduces the real sequence: a job record with a dead pid, reconciled tofailedvia/codex:status, then successfully cancelled via/codex:cancel <id>.No job found for "task-orphaned") and passes with it.npm test— 179/182 passing; the 3 failures are pre-existing onmain(unrelatedstatus/resultrendering tests), confirmed identical failure set before and after this change.Closes #42.