diff --git a/src/bot/instance.test.ts b/src/bot/instance.test.ts index 3510cb9..cfc8eae 100644 --- a/src/bot/instance.test.ts +++ b/src/bot/instance.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from "vitest"; -import { BotInstance, COMMAND_DENIED_MESSAGE } from "./instance.js"; +import { BotInstance, COMMAND_DENIED_MESSAGE, spotifyPortsForBotId } from "./instance.js"; import type { TS3TextMessage } from "../ts-protocol/client.js"; // Constructing a real BotInstance is heavy (spawns a TS3Client, AudioPlayer, @@ -404,6 +404,44 @@ describe("BotInstance.resolveAndPlay — Spotify routing (C4)", () => { expect(player.isExternalActive()).toBe(true); }); + it("returns false + sends the fallback when playTrack resolves false (dead/failed sidecar)", async () => { + const controller = makeController(); + controller.playTrack = vi.fn(async () => false); + const player = makePlayer(); + const ctx = makeResolveCtx({ controller, player, url: "spotify:track:abc" }); + + const ok = await resolveAndPlay.call(ctx, spotifySong()); + + expect(ok).toBe(false); + expect(controller.playTrack).toHaveBeenCalledTimes(1); + // Same Stage-1 fallback message as the backend-unavailable path. + expect(ctx.tsClient.sendTextMessage).toHaveBeenCalledTimes(1); + // Never attach the player to a dead stream. + expect(player.playPcmStream).not.toHaveBeenCalled(); + expect(ctx.currentSourceIsSpotify).toBe(false); + }); + + it("recovers on mid-session sidecar death: onExternalEnd stops controller+player and clears the flag", async () => { + const controller = makeController(); + const player = makePlayer(); + const ctx = makeResolveCtx({ controller, player, url: "spotify:track:abc" }); + + await resolveAndPlay.call(ctx, spotifySong()); + expect(ctx.currentSourceIsSpotify).toBe(true); + expect(player.playPcmStream).toHaveBeenCalledTimes(1); + + // The sidecar PCM stream EOFs mid-session → fire the wired onExternalEnd. + const opts = player.playPcmStream.mock.calls[0][1] as { onExternalEnd?: () => void }; + expect(typeof opts.onExternalEnd).toBe("function"); + opts.onExternalEnd!(); + + // Recovery: controller torn down (next track rebuilds), player stopped + // (drops external mode so the next track re-attaches), flag cleared. + expect(controller.stop).toHaveBeenCalledTimes(1); + expect(player.stop).toHaveBeenCalledTimes(1); + expect(ctx.currentSourceIsSpotify).toBe(false); + }); + it("pauses the sidecar and clears the flag when switching to a non-spotify track", async () => { const controller = makeController(); const player = makePlayer(); @@ -572,3 +610,30 @@ describe("BotInstance.seek — spotify routing (C4)", () => { expect(ctx.spotifyController.seek).not.toHaveBeenCalled(); }); }); + +describe("spotifyPortsForBotId — per-bot go-librespot ports (Fix 3)", () => { + it("yields the SAME ports for the same bot id (stable across restarts)", () => { + const a = spotifyPortsForBotId("bot-alpha"); + const b = spotifyPortsForBotId("bot-alpha"); + expect(a).toEqual(b); + }); + + it("yields DIFFERENT ports for different bot ids", () => { + const a = spotifyPortsForBotId("bot-alpha"); + const b = spotifyPortsForBotId("bot-beta"); + expect(a.apiPort).not.toBe(b.apiPort); + expect(a.callbackPort).not.toBe(b.callbackPort); + }); + + it("keeps apiPort and callbackPort in disjoint ranges", () => { + for (const id of ["bot-alpha", "bot-beta", "x", "a-very-long-bot-identifier-123"]) { + const { apiPort, callbackPort } = spotifyPortsForBotId(id); + expect(apiPort).toBeGreaterThanOrEqual(3700); + expect(apiPort).toBeLessThan(4700); + expect(callbackPort).toBeGreaterThanOrEqual(8700); + expect(callbackPort).toBeLessThan(9700); + // Same offset within each range → the two never collide with each other. + expect(callbackPort - apiPort).toBe(5000); + } + }); +}); diff --git a/src/bot/instance.ts b/src/bot/instance.ts index 4555c90..c667b27 100755 --- a/src/bot/instance.ts +++ b/src/bot/instance.ts @@ -31,6 +31,35 @@ import type { SpotifyTrackEndedEvent } from "../music/spotify/backend.js"; /** Reply sent when a non-admin invokes an admin-only chat command. */ export const COMMAND_DENIED_MESSAGE = "⛔ 需要管理员权限（该命令仅限管理员服务器组）"; +/** Fallback message when Spotify audio can't be served (backend unavailable + * OR a per-track playTrack failure against a dead/failed sidecar). */ +const SPOTIFY_UNAVAILABLE_MESSAGE = + "⚠️ Spotify 播放尚未启用（需要 librespot 音频后端，将在后续版本支持）。"; + +/** FNV-1a deterministic string hash (unsigned 32-bit). Stable across restarts + * and processes, unlike a random/insertion-order value — used to derive + * per-bot go-librespot ports. */ +function stableHash(s: string): number { + let h = 2166136261; + for (let i = 0; i < s.length; i++) { + h ^= s.charCodeAt(i); + h = Math.imul(h, 16777619); + } + return h >>> 0; +} + +/** + * STABLE per-bot go-librespot ports derived from the bot id. A second + * Spotify-enabled bot's sidecar must not collide on the control-API or OAuth + * callback ports; deriving them from the id keeps them fixed across restarts. + * The two ranges (37xx / 87xx) are disjoint so the API and callback ports for + * a given bot never clash with each other. + */ +export function spotifyPortsForBotId(id: string): { apiPort: number; callbackPort: number } { + const off = stableHash(id) % 1000; + return { apiPort: 3700 + off, callbackPort: 8700 + off }; +} + export interface BotInstanceOptions { id: string; name: string; @@ -54,6 +83,8 @@ export interface BotInstanceOptions { workDir: string; configDir: string; logger: Logger; + apiPort: number; + callbackPort: number; }) => SpotifyController; } @@ -136,6 +167,10 @@ export class BotInstance extends EventEmitter { options.spotifyDataDir ?? path.join(process.cwd(), "data", "spotify"); const spotifyWorkDir = path.join(spotifyBase, this.id, "work"); const spotifyConfigDir = path.join(spotifyBase, this.id, "config"); + // Distinct, stable per-bot ports so a second Spotify-enabled bot's sidecar + // doesn't fail to bind the go-librespot control API / OAuth callback. + const { apiPort: spotifyApiPort, callbackPort: spotifyCallbackPort } = + spotifyPortsForBotId(this.id); const buildController = options.spotifyControllerFactory ?? ((o) => new SpotifyController({ ...o })); @@ -144,6 +179,8 @@ export class BotInstance extends EventEmitter { workDir: spotifyWorkDir, configDir: spotifyConfigDir, logger: this.logger, + apiPort: spotifyApiPort, + callbackPort: spotifyCallbackPort, }); const profileConfig = this.database.getProfileConfig(this.id); @@ -659,15 +696,20 @@ export class BotInstance extends EventEmitter { const ready = await this.spotifyController.ensureStarted(); if (!ready) { this.logger.info({ songId: song.id, name: song.name }, "Spotify backend unavailable — skipping"); - await this.tsClient.sendTextMessage( - "⚠️ Spotify 播放尚未启用（需要 librespot 音频后端，将在后续版本支持）。" - ); + await this.tsClient.sendTextMessage(SPOTIFY_UNAVAILABLE_MESSAGE); return false; } // `spotify:track:` is the URI. go-librespot decodes into a SINGLE // continuous FIFO/PCM stream, so per-track playback is just a REST - // playTrack — the stream keeps flowing. - await this.spotifyController.playTrack(result.url); + // playTrack — the stream keeps flowing. A false result means the + // sidecar failed the play (dead/errored backend): never attach the + // player to a dead stream — send the same fallback and skip. + const played = await this.spotifyController.playTrack(result.url); + if (!played) { + this.logger.info({ songId: song.id, name: song.name }, "Spotify playTrack failed — skipping"); + await this.tsClient.sendTextMessage(SPOTIFY_UNAVAILABLE_MESSAGE); + return false; + } // 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/…) @@ -681,8 +723,18 @@ export class BotInstance extends EventEmitter { 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 — surface it rather than silently swallow. - onExternalEnd: () => this.logger.warn("Spotify PCM stream ended unexpectedly"), + // means the sidecar died mid-session — RECOVER instead of emitting + // silence forever (which would also leave the player stuck in + // externalMode, so the re-attach gate skips every future track). + // Tear the controller down (next ensureStarted() rebuilds a fresh + // backend), stop the player (drop external mode so it re-attaches), + // and clear the flag so a non-spotify track isn't mis-handled. + onExternalEnd: () => { + this.logger.warn("Spotify PCM stream ended unexpectedly — recovering"); + this.spotifyController.stop(); + this.player.stop(); + this.currentSourceIsSpotify = false; + }, }); } this.currentSourceIsSpotify = true; diff --git a/src/music/spotify/controller.test.ts b/src/music/spotify/controller.test.ts index dfe897e..fb9ced5 100644 --- a/src/music/spotify/controller.test.ts +++ b/src/music/spotify/controller.test.ts @@ -21,6 +21,37 @@ vi.mock("./binary.js", () => ({ checkGoLibrespotAvailable: async () => bin.supported && !!bin.path, })); +// Capture options the DEFAULT factory hands to the real GoLibrespotBackend so +// we can assert per-bot ports (Fix 3) are threaded through. Every other test +// injects its own backendFactory, so this mock is inert for them. +const goLibrespotCtor = vi.hoisted(() => vi.fn()); +vi.mock("./go-librespot.js", async () => { + const { EventEmitter } = await import("node:events"); + return { + GoLibrespotBackend: class extends EventEmitter { + constructor(opts: any) { + super(); + goLibrespotCtor(opts); + } + async start(): Promise {} + isReady(): boolean { + return true; + } + stop(): void {} + async playTrack(): Promise {} + async pause(): Promise {} + async resume(): Promise {} + async seek(): Promise {} + getPcmStream(): any { + return null; + } + getPositionMs(): number { + return 0; + } + }, + }; +}); + // Import AFTER vi.mock so the mocked binary module is used. const { SpotifyController } = await import("./controller.js"); @@ -401,3 +432,22 @@ describe("SpotifyController.stop", () => { expect(be.stopCalls).toBe(0); }); }); + +describe("SpotifyController per-bot ports (Fix 3)", () => { + it("threads apiPort + callbackPort into the default GoLibrespotBackend", async () => { + goLibrespotCtor.mockClear(); + // No injected backendFactory → the controller builds the (mocked) real backend. + const ctrl = new SpotifyController({ + config: cfg(), + workDir: "/tmp/work", + configDir: "/tmp/cfg", + logger: silentLogger, + apiPort: 3712, + callbackPort: 8712, + }); + expect(await ctrl.ensureStarted()).toBe(true); + expect(goLibrespotCtor).toHaveBeenCalledWith( + expect.objectContaining({ apiPort: 3712, callbackPort: 8712 }), + ); + }); +}); diff --git a/src/music/spotify/controller.ts b/src/music/spotify/controller.ts index f378cf2..32117bd 100644 --- a/src/music/spotify/controller.ts +++ b/src/music/spotify/controller.ts @@ -16,6 +16,10 @@ export interface SpotifyControllerOptions { workDir: string; configDir: string; logger: Logger; + /** Per-bot go-librespot control-API port (distinct per bot to avoid binds). */ + apiPort?: number; + /** Per-bot go-librespot OAuth callback port (distinct per bot). */ + callbackPort?: number; /** Injected for tests; defaults to constructing a real GoLibrespotBackend. */ backendFactory?: () => SpotifyAudioBackend; } @@ -39,6 +43,8 @@ export class SpotifyController extends EventEmitter { private readonly workDir: string; private readonly configDir: string; private readonly logger: Logger; + private readonly apiPort?: number; + private readonly callbackPort?: number; private readonly backendFactory: () => SpotifyAudioBackend; private backend: SpotifyAudioBackend | null = null; @@ -51,6 +57,8 @@ export class SpotifyController extends EventEmitter { this.workDir = o.workDir; this.configDir = o.configDir; this.logger = o.logger; + this.apiPort = o.apiPort; + this.callbackPort = o.callbackPort; this.backendFactory = o.backendFactory ?? (() => @@ -59,6 +67,8 @@ export class SpotifyController extends EventEmitter { bitrate: this.config.bitrate, workDir: this.workDir, configDir: this.configDir, + apiPort: this.apiPort, + callbackPort: this.callbackPort, logger: this.logger, })); } @@ -79,7 +89,14 @@ export class SpotifyController extends EventEmitter { */ async ensureStarted(): Promise { if (!this.isAvailable()) return false; - if (this.started) return true; + if (this.started) { + // A previously-started backend still counts as ready only if its process + // is alive. If the sidecar died (isReady()===false) — e.g. the go-librespot + // child exited — tear it down here so the code below rebuilds a fresh one + // instead of handing callers a dead backend. + if (this.backend?.isReady()) return true; + this.stop(); + } if (this.startPromise) return this.startPromise; this.startPromise = (async () => { @@ -169,12 +186,22 @@ export class SpotifyController extends EventEmitter { return this.backend.getPcmStream(); } - /** Tear down the backend and reset lifecycle state (safe before start). */ + /** + * Tear down the backend and reset lifecycle state (safe before start). + * Mirrors handleBackendError's teardown so the NEXT ensureStarted() rebuilds + * a fresh backend: stop() cleans the ffmpeg/go-librespot children + FIFO, + * removeAllListeners() detaches this controller's handlers so a late event + * from the now-orphaned backend can't disturb a rebuilt one. Guard stop() + * (it may throw) so teardown always completes. + */ stop(): void { - if (this.backend) { - this.backend.stop(); - this.backend = null; + try { + this.backend?.stop(); + } catch (stopErr) { + this.logger.error({ err: stopErr }, "Spotify backend stop() threw during teardown"); } + (this.backend as unknown as EventEmitter | null)?.removeAllListeners(); + this.backend = null; this.started = false; this.startPromise = null; } diff --git a/src/music/spotify/go-librespot.test.ts b/src/music/spotify/go-librespot.test.ts index 0d198d0..7fd6ab0 100644 --- a/src/music/spotify/go-librespot.test.ts +++ b/src/music/spotify/go-librespot.test.ts @@ -15,7 +15,7 @@ function makeFakeChild() { return child; } -function makeHarness() { +function makeHarness(portOpts: { apiPort?: number; callbackPort?: number } = {}) { const calls: string[] = []; const ffmpegChild = makeFakeChild(); const gliChild = makeFakeChild(); @@ -52,7 +52,8 @@ function makeHarness() { bitrate: 320, workDir: "/tmp/work", configDir: "/tmp/cfg", - apiPort: 3678, + apiPort: portOpts.apiPort ?? 3678, + callbackPort: portOpts.callbackPort, logger: log, deps: { spawn, @@ -134,6 +135,37 @@ describe("GoLibrespotBackend.start", () => { }); }); +describe("GoLibrespotBackend config binding (Fix 1 loopback + Fix 3 ports)", () => { + function writtenYml(h: ReturnType): string { + const calls = h.writeFileSync.mock.calls as unknown as any[][]; + const call = calls.find((c) => String(c[0]).endsWith("config.yml")); + expect(call).toBeDefined(); + return call![1] as string; + } + + it("binds the control API to loopback (127.0.0.1), never 0.0.0.0", async () => { + const h = makeHarness(); + await h.backend.start(); + const yml = writtenYml(h); + expect(yml).toContain("address: 127.0.0.1"); + expect(yml).not.toContain("0.0.0.0"); + }); + + it("threads apiPort + callbackPort into the rendered config", async () => { + const h = makeHarness({ apiPort: 3712, callbackPort: 8712 }); + await h.backend.start(); + const yml = writtenYml(h); + expect(yml).toContain("port: 3712"); + expect(yml).toContain("callback_port: 8712"); + }); + + it("defaults callbackPort to 8080 when unset", async () => { + const h = makeHarness(); + await h.backend.start(); + expect(writtenYml(h)).toContain("callback_port: 8080"); + }); +}); + describe("GoLibrespotBackend WebSocket event mapping", () => { it("maps a not_playing event to trackEnded{reason:'ended'}", async () => { const h = makeHarness(); diff --git a/src/music/spotify/go-librespot.ts b/src/music/spotify/go-librespot.ts index cdc1f0c..bec93f6 100644 --- a/src/music/spotify/go-librespot.ts +++ b/src/music/spotify/go-librespot.ts @@ -32,6 +32,7 @@ export interface GoLibrespotBackendOptions { workDir: string; configDir: string; apiPort?: number; + callbackPort?: number; logger: Logger; deps?: GoLibrespotBackendDeps; } @@ -67,6 +68,7 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack private readonly log: Logger; private readonly deps: GoLibrespotBackendDeps; private readonly apiPort: number; + private readonly callbackPort: number; private readonly fifoPath: string; private ffmpeg: ChildProcess | null = null; @@ -82,6 +84,7 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack this.log = o.logger; this.deps = o.deps ?? {}; this.apiPort = o.apiPort ?? DEFAULT_API_PORT; + this.callbackPort = o.callbackPort ?? DEFAULT_CALLBACK_PORT; this.fifoPath = posixPath.join(o.workDir, FIFO_NAME); } @@ -132,9 +135,12 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack deviceName: this.opts.deviceName, bitrate: this.opts.bitrate, fifoPath: this.fifoPath, - apiAddress: "0.0.0.0", + // The go-librespot control API is UNAUTHENTICATED and the client only + // ever connects via 127.0.0.1; the sidecar runs in the SAME container + // as the bot, so bind to loopback rather than exposing it on 0.0.0.0. + apiAddress: "127.0.0.1", apiPort: this.apiPort, - callbackPort: DEFAULT_CALLBACK_PORT, + callbackPort: this.callbackPort, }); writeFileSync(posixPath.join(this.opts.configDir, "config.yml"), yml, "utf8");