Fix #276: offload /verify blocking calls to threadpool#293
Conversation
Wraps verify_repo() and _scan_repo_dir() in run_in_threadpool so they no longer block the ASGI event loop during verification. Adds a concurrency regression test.
|
🎉 Thank you @CodeByD3v for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
github-actions
Bot
added
backend
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
3 similar comments
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
PR #293 — Code Review: Fix Event Loop Blocking in /verify
Summary
The PR correctly identifies and fixes a real, self-inflicted DoS: async def verify was calling verify_repo() and _scan_repo_dir() synchronously on the ASGI event loop. The two targeted wraps are correct and necessary.
Verdict: Request Changes — One new blocking call was introduced in-scope, one pre-existing blocker is exposed, and the test has a reliability gap that would let the bug regress silently.
✅ What's Correct
Core Fix — verify_repo (line 923)
# Before
result = verify_repo(repo_dir)
# After
result = await run_in_threadpool(verify_repo, repo_dir)Correct. Matches /scan's established pattern.
Core Fix — _scan_repo_dir in /verify (lines 934–941)
_, _, _, _, findings = await run_in_threadpool(
functools.partial(
_scan_repo_dir,
repo_dir,
job_dir=job_dir,
raw_dir_name="raw_verify",
)
)Correct. functools.partial is the right idiom for passing keyword args to run_in_threadpool.
Exception propagation
run_in_threadpool re-raises in the calling coroutine, so existing try/except HTTPException and except Exception blocks around the verify handler behave identically. The PR's claim here is accurate.
Test strategy
Using httpx.AsyncClient + ASGITransport on a shared event loop is the only way to catch this class of bug — a TestClient would not. The approach is sound.
❌ Issues Requiring Changes
🔴 Issue 1 — New blocking call introduced in evidence_pack (lines 1040–1045)
evidence_pack is a synchronous endpoint (def evidence_pack, not async def), so calling _scan_repo_dir there is safe — FastAPI runs sync endpoints in a thread automatically. However, the same _scan_repo_dir calls in /verify were wrong, and this PR added a related concern: evidence_pack at line 964–966 writes a JSON file synchronously on disk inside async def verify, directly on the event loop:
# Line 964–966 — INSIDE async def verify, no thread offloading
(verify_dir / "verification-report.json").write_text(
json.dumps(verify_report, indent=2), encoding="utf-8"
)This write_text is a blocking filesystem call introduced by this PR inside the async def verify handler — not wrapped in run_in_threadpool. For a small JSON payload this is low-latency in practice, but it's inconsistent with the fix's stated goal and should be offloaded or flagged.
🔴 Issue 2 — _scan_repo_dir still blocking in evidence_pack (lines 1043–1045)
if update_raw:
verify_dir = job_dir / "raw_verify"
if verify_dir.exists():
_scan_repo_dir(repo_dir, job_dir=job_dir, raw_dir_name="raw_verify") # BLOCKS
else:
_scan_repo_dir(repo_dir, job_dir=job_dir, raw_dir_name="raw") # BLOCKSevidence_pack is def (synchronous), so these calls are fine as-is — FastAPI dispatches def endpoints to a thread pool automatically. No change needed here. (Flagging to confirm the reviewer is aware, not a bug.)
🟡 Issue 3 — Test timing assertion is fragile
HEALTH_MAX_SECONDS = 1.0
assert health_done_at["elapsed"] < HEALTH_MAX_SECONDSThe test sleeps 2 seconds in a thread. On a slow/loaded CI runner, the thread pool spin-up + context switch overhead can push the health response beyond 1.0 s even with the fix in place, causing a false failure. The PR description acknowledges this pattern is risky for timing tests.
Recommended fix:
HEALTH_MAX_SECONDS = 1.5 # wider window
# OR use a relative assertion:
assert health_done_at["elapsed"] < BLOCK_SECONDS / 2🟡 Issue 4 — anyio backend not configured
@pytest.mark.anyio
async def test_verify_does_not_block_event_loop(tmp_path):pytest-anyio (or anyio mode in pytest-asyncio) requires a pytest.ini / pyproject.toml anyio_mode setting or a conftest.py fixture. If the project uses pytest-asyncio (not anyio), the @pytest.mark.anyio marker is silently ignored and the test runs synchronously, defeating its purpose entirely — it would never catch the bug.
Action required: Confirm pytest-anyio is installed and configured, or switch to @pytest.mark.asyncio to match the rest of the test suite.
Scope Note
This PR touches far more than the fix description states — the diff includes ML model changes, org-scan refactoring, path-traversal guards, frontend changes, and more. These appear to be accumulated from main or other merged PRs. The actual fix itself (2 lines in /verify) is correct, but reviewers should be aware the diff surface is large.
Decision
| Area | Status |
|---|---|
verify_repo offloaded |
✅ Correct |
_scan_repo_dir in /verify offloaded |
✅ Correct |
| Exception propagation | ✅ Unchanged, safe |
New write_text blocking call in async def verify |
🔴 Needs fix |
Test marker (anyio vs asyncio) |
🔴 Must verify |
| Test timing threshold | 🟡 Should widen |
evidence_pack sync calls |
✅ Fine (sync endpoint) |
Request Changes on Issues 1 and 4; Issue 3 is a strong suggestion.
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
Partial progress — one blocker remains before approval.
✅ Issue 1 Fixed — write_text now offloaded correctly
The blocking write_text call inside async def verify is properly wrapped:
await run_in_threadpool(
(verify_dir / "verification-report.json").write_text,
json.dumps(verify_report, indent=2),
encoding="utf-8",
)Good fix, consistent with the PR's stated goal.
🔴 Issue 4 Still Unresolved — @pytest.mark.anyio is silently a no-op
The test still uses @pytest.mark.anyio at line 26, but:
anyio/pytest-anyiois not listed inrequirements.txtor any dev-dependencies file- There is no
pyproject.toml,pytest.ini, orconftest.pythat setsanyio_mode - No other test in the suite uses this marker
This means the marker is silently ignored by pytest. The test runs synchronously, never touches the event loop, and will pass even if the original blocking bug is re-introduced. It provides zero regression protection — exactly the failure mode the review flagged.
Required fix (pick one):
Option A — Switch to pytest-asyncio (matches the existing project toolchain):
# requirements.txt or test deps
pytest-asyncio
# pyproject.toml or pytest.ini
[tool.pytest.ini_options]
asyncio_mode = "auto"
# test file
@pytest.mark.asyncio
async def test_verify_does_not_block_event_loop(tmp_path):Option B — Properly configure anyio:
# requirements.txt or test deps
anyio[trio]
pytest-anyio # or: anyio provides its own pytest plugin
# pyproject.toml
[tool.pytest.ini_options]
anyio_mode = "auto"🟡 Issue 3 — Timing threshold: acceptable but still fragile
BLOCK_SECONDS / 2 is a nice relative expression, but the numeric value is still 1.0s — unchanged from before. On a loaded CI runner, thread-pool spin-up can push the health response past 1.0s and cause a false failure. Consider widening to BLOCK_SECONDS * 0.75 or at minimum adding a comment explaining why 1.0s is sufficient margin.
Decision: Request Changes
Issue 4 is a correctness blocker — the test must actually run asynchronously to validate the fix. Once the async marker is wired up and confirmed passing in CI, this is ready to approve.
2 participants
Linked issue
Closes #276
What this PR does
/verifywas declaredasync defbut calledverify_repo()(up to 20 min fornpm install+ 10 min for tests) and_scan_repo_dir()(up to 10 min per scanner) synchronously, directly on the ASGI event loop. This blocked all concurrent requests — health checks,/scan, other users' polling — for the entire duration of any verification, a complete self-inflicted DoS. This PR wraps both calls inrun_in_threadpool, matching the pattern/scanalready uses correctly.Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
app/main.py,/verifyhandler:verify_repo(repo_dir)→await run_in_threadpool(verify_repo, repo_dir)_scan_repo_dir(...)→await run_in_threadpool(functools.partial(_scan_repo_dir, ...))verify_repoor_scan_repo_dirthemselves — only where they execute.Frontend
New dependencies
run_in_threadpoolandfunctoolswere already imported inmain.py.Database / schema changes
Testing
How did you test this?
python3 -m py_compile app/main.py— syntax check.pytest backend/tests/ -q→ all passing, no regressions.tests/test_verify_concurrency.py: anhttpx.AsyncClient+ASGITransporttest sharing a single real event loop (a threadedTestClientcan't catch this bug — each call gets its own portal/thread). Mocksverify_repo/_scan_repo_dirwithtime.sleep(2)each, fires/verifyand a concurrent/healthcheck viaasyncio.gather, and asserts health responds in under 1s regardless of/verifyrunning in the background.health took 4.06svs. asserted<1.0s), then restored the fix and confirmed it passes.Checklist
console.erroror unhandled Python exceptions introducedrequirements.txt/package.jsonupdated if new dependencies added (none needed).pkl,.pt, etc.) are gitignored, not committedAnything reviewers should focus on
Please confirm
run_in_threadpooldoesn't change exception propagation for anything depending on/verify's error responses (it re-raises in the calling coroutine, so existingtry/exceptblocks should behave identically). As a fast-follow — not part of this PR — consider a bounded semaphore/job queue if verify volume grows enough to exhaust the default thread pool.Screenshots (if UI changed)
N/A (Backend-only changes)