perf: run npm/tsc verification subprocesses off the event loop - #1240
Conversation
verify_project() is async but called subprocess.run() directly, blocking the loop for up to 420s (180+180+60) per invocation, multiplied by the retry loop in retry_verification(). Reached in production via POST /api/v1/video-to-software. Wrap all three calls in asyncio.to_thread. subprocess.run's own timeout= still bounds the worker and kills the child, so no executor slot leaks and no asyncio.wait_for wrapper is needed. Closes #1239 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesAsynchronous project verification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@linear This is PR 3 of a 10-part performance campaign (#1228 and #1233 are merged). Unlike the first two, this one is genuinely on a production hot path — reachable from Two things I want to flag proactively, both carried forward from your review on #1233:
Please review. |
|
@coderabbitai review Please be adversarial. Specific things worth attacking:
|
|
I found one real hole in the argument: in src/youtube_extension/backend/deployment_manager.py,
I didn’t find support for the other attacks:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-only origin/main...HEAD
printf '%s\n' '=== Diff ==='
git diff --unified=80 origin/main...HEAD -- \
'youtube_extension/backend/deployment_manager.py' \
'tests/unit/test_deployment_manager.py' || true
printf '%s\n' '=== Candidate files ==='
fd -a 'deployment_manager\.py|test_deployment_manager\.py' . || true
printf '%s\n' '=== DeploymentManager outline ==='
if [ -f youtube_extension/backend/deployment_manager.py ]; then
ast-grep outline youtube_extension/backend/deployment_manager.py --items all --view expanded || true
fi
printf '%s\n' '=== Relevant implementation context ==='
if [ -f youtube_extension/backend/deployment_manager.py ]; then
rg -n -C 12 --glob 'deployment_manager.py' \
'async def verify_project|subprocess\.run|os\.chdir|os\.getcwd|breadcrumb|add_breadcrumb|self\.' \
youtube_extension/backend/deployment_manager.py || true
fi
printf '%s\n' '=== Relevant test outline and context ==='
if [ -f tests/unit/test_deployment_manager.py ]; then
ast-grep outline tests/unit/test_deployment_manager.py --items all --view expanded || true
rg -n -C 14 --glob 'test_deployment_manager.py' \
'TestVerifyProjectRunsOffEventLoop|verify_project|patch\(|subprocess\.run|threading\.Event|asyncio\.' \
tests/unit/test_deployment_manager.py || true
fi
printf '%s\n' '=== Python subprocess timeout implementation ==='
python3 - <<'PY'
import inspect
import subprocess
print(inspect.getsource(subprocess.run))
PYLength of output: 17920 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Source diff ==='
git diff --unified=100 origin/main...HEAD -- \
src/youtube_extension/backend/deployment_manager.py \
tests/unit/test_deployment_manager.py
printf '%s\n' '=== Source structure ==='
ast-grep outline src/youtube_extension/backend/deployment_manager.py --items all --view expanded || true
printf '%s\n' '=== verify_project implementation ==='
rg -n -C 80 \
'async def verify_project|await asyncio\.to_thread|subprocess\.run|TimeoutExpired|FileNotFoundError|OSError|add_breadcrumb|os\.getenv|os\.chdir|cwd=' \
src/youtube_extension/backend/deployment_manager.py
printf '%s\n' '=== verify_project callers ==='
rg -n -C 8 \
'\.verify_project\s*\(|await\s+.*verify_project\s*\(' \
src tests
printf '%s\n' '=== Manager fields and mutation sites ==='
rg -n -C 4 \
'self\.[A-Za-z_][A-Za-z0-9_]*\s*=|self\.[A-Za-z_][A-Za-z0-9_]*\.(append|update|add|remove|clear)\s*\(' \
src/youtube_extension/backend/deployment_manager.py
printf '%s\n' '=== New test section ==='
start=$(rg -n '^class TestVerifyProjectRunsOffEventLoop:' tests/unit/test_deployment_manager.py | cut -d: -f1)
if [ -n "${start:-}" ]; then
sed -n "${start},$((start + 220))p" tests/unit/test_deployment_manager.py
fi
printf '%s\n' '=== pytest and dependency configuration ==='
fd -a -t f 'pyproject\.toml|pytest\.ini|tox\.ini|setup\.cfg|requirements.*\.txt|poetry\.lock|uv\.lock|Pipfile.*' . \
| while IFS= read -r file; do
echo "--- $file"
rg -n -C 3 'pytest|xdist|asyncio|addopts|testpaths' "$file" || true
doneLength of output: 49926
One operational limitation remains: all verifications use the loop’s shared default thread pool. High concurrent traffic can queue verification work and can delay unrelated ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Moves project verification subprocesses off the event loop while preserving existing behavior.
Changes:
- Offloads npm install, build, and TypeScript checks using
asyncio.to_thread. - Adds deterministic thread-identity, responsiveness, and timeout regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/youtube_extension/backend/deployment_manager.py |
Offloads three blocking subprocess calls. |
tests/unit/test_deployment_manager.py |
Verifies off-loop execution and preserved timeout handling. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@linear You are right and I am wrong. "the thread always returns and the slot is genuinely released" is an absolute claim that I cannot support. I am correcting the body. I did measure your specific mechanism, and I want to report exactly where it lands, because it is narrower than stated but the residual you identified is real. Your grandchild-holds-the-pipe case is the Windows branch, not POSIX. CPython: except TimeoutExpired as exc:
process.kill()
if _mswindows:
exc.stdout, exc.stderr = process.communicate() # reads to EOF -> a surviving
# grandchild blocks this forever
else:
process.wait() # direct child only; already killed
raiseMeasured on POSIX with a child that leaves a grandchild holding stdout — And every job in this repo is Ubuntu ( Your uninterruptible-sleep case survives that, and it defeats my claim. SIGKILL cannot preempt Where I still land on So even when a worker overruns, off-loop keeps the loop serving; on-loop freezes the whole server. The change is a strict improvement in the pathological case too — it just is not the guarantee I claimed. Rewriting |
|
That revised framing sounds substantially better to me. The only caveat I’d keep is that “bounded-in-practice-on-POSIX” is defensible only if you say what practice you mean: CPython’s POSIX timeout path kills the direct child and waits on that child, so the grandchild-holding-pipe concern doesn’t apply on your actual Ubuntu runtime, but an uninterruptible-sleep child can still overrun. If you spell out that residual explicitly, I don’t think you’re overreaching anymore. I’d avoid any wording that reads like:
But something like “keeps verification off the event loop; on our POSIX production runtime the usual pipe-inheritance case remains bounded, though rare kernel-level stalls can still make the worker outlive the nominal timeout” matches the evidence. |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review — code is sound; red CI is infra/governance, not this diff
Verdict: the change is correct, minimal, and semantics-preserving. Wrapping the three blocking subprocess.run() calls in verify_project() with await asyncio.to_thread(subprocess.run, ...) is the right fix for an async def that was freezing the event loop for up to 420 s. Reviewed call-by-call:
- Args/kwargs (
cwd,capture_output,text,timeout) are forwarded unchanged byto_thread, so command behavior is identical. - Timeout/exception contract preserved:
subprocess.run(..., timeout=N)still bounds and kills its own child inside the worker thread and raisesTimeoutExpired, whichto_threadpropagates to the awaiting coroutine — so the existingexcept subprocess.TimeoutExpired/FileNotFoundError/OSErrorhandlers still fire and still return the same{"passed": False, ...}payloads. - Your reasoning for no
asyncio.wait_foris correct:wait_forwould cancel only the coroutine while the worker kept running — strictly worse than the subprocess's own timeout. Agree with keeping the existing bound. - Test patching holds:
to_thread(subprocess.run, ...)resolvessubprocess.runvia the module attribute at call time, so the 12 pre-existingpatch(...subprocess.run)tests and the new thread-identity tests both bind correctly. The reverted-source proof (3 failed / 1 passed) demonstrates the new tests are genuine regression guards, not vacuous.
CI status — none of the red checks are caused by this PR
I pulled the failing job logs to confirm:
dependency-review❌ →"Dependency review is not supported on this repository. Please ensure that Dependency graph is enabled"— a repo security-analysis setting, unrelated to the diff.validate❌ →403 API rate limit exceeded for installation(x-ratelimit-remaining: 0) — a transient GitHub API rate-limit flake; a re-run should clear it.agent-completion/truth-gate,Agent completion enforcement,Canonical issue and evidence❌ → hit the same rate-limit 403; the gate then fail-closed (gate-verdict.jsonmissing → "no proven gate-status lease"). This is the deliberate provenance/governance gate, which needs the maintainer's completion process — not a code change.- The checks that actually exercise the code —
test,build,lint-python,lint-frontend,guards— all passed. ✅
One thing that genuinely needs a maintainer's call
deployment_manager.py L2 carries # LOCKED FILE: SYSTEM AGENT ONLY - DO NOT EDIT MANUALLY. You've disclosed this and given reasonable evidence it's stale (no .github/ reference; merged PRs #927/#207/#59 modified the file). That override is a human decision — a maintainer should confirm it before merge. I'm deliberately leaving this as a COMMENT rather than an approval for that reason.
Optional (non-blocking)
Under high concurrency, long (up to 3 min) to_thread calls draw from the default shared ThreadPoolExecutor, which could starve other offloaded work. You already track the unbounded-blocking class in #1234, so this is fine to defer — noting it only for completeness.
Net: no code changes requested. Merge is blocked on (1) the governance gate's provenance process, (2) a re-run to clear the rate-limit flake, and (3) maintainer sign-off on the LOCKED-FILE override.
Generated by Claude Code
|
Automated CI triage (remediation routine) — head The red checks on this PR are all environmental, not code defects. The diff itself is clean: CodeRabbit's adversarial pass found no blocking defect, Root-causes, from the failing job logs:
Because two of these are repo-wide (rate-limit exhaustion, dependency-graph disabled), they will keep reddening unrelated PRs until addressed. This PR is not autonomously mergeable and is held at the human publish gate regardless (no Generated by Claude Code |
Independent review — verified by execution, not by readingI picked up #1239 from the priority queue, found this PR already claimed it, and switched to reviewing rather than opening a competing PR. Everything below was run against Verdict: correct, well-tested, ship it. One material follow-up (not a blocker) at the end. 1. The change is correctThree 2. The tests are non-vacuous — I negative-controlled them
The failures are targeted at exactly the offload assertions, and they assert thread identity, not wall-clock — so they're not timing-flaky. This is a real regression guard. 3. Base is current
4. CI lint gate is clean
Zero new errors. 5. The "LOCKED FILE" argument holds — and is stronger than you argued
6. One detail your write-up understates: the retry multiplier is hardcodedI traced reachability independently:
7.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
Trivy check resolvedThe
Final tally: 27 success, 2 neutral, 6 skipped, 0 failures, 0 queued. |
* perf: scan processed-video cache off the event loop GET /api/v2/videos/list is declared async but its whole body was blocking filesystem work: a stat, a directory glob, and one open()+json.load() per cached video, with no bound on entry count. The handler never awaited, so the loop was stalled for the full scan and no other request could be served. Extract the scan into a module-level _collect_processed_videos_sync() helper and dispatch it with asyncio.to_thread(), matching the pattern used in #1194, #1196, #1228, #1233, #1240, #1245 and #1251. The scan logic is moved verbatim, so the response payload, newest-first ordering, per-entry corrupt-file skip and empty-list fallbacks are unchanged. Measured on a 2,000-entry cache, peak event-loop stall drops from ~193 ms to ~2 ms. Scan wall time is unchanged: this is a latency and fairness fix, not a throughput one. Closes #1287 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * style: Black-format _collect_processed_videos_sync helper Normalize string quotes to double and wrap the dict-append and sort call in _collect_processed_videos_sync to satisfy the 88-char limit, addressing the CodeRabbit review on #1288. Behaviour-preserving: diff is confined to the new helper and the reformat is Black's own AST-equivalent output (verified with --target-version py311). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013rG7vUAn6z9tXoEuA3dAqz * test: prove per-file cache read is off the event loop The thread-recording cache directory previously asserted only that exists()/glob() ran off-loop, and relied on the helper extraction to imply the per-entry open()/json.load() moved with them. glob() now yields path-like proxies whose __fspath__ records the calling thread. Because open() resolves a non-str argument through __fspath__, this captures the thread at the exact moment each blocking read starts, so the read is proven off-loop rather than inferred. Verified by reverting only the handler call site to the inline form: the new assertion fails independently with "blocking cache entry read ran on the event loop thread". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Canonical issue
Closes #1239
Outcome
DeploymentManager.verify_project()isasync defbut shelled out through threebare
subprocess.run()calls. Each one blocks the entire event loop for its fulltimeout window, so a single project verification could freeze every other in-flight
request in the process for up to 420 s (7 min) — and
verify_projectis invokedfrom inside a retry loop, so that ceiling is multiplied by
max_retries.All three calls are now wrapped in
asyncio.to_thread(...).npm installnpm run buildnpx tsc --noEmitThis does:
subprocess.runcalls onto worker threadsThis does not:
TimeoutExpired,FileNotFoundErrorand genericOSErrorare still caught by the same handlers and still produce the same{"passed": False, "summary": ...}payloadsupload_single_file(L610open()) or_upload_to_github(L634rglob),which have the same class of defect — deliberately deferred to keep this diff tight
Production evidence
Unlike my two previous perf PRs (#1228, #1233), which fixed library-surface code, this
path is reachable from a live HTTP route. Verified at call level, not merely by imports:
Reachability was confirmed with an import-closure BFS from
youtube_extension.main(60 modules) and then hand-checked call-by-call through each frame above.
Risk
Low. The diff is three
await asyncio.to_thread(...)wrappers; no arguments,timeouts or branches changed.
import asynciowas already present (L12).On why there is deliberately no
asyncio.wait_forwrapper. In #1233 review it wasargued that offloading without a timeout can leak an executor slot. An earlier revision of
this section claimed
subprocess.run(..., timeout=N)means the worker "always returns andthe slot is genuinely released". That was an overclaim and review correctly rejected it.
The corrected position, with the measurements behind it:
process.kill()thenprocess.wait()— the direct childonly, and it was just killed. A child that leaves a grandchild holding stdout does not
extend it. Measured with
sh -c "sleep 30 & sleep 60",capture_output=True, timeout=2:worker time 2.01 s, no overrun. The unbounded variant of that scenario is the
if _mswindows:branch, which callscommunicate()and reads to EOF; this repo runszero Windows jobs (52
ubuntu-latest+ 10ubuntu-slim).process.wait()can exceed
N. Bounded in practice, not guaranteed.wait_fordoes not close that residual — it cancels the awaiting coroutine while the workerkeeps running, so the slot stays occupied and
TimeoutExpiredis lost, turning a visibleoverrun into a silent one. And even when a worker does overrun, offloading is strictly better
than the status quo:
So: bounded in practice on the platform this runs on, with a named residual — not a guarantee.
The unbounded file-read case from #1233 remains separately tracked in #1234.
create_subprocess_execwas considered and rejected: it would require re-expressingtimeout=aswait_for+ explicitkill(), changingTimeoutExpiredsemantics. Theto_threadform preserves behaviour exactly and matches the pattern already merged in#1194, #1203, #1205, #1228 and #1233.
Disclosure — file header.
deployment_manager.pyL2 reads# LOCKED FILE: SYSTEM AGENT ONLY - DO NOT EDIT MANUALLY. I treated this as staleadvisory text rather than an active control, on the following evidence: it is referenced
nowhere in
.github/, it is the only file insrc/carrying such a header, and mergedPRs #927, #207 and #59 all modified this file. Flagging it explicitly so a maintainer can
overrule me if that is wrong.
Verification
Backward compatibility. 12 pre-existing tests patch
youtube_extension.backend.deployment_manager.subprocess.run. Becauseto_threadresolves the module attribute at call time, every one of those patches still applies —
all 101 tests in the file passed unmodified, before any new test was added.
4 new tests in
TestVerifyProjectRunsOffEventLoop. They assert thread identityat the moment the blocking work runs, never wall-clock timing, so they are deterministic
in CI. Each includes a call-count guard, so a bypassed patch fails loudly instead of
passing vacuously.
Proof they actually catch the regression — source reverted to
origin/mainwith the newtests retained (
git checkout origin/main -- <source>, verifiedgit diff --stat origin/mainempty):test_timeout_expired_still_reported_after_offloadingpasses on both sides by design —it is a contract-preservation test, not a regression detector.
test_event_loop_still_runs_tasks_during_verificationdeserves a note: the fakesubprocess blocks on a
threading.Eventthat only a coroutine scheduled on the loop canset. If the loop were blocked, that coroutine could never run. It uses a 10 s bounded
wait so it fails rather than hanging CI — which is exactly what it did pre-change.
Ruff parity:
All checks passed!on bothorigin/mainand this branch.