[Recording] Guard against re-submitting audio for a session that already has GM-edited content #397
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
Found in the August 2026 session lifecycle review (#319).
Any second recording of a session silently destroys everything a GM built from the first one. When the bot POSTs a session's audio a second time — the natural "record part 2 after a bathroom break" or "co-GM records the epilogue" workflow — the backend has no idea a first recording was ever processed. It resets the session straight back to
processing, re-queues transcription, and the resulting run overwrites the GM's transcript and summary edits and deletes every highlight row for the session (including ones a GM manually added or approved) before reinserting a fresh set. Worse, because the bot writes each speaker's audio to a fixed per-session path, the second recording's WAV files land directly on top of the first recording's WAV files on disk — so there is no raw audio left to recover from even if someone wanted to reprocess the original.Evidence
webapp/backend/app/routers/bot.py:869-931(bot_upload_audio) — no check of the session's currentaudio_processing_status,content_approved_at, or whether a transcript already exists; it accepts a newsession_dirunconditionally.webapp/backend/app/routers/bot.py:903— setssession.audio_processing_status = AudioProcessingStatus.processingon every call, even when the session was alreadyapproved/readywith GM edits.webapp/backend/app/tasks/reminder_tasks.py:2184-2185—process_audiounconditionally overwritessession.transcriptandsession.summarywith the new run's output.webapp/backend/app/tasks/reminder_tasks.py:2235-2244— highlights are deleted bysession_idalone and reinserted withapproved=False; the delete has no manual/approved-row filter, so GM-approved and manually-added highlight rows are destroyed along with the machine-generated ones.bot/questboard_bot/cogs/recording.py:708(per-userout_wav = session_dir / f"{user_id_str}.wav") and:730(speakers.jsonrewrite) — both are keyed only by session ID and Discord user ID, so a second recording of the same session overwrites the first recording's files in place.webapp/backend/app/tasks/reminder_tasks.py:2826-2860, 3001(erase_member_recordings(regenerate_summaries=True)) — reaches the sameprocess_audiore-run path for every scrubbed session, clobbering GM-edited summaries/highlights as a side effect of an unrelated privacy operation.Failure scenario
A GM runs a 4-hour session and stops recording for a bathroom break, then starts a new
/recordfor part 2. Or: the GM spends an hour after the session fixing transcription errors and hand-curating highlights, then a co-GM later hits "record" against the same session ID for a recap epilogue. In both cases, the secondPOST /api/bot/sessions/{id}/audioclobbers the first recording's transcript, summary, and highlights, and the first recording's audio is gone from disk — unrecoverable, because no versioning of transcript/summary exists anywhere (unlike lore entries, which do haveLoreEntryVersion).Proposed fix
Add a state guard to
bot_upload_audio— refuse (409) a re-submission when the session already has a non-nulltranscriptorcontent_approved_atset, unless the caller passes an explicit force flag (surfaced in the bot as an admin-only override, not the default record-stop flow). Separately, giveprocess_audio's highlight reinsert an escape hatch: skip deleting rows where a GM-editable flag (e.g.edited_by_id IS NOT NULLorapproved = true) is set, only replacing machine-generated, unreviewed rows. Longer term, apply theLoreEntryVersionpattern toSession.transcript/summary(a version snapshot written before every overwrite, with a restore endpoint) so that even a forced re-run is recoverable. This state guard must land before any Celeryacks_late/retry work (tracked separately) — otherwise task retries after a worker crash will hit the exact same unguarded overwrite path.Acceptance criteria
POST /api/bot/sessions/{id}/audioreturns 409 when the target session already has a transcript orcontent_approved_atset, unless an explicit force flag is supplied.process_audio's highlight reinsert never deletes a highlight row that has been manually edited or GM-approved.erase_member_recordings(regenerate_summaries=True)no longer discards GM-edited summary/highlight content for sessions it touches (either it skips them or the same guard applies).Moved from v4.1.0 to v4.0.0.
Not because the defect changed, but because it sits directly in the path of the accuracy work. Developing the summarisation re-architecture means re-running
process_audioagainst real sessions repeatedly to compare output — and every one of those re-runs currently destroys the GM-edited transcript, summary and curated highlights, and can overwrite the source WAVs unrecoverably.So this needs to land before the v4.0.0 iteration loop starts in earnest — specifically before the chunking/beat work (#331–#334), where comparing successive summarisation runs against the same real session is the whole point.
Note the constraint this creates in the other direction: #398 (
acks_late+ stuck-state watchdog) still cannot land until this does, wherever #398 ends up. Enabling task redelivery before re-submission is guarded means a worker loss clobbers GM edits rather than recovering them.Verified against the acceptance criteria before closing. Not closing — one criterion is genuinely unmet, and it is the same class of data loss this issue was filed for, reached through a second entry point.
Met, with evidence
app/routers/bot.py:924-932, keyed ontranscript_updated_at. Tested:tests/test_bot.py:611-661covers both the 409 and theforce=trueoverride.bot/questboard_bot/cogs/recording.py:1004-1019catchesAudioAlreadySubmittedError, names the take directory, says nothing was lost.recording.py:857-880; first take keeps the bare session-id directory, later takes get a-take{N}suffix.app/tasks/reminder_tasks.py:2344-2349, delete filtered onapproved.is_(False), edited_by_id.is_(None). Code inspection only; no end-to-end test drivesprocess_audiowith a pre-existing approved highlight.Not met
Highlights are safe (the filtered delete above). The summary is not.
The protection was added at the router layer, but there are four call sites:
app/routers/bot.py:955app/routers/sessions.py:945app/routers/admin.py:224app/tasks/reminder_tasks.py:3262run_member_erasurecallsprocess_audio.delay(...)directly, andprocess_audiounconditionally writes:with no check on
content_approved_atortranscript_updated_at. So a GM who has edited a summary, then erases a different member's recordings withregenerate_summaries=True, silently loses their edit. Erasing one person's audio destroying another person's written work is exactly the shape of loss this issue exists to prevent.No test covers it:
tests/test_member_erasure.py:455only exercisesregenerate_summaries: False.The fix is a layer, not a patch
Guarding one router leaves the invariant enforced by discipline at four call sites, which is how this reached a second entry point in the first place. The check belongs inside
process_audio, with an explicit parameter —force=True/allow_overwrite=True— that each caller must pass deliberately. Retry and admin reprocess then say so out loud; erasure does not, and gets the protection by default.That also makes the rule testable in one place rather than four.
Remaining work
process_audio, with an explicit force parametersessions.py) and admin reprocess (admin.py) pass it; erasure regenerate does noterase_member_recordings(regenerate_summaries=True)— and fails against the current code before it passes against the fixFound by an acceptance-criteria pass rather than by a test or an incident. Related merge-blocker from the same sweep: #425.
The remaining criterion is fixed in
633f841.The guard now sits with the write it protects rather than in front of one of four doors.
process_audiorefuses whentranscript_updated_atis set unless the caller passesreplace_existing, defaulting to False — so the safe behaviour is what you get by omission, and replacing is the thing that has to be spelled out:routers/bot.pydata.forcerouters/sessions.pyTruerouters/admin.pyTruetasks/reminder_tasks.pyGuarding one router left the invariant enforced by discipline across four call sites, which is exactly how it reached a second entry point in the first place. Now the dangerous case is the one that requires an explicit argument.
The bot endpoint keeps its 409 rather than relying solely on the task: a caller deserves an error, not a task that vanishes silently.
Tests: erasure's call site queues five positional arguments and never asks to replace; and the signature keeps
replace_existingdefaulting to False, so a future caller cannot acquire overwrite behaviour by accident.One criterion I did not close out: the highlight filtered-delete is still inspection-only — no end-to-end test drives
process_audiowith a pre-existing approved highlight. The filter itself (approved.is_(False), edited_by_id.is_(None)) is correct, but it is the kind of thing that regresses silently. Worth a test when someone is next in that code; not blocking, since the summary path was the actual data loss.1,227 passing before, 1,233 after with #425. Closing.