fix(spotify): loopback-bind sidecar API, recover on sidecar death, per-bot go-librespot ports

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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);
+    }
+  });
+});

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:<id>` 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;

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<void> {}
+      isReady(): boolean {
+        return true;
+      }
+      stop(): void {}
+      async playTrack(): Promise<void> {}
+      async pause(): Promise<void> {}
+      async resume(): Promise<void> {}
+      async seek(): Promise<void> {}
+      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 }),
+    );
+  });
+});

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<boolean> {
     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;
   }

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<typeof makeHarness>): 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();

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");