Repository navigation
fix(bot): stop autoplay overriding clear/kick, race in watchdog recovery - #1958
LucasSantana-Dev wants to merge 2 commits into
Conversation
Three confirmed root causes behind the long-standing "autoplay ignores user commands and disconnects" reports: - clear.ts never suppressed autoplay replenish (unlike stop.ts/leave.ts), so the queue silently refilled once the current track ended. Now calls setReplenishSuppressed, same window queueExhaustion.ts already uses. - emptyQueue handler in lifecycleHandlers.ts called replenishQueue() directly, bypassing the isIntentionalStop/isReplenishSuppressed guards the sibling playerFinish/playerSkip path respects. Now checks both. - connection handler restored the last snapshot unconditionally on reconnect, racing a fresh /play after a kick (#1948). Now skips restore when isIntentionalStop is set. - MusicWatchdogService had two independent, uncoordinated reconnect paths (checkAndRecover's timer/disconnect path and the 60s orphan monitor) that could both drive rejoins for the same guild at once, stacking join/leave cycles (#1949). Added a per-guild in-flight lock. Closes #1957, #1948, #1949
📝 WalkthroughWalkthroughThe PR prevents ChangesMusic playback resilience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ClearCommand
participant SuppressionStore
participant LifecycleHandlers
participant MusicWatchdogService
participant Playback
User->>ClearCommand: clear queue
ClearCommand->>SuppressionStore: suppress replenishment for 35 seconds
Playback->>LifecycleHandlers: empty queue event
LifecycleHandlers->>SuppressionStore: check suppression
LifecycleHandlers->>MusicWatchdogService: check intentional stop
LifecycleHandlers-->>Playback: skip autoplay replenishment
MusicWatchdogService->>Playback: start recovery
Playback->>MusicWatchdogService: complete recovery
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/functions/music/commands/clear.spec.ts`:
- Around line 135-138: Update the setReplenishSuppressedMock assertion in the
clear command test to require the suppression interval value 35_000 instead of
accepting any number, while preserving the existing guild identifier assertion.
In `@packages/bot/src/handlers/player/lifecycleHandlers.ts`:
- Around line 51-54: Update MusicWatchdogService and the lifecycle connection
recovery flow to cancel in-flight restoreSnapshot work when
markIntentionalStop() is called, including tracking active restore operations
and aborting their signals. In checkAndRecover(), recheck intentional-stop state
before each awaited rejoin/play step or propagate an abort signal so recovery
cannot resume playback after a stop. Update the affected tests in
packages/bot/src/handlers/player/lifecycleHandlers.ts (lines 51-54) and
packages/bot/src/utils/music/watchdog.ts (lines 156-162) as needed, with
corresponding coverage in lifecycleHandlers.spec.ts (lines 96-124) and
watchdog.spec.ts (lines 87-116).
In `@packages/bot/src/utils/music/watchdog.spec.ts`:
- Around line 310-320: Update the contention test setup around the mocked player
nodes so nodes.get() returns watchdogQueue instead of null. Keep the recovery
flow exercised when the lock is unavailable, allowing scanOrphanSessions to
reach restoreSnapshotMock and verify the intended contention behavior.
🪄 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: 7643c9d6-ba14-435a-9688-04b599997772
📒 Files selected for processing (6)
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/utils/music/watchdog.spec.tspackages/bot/src/utils/music/watchdog.ts
| expect(setReplenishSuppressedMock).toHaveBeenCalledWith( | ||
| 'guild-1', | ||
| expect.any(Number), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the required suppression interval.
expect.any(Number) accepts 0. setReplenishSuppressed treats 0 as removal, so this test can pass without suppression. Assert 35_000.
Proposed test change
expect(setReplenishSuppressedMock).toHaveBeenCalledWith(
'guild-1',
- expect.any(Number),
+ 35_000,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(setReplenishSuppressedMock).toHaveBeenCalledWith( | |
| 'guild-1', | |
| expect.any(Number), | |
| ) | |
| expect(setReplenishSuppressedMock).toHaveBeenCalledWith( | |
| 'guild-1', | |
| 35_000, | |
| ) |
🤖 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/functions/music/commands/clear.spec.ts` around lines 135 -
138, Update the setReplenishSuppressedMock assertion in the clear command test
to require the suppression interval value 35_000 instead of accepting any
number, while preserving the existing guild identifier assertion.
| if ( | ||
| ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED && | ||
| !musicWatchdogService.isIntentionalStop(queue.guild.id) | ||
| ) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/bot/src/utils/music --items all --type function \
--match 'markIntentionalStop|restoreSnapshot|checkAndRecover'
rg -n -C 4 \
'AbortController|AbortSignal|markIntentionalStop|isIntentionalStop|checkAndRecover|restoreSnapshot' \
packages/bot/srcRepository: LucasSantana-Dev/Lucky
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- lifecycleHandlers outline ---\n'
ast-grep outline packages/bot/src/handlers/player/lifecycleHandlers.ts --view expanded || true
printf '\n--- lifecycleHandlers relevant lines ---\n'
sed -n '1,180p' packages/bot/src/handlers/player/lifecycleHandlers.ts
printf '\n--- watchdog outline ---\n'
ast-grep outline packages/bot/src/utils/music/watchdog.ts --view expanded || true
printf '\n--- watchdog relevant lines ---\n'
sed -n '1,260p' packages/bot/src/utils/music/watchdog.ts
printf '\n--- sessionSnapshots outline ---\n'
ast-grep outline packages/bot/src/utils/music/sessionSnapshots.ts --view expanded || true
printf '\n--- sessionSnapshots restoreSnapshot lines ---\n'
sed -n '270,380p' packages/bot/src/handlers/player/lifecycleHandlers.ts
sed -n '270,410p' packages/bot/src/utils/music/sessionSnapshots.tsRepository: LucasSantana-Dev/Lucky
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- lifecycleHandlers outline ---\n'
ast-grep outline packages/bot/src/handlers/player/lifecycleHandlers.ts --view expanded || true
printf '\n--- lifecycleHandlers relevant lines ---\n'
sed -n '1,180p' packages/bot/src/handlers/player/lifecycleHandlers.ts
printf '\n--- watchdog outline ---\n'
ast-grep outline packages/bot/src/utils/music/watchdog.ts --view expanded || true
printf '\n--- watchdog relevant lines ---\n'
sed -n '1,260p' packages/bot/src/utils/music/watchdog.ts
printf '\n--- sessionSnapshots outline ---\n'
ast-grep outline packages/bot/src/utils/music/sessionSnapshots.ts --view expanded || true
printf '\n--- sessionSnapshots restoreSnapshot lines ---\n'
sed -n '270,380p' packages/bot/src/handlers/player/lifecycleHandlers.ts
sed -n '281,410p' packages/bot/src/utils/music/sessionSnapshots.tsRepository: LucasSantana-Dev/Lucky
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lifecycleHandlers relevant lines ---'
sed -n '1,180p' packages/bot/src/handlers/player/lifecycleHandlers.ts
echo
echo '--- watchdog relevant lines ---'
sed -n '1,280p' packages/bot/src/utils/music/watchdog.ts
echo
echo '--- sessionSnapshots restoreSnapshot lines ---'
sed -n '281,410p' packages/bot/src/utils/music/sessionSnapshots.tsRepository: LucasSantana-Dev/Lucky
Length of output: 20757
Cancel recovery work after intentional stop during restore or recovery.
In packages/bot/src/handlers/player/lifecycleHandlers.ts, the connection handler passes an AbortSignal, but MusicWatchdogService.markIntentionalStop() only toggles an intentional-stop flag and clears timers; it does not abort in-flight restoreSnapshot work. If the stop happens after the pre-check, restoreSnapshot() may continue restoring tracks and call play() because it only checks the passed options.signal, not intentional-stop state.
In packages/bot/src/utils/music/watchdog.ts, checkAndRecover() also checks isIntentionalStop() before awaited rejoin/play, but markIntentionalStop() runs before the timer/callback arrives. If the stop happens while awaiting those steps, recovery can still resume playback.
Track active snapshot-restore operations and abort them on intentional stop; recheck intentional stop before each awaited recovery step or accept an abort signal from callers.
📍 Affects 4 files
packages/bot/src/handlers/player/lifecycleHandlers.ts#L51-L54(this comment)packages/bot/src/handlers/player/lifecycleHandlers.spec.ts#L96-L124packages/bot/src/utils/music/watchdog.ts#L156-L162packages/bot/src/utils/music/watchdog.spec.ts#L87-L116
🤖 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/lifecycleHandlers.ts` around lines 51 - 54,
Update MusicWatchdogService and the lifecycle connection recovery flow to cancel
in-flight restoreSnapshot work when markIntentionalStop() is called, including
tracking active restore operations and aborting their signals. In
checkAndRecover(), recheck intentional-stop state before each awaited
rejoin/play step or propagate an abort signal so recovery cannot resume playback
after a stop. Update the affected tests in
packages/bot/src/handlers/player/lifecycleHandlers.ts (lines 51-54) and
packages/bot/src/utils/music/watchdog.ts (lines 156-162) as needed, with
corresponding coverage in lifecycleHandlers.spec.ts (lines 96-124) and
watchdog.spec.ts (lines 87-116).
| const nodes = { get: jest.fn().mockReturnValue(null) } | ||
| const player = { nodes, client } as unknown as Player | ||
|
|
||
| const service = new MusicWatchdogService({ timeoutMs: 1_000 }) | ||
|
|
||
| // checkAndRecover (watchdog timer path) grabs the lock first and hangs mid-recovery... | ||
| const recoverPromise = service.checkAndRecover(watchdogQueue) | ||
| // ...so the orphan monitor's independent 60s-interval pass must back off. | ||
| await service.scanOrphanSessions(player) | ||
|
|
||
| expect(restoreSnapshotMock).not.toHaveBeenCalled() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the contention test exercise recovery.
nodes.get() returns null. If the lock is removed, player.nodes.create throws and scanOrphanSessions catches the error. restoreSnapshotMock then remains uncalled, so the test still passes. Return watchdogQueue to make a missing lock reach restoreSnapshot.
Proposed test change
- const nodes = { get: jest.fn().mockReturnValue(null) }
+ const nodes = { get: jest.fn().mockReturnValue(watchdogQueue) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nodes = { get: jest.fn().mockReturnValue(null) } | |
| const player = { nodes, client } as unknown as Player | |
| const service = new MusicWatchdogService({ timeoutMs: 1_000 }) | |
| // checkAndRecover (watchdog timer path) grabs the lock first and hangs mid-recovery... | |
| const recoverPromise = service.checkAndRecover(watchdogQueue) | |
| // ...so the orphan monitor's independent 60s-interval pass must back off. | |
| await service.scanOrphanSessions(player) | |
| expect(restoreSnapshotMock).not.toHaveBeenCalled() | |
| const nodes = { get: jest.fn().mockReturnValue(watchdogQueue) } | |
| const player = { nodes, client } as unknown as Player | |
| const service = new MusicWatchdogService({ timeoutMs: 1_000 }) | |
| // checkAndRecover (watchdog timer path) grabs the lock first and hangs mid-recovery... | |
| const recoverPromise = service.checkAndRecover(watchdogQueue) | |
| // ...so the orphan monitor's independent 60s-interval pass must back off. | |
| await service.scanOrphanSessions(player) | |
| expect(restoreSnapshotMock).not.toHaveBeenCalled() |
🤖 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/utils/music/watchdog.spec.ts` around lines 310 - 320, Update
the contention test setup around the mocked player nodes so nodes.get() returns
watchdogQueue instead of null. Keep the recovery flow exercised when the lock is
unavailable, allowing scanOrphanSessions to reach restoreSnapshotMock and verify
the intended contention behavior.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR introduces a replenish suppression mechanism to prevent autoplay from silently refilling the music queue in certain scenarios. It adds a setReplenishSuppressed call to the /clear command (with a ~35s window) and wires isReplenishSuppressed plus an intentional-stop check into the player lifecycle handlers, so both snapshot restore on connection and autoplay replenish on emptyQueue are skipped when a stop was intentional or suppression is active. The surface area spans the clear command and its spec, the player lifecycleHandlers and its spec, and the music watchdog module and its spec, with new test cases covering the suppressed/intentional-stop paths.
Worth a look
- Non-atomic check-then-act on intentional-stop flag across await in connection handler —
packages/bot/src/handlers/player/lifecycleHandlers.ts:51· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 77 functions depend on the 61 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees
Verification — 77 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: 75 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).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/bot/src/functions/music/commands/clear.spec.ts">
<violation number="1" location="packages/bot/src/functions/music/commands/clear.spec.ts:135">
P3: The new replenish-suppression regression test asserts the duration as `expect.any(Number)`, so it would still pass if CLEAR_REPLENISH_SUPPRESSION_MS were accidentally reduced to a value too small to prevent autoplay refills (or even 0/negative). Assert the actual exported/known value (35_000) or its constant so the test guards the suppression window it claims to protect.</violation>
</file>
<file name="packages/bot/src/functions/music/commands/clear.ts">
<violation number="1" location="packages/bot/src/functions/music/commands/clear.ts:22">
P3: The new `CLEAR_REPLENISH_SUPPRESSION_MS = 35_000` duplicates the literal `35_000` already hard-coded in `queueExhaustion.ts` for the same suppression window. Because the two values are meant to stay in sync (per the comment) but are defined independently, they can drift over time — e.g. tuning the queue-exhaustion window would silently leave /clear using a different window. Consider defining the window once (e.g. exporting a named constant from `replenishSuppressionStore.ts`) and importing it in both `clear.ts` and `queueExhaustion.ts` so both paths share a single source of truth.</violation>
<violation number="2" location="packages/bot/src/functions/music/commands/clear.ts:48">
P2: The 35s suppression window is too short to protect /clear. After /clear keeps the current track playing, its flywheel is only suppressed for 35s; for any current track longer than ~35s (a normal song), the flag expires before the track ends, so emptyQueue sees autoplay enabled with no suppression and refills the queue — undoing the clear the comment says this prevents. Reconsider basing the window on the remaining current-track duration (e.g. length - position + margin) rather than a fixed 35s, since queueExhaustion's 35s covers a different recovery scenario.</violation>
</file>
<file name="packages/bot/src/handlers/player/lifecycleHandlers.ts">
<violation number="1" location="packages/bot/src/handlers/player/lifecycleHandlers.ts:53">
P2: The new connection guard skips snapshot restore when isIntentionalStop is set, but the 60s orphan monitor (MusicWatchdogService.recoverOrphanSession) rejoins and calls restoreSnapshot without consulting isIntentionalStop/isReplenishSuppressed. So after a kick or /clear where members remain in the channel, the orphan monitor can restore the stale snapshot within ~60s and re-queue tracks, undoing the exact #1948/#1957 race this guard is meant to prevent.</violation>
</file>
<file name="packages/bot/src/utils/music/watchdog.ts">
<violation number="1" location="packages/bot/src/utils/music/watchdog.ts:161">
P2: The per-guild `recoveryInFlight` lock is acquired in `checkAndRecover` and only released in the `finally` after the entire recovery — including `await queue.node.play()` — settles. Unlike `waitForConnectionReady`, which is bounded by `recoveryWaitTimeoutMs`, there is no TTL or cleanup on this lock. If `node.play()` (or `connection.rejoin()` / queue.connect) ever hangs or returns a promise that never settles under a network glitch, the guildId stays in `recoveryInFlight` forever. From then on every watchdog timer, disconnect-event recovery, and the 60s orphan-monitor restore for that guild will short-circuit to `'none'` (`recovery_already_in_flight`), permanently dead-lettering recovery. Since the orphan monitor restore is now gated behind this same lock, the impact is broader than just the two watchdog paths. Consider bounding the lock (e.g., clear it on an overall deadline, or only hold it across the connection-rejoin phase and release before the long `play()`), or add a watch-over-age sweep so a wedged guild can recover on a later scan.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| guildId: string, | ||
| ): Promise<void> { | ||
| queue.clear() | ||
| setReplenishSuppressed(guildId, CLEAR_REPLENISH_SUPPRESSION_MS) |
There was a problem hiding this comment.
P2: The 35s suppression window is too short to protect /clear. After /clear keeps the current track playing, its flywheel is only suppressed for 35s; for any current track longer than ~35s (a normal song), the flag expires before the track ends, so emptyQueue sees autoplay enabled with no suppression and refills the queue — undoing the clear the comment says this prevents. Reconsider basing the window on the remaining current-track duration (e.g. length - position + margin) rather than a fixed 35s, since queueExhaustion's 35s covers a different recovery scenario.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/functions/music/commands/clear.ts, line 48:
<comment>The 35s suppression window is too short to protect /clear. After /clear keeps the current track playing, its flywheel is only suppressed for 35s; for any current track longer than ~35s (a normal song), the flag expires before the track ends, so emptyQueue sees autoplay enabled with no suppression and refills the queue — undoing the clear the comment says this prevents. Reconsider basing the window on the remaining current-track duration (e.g. length - position + margin) rather than a fixed 35s, since queueExhaustion's 35s covers a different recovery scenario.</comment>
<file context>
@@ -33,6 +45,7 @@ async function clearQueueAndRespond(
guildId: string,
): Promise<void> {
queue.clear()
+ setReplenishSuppressed(guildId, CLEAR_REPLENISH_SUPPRESSION_MS)
debugLog({
</file context>
| if (ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED) { | ||
| if ( | ||
| ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED && | ||
| !musicWatchdogService.isIntentionalStop(queue.guild.id) |
There was a problem hiding this comment.
P2: The new connection guard skips snapshot restore when isIntentionalStop is set, but the 60s orphan monitor (MusicWatchdogService.recoverOrphanSession) rejoins and calls restoreSnapshot without consulting isIntentionalStop/isReplenishSuppressed. So after a kick or /clear where members remain in the channel, the orphan monitor can restore the stale snapshot within ~60s and re-queue tracks, undoing the exact #1948/#1957 race this guard is meant to prevent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/handlers/player/lifecycleHandlers.ts, line 53:
<comment>The new connection guard skips snapshot restore when isIntentionalStop is set, but the 60s orphan monitor (MusicWatchdogService.recoverOrphanSession) rejoins and calls restoreSnapshot without consulting isIntentionalStop/isReplenishSuppressed. So after a kick or /clear where members remain in the channel, the orphan monitor can restore the stale snapshot within ~60s and re-queue tracks, undoing the exact #1948/#1957 race this guard is meant to prevent.</comment>
<file context>
@@ -47,7 +48,10 @@ export const setupLifecycleHandlers = (player: {
- if (ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED) {
+ if (
+ ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED &&
+ !musicWatchdogService.isIntentionalStop(queue.guild.id)
+ ) {
const metadata = queue.metadata as QueueMetadata | undefined
</file context>
| state.lastRecoveryDetail = 'recovery_already_in_flight' | ||
| return 'none' | ||
| } | ||
| this.recoveryInFlight.add(guildId) |
There was a problem hiding this comment.
P2: The per-guild recoveryInFlight lock is acquired in checkAndRecover and only released in the finally after the entire recovery — including await queue.node.play() — settles. Unlike waitForConnectionReady, which is bounded by recoveryWaitTimeoutMs, there is no TTL or cleanup on this lock. If node.play() (or connection.rejoin() / queue.connect) ever hangs or returns a promise that never settles under a network glitch, the guildId stays in recoveryInFlight forever. From then on every watchdog timer, disconnect-event recovery, and the 60s orphan-monitor restore for that guild will short-circuit to 'none' (recovery_already_in_flight), permanently dead-lettering recovery. Since the orphan monitor restore is now gated behind this same lock, the impact is broader than just the two watchdog paths. Consider bounding the lock (e.g., clear it on an overall deadline, or only hold it across the connection-rejoin phase and release before the long play()), or add a watch-over-age sweep so a wedged guild can recover on a later scan.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/utils/music/watchdog.ts, line 161:
<comment>The per-guild `recoveryInFlight` lock is acquired in `checkAndRecover` and only released in the `finally` after the entire recovery — including `await queue.node.play()` — settles. Unlike `waitForConnectionReady`, which is bounded by `recoveryWaitTimeoutMs`, there is no TTL or cleanup on this lock. If `node.play()` (or `connection.rejoin()` / queue.connect) ever hangs or returns a promise that never settles under a network glitch, the guildId stays in `recoveryInFlight` forever. From then on every watchdog timer, disconnect-event recovery, and the 60s orphan-monitor restore for that guild will short-circuit to `'none'` (`recovery_already_in_flight`), permanently dead-lettering recovery. Since the orphan monitor restore is now gated behind this same lock, the impact is broader than just the two watchdog paths. Consider bounding the lock (e.g., clear it on an overall deadline, or only hold it across the connection-rejoin phase and release before the long `play()`), or add a watch-over-age sweep so a wedged guild can recover on a later scan.</comment>
<file context>
@@ -150,6 +153,13 @@ export class MusicWatchdogService {
+ state.lastRecoveryDetail = 'recovery_already_in_flight'
+ return 'none'
+ }
+ this.recoveryInFlight.add(guildId)
+
let action: RecoveryAction = 'none'
</file context>
| interaction: makeInteraction(), | ||
| } as any) | ||
|
|
||
| expect(setReplenishSuppressedMock).toHaveBeenCalledWith( |
There was a problem hiding this comment.
P3: The new replenish-suppression regression test asserts the duration as expect.any(Number), so it would still pass if CLEAR_REPLENISH_SUPPRESSION_MS were accidentally reduced to a value too small to prevent autoplay refills (or even 0/negative). Assert the actual exported/known value (35_000) or its constant so the test guards the suppression window it claims to protect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/functions/music/commands/clear.spec.ts, line 135:
<comment>The new replenish-suppression regression test asserts the duration as `expect.any(Number)`, so it would still pass if CLEAR_REPLENISH_SUPPRESSION_MS were accidentally reduced to a value too small to prevent autoplay refills (or even 0/negative). Assert the actual exported/known value (35_000) or its constant so the test guards the suppression window it claims to protect.</comment>
<file context>
@@ -117,6 +123,33 @@ describe('clear command', () => {
+ interaction: makeInteraction(),
+ } as any)
+
+ expect(setReplenishSuppressedMock).toHaveBeenCalledWith(
+ 'guild-1',
+ expect.any(Number),
</file context>
| // Same window queueExhaustion.ts uses when the queue runs dry with nothing | ||
| // left to recover — keeps /clear from being silently undone by autoplay | ||
| // refilling the queue the moment the current track ends. | ||
| const CLEAR_REPLENISH_SUPPRESSION_MS = 35_000 |
There was a problem hiding this comment.
P3: The new CLEAR_REPLENISH_SUPPRESSION_MS = 35_000 duplicates the literal 35_000 already hard-coded in queueExhaustion.ts for the same suppression window. Because the two values are meant to stay in sync (per the comment) but are defined independently, they can drift over time — e.g. tuning the queue-exhaustion window would silently leave /clear using a different window. Consider defining the window once (e.g. exporting a named constant from replenishSuppressionStore.ts) and importing it in both clear.ts and queueExhaustion.ts so both paths share a single source of truth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/functions/music/commands/clear.ts, line 22:
<comment>The new `CLEAR_REPLENISH_SUPPRESSION_MS = 35_000` duplicates the literal `35_000` already hard-coded in `queueExhaustion.ts` for the same suppression window. Because the two values are meant to stay in sync (per the comment) but are defined independently, they can drift over time — e.g. tuning the queue-exhaustion window would silently leave /clear using a different window. Consider defining the window once (e.g. exporting a named constant from `replenishSuppressionStore.ts`) and importing it in both `clear.ts` and `queueExhaustion.ts` so both paths share a single source of truth.</comment>
<file context>
@@ -11,6 +14,12 @@ import type { CommandExecuteParams } from '../../../types/CommandData'
+// Same window queueExhaustion.ts uses when the queue runs dry with nothing
+// left to recover — keeps /clear from being silently undone by autoplay
+// refilling the queue the moment the current track ends.
+const CLEAR_REPLENISH_SUPPRESSION_MS = 35_000
async function handleEmptyQueue(
</file context>
PR #1950 (open since 2026-08-05) already fixes #1948 and #1949 with an equivalent implementation. Adopted its version of lifecycleHandlers.ts and watchdog.ts wholesale instead of shipping a competing duplicate, and kept only the emptyQueue suppression guard on top, which #1950 does not touch. Once #1950 merges, this PR's diff on those two files should be empty and only the clear.ts + emptyQueue fix for #1957 remains.
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 adds a mechanism to suppress autoplay queue replenishment in the music bot, gated on two conditions: an explicit /clear command and the watchdog's "intentional stop" state. It introduces a new replenishSuppressionStore module (referenced via mocks), has /clear call setReplenishSuppressed with a ~35s window after clearing a non-empty queue, and updates the emptyQueue lifecycle handler to check both isReplenishSuppressed and isIntentionalStop before replenishing. The lifecycle connection handler is also changed to skip snapshot restoration when the guild is flagged as an intentional stop. Accompanying test files add coverage for the new suppression paths in the clear command, lifecycle handlers, and watchdog orphan-session behavior, along with some formatting changes.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 77 functions depend on the 61 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
setupLifecycleHandlers()— 3 callers, 8 callees
Verification — 77 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: 75 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.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/bot/src/utils/music/watchdog.ts">
<violation number="1" location="packages/bot/src/utils/music/watchdog.ts:158">
P3: In the recovery-in-progress guard, the diff dropped `state.lastRecoveryAction = 'none'` while keeping the detail update, so this is now the only 'none' exit of checkAndRecover that leaves the action field stale. Every other skip path (intentional_stop, queue_playing) and the normal completion path set both fields together. As a result the /music health diagnostic can show a mismatched pair, e.g. a previously recorded 'failed'/'requeue_current' action alongside the 'recovery_already_in_progress' detail. Consider restoring `state.lastRecoveryAction = 'none'` in this branch to keep the state consistent with the other paths.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| if (this.recoveringGuilds.has(guildId)) { | ||
| state.lastRecoveryDetail = 'recovery_already_in_progress' |
There was a problem hiding this comment.
P3: In the recovery-in-progress guard, the diff dropped state.lastRecoveryAction = 'none' while keeping the detail update, so this is now the only 'none' exit of checkAndRecover that leaves the action field stale. Every other skip path (intentional_stop, queue_playing) and the normal completion path set both fields together. As a result the /music health diagnostic can show a mismatched pair, e.g. a previously recorded 'failed'/'requeue_current' action alongside the 'recovery_already_in_progress' detail. Consider restoring state.lastRecoveryAction = 'none' in this branch to keep the state consistent with the other paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/utils/music/watchdog.ts, line 158:
<comment>In the recovery-in-progress guard, the diff dropped `state.lastRecoveryAction = 'none'` while keeping the detail update, so this is now the only 'none' exit of checkAndRecover that leaves the action field stale. Every other skip path (intentional_stop, queue_playing) and the normal completion path set both fields together. As a result the /music health diagnostic can show a mismatched pair, e.g. a previously recorded 'failed'/'requeue_current' action alongside the 'recovery_already_in_progress' detail. Consider restoring `state.lastRecoveryAction = 'none'` in this branch to keep the state consistent with the other paths.</comment>
<file context>
@@ -153,12 +154,11 @@ export class MusicWatchdogService {
- state.lastRecoveryAction = 'none'
- state.lastRecoveryDetail = 'recovery_already_in_flight'
+ if (this.recoveringGuilds.has(guildId)) {
+ state.lastRecoveryDetail = 'recovery_already_in_progress'
return 'none'
}
</file context>
| state.lastRecoveryDetail = 'recovery_already_in_progress' | |
| state.lastRecoveryAction = 'none' | |
| state.lastRecoveryDetail = 'recovery_already_in_progress' |
) ## Summary Fixes #1959 — the required Security check has failed on every PR since 2026-08-05 (5+ days), regardless of what's changed. **Root cause:** `undici` was override-pinned to exactly `7.28.0`, itself inside the current advisory range (`7.0.0-7.28.0`, fixed at `7.29.0`); `ip-address` had no ceiling clearing its vulnerable range (`<=10.3.0`). Both predate this PR, confirmed via `git stash` + `node scripts/audit-gate.mjs` against clean `main`. **Fix:** - Bumped `undici` to `^7.29.0` and `ip-address` to `>=10.3.1` in root `overrides`. - `packages/frontend` had its own direct `undici@7.28.0` devDependency, independent of the root override — bumped it too. (Root cause of a detour: a bare `overrides` entry alone doesn't reliably force-resolve a hoisted version when a workspace has a conflicting direct pin in this repo — same shape as the documented piscina fix in 10b68e6. The direct pin wins; bump it in place rather than fighting the override.) - The `react-router` advisory (previously accepted with an exit condition tied to a v8 migration, #1878) cleared on its own during the lockfile regen — `react-router-dom` now resolves to `7.18.2`, past the vulnerable range, via ordinary semver resolution. Removed the now-stale `ACCEPTED` entries per the gate's own hygiene enforcement, closed #1878 as moot. **A detour worth flagging:** mid-fix I hit what looked like nondeterministic package resolution (a `busboy` module-not-found, then an unrelated `jsdom` one) even across repeated clean reinstalls. Root cause was local npm cache corruption (`npm cache verify` reported garbage-collecting 94 stale entries) — unrelated to this fix, resolved with `npm cache clean --force` + reinstall. Mentioning in case it recurs for someone else. ## Test plan - [x] `npm ci` (exact lockfile install, no re-resolution — same install mode as this repo's own CI) succeeds clean - [x] `node scripts/audit-gate.mjs` — zero findings - [x] `npx tsc --noEmit` clean across all 4 workspaces - [x] Full test suite green: shared 1404, bot 3212 (+1 skipped), backend 1352, frontend 1038 — 8006 tests total - [x] `npm run lint` — no new findings (pre-existing warnings in `.agents/browser-automation/*.js` untouched) ## Follow-up Once this merges, #1950 and #1958 (both currently blocked on this same check) should pass once rebased. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Unblocks the failing Security check by bumping vulnerable packages and removing stale audit-gate overrides. Fixes #1959. - **Dependencies** - Override `undici` to `^7.29.0`; bump frontend devDependency to `^7.29.0`. - Set `ip-address` to `>=10.3.1`. - Add `fast-uri` override `^3.1.5` (cap to 3.x to match `ajv`). - Bump frontend vendor-state gzip limit to 26.5 KB after `axios` minor update increased bundle size slightly. - **Bug Fixes** - Clear `ACCEPTED` entries for `react-router`/`react-router-dom` now that resolved versions are past advisories. - Root cause was `undici@7.28.0` and missing `ip-address` ceiling keeping installs in vulnerable ranges. - Security gate now passes on `npm ci`; CI is unblocked. <sup>Written for commit 153c909. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1960?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. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
1 participant
Summary
Root-caused the "autoplay overrides commands / bot comes back on its own after disconnect" reports (open ~6 months).
Reconciled with #1950 (open since 2026-08-05, not yet merged): it already fixes #1948 and #1949 with an equivalent implementation (same guard, same per-guild lock pattern, independently converged). Rather than ship a competing duplicate, this PR now adopts #1950's
lifecycleHandlers.ts/watchdog.tsverbatim and adds only what #1950 doesn't touch:/clearnever suppressed autoplay replenish (unlike/stop//leave) — the queue silently refilled once the current track ended.clear.tsnow callssetReplenishSuppressed(), reusing the windowqueueExhaustion.tsalready uses.emptyQueueplayer-event handler calledreplenishQueue()directly, bypassing theisIntentionalStop/isReplenishSuppressedguards its siblingplayerFinish/playerSkippath respects. Now checks both — filed as bug(bot): autoplay silently undoes /clear and bypasses suppression guards on emptyQueue #1957.Once #1950 merges, this PR's diff on
lifecycleHandlers.ts/watchdog.tsshould be empty (rebase will show no conflict) and only theclear.ts+emptyQueuefix for #1957 remains as net-new content. Recommend merging #1950 first.YouTube extractor: code itself checked clean (search/playlist/error-handling paths). The "extractor has issues" complaint maps to already-open #1929 (extractor silently degrades — registers truthy but non-functional on YouTube-side session failures); its fix (Prometheus metric + user-facing wording) is a distinct, larger scoped task, left as a follow-up rather than bolted on here.
Blocked on a repo-wide CI issue (not this PR)
Both this PR and #1950 currently fail the required Security check — this is not caused by either PR's diff. Root cause:
package.json'soverridespinsundicito exactly7.28.0(itself now a vulnerable version — advisory range7.0.0–7.28.0) and floorsip-addressat>=10.1.1with no ceiling past its vulnerable range (<=10.3.0). Confirmed pre-existing on cleanmainviagit stash+node scripts/audit-gate.mjs.I attempted a fix (bump the override pins to
undici: 7.29.0,ip-address: >=10.3.1, addfast-uri: >=3.1.5) butnpm installdid not consistently re-resolve the root-level packages to the new floors even after a full reinstall, and the local tree showed additional drift (nanoidfindings appearing/disappearing between runs, a pre-existingreact-routeradvisory-id mismatch unrelated to my changes). This needs a careful, dedicated lockfile-health pass with full CI verification — not something to force through inline here. Did not commit a half-verifiedpackage-lock.json.Closes #1957
Test plan
npx jest clear.spec.ts lifecycleHandlers.spec.ts watchdog.spec.ts— 50 passed (watchdog/lifecycleHandlers tests are now fix(bot): music voice reconnect races — stale-song revert + stacked join/leave #1950's, adopted as-is; new emptyQueue suppression tests added)tsc --noEmitclean on all touched fileseslintclean