diff --git a/milestones/README.md b/milestones/README.md index ed3ee2e..acd9d43 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +5,13 @@ repositories that install `docgen` and maintain their own demo bundle. The library no longer ships an in-repo dogfood; consumers are the integration test of record. -**Active:** **[visual-beats-numeric.md](visual-beats-numeric.md)** — -`visual_beats` / `default_visual_beats` must be YAML numbers (bool -`true` used to become one beat; invalid types auto-estimated). +**Active:** **[compose-ffmpeg-timeout.md](compose-ffmpeg-timeout.md)** — +compose must not treat a timed-out ffmpeg mux as success because a +partial output file exists. **Shipped:** +- **[visual-beats-numeric.md](visual-beats-numeric.md)** — + `visual_beats` / `default_visual_beats` must be YAML numbers (#120). - **[validation-enable-bools.md](validation-enable-bools.md)** — validation / manim enable flags must be YAML booleans (#119). - **[validation-numeric-tunables.md](validation-numeric-tunables.md)** — diff --git a/milestones/compose-ffmpeg-timeout.md b/milestones/compose-ffmpeg-timeout.md new file mode 100644 index 0000000..ac9c14f --- /dev/null +++ b/milestones/compose-ffmpeg-timeout.md @@ -0,0 +1,38 @@ +# Milestone: compose must not accept a timed-out ffmpeg mux + +**Status:** Active +**PR:** [#121](https://github.com/jmjava/documentation-generator/pull/121) +**Depends on:** `milestones/pipeline-fail-closed.md`, +`milestones/visual-beats-numeric.md` (PR #120) + +## Problem + +``Composer._run_ffmpeg`` treated ``subprocess.TimeoutExpired`` as +**success** when the output path already existed and had a non-zero +size. A hung mux could leave a truncated ``recordings/.mp4``; +compose printed a warning, returned, and counted the segment as +composed. Concat already raises ``ConcatError`` on ffmpeg timeout. + +## Goal + +Fail closed. A timed-out ffmpeg compose run must raise +``ComposeError`` even when a partial output file exists. Remove the +incomplete file so later stages cannot treat it as a finished +recording. + +## Done when + +- [x] Timeout with a partial output raises ``ComposeError``. +- [x] Incomplete output is removed. +- [x] Tests cover timeout with and without an existing output file. +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (703 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Changing ``compose.ffmpeg_timeout_sec`` defaults. +- Duration-probe SKIP when ffprobe is missing (pipeline still fails + when mapped segments are not composed). +- Manim render timeouts (already treated as a failed scene, not a + successful mp4). diff --git a/milestones/visual-beats-numeric.md b/milestones/visual-beats-numeric.md index e458733..8760b42 100644 --- a/milestones/visual-beats-numeric.md +++ b/milestones/visual-beats-numeric.md @@ -1,6 +1,6 @@ # Milestone: visual_beats must be YAML numbers (no silent auto-fallback) -**Status:** Active +**Status:** Shipped **PR:** [#120](https://github.com/jmjava/documentation-generator/pull/120) **Depends on:** `milestones/generation-numeric-tunables.md` (PR #117), `milestones/validation-enable-bools.md` (PR #119) diff --git a/src/docgen/compose.py b/src/docgen/compose.py index 038f202..c8722be 100644 --- a/src/docgen/compose.py +++ b/src/docgen/compose.py @@ -331,14 +331,17 @@ def _run_ffmpeg(self, cmd: list[str]) -> None: except subprocess.CalledProcessError as exc: detail = (exc.stderr or exc.stdout or "")[:400] raise ComposeError(f"ffmpeg failed: {detail}") - except subprocess.TimeoutExpired: - if out_path and out_path.exists() and out_path.stat().st_size > 0: - print( - f" WARNING: ffmpeg timed out after {timeout_sec}s, " - f"but output exists at {out_path}." - ) - return - raise ComposeError(f"ffmpeg timed out after {timeout_sec}s") + except subprocess.TimeoutExpired as exc: + removed = "" + if out_path is not None and out_path.exists(): + try: + out_path.unlink() + removed = f" (removed incomplete {out_path.name})" + except OSError: + removed = f" (incomplete {out_path.name} still present)" + raise ComposeError( + f"ffmpeg timed out after {timeout_sec}s{removed}" + ) from exc def _manim_video_dirs(self) -> list[Path]: root = self.config.animations_dir / "media" / "videos" diff --git a/tests/test_compose.py b/tests/test_compose.py index 7513a7f..5243cfc 100644 --- a/tests/test_compose.py +++ b/tests/test_compose.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import subprocess import time from pathlib import Path @@ -265,3 +266,48 @@ def test_cli_compose_empty_segments_is_click_error(tmp_path: Path) -> None: result = runner.invoke(main, ["--config", str(c.yaml_path), "compose"]) assert result.exit_code != 0 assert "no segments to compose" in (result.output + result.stderr).lower() + + +def test_run_ffmpeg_timeout_with_partial_output_raises(tmp_path: Path, monkeypatch) -> None: + """A timed-out mux must not count as success just because a partial file exists.""" + cfg = { + "dirs": {"animations": "animations", "audio": "audio", "recordings": "recordings"}, + "segments": {"default": ["01"], "all": ["01"]}, + "segment_names": {"01": "01-demo"}, + "visual_map": {"01": {"type": "manim", "source": "Scene01.mp4"}}, + } + c = _write_cfg(tmp_path, cfg) + out = tmp_path / "recordings" / "01-demo.mp4" + out.parent.mkdir(parents=True, exist_ok=True) + out.write_bytes(b"partial-mux") + + def fake_run(cmd, **_kwargs): + raise subprocess.TimeoutExpired(cmd=cmd, timeout=1) + + monkeypatch.setattr(subprocess, "run", fake_run) + composer = Composer(c) + composer.ffmpeg_timeout_sec = 1 + with pytest.raises(ComposeError, match="removed incomplete 01-demo.mp4"): + composer._run_ffmpeg(["ffmpeg", "-y", str(out)]) + assert not out.exists() + + +def test_run_ffmpeg_timeout_without_output_raises(tmp_path: Path, monkeypatch) -> None: + cfg = { + "dirs": {"animations": "animations", "audio": "audio", "recordings": "recordings"}, + "segments": {"default": ["01"], "all": ["01"]}, + "visual_map": {"01": {"type": "manim", "source": "Scene01.mp4"}}, + } + c = _write_cfg(tmp_path, cfg) + missing = tmp_path / "recordings" / "missing.mp4" + missing.parent.mkdir(parents=True, exist_ok=True) + + def fake_run(cmd, **_kwargs): + raise subprocess.TimeoutExpired(cmd=cmd, timeout=2) + + monkeypatch.setattr(subprocess, "run", fake_run) + composer = Composer(c) + composer.ffmpeg_timeout_sec = 2 + with pytest.raises(ComposeError, match="ffmpeg timed out after 2s"): + composer._run_ffmpeg(["ffmpeg", "-y", str(missing)]) + assert not missing.exists()