fix(conductor): crashed/killed task subprocess is retryable (#742)

[frankbria]

Closes #742

Problem

_execute_task_subprocess returned the active run's status verbatim even when the child crashed / was SIGTERMed before finalizing its run row (still RUNNING). The batch then recorded results[task_id]="RUNNING" as terminal. Because resume_batch only re-ran FAILED/BLOCKED/missing tasks, the task was permanently skipped, and the stale RUNNING run made the next start_task_run raise "already has an active run".

Fix

• _execute_task_subprocess: on nonzero exit with the run still RUNNING, reconcile it to FAILED via fail_run before returning. This clears the stale active run (so a later restart isn't blocked) and records a retryable result. Scoped to returncode != 0, which covers both crash exits (positive) and signal kills (negative, e.g. -15 SIGTERM / -9 SIGKILL).
• resume_batch: add "RUNNING" to the retryable set so any legacy batches that already recorded it can be resumed.

Acceptance criteria

• On nonzero exit with the run still RUNNING, the run is reconciled to FAILED before returning.
• "RUNNING" is retryable on resume.

Tests

• test_subprocess_reconciles_stale_running_run_to_failed — real RUNNING run + mocked nonzero-exit Popen → asserts FAILED returned, no stale active run, run row is FAILED.
• test_resume_reruns_stale_running_task — batch that records RUNNING → resume re-runs only that task and it completes.

Full tests/core/test_conductor.py (67) passes; ruff clean.

Review

Pre-PR cross-family review via opencode (GLM): No blocking issues — confirmed fail_run's ValueError contract, the returncode != 0 scoping, and no double-transition/race.

Known limitations

• The returncode == 0 + still-RUNNING edge (child exited clean but never finalized) is a separate child-side bug, out of scope here (the criteria scope to nonzero exit). It is retryable via the resume change but costs one wasted resume cycle: the first retry hits the stale active run, self-heals it to FAILED on that retry's nonzero exit, and a second resume actually re-executes. Reconciling a clean-exit run to FAILED is deliberately avoided since it would risk misreporting a task that actually succeeded.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 15 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 556abe49-7b11-42be-bdd7-d87caefe0ca3

📥 Commits

Reviewing files that changed from the base of the PR and between f917017 and 77542c6.

📒 Files selected for processing (2)

• codeframe/core/conductor.py
• tests/core/test_conductor.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/742-crashed-task-retryable

---

_{Comment @coderabbitai help to get the list of available commands.}

[frankbria]

Third-party review (opencode / GLM) — pre-PR

Verdict: No blocking issues. Verified against runtime.py:

• fail_run contract holds — raises ValueError for not-found and already-terminal runs, so except ValueError catches the only realistic TOCTOU race. Child is already dead when process.wait() returns, so the catch is purely defensive.
• returncode != 0 scoping is correct — covers crash exits (positive) and signal kills (negative: -15/-9). The returncode == 0 + still-RUNNING case is a separate child-side bug, out of scope, independently mitigated by the resume change.
• get_active_run returns RUNNING or BLOCKED — reconcile targets only RUNNING; a BLOCKED run falls through to return "BLOCKED", the right retryable signal.
• No double-transition — fail_run also flips the task to FAILED; run + task stay consistent.
• Test exercises the real path — returncode = process.wait(); test sets fake_proc.wait.return_value = 1.

Posted from the automated issue-lifecycle review gate.

[claude]

Review

Small, well-scoped fix: _execute_task_subprocess now reconciles a run that's still RUNNING after a nonzero-exit subprocess to FAILED via fail_run, and resume_batch treats "RUNNING" as retryable. The two new tests exercise exactly the crash-and-reconcile path and the resume-retry path described in the PR body. I dug into the edges the PR itself flags as "out of scope" / "known limitations," and one interaction wasn't fully covered by the stated mitigation — findings below, most severe first.

1. codeframe/core/conductor.py:967-969 + :2167 — The PR's own "Known limitations" note says the returncode == 0 + still-RUNNING edge is "independently mitigated by the resume change," but tracing it through shows the mitigation doesn't actually land on the first retry. When the child exits 0 while its run row is still RUNNING, the new if run.status == RunStatus.RUNNING and returncode != 0 guard doesn't fire, so the function falls through to return run.status.value → "RUNNING", and the DB row is never reconciled. resume_batch's new failed_statuses = {"FAILED", "BLOCKED", "RUNNING"} then re-queues that task, but _execute_serial_resume re-invokes _execute_task_subprocess, which spawns a fresh cf work start <task_id> --execute → start_task_run's active-run guard (raise ValueError("Task already has an active run: ...")) fires immediately since the stale run was never closed. So the retry fails instantly rather than re-executing the task — it just so happens that this failure has a nonzero returncode, so it self-heals the run row and a second resume actually retries. Not a regression from this PR (the edge is pre-existing and explicitly called out), but worth tightening the PR description's limitation note since "mitigated" currently means "mitigated after one wasted resume cycle," not "mitigated."

2. codeframe/core/conductor.py:2170 (via codeframe/core/runtime.py fail_run → tasks.update_status) — fail_run writes the run row to FAILED first, then calls tasks.update_status(..., TaskStatus.FAILED), which is only a legal transition from IN_PROGRESS (ALLOWED_TRANSITIONS in state_machine.py) — any other current task status raises InvalidTransitionError, which extends Exception, not ValueError. The new except ValueError: pass here won't catch it; it propagates to the outer except Exception handler, which still returns "FAILED" but leaves the task's status row un-transitioned (mismatched with the batch result). This is reachable today: codeframe/core/checkpoints.py's restore() writes task status via raw SQL (UPDATE tasks SET status = ?), bypassing the state machine and the runs table entirely — restoring a checkpoint while a task's run is still RUNNING desyncs task status (e.g. to READY/DONE) from the run row, and the next subprocess-crash reconciliation for that task hits the uncaught InvalidTransitionError. Narrow trigger (checkpoint restore racing an active run), but worth either widening the except to also catch InvalidTransitionError (log-and-continue, same as the ValueError case) or noting it as a follow-up.

3. codeframe/core/conductor.py:2175 — except ValueError: pass is silent, while the function's other error path (except Exception as e: at the bottom) does logger.error(...). A double-reconciliation race between concurrent workers hitting fail_run on the same already-terminal run would be swallowed with no log trace at all. Even a one-line logger.debug/logger.info here would preserve the "this is expected and benign" intent while keeping it observable.

4. Altitude (non-blocking observation) — This adds a second, structurally independent reconciliation mechanism: the new one is RunStatus-based and fires inline on subprocess exit; the existing ReconciliationEngine (codeframe/core/reconciliation.py, driven by _start_reconciliation_thread's 30s timer) is purely TaskStatus-based and never looks at Run/RunStatus. Both exist to catch "stale state after something died outside normal control flow." Not asking for a rework in this PR — it's correctly scoped to the crash-on-subprocess-exit case per the issue — but a future maintainer extending crash-recovery to another execution path (e.g. the cloud/E2B engine, which has no subprocess.wait()) now has two independent mechanisms to reconcile with rather than one canonical place.

Nothing here contradicts the pre-PR opencode review's verdict for the core scoped case (returncode != 0, single worker, no checkpoint-restore race) — that path is correctly fixed and tested. These are additional edges surfaced by tracing the "known limitations" section further.

[frankbria]

Thanks — triaged all four:

• Brainstorming: Integrate remaining general concepts into specification #2 (fixed, 223cd3f): broadened the reconcile except ValueError → except Exception with a debug log. fail_run writes the run row to FAILED before the task transition, so the stale active run (the core bug) is cleared even when tasks.update_status raises InvalidTransitionError (e.g. after a checkpoint-restore desync). New test test_subprocess_reconcile_survives_non_valueerror locks it in.
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (fixed, 223cd3f): the catch now logs at debug instead of silently passing.
• Add Claude Code GitHub Workflow #1 (doc): correct — 'mitigated' meant 'after one wasted resume cycle' for the returncode == 0 + still-RUNNING edge. That case is deliberately out of the acceptance criteria (scoped to nonzero exit) since reconciling a clean-exit run to FAILED risks misreporting a task that actually succeeded. Updating the PR limitation note to say so precisely.
• feat: Project schema refactoring with source types and workspace management #4 (noted): agreed the inline RunStatus-based reconcile and the ReconciliationEngine's TaskStatus-based timer are now two mechanisms; correctly scoped out of this PR. Worth a follow-up if crash-recovery extends to the cloud/E2B engine (no subprocess.wait()).

[claude]

Follow-up review

I reviewed the changes made since my previous pass (commit 223cd3f) against the four findings I raised earlier. Status:

• Brainstorming: Integrate remaining general concepts into specification #2 (broaden except ValueError -> except Exception) — fixed correctly. conductor.py:2175 now catches any exception from fail_run and logs it, so an InvalidTransitionError from tasks.update_status (verified: state_machine.InvalidTransitionError extends Exception, not ValueError) no longer propagates. Confirmed fail_run (runtime.py:394-406) commits the run row to FAILED before calling tasks.update_status, so the core guarantee (no stale active run) holds even when the task-transition half fails.
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (silent pass -> observability) — fixed. The catch now does logger.debug(...) with the run id and exception.
• feat: Project schema refactoring with source types and workspace management #4 (dual reconciliation mechanisms) — acknowledged as an accepted, correctly-scoped follow-up; nothing further needed here.
• Add Claude Code GitHub Workflow #1 (doc precision on the returncode == 0 limitation) — the triage comment said the "Known limitations" note would be updated to say the mitigation only lands "after one wasted resume cycle" (first resume still hits start_task_run's "already has an active run" ValueError since the run was never reconciled; only that failure — which is nonzero — self-heals the row for the next resume). Looking at the current PR description, it still reads "independently mitigated by the resume change" without that caveat. Minor, but worth updating since it's the one item marked "doc" rather than "fixed" and readers relying on the description alone will expect single-resume recovery.

New (minor) observation: test_subprocess_reconcile_survives_non_valueerror coverage gap

The test's docstring says "the run row is still cleared" for the non-ValueError case, but the test mocks codeframe.core.runtime.fail_run wholesale (side_effect=RuntimeError(...)), so fail_run's real body — including the UPDATE runs SET status = 'FAILED' write — never executes. The test only proves the outer except Exception in _execute_task_subprocess swallows the error and returns "FAILED"; it doesn't exercise the actual claim that the run row is committed to FAILED before the task-status transition raises. That ordering guarantee (the crux of why broadening the catch is safe) is currently only verified by reading fail_run's source, not by a test. A more faithful test would let fail_run run for real and instead mock tasks.update_status (or the state-machine transition) to raise, then assert get_run(workspace, run.id).status == RunStatus.FAILED and get_active_run(...) is None afterward. Not blocking — the mocked-away version still guards the "don't propagate" behavior — but flagging since it doesn't cover what the docstring claims.

Everything else (the core reconcile logic, resume_batch's retryable-set change, RunStatus/get_active_run interactions) checks out against the current code — no new correctness issues beyond what's noted above.

[frankbria]

Addressed the follow-up's two minor items in 77542c6:

• Test coverage gap: test_subprocess_reconcile_survives_non_valueerror now lets fail_run run for real and mocks only tasks.update_status to raise, then asserts get_run(...).status == FAILED and get_active_run(...) is None. This exercises the actual ordering guarantee (run row committed to FAILED before the task transition raises), not just the swallow-and-return behavior.
• Add Claude Code GitHub Workflow #1 doc note: PR description's Known Limitations now states the returncode == 0 edge is retryable but costs one wasted resume cycle, with the self-heal sequence spelled out.

Thanks for the thorough tracing.

[claude]

Follow-up review (commit 77542c6, current HEAD)

Re-verified the current state of the branch against the four findings from my earlier passes — all are resolved and I traced each claim against the actual source rather than just the PR description:

• Brainstorming: Integrate remaining general concepts into specification #2 (broadened except ValueError -> except Exception) — confirmed at codeframe/core/conductor.py:2167-2176. fail_run (codeframe/core/runtime.py:371-408) does conn.commit() on the runs row before calling tasks.update_status(...) a few lines later, so the ordering guarantee holds: the stale active run is cleared even when the task-status transition raises InvalidTransitionError (or anything else).
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (silent pass -> observability) — confirmed, now logger.debug(f"Reconcile of run {run.id} raised (benign): {e}").
• Add Claude Code GitHub Workflow #1 (doc precision) — the PR description's "Known limitations" section now correctly states the returncode == 0 + still-RUNNING edge costs one wasted resume cycle before self-healing, matching what's actually implemented.
• Test coverage gap — test_subprocess_reconcile_survives_non_valueerror now lets fail_run execute for real and mocks only codeframe.core.runtime.tasks.update_status (verified this is the correct patch target — runtime.py imports tasks directly at module level) to raise, then asserts the run row is FAILED and no active run remains. This actually exercises the ordering guarantee instead of just the swallow-and-return path.

Two things I checked that weren't explicitly raised before, both come back clean:

• _execute_retries (the separate auto-retry loop used by --strategy auto --retry N) still only retries {RunStatus.FAILED.value}, not RUNNING — this is fine, not an inconsistency with resume_batch's new {"FAILED", "BLOCKED", "RUNNING"} set. _execute_retries runs immediately after start_batch in the same process, so any crash within that pass already goes through the new inline reconcile in _execute_task_subprocess and comes back as "FAILED" directly — it never surfaces "RUNNING" to that layer. The resume_batch set exists specifically for legacy batches with a pre-fix "RUNNING" already persisted, per the PR description.
• The lazy from codeframe.core.runtime import fail_run inside the function (vs. the top-level import of RunStatus/get_active_run) mirrors a pre-existing local list_runs import a few lines below in the same function — consistent with existing style in this file, not a new nitpick.

I wasn't able to execute the test suite in this environment (sandbox blocked the pytest invocation), so I verified by reading runtime.py's fail_run/get_active_run and the test bodies directly rather than by running them. Nothing else outstanding — this is good to merge from my side.