fix(transcript): preserve process_video error over cleanup cancellation (for #1245)

[groupthinking]

Canonical issue

No competing issue claimed. #1245 remains the canonical PR for #1244; this PR targets #1245's own branch (perf/transcript-cleanup-off-loop) so its diff is exactly the single follow-up fix commit, which #1245 can absorb by merging this. It does not open a second PR against main for #1244.

Outcome

Resolves the one confirmed HIGH review finding holding #1245: the worker-thread cleanup offload changed exception precedence under cancellation. This restores the documented "raised-exception behaviour does not change" contract so a recoverable per-source transcript failure is still caught by the caller instead of escaping as an uncaught cancellation.

Scope

• Included:
  • transcript_action_workflow.py — the finally in _fallback_transcript_with_gemini now captures any in-flight exception via sys.exc_info() before the cleanup await and suppresses a cleanup-time CancelledError only when an exception is already unwinding; adds import sys.
  • One regression test that drives the real fallback finally (process_video raises, task cancelled mid-cleanup) and asserts the original ValueError — not CancelledError — surfaces while the temp tree is still removed.
• Explicitly excluded:
  • No change to _cleanup_download_artifacts, the shield, or which paths are removed — cleanup still runs to completion and still re-raises CancelledError on the normal (no-exception) path.

Risk

• Risk level: low
• Failure mode: narrow — requires both a process_video raise and a cancellation delivered during cleanup. Fix is a targeted precedence guard at the call site; shield semantics are untouched.
• Rollback: revert this commit; perf: offload transcript download cleanup to a worker thread #1245 returns to its prior (flagged) state.

Verification

Tied to head ff19580.

• Focused tests — pytest tests/unit/test_transcript_action_workflow.py → 114 passed
• Control experiment — reverting the guard to the pre-fix bare await makes the new test fail with asyncio.CancelledError (proving it catches the exact bug); restored
• ruff check on both changed files → All checks passed
• Required CI — will run on push
• Review threads resolved — defer to perf: offload transcript download cleanup to a worker thread #1245's threads

Production evidence

No runtime surface change; the fix only alters which exception propagates on the cancellation-during-cleanup edge. Behaviour is pinned by the new async regression test rather than a deployment.

Agent handoff

• One canonical issue is linked (perf: transcript Gemini fallback cleans up downloaded video on the event loop #1244 via perf: offload transcript download cleanup to a worker thread #1245; this PR claims none)
• No competing PR implements the same issue — targets perf: offload transcript download cleanup to a worker thread #1245's branch, not main
• Acceptance criteria are satisfied (finding resolved + regression test)
• Required checks pass on the current head — pending CI
• Human decision requested only for merge approval

Agent provenance

Human-authored sections may be trimmed by the owner. This is an agent-authored follow-up; the authoritative scope and canonical issue live in #1245 / #1244.

---

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 2, 2026 4:50pm

[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: e0114de9-b33b-40d1-9b8f-f8e36800251f

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": {
    "invalid_fields": [
      "issue.number",
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

[groupthinking]

CI status + the one blocker (not auto-fixable by me).

The code-level and governance sub-checks are green on ff19580:

• validate ✅ · Canonical issue and evidence ✅ · Agent completion enforcement ✅ · Vercel preview building
• Fix itself verified: tests/unit/test_transcript_action_workflow.py → 114 passed; the added regression test fails against the pre-fix bare-await form (CancelledError) and passes with the fix; ruff clean.

Blocker: agent-completion/truth-gate → invalid_payload, on issue.number, policy.agent_login, policy.run_id. The gate wants a filled agent-lock provenance manifest. I'm deliberately not populating it, for two reasons:

1. I have no legitimate provider run_id — this is a Claude Code session, not a GitHub Actions run — and inventing one to pass a provenance gate would be gaming it.
2. Setting issue.number: 1244 would turn this into a competing agent-PR for perf: transcript Gemini fallback cleans up downloaded video on the event loop #1244, which is exactly what targeting perf: offload transcript download cleanup to a worker thread #1245's branch (instead of main) was meant to avoid — and what the PR template's "No competing PR implements the same issue" rule forbids.

So the merge decision is yours, and there are two clean paths — no provenance gymnastics needed either way:

• Cherry-pick ff19580 from claude/determined-maxwell-l04xha onto perf/transcript-cleanup-off-loop — the fix lands directly on perf: offload transcript download cleanup to a worker thread #1245 under its existing provenance, and this wrapper PR can be closed.
• Or merge this PR into perf/transcript-cleanup-off-loop if you're fine with the truth-gate state on a non-main base.

I'll keep watching #1247; I won't fabricate the manifest to force it green.

---

Generated by Claude Code

[groupthinking]

Closing this — the finding it implements was withdrawn by its own author after re-review, and the change it makes is measurably harmful.

1. The originating finding was retracted

CodeRabbit raised the exception-precedence concern on #1245, then re-reviewed 4ab5a7ddc and withdrew it verbatim:

I also agree with the cancellation decision. asyncio.shield keeps cleanup running but correctly raises CancelledError in the caller. Suppressing that cancellation would break asyncio.timeout, make task cancellation state inaccurate, and allow _extract_transcript to treat cancellation as a recoverable source failure.

The new await adds a cancellation-delivery point that the old synchronous cleanup did not have. This is an intentional and necessary consequence of moving cleanup off the event loop. It does not justify suppressing cancellation.

No remaining production correctness issue found in this update.

That comment carries a review_comment_withdrawn marker. This PR implements the retracted prescription.

2. The masking defect is already fixed on the base branch — differently and more completely

4ab5a7ddc removed the two Path.exists() probes from _cleanup_download_artifacts. Path.exists() calls stat(), and CPython only whitelists ENOENT/ENOTDIR/EBADF/ELOOP — a generic OSError (EIO, EACCES) re-raises out of the finally and replaces the in-flight exception. Removing them makes the helper total by construction: unlink() raises FileNotFoundError (already caught) for absent paths and rmtree(..., ignore_errors=True) is a no-op.

That fix is covered by test_cleanup_does_not_mask_in_flight_exception, prove-failed against the previous body:

E       OSError: stat exploded
1 failed, 5 passed

After that change the only thing that can emerge from the cleanup await is CancelledError — and that is the one thing that must not be suppressed.

3. Suppressing CancelledError causes measured downstream breakage

scenario	base branch (4ab5a7ddc)	this PR
asyncio.timeout(0.1) around the call	TimeoutError ✅	inner exception — the timeout is silently swallowed ❌
TaskGroup sibling failure	ExceptionGroup['ValueError'] ✅	ExceptionGroup['Boom','ValueError'] — spurious extra ❌
task.cancelled() after a real cancel	True ✅	False ❌

The production consequence is at transcript_action_workflow.py:340:

except Exception as exc:  # noqa: BLE001 - resilient multi-source fallback
    # A raising source must not abort the whole pipeline; record it and
    # continue to the next source so we can still degrade gracefully.

CancelledError is a BaseException, so today it escapes that handler and correctly aborts the request. Downgrade it to a caught Exception and the workflow keeps issuing network calls to further transcript sources after the client has already abandoned the request. That is the exact behaviour CodeRabbit named in its retraction.

4. The sys.exc_info() guard is additionally unsound

sys.exc_info() returns the innermost exception being handled anywhere up the stack, not just in this frame. If any caller ever invokes this from inside an except block, pending_exc is non-None even when nothing is in flight locally, and a genuine cancellation is swallowed:

=== caller NOT inside except (baseline) ===
      finally: sys.exc_info()[1] = None
  RESULT: CancelledError propagated (correct)
  task.cancelled() = True

=== caller INSIDE except block ===
      finally: sys.exc_info()[1] = RuntimeError('outer source failure')
      finally: CancelledError SUPPRESSED
  RESULT: returned 'ok'  <-- cancellation LOST
  task.cancelled() = False

In fairness: I checked, and this is not reachable today — neither _extract_transcript (L132) nor _fallback_transcript_with_gemini (L335) is called from inside a handler, and the finally at L852 has no enclosing except. So this is a latent trap rather than a live bug. But it makes correctness depend on a caller-side invariant that nothing enforces, in a file whose whole design point is a resilient multi-source fallback built out of except blocks.

5. On the test

test_fallback_finally_keeps_process_video_error_over_cancellation constructs its scenario with gemini_service.process_video = AsyncMock(side_effect=original_error). GeminiService.process_video (gemini_service.py:729-840) contains zero raise statements and catches Exception at L795 and L831, returning GeminiResult(success=False, error=...). It cannot raise in production, so the test pins behaviour that only the mock exhibits.

The genuinely reachable propagation path is _parse_gemini_transcript_payload at L840 — inside the guarded try body (L821-850), and it only catches json.JSONDecodeError. Even for that path, a CancelledError replacing a parse failure is correct: the request is gone, and the parse result is moot.

---

Net: the base branch already fixes the real masking defect and is strictly safer than the pre-change inline cleanup. This PR trades that for a cancellation-suppression bug that CodeRabbit, Linear, and the measurements above all agree is a regression.

Happy to reopen if someone can produce a case where the base branch masks an exception that this change would preserve — that would invert the argument and I'd want to know before #1245 merges.

[groupthinking]

You're right on every point, and I verified rather than take it on faith. Conceding:

• My premise was stale. I worked from head f2329a1; the base branch is now 4ab5a7ddc ("make temp-tree cleanup total to avoid masking exceptions"), which I hadn't seen. I confirmed it in the tree: the exists() probes are gone, both _cleanup branches are total by construction (unlink swallows OSError, rmtree(ignore_errors=True)), and the docstring already spells out why CancelledError must propagate.
• That makes my change actively harmful, not redundant. Once cleanup is total, CancelledError is the only thing that can emerge from the await — and suppressing it is exactly the regression you measured: swallowed asyncio.timeout, task.cancelled() reporting False, and the L340 except Exception downgrading a real cancellation into a recoverable per-source error so the pipeline keeps calling transcript sources after the client is gone.
• My sys.exc_info() guard is unsound as you describe — it reads the innermost exception being handled anywhere up the stack, so it's one except-nested caller away from swallowing a genuine cancellation. Latent today, but it makes correctness depend on an unenforced caller invariant in a file built out of except blocks.
• My test pinned mock-only behaviour. process_video can't raise in production (it catches Exception and returns a GeminiResult), so AsyncMock(side_effect=...) tested a path that doesn't exist; and for the genuinely reachable parse path, a cancellation replacing a parse failure is correct anyway.

I can't produce the inverting counterexample you asked for — the base branch masks nothing my change would have preserved — so there's nothing to reopen. This also supersedes my earlier "resolves the hold-merge finding" note on #1245: the finding was retracted and already fixed on the base, better. Thanks for the thorough write-up; 4ab5a7ddc is the correct fix.

---

Generated by Claude Code