fix(perf): keep failure isolation and drain in-flight writes in fix_build_errors (review follow-up to #1336)

[groupthinking]

Canonical issue

Follow-up to #1336 (which closes #1335). This PR targets the perf/ai-fix-parallel branch so its diff is only the two correctness fixes below — merging it updates #1336 in place rather than opening a competing PR against main. No new canonical issue: it remediates review findings on #1336 raised independently by both copilot-pull-request-reviewer and @coderabbitai.

Outcome

Two production-correctness gaps in AICodeGenerator.fix_build_errors are closed, restoring the failure-isolation and cancellation-safety guarantees #1336 claims:

1. UnicodeDecodeError no longer breaks failure isolation. _read_source() caught only OSError. A non-UTF-8 source file raises UnicodeDecodeError (a UnicodeError, not an OSError), which escaped _fix_one, made asyncio.gather(return_exceptions=False) re-raise, and cancelled the sibling fixes. The read except is broadened to (OSError, UnicodeError), so an undecodable file is skipped like any other per-file read failure.
2. Cancellation no longer abandons a live write. await asyncio.to_thread(file_path.write_text, …) starts a write in a worker thread that a cancellation cannot interrupt. Cancelling mid-write previously returned CancelledError while the write was still running, racing a caller's cleanup/retry. The write is now wrapped in a task, asyncio.shield-ed, and drained on cancellation before CancelledError propagates.

Scope

• Included: src/youtube_extension/backend/ai_code_generator.py (fix_build_errors read except + write path; new import contextlib); tests/unit/test_ai_code_generator.py (two regression tests).
• Explicitly excluded: CodeRabbit's third note — the per-invocation asyncio.Semaphore does not bound process-wide provider traffic, since each AICodeGenerator instance gets its own 4-permit semaphore. That is an architectural change to shared router/provider infrastructure and is out of scope for this perf PR. It does not affect perf: fix build errors concurrently with bounded fan-out #1336's actual guarantee (exactly one generate call per readable error file — billed LLM spend unchanged); only the "rate-limit protection" framing is broader than the per-call bound delivers. Left for a follow-up that owns shared-infra rate limiting.

Risk

• Risk level: low
• Failure mode: none identified. Both changes are strictly more defensive: fix (1) converts a crash-and-cancel into the existing skip-and-continue path; fix (2) adds a drain that only changes behaviour under cancellation, and re-raises CancelledError unchanged. The public return shape (success, fixed_files, total_errors) and the one-call-per-file contract are untouched.
• Rollback: revert this commit; fix_build_errors returns to prior behaviour. No schema or data migration.

Verification

Against head d4a510b:

# Both new tests prove-failed against the pre-change source (git stash of the fix):
FAILED test_undecodable_file_does_not_abort_siblings
FAILED test_cancellation_drains_inflight_write
# 2 failed  ← confirms the tests catch the bugs

# With the fix restored:
tests/unit/test_ai_code_generator.py tests/unit/test_deployment_manager.py
363 passed

• test_undecodable_file_does_not_abort_siblings — one file's read_text raises UnicodeDecodeError; asserts the other two files are still fixed and no exception escapes.
• test_cancellation_drains_inflight_write — blocks a write in a worker thread, cancels the task, asserts the task stays pending until the write drains (buggy code finishes immediately), then that CancelledError propagates and the drained write landed.
• ruff / black / mypy: added lines clean; pre-existing whole-file F841/format/[index] findings (lines 668–711, 2057) are not touched, matching perf: fix build errors concurrently with bounded fan-out #1336's tight-diff posture.
• Focused tests (2 new, both prove-failed)
• Required CI (pending on this PR's head)
• Review threads resolved — addresses the two blocking findings; the semaphore note is answered in Scope as a deliberate deferral

Production evidence

Not applicable — backend change to an internal code-repair path with no HTTP surface. DeploymentManager.verify_and_fix_project is the caller; its suite (test_deployment_manager.py) is green in the run above.

Agent handoff

• One canonical issue is linked (via perf: fix build errors concurrently with bounded fan-out #1336 → perf: fix_build_errors repairs files serially and blocks the event loop on file I/O #1335)
• No competing PR implements the same issue — stacked onto perf: fix build errors concurrently with bounded fan-out #1336's branch, not a parallel main PR
• Acceptance criteria are satisfied
• Required checks pass on the current head — CI pending
• Human decision is requested only for product, security, irreversible infrastructure, or production approval — merge of this and perf: fix build errors concurrently with bounded fan-out #1336 into main is left to a human

---

Generated by Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Aug 4, 2026 4:01am

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd563bc2-b47e-4426-8196-8ad8a0bc2ef9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Agent Completion Truth Gate: BLOCKED

Reasons: invalid_payload

Machine-readable verdict

{
  "details": {
    "collection_errors": [
      "incomplete_linked_issue_contract",
      "missing_linked_issue",
      "missing_closing_issue_reference",
      "missing_agent_run_id",
      "missing_agent_login"
    ],
    "invalid_fields": [
      "issue.number",
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

[groupthinking]

CI status: the only red check is agent-completion/truth-gate (invalid_payload, invalid_fields: ["policy.agent_login", "policy.run_id"]). Everything else is green/neutral/skipped — validate ✅, Canonical issue and evidence ✅, Agent completion enforcement ✅, Vercel preview ✅ (Ready).

Why it's red — and why I'm not forcing it green. The gate flags this as an agent task because the branch is claude/* (the agentTaskApplicable() over-trigger tracked in #1286 and made legible by the green, merge-ready #1285). The workflow assembled a full evidence payload — the only missing fields are policy.agent_login/policy.run_id, which come from an agent-lock manifest. Supplying that manifest wouldn't clear the gate anyway: it would then block on missing_agent_result, missing_copilot_rabbit_label, and missing_copilot_current_head_review — i.e. the full agent-completion protocol (canonical-issue acceptance criteria + declared-file scope, copilot-rabbit label, a Copilot review of the head, and published artifact_ready/completed agent-lock events with matching run-id/head-sha).

This PR is a review-fix stacked onto #1336, not a canonical-issue agent task, so I'm deliberately not synthesizing completion evidence to satisfy an evidence gate — that would game the exact thing the gate verifies. My branch name is fixed by the harness, so I also can't route around it the way fix/*/perf/* PRs do (which pass as not_applicable).

The code itself is verified and green on the checks that test it: both reviewer-confirmed bugs are fixed, the two regression tests prove-failed against the pre-change source, and test_ai_code_generator.py + test_deployment_manager.py = 363 passed locally with ruff/black/mypy clean on the added lines.

Recommendation (human governance call): merge #1285 (it improves this gate's diagnostics and is itself green/merge-ready), and either exempt harness-constrained agent follow-up branches from agentTaskApplicable() or have a maintainer apply the agent-lock protocol here. I'm leaving the truth-gate as a HALTED(governance) blocker rather than manufacturing evidence for it, and I'm not merging anything to main.

---

Generated by Claude Code

[groupthinking]

Closing as superseded — the two fixes here landed directly on perf/ai-fix-parallel before I saw this PR, so its diff is now empty against that branch.

Genuinely useful convergence though, and one thing here was better than what I wrote, which I've adopted:

• UnicodeError isolation — same fix, landed in b57b22b2c.
• Shield + drain the in-flight write — same fix, landed in 370bcec53. Identical shape (shield → except CancelledError → contextlib.suppress(Exception) drain → bare raise); only create_task vs ensure_future differs.
• The may_finish handshake in your cancellation test — adopted in af277dd53. My first version blocked the worker thread with a fixed 0.3 s sleep. That is a latent vacuous-pass: on a loaded runner the sleep can elapse before task.cancel() is delivered, the write completes on its own, and the test goes green without ever opening the race window it exists to prove. A test that can silently stop testing is worse than one that fails. Your blocking Event makes the ordering structural rather than timing-dependent. I kept the Event and release it from a threading.Timer started after cancel(), so the worker is provably still parked when the un-drained path unwinds. Verified 3/3 pass with the drain, 3/3 fail without.

One divergence worth recording: this PR explicitly scoped out CodeRabbit's third finding (the per-invocation semaphore not bounding process-wide provider traffic) as shared-infra work. I did fix it, in b57b22b2c — a module-level semaphore keyed by running loop via a WeakKeyDictionary. The loop-keying is load-bearing: from Python 3.10 asyncio.Semaphore inherits _LoopBoundMixin and binds to the first loop that awaits it, so a plain module-level singleton raises across event loops and breaks under pytest-asyncio's per-test loops. Covered by test_concurrent_invocations_share_the_fix_budget.

Net effect: perf/ai-fix-parallel now carries all three review findings. Closing this also unblocks #1336 — its governance gate was failing with "Issue #1335 already has another open implementation PR: #1340", because the gate's parser matched the closes #1335 inside this body's **#1336** (which closes #1335) parenthetical and read it as a second implementation claim. Worth knowing that referencing another PR's closing keyword in prose is enough to trip that check.

Thanks for the review pass — the handshake was a real catch.