fix(core): stop reconciliation thread + mark batch FAILED when execution raises (#763)

[frankbria]

Closes #763

Problem

_execute_serial / _execute_parallel called reconcile_stop.set() only on the success path. An unexpected worker/setup error (e.g. create_execution_context failing) leaked the polling reconciliation daemon thread and left the batch row RUNNING forever in the long-lived server.

Fix

• Move the reconcile stop signal into finally — guaranteed teardown whether execution succeeds or raises.
• Add an except Exception block that marks the batch FAILED, emits the terminal BATCH_FAILED event, then re-raises so callers keep their existing propagation/fallback behavior (the auto-strategy path still falls back to serial on a non-cycle error).

Applied symmetrically to both _execute_serial and _execute_parallel.

Acceptance criteria

• reconcile_stop.set() moved into finally
• Unexpected worker exceptions mark the batch FAILED

Tests

Added test_unexpected_exception_marks_failed_and_stops_reconcile: forces a worker to raise, asserts the batch ends FAILED (not stuck RUNNING) and the reconcile event is set (no daemon leak). uv run pytest tests/core/test_conductor.py → 65 passed (1 unrelated pre-existing database is locked contention flake under the full 26-min serial run; passes in isolation). ruff + mypy clean.

Known limitations

On the auto-strategy fallback path, marking FAILED before re-raising is transient — the fallback serial run immediately overwrites it with the true terminal status. Harmless; the FAILED marking only sticks when the exception is genuinely unhandled.

[github-actions]

Claude finished @frankbria's task in 6m 45s —— View job

---

Bug-hunting review of #763

• Pull PR context + full diff (gh pr view / gh pr diff)
• Check existing comments (no prior findings to duplicate)
• Read conductor.py execution paths + reconcile thread + callers/callees
• Confirm each suspect against real code (emit_for_workspace is an unguarded INSERT+commit; the webhook dispatch swallows its own errors; create_execution_plan runs before the try so CycleDetectedError is unaffected)
• Post inline comments for confirmed defects
• Post summary table

Scope: concrete defects only (logic errors, error-path data loss, regressions). Style/architecture/test-coverage left to the general review.

Summary

severity	file:line	finding
major	codeframe/core/conductor.py:1606 (_execute_serial)	Over-broad except Exception wraps the whole finalization tail. An error raised after the terminal status is already persisted — e.g. the unguarded events.emit_for_workspace INSERT raising sqlite3.OperationalError("database is locked") at line 1580 — flips a correct COMPLETED/PARTIAL record to FAILED and emits a spurious BATCH_FAILED. Regression vs. pre-PR behavior (which left the persisted status intact).
major	codeframe/core/conductor.py:1814 (_execute_parallel)	Identical defect on the parallel path (post-save emit_for_workspace at line 1786). Same regression.

Both have inline comments with drop-in suggestion blocks that guard the FAILED transition on the persisted status (only flip to FAILED when no terminal status was saved yet — the actual #763 "stuck RUNNING" case).

What's correct here: moving reconcile_stop.set() / _reconcile_stop_p.set() into finally is the right fix for the daemon-thread leak and is not in dispute — the finding is only about the unconditional FAILED overwrite inside the new except.

[frankbria]

Demo verification (outcome evidence)

Non-web correctness fix — evidence comes from driving the real batch failure path via test_unexpected_exception_marks_failed_and_stops_reconcile, which patches a worker to raise RuntimeError("boom") and observes the results (not just "exited 0"):

Acceptance criterion	Observed outcome
reconcile_stop.set() moved into finally	After the worker raises, captured["event"].is_set() is True — the reconciliation daemon was signalled to stop (no leak). Diff shows the .set() now lives in finally, removed from the success path.
Unexpected worker exceptions mark the batch FAILED	list_batches(status=RUNNING) == [] and exactly one batch is FAILED — no longer stuck RUNNING. Exception still propagates (pytest.raises(RuntimeError)), preserving caller fallback semantics.

tests/core/test_conductor.py::TestBatchExecution::test_unexpected_exception_marks_failed_and_stops_reconcile PASSED

[claude]

Review

Solid fix for the core leak (reconcile thread never stopped + batch stuck RUNNING on an unhandled worker exception) — moving reconcile_stop.set()/config_watcher.stop() into finally is the right guarantee, and the new regression test (test_unexpected_exception_marks_failed_and_stops_reconcile) exercises the scenario end-to-end rather than just checking exit codes. Catching Exception (not BaseException) so KeyboardInterrupt/SystemExit still propagate is also correct.

One correctness concern and a couple of scope/coverage notes:

1. The try/except scope is wider than the stated intent, and can downgrade an already-successful batch to FAILED

In both _execute_serial (codeframe/core/conductor.py:1442-1615) and _execute_parallel (:1684-1823), the try wraps not just the execution loop but the entire finalization block — status determination, _save_batch, events.emit_for_workspace, and _run_batch_level_validation. _save_batch and events.emit_for_workspace do direct DB writes, and the PR's own description notes a database is locked contention flake was observed in this exact test suite. If either call raises after batch.status has already been correctly set to COMPLETED/PARTIAL/CANCELLED and persisted (e.g. line 1577/1783's _save_batch succeeds, then the events.emit_for_workspace right after it at line 1580/1786 hits a transient lock), the new except Exception block will unconditionally overwrite batch.status back to FAILED, re-save it, emit a spurious BATCH_FAILED, and re-raise — turning a batch where every task actually completed into a persisted FAILED record.

The added test only forces the failure inside _execute_task_subprocess (mid-loop), so it doesn't cover this finalization-phase case. Suggest either narrowing the try to wrap just the execution loop (leave status-determination/save/emit outside it), or guarding the handler so it only overwrites batch.status/emits BATCH_FAILED if the batch isn't already in a terminal state.

2. Same 'stuck forever' bug class is still open in the resume/retry paths

_execute_serial_resume (called from resume_batch, line 1013) and _execute_retries (called from start_batch, line 735) have no try/except at all. resume_batch explicitly sets batch.status = BatchStatus.RUNNING at line 1008 right before calling _execute_serial_resume — if a worker raises there, the batch is left pinned at RUNNING indefinitely, the same root symptom issue 763 describes for the serial/parallel paths. Might be intentionally out of scope for this PR, but worth a fast follow-up issue since it's the identical failure mode.

3. Test coverage is serial-only

The fix is duplicated by hand across _execute_serial and _execute_parallel (pre-existing duplication pattern in this file, not introduced here), but the new test only exercises the serial branch. A copy-paste slip in the parallel copy (e.g. wrong stop-event variable) wouldn't be caught. Worth mirroring the test for _execute_parallel given how easy that duplication is to drift.

Minor: If _save_batch/events.emit_for_workspace inside the new except block itself raises (plausible under the same DB contention the PR description mentions), the secondary exception propagates and masks the original error in logs/traceback — finally still guarantees the thread/watcher cleanup, so no leak, but debugging the real cause gets harder.

No security concerns; nothing here touches user input or auth boundaries.

[github-actions]

Claude finished @frankbria's task in 5m 53s —— View job

---

Bug-hunting re-review of #763 (f93d1b4)

Triggered by the new commit scoping the FAILED override. Scope: concrete defects only, no repeat of already-posted/fixed findings.

• Pull PR context + full diff (gh pr view / git diff origin/main...HEAD)
• Confirm prior findings (2 majors) were actually resolved by f93d1b4
• Read both _execute_serial (:1410) and _execute_parallel (:1628) finalization + new except/finally
• Trace exception propagation into the parallel except (test covers serial only)
• Verify CycleDetectedError and auto-strategy fallback interactions
• Post verdict

Verdict: ✅ GLM review: no new defects found in f93d1b4.

The follow-up commit correctly resolves the two major findings from the earlier pass:

prior finding	resolution in f93d1b4	verified
major — _execute_serial:1606 over-broad except flips a persisted COMPLETED/PARTIAL to FAILED	batch_finalized flag set right after the terminal _save_batch (:1579); except guarded by if not batch_finalized (:1609)	✅
major — _execute_parallel:1814 identical defect	same guard, parallel path (:1789, :1822)	✅

What I confirmed against the real code (not just the diff):

• Guard placement is correct. In both paths the flag flips to True on the line immediately after _save_batch (serial :1578→1579, parallel :1788→1789), so a raise in the post-save tail — the completion-event emit, the webhook dispatch, or the summary logging — lands in the except with batch_finalized == True and the persisted terminal status is preserved. CycleDetectedError is safe regardless: create_execution_plan (:1656) runs before the try.
• The parallel except actually fires on worker crashes — the new test only exercises serial, so I traced the parallel path. _execute_single_task (:1906) does not catch errors from _execute_task_subprocess/create_execution_context, and _execute_group_parallel consumes futures via future.result() (:2098), which re-raises worker exceptions. So both the single-task and thread-pool branches propagate into _execute_parallel's except (:1815). No copy-paste slip in the parallel copy.
• finally teardown is unconditional. reconcile_stop.set() (:1623) / _reconcile_stop_p.set() (:1836) run regardless, and the stop-events are assigned before each try, so no NameError/leak path exists. Catching Exception (not BaseException) correctly lets KeyboardInterrupt/SystemExit propagate.

Not re-reported (already raised or explicitly acknowledged): the resume/retry paths (_execute_serial_resume, _execute_retries) still have no try/except — same stuck-RUNNING class, but out of scope here and flagged in the earlier review as a fast follow-up; serial-only test coverage; the minor exception-masking if _save_batch/emit_for_workspace inside the new handler itself raises; and the transient FAILED on the auto-strategy fallback (documented in the PR body's "Known limitations," final status is correct).

[claude]

Review (follow-up)

Looked at the latest commit (f93d1b4) on top of the state my previous review covered. It resolves the correctness issue I flagged earlier — confirmed by reading the current code, not just the description.

Point 1 from my prior review is fixed. The batch_finalized flag is set immediately after each terminal _save_batch call (conductor.py:1579 in _execute_serial, :1789 in _execute_parallel), and the except Exception handler now only overwrites batch.status to FAILED when that flag is still False. So an error in the finalization tail (events.emit_for_workspace hitting a database is locked, etc.) after a correct COMPLETED/PARTIAL is persisted will no longer flip it to FAILED. This is symmetric across both _execute_serial and _execute_parallel — verified line-by-line, both apply the guard the same way. The bug-hunt review's inline findings on this same issue are addressed by the same change.

The new test_finalization_tail_error_preserves_terminal_status test is a good regression guard: it patches _dispatch_batch_completed_webhook wholesale so the mock's RuntimeError bypasses that function's own internal try/except (worth noting for future readers — the real trigger for this scenario in production is more likely the unguarded events.emit_for_workspace INSERT than the webhook dispatch, which already swallows its own errors and logs a warning). The test still exercises the right code path in _execute_serial regardless.

Still open from my prior review (not new, not blocking this PR, just flagging they weren't addressed here):

1. _execute_serial_resume (resume_batch → line ~1013) and _execute_retries (start_batch → line ~735) still have no try/except/finally around their loops. resume_batch sets batch.status = RUNNING right before calling _execute_serial_resume, so a worker exception there still leaves the batch stuck RUNNING forever — same root bug as [P2.14] Reconciliation thread leaks (batch stuck RUNNING) when execution raises — stop_event.set() not in finally #763, just on a different code path. Worth a fast follow-up issue since it's the identical failure mode this PR fixes elsewhere.
2. Test coverage for the exception-handling behavior is still serial-only — both new tests call start_batch(..., strategy="serial"). I traced the parallel path by hand and the fix is correctly mirrored, but there's no automated guard against future drift between the two (e.g., a copy-paste slip in the strategy field of the BATCH_FAILED event payload, or the wrong stop-event variable).
3. Minor, unchanged: if _save_batch/events.emit_for_workspace inside the except block itself raises (plausible under the same DB contention noted in the PR description), the secondary exception masks the original traceback in logs. finally still guarantees thread/watcher cleanup either way, so no leak — just a debuggability wrinkle.

No security concerns. Nothing here touches auth or user input boundaries.