fix(perf): offload sync SQLite in async task/costs handlers (#751)

[frankbria]

Closes #751

Problem

async def handlers in tasks_v2.py and costs_v2.py called blocking SQLite
directly, pinning that disk I/O to the event-loop thread and serializing every
concurrent request behind it.

Fix

Declare the purely-synchronous handlers plain def. FastAPI then dispatches them
to its anyio threadpool instead of the event loop — the acceptance-criteria
option ("handlers declared plain def") and the pattern already used in
interactive_sessions_v2.py.

Converted (9 handlers):

• tasks_v2.py: list_tasks, get_task, update_task, delete_task, get_assignment_status, get_task_run
• costs_v2.py: get_costs_summary, get_costs_by_task, get_costs_by_agent_endpoint

Why this is safe

• Rate limiting unaffected: slowapi.Limiter.limit branches on iscoroutinefunction, producing a sync wrapper for sync handlers (extension.py:717).
• GitHub auto-close unaffected: update_task → tasks.update_status fires _close_issue_background, which already handles the no-running-loop case (threadpool thread) by spawning a non-daemon thread — the same path the CLI uses.
• Zero await existed in any converted handler, so nothing needed unwinding.

Evidence

Regression test (tests/ui/test_async_handler_offload_751.py): structurally
asserts the 9 handlers are sync and that streaming/orchestration handlers stay async.

Concurrency demo — 5 concurrent requests, 0.4s blocking DB call each:

handler is coroutine? False
status codes: [200, 200, 200, 200, 200]
  elapsed          = 0.41s   (parallel — offloaded ✔)
  serial would be  = 2.00s

Before the fix these would serialize to ~2.0s on the event loop.

Existing suites: 121 passed (test_costs_v2, test_v2_routers_integration,
test_tasks_v2_engine, test_tasks_start_failure) + ruff clean.

Known Limitations

• Execution-control and streaming handlers stay async by design —
  start_execution, start_single_task, stop_task, resume_task,
  approve_tasks_endpoint, stream_task_output_lines, stream_task_events.
  These orchestrate the conductor/runtime, spawn worker threads, use
  BackgroundTasks, or return StreamingResponse, and rely on the running event
  loop. They're outside the cited "blocking SQLite serializes requests" scope and
  converting them carries real risk.

[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: 41 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: f96fa1e2-f531-4103-839b-9ff13653aa48

📥 Commits

Reviewing files that changed from the base of the PR and between 9612f24 and cba50dc.

📒 Files selected for processing (3)

• codeframe/ui/routers/costs_v2.py
• codeframe/ui/routers/tasks_v2.py
• tests/ui/test_async_handler_offload_751.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/751-offload-sync-sqlite-async-handlers

---

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

[claude]

Review

This is my first pass on this PR (no earlier review from me to reconcile with — the only other comment is CodeRabbit's rate-limit notice, which carries no content).

Scope and approach match CLAUDE.md's core-first/CLI-first architecture rules — nothing here touches codeframe/core/** or adds a server dependency for CLI paths, and the change is a pure async def → def conversion with no logic changes.

Verified by reading the diff and the relevant code paths (couldn't execute pytest in this sandbox):

• All 9 converted handlers (list_tasks, get_task, update_task, delete_task, get_assignment_status, get_task_run, get_costs_summary, get_costs_by_task, get_costs_by_agent_endpoint) are genuinely free of await — the conversion is safe as claimed.
• rate_limit_standard() → _create_rate_limit_decorator (codeframe/lib/rate_limiter.py:250-268) always applies limiter.limit(...) and lets slowapi introspect sync-vs-async, so no double-wrapping or lost rate limiting on the now-sync handlers.
• The auto-close claim for update_task checks out: tasks.update_status → _close_issue_background (codeframe/core/tasks.py:588-619) does asyncio.get_running_loop() and falls back to a non-daemon thread on RuntimeError. Since update_task now runs on FastAPI's threadpool (no event loop in that thread), it correctly takes the thread-spawn branch instead of loop.create_task(...) — same fire-and-forget semantics, different mechanism, no behavior change from the caller's perspective.
• get_v2_workspace's dependency chain (including the async require_auth) is unaffected — FastAPI resolves dependencies the same way regardless of the endpoint's own sync/async-ness, so mixing an async dependency with a sync endpoint is fine.

Minor, non-blocking observations:

1. tests/ui/test_async_handler_offload_751.py's STREAMING_OR_ORCHESTRATION_HANDLERS list only guards 3 of the 7 handlers called out in the PR description's "Known Limitations" section (start_single_task, stream_task_output_lines, stream_task_events are covered; start_execution, stop_task, resume_task, approve_tasks_endpoint are not). Not a bug, but if one of the uncovered four ever gets accidentally flipped to def in a future edit, this regression test won't catch it. Worth adding for completeness if it's cheap.
2. Every DONE-transition-with-auto-close now spawns a brand-new non-daemon OS thread (with its own asyncio.run() loop) per request instead of scheduling a task on the already-running event loop. Functionally correct per the fallback design, and this path is presumably infrequent, but worth keeping in mind if issue-closing volume ever gets high enough for thread-creation overhead to matter — not a blocker for this PR.

No correctness, security, or test-coverage issues found beyond the above. The regression test is a good structural guard (asserts sync-ness by name rather than re-testing business logic already covered elsewhere), and the PR description's concurrency demo + reasoning about slowapi/auto-close is accurate based on the code as it stands.

🤖 Generated with Claude Code

[frankbria]

Thanks for the review. Triage of the two non-blocking notes:

1. Incomplete stay-async guard — ✅ Fixed in cba50dc. Extended STREAMING_OR_ORCHESTRATION_HANDLERS from 3 to all 7 handlers named in Known Limitations (added start_execution, stop_task, resume_task, approve_tasks_endpoint). The guard now catches an accidental def-flip of any of them (16 tests pass).

2. Thread-per-DONE-transition overhead — Acknowledged, no change. This is inherent to the pre-existing _close_issue_background fallback design, not introduced here, and the auto-close path is low-frequency (only opted-in imported tasks transitioning to DONE). If issue-closing volume ever warrants it, the fix belongs in core (e.g. a shared executor), not in this router change.

[claude]

Follow-up review

Both items from my previous pass are resolved:

1. Incomplete stay-async guard — fixed in 582dfa4/cba50dc6. STREAMING_OR_ORCHESTRATION_HANDLERS in tests/ui/test_async_handler_offload_751.py now lists all 7 handlers named in the PR's Known Limitations (start_execution, stop_task, resume_task, approve_tasks_endpoint added to the original 3). Verified against codeframe/ui/routers/tasks_v2.py: all 7 are still async def today, so the parametrized test_streaming_and_orchestration_handlers_stay_async accurately guards them — an accidental def-flip of any one would now fail the suite.
2. Thread-per-DONE-transition overhead — acknowledged as pre-existing _close_issue_background fallback behavior, correctly scoped out of this PR. Agreed that's a core-level concern (shared executor, if it ever becomes a bottleneck), not a router change.

No new issues in the diff. The 9 handler conversions (costs_v2.py, tasks_v2.py) are unchanged from the first commit and remain await-free, so the offload is still safe as originally verified. Good to merge from my side.

🤖 Generated with Claude Code