rbrooks/Quest-Board

Watch

Fork 0

[Backend] The audio retry endpoint disarms both duration guards #421

New issue

Closed

opened 2026-08-26 03:50:05 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-26 03:50:05 +00:00

Contributor

Severity: MEDIUM. Found while recovering the 2026-08-26 production incident.

The defect

POST /sessions/{id}/audio/retry (routers/sessions.py) re-queues process_audio with a hardcoded duration of 0:

process_audio.delay(
    str(session_id),
    str(audio_path),
    guild_id,
    channel_id,
    0,  # duration unknown on retry
)

Both duration invariants added in #324 early-return below DURATION_GUARD_MIN_SECONDS (600):

check_tracks_cover_session — every per-speaker track must span the session
check_transcript_covers_session — the merged timeline must cover most of it

So a retry runs with no coverage checking at all. These are the guards that exist specifically to catch the #320 failure — per-speaker tracks on their own compressed clocks, producing a scrambled, misattributed transcript that reads fine to a human. A retry is precisely when you most want them: something already went wrong with this recording.

The comment is honest about the cause (duration unknown on retry) but the consequence was not thought through: unknown duration silently became no verification, rather than derive it or say so.

Proposed fix

The duration is not actually unknown — it is recoverable from the audio itself. _wav_duration_seconds already reads it, and every track in a session directory is the same length by construction since #320.

Derive the session duration from the longest track in the audio directory, and pass that.
Failing that (unreadable audio), make the guards' skip visible — log a warning naming which invariants were not checked — rather than silently passing.
Consider storing the recording duration on the session at handoff so it never has to be re-derived. The bot knows it exactly.

Option 3 is the real fix; 1 is the cheap one that removes the hole today.

Note

This was worked around manually during the 2026-08-26 recovery by queueing process_audio with the true duration (4639s) instead of using the endpoint, so that session's reprocess was guarded. Nobody else gets that.

Acceptance criteria

A retry runs with a real session duration, not 0
Both duration guards are active on a retried session
If a duration genuinely cannot be determined, the skipped guards are logged at WARNING
Test: a retried session with deliberately short tracks is rejected by check_tracks_cover_session

**Severity: MEDIUM.** Found while recovering the 2026-08-26 production incident. ## The defect `POST /sessions/{id}/audio/retry` (`routers/sessions.py`) re-queues `process_audio` with a hardcoded duration of `0`: ```python process_audio.delay( str(session_id), str(audio_path), guild_id, channel_id, 0, # duration unknown on retry ) ``` Both duration invariants added in #324 early-return below `DURATION_GUARD_MIN_SECONDS` (600): - `check_tracks_cover_session` — every per-speaker track must span the session - `check_transcript_covers_session` — the merged timeline must cover most of it So **a retry runs with no coverage checking at all.** These are the guards that exist specifically to catch the #320 failure — per-speaker tracks on their own compressed clocks, producing a scrambled, misattributed transcript that reads fine to a human. A retry is precisely when you most want them: something already went wrong with this recording. The comment is honest about the cause (`duration unknown on retry`) but the consequence was not thought through: unknown duration silently became *no verification*, rather than *derive it* or *say so*. ## Proposed fix The duration is not actually unknown — it is recoverable from the audio itself. `_wav_duration_seconds` already reads it, and every track in a session directory is the same length by construction since #320. 1. Derive the session duration from the longest track in the audio directory, and pass that. 2. Failing that (unreadable audio), make the guards' skip **visible** — log a warning naming which invariants were not checked — rather than silently passing. 3. Consider storing the recording duration on the session at handoff so it never has to be re-derived. The bot knows it exactly. Option 3 is the real fix; 1 is the cheap one that removes the hole today. ## Note This was worked around manually during the 2026-08-26 recovery by queueing `process_audio` with the true duration (4639s) instead of using the endpoint, so that session's reprocess *was* guarded. Nobody else gets that. ## Acceptance criteria - [ ] A retry runs with a real session duration, not `0` - [ ] Both duration guards are active on a retried session - [ ] If a duration genuinely cannot be determined, the skipped guards are logged at WARNING - [ ] Test: a retried session with deliberately short tracks is rejected by `check_tracks_cover_session`

claude-bot added this to the v4.0.0 - Session Pipeline Accuracy milestone

2026-08-26 03:50:09 +00:00

claude-bot added the

backend

bug

labels

2026-08-26 03:50:10 +00:00

claude-bot commented

2026-08-26 05:48:51 +00:00

Author

Contributor

Correction to this issue's premise

I wrote that "the consequence was not thought through". That was wrong, and the code says so plainly — check_tracks_cover_session's docstring reads:

Skipped when duration_seconds is unknown or trivially small: manual reprocess endpoints pass 0, so re-running an already-suspect recording is deliberately allowed as an explicit admin action.

So the bypass was a decision, not an oversight, and it has a real rationale: if a recording genuinely is mis-clocked, a guard that blocks reprocessing means you can never retry it at all.

The concern survives in a narrower form, which is what got fixed:

It disables the wrong guard too. check_transcript_covers_session checks whether transcription covered the session — nothing to do with whether the source tracks are suspect. "Let an admin re-run a bad recording" does not justify skipping that one, and passing 0 skipped both.
It is silent. Nothing told the GM the invariants had not run. A deliberate bypass that leaves no trace is indistinguishable from a bug.
The duration was never actually unknown. Every track spans the whole session by construction since #320, so the longest track is the session.

What shipped

derive_session_duration(audio_dir) recovers the length from the tracks, and both reprocess endpoints — GM-facing POST /sessions/{id}/audio/retry and the admin one, which had the same hole — now pass it. The guards run on a retry exactly as on a first pass. When nothing readable can be measured the duration is still 0 and the guards still skip, but the endpoint logs which invariants were not checked rather than passing over it.

On the original rationale: a retry that now fails with "Track for 'Sol_Invictus' covers 400s of a 4639s recording" is a better outcome than one that silently rebuilds the same scrambled transcript. The GM learns the recording is unrepairable — which is what pre_timeline_fix (#328) exists to record — instead of getting plausible-looking nonsense.

One case deliberately kept: a session under DURATION_GUARD_MIN_SECONDS (600s) is still exempt, because below that timing noise dominates and the check is meaningless.

Tests in test_reprocess_duration_guards.py, including the one that matters — a short track must not be allowed to define the session length, or the guard would compare every track against the broken one and validate itself into always passing.

## Correction to this issue's premise I wrote that "the consequence was not thought through". That was wrong, and the code says so plainly — `check_tracks_cover_session`'s docstring reads: > Skipped when *duration_seconds* is unknown or trivially small: manual reprocess endpoints pass 0, so re-running an already-suspect recording is **deliberately allowed** as an explicit admin action. So the bypass was a decision, not an oversight, and it has a real rationale: if a recording genuinely is mis-clocked, a guard that blocks reprocessing means you can never retry it at all. The concern survives in a narrower form, which is what got fixed: 1. **It disables the wrong guard too.** `check_transcript_covers_session` checks whether *transcription* covered the session — nothing to do with whether the source tracks are suspect. "Let an admin re-run a bad recording" does not justify skipping that one, and passing `0` skipped both. 2. **It is silent.** Nothing told the GM the invariants had not run. A deliberate bypass that leaves no trace is indistinguishable from a bug. 3. **The duration was never actually unknown.** Every track spans the whole session by construction since #320, so the longest track *is* the session. ## What shipped `derive_session_duration(audio_dir)` recovers the length from the tracks, and both reprocess endpoints — GM-facing `POST /sessions/{id}/audio/retry` **and** the admin one, which had the same hole — now pass it. The guards run on a retry exactly as on a first pass. When nothing readable can be measured the duration is still 0 and the guards still skip, but the endpoint logs which invariants were not checked rather than passing over it. On the original rationale: a retry that now fails with *"Track for 'Sol_Invictus' covers 400s of a 4639s recording"* is a better outcome than one that silently rebuilds the same scrambled transcript. The GM learns the recording is unrepairable — which is what `pre_timeline_fix` (#328) exists to record — instead of getting plausible-looking nonsense. One case deliberately kept: a session under `DURATION_GUARD_MIN_SECONDS` (600s) is still exempt, because below that timing noise dominates and the check is meaningless. Tests in `test_reprocess_duration_guards.py`, including the one that matters — a short track must not be allowed to *define* the session length, or the guard would compare every track against the broken one and validate itself into always passing.

rbrooks referenced this issue from a commit

2026-08-26 16:23:01 +00:00

feat(backend): let a player run more than one character (#330)

claude-bot referenced this issue

2026-08-26 16:59:25 +00:00

[Frontend] Let a GM pin which character a player was running for a past session #422

claude-bot commented

2026-08-27 23:27:47 +00:00

Author

Contributor

Verified against the acceptance criteria before closing. All met.

Retry runs with a real duration — derive_session_duration(audio_path) is called before process_audio.delay(...) in both routers/sessions.py:930-935 and routers/admin.py:208-214.
Both guards re-armed — check_tracks_cover_session and check_transcript_covers_session run unconditionally inside process_audio against whatever duration they are handed, so a nonzero derived value re-arms both.
Undeterminable duration logged at WARNING — sessions.py:936-943, admin.py:215-222, naming that the guards will not run.
Regression test — test_a_mis_clocked_track_is_caught_once_the_duration_is_real (tests/test_reprocess_duration_guards.py:88) reproduces the real 4639s/400s shape and asserts the RuntimeError names Sol_Invictus.

On the quiet-player risk

I specifically asked for this to be scrutinised, since a guard that rejects audio is exactly where a quiet player gets deleted. It does not apply here, and the reason is worth recording.

check_tracks_cover_session compares each track's WAV file duration, not its speech content. Since #320, the bot pads every speaker's WAV to the full session length (recording.py:842, sink.close(pad_to=duration_s)), so a player who spoke 163 seconds of a 4,639-second session still produces a ~4,639-second file and passes trivially. test_a_healthy_recording_still_passes covers exactly that shape.

So the guard is wall-clock-aware and speech-blind by construction, which is what makes it safe. Worth knowing, because the other silence guard is not — see #425, which came out of this same verification pass and is a merge blocker.

Closing. Part of a full acceptance-criteria pass across the v4.0.0 milestone.

Verified against the acceptance criteria before closing. All met. - **Retry runs with a real duration** — `derive_session_duration(audio_path)` is called before `process_audio.delay(...)` in both `routers/sessions.py:930-935` and `routers/admin.py:208-214`. - **Both guards re-armed** — `check_tracks_cover_session` and `check_transcript_covers_session` run unconditionally inside `process_audio` against whatever duration they are handed, so a nonzero derived value re-arms both. - **Undeterminable duration logged at WARNING** — `sessions.py:936-943`, `admin.py:215-222`, naming that the guards will not run. - **Regression test** — `test_a_mis_clocked_track_is_caught_once_the_duration_is_real` (`tests/test_reprocess_duration_guards.py:88`) reproduces the real 4639s/400s shape and asserts the `RuntimeError` names Sol_Invictus. ## On the quiet-player risk I specifically asked for this to be scrutinised, since a guard that rejects audio is exactly where a quiet player gets deleted. **It does not apply here, and the reason is worth recording.** `check_tracks_cover_session` compares each track's **WAV file duration**, not its speech content. Since #320, the bot pads every speaker's WAV to the full session length (`recording.py:842`, `sink.close(pad_to=duration_s)`), so a player who spoke 163 seconds of a 4,639-second session still produces a ~4,639-second file and passes trivially. `test_a_healthy_recording_still_passes` covers exactly that shape. So the guard is wall-clock-aware and speech-blind by construction, which is what makes it safe. Worth knowing, because the *other* silence guard is not — see #425, which came out of this same verification pass and is a merge blocker. Closing. Part of a full acceptance-criteria pass across the v4.0.0 milestone.

claude-bot closed this issue

2026-08-27 23:27:56 +00:00

rbrooks referenced this issue from a commit

2026-08-28 23:16:57 +00:00

fix(backend): stop a silent tail destroying a good transcript (#431)

claude-bot referenced this issue

2026-08-28 23:26:29 +00:00

v4.0.0 — deterministic attribution: the summary is composed from checked events, not asserted #434

claude-bot referenced this issue

2026-08-29 07:57:13 +00:00

[Backend] Fail processing when per-speaker track durations do not match the recorded session length #324

rbrooks referenced this issue from a commit