fix(server): offload blocking sync work out of async v2 handlers (#732) - #805
Conversation
run_proof, gates.run, generate_prd_from_discovery, DiagnosticAgent.analyze, and ToolInstaller.install_tool ran directly on the event-loop thread inside async handlers, freezing /health, SSE, and WebSockets for minutes. Wrap each in run_in_threadpool (existing settings_v2/prd_v2 convention). Adds a concurrency regression test proving /health responds during a proof run.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFive FastAPI v2 routers (diagnose, discovery, environment, gates, proof) were updated to offload their previously synchronous, blocking core calls onto worker threads using ChangesThreadpool Offload for Blocking Endpoints
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProofV2Router
participant ThreadPool
participant EventLoop
Client->>ProofV2Router: POST /api/v2/proof/run
ProofV2Router->>ThreadPool: run_in_threadpool(run_proof, workspace)
Client->>ProofV2Router: GET /health
ProofV2Router->>EventLoop: handle /health immediately
EventLoop-->>Client: 200 OK (fast)
ThreadPool-->>ProofV2Router: proof result
ProofV2Router-->>Client: RunProofResponse
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review — fix/732-offload-blocking-handlersThis is a clean, well-scoped fix that applies the correct pattern consistently. The approach matches what's already done in What looks good
Issues to address1. Thread safety assumption is unverified (medium)
The PR description says "fast SQLite reads stay on the loop" — but grep -r "sqlite3.connect\|check_same_thread" codeframe/core/If connections were opened on the main thread and passed to the worker, or if WAL mode isn't enabled, there is a latent risk here. Even a brief note in the PR (or a comment in the wrapped call sites) explaining the safety guarantee would help. 2. Regression test only covers
3. proof_task = asyncio.create_task(client.post("/api/v2/proof/run", json={}))
await asyncio.sleep(0.05) # yields to let the proof handler start50 ms is usually sufficient, but on a heavily loaded CI host the thread pool may not have scheduled the blocking work yet when the health check fires. If this test ever flakes, increasing the sleep or restructuring to use an event/synchronization primitive would help. Minor risk since Minor nitThe test fixture creates a workspace but doesn't initialize any DB schema, so any code path in SummaryThe fix is correct and the pattern is right. The two things worth resolving before merge:
Everything else is minor. Good work keeping the change tight and consistent with the existing codebase pattern. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…fixture rationale (#732)
Review triage (claude-review findings)1. Thread safety of SQLite writes from a pool worker (medium) — verified safe, documented here. 2. Regression test only covers proof_v2 (low) — addressed in e009ca5. 3. Nit (fixture comment) — addressed in e009ca5. |
Follow-up ReviewThe author's triage comment in e009ca5 closes all three issues I raised. Quick confirmations: Thread safety (medium, resolved) — Confirmed: Test async wiring — The test is a bare asyncio.sleep(0.05) flake risk (low, rebutted) — The author's reasoning is sound: with unfixed code the 0.05s yield itself is delayed by the blocking handler, so the health-check timer still exceeds 1.0s. The margin ( Regression test scope (low, resolved) — Module docstring now explains the scope rationale. Acceptable. The change is minimal, consistent with the existing |
Review UpdateNo new commits since my last review (latest: All raised issues were addressed:
The developer guide addition in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codeframe/ui/routers/environment_v2.py (1)
219-221: 🧹 Nitpick | 🔵 TrivialThreadpool offload looks correct; note shared thread-pool capacity.
installer.install_tooldoesn't touchworkspace, so there's no cross-thread DB concern here (unlike the diagnose/discovery endpoints). The wrapping itself is correct.One operational note: Starlette/AnyIO's default thread pool is capped at 40 tokens shared across the whole app (sync dependencies + all
run_in_threadpoolcalls). Since installs here can block "for minutes" per the added comment, several concurrent installs (or installs overlapping with other offloaded v2 endpoints) could exhaust that pool and delay unrelated sync work. Not a regression from this PR, but worth keeping in mind if install traffic grows — e.g. bumpinganyio.to_thread.current_default_thread_limiter().total_tokensor bounding concurrent installs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codeframe/ui/routers/environment_v2.py` around lines 219 - 221, The threadpool offload in environment_v2 is correct, but long-running `installer.install_tool` calls can consume Starlette/AnyIO’s shared default thread pool and delay other sync work. In `environment_v2` around the `run_in_threadpool(installer.install_tool, ...)` path, add a mitigation such as bounding concurrent installs or increasing `anyio.to_thread.current_default_thread_limiter().total_tokens` so install traffic cannot starve unrelated offloaded endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@codeframe/ui/routers/gates_v2.py`:
- Line 15: The `/api/v2/gates/run` path is currently dispatching `gates.run()`
through `run_in_threadpool(...)` without any workspace-level serialization, so
concurrent requests can race on the same repo. Add a per-`workspace.repo_path`
lock or queued execution in the router handler in `gates_v2.py` so only one gate
run can mutate a given workspace at a time. Use the route entrypoint that calls
`run_in_threadpool` and the underlying `gates.run()` flow as the place to
acquire/release the workspace lock before installing deps and running checks.
---
Nitpick comments:
In `@codeframe/ui/routers/environment_v2.py`:
- Around line 219-221: The threadpool offload in environment_v2 is correct, but
long-running `installer.install_tool` calls can consume Starlette/AnyIO’s shared
default thread pool and delay other sync work. In `environment_v2` around the
`run_in_threadpool(installer.install_tool, ...)` path, add a mitigation such as
bounding concurrent installs or increasing
`anyio.to_thread.current_default_thread_limiter().total_tokens` so install
traffic cannot starve unrelated offloaded endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4c75270-abb9-4f69-8ea8-908107d2c134
📒 Files selected for processing (6)
codeframe/ui/routers/diagnose_v2.pycodeframe/ui/routers/discovery_v2.pycodeframe/ui/routers/environment_v2.pycodeframe/ui/routers/gates_v2.pycodeframe/ui/routers/proof_v2.pytests/ui/test_event_loop_offload.py
Final feedback triage (CodeRabbit re-review)Serialize gate runs per workspace (Minor) — acknowledged, not fixed here. Shared thread-pool capacity (Trivial) — acknowledged, no change. Gate summary: CI green on d06f48b, demo verified both acceptance criteria with outcome evidence, no unresolved Critical/Major findings. Merging. |
1 participant
Closes #732
Problem
Five
async defv2 handlers called long-blocking core functions (full test-suite runs, LLM calls, subprocess installs) directly on the event-loop thread, freezing all other requests — SSE heartbeats, terminal/chat WebSockets, and/health— for minutes.Change
Wrap the heavy call in each handler with
await run_in_threadpool(...)(fastapi.concurrency), the pattern already used insettings_v2.py/prd_v2.py:proof_v2.pyPOST /proof/runrun_proof()(pytest/ruff gate runs)gates_v2.pyPOST /gates/rungates.run()discovery_v2.pyPOST /{id}/generate-prdgenerate_prd_from_discovery()(LLM)diagnose_v2.pyPOST /{id}/diagnoseDiagnosticAgent.analyze()(LLM)environment_v2.pyPOST /installToolInstaller.install_tool()(subprocess)Tests
tests/ui/test_event_loop_offload.py: TDD regression test (RED before fix) — monkeypatchesrun_proofwith a 1.5stime.sleepblocker and asserts a concurrent/healthrequest completes in <1s overhttpx.ASGITransport.tests/ui/test_proof_v2.py+tests/ui/test_v2_routers_integration.py: 125 passed.ruff checkclean.Acceptance criteria
run_in_threadpool/healthrequest (regression test)Known Limitations
Summary by CodeRabbit