From ff1958075fded4d3c202fabedfd313ea91ba2b79 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 16:48:32 +0000 Subject: [PATCH] fix(transcript): preserve process_video error over cleanup cancellation (#1245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worker-thread cleanup offload in `_fallback_transcript_with_gemini` introduced an `await` inside the `finally` block where the previous inline (synchronous) cleanup had none. That await is a cancellation point: if `process_video` raises and the task is cancelled while the shielded cleanup is still running, `CancelledError` — a `BaseException` — replaces the in-flight error per `finally` semantics. The caller's `except Exception` handler (transcript_action_workflow.py:336) then no longer catches it, so a recoverable per-source failure escapes as an uncaught cancellation, breaking the documented "raised-exception behaviour does not change" contract. Capture any in-flight exception with `sys.exc_info()` before the await and suppress a cleanup-time `CancelledError` only when an exception is already unwinding, keeping the original error primary. On the normal path a genuine cancellation still propagates, and cleanup still completes (shield is unchanged). Adds a regression test that drives the real fallback finally: process_video raises, the task is cancelled mid-cleanup, and the original ValueError — not CancelledError — must surface while the temp tree is still removed. Verified to fail against the pre-fix bare-await form (raises CancelledError) and pass with the fix. Full file: 114 passed; ruff clean. Resolves the exception-precedence finding raised by CodeRabbit and independently confirmed on PR #1245. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EUEMeY1ZBRmF5jXrRYPre2 --- .../workflows/transcript_action_workflow.py | 19 ++++- tests/unit/test_transcript_action_workflow.py | 77 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/youtube_extension/services/workflows/transcript_action_workflow.py b/src/youtube_extension/services/workflows/transcript_action_workflow.py index c072d88d8..cbd6aa227 100644 --- a/src/youtube_extension/services/workflows/transcript_action_workflow.py +++ b/src/youtube_extension/services/workflows/transcript_action_workflow.py @@ -7,6 +7,7 @@ import json import logging import shutil +import sys import tempfile from dataclasses import asdict from pathlib import Path @@ -849,7 +850,23 @@ async def _fallback_transcript_with_gemini( if file_result.error: errors.append(file_result.error) finally: - await self._cleanup_download_artifacts(video_path, temp_root) + # Preserve exception precedence across the cleanup await. Cleanup + # is shielded and still runs to completion, but the await it adds + # is a cancellation point the previous inline (synchronous) + # cleanup did not have. Per ``finally`` semantics a + # ``CancelledError`` raised here would replace an exception + # already unwinding from the ``try`` body, turning a + # ``process_video`` error that the caller's ``except Exception`` + # handles into an uncaught ``BaseException``. Capture any + # in-flight exception before the await and, if one exists, keep + # it primary; only let a cancellation propagate when it is not + # masking one. + pending_exc = sys.exc_info()[1] + try: + await self._cleanup_download_artifacts(video_path, temp_root) + except asyncio.CancelledError: + if pending_exc is None: + raise error_message = errors[0] if errors else "Gemini transcription failed" logger.warning("Gemini transcription fallback failed: %s", error_message) diff --git a/tests/unit/test_transcript_action_workflow.py b/tests/unit/test_transcript_action_workflow.py index 506597b2e..4a4933234 100644 --- a/tests/unit/test_transcript_action_workflow.py +++ b/tests/unit/test_transcript_action_workflow.py @@ -1344,3 +1344,80 @@ def slow_rmtree(path, *args, **kwargs): # Let the shielded inner task settle before the loop closes. await asyncio.sleep(0.1) + + async def test_fallback_finally_keeps_process_video_error_over_cancellation( + self, monkeypatch, tmp_path + ): + """A cancellation during cleanup must not mask a ``process_video`` error. + + The cleanup ``await`` in ``_fallback_transcript_with_gemini``'s ``finally`` + is a cancellation point the previous synchronous cleanup did not have. If + ``process_video`` raises and the task is cancelled while cleanup is still + running, ``finally`` semantics would let ``CancelledError`` replace the + original error — turning an exception the caller's ``except Exception`` + handles into an uncaught ``BaseException``. This pins the original + exception as primary while cleanup still completes. + """ + gemini_service = MagicMock() + gemini_service.is_available.return_value = True + gemini_service.select_model = MagicMock() + # process_youtube fails cleanly so control falls through to the download + # path; error is None so no transient retry fires. + gemini_service.process_youtube = AsyncMock( + return_value=SimpleNamespace( + success=False, response=None, error=None, latency=0.0 + ) + ) + original_error = ValueError("boom from process_video") + gemini_service.process_video = AsyncMock(side_effect=original_error) + + hybrid = MagicMock() + hybrid.gemini = gemini_service + wf = _make_workflow(hybrid_processor=hybrid) + + temp_root = tmp_path / "gemini_video_abc" + temp_root.mkdir() + video_file = temp_root / "auJzb1D-fag.mp4" + video_file.write_bytes(b"video-bytes") + wf._download_video_file = AsyncMock(return_value=(video_file, temp_root)) + + started = threading.Event() + release = threading.Event() + finished = threading.Event() + real_rmtree = shutil.rmtree + + def blocking_rmtree(path, *args, **kwargs): + started.set() + assert release.wait(timeout=10), "cleanup was never released" + real_rmtree(path, *args, **kwargs) + finished.set() + + monkeypatch.setattr(shutil, "rmtree", blocking_rmtree) + + task = asyncio.create_task( + wf._fallback_transcript_with_gemini( + "https://www.youtube.com/watch?v=auJzb1D-fag", + language="en", + video_metadata=None, + ) + ) + + deadline = time.monotonic() + 30.0 + while not started.is_set(): + assert time.monotonic() < deadline, "cleanup never started" + await asyncio.sleep(0.01) + + # Cancel while cleanup is parked, then let cleanup finish. + task.cancel() + release.set() + + # The original process_video error wins, not CancelledError. + with pytest.raises(ValueError) as excinfo: + await task + assert excinfo.value is original_error + + # Cleanup still ran to completion despite the cancellation. + assert finished.wait(timeout=10), "shielded cleanup did not finish" + assert not temp_root.exists() + + await asyncio.sleep(0.1)