Compose #8 streaming with #16 resume: --state-file override + 10h crash/resume integration test #21

New issue

Closed

opened 2026-09-09 23:40:58 +00:00 by Mark · 0 comments

Mark commented

2026-09-09 23:40:58 +00:00

Owner

Post-merge composition audit of #8 (long-file streaming) against #16 / #19 (chunk resume). Both are on main (4a0b976); this issue carries the three items the audit found missing. Source card: kanban t_02afa75b (fixit board). Full audit writeup: ~/.hermes/kanban/boards/fixit/workspaces/t_d5c5d159/README-t_02afa75b-integration.md.

Already verified — no work needed

Chunk indices are stable across runs. chunking.plan_chunks is a pure function of (duration_sec, chunk_minutes) — no wall clock, no random, no ordering dependence. Confirmed empirically: plan_chunks(10h, 30min) yields 20 chunks, identical over repeated calls; a +0.5s ffprobe duration jitter shifts only the final chunk's end_sec and leaves every non-final (index, start_sec) byte-identical, so a resume after a duration re-probe still joins on index. The one case that would break the join — the user changing --chunk-minutes between runs — is already hard-rejected by the chunk_seconds mismatch guard in state.validate_state (src/whisper_transcribe/state.py:396). Nothing to change here.

1. Add a `--state-file PATH` override

Today the state file location is controlled only by --workdir DIR; the filename is the fixed STATE_FILENAME constant (transcribe.state.json). There is no way to point at an explicit path.

Plumbing note — this is not a one-line addition. state.py derives every path from a workdir today (state_path, chunk_json_path, cleanup_state, find_stale_state_workspaces, purge_stale_state_workspaces). Two viable shapes:

(a) Thread an explicit state_file: Path | None through _run_transcription and have state_path(workdir) become the fallback when it is None. Cleanest semantics; touches every state.py helper signature.
(b) Derive the workdir from the override — --state-file /x/y/custom.json implies workdir /x/y plus a non-default state filename, keeping per-chunk JSONs (transcribe.chunk_NNNN.json) alongside it. Smaller diff; keeps the one-directory-per-job invariant that --clean-stale scanning relies on.

Shape (b) is preferred unless there is a reason to let the state file and its per-chunk JSONs diverge.

Acceptance:

--state-file PATH sets the state-file path explicitly and takes precedence over --workdir.
--state-file together with --workdir is either a usage error (EXIT_USAGE) or documented precedence — pick one and test it.
Parent directory is created if missing, consistent with the current job_workdir.mkdir(parents=True, exist_ok=True) at cli.py:1050.
--fresh and --clean-stale respect the override.
--help lists the flag (mirror the existing assertions in tests/test_resume.py:463 / :473).

2. Decide + document the default state-file location

The source card that generated this issue asks for the state file to be co-located with the output file by default. The shipped behaviour is source-audio-adjacent (cli._resolve_workdir, cli.py:761), and docs/state-file.md §1 states that requirement deliberately: "The state file lives in the workdir, not next to every output JSON, so --out paths can move freely without invalidating resume."

The tension is real:

Source-adjacent (shipped): one state file per source audio; changing --out between runs does not throw away 7 hours of completed chunks.
Output-adjacent (card): state follows the artifact the user is producing; read-only or shared media directories stay clean.

Recommendation: keep the shipped source-adjacent default — resume correctness is the entire point of #16, and output-adjacency loses it the moment --out moves — and treat the read-only-source-dir case as what --state-file (item 1) is for. Flipping the default is a behaviour change for anyone with state files already on disk, so it needs an explicit decision rather than a silent change.

Acceptance: whichever way this is decided, docs/state-file.md §1 and the --workdir / --state-file help text state the same rule, and a test pins it.

3. Add the combined 10h streaming + resume integration test

The existing tests/test_resume.py:614 test_resume_picks_up_after_interrupt has the right shape but the wrong scale: 120-minute stubbed duration (4 chunks), crash on the 4th chunk, 3 done / 1 pending. The card's acceptance criterion is a 10h file crashing at hour 7.

Acceptance for the new test:

Synthetic 10h source (stub probe_duration -> 36000.0 in line with the existing monkeypatch pattern; do not generate 10h of real audio).
Default 30-minute chunking gives 20 chunks; the hour-7 boundary is chunk index 14 (start_sec=25200.0, end_sec=27000.0).
First pass raises mid-loop at chunk 14, leaving chunks 0-13 done.
Second pass with --resume re-processes exactly the 6 remaining chunks (14-19) and no others.
Final transcript asserts no duplicates (unique segment offsets, as the existing test does) and no gaps — segment coverage is contiguous across the full 0..36000s timeline. The existing test only checks uniqueness.
Exercise the streaming half of the pipeline, not just the resume bookkeeping: audio_stream.decode_chunk is currently only covered in isolation against a 1-second ffmpeg sine (tests/test_audio_stream.py), never jointly with the resume loop. Mark the test @requires_ffmpeg if it needs real decoding, matching tests/conftest.py.

Notes

Baseline at audit time: tree clean, ruff check src tests clean, suite 285 passed, 16 skipped (excluding tests/test_benchmark_gpu_sizing.py).
Related: #8, #16, #19.

Post-merge composition audit of #8 (long-file streaming) against #16 / #19 (chunk resume). Both are on `main` (`4a0b976`); this issue carries the three items the audit found missing. Source card: kanban `t_02afa75b` (fixit board). Full audit writeup: `~/.hermes/kanban/boards/fixit/workspaces/t_d5c5d159/README-t_02afa75b-integration.md`. ## Already verified — no work needed **Chunk indices are stable across runs.** `chunking.plan_chunks` is a pure function of `(duration_sec, chunk_minutes)` — no wall clock, no `random`, no ordering dependence. Confirmed empirically: `plan_chunks(10h, 30min)` yields 20 chunks, identical over repeated calls; a +0.5s `ffprobe` duration jitter shifts only the final chunk's `end_sec` and leaves every non-final `(index, start_sec)` byte-identical, so a resume after a duration re-probe still joins on index. The one case that would break the join — the user changing `--chunk-minutes` between runs — is already hard-rejected by the `chunk_seconds mismatch` guard in `state.validate_state` (`src/whisper_transcribe/state.py:396`). Nothing to change here. ## 1. Add a `--state-file PATH` override Today the state file location is controlled only by `--workdir DIR`; the filename is the fixed `STATE_FILENAME` constant (`transcribe.state.json`). There is no way to point at an explicit path. Plumbing note — this is not a one-line addition. `state.py` derives every path from a workdir today (`state_path`, `chunk_json_path`, `cleanup_state`, `find_stale_state_workspaces`, `purge_stale_state_workspaces`). Two viable shapes: - **(a) Thread an explicit `state_file: Path | None` through `_run_transcription`** and have `state_path(workdir)` become the fallback when it is `None`. Cleanest semantics; touches every `state.py` helper signature. - **(b) Derive the workdir from the override** — `--state-file /x/y/custom.json` implies workdir `/x/y` plus a non-default state filename, keeping per-chunk JSONs (`transcribe.chunk_NNNN.json`) alongside it. Smaller diff; keeps the one-directory-per-job invariant that `--clean-stale` scanning relies on. Shape (b) is preferred unless there is a reason to let the state file and its per-chunk JSONs diverge. Acceptance: - `--state-file PATH` sets the state-file path explicitly and takes precedence over `--workdir`. - `--state-file` together with `--workdir` is either a usage error (`EXIT_USAGE`) or documented precedence — pick one and test it. - Parent directory is created if missing, consistent with the current `job_workdir.mkdir(parents=True, exist_ok=True)` at `cli.py:1050`. - `--fresh` and `--clean-stale` respect the override. - `--help` lists the flag (mirror the existing assertions in `tests/test_resume.py:463` / `:473`). ## 2. Decide + document the default state-file location The source card that generated this issue asks for the state file to be **co-located with the output file** by default. The shipped behaviour is **source-audio-adjacent** (`cli._resolve_workdir`, `cli.py:761`), and `docs/state-file.md` §1 states that requirement deliberately: *"The state file lives in the workdir, not next to every output JSON, so `--out` paths can move freely without invalidating resume."* The tension is real: - Source-adjacent (shipped): one state file per source audio; changing `--out` between runs does not throw away 7 hours of completed chunks. - Output-adjacent (card): state follows the artifact the user is producing; read-only or shared media directories stay clean. Recommendation: **keep the shipped source-adjacent default** — resume correctness is the entire point of #16, and output-adjacency loses it the moment `--out` moves — and treat the read-only-source-dir case as what `--state-file` (item 1) is for. Flipping the default is a behaviour change for anyone with state files already on disk, so it needs an explicit decision rather than a silent change. Acceptance: whichever way this is decided, `docs/state-file.md` §1 and the `--workdir` / `--state-file` help text state the same rule, and a test pins it. ## 3. Add the combined 10h streaming + resume integration test The existing `tests/test_resume.py:614 test_resume_picks_up_after_interrupt` has the right shape but the wrong scale: 120-minute stubbed duration (4 chunks), crash on the 4th chunk, 3 done / 1 pending. The card's acceptance criterion is a 10h file crashing at hour 7. Acceptance for the new test: - Synthetic 10h source (stub `probe_duration -> 36000.0` in line with the existing monkeypatch pattern; do not generate 10h of real audio). - Default 30-minute chunking gives 20 chunks; the hour-7 boundary is chunk index 14 (`start_sec=25200.0`, `end_sec=27000.0`). - First pass raises mid-loop at chunk 14, leaving chunks 0-13 `done`. - Second pass with `--resume` re-processes exactly the 6 remaining chunks (14-19) and no others. - Final transcript asserts **no duplicates** (unique segment offsets, as the existing test does) **and no gaps** — segment coverage is contiguous across the full 0..36000s timeline. The existing test only checks uniqueness. - Exercise the streaming half of the pipeline, not just the resume bookkeeping: `audio_stream.decode_chunk` is currently only covered in isolation against a 1-second ffmpeg sine (`tests/test_audio_stream.py`), never jointly with the resume loop. Mark the test `@requires_ffmpeg` if it needs real decoding, matching `tests/conftest.py`. ## Notes - Baseline at audit time: tree clean, `ruff check src tests` clean, suite `285 passed, 16 skipped` (excluding `tests/test_benchmark_gpu_sizing.py`). - Related: #8, #16, #19.