fix(perf): run batch execution off the uvicorn event loop (#901, P0.7)

[frankbria]

Closes #901 (P0.7 — critical / perf, filed by the SaaS launch review).

Problem

POST /api/v2/tasks/execute and /tasks/approve called conductor.start_batch(...) directly from an async def handler. start_batch does not detach — it drives every task through _execute_task_subprocess, ending in process.wait().

So the complete agent execution of the whole batch ran on the uvicorn event loop. Every other request, SSE stream, WebSocket and /health froze for its duration — minutes to hours for a real batch. The web UI calls this endpoint. As the issue puts it: this alone makes the server unusable for more than one concurrent tenant.

The unused BackgroundTasks parameter on /tasks/approve shows offloading was intended and never wired.

Fix

start_batch splits at its natural seam:

Function	Role
create_batch()	validates, persists, emits BATCH_STARTED, returns a PENDING record — so a client polling /batches sees it at once
execute_batch()	takes a persisted batch and runs it to completion; reads strategy/max_parallel/engine off the record, so a resumed batch executes exactly as created
start_batch()	unchanged blocking create+execute wrapper for the CLI and tests, with a docstring warning handlers off it

Both handlers now create the batch, return batch_id, and hand execution to a daemon thread — the same shape start_single_task already used for a single run.

Why a thread and not BackgroundTasks

Starlette would run it in the shared anyio threadpool, where an hours-long batch permanently occupies one of its limited workers, and enough concurrent batches starve every other sync endpoint. The issue's own criterion blesses the thread ("as start_single_task does"). The unused parameter is removed.

One more blocking call, found by the test

create_batch's SQLite INSERT was still synchronous on the loop. Milliseconds normally — but it made the concurrency test fail when run after the full conductor suite on an fsync-bound machine: the whole 2s budget went on the persist, not the batch. Loosening the threshold would have hidden a real blocking call, so both handlers wrap create_batch in run_in_threadpool (the pattern diagnose_v2/environment_v2 already use). The handlers are now non-blocking end to end.

Acceptance criteria

Criterion	Where
start_batch split into create-record and execute phases	conductor.create_batch / conductor.execute_batch
POST /tasks/execute returns batch_id immediately	test_execute_returns_while_the_batch_is_still_running
Test starts a batch via the API and asserts another endpoint responds while it runs	test_health_responds_while_a_batch_runs
The unused BackgroundTasks parameter is used or removed	removed, with its docstring line and the stale module-docstring reference

On the tests

They assert the properties that actually distinguish fixed from broken — the handler returns while the batch is still running, and a concurrent request is served meanwhile. A plain assert status_code == 200 passes on the broken code.

Two details worth noting:

• The /health request is issued with the execute POST still in flight (asyncio.create_task). Awaiting the POST first would let a blocking handler finish before /health is even sent — the timing would look fine on exactly the code this is meant to catch. My first draft made that mistake and the mutation check caught it.
• The blocking stand-in waits on an Event the test releases rather than sleeping, so "still running" is a controlled fact instead of a wall-clock race, and teardown releases it so no detached thread leaks into the next test's timing budget.

Mutation-verified: replacing the threading.Thread(...) dispatch with a direct inline call fails 4 of the 5 tests. The fifth pins persist-before-response, which is true either way by design.

Verification

• 81 passed — the offload tests together with the entire test_conductor.py suite, the combination that surfaced the persist-on-loop blocking.
• 252 passed across conductor, the v2 router integration suite, the existing event-loop offload suites, task-start failure handling and the CLI integration suite (that run predates the persist fix; its single failure was the health test now fixed).
• ruff clean, strict mypy clean on all 192 files.

Known limitations

• Execution runs on an unbounded per-batch daemon thread. Fine for the current single-workspace server, but there is no global cap on concurrent batches — a queue/worker-pool is a larger change than this issue's scope.
• A batch is lost if the server restarts mid-run; cf work batch resume already covers recovery, and execute_batch reading its config off the record is what makes resume behave identically.

[github-actions]

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

---

GLM precision review — complete

Scope: concrete defects only (logic errors, data loss, races, resource leaks, contract breaks, security) in the three changed files. Style/coverage left to the general review.

• Gather PR diff + existing comments
• Read changed files in full context (conductor.py, tasks_v2.py)
• Trace callers/callees to confirm each suspected defect
• Post inline findings (concrete failure scenarios only) — none survived verification
• Summary

✅ GLM review: no defects found.

What I specifically checked and ruled out:

• Split fidelity — create_batch/execute_batch together reproduce the old start_batch body exactly; the dry_run short-circuit is preserved in both the creator and the wrapper (conductor.py:684, 742).
• DB round-trip after the split — this is the one real risk the split introduces: the worker thread now reconstructs the batch via get_batch→_row_to_batch instead of using the in-memory object. Confirmed _save_batch persists and _row_to_batch restores every field the executor reads (engine, isolation, stall_*, concurrency.by_status, llm_provider/model, on_failure, strategy, max_parallel, task_ids), so resumed/background batches execute with their original config.
• No orphaned batch on background failure — _run_batch_in_background swallows exceptions, but _execute_serial/_execute_parallel finalize a terminal status in their except ([P2.14] Reconciliation thread leaks (batch stuck RUNNING) when execution raises — stop_event.set() not in finally #763) before re-raising (conductor.py:1792-1809), so a swallowed error can't strand a batch in RUNNING. The only un-finalized raise point is the top-of-execute_batch RUNNING save, which the old synchronous path hit identically.
• Thread safety of the new daemon thread — get_db_connection opens a fresh connection per call and batch writes serialize on _batch_db_lock, so the new outer thread adds no cross-thread DB hazard beyond what the parallel worker pool already had.
• Preserved contracts — both handlers still surface batch_id, keep create_batch inside the existing try/except (ValueError→500), and body.engine/body.retry_count/body.strategy/body.max_parallel all exist on their request models. max_retries flow to execute_batch via the detached call; /approve correctly keeps max_retries=0 as before.

[github-actions]

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

---

GLM precision review — complete

Scope: concrete defects only (logic errors, data loss, races, resource leaks, contract breaks, security) across all three commits. This pass specifically covers e175490 (the per-workspace lock + 409 guard + fail_batch + /approve HTTPException fix), which the earlier GLM summary at 13:43 predates — that one reviewed only the first two commits. Style/coverage left to the general review.

• Gather PR diff + existing comments
• Read changed files in full context (conductor.py, tasks_v2.py)
• Trace callers/callees to confirm each suspected defect
• Post inline findings (concrete failure scenarios only) — none survived verification
• Summary

✅ GLM review: no defects found.

What I specifically checked and ruled out (focus on the new commit e175490):

• One-batch-per-workspace guard is race-free. _create_batch_exclusively holds _workspace_batch_lock(workspace) across both _reject_if_batch_active and create_batch, so two concurrent /execute POSTs cannot both pass — the second serializes onto the lock and reads the first's already-committed PENDING row (create_batch→_save_batch commits before the lock releases). _reject_if_batch_active checks both PENDING and RUNNING, which covers the create→dispatch window. Pinned by test_concurrent_executes_admit_exactly_one.
• No deadlock. Lock ordering is strictly _workspace_batch_lock → _batch_db_lock (inside create_batch→_save_batch); nothing acquires them in reverse, and the parallel worker pool / reconciliation thread take only _batch_db_lock. The workspace lock is held only during create (ms), never during execution, and the whole thing runs via run_in_threadpool, so it never touches the event loop.
• fail_batch can't clobber a legitimately-finished batch. It re-get_batchs and no-ops on anything in TERMINAL_BATCH_STATUSES; the only non-terminal statuses are PENDING/RUNNING, exactly the wedged cases it's meant to fix. A concurrent cancel_batch during the FAILED write is harmless — _save_batch's preserve_terminal_cancel leaves the row CANCELLED (still terminal).
• Every pre-finalization raise point is covered. execute_batch sets RUNNING/saves, then dispatches. The places that can raise before _execute_parallel/_execute_serial finalize their own except ([P2.14] Reconciliation thread leaks (batch stuck RUNNING) when execution raises — stop_event.set() not in finally #763) — the RUNNING save itself, create_execution_plan, _start_reconciliation_thread — all propagate to _run_batch_in_background's except → _finalize_wedged_batch → fail_batch. A crash can no longer strand the row in RUNNING (which resume_batch refuses). _execute_parallel's finally still stops the reconciliation thread on the in-try path, and the pre-try raise points are upstream of _start_reconciliation_thread, so no reconcile thread is leaked on that path.
• HTTPException propagation through the threadpool. The 409 from _reject_if_batch_active is raised inside run_in_threadpool; anyio.to_thread.run_sync re-raises it in the coroutine, and both handlers now except HTTPException: raise before the catch-all — so the deliberate 409 reaches the client instead of being re-wrapped as 500. (/approve previously re-wrapped it.)
• No contract regressions. Both handlers still return batch_id; body.engine/body.retry_count/body.strategy/body.max_parallel all exist on their request models; /execute routes body.retry_count → max_retries into execute_batch, /approve keeps max_retries=0 as before; on_event is None in the server path exactly as it was pre-split (the CLI wrapper start_batch still threads it through both halves). All existing start_batch(...) call sites hit the unchanged wrapper.
• Symbols resolve. ErrorCodes.CONFLICT, api_error, run_in_threadpool, functools.partial are all imported/defined (the 409 path can't AttributeError into a 500).

Considered and deliberately rejected (no concrete wrong outcome / not a regression):

• /approve transitioning BACKLOG→READY before the 409 — the original inline path did the same (and then also spawned a second batch); READY is the legitimate effect of "approve", so this is strictly better, not a regression.
• Stuck-RUNNING batch after a server restart blocks new batches with 409 — explicitly documented under Known Limitations with a recovery path (the 409 body names the batch and the cancel endpoint); the guard is behaving correctly.
• _workspace_batch_locks dict growth — bounded by workspace count (operator-controlled via WORKSPACE_ROOT); trivial Lock objects.

[frankbria]

Third-party review round — a regression I introduced

Both CodeRabbit and the opencode/GLM pass independently found the same thing, and it is the most interesting finding in this PR.

The blocking bug was accidentally serializing batches

opencode (HIGH): Before this PR, start_batch ran inline on the event loop, so a second POST /execute couldn't even be served until the first batch finished — the blocking "accidentally" serialized batches. The offload removes that, and the only replacement guard is check_assignment_status, which is a textbook check-then-act.

check_assignment_status counts IN_PROGRESS tasks — but tasks only flip to IN_PROGRESS inside the detached thread, long after create_batch returned and the handler answered 200. Two clicks inside that window both pass, both persist, both spawn: two agent subprocesses writing the same git worktree.

And /approve with start_execution=true never called the guard at all.

Fix: a workspace admits one batch at a time. The check and the create happen under a single per-workspace lock — checking first and creating after leaves exactly the window two clicks need. A second request gets 409 naming the blocking batch. It runs on the worker thread both handlers already use, so the lock never touches the event loop.

This also resolves the thread-bound concern both reviewers raised separately (CodeRabbit MAJOR, opencode LOW): concurrent batch threads are now capped by workspace count rather than request rate, without inventing a queue whose overflow semantics this issue never specified.

Tests: test_second_execute_is_refused_while_one_runs, test_concurrent_executes_admit_exactly_one (both requests genuinely in flight via asyncio.gather), test_approve_is_guarded_too, and test_a_finished_batch_does_not_block_the_next_one so the gate provably lifts.

A wedged batch could not even be resumed

opencode (MEDIUM): execute_batch sets status=RUNNING and saves before dispatching… _execute_parallel: create_execution_plan(...) and _start_reconciliation_thread(...) execute before the try. A raise here propagates un-finalized… Worse, resume_batch refuses anything not in (PARTIAL, FAILED, CANCELLED) — a RUNNING batch is unresumable.

Sharper than the old behaviour, where the same exception became a 500 the client saw. Now the client holds a 200 + batch_id and the batch is silently stuck forever.

Fix: conductor.fail_batch() finalizes a non-terminal batch and emits BATCH_FAILED; the background runner calls it on any escape. get_batch moved inside the same try (CodeRabbit MAJOR) so a DB error can't escape the thread either. Pinned by test_thread_failure_marks_the_batch_failed.

That surfaced a real handler bug

/approve's catch-all re-wrapped the deliberate 409 as a 500 "Approval failed". It now re-raises HTTPException, as /execute already did. My own new test caught this.

CI caught a third test file

tests/integration/test_tasks_v2_engine.py patches conductor.start_batch to assert the engine kwarg reaches the conductor. Same subject, new seam: it patches create_batch, plus _start_batch_detached so no thread spawns against an unpersisted mock.

Test strengthened (both reviewers)

test_health_responds_while_a_batch_runs used a fixed 50 ms sleep. If the thread hadn't entered execute_batch yet, /health was measured while nothing was running — passing vacuously, and passing for a regression that merely delayed the block. It now waits for the batch to be genuinely in flight (via the executor, so the wait itself never blocks the loop).

Confirmed clean by opencode

Cancel/stop racing the detached thread (_save_batch's preserve_terminal_cancel already refuses the RUNNING write and flips to CANCELLED); SQLite across threads (fresh connection per call, WAL + busy_timeout, module-level _batch_db_lock); event publishers cross-thread; max_retries routing; dry_run; and all 50+ existing start_batch(...) call sites against the unchanged wrapper.

Accepted, documented

• Daemon threads killed at shutdown (opencode MEDIUM). A graceful uvicorn shutdown abandons an in-flight batch mid-subprocess. A shutdown registry + join is a larger change than this issue; recovery today is cancel then resume. Noted in Known Limitations.
• started_at set at create, not at execute (CodeRabbit MINOR). They are the same instant in every current path; "started" meaning "accepted" is the reading /batches already displays.
• cloud_timeout_minutes never plumbed into batches — opencode flagged it as pre-existing and out of scope. Confirmed: that's [P2.9] Plumb cloud_timeout_minutes through batch execution and stop wiping manual task dependencies #959 (P2.9).