diff --git a/src/audio/player.test.ts b/src/audio/player.test.ts index fe0cb3d..7b9384c 100644 --- a/src/audio/player.test.ts +++ b/src/audio/player.test.ts @@ -377,6 +377,21 @@ describe("AudioPlayer external-PCM mode (playPcmStream)", () => { player.stop(); }); + it("isExternalActive() is false initially, true after playPcmStream, false after stop()", () => { + const player = new AudioPlayer(silentLogger); + // Idle: never attached. + expect(player.isExternalActive()).toBe(false); + + const stream = openPcmReadable(); + player.playPcmStream(stream, {}); + // Attached to the external sidecar stream. + expect(player.isExternalActive()).toBe(true); + + player.stop(); + // Detached again — the orchestrator uses this to know it must re-attach. + expect(player.isExternalActive()).toBe(false); + }); + it("pause()/resume() still gate local emission in external mode (unchanged semantics)", async () => { const player = new AudioPlayer(silentLogger); const stream = openPcmReadable(); diff --git a/src/audio/player.ts b/src/audio/player.ts index 33ba356..4211dd9 100644 --- a/src/audio/player.ts +++ b/src/audio/player.ts @@ -746,4 +746,8 @@ export class AudioPlayer extends EventEmitter { setVolume(vol: number): void { this.volume = Math.max(0, Math.min(100, vol)); } getVolume(): number { return this.volume; } getState(): PlayerState { return this.state; } + // True only while attached to an external (Spotify sidecar) PCM stream. Used + // by the orchestrator to decide whether to re-attach: stop() detaches (sets + // externalMode=false) so this is false after any player.stop(). + isExternalActive(): boolean { return this.externalMode; } } \ No newline at end of file diff --git a/src/bot/instance.test.ts b/src/bot/instance.test.ts index 4d0dc3b..3510cb9 100644 --- a/src/bot/instance.test.ts +++ b/src/bot/instance.test.ts @@ -241,7 +241,7 @@ const handleOccupancy = (BotInstance.prototype as any).handleOccupancy as ( this: unknown, userCount: number, ) => void; -const seek = (BotInstance.prototype as any).seek as (this: unknown, ms: number) => void; +const seek = (BotInstance.prototype as any).seek as (this: unknown, seconds: number) => void; function makeController() { return { @@ -256,13 +256,18 @@ function makeController() { }; } 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. + let externalActive = false; return { - play: vi.fn(), - stop: vi.fn(), - playPcmStream: vi.fn(), + 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(), seek: vi.fn(), + isExternalActive: vi.fn(() => externalActive), }; } function makeResolveCtx(opts: { @@ -370,6 +375,35 @@ describe("BotInstance.resolveAndPlay — Spotify routing (C4)", () => { expect(player.stop).not.toHaveBeenCalled(); }); + it("RE-attaches on a spotify -> (command player.stop) -> spotify sequence (does not stay silent)", async () => { + // Regression: command paths (cmdPlay/cmdPlaylist/cmdAlbum/cmdFm) call + // player.stop() — which DETACHES the external stream — WITHOUT clearing the + // currentSourceIsSpotify flag. Gating re-attach on the stale flag skipped + // playPcmStream, silencing the next spotify track. We now gate on the + // player's actual external state, so the re-attach happens. + const controller = makeController(); + const player = makePlayer(); + const ctx = makeResolveCtx({ + controller, player, url: "spotify:track:abc", currentSourceIsSpotify: false, + }); + + // First spotify track attaches the persistent PCM stream. + await resolveAndPlay.call(ctx, spotifySong()); + expect(player.playPcmStream).toHaveBeenCalledTimes(1); + expect(player.isExternalActive()).toBe(true); + + // A command path stops the player (detaches the stream) but leaves the + // spotify flag stale-true — exactly the state that used to cause silence. + player.stop(); + expect(player.isExternalActive()).toBe(false); + expect(ctx.currentSourceIsSpotify).toBe(true); // flag NOT cleared by stop() + + // Next spotify track MUST re-attach (gate on player external state, not flag). + await resolveAndPlay.call(ctx, spotifySong()); + expect(player.playPcmStream).toHaveBeenCalledTimes(2); + expect(player.isExternalActive()).toBe(true); + }); + it("pauses the sidecar and clears the flag when switching to a non-spotify track", async () => { const controller = makeController(); const player = makePlayer(); @@ -523,14 +557,15 @@ describe("BotInstance.seek — spotify routing (C4)", () => { } as any; } - it("routes seek to the controller for a spotify track", () => { + it("routes seek to the controller for a spotify track, converting seconds -> ms", () => { const ctx = makeSeekCtx("spotify"); - seek.call(ctx, 30); - expect(ctx.spotifyController.seek).toHaveBeenCalledWith(30); + seek.call(ctx, 30); // 30 seconds + // SpotifyController.seek is millisecond-based: 30s -> 30000ms (not 30). + expect(ctx.spotifyController.seek).toHaveBeenCalledWith(30000); expect(ctx.player.seek).not.toHaveBeenCalled(); }); - it("routes seek to the player for a non-spotify track", () => { + it("routes seek to the player (seconds-based) for a non-spotify track", () => { const ctx = makeSeekCtx("netease"); seek.call(ctx, 30); expect(ctx.player.seek).toHaveBeenCalledWith(30); diff --git a/src/bot/instance.ts b/src/bot/instance.ts index c5b3481..4555c90 100755 --- a/src/bot/instance.ts +++ b/src/bot/instance.ts @@ -668,17 +668,21 @@ export class BotInstance extends EventEmitter { // continuous FIFO/PCM stream, so per-track playback is just a REST // playTrack — the stream keeps flowing. await this.spotifyController.playTrack(result.url); - // C4: only ATTACH the persistent PCM stream when coming from a - // non-spotify source. playPcmStream internally fences the prior - // url-ffmpeg (so NO player.stop() here). On a spotify→spotify handoff - // the sidecar changes tracks into the SAME FIFO — re-attaching would - // tear down and re-subscribe the shared stream and silence playback. - if (!this.currentSourceIsSpotify) { + // Only ATTACH the persistent PCM stream when the player is NOT already + // attached to it. Gate on the player's ACTUAL external state, not the + // currentSourceIsSpotify flag: command paths (cmdPlay/cmdPlaylist/…) + // call player.stop() (which detaches the external stream) WITHOUT + // clearing the flag, so a stale-true flag would skip the re-attach and + // silence playback. playPcmStream internally fences the prior url-ffmpeg + // (so NO player.stop() here). On the gapless auto-advance path the + // player is still attached (isExternalActive() === true) so we do NOT + // re-attach — the sidecar rolls the SAME FIFO into the next track. + if (!this.player.isExternalActive()) { this.player.playPcmStream(this.spotifyController.getPcmStream(), { // The sidecar PCM pipe is long-lived; per-track end arrives via the // controller "trackEnded" WS event, not stream EOF. A real EOF here - // means the sidecar died — recovery is the controller's job. - onExternalEnd: () => {}, + // means the sidecar died — surface it rather than silently swallow. + onExternalEnd: () => this.logger.warn("Spotify PCM stream ended unexpectedly"), }); } this.currentSourceIsSpotify = true; @@ -1361,13 +1365,15 @@ export class BotInstance extends EventEmitter { * external — AudioPlayer.seek would respawn ffmpeg on the `spotify:` sentinel * and collide with the running stream), otherwise to the URL player. */ - seek(ms: number): void { + seek(seconds: number): void { if (this.queue.current()?.platform === "spotify") { - this.spotifyController.seek(ms).catch((err) => + // The web route + AudioPlayer.seek are seconds-based, but + // SpotifyController.seek expects milliseconds — convert here. + this.spotifyController.seek(seconds * 1000).catch((err) => this.logger.warn({ err }, "Spotify seek failed")); return; } - this.player.seek(ms); + this.player.seek(seconds); } getQueueManager(): PlayQueue {