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

New issue

Closed

opened 2026-07-14 19:46:21 +00:00 by claude-bot · 2 comments

claude-bot commented

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

Contributor

Context

/record start in bot/questboard_bot/cogs/recording.py uses a per-guild reservation set self._starting (initialized at line 189) to stop a second concurrent start from racing through the async setup. The guard at line 212 rejects any start while guild.id is in self._active or self._starting.

Current behavior

The slash-command path adds the guild to the reservation set at line 222 — but the try: whose finally discards it (lines 317–318) only begins at line 246. Two early returns sit in the gap:

invalid session-UUID format → return at line 233
invoking user not in a voice channel → return at line 242

Either return leaves guild.id stranded in self._starting, so every subsequent /record start in that guild fails the line-212 guard with "A recording is already in progress for this server" until the bot process restarts. One typo'd UUID permanently locks out recording for the guild.

The API-triggered path is already correct: start_from_api adds at line 342 immediately followed by try: at 343 with finally: self._starting.discard(...) at 416–417, and no early exits in between.

Fix / Spec

In record_start, move self._starting.add(guild.id) from line 222 down to immediately before the existing try: at line 246 (mirroring the start_from_api pattern). This is safe: the code between the guard (line 212) and line 246 — uuid.UUID(...) parse, isinstance/member.voice checks — is fully synchronous, so no other coroutine can interleave between guard check and reservation.
Keep the finally: self._starting.discard(guild.id) at 317–318 unchanged.
Add a regression test in bot/tests/test_recording.py: invoke record_start with an invalid session ID (and separately with the member not in voice), assert the guild is not left in cog._starting afterward / that a second start attempt is not rejected by the guard.

Acceptance criteria

After a /record start with an invalid UUID, a subsequent valid /record start for the same guild proceeds past the guard.
Same after an attempt where the user is not in a voice channel.
Concurrent double-start protection still holds (reservation is set before any await on the success path).
Regression test added to bot/tests/test_recording.py.

References

bot/questboard_bot/cogs/recording.py:189 (_starting set), :212 (guard), :222 (leaked add), :233 / :242 (early returns), :246 (try:), :317-318 (finally discard)
Correct pattern: bot/questboard_bot/cogs/recording.py:339-343, :416-417 (start_from_api)

Filed from the July 2026 full-project review.

## Context `/record start` in `bot/questboard_bot/cogs/recording.py` uses a per-guild reservation set `self._starting` (initialized at line 189) to stop a second concurrent start from racing through the async setup. The guard at line 212 rejects any start while `guild.id` is in `self._active` **or** `self._starting`. ## Current behavior The slash-command path adds the guild to the reservation set at line 222 — but the `try:` whose `finally` discards it (lines 317–318) only begins at line 246. Two early returns sit in the gap: - invalid session-UUID format → `return` at line 233 - invoking user not in a voice channel → `return` at line 242 Either return leaves `guild.id` stranded in `self._starting`, so **every** subsequent `/record start` in that guild fails the line-212 guard with "A recording is already in progress for this server" until the bot process restarts. One typo'd UUID permanently locks out recording for the guild. The API-triggered path is already correct: `start_from_api` adds at line 342 immediately followed by `try:` at 343 with `finally: self._starting.discard(...)` at 416–417, and no early exits in between. ## Fix / Spec 1. In `record_start`, move `self._starting.add(guild.id)` from line 222 down to immediately before the existing `try:` at line 246 (mirroring the `start_from_api` pattern). This is safe: the code between the guard (line 212) and line 246 — `uuid.UUID(...)` parse, `isinstance`/`member.voice` checks — is fully synchronous, so no other coroutine can interleave between guard check and reservation. 2. Keep the `finally: self._starting.discard(guild.id)` at 317–318 unchanged. 3. Add a regression test in `bot/tests/test_recording.py`: invoke `record_start` with an invalid session ID (and separately with the member not in voice), assert the guild is **not** left in `cog._starting` afterward / that a second start attempt is not rejected by the guard. ## Acceptance criteria - After a `/record start` with an invalid UUID, a subsequent valid `/record start` for the same guild proceeds past the guard. - Same after an attempt where the user is not in a voice channel. - Concurrent double-start protection still holds (reservation is set before any `await` on the success path). - Regression test added to `bot/tests/test_recording.py`. ## References - `bot/questboard_bot/cogs/recording.py:189` (`_starting` set), `:212` (guard), `:222` (leaked add), `:233` / `:242` (early returns), `:246` (`try:`), `:317-318` (`finally` discard) - Correct pattern: `bot/questboard_bot/cogs/recording.py:339-343`, `:416-417` (`start_from_api`) _Filed from the July 2026 full-project review._

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

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

claude-bot added the

bot

bug

recording

labels

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

claude-bot commented

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

Author

Contributor

Picking this up as part of a v3.3.0 push. Landing on branch hardening/bot together with #85, #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 #85, #94, and #111 (grouped by component to keep the diffs reviewable).

claude-bot referenced this issue

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

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

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:09 +00:00

Author

Contributor

Fixed on main (commit 42664a3, merged via cd315d6). self._starting.add(guild.id) moved down to immediately before the try: in record_start, past the synchronous UUID-parse and voice-channel checks, mirroring the already-correct start_from_api pattern. Regression tests in bot/tests/test_recording.py cover both early-return paths (invalid UUID; user not in voice) — the guild is no longer stranded in _starting, and a subsequent valid start proceeds. Bot suite green (158 passed).

Fixed on `main` (commit `42664a3`, merged via `cd315d6`). `self._starting.add(guild.id)` moved down to immediately before the `try:` in `record_start`, past the synchronous UUID-parse and voice-channel checks, mirroring the already-correct `start_from_api` pattern. Regression tests in `bot/tests/test_recording.py` cover both early-return paths (invalid UUID; user not in voice) — the guild is no longer stranded in `_starting`, and a subsequent valid start proceeds. Bot suite green (158 passed).

claude-bot closed this issue

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