From 49a47f5e2bf6daf6ae554562198851d8dc1c53f3 Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Sat, 4 Jul 2026 11:17:59 +0800 Subject: [PATCH] fix(spotify): reconcile sidecar/player state on remove-current, skip-while-paused, and queue-exhaust [corner-case R3-2,R3-3,R3-6] Co-Authored-By: Claude Opus 4.8 (1M context) --- src/bot/instance.test.ts | 205 ++++++++++++++++++++++++++++++++++++++- src/bot/instance.ts | 42 +++++++- 2 files changed, 241 insertions(+), 6 deletions(-) diff --git a/src/bot/instance.test.ts b/src/bot/instance.test.ts index 9241524..be46ff2 100644 --- a/src/bot/instance.test.ts +++ b/src/bot/instance.test.ts @@ -249,6 +249,14 @@ const handleOccupancy = (BotInstance.prototype as any).handleOccupancy as ( userCount: number, ) => void; const seek = (BotInstance.prototype as any).seek as (this: unknown, seconds: number) => void; +const playNext = (BotInstance.prototype as any).playNext as ( + this: unknown, + maxRetries?: number, +) => Promise; +const cmdRemove = (BotInstance.prototype as any).cmdRemove as ( + this: unknown, + cmd: any, +) => Promise | string; function makeController() { return { @@ -266,15 +274,22 @@ function makePlayer() { // `externalActive` mirrors the real AudioPlayer: playPcmStream attaches the // external stream (true), and both stop() and play() detach it (false). The // re-attach guard reads isExternalActive(), so this must track that state. + // `state` mirrors the real player's PlayerState transitions so tests can + // observe paused→playing (R3-3): playPcmStream/play → "playing", stop → + // "idle", pause → "paused" (only from playing), resume → "playing" (only + // from paused). Existing tests never read state, so tracking it is inert + // for them. let externalActive = false; + let state: "idle" | "playing" | "paused" = "idle"; return { - play: vi.fn((..._args: any[]) => { externalActive = false; }), - stop: vi.fn(() => { externalActive = false; }), - playPcmStream: vi.fn((..._args: any[]) => { externalActive = true; }), - pause: vi.fn(), - resume: vi.fn(), + play: vi.fn((..._args: any[]) => { externalActive = false; state = "playing"; }), + stop: vi.fn(() => { externalActive = false; state = "idle"; }), + playPcmStream: vi.fn((..._args: any[]) => { externalActive = true; state = "playing"; }), + pause: vi.fn(() => { if (state === "playing") state = "paused"; }), + resume: vi.fn(() => { if (state === "paused") state = "playing"; }), seek: vi.fn(), isExternalActive: vi.fn(() => externalActive), + getState: vi.fn(() => state), }; } function makeResolveCtx(opts: { @@ -465,6 +480,54 @@ describe("BotInstance.resolveAndPlay — Spotify routing (C4)", () => { expect(player.play).toHaveBeenCalledWith("http://cdn/x.mp3", 0, 200); expect(player.playPcmStream).not.toHaveBeenCalled(); }); + + // R3-3: spotify A playing → pause → skip to spotify B. The persistent PCM + // stream stays attached through the pause, so the re-attach gate skips + // playPcmStream (which is what sets state='playing'). Without an explicit + // resume the player would sit 'paused' and emit silence while the sidecar + // decodes B. The reuse path must force the player back to PLAYING. + it("resumes the player when skipping from a PAUSED spotify track to another spotify track (R3-3)", async () => { + const controller = makeController(); + const player = makePlayer(); + const ctx = makeResolveCtx({ + controller, player, url: "spotify:track:abc", currentSourceIsSpotify: false, + }); + + // Spotify A starts → PCM stream attached, player playing. + await resolveAndPlay.call(ctx, spotifySong()); + expect(player.getState()).toBe("playing"); + expect(player.isExternalActive()).toBe(true); + + // User pauses: player paused; the external stream stays attached. + player.pause(); + expect(player.getState()).toBe("paused"); + expect(player.isExternalActive()).toBe(true); + + // Skip to spotify B via the external-stream-reuse path (playPcmStream skipped). + await resolveAndPlay.call(ctx, spotifySong()); + + // Player must be PLAYING again (frames would emit), not stuck paused. + expect(player.getState()).toBe("playing"); + // Reuse path — the persistent stream was NOT re-attached. + expect(player.playPcmStream).toHaveBeenCalledTimes(1); + expect(player.resume).toHaveBeenCalled(); + }); + + // The normal (non-paused) spotify→spotify handoff must be unaffected: resume + // on an already-playing player is a no-op, so state stays 'playing'. + it("keeps a non-paused spotify→spotify handoff playing (R3-3 no-regression)", async () => { + const controller = makeController(); + const player = makePlayer(); + const ctx = makeResolveCtx({ + controller, player, url: "spotify:track:abc", currentSourceIsSpotify: false, + }); + + await resolveAndPlay.call(ctx, spotifySong()); + await resolveAndPlay.call(ctx, spotifySong()); + + expect(player.getState()).toBe("playing"); + expect(player.playPcmStream).toHaveBeenCalledTimes(1); + }); }); describe("BotInstance.setupPlayerEvents — controller trackEnded wiring", () => { @@ -552,6 +615,138 @@ describe("BotInstance transport delegation — spotify current song", () => { }); }); +// --- R3-2: removing the currently-playing Spotify track --------------------- +// queue.remove() of the current index only decrements currentIndex; it never +// stops the player or sidecar. A spotify track has NO player self-EOF advance +// path, so leaving the sidecar running while queue.current() is no longer that +// track wedges the bot in silence. cmdRemove must reconcile: stop the sidecar +// and advance (or stop cleanly) — but ONLY for a current spotify track. +describe("BotInstance.cmdRemove — spotify current-track reconciliation (R3-2)", () => { + function makeRemoveCtx(opts: { + currentIndex: number; + currentSourceIsSpotify: boolean; + removed: any; + }) { + return { + currentSourceIsSpotify: opts.currentSourceIsSpotify, + queue: { + getCurrentIndex: vi.fn(() => opts.currentIndex), + remove: vi.fn(() => opts.removed), + }, + spotifyController: makeController(), + player: makePlayer(), + playNext: vi.fn(async () => true), + sweepLocalAudio: vi.fn(), + emit: vi.fn(), + logger: { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as any; + } + + it("stops the sidecar and advances when removing the CURRENT spotify track", async () => { + const ctx = makeRemoveCtx({ + currentIndex: 0, + currentSourceIsSpotify: true, + removed: { name: "A", platform: "spotify" }, + }); + + const reply = await cmdRemove.call(ctx, { args: "1" }); // index 0 == current + + expect(reply).toContain("Removed: A"); + expect(ctx.spotifyController.stop).toHaveBeenCalledTimes(1); + expect(ctx.currentSourceIsSpotify).toBe(false); + expect(ctx.player.stop).toHaveBeenCalledTimes(1); + // Advance to whatever is now current (or stop cleanly if empty). + expect(ctx.playNext).toHaveBeenCalledTimes(1); + }); + + it("does NOT stop the sidecar when removing a NON-current track", async () => { + const ctx = makeRemoveCtx({ + currentIndex: 0, + currentSourceIsSpotify: true, + removed: { name: "B", platform: "spotify" }, + }); + + await cmdRemove.call(ctx, { args: "2" }); // index 1 != current (0) + + expect(ctx.spotifyController.stop).not.toHaveBeenCalled(); + expect(ctx.player.stop).not.toHaveBeenCalled(); + expect(ctx.playNext).not.toHaveBeenCalled(); + expect(ctx.currentSourceIsSpotify).toBe(true); + }); + + it("does NOT stop the sidecar when the current track is NOT spotify (URL self-heals)", async () => { + const ctx = makeRemoveCtx({ + currentIndex: 0, + currentSourceIsSpotify: false, // current is a URL track + removed: { name: "A", platform: "netease" }, + }); + + await cmdRemove.call(ctx, { args: "1" }); // index 0 == current + + expect(ctx.spotifyController.stop).not.toHaveBeenCalled(); + expect(ctx.player.stop).not.toHaveBeenCalled(); + expect(ctx.playNext).not.toHaveBeenCalled(); + }); + + it("returns 'Invalid position' without touching the sidecar on a bad index", async () => { + const ctx = makeRemoveCtx({ + currentIndex: 0, + currentSourceIsSpotify: true, + removed: null, // queue.remove() rejects the index + }); + + const reply = await cmdRemove.call(ctx, { args: "9" }); + + expect(reply).toBe("Invalid position"); + expect(ctx.spotifyController.stop).not.toHaveBeenCalled(); + expect(ctx.playNext).not.toHaveBeenCalled(); + }); +}); + +// --- R3-6: queue exhausts on a spotify track -------------------------------- +// playNext's exhausted (non-FM) branch only called player.stop(); it left the +// sidecar decoding and currentSourceIsSpotify stale — diverging from cmdStop. +describe("BotInstance.playNext — spotify teardown on queue exhaust (R3-6)", () => { + function makeExhaustCtx(opts: { currentSourceIsSpotify: boolean }) { + return { + connected: true, + isAdvancing: false, + isFmMode: false, + currentSourceIsSpotify: opts.currentSourceIsSpotify, + voteSkipUsers: new Set(), + queue: { next: vi.fn(() => null), unplayedCount: vi.fn(() => 0) }, + player: makePlayer(), + spotifyController: makeController(), + profileManager: { onSongChange: vi.fn(async () => {}) }, + resolveAndPlay: vi.fn(async () => true), + sweepLocalAudio: vi.fn(), + emit: vi.fn(), + logger: { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as any; + } + + it("stops the sidecar and clears the flag when a spotify queue exhausts", async () => { + const ctx = makeExhaustCtx({ currentSourceIsSpotify: true }); + + const started = await playNext.call(ctx); + + expect(started).toBe(false); + expect(ctx.spotifyController.stop).toHaveBeenCalledTimes(1); + expect(ctx.currentSourceIsSpotify).toBe(false); + expect(ctx.player.stop).toHaveBeenCalledTimes(1); + }); + + it("does NOT stop the sidecar when a NON-spotify queue exhausts", async () => { + const ctx = makeExhaustCtx({ currentSourceIsSpotify: false }); + + const started = await playNext.call(ctx); + + expect(started).toBe(false); + expect(ctx.spotifyController.stop).not.toHaveBeenCalled(); + expect(ctx.player.stop).toHaveBeenCalledTimes(1); + }); +}); + describe("BotInstance.handleOccupancy — spotify auto-pause delegation (C4)", () => { function makeOccupancyCtx(currentPlatform: string, state: string) { return { diff --git a/src/bot/instance.ts b/src/bot/instance.ts index 5de31a6..3003497 100755 --- a/src/bot/instance.ts +++ b/src/bot/instance.ts @@ -760,6 +760,16 @@ export class BotInstance extends EventEmitter { this.currentSourceIsSpotify = false; }, }); + } else { + // External-stream-reuse path: the persistent PCM stream is still + // attached (e.g. we arrived here via pause → skip-within-spotify, + // where the stream stays attached through the pause). playPcmStream — + // which is what puts the player into the 'playing' state — is skipped, + // so without this the player would stay 'paused' and emit silence + // while the sidecar decodes the new track (corner-case R3-3). resume() + // is a no-op on an already-playing player, so the normal (non-paused) + // spotify→spotify handoff is unaffected. + this.player.resume(); } this.currentSourceIsSpotify = true; song.url = result.url; @@ -1058,11 +1068,31 @@ export class BotInstance extends EventEmitter { return "Queue cleared"; } - private cmdRemove(cmd: ParsedCommand): string { + private async cmdRemove(cmd: ParsedCommand): Promise { const index = parseInt(cmd.args, 10) - 1; if (isNaN(index) || index < 0) return "Usage: !remove "; + // Capture BEFORE the splice whether we're removing the currently-playing + // Spotify track: queue.remove() decrements currentIndex, so getCurrentIndex() + // is only meaningful pre-remove. + const removingCurrentSpotify = + index === this.queue.getCurrentIndex() && this.currentSourceIsSpotify; const removed = this.queue.remove(index); if (!removed) return "Invalid position"; + // Corner-case R3-2: removing the track the Spotify sidecar is decoding + // right now leaves it running while queue.current() is no longer that + // track. Since a spotify track has NO player self-EOF advance path, the + // controller "trackEnded" handler would return early (current is no longer + // spotify) and the bot would wedge in silence. Reconcile like cmdStop/skip: + // tear the sidecar down, then advance to whatever is now current — or stop + // cleanly if the queue is now empty (playNext's exhausted branch stops the + // player). Non-current or non-spotify removals are untouched (a URL current + // track self-heals via its own EOF). + if (removingCurrentSpotify) { + this.spotifyController.stop(); + this.currentSourceIsSpotify = false; + this.player.stop(); + await this.playNext(); + } // Sweep after the entry is gone — the file is deleted only if no other // queue position (or bot) still references this upload. this.sweepLocalAudio("removed_from_queue"); @@ -1381,6 +1411,16 @@ export class BotInstance extends EventEmitter { this.profileManager.onSongChange(null).catch(() => {}); } } else { + // Queue exhausted on a non-FM source (skip-past-end or natural + // last-track end via trackEnded→playNext). If the ending track was + // served by the Spotify sidecar, tear it down like cmdStop — + // otherwise the go-librespot Connect device stays active with the + // track loaded (decoding into a detached/backpressured stream) and + // currentSourceIsSpotify stays stale (corner-case R3-6). + if (this.currentSourceIsSpotify) { + this.spotifyController.stop(); + this.currentSourceIsSpotify = false; + } this.player.stop(); this.profileManager.onSongChange(null).catch(() => {}); }