[Bot] Stop the startup sweep from deleting recordings it told the user were saved #399
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).
The bot can tell a GM their recording is safe and then permanently delete it on its own next restart. Three separate paths leave a real, non-empty recording without the marker file the bot's startup cleanup relies on to know a directory is "owned" by the backend — and once 60 minutes pass, the sweep treats every unmarked directory as an abandoned scratch file and wipes it with
shutil.rmtree, with no distinction between "genuinely never recorded" and "recorded but not yet handed off."Evidence
bot/questboard_bot/main.py:361-367— the startup sweep iterates every session subdirectory in the audio temp dir; only a directory containingHANDOFF_MARKERis skipped (:364-365, "regardless of age"); anything else older thanmin_age_minutes(default 60,_DEFAULT_MIN_ORPHAN_AGE_MINUTESat:309) isshutil.rmtree'd at:367.bot/questboard_bot/cogs/recording.py:738-753— the marker is written (:753) only after a successfulPOST /api/bot/sessions/{id}/audio. If that POST raises (backend unreachable, network blip), theexceptblock at:745-753logs the error and tells the channel: "⚠️ The recording was saved but could not be submitted to Quest Board… an admin can retry processing from the console" — but no marker is written and nothing retries automatically. The next bot restart ≥60 minutes later deletes the whole directory the message just promised was safe.bot/questboard_bot/main.py:355-358andbot/questboard_bot/cogs/recording.py:127-134— a bot crash during recording leaves per-speaker raw WAVs ({session_id}_{user_id}_raw.wav) flushed to disk (crash-safe by design — they're opened inwbmode and written incrementally). The sweep's own docstring (main.py:320-322) asserts these directories have "nothing to preserve" and deletes them unconditionally on the next restart via the*_raw.wavglob (:355) — destroying hours of already-captured speech that nothing ever attempted to finalise or resume.recording.py:745-753) leaves a backend-owned directory (the DB row already points at it, Celery may already be reading it) unmarked, so it too is swept on a later restart if the bot goes down again in that narrow window.Failure scenario
A GM finishes a 4-hour session at the same moment a scheduled backend maintenance window starts. The bot's upload POST fails, the GM sees "saved but could not be submitted, an admin can retry," and moves on trusting that message. The bot gets redeployed for an unrelated fix two hours later. On startup, the sweep finds the directory unmarked and older than 60 minutes, and deletes it. The recording — the entire session — is gone, and no one finds out until someone tries to retry it from the console.
Proposed fix
Persist a manifest per recording at creation time (session dir path +
uploaded: bool) instead of relying purely on a post-hoc marker file, and have the bot retry the backend handoff on its own startup before ever considering sweeping unmarked directories — turn "sweep on age" into "sweep only after a bounded number of failed retry attempts, and only for directories the manifest says were never even attempted." For the mid-recording crash case, finalise (don't delete) raw WAVs found on restart — convert whatever was captured and attempt the normal handoff rather than discarding it. As a structural improvement, write theHANDOFF_MARKER(or the manifest'suploaded=true) before the POST, and let the backend's ack own transitioning ownership, so a crash right after the POST is indistinguishable from before it.Acceptance criteria
Criterion 3 — the last one open — is implemented in PR #470.
What it does. On its next start the bot converts the raw per-speaker tracks a mid-session crash left behind and hands them to the backend, so the session is transcribed and appears like any other. The earlier pass on this issue stopped the sweep deleting that audio and logged
needs finalising by hand; on a self-hosted bot at 2am there are no hands, so an interrupted recording sat on the volume indefinitely.Why this is safe to automate. Placement in the raw tracks is absolute — each is silence-padded from the session's t0 before every append — so the tracks are already mutually aligned and the session length is exactly
max(file size) / BYTES_PER_SECOND. Nothing is reconstructed or estimated; the timings and attribution are the recorded ones. That property is a hard dependency and is now documented inCLAUDE.md, because per-speaker private clocks are the whole of #320 and breaking it again would silently make recovery produce a scrambled transcript rather than fail.Two decisions worth recording:
Backend lookup for names, resolved at
on_readyrather thansetup_hook. The raw filenames carry only the session and user ids. Guild nickname is preferred over the global Discord name — a player who renamed themselves to their character keeps that identity (#344), and the sessions that already went wrong shouldn't also read worst. Nicknames need a warm member cache, which does not exist insetup_hook. It also runs as a background task so re-encoding hours of audio doesn't sit in front of the bot answering commands, and is guarded to fire once per process sinceon_readyrepeats on every gateway reconnect.GET /api/bot/sessions/{id}/summarynow also returnsnotification_channel_id, not justguild_id. This wasn't in the original plan and turned out to be load-bearing:process_audiopasses the channel straight through tosession_summarised, and the bot drops an event with an empty channel_id. Without it a recovered session would transcribe, summarise, and then say nothing in Discord — which from the GM's chair is indistinguishable from the recovery never having run. Both fields are additive and optional, so no contract bump.One hazard found while building it, worth noting because it is not obvious:
_retry_pending_handoffsruns first, insetup_hook, and can legitimately have just succeeded on a directory whose raw tracks are still present (a crash landing between the conversion and the ack). Re-submitting would queueprocess_audioa second time — and #397's 409 guard would not catch it, because the first run has not written a transcript yet. Recovery now skips any directory already carrying the handoff marker.Every other path errs towards leaving the audio alone. A raw track is unlinked only after the handoff is acked. A failed session lookup, or a campaign with no Discord server, converts nothing at all rather than sending a request guaranteed to be rejected. #397's refusal keeps the take and stops retrying it. Sub-threshold tracks are reported as present rather than as speakers, so #425 stays fixed.
15 new bot tests and 2 backend tests, each run against unfixed code first and confirmed to fail — including raws-deleted-before-ack, global-name-before-nickname, blank-guild-proceeds, duration-reported-as-zero, and the double-submit guard above.
Leaving this open until #470 merges. #416 can close at the same time — its remaining dependency was this.
Merged in #470 (
26d4328). All three criteria are now met, so closing.For the record, the three landed in two passes:
Criteria 1 and 2 — the startup sweep no longer deletes a directory holding audio, marked or not, and a converted-but-unsent recording is re-posted on the next start (
_retry_pending_handoffs). The test that asserted the opposite — it wrote a non-empty123.wavand required the sweep to delete it — was the defect encoded as the specification, and was rewritten to cover the empty case the sweep is actually for.Criterion 3 (#470) — a crash during capture leaves only raw per-speaker tracks, with no directory, manifest or marker to find, because all three are written at stop. The bot now converts and hands those off on its next start, resolving speaker names from the guild so nicknames survive (#344).
The property that makes the third one safe to automate at all: placement in the raw tracks is absolute — each is silence-padded from the session's t0 before every append — so the tracks are already mutually aligned and the length is exactly
max(file size) / BYTES_PER_SECOND. Nothing is estimated. This is now written intoCLAUDE.md, because it is a silent dependency: per-speaker private clocks are the whole of #320, and reintroducing them would make recovery emit a scrambled transcript rather than fail visibly.Nothing on any path deletes audio. A raw track is unlinked only after the handoff is acked; a failed lookup or a campaign with no Discord server converts nothing at all rather than sending a request certain to be rejected; #397's refusal keeps the take and stops retrying it.