fix: make write_smil strictly additive, add broken-SMIL scanner#9
fix: make write_smil strictly additive, add broken-SMIL scanner#9yidakra wants to merge 2 commits into
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: 18 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 expanded tests for preservation, idempotency, backups, and subtitle handling. ChangesSMIL integrity workflows
Estimated code review effort: 4 (Complex) | ~45 minutes 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
| try: | ||
| tree = ET.parse(smil_path) | ||
| except ET.ParseError as exc: | ||
| record["verdict"] = "UNPARSEABLE" | ||
| record["error"] = str(exc) | ||
| return record |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Catch OSError alongside ET.ParseError in classify.
ET.parse can raise OSError (e.g., PermissionError) on unreadable or deleted files. On a large filesystem, a single permission error crashes the entire scan. oldest_valid_bak already catches both — classify should too.
🛡️ Proposed fix
- except ET.ParseError as exc:
+ except (ET.ParseError, OSError) as exc:
record["verdict"] = "UNPARSEABLE"
record["error"] = str(exc)
return record📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| tree = ET.parse(smil_path) | |
| except ET.ParseError as exc: | |
| record["verdict"] = "UNPARSEABLE" | |
| record["error"] = str(exc) | |
| return record | |
| try: | |
| tree = ET.parse(smil_path) | |
| except (ET.ParseError, OSError) as exc: | |
| record["verdict"] = "UNPARSEABLE" | |
| record["error"] = str(exc) | |
| return record |
🤖 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 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.
| has_codec_params = any( | ||
| param.get("name") in ("videoCodecId", "audioCodecId") for v in videos for param in v.findall("param") | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the transcriber-fingerprint check into a shared helper.
The codec-param fingerprint logic is duplicated in classify (lines 102–104) and oldest_valid_bak (lines 135–137). If the fingerprint definition changes, both must be updated in sync.
♻️ Proposed helper extraction
+def has_transcriber_fingerprint(videos: list[ET.Element]) -> bool:
+ """Return True if any <video> node carries videoCodecId/audioCodecId params."""
+ return any(
+ param.get("name") in ("videoCodecId", "audioCodecId")
+ for v in videos
+ for param in v.findall("param")
+ )
+Then in classify:
- has_codec_params = any(
- param.get("name") in ("videoCodecId", "audioCodecId") for v in videos for param in v.findall("param")
- )
+ has_codec_params = has_transcriber_fingerprint(videos)And in oldest_valid_bak:
- fingerprint = any(
- param.get("name") in ("videoCodecId", "audioCodecId") for v in videos for param in v.findall("param")
- )
+ fingerprint = has_transcriber_fingerprint(videos)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| has_codec_params = any( | |
| param.get("name") in ("videoCodecId", "audioCodecId") for v in videos for param in v.findall("param") | |
| ) | |
| has_codec_params = has_transcriber_fingerprint(videos) |
🤖 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 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.
| if args.apply: | ||
| shutil.copy2(bak, rec["smil"]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle shutil.copy2 failures in the restore loop.
A single I/O error (permission, disk full) crashes the script, leaving remaining files unprocessed and the regen-list rewrite at line 201 never executes. Wrap the copy in try/except and continue.
🛡️ Proposed fix
if args.apply:
- shutil.copy2(bak, rec["smil"])
+ try:
+ shutil.copy2(bak, rec["smil"])
+ except OSError as exc:
+ print(f"ERROR restoring {rec['smil']}: {exc}", file=sys.stderr)
+ regen_needed.append(rec["smil"])
+ skipped += 1
+ continue
print(f"{'RESTORED' if args.apply else 'WOULD RESTORE'} {rec['smil']} <- {bak.name}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.apply: | |
| shutil.copy2(bak, rec["smil"]) | |
| if args.apply: | |
| try: | |
| shutil.copy2(bak, rec["smil"]) | |
| except OSError as exc: | |
| print(f"ERROR restoring {rec['smil']}: {exc}", file=sys.stderr) | |
| regen_needed.append(rec["smil"]) | |
| skipped += 1 | |
| continue | |
| print(f"{'RESTORED' if args.apply else 'WOULD RESTORE'} {rec['smil']} <- {bak.name}") |
🤖 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 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.
| Returns: | ||
| bool: True if the SMIL was updated, False if the update was skipped. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return False when no subtitle was associated.
When all requested VTT or TTML files are missing, execution still reaches Line 791 and reports success despite adding nothing. Track whether a textstream was added and skip the write with False otherwise; also assert this result in test_smil_missing_vtt_warning.
Also applies to: 791-791
🤖 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 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.
| # Write atomically so a crash mid-write can never leave a truncated SMIL | ||
| tmp_path = job.smil.with_suffix(job.smil.suffix + ".tmp") | ||
| tree.write(tmp_path, encoding="utf-8", xml_declaration=True) | ||
| tmp_path.replace(job.smil) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the original SMIL file permissions during atomic replacement.
Writing a fresh .tmp file and replacing the SMIL changes its inode and mode to the process umask defaults. This can either expose the manifest or make it unreadable to the playback service. Seed a unique temporary file with shutil.copy2(job.smil, tmp_path) before serializing, then replace it and verify the mode in tests.
🤖 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 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.
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
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/— 96 passed_audio.smilskipped)🤖 Generated with Claude Code
https://claude.ai/code/session_01GPhtp4PREZcRfm2WXL5NF7