perf: fix build errors concurrently with bounded fan-out - #1336
Conversation
fix_build_errors repaired each failing file in a strictly serial loop, performing blocking Path.read_text/write_text on the event loop thread around each LLM round-trip. The files are mutually independent, so wall clock scaled linearly with the number of broken files. - offload read_text/write_text via asyncio.to_thread - fan out per-file fixes with asyncio.gather, bounded by a semaphore (default 4, overridable via the new max_concurrency parameter) - clamp non-positive max_concurrency to 1 - sort fixed_files for deterministic results (error_files is a set) Failure semantics are unchanged: a missing, unreadable, or provider-failed file is skipped without aborting its siblings. DeploymentManager.verify_and_fix_project calls this from inside a retry loop, so the serial cost was multiplied by max_retries per deployment. Closes #1335 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesBuild error fix processing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant fix_build_errors
participant FileSystem
participant Router
fix_build_errors->>FileSystem: Read file via asyncio.to_thread
fix_build_errors->>Router: Generate fix
Router-->>fix_build_errors: Return generated response
fix_build_errors->>FileSystem: Write fix via asyncio.to_thread
fix_build_errors-->>fix_build_errors: Gather sorted successful paths
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
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 |
|
@linear-code @coderabbitai review Concurrency change to Specific things worth your attention:
Billed LLM call count is unchanged — exactly one |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/youtube_extension/backend/ai_code_generator.py --items all --match 'fix_build_errors|_MAX_CONCURRENT_FIXES'
ast-grep outline tests/unit/test_ai_code_generator.py --items all --match 'TestFixBuildErrors|TestFixBuildErrorsConcurrency'
printf '\n--- implementation references ---\n'
rg -n -C 5 '_MAX_CONCURRENT_FIXES|async def fix_build_errors|def fix_build_errors|asyncio\.gather|asyncio\.Semaphore|to_thread' \
src/youtube_extension/backend/ai_code_generator.py
printf '\n--- concurrency test references ---\n'
rg -n -C 5 'class TestFixBuildErrorsConcurrency|test_files_are_fixed_concurrently|test_default_concurrency_is_bounded|test_max_concurrency_override_is_respected|test_non_positive_max_concurrency_clamps_to_one|test_fixed_files_order_is_deterministic|test_file_io_runs_off_the_event_loop_thread' \
tests/unit/test_ai_code_generator.py
printf '\n--- call sites ---\n'
rg -n -C 3 '\.fix_build_errors\s*\(|\bfix_build_errors\s*\(' src testsLength of output: 5275 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate test files ---'
fd -t f -i 'test_ai_code_generator.py' .
fd -t f -i '*ai*code*generator*' tests . 2>/dev/null || true
printf '%s\n' '--- imports and complete fix_build_errors body ---'
sed -n '1,55p' src/youtube_extension/backend/ai_code_generator.py
sed -n '1265,1415p' src/youtube_extension/backend/ai_code_generator.py
printf '%s\n' '--- existing semaphore pattern near stated location ---'
sed -n '590,650p' src/youtube_extension/backend/ai_code_generator.py
test_file="$(fd -t f -i 'test_ai_code_generator.py' . | head -n 1)"
if [ -n "$test_file" ]; then
printf '%s\n' "--- test outline: $test_file ---"
ast-grep outline "$test_file" --items all --match 'TestFixBuildErrors|TestFixBuildErrorsConcurrency'
printf '%s\n' '--- relevant test blocks ---'
rg -n -C 8 'class TestFixBuildErrors\b|class TestFixBuildErrorsConcurrency\b|test_files_are_fixed_concurrently|test_default_concurrency_is_bounded|test_max_concurrency_override_is_respected|test_non_positive_max_concurrency_clamps_to_one|test_fixed_files_order_is_deterministic|test_file_io_runs_off_the_event_loop_thread|test_.*(missing|read|provider|exception)' "$test_file"
else
printf '%s\n' 'Test file not found.'
fiLength of output: 11193 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed paths relative to merge base ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "$base" ]; then
git diff --name-status "$base"...HEAD
printf '%s\n' '--- relevant diff ---'
git diff --unified=40 "$base"...HEAD -- src/youtube_extension/backend/ai_code_generator.py
git diff --unified=20 "$base"...HEAD -- ':!src/youtube_extension/backend/ai_code_generator.py'
else
printf '%s\n' 'Could not determine a local main merge base.'
git status --short
fi
printf '%s\n' '--- tracked test directories and matching symbols ---'
fd -t d -d 4 'test*' . | head -n 80
rg -n -C 3 --glob '*.py' 'class TestFixBuildErrors\b|class TestFixBuildErrorsConcurrency\b|fix_build_errors\s*\(' . \
-g '!src/youtube_extension/backend/ai_code_generator.py'Length of output: 22811
Catch
A cancellation during Use a tracked write task with The other requested behavior is correct:
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Improves build-error repair performance for issue #1335 through concurrent, off-thread processing.
Changes:
- Adds configurable, bounded per-file concurrency.
- Offloads file I/O from the event loop.
- Adds concurrency, ordering, and failure-isolation tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/youtube_extension/backend/ai_code_generator.py |
Implements concurrent repairs and off-thread I/O. |
tests/unit/test_ai_code_generator.py |
Tests concurrency limits, ordering, I/O threads, and isolation. |
|
|
||
| try: | ||
| current_content = await asyncio.to_thread(_read_source) | ||
| except OSError as e: |
| if max_concurrency is None | ||
| else max(1, max_concurrency) | ||
| ) | ||
| semaphore = asyncio.Semaphore(limit) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/ai_code_generator.py (1)
1368-1384: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate generated TypeScript before overwriting the source file.
Line 1384 writes raw LLM output after fence extraction. A malformed or prose response can replace a source file and make the next build fail.
Reject invalid output before the write. Use an actual TypeScript parser or compiler when available. At minimum, call
validate_typescript_syntaxand returnNonewhen it reports errors.As per path instructions, “Flag any code generation output that reaches users without AST validation or syntax checking.”
🤖 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/ai_code_generator.py` around lines 1368 - 1384, Validate the extracted generated content with validate_typescript_syntax before the file_path.write_text call, and return None when validation reports errors. Keep the existing markdown fence extraction unchanged, and ensure no raw or prose LLM output reaches the source file without syntax validation.Source: 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/ai_code_generator.py`:
- Around line 1308-1318: Enforce a finite timeout around each LLMRouter.generate
invocation in the bounded repair flow using asyncio’s timeout mechanism,
ensuring stalled provider calls release their semaphore slot and do not block
repairs indefinitely. Locate the generate call associated with the semaphore
created from _MAX_CONCURRENT_FIXES; preserve the existing retry and concurrency
behavior while applying the timeout to the complete asynchronous operation.
- Around line 1331-1335: Update the read failure handler in fix_build_errors
around _read_source to catch Exception rather than only OSError, while
preserving cancellation propagation by not catching BaseException. Add a
regression test covering one file raising UnicodeDecodeError during reading
while another file is still fixed successfully.
- Around line 1384-1386: Update the write flow in the surrounding repair method
to create a task for file_path.write_text via asyncio.to_thread, await it
through asyncio.shield, and catch CancelledError to await the underlying task
before re-raising cancellation. Keep the existing success logging and rel_path
return unchanged.
---
Outside diff comments:
In `@src/youtube_extension/backend/ai_code_generator.py`:
- Around line 1368-1384: Validate the extracted generated content with
validate_typescript_syntax before the file_path.write_text call, and return None
when validation reports errors. Keep the existing markdown fence extraction
unchanged, and ensure no raw or prose LLM output reaches the source file without
syntax validation.
🪄 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: d7c0f55f-7145-45e2-9ffb-75ef2be46c39
⛔ Files ignored due to path filters (1)
tests/unit/test_ai_code_generator.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/ai_code_generator.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: trivy
- GitHub Check: Generate and Upload Coverage
- GitHub Check: test
🧰 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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_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/ai_code_generator.py
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#1336is open againstmain, changing onlyai_code_generator.pyand its unit tests; GitHub reports the PR as mergeable: unstable. - The production call occurs inside
DeploymentManager.verify_and_fix_project, which retries verification up to two times and callsfix_build_errorswith the existing three positional arguments; the new optional parameter is therefore source-compatible. - The implementation bounds the entire per-file operation—including read, LLM call, and write—behind a semaphore, uses
gatherwith defaultreturn_exceptions=False, and sorts input paths before gathering. - Review risk: only
OSErroris converted to a per-file failure during reads. Other read exceptions can escape_fix_one, defeating the intended failure isolation; add a regression test for a non-OSErrorread failure. - Review risk: writes use a bare
await asyncio.to_thread(...). The repository already contains_run_to_completion, which explicitly shields and drains thread work because cancellation cannot stop an already-running worker thread. This pattern should be considered here for cancellation-safe writes. - The repository has an established precedent in PR
#1188for explicitly preserving cancellation when usinggather(return_exceptions=True). - No review threads currently exist. Several CI jobs—including test, build, coverage, Trivy, and Python security scanning—were still in progress; CodeQL was neutral.
Context7 documentation lookup was rate-limited, so no external documentation was used.
| # Each error file is independent: its own read, its own AI call and its | ||
| # own write. self.router.generate() is a multi-second network round-trip, | ||
| # so running them one after another made wall-clock cost scale linearly | ||
| # with the number of failing files. Bound the fan-out so we do not trip | ||
| # provider rate limits. | ||
| limit = ( | ||
| _MAX_CONCURRENT_FIXES | ||
| if max_concurrency is None | ||
| else max(1, max_concurrency) | ||
| ) | ||
| semaphore = asyncio.Semaphore(limit) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 --glob '*.py' \
'async def generate|def generate|router\.generate|timeout' srcRepository: groupthinking/EventRelay
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== llm_router outline =="
ast-grep outline src/youtube_extension/backend/llm_router.py --view expanded || true
echo
echo "== llm_router relevant section =="
sed -n '1,220p' src/youtube_extension/backend/llm_router.py
echo
echo "== router occurrences in ai_code_generator =="
rg -n -C 4 'self\.router|_MAX_CONCURRENT_FIXES|asyncio\.Semaphore|wait_for|generate\(' src/youtube_extension/backend/ai_code_generator.py | sed -n '1,240p'Repository: groupthinking/EventRelay
Length of output: 15449
Enforce a timeout on LLMRouter.generate.
LLMRouter.generate launches asyncio.to_thread(self._dispatch_sync, ...) but _dispatch_sync retries three providers sequentially without any timeout. A stalled provider call can hold one semaphore slot and wait through other providers; with _MAX_CONCURRENT_FIXES = 4, four stalled calls can exhaust the semaphore and block all repairs indefinitely. Wrap the generate call or its internal SDK client with a finite timeout before accepting semaphore capacity.
🤖 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/ai_code_generator.py` around lines 1308 - 1318,
Enforce a finite timeout around each LLMRouter.generate invocation in the
bounded repair flow using asyncio’s timeout mechanism, ensuring stalled provider
calls release their semaphore slot and do not block repairs indefinitely. Locate
the generate call associated with the semaphore created from
_MAX_CONCURRENT_FIXES; preserve the existing retry and concurrency behavior
while applying the timeout to the complete asynchronous operation.
Source: Path instructions
Addresses both review findings on #1336. 1. UnicodeDecodeError escaped the read guard. Path.read_text() raises UnicodeDecodeError on a non-UTF-8 source. That is a ValueError subclass, not an OSError, so it slipped past `except OSError` and out of _fix_one. With gather(return_exceptions=False) that does not merely lose one file -- it aborts the entire fan-out and discards every sibling's already-completed fix. The guard now catches (OSError, UnicodeError). 2. The concurrency bound was per-invocation, not process-wide. get_deployment_manager() constructs a fresh DeploymentManager, and therefore a fresh AICodeGenerator, for every pipeline run. A semaphore owned by a single fix_build_errors call let M concurrent deployments issue M * limit LLM calls, defeating the rate-limit protection the bound exists to provide. _shared_fix_semaphore() now returns one semaphore per (running loop, limit). Keying on the loop is required because asyncio primitives bind to the first loop that awaits them, so a plain module-level singleton would raise "bound to a different event loop" as soon as a second loop used it. A WeakKeyDictionary keeps finished loops from leaking, and a TypeError fallback covers loop implementations that are not weak-referenceable. Both fixes are prove-failed: each new test fails against the previous commit and passes here. 252 passed (was 250). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@linear-code @coderabbitai Both review findings actioned in 1. New test 2.
One subtlety worth flagging, since the obvious fix is wrong: a plain module-level New test Verification: 252 passed (was 250). One open question I'd like your call on: I scoped this to the generator because the PR is a perf change. The arguably more correct home for a provider rate limit is the router/provider layer, where it would also cover |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR commit and changed files ---'
git show --no-ext-diff --stat --oneline b57b22b2c
git diff --no-ext-diff --unified=35 b57b22b2c^ b57b22b2c -- \
src/youtube_extension/backend/ai_code_generator.py \
tests/unit/test_ai_code_generator.py
printf '%s\n' '--- implementation outline ---'
ast-grep outline src/youtube_extension/backend/ai_code_generator.py --items all \
--match '_shared_fix_semaphore|fix_build_errors|generate_file|generate'
printf '%s\n' '--- shared semaphore and repair implementation ---'
rg -n -C 20 --glob '*.py' \
'def _shared_fix_semaphore|async def fix_build_errors|except \(OSError, UnicodeError\)|asyncio\.shield|WeakKeyDictionary|_shared_fix' \
src/youtube_extension/backend/ai_code_generator.py
printf '%s\n' '--- new test implementations ---'
rg -n -C 15 --glob '*.py' \
'test_undecodable_source_does_not_abort_siblings|test_concurrent_invocations_share_the_fix_budget' \
tests/unit/test_ai_code_generator.py
printf '%s\n' '--- provider or router generation entry points ---'
rg -n -C 4 --glob '*.py' \
'\b(generate_file|generate_architecture|generate)\s*\(' \
src/youtube_extension/backendLength of output: 25744 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate relevant test files ---'
fd -t f -i 'test_ai_code_generator.py' .
fd -t f -i '*ai*code*generator*.py' . | sed -n '1,40p'
printf '%s\n' '--- complete per-file repair body ---'
sed -n '1355,1465p' src/youtube_extension/backend/ai_code_generator.py
test_file="$(fd -t f -i 'test_ai_code_generator.py' . | head -n 1)"
if [ -z "$test_file" ]; then
echo 'ERROR: test_ai_code_generator.py was not found' >&2
exit 1
fi
printf '%s\n' "--- relevant tests: $test_file ---"
rg -n -C 18 \
'test_undecodable_source_does_not_abort_siblings|test_concurrent_invocations_share_the_fix_budget|cancel|shield|write' \
"$test_file"
printf '%s\n' '--- existing shield-and-drain precedents ---'
rg -n -C 10 --glob '*.py' \
'_run_to_completion|asyncio\.shield\(|CancelledError' \
src testsLength of output: 1482 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate relevant test files ---'
fd --glob --type f 'test_ai_code_generator.py' .
fd --glob --type f '*ai*code*generator*.py' . | sed -n '1,40p'
printf '%s\n' '--- complete per-file repair body ---'
sed -n '1355,1465p' src/youtube_extension/backend/ai_code_generator.py
test_file="$(fd --glob --type f 'test_ai_code_generator.py' . | head -n 1)"
if [ -z "$test_file" ]; then
echo 'ERROR: test_ai_code_generator.py was not found' >&2
exit 1
fi
printf '%s\n' "--- relevant tests: $test_file ---"
rg -n -C 18 \
'test_undecodable_source_does_not_abort_siblings|test_concurrent_invocations_share_the_fix_budget|cancel|shield|write' \
"$test_file" || true
printf '%s\n' '--- existing shield-and-drain precedents ---'
rg -n -C 10 --glob '*.py' \
'_run_to_completion|asyncio\.shield\(|CancelledError' \
src tests || trueLength of output: 5753
The cancellation finding is still open. The current write path is: await asyncio.to_thread(file_path.write_text, fixed_code)It has no tracked task, The shared semaphore change correctly limits concurrent repairs across generator instances on the same event loop. Its comment should not say “process-wide” without qualification. Separate event loops use separate semaphores. This is necessary for asyncio loop affinity. The generator-level bound is sufficient for this PR. Do not move the limit into the router/provider layer here. A router/provider limiter would change the concurrency policy for Verification of the new tests is inconclusive because this sandbox checkout does not contain ✏️ Learnings added
You are interacting with an AI system. |
|
Red-team verdict (automated PR-remediation routine) — reviewed the diff and the CodeRabbit/Copilot threads independently. 🔴 Merge-blocking — confirmed real: 🟠 Cancellation sanity-check (you asked directly): CodeRabbit has the better of it, and the "no ⚪ Out of scope — defer deliberately: the Not merging from this routine. CI is green and the truth-gate passed, but the PR is Generated by Claude Code |
|
Both reviewers converged on the same two correctness issues — I've addressed them in a stacked follow-up, #1340 (base 1. 2. Cancellation abandons an in-flight write — fixed. Confirmed: a 3. Per-invocation semaphore (CodeRabbit) — acknowledged, deliberately deferred, not silently resolved. You're right that each Verification on #1340: both new tests prove-failed against the unpatched source; full Generated by Claude Code |
asyncio.to_thread cannot interrupt a worker thread, so cancelling a bare `await asyncio.to_thread(file_path.write_text, ...)` returns control to the caller while the thread is still truncating and rewriting the file. gather() cancels every sibling as soon as one task raises, so this is reachable in normal operation: the caller sees a failed repair and may start cleanup on a file that is actively being written. Shield the write so cancellation cannot detach it, then drain it before re-raising. Exceptions from the drain are suppressed so a failing write cannot swallow the cancellation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@linear-code @coderabbitai Thanks — third finding actioned in Finding: Fix: shield the write so cancellation cannot detach it, then drain before re-raising. write = asyncio.create_task(asyncio.to_thread(file_path.write_text, fixed_code))
try:
await asyncio.shield(write)
except asyncio.CancelledError:
with contextlib.suppress(Exception):
await write
raiseTwo details worth flagging for review:
Prove-fail (new test
All three findings from this review are now addressed ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed files and cancellation-fix diff ---'
git show --no-ext-diff --stat --oneline 370bcec53
git diff --no-ext-diff --unified=45 370bcec53^ 370bcec53 -- \
src/youtube_extension/backend/ai_code_generator.py \
tests/unit/test_ai_code_generator.py
printf '%s\n' '--- repair implementation map ---'
ast-grep outline src/youtube_extension/backend/ai_code_generator.py --items all \
--match 'fix_build_errors|_shared_fix_semaphore|_run_to_completion'
printf '%s\n' '--- cancellation, read isolation, and write control flow ---'
rg -n -C 25 --glob '*.py' \
'async def fix_build_errors|asyncio\.create_task|asyncio\.shield|CancelledError|contextlib\.suppress|to_thread|except \(OSError, UnicodeError\)|except Exception' \
src/youtube_extension/backend/ai_code_generator.py
printf '%s\n' '--- cancellation regression test ---'
test_file="$(fd --type f --glob 'test_ai_code_generator.py' . | head -n 1)"
if [ -z "$test_file" ]; then
echo 'ERROR: test_ai_code_generator.py not found' >&2
exit 1
fi
ast-grep outline "$test_file" --items all --match 'test_cancellation_drains_the_in_flight_write'
rg -n -C 35 --glob '*.py' \
'test_cancellation_drains_the_in_flight_write|threading\.Event|write_text|CancelledError' \
"$test_file"Length of output: 34906
The write task is tracked. The regression test opens the race window before it cancels the outer task. It verifies that cancellation returns only after the worker-thread write completes. This test covers the production failure mode. One correction is required in the code comments and test docstring. I did not execute the test suite in this environment. The supplied diff confirms the implementation and test structure. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
The worker thread now blocks on an explicit Event that the test releases only after cancelling, instead of sleeping for a fixed interval. A fixed sleep can elapse before the cancellation lands on a loaded runner, which would let the test pass without ever opening the race window it exists to prove. Verified 3/3 pass with the drain and 3/3 fail without it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Status triage (automated remediation pass)Review findings — resolved in code (head
Governance checks: The remaining red This PR touches only
Net: #1336's own diff is sound and its checks are green except the external base-branch Generated by Claude Code |
|
@coderabbitai @linear-code — all three review findings are fixed and verified. Requesting a re-review; the 1.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
printf '%s\n' '--- current head and changed paths ---'
git show --no-ext-diff --format='%H%n%s' -s HEAD
git diff --name-status "$base"...HEAD
printf '%s\n' '--- relevant implementation outline ---'
ast-grep outline src/youtube_extension/backend/ai_code_generator.py --items all \
--match 'fix_build_errors|_shared_fix_semaphore|_FIX_SEMAPHORES|_read_source'
printf '%s\n' '--- implementation: concurrency, read handling, and write cancellation ---'
rg -n -C 28 --glob '*.py' \
'_FIX_SEMAPHORES|_shared_fix_semaphore|async def fix_build_errors|asyncio\.Semaphore|WeakKeyDictionary|read_text|except \(OSError, UnicodeError\)|create_task|asyncio\.shield|CancelledError|contextlib\.suppress|asyncio\.gather' \
src/youtube_extension/backend/ai_code_generator.py
test_file="$(fd --type f --glob 'test_ai_code_generator.py' . | head -n 1)"
if [ -z "$test_file" ]; then
echo 'ERROR: test_ai_code_generator.py not found' >&2
exit 1
fi
printf '%s\n' "--- relevant tests: $test_file ---"
ast-grep outline "$test_file" --items all --match 'TestFixBuildErrorsConcurrency|test_undecodable|test_concurrent_invocations|test.*loop|test_cancellation_drains'
rg -n -C 35 --glob '*.py' \
'test_undecodable|test_concurrent_invocations_share_the_fix_budget|test_.*loop|test_cancellation_drains_the_in_flight_write|UnicodeDecodeError|asyncio\.run|threading\.Event|write_text' \
"$test_file"
printf '%s\n' '--- full relevant diff ---'
git diff --no-ext-diff --unified=50 "$base"...HEAD -- \
src/youtube_extension/backend/ai_code_generator.py \
"$test_file"
printf '%s\n' '--- GitHub PR status ---'
gh pr view 1336 --repo groupthinking/EventRelay \
--json headRefOid,mergeStateStatus,reviewDecision,statusCheckRollup \
--jq '{headRefOid, mergeStateStatus, reviewDecision, checks: [.statusCheckRollup[] | {name, conclusion, status}]}'Length of output: 15678 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
printf '%s\n' '--- confirm changed test path exists in HEAD ---'
git ls-tree -r --name-only HEAD | rg -n '^tests/unit/test_ai_code_generator\.py$'
printf '%s\n' '--- targeted test definitions and bodies ---'
git show "HEAD:tests/unit/test_ai_code_generator.py" | nl -ba | \
rg -n -C 36 \
'test_undecodable|test_concurrent_invocations_share_the_fix_budget|test_.*loop|test_cancellation_drains_the_in_flight_write|UnicodeDecodeError|asyncio\.run|threading\.Event|write_text'
printf '%s\n' '--- exact current gather-cancellation statements ---'
git grep -n -C 3 -E 'gather\(\).*cancel|cancels every sibling|sibling.*cancel' HEAD -- \
src/youtube_extension/backend/ai_code_generator.py \
tests/unit/test_ai_code_generator.py || true
printf '%s\n' '--- full PR diff for the two changed files ---'
git diff --no-ext-diff --unified=35 "$base"...HEAD -- \
src/youtube_extension/backend/ai_code_generator.py \
tests/unit/test_ai_code_generator.pyLength of output: 1559 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
test_path="tests/unit/test_ai_code_generator.py"
printf '%s\n' '--- targeted regression-test bodies ---'
git show "HEAD:${test_path}" | rg -n -C 42 \
'def test_undecodable|def test_concurrent_invocations_share_the_fix_budget|def test_.*loop|def test_cancellation_drains_the_in_flight_write'
printf '%s\n' '--- current cancellation documentation ---'
git show 'HEAD:src/youtube_extension/backend/ai_code_generator.py' | rg -n -C 8 \
'gather\(\).*cancel|cancels every sibling|Shield the write|drain it before propagating'
printf '%s\n' '--- changed test names in this PR ---'
git diff --no-ext-diff --unified=3 "$base"...HEAD -- "$test_path" | \
rg -n -C 3 '^\+.*def test_|^\+.*async def test_'
printf '%s\n' '--- relevant current PR diff ---'
git diff --no-ext-diff --unified=28 "$base"...HEAD -- \
src/youtube_extension/backend/ai_code_generator.py \
"$test_path"Length of output: 45645
One non-blocking correction remains. Line 1431 and the cancellation-test docstring state that I did not execute the test suite in this sandbox. The inspected implementation and regression-test structure cover the reported production issues. ✅ Action performedReview finished.
|
Canonical issue
Closes #1335
Scope
Two files:
src/youtube_extension/backend/ai_code_generator.py—fix_build_errorsonlytests/unit/test_ai_code_generator.py— newTestFixBuildErrorsConcurrencyclassNo call sites change. The public return shape (
{"success", "fixed_files", "attempted"}) is untouched, andmax_concurrencyis a new optional keyword,so every existing caller is source-compatible.
Outcome
fix_build_errorsrepaired each failing file in a strictly serial loop. Foreach file it ran a blocking
Path.exists()+Path.read_text(), awaited an LLMround-trip, then ran a blocking
Path.write_text()— all on the event loopthread.
The per-file work is mutually independent: each file gets its own prompt, its
own completion and its own write, and no iteration consumes state produced by a
previous one. The serialisation was incidental, not required.
This PR:
read_text/write_textwithasyncio.to_thread, so the loop isno longer blocked on file syscalls;
asyncio.gather, bounded by anasyncio.Semaphore;_MAX_CONCURRENT_FIXES = 4and exposes an optionalmax_concurrencyoverride;fixed_files, which was previously in nondeterministic order becauseerror_filesis aset.With an LLM round-trip dominating each iteration, N broken files now cost
ceil(N / 4)sequential batches instead of N.Risk
Low. Deliberately bounded rather than unbounded.
gatherover a large error list would fireevery request at once. The semaphore caps in-flight requests at 4 by default.
A dedicated test asserts the default bound is never exceeded.
amplification. Exactly one
generatecall per error file is issued, beforeand after — so there is no change in billed LLM spend.
continueto skip a bad file. Thehelper now returns
Nonefor the same cases andgatherruns with thedefault
return_exceptions=False, which is safe precisely because the helpernever propagates: missing files,
OSErroron read, and provider exceptionsare all caught and converted to
None. Two tests cover this.finallyblock in this path touches shared state, so theshield/drain pattern needed in perf: offload blocking SQLite I/O off the event loop #1327 does not apply here; a plainto_threadis correct.one — the previous order was set-iteration order. No existing test asserted on
it (verified across all 10 tests in
TestFixBuildErrors).Verification
Prove-fail. The new class was run against the unmodified source via
git stash push -- src/youtube_extension/backend/ai_code_generator.py:The 7 failures are the new-behaviour assertions:
The 2 that passed pre-change are the failure-isolation cases — they are
regression guards for semantics this PR must preserve, so passing on both
sides is the correct result.
After restoring the change:
That is the pre-existing 352 plus the 9 added here — no regressions in the 10
existing
TestFixBuildErrorstests, nor intest_deployment_manager.py, whichmocks
fix_build_errorsat three call sites.How the concurrency assertions work:
router.generateis replaced with a stubthat increments a counter,
await asyncio.sleep(0.05), then decrements —recording the peak. A serial loop can never record a peak above 1, so
max_inflight > 1is only satisfiable by genuine concurrency. Thread placementis asserted by monkeypatching
Path.read_text/write_textto recordthreading.get_ident()and checking the loop's own ident appears in neitherlist.
ruff checkon both changed files reports 5F841s, all pre-existing onorigin/main(lines 668/675/682/689/1988) and none inside the added class.Production evidence
DeploymentManager.verify_and_fix_project(
src/youtube_extension/backend/deployment_manager.py:343) callsfix_build_errorsfrom inside a retry loop declared at line 301. The serialcost was therefore multiplied by
max_retrieson every deployment whose buildfails — the exact path where latency is most visible to a waiting user.
The semaphore idiom used here matches the existing one in the same file at
line 621, so this introduces no new concurrency pattern to the codebase.
Agent handoff
Nothing outstanding. Behaviour is covered by 9 tests, 7 of which are
prove-failed against the pre-change source. If provider limits ever tighten,
_MAX_CONCURRENT_FIXESis a single module-level constant, and callers canalready pass
max_concurrencyper invocation without a code change here.