diff --git a/milestones/README.md b/milestones/README.md index e8bb20e..6b0c270 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +5,14 @@ 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:** **[generation-zero-values.md](generation-zero-values.md)** — -`temperature: 0` / `max_whisper_segment_text_chars: 0` must not be -replaced by ``or`` defaults. +**Active:** **[concat-ffmpeg-timeout.md](concat-ffmpeg-timeout.md)** — +concat must not leave a truncated full-demo mp4 after ffmpeg +timeout or failure. **Shipped:** +- **[generation-zero-values.md](generation-zero-values.md)** — + `temperature: 0` / `max_whisper_segment_text_chars: 0` must not be + replaced by ``or`` defaults (#122). - **[compose-ffmpeg-timeout.md](compose-ffmpeg-timeout.md)** — compose must not treat a timed-out ffmpeg mux as success (#121). - **[visual-beats-numeric.md](visual-beats-numeric.md)** — diff --git a/milestones/concat-ffmpeg-timeout.md b/milestones/concat-ffmpeg-timeout.md new file mode 100644 index 0000000..4925974 --- /dev/null +++ b/milestones/concat-ffmpeg-timeout.md @@ -0,0 +1,34 @@ +# Milestone: concat must not leave a truncated ffmpeg output + +**Status:** Active +**PR:** [#123](https://github.com/jmjava/documentation-generator/pull/123) +**Depends on:** `milestones/compose-ffmpeg-timeout.md` (PR #121), +`milestones/generation-zero-values.md` (PR #122) + +## Problem + +Compose now raises and unlinks a partial mux on ffmpeg timeout (#121). +Concat already raised ``ConcatError`` on timeout / non-zero ffmpeg, but +it left ``recordings/.mp4`` in place. ``ffmpeg -y`` writes as +it goes, so a hung or failed concat could leave a truncated full-demo +file that ``pages`` / validate treat as a finished recording. + +## Goal + +On concat ffmpeg timeout, ``CalledProcessError``, or missing ffmpeg, +unlink the incomplete output (same contract as compose). Keep raising +``ConcatError``. Empty concat maps stay a no-op. + +## Done when + +- [x] Timeout / failed concat removes the incomplete target mp4. +- [x] Tests cover timeout and CalledProcessError with a pre-existing + partial file. +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (709 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Changing the 300s concat ffmpeg timeout. +- Empty ``concat:`` maps (still a no-op). diff --git a/milestones/generation-zero-values.md b/milestones/generation-zero-values.md index 9c0d94c..d527133 100644 --- a/milestones/generation-zero-values.md +++ b/milestones/generation-zero-values.md @@ -1,6 +1,6 @@ # Milestone: honor explicit generation zeros (temperature / max_chars) -**Status:** Active +**Status:** Shipped **PR:** [#122](https://github.com/jmjava/documentation-generator/pull/122) **Depends on:** `milestones/generation-numeric-tunables.md` (PR #117), `milestones/compose-ffmpeg-timeout.md` (PR #121) diff --git a/src/docgen/concat.py b/src/docgen/concat.py index bd2e447..a7759af 100644 --- a/src/docgen/concat.py +++ b/src/docgen/concat.py @@ -89,11 +89,25 @@ def _build_one(self, out_name: str, seg_ids: list[str]) -> None: cwd=str(recordings_dir), ) except FileNotFoundError as exc: + _unlink_incomplete(out) raise ConcatError("[concat] ffmpeg not found in PATH") from exc except subprocess.CalledProcessError as exc: + _unlink_incomplete(out) detail = (exc.stderr or exc.stdout or "")[:400] raise ConcatError(f"[concat] ffmpeg failed: {detail}") from exc - except subprocess.TimeoutExpired as exc: - raise ConcatError("[concat] ffmpeg timed out") from exc + except subprocess.TimeoutExpired as ext: + existed = out.exists() + _unlink_incomplete(out) + extra = f" (removed incomplete {out.name})" if existed else "" + raise ConcatError(f"[concat] ffmpeg timed out{extra}") from ext finally: concat_list.unlink(missing_ok=True) + + +def _unlink_incomplete(path: Path) -> None: + """Remove a truncated concat output so later stages cannot treat it as finished.""" + if path.exists(): + try: + path.unlink() + except OSError: + pass diff --git a/tests/test_concat.py b/tests/test_concat.py index ada6d45..6e41920 100644 --- a/tests/test_concat.py +++ b/tests/test_concat.py @@ -2,6 +2,7 @@ from __future__ import annotations +import subprocess from pathlib import Path import pytest @@ -71,3 +72,45 @@ def test_concat_builder_rejects_integer_segment_id() -> None: ) with pytest.raises(ConcatError, match="quoted string segment id"): ConcatBuilder(cfg).build() # type: ignore[arg-type] + + +def _seed_recordings(tmp_path: Path) -> None: + rec = tmp_path / "recordings" + rec.mkdir() + (rec / "01-a.mp4").write_bytes(b"seg-a") + (rec / "02-b.mp4").write_bytes(b"seg-b") + + +def test_concat_ffmpeg_timeout_removes_incomplete_output( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + cfg = _cfg(tmp_path, {"full": ["01", "02"]}) + _seed_recordings(tmp_path) + out = tmp_path / "recordings" / "full.mp4" + out.write_bytes(b"partial-concat") + + def fake_run(cmd, **_kwargs): + raise subprocess.TimeoutExpired(cmd=cmd, timeout=300) + + monkeypatch.setattr(subprocess, "run", fake_run) + with pytest.raises(ConcatError, match="removed incomplete full.mp4"): + ConcatBuilder(cfg).build(name="full") + assert not out.exists() + assert not list((tmp_path / "recordings").glob(".concat-*.txt")) + + +def test_concat_ffmpeg_failure_removes_incomplete_output( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + cfg = _cfg(tmp_path, {"full": ["01", "02"]}) + _seed_recordings(tmp_path) + out = tmp_path / "recordings" / "full.mp4" + out.write_bytes(b"partial-concat") + + def fake_run(cmd, **_kwargs): + raise subprocess.CalledProcessError(1, cmd, stderr="mux error") + + monkeypatch.setattr(subprocess, "run", fake_run) + with pytest.raises(ConcatError, match="ffmpeg failed"): + ConcatBuilder(cfg).build(name="full") + assert not out.exists()