Repository navigation
fix(bot): music voice reconnect races — stale-song revert + stacked join/leave - #1950
LucasSantana-Dev wants to merge 4 commits into
Conversation
The connection event handler restored the guild's saved music snapshot unconditionally, including right after a kick was flagged as an intentional stop. That let a stale snapshot (the pre-kick track) get replayed a few seconds into a freshly issued /play, reverting playback to the old song. Fixes #1948
checkAndRecover (armed on a 25s timer, and called directly from the disconnect event) and the orphan session monitor (a separate 60s interval) could both act on the same guild at once with no coordination. On a flaky connection this produced multiple stacked rejoin/connect cycles, visible as the bot repeatedly joining and leaving the voice channel. Adds a per-guild in-flight lock both paths check before acting. Fixes #1949
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
Three new cases, each verified to fail against the pre-fix source before the fix commits landed: - connection handler skips restoreSnapshot when isIntentionalStop is true (#1948) - checkAndRecover called twice concurrently for the same guild only drives one recovery (#1949) - recoverOrphanSession skips a guild already locked by an in-flight checkAndRecover (#1949)
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
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 concurrency guarding to the MusicWatchdogService. It introduces a recoveringGuilds set used in both checkAndRecover and recoverOrphanSession to prevent those two recovery paths from running simultaneously for the same guild, with early-return/no-op behavior when a recovery is already in progress. The orphan-recovery logic is also restructured into a try/finally block so the guild is cleared from the set afterward. Separately, the lifecycle handler now checks musicWatchdogService.isIntentionalStop(...) as an additional condition before triggering session restore. There are also minor formatting-only changes in the constructor. Note: I'm describing intent and surface area only, not asserting correctness.
Worth a look
- recoveringGuilds not cleared on early return before try block in checkAndRecover —
packages/bot/src/utils/music/watchdog.ts:160· Escalate · high- 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 — 43 functions depend on the 27 functions this change touches.
Health — grade A; 6 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
setupTrackHandlers()— 4 callers, 6 callees (medium)handlePlayerSkip()— 2 callers, 11 callees (medium)setupLifecycleHandlers()— 3 callers, 7 callees (medium)handlePlayerStart()— 2 callers, 9 callees (medium)handlePlayerFinish()— 2 callers, 8 callees (medium)handleQueueExhaustion()— 3 callers, 5 callees (medium)
Verification — 43 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: 41 function(s) in the blast radius were not formally verified this run
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
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 modifies the music watchdog service and player lifecycle handlers to address concurrent/duplicate recovery scenarios (referencing issues #1948 and #1949). It adds a recoveringGuilds lock set shared between checkAndRecover and recoverOrphanSession to prevent both paths from running for the same guild at once, and it wraps the orphan-session recovery logic in a try/finally to release that lock. It also updates the connection lifecycle handler to skip snapshot restoration when the guild is flagged as an intentional stop. Surface area includes the watchdog service internals, the lifecycle connection handler, and corresponding new/updated test cases and mocks.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 60 functions depend on the 44 functions this change touches.
Health — grade A; 6 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
setupTrackHandlers()— 4 callers, 6 callees (medium)handlePlayerSkip()— 2 callers, 11 callees (medium)setupLifecycleHandlers()— 3 callers, 7 callees (medium)handlePlayerStart()— 2 callers, 9 callees (medium)handlePlayerFinish()— 2 callers, 8 callees (medium)handleQueueExhaustion()— 3 callers, 5 callees (medium)
Verification — 60 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: 58 function(s) in the blast radius were not formally verified this run
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Fixes race conditions with a stale snapshot restore guard and a per-guild recovery lock. Both changes are defensive, test-covered, and preserve existing behavior.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found and verified against the latest diff
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/handlers/player/lifecycleHandlers.ts">
<violation number="1" location="packages/bot/src/handlers/player/lifecycleHandlers.ts:52">
P1: A kick that happens after this one-time check can still let the in-flight snapshot restore enqueue the stale song after `/play` has queued a replacement. The handler starts `restoreSnapshot()` immediately after this condition and only aborts its controller when the 2-second deadline wins; `markIntentionalStop()` does not cancel that controller, and the restore loop only rechecks its abort signal before `queue.addTrack()`. Passing a stop-linked cancellation signal (and/or rechecking the intentional-stop generation before each add, with rollback) would make the kick guard cover the async restore window.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 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.
P1: A kick that happens after this one-time check can still let the in-flight snapshot restore enqueue the stale song after /play has queued a replacement. The handler starts restoreSnapshot() immediately after this condition and only aborts its controller when the 2-second deadline wins; markIntentionalStop() does not cancel that controller, and the restore loop only rechecks its abort signal before queue.addTrack(). Passing a stop-linked cancellation signal (and/or rechecking the intentional-stop generation before each add, with rollback) would make the kick guard cover the async restore window.
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 52:
<comment>A kick that happens after this one-time check can still let the in-flight snapshot restore enqueue the stale song after `/play` has queued a replacement. The handler starts `restoreSnapshot()` immediately after this condition and only aborts its controller when the 2-second deadline wins; `markIntentionalStop()` does not cancel that controller, and the restore loop only rechecks its abort signal before `queue.addTrack()`. Passing a stop-linked cancellation signal (and/or rechecking the intentional-stop generation before each add, with rollback) would make the kick guard cover the async restore window.</comment>
<file context>
@@ -47,7 +47,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>
Dismissed because Cubic found issues in a newer review.
Mutation testing (Stryker, --mutate on watchdog.ts + lifecycleHandlers.ts) found the finally-block lock release in both checkAndRecover and recoverOrphanSession had no coverage proving it actually runs. If the lock never released, a guild would be permanently stuck unable to recover after the first attempt. Two new tests call the recovery path twice sequentially and assert the second call is not blocked. Both target mutants now killed (confirmed via reports/mutation/mutation.html). Overall file scores (watchdog.ts 47.9%, lifecycleHandlers.ts 59.3%) are below the repo's mutation-gate threshold — the remaining survivors are on pre-existing logging/env-parsing code, out of scope for this PR. Not added to stryker.conf.json's gated mutate array.
|
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
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 per-guild recovery lock (recoveringGuilds set) to MusicWatchdogService, guarding both checkAndRecover and the orphan session recovery path so they don't run concurrently for the same guild, with the lock released in a finally block. It also updates setupLifecycleHandlers to skip session snapshot restoration when the guild is flagged as an intentional stop. New tests cover the intentional-stop restore skip, concurrent/overlapping recovery behavior, lock release after completion, and cross-path (checkAndRecover vs orphan monitor) contention. There's also a minor formatting change to the constructor's timeoutMs initialization.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 60 functions depend on the 44 functions this change touches.
Health — grade A; 6 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
setupTrackHandlers()— 4 callers, 6 callees (medium)handlePlayerSkip()— 2 callers, 11 callees (medium)setupLifecycleHandlers()— 3 callers, 7 callees (medium)handlePlayerStart()— 2 callers, 9 callees (medium)handlePlayerFinish()— 2 callers, 8 callees (medium)handleQueueExhaustion()— 3 callers, 5 callees (medium)
Verification — 60 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: 58 function(s) in the blast radius were not formally verified this run
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.
) ## 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}
|
Superseded by #1998 (merged), which already ships the per-guild recovery lock (with stale-lock TTL, actually more robust than this branch's plain |
1 participant
Summary
connectionevent's session-snapshot restore against an active intentional-stop (kick) flag, so a stale pre-kick track can't override a freshly issued/play.checkAndRecoverand the orphan-session monitor, so a flaky connection can't trigger both reconnect paths at once and produce stacked join/leave cycles.Found via a multi-agent deep-debug session tracing two user-reported symptoms.
Closes #1948
Closes #1949
Test plan
npx jest src/utils/music/watchdog.spec.ts src/handlers/player/lifecycleHandlers.spec.ts— 29 passedtype:check(ran via pre-commit hook) — clean/playa different song, confirm it doesn't revertSummary by cubic
Prevents stale tracks from overriding a new /play after a kick and stops duplicate voice recoveries that caused repeated join/leave loops.
checkAndRecoverand the orphan monitor to stop stacked join/leave; tests cover both fixes and verify the lock releases to avoid stuck recoveries.Written for commit 3be30e9. Summary will update on new commits.