[Bot] Fix /record start's broken session identity, unresponsive stop, and silent data loss on restart #393
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?
Impact: HIGH
Found in the August 2026 session lifecycle review (#319).
What the user experiences
The bot's flagship command is broken in several compounding ways.
/record startrequires a session UUID that no other bot command can supply —/historyfilters to completed sessions only and/nextdeliberately omits the ID — so a GM has to leave Discord and find the ID in the session page's URL just to start the command the product advertises as central. Every recording started via the slash command posts "🔴 Recording started — Unknown Session" and every later "⏳ Recording uploaded" repeats it, because the metadata block that setssession_titleon this path never actually assigns it. A well-formed but wrong UUID isn't validated at start — the 404 only surfaces at stop, hours later, after a whole session has been captured against a nonexistent session./record stopnever fulfils the deferred interaction, so the invoking GM's "Quest Board is thinking…" indicator hangs and errors out while the actual confirmation arrives as a separate message. And if the bot process restarts mid-recording, the active-recording state — held only in an in-process dict — is gone: the channel still shows "Recording started", no stop message ever arrives,/record stopreplies "No recording is currently active," and the un-handed-off audio directory is deleted after 60 minutes with no notice to anyone.Evidence
bot/questboard_bot/cogs/sessions.py:87-98(/nextomits session ID) androuters/bot.py:669-676(/historyfilters tocompletedonly) — together, no bot command can supply the UUID/record startrequires.bot/questboard_bot/cogs/recording.py:373-379— the metadata block assignscampaign_nameandgame_systembut neversession_title; compare the API path at:493-496, which does assign it.bot/tests/test_recording.py:263,291,320— tests construct_RecordingSessiondirectly and never exercise the slash-command metadata path, so this bug has no test coverage.bot/questboard_bot/cogs/recording.py:376-381— the metadata fetch failure is swallowed at start, so a bad UUID only 404s at stop (:759-760), after the session was fully captured.bot/questboard_bot/cogs/recording.py:574-575—record_stopdefers, then only everchannel.sends (:628), with nofollowup.send/edit_original_responseanywhere in the stop path.bot/questboard_bot/cogs/recording.py:277—_activeis an in-process dict with no persistence;bot/questboard_bot/main.py:361-368deletes the un-handed-off audio directory 60 minutes after a restart with no notice.bot/questboard_bot/cogs/recording.py:759-760,774-775— failure strings ("Check the bot logs — an admin can retry processing from the console") are written for a self-hoster, not a hosted customer, and the latter leaks a raw Python exception into a public channel.Why it matters for a hosted product
The command the product's own command picker advertises as central is unusable without leaving Discord to find a UUID, silently mislabels every recording it starts, and can lose an entire recorded session with no notice at all if the bot process restarts mid-session — which, for a self-hosted deploy, is not a rare event.
Proposed fix
Fix the
session_titleassignment on the slash-command metadata path (recording.py:373-379) to match the API path, and add a regression test exercising it. Validate the session UUID at/record startrather than deferring the 404 to stop. Fulfil the deferred interaction inrecord_stopinstead of leaving it hanging. Persist active-recording state (even a simple durable marker) so a restart can detect and surface an interrupted recording instead of silently discarding it. Rewrite the failure strings for a hosted customer rather than a self-hoster, and stop leaking raw exception text into a public channel. This is the audit's P27 and P28.Acceptance criteria
/record startrecording gets the real session title, not "Unknown Session", in both the start and upload-confirmation messages.session_title./record start, not only discovered at stop./record stopfulfils its deferred interaction instead of leaving "Quest Board is thinking…" hanging.Picking this up as v4.3.0 phase 5, lane B (#514). Since this was filed, #399 added crash recovery for the audio itself (raw tracks left by a crash are converted and handed off at startup), so the remaining work is identity, state and messages: an autocomplete on
/record start's session argument so nobody has to copy a UUID, validation at start rather than at stop, the real session title on the slash path, a fulfilled/record stopinteraction, the active recording persisted in Redis so a restart can tell the channel what happened and answer/record stophonestly, and failure strings written for a hosted customer with the exception kept in the log.Done in PR #521 (merged); ships with v4.3.0.
Read #399 first and left it entirely intact: the 60-minute deletion this issue describes is already gone (
_cleanup_audio_tempkeeps anything holding audio), andrecover_interrupted_recordings/_retry_pending_handoffsare unchanged, including their return shapes.Session identity.
/record start's argument now autocompletes against a new additiveGET /api/bot/guilds/{id}/sessions/recordable: confirmed sessions from the last 12 hours onwards plus anything in progress, because a GM reaches for the command after the start time. The slash path now setssession_titlefrom the fetched metadata (it assigned campaign and system and silently skipped the title, while the API path always set it), with a regression test on the slash path, which nothing had exercised. A definite 404 refuses the start; a timeout or 5xx still starts, since that is no evidence the session is wrong and capture cannot be redone.defer()moved ahead of the fetch (3 s ack window against a 30 s timeout).State across a restart. #399 keeps the audio; what died with the process was everything needed to say so. Each start now writes guild, channel, session, title and start time to Redis DB 0 under
qb:bot:recording:<guild_id>(7-day TTL), andannounce_interrupted_recordingsruns straight after #399's recovery so it can report which of three things happened: recovered and being transcribed, kept but not yet submitted (retried automatically), or nothing captured./record stopreads the same record and answers honestly instead of denying the session happened. All Redis access is best-effort: an outage costs the message, never the recording.Messages.
/record stopfulfils its deferred interaction as well as posting to the channel. Failure strings are rewritten for a hosted customer: no exception text in a public channel (it goes to the log), and "a GM can retry processing from the session page: " instead of pointing at a console. The invalid-ID message points at the picker rather than at copying a UUID out of a browser.No contract bump (all additive). Tests cover the state round-trip, all three announcement outcomes, the restart-aware stop, the slash-path title, 404 rejection, interaction fulfilment, autocomplete, and no exception leaks. An adjacent finding is filed as #520: a failure midway through converting raw tracks deletes the ones not yet converted.