[Backend] The silence guard can delete a quiet player from a full-length session #425
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: CRITICAL. Blocks the v4.0.0 merge. Found while verifying #348's acceptance criteria before closing it.
The defect
drop_silent_tracks(app/services/audio_service.py:1305) runs unconditionally on every session (app/tasks/reminder_tasks.py:2158) and drops any track whose_peak_amplitudefalls below the floor. That function samples the file rather than scanning it:Reads are capped at 16,000 frames — one second — while the spacing between them scales with file length. So coverage collapses as sessions get longer:
Stated exactly: the guard only guarantees detection for a speaker whose track contains one contiguous non-silent stretch longer than the window spacing — ~145 s on a 77-minute session. Anything shorter is caught only if a 1-second probe happens to land on it.
Why this is the failure the guard exists to prevent
Sol_Invictus spoke 163 seconds out of 4,639 in the 2026-08-26 session. That is above 145 s in total, but only safe if it was one unbroken block. A quiet player's speech is not one block — it is a few words every several minutes, which is precisely the distribution this sampling cannot see.
The consequence is silent: the track is dropped before transcription, the player vanishes from the transcript, the summary is generated without them, and the pipeline reports success. The GM is told nothing.
The test does not test this
test_one_brief_utterance_is_enough_to_keep_a_track(tests/test_silent_tracks.py:76) cites this exact production incident in its docstring — "Sol_Invictus in the 2026-08-26 session spoke for 163 seconds out of 4639" — and then builds a 60-second file. At that length windows are 1.9 s apart and 1 s wide, so coverage is total and the assertion cannot fail regardless of the bug.It is green, and it validates nothing. Same shape as the double corrected in
6ba6202: a test that models an impossible case will eventually assert that the impossible is fine.Not the bot-side guard
The bot's own drop logic is sound and is not in scope here. It uses
speech_bytes_written()— a running counter of decoded audio accumulated during capture (bot/questboard_bot/cogs/recording.py:296) — against a 0.5 s floor, so it has no sampling blind spot, and it reports dropped users throughuncaptured_member_ids.bot/tests/test_recording.py:1006covers the 163-of-4639 case properly.This issue is only about the backend's second line of defence, which is currently more dangerous than the thing it defends against.
Acceptance criteria
uncaptured_member_ids/ attendance surface the bot-side path uses, rather than a log lineNotes
SPEAKER_MISSING_MIN_SPEECH_SECONDS,audio_service.py:1246). Reusing that measure would remove the sampling question rather than tune it.Fixed in
aa0b6c9.has_audible_speechreads the file sequentially and stops at the first sample above the floor. No sampling, so no length-dependent degradation — and faster in the case that matters, since anyone who speaks ends the loop as soon as they do. Only a genuinely silent track is read to the end, which is the one file worth being certain about.The bug was worse than this issue described
I wrote it up as "coverage collapses on long files", which is true but understates it. Measuring the old implementation against realistic speech shapes, survival came down to whether a player's speech happened to align with the probe positions:
Eighty seconds of speech deleted while six seconds survives. Not monotonic in how much someone spoke, and not a threshold anyone chose — an alignment artefact between two evenly-spaced sequences. Sol_Invictus surviving the real session was luck.
I nearly shipped the same broken test
My first replacement used 40 utterances of 4 seconds at real session length. It looks like the harder case, and it passes — but so does the old code, because evenly-spaced speech resonates with evenly-spaced probes. A test built from it would have passed before and after the fix and proved nothing, which is exactly what the test this issue criticises did.
I caught it by rebuilding the old implementation and running the candidate fixtures against it, rather than assuming a longer file was automatically a better test. The shapes that ship (20 × 4s, 8 × 1s, one word late in an hour) are ones measured to fail before the fix.
Criteria
The two GM-visibility criteria are not done and I am not closing this. They are the same problem as #424 — the pipeline knowing something the GM cannot see — and better solved there than bolted on here. The data-loss half, which is what made this a merge blocker, is fixed.
1,227 passing before, 1,233 after.
Verified against acceptance criteria. Two were met, two were not — the second pair is fixed in
540b0a2.Criteria
Detection does not degrade with file length —
has_audible_speechscans sequentially and stops at the first audible sample. No probing, no spacing that grows with the file. It is also faster in the case that matters: anyone who speaks at all ends the loop when they do, and only a genuinely silent track is read to the end.A test at realistic session length with speech scattered as many short utterances — three of them, on 4,639-second files:
test_a_quiet_player_survives_a_full_length_session,test_speech_scattered_as_single_seconds_survives,test_a_single_word_late_in_a_long_session_is_still_found.And they do fail against the old implementation. I did not take the docstrings' word for it — I restored the pre-fix sampler (32 windows, reads capped at 16,000 frames) into
audio_serviceand re-ran the file: those three fail, the other twelve pass. Given this issue exists becausetest_one_brief_utterance_is_enough_to_keep_a_trackcited a real incident and then built a 60-second file, that check seemed worth doing rather than assuming.→ [x] A dropped track reaches the GM through the same
uncaptured_member_ids/ attendance surface — was not met. Fixed in540b0a2.→ [x] #348's fourth criterion holds for backend-dropped tracks too — was not met. Same fix.
What was missing
drop_silent_tracksreturned its silent list into alogger.infoand nothing else (reminder_tasks.py:2207-2213).uncaptured_member_idswas built solely frompresence.json(reminder_tasks.py:2454-2463), which carries only the members the bot declined to upload.So the backend guard was half a guard. It stopped Whisper inventing speech under a quiet person's name — #348's concern — and then let that person disappear from the transcript with a worker log as the only trace, which is this issue's concern. The GM gets a summary written without them and a pipeline reporting success.
The gap sat exactly where the guard is most needed:
drop_silent_tracksexists for recordings the current bot did not produce — an older session, a hand-placed directory, a bot a version behind — and those are precisely the cases with no bot-side report to fall back on.Track.owner_idis already the Discord user id, so the fix is a merge, not new plumbing. Merged rather than assigned, mirroring the bot's own merge at recording.py:975: the two paths reach the same state differently and either can find someone the other did not.Extracted as
merge_uncaptured_membersrather than left inline —process_audiohas no test harness, and building one to assert four lines would have been disproportionate.Note for #348
Its first criterion — "
_peak_amplitudescans the full file, not the first 0.5 s" — is unmet as worded: it still samples five 64 KB windows (recording.py:374-400). But it is no longer the drop decision. That now usesspeech_bytes_written(), an exact running counter with no sampling blind spot, and_peak_amplitudesurvives only as a log diagnostic — its docstring says so. The letter is unmet; the intent is superseded by something stronger. Flagging rather than quietly ticking it.1,296 → 1,300 passing, lint clean. Closing.