fix: make write_smil strictly additive, add broken-SMIL scanner - #9
Conversation
Incident 2026-07-13: when a video's SMIL manifest was missing or failed XML parsing, write_smil created/regenerated it from scratch with a single <video> node (the one variant we transcribed) and bare codec params. Wowza then served a one-stream playlist with malformed CODECS="avc1,mp4a", breaking adaptive playback on production videos. write_smil now only adds <textstream> entries to an existing, valid, transcoder-generated SMIL: - never creates a SMIL when none exists - never regenerates on parse error - never adds or modifies <video> nodes - refuses to proceed if the backup copy fails - writes atomically (tmp + rename) - returns bool so callers can tell the update was skipped scripts/find_broken_smils.py (stdlib-only, run on a host with storage access) scans the archive and classifies every video: - BROKEN_HAS_BACKUP: restore from oldest valid .bak (--restore-bak --apply) - BROKEN_TRANSCRIBER_CREATED / MISSING_SMIL / UNPARSEABLE: written to a regen list for the transcoder-side SMIL generator Tests rewritten to encode the additive-only contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7
|
Warning Review limit reached
Next review available in: 56 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a standard-library SMIL integrity scanner with reporting and optional backup restoration. It also changes subtitle association to update only existing valid manifests atomically, with processing prechecks and expanded tests for preservation, idempotency, backups, and subtitle handling. ChangesSMIL integrity workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessingPhase
participant SMILPrecheck
participant Manifest
participant WriteSMIL
participant SMILFile
ProcessingPhase->>SMILPrecheck: validate SMIL
SMILPrecheck-->>ProcessingPhase: valid or invalid reason
ProcessingPhase->>Manifest: record skipped job when invalid
ProcessingPhase->>WriteSMIL: associate subtitles when valid
WriteSMIL->>SMILFile: create backup and atomically update SMIL
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/find_broken_smils.py (1)
52-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure subprocess cleanup in
iter_pathsgenerator.
proc.wait()is only reached if the generator is fully consumed. If iteration stops early, thefindprocess is left running. Usetry/finallyto guarantee cleanup.♻️ Proposed fix
proc = subprocess.Popen(find_args, stdout=subprocess.PIPE, text=True) - assert proc.stdout is not None - for line in proc.stdout: - yield Path(line.rstrip("\n")) - proc.wait() + try: + assert proc.stdout is not None + for line in proc.stdout: + yield Path(line.rstrip("\n")) + finally: + proc.wait()🤖 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 `@scripts/find_broken_smils.py` around lines 52 - 56, Update the iter_paths generator to wrap iteration over proc.stdout in a try/finally block, ensuring proc is terminated or otherwise cleaned up and waited on when iteration stops early as well as when fully consumed. Preserve the existing yielded Path values and subprocess invocation.
🤖 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.
Inline comments:
In `@scripts/find_broken_smils.py`:
- Around line 84-89: Update the exception handling around ET.parse in classify
to catch OSError alongside ET.ParseError, preserving the existing UNPARSEABLE
verdict and error recording so unreadable or missing files do not abort the
scan.
- Around line 195-196: Update the restore loop in the args.apply branch to wrap
shutil.copy2(bak, rec["smil"]) in a targeted try/except for I/O or filesystem
errors, report the failure consistently, and continue processing subsequent
records so the regen-list rewrite still executes.
- Around line 102-104: Extract the duplicated codec-parameter fingerprint logic
into a shared helper in scripts/find_broken_smils.py, then update both classify
and oldest_valid_bak to call it instead of defining their own any(...) checks.
Preserve the existing detection for videoCodecId and audioCodecId parameters and
use the helper consistently in both callers.
In `@src/python/tools/archive_transcriber.py`:
- Around line 666-667: Update the SMIL subtitle update flow to track whether any
textstream was added; when all requested VTT or TTML files are missing, skip the
write and return False instead of reporting success. Preserve the existing
successful write path when a subtitle is added, and update
test_smil_missing_vtt_warning to assert the False result.
- Around line 787-790: Update the atomic SMIL write flow around job.smil and
tmp_path to preserve the original file metadata: seed the temporary file with
shutil.copy2(job.smil, tmp_path) before tree.write, then replace the original as
currently done. Add or update tests to verify the replaced SMIL retains the
original permissions.
---
Nitpick comments:
In `@scripts/find_broken_smils.py`:
- Around line 52-56: Update the iter_paths generator to wrap iteration over
proc.stdout in a try/finally block, ensuring proc is terminated or otherwise
cleaned up and waited on when iteration stops early as well as when fully
consumed. Preserve the existing yielded Path values and subprocess invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0fed3f9-605e-48eb-a5c8-16d6c3399e8f
📒 Files selected for processing (3)
scripts/find_broken_smils.pysrc/python/tools/archive_transcriber.pytests/test_smil_generation.py
Early versions of write_smil (before timestamped backups) wrote a static "<name>.smil.bak". The scanner now lists it as the oldest backup so --restore-bak recovers the pristine original for videos processed by those runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7
Pre-flight check requested by the transcoder team: if a video's SMIL is missing, unparseable, or has no <video> entries, skip the video entirely (no GPU time spent) and append a manifest record with status="skipped", error_type="invalid_smil" and the reason. This flags generator failures while the trail is fresh instead of papering over them, and the video is retried automatically once a valid SMIL appears. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/python/tools/archive_transcriber.py`:
- Around line 655-658: Update the exception handlers around ET.parse in
smil_precheck (src/python/tools/archive_transcriber.py#L655-L658) and write_smil
(src/python/tools/archive_transcriber.py#L736-L741) to catch both ET.ParseError
and OSError, preserving each function’s existing graceful error or skip
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea9e16e8-7fd2-4b61-85f6-5513479baa2b
📒 Files selected for processing (3)
scripts/find_broken_smils.pysrc/python/tools/archive_transcriber.pytests/test_smil_generation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/find_broken_smils.py
- catch OSError alongside ET.ParseError when parsing SMILs (precheck, write_smil, scanner classify) so an unreadable file can't crash a run - write_smil returns False and leaves the SMIL (and no backup) untouched when no textstream was actually added - preserve the original SMIL's permission bits across the atomic replace - scanner: extract shared has_transcriber_fingerprint helper, survive copy2 failures in the restore loop (counted and reported) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7
1 participant
Summary
Fixes the production incident of 2026-07-13 where transcriber-written SMIL manifests broke adaptive playback (single unnamed 1080p stream, malformed
CODECS="avc1,mp4a"in the Wowza playlist).Root cause: when a video's
.smilwas missing or failed XML parsing,write_smilcreated/regenerated it from scratch with a single<video>node — the one variant we transcribed — plus barevideoCodecId/audioCodecIdparams. This stacked on a second failure: the transcoder-side SMIL generator had lost write permission to storage, so a growing set of videos had no original SMIL for us to update.Changes
write_smilis now strictly additive — it only adds<textstream>entries to an existing, valid, transcoder-generated SMIL:False)<video>nodesscripts/find_broken_smils.py— stdlib-only remediation tool for a host with storage access (e.g. prod12):BROKEN_HAS_BACKUP→ auto-restore from the oldest valid.smil.bak.*(--restore-bak --apply, dry-run by default)BROKEN_TRANSCRIBER_CREATED,MISSING_SMIL(video has_1080p.mp4but no SMIL at all),UNPARSEABLE→ written tosmils_to_regenerate.txtfor the transcoder-side generatorTests rewritten to encode the additive-only contract — the old tests asserted the harmful create-from-scratch behavior.
Test plan
uv run pytest tests/— 101 passed_audio.smilskipped)smils_to_regenerate.txtfor the transcoder-side generator), 0 restorable from backup🤖 Generated with Claude Code
https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7