rbrooks/Quest-Board

Watch

Fork 0

[Bot] Fix /record start's broken session identity, unresponsive stop, and silent data loss on restart #393

New issue

Closed

opened 2026-08-25 20:42:29 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-25 20:42:29 +00:00

Collaborator

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 start requires a session UUID that no other bot command can supply — /history filters to completed sessions only and /next deliberately 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 sets session_title on 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 stop never 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 stop replies "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 (/next omits session ID) and routers/bot.py:669-676 (/history filters to completed only) — together, no bot command can supply the UUID /record start requires.
bot/questboard_bot/cogs/recording.py:373-379 — the metadata block assigns campaign_name and game_system but never session_title; compare the API path at :493-496, which does assign it.
bot/tests/test_recording.py:263,291,320 — tests construct _RecordingSession directly 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_stop defers, then only ever channel.sends (:628), with no followup.send/edit_original_response anywhere in the stop path.
bot/questboard_bot/cogs/recording.py:277 — _active is an in-process dict with no persistence; bot/questboard_bot/main.py:361-368 deletes 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_title assignment 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 start rather than deferring the 404 to stop. Fulfil the deferred interaction in record_stop instead 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

Every /record start recording gets the real session title, not "Unknown Session", in both the start and upload-confirmation messages.
A regression test covers the slash-command metadata path setting session_title.
A malformed or nonexistent session UUID is rejected at /record start, not only discovered at stop.
/record stop fulfils its deferred interaction instead of leaving "Quest Board is thinking…" hanging.
Active-recording state survives a bot restart, or the channel is notified that the recording was lost, rather than silently discarding the session with no message.
Failure messages in the recording flow do not leak raw exception text and are written for a hosted customer.

**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 start` requires a session UUID that no other bot command can supply — `/history` filters to completed sessions only and `/next` deliberately 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 sets `session_title` on 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 stop` never 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 stop` replies "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` (`/next` omits session ID) and `routers/bot.py:669-676` (`/history` filters to `completed` only) — together, no bot command can supply the UUID `/record start` requires. - `bot/questboard_bot/cogs/recording.py:373-379` — the metadata block assigns `campaign_name` and `game_system` but never `session_title`; compare the API path at `:493-496`, which does assign it. - `bot/tests/test_recording.py:263,291,320` — tests construct `_RecordingSession` directly 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_stop` defers, then only ever `channel.send`s (`:628`), with no `followup.send`/`edit_original_response` anywhere in the stop path. - `bot/questboard_bot/cogs/recording.py:277` — `_active` is an in-process dict with no persistence; `bot/questboard_bot/main.py:361-368` deletes 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_title` assignment 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 start` rather than deferring the 404 to stop. Fulfil the deferred interaction in `record_stop` instead 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 - [ ] Every `/record start` recording gets the real session title, not "Unknown Session", in both the start and upload-confirmation messages. - [ ] A regression test covers the slash-command metadata path setting `session_title`. - [ ] A malformed or nonexistent session UUID is rejected at `/record start`, not only discovered at stop. - [ ] `/record stop` fulfils its deferred interaction instead of leaving "Quest Board is thinking…" hanging. - [ ] Active-recording state survives a bot restart, or the channel is notified that the recording was lost, rather than silently discarding the session with no message. - [ ] Failure messages in the recording flow do not leak raw exception text and are written for a hosted customer.

claude-bot added this to the v4.3.0 - Product Surface: Navigation, Attention & Feedback milestone

2026-08-25 20:42:29 +00:00

claude-bot added the

bug

bot

labels

2026-08-25 20:42:29 +00:00

claude-bot referenced this issue

2026-09-06 00:34:01 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot referenced this issue

2026-09-06 02:03:19 +00:00

[Bot] Make the bot self-diagnosing: /help, guild-join welcome, and real error strings instead of a generic "try again" #373

claude-bot commented

2026-09-06 02:03:23 +00:00

Author

Collaborator

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 stop interaction, the active recording persisted in Redis so a restart can tell the channel what happened and answer /record stop honestly, and failure strings written for a hosted customer with the exception kept in the log.

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 stop` interaction, the active recording persisted in Redis so a restart can tell the channel what happened and answer `/record stop` honestly, and failure strings written for a hosted customer with the exception kept in the log.

claude-bot referenced this issue

2026-09-06 02:23:52 +00:00

v4.3.0 build order: Product Surface in eight phases #514

rbrooks referenced this issue from a commit

2026-09-06 03:03:26 +00:00

fix(bot/webapp): a session picker for /record start, and a restart that explains itself (#393)

claude-bot referenced this issue

2026-09-06 03:03:28 +00:00

[Bot] A failure midway through converting raw tracks deletes the tracks not yet converted #520

claude-bot referenced this issue from a pull request that will close it,

2026-09-06 03:03:53 +00:00

feat(bot): a self-diagnosing bot, a /record start that knows the session, and linking that names the way out (phase 5 lane B) #521

rbrooks referenced this issue from a commit

2026-09-06 03:10:13 +00:00

fix(bot/webapp): a session picker for /record start, and a restart that explains itself (#393)

claude-bot referenced this issue

2026-09-06 03:12:47 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot closed this issue

2026-09-06 03:39:44 +00:00

claude-bot commented

2026-09-06 03:40:30 +00:00

Author

Collaborator

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_temp keeps anything holding audio), and recover_interrupted_recordings / _retry_pending_handoffs are unchanged, including their return shapes.

Session identity. /record start's argument now autocompletes against a new additive GET /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 sets session_title from 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), and announce_interrupted_recordings runs 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 stop reads 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 stop fulfils 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.

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_temp` keeps anything holding audio), and `recover_interrupted_recordings` / `_retry_pending_handoffs` are unchanged, including their return shapes. **Session identity.** `/record start`'s argument now autocompletes against a new additive `GET /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 sets `session_title` from 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), and `announce_interrupted_recordings` runs 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 stop` reads 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 stop` fulfils 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: <link>" 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.

claude-bot referenced this issue

2026-09-06 04:22:13 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot referenced this issue

2026-09-06 09:30:26 +00:00

chore(release): v4.3.0 #533

claude-bot referenced this issue

2026-09-19 21:42:54 +00:00

feat(feedback-site): lane 8 — outside the app (#659) #711

claude-bot referenced this issue

2026-09-19 22:04:51 +00:00

[UX] Mock up every catalogued surface, for players and GMs, before building #659