[Bot] Stop the startup sweep from deleting recordings it told the user were saved

claude-bot commented

2026-08-25 20:44:39 +00:00

Collaborator

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 containing HANDOFF_MARKER is skipped (:364-365, "regardless of age"); anything else older than min_age_minutes (default 60, _DEFAULT_MIN_ORPHAN_AGE_MINUTES at :309) is shutil.rmtree'd at :367.
bot/questboard_bot/cogs/recording.py:738-753 — the marker is written (:753) only after a successful POST /api/bot/sessions/{id}/audio. If that POST raises (backend unreachable, network blip), the except block at :745-753 logs 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-358 and bot/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 in wb mode 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.wav glob (:355) — destroying hours of already-captured speech that nothing ever attempted to finalise or resume.
A narrower third case: a crash between the successful POST ack and the marker write (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 the HANDOFF_MARKER (or the manifest's uploaded=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

A recording whose backend handoff fails is retried automatically on the next bot startup (and ideally periodically) before it becomes eligible for deletion.
The startup sweep no longer deletes a directory solely because it is unmarked and old — it must also have exhausted retry attempts or be provably never-recorded (empty/zero-byte).
Raw per-speaker WAVs found on restart (mid-recording crash case) are finalised and handed off rather than deleted.
The "recording was saved but could not be submitted" message is only shown when that is actually still true at read time — not a stale promise.
Regression test: simulate a POST failure followed by a bot restart past the age threshold; assert the recording is retried/preserved, not deleted.

**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 containing `HANDOFF_MARKER` is skipped (`:364-365`, "regardless of age"); anything else older than `min_age_minutes` (default 60, `_DEFAULT_MIN_ORPHAN_AGE_MINUTES` at `:309`) is `shutil.rmtree`'d at `:367`. - `bot/questboard_bot/cogs/recording.py:738-753` — the marker is written (`:753`) only *after* a successful `POST /api/bot/sessions/{id}/audio`. If that POST raises (backend unreachable, network blip), the `except` block at `:745-753` logs 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-358` and `bot/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 in `wb` mode 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.wav` glob (`:355`) — destroying hours of already-captured speech that nothing ever attempted to finalise or resume. - A narrower third case: a crash between the successful POST ack and the marker write (`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 the `HANDOFF_MARKER` (or the manifest's `uploaded=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** - [ ] A recording whose backend handoff fails is retried automatically on the next bot startup (and ideally periodically) before it becomes eligible for deletion. - [ ] The startup sweep no longer deletes a directory solely because it is unmarked and old — it must also have exhausted retry attempts or be provably never-recorded (empty/zero-byte). - [ ] Raw per-speaker WAVs found on restart (mid-recording crash case) are finalised and handed off rather than deleted. - [ ] The "recording was saved but could not be submitted" message is only shown when that is actually still true at read time — not a stale promise. - [ ] Regression test: simulate a POST failure followed by a bot restart past the age threshold; assert the recording is retried/preserved, not deleted.

claude-bot added this to the v4.1.0 - Data Durability & Recovery milestone

2026-08-25 20:44:39 +00:00

claude-bot added the

bot

bug

labels

2026-08-25 20:44:39 +00:00