perf(pipeline): harden transcript action workflow cleanup - #1248
perf(pipeline): harden transcript action workflow cleanup#1248groupthinking wants to merge 4 commits into
Conversation
The `finally` block in `_fallback_transcript_with_gemini` deleted the downloaded video and recursively removed its temp tree directly on the event loop. `Path.exists`, `Path.unlink` and `shutil.rmtree` are all blocking syscalls, and the temp tree can hold a merged mp4 plus unmerged `.fNNN` fragments, so every request that reaches the Gemini video fallback stalls the loop for the duration of the delete. Move the cleanup into `_cleanup_download_artifacts`, a static helper that runs the same logic under `asyncio.to_thread`. The call is wrapped in `asyncio.shield` because the original inline code was uncancellable: a bare `await` in a `finally` can be interrupted by a second cancellation, which would turn a loop stall into a disk leak. Filesystem semantics are preserved verbatim, including the `exists()` guards, `except OSError` and `ignore_errors=True`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The event-loop liveness test polled a fixed 500 iterations while rmtree was parked on an unset Event. That is a ~5s budget expressed as an iteration count, so a loaded CI box that is slow to hand asyncio.to_thread a worker would fail the test for scheduling reasons rather than for the behaviour under test. Poll against time.monotonic() with a 30s deadline instead. The loop still exits as soon as the worker starts (milliseconds in practice), so the test is not slower; it simply stops being brittle under load. Also assert the tick count directly. Each completed tick is one turn of the event loop taken while the deletion was in flight, which is precisely the property being proven, and it was previously only implied by reaching the assertion at all. The polling is deliberately retained rather than replaced with a blocking wait on the Event: blocking the loop to wait for proof that the loop is not blocked would invert the test. Re-verified against pre-change semantics -- reducing the helper to a direct _cleanup() call still yields 3 failed, 2 passed, so this test is no weaker than before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cleanup helper runs from a `finally` block. Its `Path.exists()` probes performed a stat that can itself raise `OSError`, which would propagate out of the `finally` and replace the exception already in flight. Remove both probes. `unlink()` raises `FileNotFoundError` (an `OSError`, already caught) for absent paths and `rmtree(ignore_errors=True)` is a no-op, so the guards were redundant as well as unsafe. Also harden the cancellation test's poll loop to a wall-clock deadline instead of a fixed iteration budget, matching the sibling test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cleanup helper runs from a `finally`, so anything it raises replaces the exception already propagating. Its guards caught `OSError` only, but neither call is OSError-total: a NUL byte in a path makes `Path.unlink` raise `ValueError: embedded null character`, and makes `shutil.rmtree` raise the same from its internal `lstat` despite `ignore_errors=True` -- that flag suppresses `OSError` alone. Both branches now catch `Exception` and log at debug with `exc_info`. `CancelledError` is a `BaseException`, so cancellation still propagates. Not reachable in production today: `temp_root` comes from `mkdtemp` and a NUL `video_path` is already rejected by the `exists()` guard in `_download`. This corrects a false totality claim in the contract. Verified: reverting to the `OSError`-only guards fails the new test with the escaping `ValueError` (1 failed, 6 passed); restored 115 passed. 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: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
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 |
Agent Completion Truth Gate: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"issue.number",
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
CI status + blocker (unattended remediation run)Green / clean: Skipped (not a failure): Single blocker — I'm deliberately not clearing this autonomously. This PR rescues 4 pre-existing, tested-but-never-pushed commits on the designated branch (transcript-workflow cleanup hardening) so they aren't lost on container reclaim — it has no canonical tracking issue, and the provenance truth-gate requires one. Filling Human decision needed (one of):
Note: I couldn't run the focused tests locally (the ephemeral sandbox lacks the full runtime dep tree and the editable install is blocked by a Debian Generated by Claude Code |
|
✅ Action performedReview finished.
|
|
Closing as superseded by #1245, which is now merged into I opened this to rescue 4 tested-but-unpushed commits on This PR therefore adds nothing over Generated by Claude Code |
1 participant
Canonical issue
Closes # (none — surfacing pre-existing, unpushed branch work; link a canonical issue before promotion)
Outcome
Hardens cleanup in the transcript action workflow so failures during download/temp-tree teardown neither mask the original exception nor block the event loop:
OSErrorfailures too.Scope
src/.../services/workflows/transcript_action_workflow.py,tests/unit/test_transcript_action_workflow.pyRisk
Verification
List exact automated and manual checks, tied to the current head SHA (
627ebe2).httpx,aiohttp,sqlalchemy,pydantic, …) and the editable install is blocked by a Debianpackaginguninstall conflict. Each of the 4 commits ships accompanying unit tests; CI must validate.Production evidence
Not applicable — Python-only backend change; not exercised by the Vercel
apps/webpreview.Agent handoff
mainGenerated by Claude Code