Repository navigation
fix(bot): guard voice/session mutations with stop/suppress flags - #1998
Conversation
three confirmed bugs (#1949, #1948, #1957) share one root cause: the intentional-stop and replenish-suppression flags already exist but are checked inconsistently. recoverOrphanSession, the connection event's snapshot restore, and the emptyQueue autoplay refill could all run without checking them; /clear never set the suppression flag at all. wire the existing guards into all four gaps, and add a recovery-in-progress lock so checkAndRecover and the orphan monitor can't act on the same guild concurrently.
📝 WalkthroughWalkthroughThe change adds one-hour autoplay suppression after ChangesPlayback suppression and recovery coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClearCommand
participant SuppressionStore
participant LifecycleHandlers
participant TrackHandlers
participant WatchdogService
ClearCommand->>SuppressionStore: Suppress guild replenishment for 60 minutes
LifecycleHandlers->>SuppressionStore: Check suppression before empty-queue replenishment
LifecycleHandlers-->>LifecycleHandlers: Skip replenishment when suppressed or intentionally stopped
TrackHandlers->>SuppressionStore: Clear suppression for an explicit track
WatchdogService->>SuppressionStore: Check suppression before orphan recovery
WatchdogService-->>WatchdogService: Reject recovery when the guild lock is active
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces a replenish suppression mechanism to prevent autoplay from refilling the queue after certain actions. It adds a new replenishSuppressionStore service (used via setReplenishSuppressed/isReplenishSuppressed), wires the /clear command to suppress autoplay refill for ~35 seconds after clearing, and updates the emptyQueue lifecycle handler to skip replenishing when suppression or an intentional stop is active. The changes also add guards in the player lifecycle handlers to skip snapshot restoration when an intentional stop is set, and touch the music watchdog service around orphan session scanning and concurrent checkAndRecover handling. Accompanying spec files add tests covering these suppression, intentional-stop, and concurrency scenarios (referencing issues #1948, #1949, #1957).
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 76 functions depend on the 60 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees
Verification — 76 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 74 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
recoverOrphanSession only checked isIntentionalStop, not isReplenishSuppressed - same class of bug as #1957 via a different path. also bump /clear's suppression window from 35s to 30min: the old window expired long before a still-playing track naturally ends, so emptyQueue's guard would already be stale by the time it mattered.
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces a replenish-suppression mechanism for the music bot, adding a new replenishSuppressionStore service (referenced via setReplenishSuppressed/isReplenishSuppressed) that the /clear command now sets for a 30-minute window after clearing a queue. It updates the player lifecycle handlers so the emptyQueue autoplay-replenish path and the connection snapshot-restore path are skipped when an intentional stop or active replenish suppression is present, and adjusts when markIntentionalStop is called. The changes also extend the watchdog's orphan-session scan to skip guilds that have an intentional stop or active replenish suppression. Corresponding unit tests are added across the clear command, lifecycle handlers, and watchdog specs to cover these new skip conditions.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 76 functions depend on the 60 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees
Verification — 76 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 74 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
a fixed suppression timer can't correctly express until the current track ends: too short for tracks over the window (podcasts, mixes) reproduces #1957 again, too long silently blocks legitimate autoplay after the user queues something new post-clear. lift suppression in playerStart when a non-autoplay track starts, mirroring the existing web handlePlay convention; the clear.ts duration is now just a safety ceiling, per cubic review on pr 1998.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/bot/src/services/musicManagement/watchdog.spec.ts (2)
350-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass a short
timeoutMsto avoid a long pending timer.
markIntentionalStopschedules asetTimeoutoftimeoutMs + 10_000. This service uses the defaulttimeoutMs, so the test leaves a pending timer of about 35 seconds. The concurrency test at Line 544 already usesnew MusicWatchdogService({ timeoutMs: 100 }). Use the same pattern here to keep teardown fast.♻️ Proposed change
- const service = new MusicWatchdogService() + const service = new MusicWatchdogService({ timeoutMs: 100 }) service.markIntentionalStop('guild-stopped')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/watchdog.spec.ts` around lines 350 - 366, Update the test’s MusicWatchdogService construction in “scanOrphanSessions skips guild when intentional stop is set” to pass a short timeoutMs, matching the existing concurrency test pattern (100ms), so markIntentionalStop does not leave a long pending timer.
536-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that covers lock release on a failed recovery.
This test proves the lock blocks a concurrent call and that the first call completes. It does not prove the
finallyblock releases the lock whenplay()rejects. Add a case whereplayrejects, then assert that a followingcheckAndRecoveris not blocked. This case also documents the lock lifetime concern raised onwatchdog.tsLines 150-156.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/watchdog.spec.ts` around lines 536 - 567, Add a test alongside the concurrent checkAndRecover case using a rejecting play mock, await the failed recovery, then invoke checkAndRecover again for the same guild and verify it is not treated as recovery_already_in_progress. Assert the lock is released through the failure path and preserve the expected recovery failure result or error behavior exposed by checkAndRecover.packages/bot/src/handlers/player/trackHandlers.ts (1)
242-248: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the intended interaction with
handleQueueReplenishmenton the same event.Line 247 clears suppression, and Line 251 calls
handleQueueReplenishment, which reachesreplenishIfAutoplayand itsisReplenishSuppressedcheck. The clear therefore takes effect within the sameplayerStartcall, so an explicit track withrepeatMode === AUTOPLAYrefills the queue immediately after/clear. If you want the explicit track to play alone before autoplay resumes, clear the suppression afterhandleQueueReplenishment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/handlers/player/trackHandlers.ts` around lines 242 - 248, The suppression flag is cleared before handleQueueReplenishment in the playerStart flow, allowing an explicit track with repeatMode AUTOPLAY to trigger immediate replenishment. Move setReplenishSuppressed(queue.guild.id, 0) to after handleQueueReplenishment while preserving the non-autoplay guard, so the explicit track plays alone before autoplay resumes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/handlers/player/lifecycleHandlers.ts`:
- Around line 117-131: Update handleQueueExhaustion so suppressed and
intentional-stop exhaustion paths clear or disarm the musicWatchdogService
instead of returning with an active timer. Ensure this cleanup occurs when
autoplay is suppressed or the queue is intentionally stopped, including cases
where queue.currentTrack or queued tracks remain, while preserving the existing
markIntentionalStop behavior for autoplay-disabled or trackless queues.
In `@packages/bot/src/services/musicManagement/watchdog.ts`:
- Around line 150-156: Update the recovery lock handling in watchdog methods
checkAndRecover and recoverOrphanSession so recoveryInProgress stores
acquisition timestamps rather than only guild membership. Add an
isRecoveryInProgress helper using recoveryLockMaxMs to delete and release stale
entries before treating a guild as locked, and record Date.now() when acquiring
the lock; preserve cleanup when recovery completes.
---
Nitpick comments:
In `@packages/bot/src/handlers/player/trackHandlers.ts`:
- Around line 242-248: The suppression flag is cleared before
handleQueueReplenishment in the playerStart flow, allowing an explicit track
with repeatMode AUTOPLAY to trigger immediate replenishment. Move
setReplenishSuppressed(queue.guild.id, 0) to after handleQueueReplenishment
while preserving the non-autoplay guard, so the explicit track plays alone
before autoplay resumes.
In `@packages/bot/src/services/musicManagement/watchdog.spec.ts`:
- Around line 350-366: Update the test’s MusicWatchdogService construction in
“scanOrphanSessions skips guild when intentional stop is set” to pass a short
timeoutMs, matching the existing concurrency test pattern (100ms), so
markIntentionalStop does not leave a long pending timer.
- Around line 536-567: Add a test alongside the concurrent checkAndRecover case
using a rejecting play mock, await the failed recovery, then invoke
checkAndRecover again for the same guild and verify it is not treated as
recovery_already_in_progress. Assert the lock is released through the failure
path and preserve the expected recovery failure result or error behavior exposed
by checkAndRecover.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c2eba0-7fe5-444d-a73e-bec153fa4d7b
📒 Files selected for processing (8)
packages/bot/src/functions/music/commands/clear.spec.tspackages/bot/src/functions/music/commands/clear.tspackages/bot/src/handlers/player/lifecycleHandlers.spec.tspackages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/services/musicManagement/watchdog.spec.tspackages/bot/src/services/musicManagement/watchdog.ts
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces autoplay replenish suppression tied to the /clear command. When a queue is cleared, it now calls setReplenishSuppressed with a 1-hour ceiling so that a still-playing track's natural end doesn't trigger an autoplay refill that undoes the clear. Correspondingly, the emptyQueue and connection lifecycle handlers gain guards checking isReplenishSuppressed and isIntentionalStop, and the playerStart track handler lifts the suppression when an explicit (non-autoplay) track begins. The surface area spans the clear command, the player lifecycle handlers, and the track handlers, plus their accompanying spec files with new test cases (referencing issues #1948, #1957, #1998). The changed symbol list also includes watchdog service and queue-related items, suggesting related mocks and interactions were touched in tests. Note: the diff was truncated in the prompt, so parts of trackHandlers.ts/.spec.ts and the watchdog files aren't fully visible here.
No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 120 functions depend on the 111 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees - worse:
handlePlayerStart()— 2 callers, 10 callees
Verification — 120 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 120 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Auto-approved: Focused bug fix enforcing stop/suppress guards across mutation points with a recovery lock, validated by tests. No exposure beyond the 1h suppression ceiling; user-intent-driven suppression lift is designed and tested.
Re-trigger cubic
recoveryInProgress had no timeout - a hung queue.node.play() or restoreSnapshot() would wedge the lock for the process lifetime, permanently disabling recovery for that guild. store the acquisition timestamp and treat entries older than 30s as stale, per coderabbit review on pr 1998.
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces suppression of autoplay queue replenishment tied to the /clear command and related player lifecycle events, coordinated through a replenishSuppressionStore. The /clear command now sets a suppression flag (with a 1h ceiling), the emptyQueue lifecycle handler skips replenishment when suppression or an intentional stop is active, and the playerStart track handler clears suppression when an explicit (non-autoplay) track starts. It also adds an intentional-stop guard to the connection-based snapshot restore path. The surface area spans the clear command, player lifecycle/track handlers, and the watchdog service module, along with accompanying spec files that add mocks and test cases for these new suppression and intentional-stop conditions.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 121 functions depend on the 112 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees - worse:
handlePlayerStart()— 2 callers, 10 callees
Verification — 121 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 121 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
the stale-lock expiry from the last fix let a slow-but-legit recovery clear a newer recovery's lock: if attempt a outlives the 30s staleness window, attempt b acquires a fresh lock, then a's delayed release wiped b's active lock unconditionally, letting a third attempt start while b was still running. capture the acquisition token and only release if it still matches, per cubic review on pr 1998.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Focused bug fix wiring existing stop/suppress guards into four mutation sites and adding a per-guild recovery lock with expiry, each backed by new tests; no public contract, data, or auth changes.
Re-trigger cubic
|
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces autoplay replenish suppression logic tied to the /clear command and player lifecycle events. When a user clears the queue, it now sets a suppression flag (with a 1-hour safety ceiling) so the still-playing track's natural end doesn't silently trigger autoplay refill and undo the clear. The change touches several handlers: the emptyQueue lifecycle handler now checks both intentional-stop and replenish-suppression state before replenishing, the connection/snapshot-restore path adds an intentional-stop guard, and the playerStart track handler clears suppression when an explicit (non-autoplay) track begins. Corresponding unit tests were added/updated across the clear command, lifecycle handlers, and track handlers to exercise these new conditions.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 121 functions depend on the 112 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees - worse:
handlePlayerStart()— 2 callers, 10 callees
Verification — 121 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 121 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
🤖 I have created a release *beep* *boop* --- <details><summary>2.39.2</summary> ## [2.39.2](v2.39.1...v2.39.2) (2026-08-10) ### Bug Fixes * **bot:** guard voice/session mutations with stop/suppress flags ([#1998](#1998)) ([23eaba2](23eaba2)) * **ci:** clear stale audit-gate entries and bump fixed advisories ([#1942](#1942)) ([adf710b](adf710b)) * **ci:** clear stale audit-gate entries and bump fixed advisories ([#1960](#1960)) ([2c17c99](2c17c99)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
## What Fixes #2241. Bot resumed playing music after an explicit `/stop`, sometimes with tracks unrelated to the current listening session. `/stop` deletes the session snapshot and marks an intentional stop, then deletes the queue. Deleting the queue fires `connectionDestroyed`, whose handler in `lifecycleHandlers.ts` called `saveSnapshot()` unconditionally — resurrecting the snapshot `/stop` had just removed. `emptyChannel` and `disconnect` had the same unconditional call. Once the 35s intentional-stop window expires, the orphan-session monitor (60s interval) or `checkAndRecover` find the resurrected snapshot, see members still in the voice channel, and restore/resume playback from a session the user had already ended — explaining both the unwanted resume and the stale/out-of-context track choices (they come from the ended session's leftover autoplay queue, not a fresh recommendation). PR #1998 guarded 4 of 6 known mutation sites with `isIntentionalStop`/`isReplenishSuppressed` but never audited these three `saveSnapshot` calls. ## Fix Guard the `saveSnapshot` calls in `connectionDestroyed`, `emptyChannel`, and `disconnect` with `!musicWatchdogService.isIntentionalStop(guildId)`, matching the existing pattern already used on sibling handlers in the same file. ## Testing - Updated `lifecycleHandlers.spec.ts`: the pre-existing `disconnect` intentional-stop test asserted the buggy behavior (`saveSnapshot` called) — flipped to assert it is NOT called. - Added regression tests for `connectionDestroyed` and `emptyChannel` under intentional stop. - `npx jest --config packages/bot/jest.config.cjs --testPathPatterns="lifecycleHandlers.spec"` → 28/28 passing. - `npm run type:check --workspace=packages/bot` clean. ## Test plan - [ ] `/play` something, `/stop`, wait >60s with someone still in the voice channel, confirm playback does not resume. - [ ] `/stop` then leave and rejoin the voice channel, confirm no stale autoplay tracks come back. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes the bot resuming playback after an explicit `/stop` by preventing lifecycle handlers from re-saving the session snapshot. Previously, `/stop` deleted the snapshot and queue, but `connectionDestroyed`, `emptyChannel`, and `disconnect` called `saveSnapshot()` unconditionally, resurrecting the snapshot and causing playback to resume later with stale autoplay tracks from the ended session. **Bug Fixes** - Guarded the three `saveSnapshot()` calls with `!musicWatchdogService.isIntentionalStop(guildId)` to match the existing pattern from earlier `isIntentionalStop` guards. - Updated `lifecycleHandlers.spec.ts` to assert intentional-stop cases no longer save snapshots, adding regression tests for `connectionDestroyed` and `emptyChannel`. <sup>Written for commit cabb3c2. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2242?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->



What
Three confirmed bugs — #1949, #1948, #1957 — share one root cause: two guard primitives (
musicWatchdogService.isIntentionalStop(guildId),isReplenishSuppressed(guildId)) already exist and are correctly checked on thedisconnectevent and theplayerFinish/playerSkip→handleQueueExhaustionpath, but 4 other mutation sites never check them at all:watchdog.ts'srecoverOrphanSession()— reconnects + restores a snapshot with noisIntentionalStopchecklifecycleHandlers.ts'sconnectionevent — restores a snapshot with noisIntentionalStopcheck (siblingdisconnecthandler already guards correctly)lifecycleHandlers.ts'semptyQueueevent — refills via autoplay with no guard check at allclear.ts— never sets the suppression flag, so autoplay silently refills after the current track ends naturallyFix
Wire the two existing guards into all four gaps. Also add a
recoveryInProgressper-guild lock inwatchdog.tssocheckAndRecover(25s timer) and the orphan-session monitor (60s interval) can't both act on the same guild concurrently — the other half of #1949's join/leave-loop symptom.No new state model, no removed watchdog logic — this is the existing guard pattern (already correct on 2 of 6 call sites) applied consistently to the other 4.
Fixes
Closes #1949, Closes #1948, Closes #1957
Testing
watchdog.spec.ts,lifecycleHandlers.spec.ts,clear.spec.tsexercising each new guard + the concurrency locktest:botsuite green (244/245, 1 pre-existing skip unchanged)type:checkclean/playafter an intentional stopContext
Surfaced by a deep-dive session/voice reliability investigation (discord.js/discord-player lifecycle research + causal trace +
/playsilent-failure sweep). 5 more findings from that investigation filed separately as #1993–#1997 (not part of this PR).Summary by cubic
Guard connection restore on intentional stop, and make orphan-session recovery and autoplay honor both stop and suppression flags. Add a per‑guild recovery lock with a 30s expiry and ownership‑aware release to prevent duplicate or wedged recoveries, and clear suppression when the next non‑autoplay track starts so
/clearisn’t undone. This resolves #1949, #1948, and #1957./clearsets suppression with a 1‑hour safety ceiling, and we lift suppression when the next non‑autoplay track starts. We don’t force-stop when suppression applies.Written for commit f7bb546. Summary will update on new commits.
Summary by CodeRabbit