perf: batch project scaffolding disk writes off the event loop - #1251
Conversation
ProjectCodeGenerator performed every scaffolding filesystem call inline inside async def bodies, so a blocked write parked the whole event loop rather than just the requesting coroutine. Move all 28 filesystem calls off the loop, batched into one asyncio.to_thread hop per generator (O(1) hops instead of O(files)). Content generation is pure in-memory string building and stays on the loop; only the writes are offloaded. Output is byte-for-byte identical, verified by loading the pre- and post-change modules side by side and comparing SHA-256 digests of every emitted file plus every returned dict across all three generators. Refs #1250 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
WalkthroughProject generation now builds artifact contents in memory and performs ordered filesystem operations in worker threads. Temporary project-directory creation also runs off the event loop. React, vanilla JavaScript, and FastAPI generators use the shared write-plan mechanism. ChangesAsync project scaffolding
Estimated code review effort: 3 (Moderate) | ~20 minutes 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-code @coderabbitai review Adversarial framing — the three things most worth attacking here:
Deliberately not claimed: no throughput improvement. The payloads are small |
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"
} |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files vs PR base ==='
git diff --name-status origin/main...HEAD
echo '=== Diff for generator and related tests ==='
git diff --find-renames --unified=80 origin/main...HEAD -- \
'*code_generator.py' '*test_code_generator.py'
echo '=== Candidate generator files ==='
fd -t f 'code_generator\.py$' .
echo '=== Candidate generator test files ==='
fd -t f 'test_code_generator\.py$' .
echo '=== Source outlines ==='
for f in $(fd -t f 'code_generator\.py$' .); do
ast-grep outline "$f" --items all --type function,class --view expanded
done
echo '=== Targeted helper and generator references ==='
rg -n -C 5 \
'(_apply_write_plan|asyncio\.to_thread|json\.(dump|dumps)|def generate_project|async def generate_project|_generate_(react_project|vanilla_js_project|python_api))' \
-g '*code_generator.py' -g '*test_code_generator.py' .Length of output: 89533 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== generate_project call sites and nearby cleanup/cancellation handling ==='
rg -n -C 12 \
'\.generate_project\s*\(|generate_project\s*\(|CancelledError|cancel\(|shutil\.rmtree|rmtree\(|TemporaryDirectory' \
src tests \
-g '*.py'
echo '=== Endpoint path identified in the PR objective ==='
fd -t f 'video_processing_service.py|router.py|main.py' src | while read -r f; do
echo "--- $f"
rg -n -C 18 \
'(process_video_to_software|generate_project\s*\(|CancelledError|cancel\(|rmtree\()' \
"$f" || true
done
echo '=== Existing cancellation-focused tests ==='
rg -n -C 8 \
'(CancelledError|cancel\(|asyncio\.wait_for|timeout)' \
tests -g '*.py'Length of output: 93334
Blocking: cancellation can leave an unowned project directory while scaffolding continues. At Line 182 and Lines 299, 353, and 403,
Use the existing pattern in The three stated concerns otherwise check out by inspection:
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Moves project scaffolding filesystem operations off the event loop while preserving generated output and write ordering.
Changes:
- Adds ordered, batched write plans executed through
asyncio.to_thread. - Offloads temporary-directory creation.
- Adds regression tests for batching, threading, ordering, and error propagation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/youtube_extension/backend/code_generator.py |
Offloads scaffolding filesystem operations. |
tests/unit/test_code_generator.py |
Adds threading and write-plan tests. |
Suppressed comments (1)
tests/unit/test_code_generator.py:874
- The tracked call delegates to the real
mkdtempwithout placing it undertmp_path, so every test run leaves a generated project in the system temp directory. Setdir=tmp_pathso pytest owns and removes the real temporary directory after the test.
def _tracked(*args, **kwargs):
seen.append(threading.current_thread().name)
return real_mkdtemp(*args, **kwargs)
| seen: list[str] = [] | ||
| monkeypatch.setattr("builtins.open", self._recording_open(seen)) | ||
|
|
||
| project = tmp_path / "project" | ||
| project.mkdir() | ||
| await getattr(gen, generator_name)(project, analysis, ["database"]) |
There was a problem hiding this comment.
You were right, and it was not a theoretical gap — I injected the exact defect you described rather than reasoning about it. src_dir.mkdir() and public_dir.mkdir() were moved out of the write plan and back onto the loop, then both versions of the test file were run against that identical tree:
| Test file | Result against the injected regression |
|---|---|
| Before (85 tests) | 85 passed — undetected |
| After (89 tests) | 2 failed, 87 passed — caught |
So the assertions were passing for the wrong reason: directories are created inside _apply_write_plan as (path, None) plan steps, and nothing was watching that seam.
Fixed in d178e210a. The recorder now hooks Path.mkdir alongside open and asserts over the union of both records. Two ordering details turned out to matter:
- The project root has to be created before patching, otherwise the hook fires during setup and records the calling thread rather than the worker.
_generate_vanilla_js_projectand_generate_python_apicreate no subdirectories at all, so the parametrised test cannot assert themkdirrecord is non-empty without failing on two of its three cases. A dedicated react-only test (test_directory_creation_never_touches_loop_thread) carries that assertion, which is what stops the union check from being vacuously satisfiable.
Both files were restored afterwards and confirmed with an empty diff.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/youtube_extension/backend/code_generator.py (1)
271-299: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftSanitize generated source before writing and deploying it.
These plans persist artifacts that interpolate analysis-derived values into HTML, JavaScript, and Python source. A malicious video title can break generated Python syntax or inject executable Python when the deployed FastAPI app starts. Raw title, summary, technology, and concept values can also create stored XSS in generated HTML.
Encode each value for its target context before source construction. Use safe Python string literals for
main.py, HTML escaping for markup, and safe JavaScript serialization for source values. Parse generated Python withast.parseand syntax-check generated JavaScript before applying the write plan.As per coding guidelines, “Implement comprehensive input validation and sanitize outputs for security.” As per path instructions, “Flag any code generation output that reaches users without AST validation or syntax checking.”
Also applies to: 347-353, 398-403
🤖 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 `@src/youtube_extension/backend/code_generator.py` around lines 271 - 299, Sanitize all analysis-derived values before constructing generated artifacts in the code-generation flow, including the paths around _generate_index_html, _generate_react_app_component, _generate_readme, and the related Python output generation. Use context-appropriate safe Python literals, HTML escaping, and JavaScript serialization for titles, summaries, technologies, features, and concepts. Parse generated Python with ast.parse and syntax-check generated JavaScript before calling _apply_write_plan, ensuring only validated artifacts are written or deployed.Sources: Coding guidelines, Path instructions
🤖 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 `@src/youtube_extension/backend/code_generator.py`:
- Around line 398-403: Update the generated project flow around main_py and
process_video_to_software to stop deploying placeholder FastAPI behavior,
including fixed health data, fake responses, and unimplemented database or
authentication endpoints. Generate real implementations for every requested
feature, or reject unsupported feature combinations before _apply_write_plan
writes and deploys the project; do not emit mock, simulated, or TODO-based
production behavior.
- Around line 182-184: Update generate_project and each asyncio.to_thread
operation, including tempfile.mkdtemp and _apply_write_plan, to retain the
worker task and await it through cancellation so thread work completes safely.
On CancelledError, remove project_path before propagating cancellation,
including cleanup when cancellation occurs during project creation or write-plan
application. Add a cancellation test that blocks _apply_write_plan and verifies
no uvai_project_* directory remains.
---
Outside diff comments:
In `@src/youtube_extension/backend/code_generator.py`:
- Around line 271-299: Sanitize all analysis-derived values before constructing
generated artifacts in the code-generation flow, including the paths around
_generate_index_html, _generate_react_app_component, _generate_readme, and the
related Python output generation. Use context-appropriate safe Python literals,
HTML escaping, and JavaScript serialization for titles, summaries, technologies,
features, and concepts. Parse generated Python with ast.parse and syntax-check
generated JavaScript before calling _apply_write_plan, ensuring only validated
artifacts are written or deployed.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2be2b63-07e4-4173-a389-d3a9b68ae098
⛔ Files ignored due to path filters (1)
tests/unit/test_code_generator.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/code_generator.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: trivy
- GitHub Check: Generate and Upload Coverage
- GitHub Check: Security Scan - python
- GitHub Check: Security Scan - javascript
- GitHub Check: test
⚠️ CI failures not shown inline (4)
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: batch project scaffolding disk writes off the event loop
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const owner = context.repo.owner;
const repo = context.repo.repo;
const marker = '<!-- agent-completion-truth-gate:v1 -->';
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {...
GitHub Actions: PR Checks / 0_agent-completion_truth-gate.txt: perf: batch project scaffolding disk writes off the event loop
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const owner = context.repo.owner;
const repo = context.repo.repo;
const marker = '<!-- agent-completion-truth-gate:v1 -->';
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {...
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: batch project scaffolding disk writes off the event loop
Conclusion: failure
##[group]Run exit 1
�[36;1mexit 1�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
##[error]Process completed with exit code 1.
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: batch project scaffolding disk writes off the event loop
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const owner = context.repo.owner;
const repo = context.repo.repo;
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {
return 'fail_closed';
}
if (ownerId > expectedId) {
return 'successor';...
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/backend/code_generator.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
Files:
src/youtube_extension/backend/code_generator.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
src/youtube_extension/backend/code_generator.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/backend/code_generator.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Run Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Use Anthropic SDK featuresthinking={"type": "adaptive"}andoutput_config={"effort": "..."}withanthropic>=0.105.0; do not addTypeErrorfallbacks for these parameters.Use the service container dependency-injection pattern in
backend/containers/.
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi}: Use Black formatting with an 88-character line length for Python code.
Use Ruff with rules E, W, F, I, B, C4, and UP for Python linting.
Use strict mypy type checking; do not define untyped functions.
Target Python 3.9 or newer.
Validate Python inputs with Pydantic.
Sanitize subprocess arguments before execution.
Do not add mock delays or fake data to production Python code; production runs in REAL_MODE_ONLY.
Keep secrets out of Python source code; load keys and credentials from environment variables instead.
Use absolute imports that resolve withPYTHONPATH=srcin the Python backend.
Files:
src/youtube_extension/backend/code_generator.py
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
src/youtube_extension/backend/code_generator.py
🪛 ast-grep (0.45.0)
src/youtube_extension/backend/code_generator.py
[warning] 49-49: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 289-289: use jsonify instead of json.dumps for JSON output
Context: json.dumps(package_json, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔍 Remote MCP GitHub Copilot
Relevant review context
-
Blocking concern: cancellation cleanup. PR
#1251adds cancellation points aroundmkdtempand each batched write. Cancelling the awaiting coroutine does not stop the worker thread, andvideo_processing_service.pycurrently has no cleanup for the generated directory. This can leave partial or completeuvai_project_*directories orphaned. -
The repository already uses a cancellation-safe pattern: create a task for
asyncio.to_thread, shield it, wait for completion, then re-raiseCancelledError. -
Shared executor follow-up:
asyncio.to_threaduses the shared default executor. Issue#1234documents that the repository has 63asyncio.to_threadand 31run_in_executor(None, ...)call sites sharing that pool; this PR adds four more. This does not invalidate the event-loop fix, but stalled filesystem workers can still contribute to executor starvation. -
The PR’s diff preserves ordered writes and explicitly tests byte content, ordering/error behavior, thread identity, and one-hop batching. The current review discussion contains no formal review threads, but CodeRabbit identified the cancellation issue above.
|
Context for reviewers: the blocking cancellation-leak finding CodeRabbit raised on this PR (a request cancelled mid-scaffold could leave an orphan It is fixed in #1252, which builds on this exact commit (
#1252 is a strict superset of this PR, so merging this branch on its own would ship the perf change with the leak still present. Recommend reviewing/merging #1252 instead (and closing this once its commit lands), or landing this first and rebasing #1252 down to just the cancellation fix — reviewer's call. Both PRs are otherwise green except the shared I can't push the fix here directly — this PR is on Generated by Claude Code |
Moving the project writes onto worker threads added suspension points the inline sequence did not have, so a cancelled request could unwind while a worker thread was still writing into a directory no caller would ever receive. Drain the scaffolding task through a shield loop before propagating the cancellation, then remove the directory that generate_project created. Mirrors _run_sync_rpc in services/cloud/cloud_tasks_queue.py. CancelledError is never suppressed. The pre-existing generic-exception leak is tracked separately in #1254. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
generate_project is the only holder of the scaffold path until it returns, so an exception on the way out left a directory that no caller could name, let alone remove. Wrap the dispatch-to-return region in try/except Exception and discard the directory before re-raising. CancelledError derives from BaseException, so the explicit cancellation path is unaffected and cannot double-remove. _discard_project_dir suppresses everything and drains its worker, so cleanup can never mask the original exception. Also close a coverage gap the Copilot reviewer found: the off-loop test recorded only open(), so moving Path.mkdir back onto the loop still passed. It now hooks Path.mkdir as well, and a dedicated test covers the one generator that creates subdirectories. Resolves #1253, #1254. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🔍 PR Validation |
|
Correction to my earlier comment above (5159578559), and status of the overlapping work. That comment claimed the orphan-directory leak "is not fixed on this branch — head Overlap with #1252. #1252 addresses the same two defects on the same file. Having read its diff in full, it converges independently on the same design — a shield-and-drain helper derived from the existing The differences:
The last two rows are why I'm proposing this branch carry the work. I want to be fair about the direction of that critique: #1252 was ahead of this branch on the generic-exception path, and that gap was real. It is now closed here in Closing #1252, #1253 and #1254 as superseded by this PR. Reopening #1252 is the right move if this branch stalls — the analysis in it is sound, and I'd rather it land than nothing land. |
|
@coderabbitai review @linear-code this one is still awaiting your read — flagging it since you've reviewed the previous four in this series. Both blocking findings are addressed at
The one finding I have declined is the placeholder-FastAPI template at L470 — pre-existing content this PR does not touch, tracked as #1257. Byte-for-byte output equality is proven by differential execution, which is also the evidence that the template is unmodified. Worth attacking if you want a target: the failed-generation cleanup now removes a directory the caller never sees. I checked that no test asserts on |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR changes ==='
git diff --name-status origin/main...HEAD
git diff --find-renames --unified=100 origin/main...HEAD -- \
'src/youtube_extension/backend/code_generator.py' \
'*test*code_generator*.py' \
'src/youtube_extension/**/video_processing_service.py'
echo '=== Generator outline ==='
ast-grep outline src/youtube_extension/backend/code_generator.py \
--items all --type class,function --view expanded
echo '=== Cancellation, cleanup, thread, and generation references ==='
rg -n -C 12 \
'(_run_to_completion|_discard_project_dir|_make_scaffold_dir|generate_project|_apply_write_plan|asyncio\.to_thread|CancelledError|rmtree|mkdtemp)' \
src/youtube_extension/backend/code_generator.py \
src/youtube_extension \
tests \
-g '*.py'
echo '=== Direct generator call sites and use of returned project paths ==='
rg -n -C 16 \
'(await\s+.*generate_project\s*\(|generate_project\s*\(|project_path|output_dir|rmtree\()' \
src tests \
-g '*.py'Length of output: 201305 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed paths ==='
git diff --name-status origin/main...HEAD
echo '=== Exact generator diff ==='
git diff --unified=35 origin/main...HEAD -- src/youtube_extension/backend/code_generator.py
echo '=== Current generator control flow ==='
nl -ba src/youtube_extension/backend/code_generator.py | sed -n '1,115p;190,290p;330,490p'
echo '=== Locate focused tests ==='
fd -t f -i 'code*generator' tests src | sort
echo '=== Test definitions that exercise scaffold cleanup and cancellation ==='
rg -n -C 10 \
'(run_to_completion|discard_project_dir|generate_project|CancelledError|cancel\(|rmtree|mkdtemp|write_plan|original_exception|failure)' \
tests -g '*.py' -g '*code*generator*'
echo '=== Python-version and lint configuration ==='
rg -n -C 3 '(target-version|python_requires|requires-python|python_version)' \
pyproject.toml setup.cfg setup.py tox.ini .python-version 2>/dev/null || trueLength of output: 24523 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Focused test names ==='
grep -nE \
'^(async )?def test_.*(cancel|cleanup|failure|exception|thread|write_plan|mkdir|byte|order)|^(class )?Test' \
tests/unit/test_code_generator.py || true
echo '=== Focused test bodies and imports ==='
grep -n -A 55 -B 10 -E \
'(test_.*(cancel|cleanup|failure|exception|thread|write_plan|mkdir|byte|order)|_run_to_completion|_discard_project_dir|generate_project)' \
tests/unit/test_code_generator.py || true
echo '=== Generator helpers and generate_project ==='
sed -n '1,115p;197,288p' src/youtube_extension/backend/code_generator.py
echo '=== Runtime version declarations ==='
for f in pyproject.toml setup.cfg setup.py tox.ini .python-version; do
if [ -f "$f" ]; then
echo "--- $f"
grep -nEi -C 3 'target-version|python_requires|requires-python|python_version' "$f" || true
fi
done
echo '=== Direct ProjectCodeGenerator calls only ==='
grep -R -n -E -C 8 \
'(get_code_generator\(\)|ProjectCodeGenerator\(|code_generator\.generate_project\()' \
src tests --include='*.py' || trueLength of output: 14958 🏁 Script executed: #!/bin/bash
set -euo pipefail
test_path="tests/unit/test_code_generator.py"
echo '=== Confirm committed test path ==='
git ls-tree -r --name-only HEAD | grep -Fx "$test_path"
echo '=== Relevant test definitions from HEAD ==='
git show "HEAD:$test_path" | grep -nE \
'^(async )?def test_.*(cancel|cleanup|failure|exception|thread|write_plan|mkdir|byte|order)|^(class )?Test' || true
echo '=== Relevant test bodies from HEAD ==='
git show "HEAD:$test_path" | grep -n -A 70 -B 12 -E \
'(test_.*(cancel|cleanup|failure|exception|thread|write_plan|mkdir|byte|order)|_run_to_completion|_discard_project_dir|generate_project)' || true
echo '=== Test-file diff ==='
git diff --unified=25 origin/main...HEAD -- "$test_path"Length of output: 59215
The failed-generation cleanup is correct for the current ownership model.
The committed tests cover the required cases:
I inspected the committed tests through Git because the sandbox worktree omits ✅ Action performedReview finished.
|
* 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 #1250.
ProjectCodeGeneratorperforms every scaffolding filesystem call inline insideasync defbodies. A blocked write therefore parks the entire event loop, notjust the requesting coroutine.
Outcome
code_generator.pyoff the event loop, batched into oneasyncio.to_threadhop per generator.vm.dirty_ratiowriteback throttling, and it is paid by every coroutine on the worker, not just the requester.Sites converted:
generate_projecttempfile.mkdtemp(...)await asyncio.to_thread(tempfile.mkdtemp, ...)_generate_react_projectopen()/mkdir()calls_generate_vanilla_js_projectopen()calls_generate_python_apiopen()callsRisk
Low, with three risks named explicitly rather than waved away.
Ordering. The write plan is an ordered
list[tuple[Path, Optional[str]]]applied strictly in sequence, so intermediate on-disk states match the old
inline sequence exactly. Directory steps (
Nonecontent) still precede thefiles that live in them. Covered by
test_applies_steps_in_order_so_dirs_precede_their_files.Error semantics.
_apply_write_plansuppresses nothing. A failing stepraises exactly what the inline call would have raised, on the same step, with
earlier steps already applied — identical to the previous behaviour. Covered
by
test_does_not_suppress_errors_and_leaves_earlier_steps_applied, whichuses a real NUL-byte path rather than a mock so the stdlib itself produces
the failure.
json.dump→json.dumps. The React generator previously usedjson.dump(obj, f, indent=2); content is now built withjson.dumps(obj, indent=2). These are byte-identical —dumpwrites exactlythe chunks
dumpsjoins, and neither appends a trailing newline. Asserteddirectly by
test_writes_content_verbatim_without_adding_a_trailing_newlineand confirmed by the differential run below.
Cancellation atomicity — found in review, fixed here. This is the one real
behaviour change and it deserves the detail. Before this PR the three leaf
generators (
_generate_react_project,_generate_vanilla_js_project,_generate_python_api) contained zeroawaitexpressions, so awaiting themnever suspended the task: everything from
mkdtemptoreturnran without asingle cancellation point. Adding
await asyncio.to_thread(...)created fourreal suspension points inside a previously uninterruptible region, so a
cancelled request could unwind while a worker thread was still writing into a
directory whose path no caller would ever receive.
Measured on all three variants, cancelling mid-scaffold in an isolated
TMPDIR:maintodayCancelledErrorCancelledErrorThe fix drains the scaffolding task through a shield loop, removes the
directory
generate_projectcreated, then re-raises.CancelledErroris neversuppressed and the drain is what makes removal safe — deleting a tree while a
worker may still be writing into it is its own bug. The pattern mirrors
_run_sync_rpcinservices/cloud/cloud_tasks_queue.py. Cancellation latencycost is the remainder of one write batch. Covered by
test_cancellation_removes_the_project_directory,test_cancellation_drains_the_writer_before_unwindingandtest_cancellation_is_reported_as_cancellation; reverting only the fix failsexactly the first two.
Branch selection collapsed 4 → 3.
project_type == "web"and theelsefallback both called
_generate_web_project, so they are now one branch.Guarded by
test_uncancelled_web_request_still_returns_a_project, whichcompares the full on-disk tree produced by an explicit
"web"request againstan unrecognised type.
Failed generations no longer strand a directory. The pre-existing
generic-exception path (
except Exception: logger.error(...); raise) alsoleft the directory behind. That predates this change, but
generate_projectis the only holder of the path until it returns, so nothing downstream can
ever clean it up; the caller in
video_processing_service.pyreadsproject_pathonly on the success path (L390). The whole region from branchselection through
returnis now wrapped intry/except Exception: await _discard_project_dir(...); raise.CancelledErrorderives fromBaseException, so the cancellationpath above is unaffected and cannot double-remove.
_discard_project_dirsuppresses every exception and drains its worker, socleanup can never mask the error that caused it — asserted directly by
test_original_exception_is_not_masked_by_cleanup, which makes both thegeneration and
shutil.rmtreeraise and requires the original error tosurface.
test_failed_generation_leaves_no_orphan_directoryfails withoutthe fix;
test_successful_generation_keeps_its_directoryguards against thecleanup firing on the happy path. This subsumes the separate reports Cancelled video-to-software request leaks the scaffold temp directory #1253
and Scaffolding failure leaves an orphaned project directory #1254, both now closed as superseded.
Also not claimed: the differential harness compares file contents and return
values, not file metadata, and the new suspension points do let a concurrent
coroutine on the same loop observe intermediate on-disk states that were
previously invisible. Nothing in this repo reads a project directory while it is
being generated, but the window is real and is stated rather than hidden.
Not claimed: this does not bound worst-case completion time. If the filesystem
hangs indefinitely, the worker thread still hangs — it just no longer takes the
event loop with it. The bound is on blast radius, not on latency.
Out of scope:
ensure_templates_directoryis a plaindef, not a coroutine, soits
mkdirnever touches the loop and is deliberately left alone.Verification
Static. An AST scan for blocking filesystem primitives inside
async defbodies reports 0 remaining (was 28). The scan also caught a site the initial
sweep missed —
tempfile.mkdtemp— which is included above.Differential — byte-for-byte output equality. The pre-change module and the
post-change module were loaded side by side in one process and driven through
all three generators with identical input. Every emitted file was SHA-256'd and
every returned dict compared:
Tests. 19 new tests,
70 passed→89 passed.Off-loop proofs assert thread identity, never wall-clock elapsed time: a
timing threshold would be flaky under CI contention and would still pass if the
work ran on the loop but happened to be fast.
Three of the new tests guard the batching specifically — a regression that
offloaded each write individually would still satisfy the thread-identity
assertions while paying one context switch per file, so
to_threadisinstrumented and asserted to be called exactly once per generator.
Prove-fail, twice, each against the specific semantics rather than the file.
Off-loop hops. The three
to_threadhops and themkdtemphop were reverted inplace while leaving
_apply_write_plandefined, so the failures are genuineassertion failures rather than import errors:
Exactly the 7 off-loop tests failed. The 4
_apply_write_plancontract testscorrectly continued to pass, since the helper itself was unchanged.
Cancellation fix. Reverting only the drain-and-discard change, leaving the
off-loop work intact:
Exactly
test_cancellation_removes_the_project_directoryandtest_cancellation_drains_the_writer_before_unwindingfailed.test_cancellation_is_reported_as_cancellationcorrectly passed both ways —the unfixed code did propagate
CancelledError, it just leaked the directory —and the branch-selection guard is a behaviour-preservation test, so it also
passes both ways.
Failed-generation cleanup. Reverting only the
try/except Exceptionblock:test_failed_generation_leaves_no_orphan_directoryfailed.test_original_exception_is_not_masked_by_cleanupcorrectly passed both ways —without cleanup there is nothing to mask — and
test_successful_generation_keeps_its_directoryis a happy-path guard, so italso passes both ways. Both are still worth having: the first is the assertion
that would fail if the guard inside
_discard_project_dirwere ever narrowed,and the second is what fails if the cleanup is made unconditional.
The
Path.mkdircoverage gap, closed. The Copilot reviewer observed that theoff-loop test hooked only
open(), so directory creation was unguarded. Ratherthan assert that from inspection, the defect was injected:
src_dir.mkdir()andpublic_dir.mkdir()were moved out of the write plan and back onto the loop,then both versions of the test file were run against that same tree.
85 passed— undetected2 failed, 87 passed— caughtThe hook is
Path.mkdir, recorded alongsideopenand asserted over the union.Two ordering details matter: the project root is created before patching, or
the hook fires during setup; and
_generate_vanilla_js_projectand_generate_python_apicreate no subdirectories at all, so the parametrised testcannot assert the
mkdirrecord is non-empty. A dedicated react-only test doesthat, which is what stops the union assertion from being vacuous.
All files were restored after each injection and verified with an empty
diff.Wider sweep.
638 passedacrosstest_code_generator.py,test_code_generator_agent.py,test_ai_code_generator.py,test_video_processing_service.py,test_deployment_manager.pyandtest_transcript_action_workflow.py.Byte equality re-confirmed after the cancellation fix — the differential
harness was re-run against the final tree and still reports
BYTE-FOR-BYTE IDENTICAL: True.Lint. Ruff diagnostic parity against
origin/main—PARITY OKon both thesource and the test file. No new diagnostics, no suppressed ones.
Production evidence
Reachable from a live HTTP endpoint, verified at call level rather than by
import graph alone:
This is the same endpoint whose subprocess stalls were accepted and fixed in
#1239 / #1240. That change moved
npm install/npm run build/npx tscoffthe loop; this change closes the remaining inline filesystem work on the same
request path, so the endpoint no longer blocks the loop anywhere in its
scaffolding phase.