fix: stop daily OOM crash loop and hours-long post-Phase-1 refilter - #12
Conversation
The service has been OOM-killed daily since Jul 16: every cycle Phase 2 picked the same four multi-hour videos first, four concurrent model.transcribe() calls grew past 31 GB RSS (no swap), and the kernel killed the process before any translations completed. Between Phase 1 and Phase 2 the job list was also re-filtered by re-statting all 143k jobs sequentially on NFS, adding 6-12 h of dead time per cycle. - Re-filter only the jobs Phase 1 touched (a translation becomes newly needed only when a fresh ru.vtt appears), in parallel, deduped against the already-queued translation jobs - Sort both phase queues smallest video first: concurrent workers hold similarly-sized jobs (bounded combined peak RSS) and steady progress happens before the multi-hour videos - Serialize long videos: anything over --long-video-minutes (default 45) takes an exclusive semaphore so at most one long transcribe runs at a time, in both phases - Record Phase 1 failures in the manifest with video_mtime; audio extraction failures (corrupt containers) are permanent and skipped on later cycles until the video file changes - the 93 known-broken videos stop retrying every cycle - Decode ffmpeg/ffprobe output with errors=replace: corrupt containers emit non-UTF-8 bytes on stderr and the UnicodeDecodeError was masking the real extraction error - Probe video metadata inside the Phase 2 try block so a corrupt video yields an error record instead of an exception escaping the worker
|
Warning Review limit reached
Next review available in: 50 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe archive transcriber now records permanent FFmpeg extraction failures, limits concurrent long-video processing, orders jobs by file size, and performs targeted translation rechecks in two-phase runs. ChangesArchive transcription
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveTranscriber
participant Manifest
participant TranscriptionJobs
participant TranslationJobs
ArchiveTranscriber->>Manifest: read failure and mtime records
ArchiveTranscriber->>TranscriptionJobs: filter and sort transcription jobs
ArchiveTranscriber->>TranslationJobs: recheck relevant jobs
ArchiveTranscriber->>TranslationJobs: sort translation jobs
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses stability and throughput issues in the archive transcriber’s --two-phase workflow by reducing unnecessary post-Phase-1 refilter work, prioritizing smaller videos to avoid OOM-prone queue behavior, adding long-video concurrency gating, and improving handling/recording of permanent failures from corrupt inputs.
Changes:
- Add long-video exclusivity gating (
--long-video-minutes) and sort both phase queues smallest-first to reduce OOM risk from concurrent multi-hour transcribes. - Record and skip permanently broken inputs (audio extraction failures) across cycles using manifest metadata (
error_type,video_mtime), and decode ffmpeg/ffprobe output witherrors="replace". - Avoid hours-long post-Phase-1 refilter by only re-checking translation needs for jobs touched by Phase 1, in parallel.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/python/tools/archive_transcriber.py |
Adds long-video gating, permanent-failure tracking, errors="replace" for ffmpeg/ffprobe decoding, queue sorting, and targeted post-Phase-1 translation recheck. |
tests/test_archive_transcriber.py |
Adds tests for permanent-failure skip semantics, size-based ordering, long-video gate behavior, and transcription error record classification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/python/tools/archive_transcriber.py (1)
1287-1297: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a timeout to the FFmpeg extraction subprocess.
extract_audionow runs underLONG_VIDEO_GATEinprocess_transcription_onlyandprocess_translation_only. The currentsubprocess.runhas notimeout; if FFmpeg hangs, the worker still entersexcept Exception, but the exclusive gate remains held until the process is killed, blocking all subsequent long-video jobs. Add a finitetimeouthere and keep the existingexcept Exceptionblock so this is emitted as a retryable"processing"error rather than a permanentaudio_extractionfailure.🤖 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/python/tools/archive_transcriber.py` around lines 1287 - 1297, Add a finite timeout to the FFmpeg subprocess invocation in extract_audio while preserving the existing RuntimeError handling and surrounding except Exception flow. Ensure subprocess timeout failures propagate through the existing retryable “processing” error path rather than being converted into a permanent audio_extraction failure.
🧹 Nitpick comments (3)
src/python/tools/archive_transcriber.py (3)
1642-1652: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider avoiding the extra ffprobe call for videos that are clearly short.
This adds an unconditional
probe_video_metadatacall (spawnsffprobe) to every Phase 1 job wheneverlong_video_minutes > 0, which is the default. On a 143k-video NFS archive, this reintroduces one extra subprocess round trip per video, the same category of overhead this PR's targeted recheck (Line 2333-2358) works to eliminate elsewhere.
sort_jobs_by_sizealready stats file size cheaply for every job. Consider using a size-based lower bound (a size clearly too small to reach the duration threshold) to skip theffprobecall for the bulk of short videos, only probing borderline-sized files.🤖 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/python/tools/archive_transcriber.py` around lines 1642 - 1652, Update the long-video slot logic near acquire_long_video_slot to use each job’s already-available file size from sort_jobs_by_size and establish a conservative size lower bound for long_video_minutes; skip probe_video_metadata for files clearly below that bound, while probing only borderline or larger files and preserving the existing fallback behavior for probe failures.
2290-2293: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPermanent-failure tracking is asymmetric between phases.
Only
process_transcription_only's error records carryerror_type/video_mtime(Line 1707-1717), and only those are persisted here forknown_permanent_failureto consume.process_translation_only's error record (Line 1858-1872) has noerror_typeorvideo_mtime, even though its ownextract_audiocall (Line 1758) can fail with the exact same unfixable corrupt-container error. A video needing only translation whose extraction permanently fails will be retried every Phase 2 cycle indefinitely, unlike the Phase 1 case this PR fixes.Consider extending the same classification and persistence to
process_translation_only's error path for consistency.🤖 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/python/tools/archive_transcriber.py` around lines 2290 - 2293, Extend process_translation_only’s error-record path to classify extract_audio failures using the same error_type and video_mtime fields produced by process_transcription_only, including permanent corrupt-container failures. Ensure the resulting status="error" record is persisted by the manifest.append logic so known_permanent_failure() excludes these inputs on later Phase 2 cycles.
1699-1721: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string-based error classification for permanent failures.
is_extract_failureclassifies the exception by checkingisinstance(exc, RuntimeError) and str(exc).startswith("FFmpeg failed"). This string prefix is only defined by convention inextract_audio's ownraise RuntimeError(f"FFmpeg failed for {video_path}: ...")(Line 1293-1295). A future change to that message text silently breaksknown_permanent_failuredetection, since nothing enforces the coupling between the two locations.test_ffmpeg_failure_marked_permanentintests/test_archive_transcriber.py(Line 756-757) reinforces this risk: it hardcodes the same string independently in itsboomstub rather than deriving it fromextract_audio, so a drift in the real message would not be caught by the test.Define a dedicated exception type (for example
AudioExtractionError) thatextract_audioraises on failure, and checkisinstance(exc, AudioExtractionError)instead of matching on the message text.♻️ Proposed refactor using a dedicated exception type
+class AudioExtractionError(RuntimeError): + """Raised when FFmpeg fails to extract audio from a video.""" + + def extract_audio(video_path: Path, sample_rate: int) -> Path: ... if result.returncode != 0: stderr_preview = (result.stderr or "").splitlines()[-5:] - raise RuntimeError( + raise AudioExtractionError( f"FFmpeg failed for {video_path}: return code {result.returncode}\n" + "\n".join(stderr_preview) )- is_extract_failure = isinstance(exc, RuntimeError) and str(exc).startswith("FFmpeg failed") + is_extract_failure = isinstance(exc, AudioExtractionError)🤖 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/python/tools/archive_transcriber.py` around lines 1699 - 1721, Define a dedicated AudioExtractionError near the audio-extraction code, update extract_audio to raise it for FFmpeg failures while preserving the existing failure message, and change is_extract_failure in the error-handling flow to use isinstance(exc, AudioExtractionError) instead of RuntimeError string-prefix matching. Update test_ffmpeg_failure_marked_permanent and its boom stub to raise the dedicated exception so permanent-failure classification remains covered.
🤖 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.
Outside diff comments:
In `@src/python/tools/archive_transcriber.py`:
- Around line 1287-1297: Add a finite timeout to the FFmpeg subprocess
invocation in extract_audio while preserving the existing RuntimeError handling
and surrounding except Exception flow. Ensure subprocess timeout failures
propagate through the existing retryable “processing” error path rather than
being converted into a permanent audio_extraction failure.
---
Nitpick comments:
In `@src/python/tools/archive_transcriber.py`:
- Around line 1642-1652: Update the long-video slot logic near
acquire_long_video_slot to use each job’s already-available file size from
sort_jobs_by_size and establish a conservative size lower bound for
long_video_minutes; skip probe_video_metadata for files clearly below that
bound, while probing only borderline or larger files and preserving the existing
fallback behavior for probe failures.
- Around line 2290-2293: Extend process_translation_only’s error-record path to
classify extract_audio failures using the same error_type and video_mtime fields
produced by process_transcription_only, including permanent corrupt-container
failures. Ensure the resulting status="error" record is persisted by the
manifest.append logic so known_permanent_failure() excludes these inputs on
later Phase 2 cycles.
- Around line 1699-1721: Define a dedicated AudioExtractionError near the
audio-extraction code, update extract_audio to raise it for FFmpeg failures
while preserving the existing failure message, and change is_extract_failure in
the error-handling flow to use isinstance(exc, AudioExtractionError) instead of
RuntimeError string-prefix matching. Update test_ffmpeg_failure_marked_permanent
and its boom stub to raise the dedicated exception so permanent-failure
classification remains covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01068980-4c0c-4917-9d1a-a018f1a2f4dc
📒 Files selected for processing (2)
src/python/tools/archive_transcriber.pytests/test_archive_transcriber.py
…lter dedup - LONG_VIDEO_GATE is a BoundedSemaphore so an unbalanced release raises instead of silently allowing two concurrent long videos - Persist every non-success Phase 1 record, not just errors: invalid-SMIL skips now survive restarts as skip_record_for_invalid_smil documents - Add a run_two_phase integration test covering the post-Phase-1 re-filter: a video needing both phases must not be queued for translation twice (verified to fail without the dedup guard)
Problem
The archive-transcriber service has been in a daily OOM crash loop since ~Jul 16 and completed almost no translations in two weeks (journal shows kernel OOM kills on Jul 28, 29×2, 30×2, 31×2, Aug 1). Each cycle:
'utf-8' codec can't decode byte 0x8e...) was Python failing to decode FFmpeg's stderr, masking the real extraction error.run_two_phasere-filtered translations by re-statting all 143,384 jobs sequentially on NFS — 6–12 h of dead time per cycle.model.transcribe()grows with audio duration (~3 GB measured for a 27-minute video on faster-whisper 1.2.1); four concurrent long transcribes exceed the box's 31 GB (no swap) within minutes → SIGKILL → systemd restarts → repeat.Fix
ru.vttappears), in parallel with the existingSTARTUP_STAT_THREADSpool, deduped against the already-queued translation jobs. Hours → seconds.--long-video-minutes(default 45,0disables) takes an exclusive semaphore before transcribing, in both phases — at most one long video in memory at a time, worst case ≈ 3 small workers + 1 long one.error_typeandvideo_mtime; audio-extraction failures (corrupt containers) are excluded from later cycles until the video file's mtime changes.errors="replace"on ffmpeg/ffprobe output decoding so corrupt containers surface the real FFmpeg error instead of aUnicodeDecodeError.probe_video_metadatamoved inside the Phase 2 try block — a corrupt video now yields an error record instead of an exception escaping the worker thread and killing the whole run.Caveat
If the 6.5-hour video still exceeds 31 GB even running alone, it will fail at the very end of the queue after everything else has completed — not block the whole archive as it does today. Chunked transcription for such outliers can be a follow-up if it proves necessary.
Testing
pytest tests/), including new coverage:known_permanent_failure(skip/retry semantics incl. mtime change),sort_jobs_by_size,acquire_long_video_slot(threshold/disable/exclusivity), and error-record classification for FFmpeg vs transient failures.ruffandmypyclean.--two-phaseend-to-end on an empty archive;--long-video-minutesvisible in--help.