[Hardening] Stop on_ready audio_temp cleanup from deleting recordings queued for processing

claude-bot commented

2026-07-14 19:46:43 +00:00

Contributor

Context

The bot wipes the shared audio_temp volume on startup to clear orphans from a crashed run: on_ready (bot/questboard_bot/main.py:61-63) calls _cleanup_audio_temp (defined at main.py:229), whose loop at main.py:257-260 shutil.rmtrees every subdirectory of audio_temp.

The problem: on_ready fires on every gateway reconnect/resume, not just at process start. Discord reconnects are routine (network blips, Discord-side resumes).

Current behavior

Two races, both destroying live data:

Mid-recording: the processing pipeline _process (bot/questboard_bot/cogs/recording.py:523) writes per-speaker WAV files and speakers.json into a per-session directory on the volume. A gateway reconnect during a recording/processing deletes another (or the same) session's directory mid-write.
Post-upload window (worse): after the bot calls post_audio_tracks (recording.py:595), the backend endpoint (webapp/backend/app/routers/bot.py:698-705) only queues a Celery process_audio task that reads the directory later. A reconnect between upload-ack and the worker actually reading the files silently destroys the recording; the Celery task then fails on a missing directory. Hours of session audio are unrecoverable.

Fix / Spec

Move the _cleanup_audio_temp(...) call out of on_ready (main.py:63) into setup_hook (main.py:55), which runs exactly once per process — or guard it with a "ran once" flag. Reconnects must not trigger cleanup.
Age-gate deletion inside _cleanup_audio_temp: only remove subdirectories whose mtime is older than a threshold (e.g. 60 minutes). A dir younger than the threshold at process start could belong to an upload the backend hasn't consumed yet (worker backlog across a bot restart).
Add a comment noting the long-term ownership plan: the backend/Celery task should delete the session dir after successful processing, so the bot-side sweep is only a stale-orphan fallback. (Verify whether process_audio already deletes the dir on success; if it does, say so in the comment.)
Update/add a test in bot/tests/ covering: a fresh dir survives cleanup, an old dir is removed.

Acceptance criteria

A second on_ready (simulated reconnect) does not remove any session directory that is being written or was just uploaded.
Startup cleanup still removes genuinely stale directories (older than the threshold).
A directory created moments before a bot restart (queued-but-unprocessed upload) survives the restart sweep.
Tests cover the age gate.

References

bot/questboard_bot/main.py:55 (setup_hook), :61-63 (on_ready → cleanup call), :229 (_cleanup_audio_temp), :257-260 (unconditional rmtree loop)
bot/questboard_bot/cogs/recording.py:523 (_process writes tracks), :595 (post_audio_tracks)
webapp/backend/app/routers/bot.py:698-705 (upload endpoint queues process_audio — deferred read)

Filed from the July 2026 full-project review.

## Context The bot wipes the shared `audio_temp` volume on startup to clear orphans from a crashed run: `on_ready` (`bot/questboard_bot/main.py:61-63`) calls `_cleanup_audio_temp` (defined at `main.py:229`), whose loop at `main.py:257-260` `shutil.rmtree`s **every** subdirectory of `audio_temp`. The problem: `on_ready` fires on every gateway reconnect/resume, not just at process start. Discord reconnects are routine (network blips, Discord-side resumes). ## Current behavior Two races, both destroying live data: 1. **Mid-recording**: the processing pipeline `_process` (`bot/questboard_bot/cogs/recording.py:523`) writes per-speaker WAV files and `speakers.json` into a per-session directory on the volume. A gateway reconnect during a recording/processing deletes another (or the same) session's directory mid-write. 2. **Post-upload window (worse)**: after the bot calls `post_audio_tracks` (`recording.py:595`), the backend endpoint (`webapp/backend/app/routers/bot.py:698-705`) only **queues** a Celery `process_audio` task that reads the directory later. A reconnect between upload-ack and the worker actually reading the files silently destroys the recording; the Celery task then fails on a missing directory. Hours of session audio are unrecoverable. ## Fix / Spec 1. Move the `_cleanup_audio_temp(...)` call out of `on_ready` (`main.py:63`) into `setup_hook` (`main.py:55`), which runs exactly once per process — or guard it with a "ran once" flag. Reconnects must not trigger cleanup. 2. Age-gate deletion inside `_cleanup_audio_temp`: only remove subdirectories whose mtime is older than a threshold (e.g. 60 minutes). A dir younger than the threshold at process start could belong to an upload the backend hasn't consumed yet (worker backlog across a bot restart). 3. Add a comment noting the long-term ownership plan: the backend/Celery task should delete the session dir after successful processing, so the bot-side sweep is only a stale-orphan fallback. (Verify whether `process_audio` already deletes the dir on success; if it does, say so in the comment.) 4. Update/add a test in `bot/tests/` covering: a fresh dir survives cleanup, an old dir is removed. ## Acceptance criteria - A second `on_ready` (simulated reconnect) does not remove any session directory that is being written or was just uploaded. - Startup cleanup still removes genuinely stale directories (older than the threshold). - A directory created moments before a bot restart (queued-but-unprocessed upload) survives the restart sweep. - Tests cover the age gate. ## References - `bot/questboard_bot/main.py:55` (`setup_hook`), `:61-63` (`on_ready` → cleanup call), `:229` (`_cleanup_audio_temp`), `:257-260` (unconditional `rmtree` loop) - `bot/questboard_bot/cogs/recording.py:523` (`_process` writes tracks), `:595` (`post_audio_tracks`) - `webapp/backend/app/routers/bot.py:698-705` (upload endpoint queues `process_audio` — deferred read) _Filed from the July 2026 full-project review._

claude-bot added the

bot

bug

recording

labels

2026-07-14 19:46:53 +00:00

claude-bot added this to the v3.3.0 - Hardening: Bugs & Security milestone

2026-07-14 19:46:54 +00:00

claude-bot referenced this issue

2026-07-14 20:26:30 +00:00

[Hardening] Fix /record start guard leak that permanently locks out recording for a guild #83

claude-bot commented

2026-07-14 20:26:31 +00:00

Author

Contributor

Picking this up as part of a v3.3.0 push. Landing on branch hardening/bot together with #83, #94, and #111 (grouped by component to keep the diffs reviewable).

Picking this up as part of a v3.3.0 push. Landing on branch `hardening/bot` together with #83, #94, and #111 (grouped by component to keep the diffs reviewable).

claude-bot referenced this issue

2026-07-14 20:26:32 +00:00

[Hardening] Budget Discord embed total size (6000 chars) across all bot embeds #94

claude-bot referenced this issue

2026-07-14 20:26:33 +00:00

[Hardening] Bot HTTP server: default bind to 127.0.0.1 and cap /notify request size #111

claude-bot commented

2026-07-14 21:33:15 +00:00

Author

Contributor

Fixed on main (commits e62f5c8 + f52ae01, merged via cd315d6).

The cleanup call moved from on_ready (fires on every gateway reconnect) to setup_hook (once per process), and is now age-gated.

Scope note — the issue's premise was incomplete, and this went further than the original spec. While implementing, we verified the backend process_audio task's own docstring: "The session directory is NEVER deleted here — it stays until a GM approves it." Deletion only happens later in cleanup_trashed_audio for GM-trashed sessions after a retention window (default 7 days). So session directories legitimately live on the volume for days, and an age gate alone would still let a real bot restart (deploy/crash/reboot) delete backend-owned audio. Fixed properly with a handoff marker: the bot writes .handed-off into the session dir on a successful upload ack, and the startup sweep skips any marked dir regardless of age. Only unmarked dirs (a crashed recording that never reached the backend) are subject to the age gate. Tests cover marked-survives-when-old, unmarked-old-removed, unmarked-fresh-survives, marker-written-on-success/absent-on-failure. Bot suite green (158 passed).

Fixed on `main` (commits `e62f5c8` + `f52ae01`, merged via `cd315d6`). The cleanup call moved from `on_ready` (fires on every gateway reconnect) to `setup_hook` (once per process), and is now age-gated. **Scope note — the issue's premise was incomplete, and this went further than the original spec.** While implementing, we verified the backend `process_audio` task's own docstring: *"The session directory is NEVER deleted here — it stays until a GM approves it."* Deletion only happens later in `cleanup_trashed_audio` for GM-trashed sessions after a retention window (default 7 days). So session directories legitimately live on the volume for days, and an age gate alone would still let a real bot restart (deploy/crash/reboot) delete backend-owned audio. Fixed properly with a handoff marker: the bot writes `.handed-off` into the session dir on a successful upload ack, and the startup sweep skips any marked dir regardless of age. Only unmarked dirs (a crashed recording that never reached the backend) are subject to the age gate. Tests cover marked-survives-when-old, unmarked-old-removed, unmarked-fresh-survives, marker-written-on-success/absent-on-failure. Bot suite green (158 passed).

claude-bot closed this issue

2026-07-14 21:33:24 +00:00

claude-bot referenced this issue

2026-07-14 21:34:37 +00:00

[Hardening] Correct stale documentation: recording architecture, DB roles, stack versions #113