From 19306002e36d07effd0b52fe4c7aa13c72cbc65a Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Sat, 4 Jul 2026 12:08:29 +0800 Subject: [PATCH] fix(spotify): go-librespot recover on sidecar death + WS-reconnect status re-sync + stable-connection backoff [corner-case R4-2,R4-3,R4-5] Co-Authored-By: Claude Opus 4.8 (1M context) --- src/music/spotify/go-librespot-api.test.ts | 81 ++++++++++++++ src/music/spotify/go-librespot-api.ts | 54 +++++++++- src/music/spotify/go-librespot.test.ts | 118 +++++++++++++++++++++ src/music/spotify/go-librespot.ts | 108 ++++++++++++++++++- 4 files changed, 354 insertions(+), 7 deletions(-) diff --git a/src/music/spotify/go-librespot-api.test.ts b/src/music/spotify/go-librespot-api.test.ts index 47247ae..2256ff4 100644 --- a/src/music/spotify/go-librespot-api.test.ts +++ b/src/music/spotify/go-librespot-api.test.ts @@ -215,4 +215,85 @@ describe("GoLibrespotEventClient", () => { expect(() => FakeWebSocket.instances[0].emit("error", new Error("net"))).not.toThrow(); client.stop(); }); + + // R4-3: a WS drop at a track boundary can lose the not_playing/stopped event. + // After a successful RE-open the client emits "reconnected" so the backend can + // re-query GET /status and reconcile. The FIRST connect must NOT emit it (there + // is nothing to reconcile yet). + it("emits 'reconnected' after a reconnect open, but NOT on the initial connect", () => { + vi.useFakeTimers(); + try { + const client = new GoLibrespotEventClient("ws://x/events", { WebSocketCtor: FakeWebSocket as any }); + const onReconnected = vi.fn(); + client.on("reconnected", onReconnected); + client.start(); + + FakeWebSocket.instances[0].emit("open"); // initial connect + expect(onReconnected).not.toHaveBeenCalled(); // no re-sync on first connect + + FakeWebSocket.instances[0].emit("close"); + vi.advanceTimersByTime(500); + expect(FakeWebSocket.instances).toHaveLength(2); // reconnected socket + FakeWebSocket.instances[1].emit("open"); // reconnect open + expect(onReconnected).toHaveBeenCalledTimes(1); + client.stop(); + } finally { + vi.useRealTimers(); + } + }); + + // R4-5: a socket that is accepted then immediately closed (a flap) must let the + // exponential backoff GROW — the old code reset it to 500ms on every 'open', + // pinning reconnects at ~2 Hz. + it("grows the reconnect backoff across open→immediate-close flaps (no 500ms pin)", () => { + vi.useFakeTimers(); + try { + const client = new GoLibrespotEventClient("ws://x/events", { WebSocketCtor: FakeWebSocket as any }); + client.start(); + // Flap #1: accept then immediately drop -> next reconnect at 500ms. + FakeWebSocket.instances[0].emit("open"); + FakeWebSocket.instances[0].emit("close"); + vi.advanceTimersByTime(500); + expect(FakeWebSocket.instances).toHaveLength(2); + + // Flap #2: accept then immediately drop -> backoff has doubled to 1000ms. + FakeWebSocket.instances[1].emit("open"); + FakeWebSocket.instances[1].emit("close"); + vi.advanceTimersByTime(500); + expect(FakeWebSocket.instances).toHaveLength(2); // still 2: 500ms is NOT enough now + vi.advanceTimersByTime(500); + expect(FakeWebSocket.instances).toHaveLength(3); // reconnects only after 1000ms + client.stop(); + } finally { + vi.useRealTimers(); + } + }); + + // R4-5: once a connection has been STABLE (up for >= STABLE_CONNECTION_MS) the + // backoff resets, so a later drop reconnects promptly again. + it("resets the reconnect backoff after a stable connection", () => { + vi.useFakeTimers(); + try { + const client = new GoLibrespotEventClient("ws://x/events", { WebSocketCtor: FakeWebSocket as any }); + client.start(); + // Flap once to grow the backoff to 1000ms. + FakeWebSocket.instances[0].emit("open"); + FakeWebSocket.instances[0].emit("close"); + vi.advanceTimersByTime(500); + expect(FakeWebSocket.instances).toHaveLength(2); + + // Now a STABLE connection: open and stay up past the stability threshold. + FakeWebSocket.instances[1].emit("open"); + vi.advanceTimersByTime(5000); // >= STABLE_CONNECTION_MS -> backoff reset to 500 + + FakeWebSocket.instances[1].emit("close"); + vi.advanceTimersByTime(499); + expect(FakeWebSocket.instances).toHaveLength(2); // not yet + vi.advanceTimersByTime(1); + expect(FakeWebSocket.instances).toHaveLength(3); // reconnected at 500ms -> backoff was reset + client.stop(); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/src/music/spotify/go-librespot-api.ts b/src/music/spotify/go-librespot-api.ts index d516644..bf0007f 100644 --- a/src/music/spotify/go-librespot-api.ts +++ b/src/music/spotify/go-librespot-api.ts @@ -109,6 +109,19 @@ type WebSocketCtor = new (url: string) => WsLike; const INITIAL_RECONNECT_MS = 500; const MAX_RECONNECT_MS = 10000; +// R4-5: a connection must stay up at least this long before we treat it as +// "stable" and reset the reconnect backoff. A socket that is accepted and then +// immediately closed (a flap) never reaches this, so the exponential backoff +// keeps growing instead of pinning the reconnect interval at INITIAL_RECONNECT_MS. +const STABLE_CONNECTION_MS = 5000; + +/** + * Emitted (in addition to the go-librespot event types) after the socket has + * SUCCESSFULLY re-opened following a drop — never on the very first connect. + * Consumers use it to re-query GET /status and reconcile any track-end that was + * emitted by go-librespot during the WS-down window (R4-3). + */ +export type GoLibrespotSyntheticEvent = "reconnected"; export class GoLibrespotEventClient extends EventEmitter { private wsUrl: string; @@ -117,6 +130,11 @@ export class GoLibrespotEventClient extends EventEmitter { private stopped = false; private reconnectDelay = INITIAL_RECONNECT_MS; private reconnectTimer: ReturnType | null = null; + // R4-3: false until the FIRST successful open. A later open is therefore a + // reconnect and warrants a "reconnected" re-sync signal. + private hasConnected = false; + // R4-5: fires STABLE_CONNECTION_MS after an open; only then is the backoff reset. + private stableTimer: ReturnType | null = null; constructor(wsUrl: string, deps?: { WebSocketCtor?: WebSocketCtor }) { super(); @@ -131,6 +149,7 @@ export class GoLibrespotEventClient extends EventEmitter { stop(): void { this.stopped = true; + this.clearStableTimer(); if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; @@ -145,12 +164,13 @@ export class GoLibrespotEventClient extends EventEmitter { if (this.stopped) return; const ws = new this.WebSocketCtor(this.wsUrl); this.ws = ws; - ws.on("open", () => { - this.reconnectDelay = INITIAL_RECONNECT_MS; - }); + ws.on("open", () => this.onOpen()); ws.on("message", (buf: unknown) => this.handleMessage(buf)); ws.on("close", () => { this.ws = null; + // R4-5: the connection is gone — cancel the pending stability reset so a + // short-lived (flapping) socket never resets the backoff. + this.clearStableTimer(); this.scheduleReconnect(); }); ws.on("error", (err: unknown) => { @@ -158,6 +178,34 @@ export class GoLibrespotEventClient extends EventEmitter { }); } + private onOpen(): void { + // R4-3: only a RE-open (a socket that had connected before, then dropped) + // needs reconciliation; the initial connect has nothing to catch up on. + const isReconnect = this.hasConnected; + this.hasConnected = true; + // R4-5: do NOT reset the backoff here. Arm a timer that resets it only once + // the connection has stayed up for STABLE_CONNECTION_MS; a flap that closes + // before then leaves the exponential backoff to keep growing. + this.armStableTimer(); + if (isReconnect) this.emit("reconnected"); + } + + private armStableTimer(): void { + this.clearStableTimer(); + this.stableTimer = setTimeout(() => { + this.stableTimer = null; + this.reconnectDelay = INITIAL_RECONNECT_MS; + }, STABLE_CONNECTION_MS); + (this.stableTimer as { unref?: () => void }).unref?.(); + } + + private clearStableTimer(): void { + if (this.stableTimer) { + clearTimeout(this.stableTimer); + this.stableTimer = null; + } + } + private handleMessage(buf: unknown): void { let parsed: unknown; try { diff --git a/src/music/spotify/go-librespot.test.ts b/src/music/spotify/go-librespot.test.ts index 7fd6ab0..72816cd 100644 --- a/src/music/spotify/go-librespot.test.ts +++ b/src/music/spotify/go-librespot.test.ts @@ -291,3 +291,121 @@ describe("GoLibrespotBackend child-process error handling", () => { expect(onErr).toHaveBeenCalledWith(err); }); }); + +// Let queued microtasks (the async reconnect re-sync) settle. +const flush = () => new Promise((r) => setTimeout(r, 0)); + +describe("GoLibrespotBackend R4-2: unexpected sidecar exit recovery", () => { + it("degrades to trackEnded{reason:'error'}, surfaces 'error', and stops the WS on an UNEXPECTED exit", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); // sets the current uri + + const ended = vi.fn(); + const onErr = vi.fn(); + h.backend.on("trackEnded", ended); + h.backend.on("error", onErr); + + // Sidecar dies under us (nonzero exit, no stop() from us). + h.gliChild.emit("exit", 1, null); + + expect(ended).toHaveBeenCalledWith({ uri: "spotify:track:cur", reason: "error" }); + expect(onErr).toHaveBeenCalledTimes(1); // controller told -> rebuilds on next ensureStarted + expect(h.events.stop).toHaveBeenCalled(); // WS reconnect loop stopped (no dead-port hammer) + expect(h.backend.isReady()).toBe(false); + }); + + it("emits trackEnded BEFORE error so a listener that tears down still receives the skip", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); + + const order: string[] = []; + h.backend.on("trackEnded", () => order.push("trackEnded")); + h.backend.on("error", () => order.push("error")); + h.gliChild.emit("exit", null, "SIGKILL"); + + expect(order).toEqual(["trackEnded", "error"]); + }); + + it("a stop()-initiated exit emits NEITHER trackEnded NOR error", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); + + const ended = vi.fn(); + const onErr = vi.fn(); + h.backend.on("trackEnded", ended); + h.backend.on("error", onErr); + + h.backend.stop(); // intentional teardown -> the ensuing 'exit' must be silent + h.gliChild.emit("exit", 0, "SIGTERM"); + + expect(ended).not.toHaveBeenCalled(); + expect(onErr).not.toHaveBeenCalled(); + }); +}); + +describe("GoLibrespotBackend R4-3: WS reconnect re-sync", () => { + const notPlaying = { stopped: true, paused: false, buffering: false, track: null }; + const stillPlaying = { + stopped: false, + paused: false, + buffering: false, + track: { + uri: "spotify:track:cur", + name: "Song", + artist_names: ["A"], + album_name: "Alb", + album_cover_url: null, + position: 1000, + duration: 200000, + }, + }; + + it("re-queries GET /status on reconnect and emits trackEnded when playback is no longer active", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); + h.rest.getStatus.mockResolvedValue(notPlaying as any); + + const ended = vi.fn(); + h.backend.on("trackEnded", ended); + h.events.emit("reconnected"); + await flush(); + + expect(h.rest.getStatus).toHaveBeenCalled(); + expect(ended).toHaveBeenCalledWith({ uri: "spotify:track:cur", reason: "ended" }); + }); + + it("does NOT emit a spurious trackEnded on reconnect when status shows still-playing", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); + h.rest.getStatus.mockResolvedValue(stillPlaying as any); + + const ended = vi.fn(); + h.backend.on("trackEnded", ended); + h.events.emit("reconnected"); + await flush(); + + expect(h.rest.getStatus).toHaveBeenCalled(); + expect(ended).not.toHaveBeenCalled(); + }); + + it("re-sync is idempotent: a reconnect then a live not_playing emits trackEnded only once", async () => { + const h = makeHarness(); + await h.backend.start(); + await h.backend.playTrack("spotify:track:cur"); + h.rest.getStatus.mockResolvedValue(notPlaying as any); + + const ended = vi.fn(); + h.backend.on("trackEnded", ended); + h.events.emit("reconnected"); + await flush(); + // A duplicate live event for the same track must not double-advance the queue. + h.events.emit("not_playing", { uri: "spotify:track:cur" }); + + expect(ended).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/music/spotify/go-librespot.ts b/src/music/spotify/go-librespot.ts index bec93f6..40c5181 100644 --- a/src/music/spotify/go-librespot.ts +++ b/src/music/spotify/go-librespot.ts @@ -77,6 +77,18 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack private events: GoLibrespotEventClient | null = null; private ready = false; private positionMs = 0; + // R4-2: distinguishes an INTENTIONAL teardown (stop()/failed start()) — where a + // sidecar exit is expected and must be silent — from an UNEXPECTED death that + // must degrade-to-skip + surface an error so the controller relaunches. + private stopping = false; + // The uri of the track currently loaded in the sidecar (set by playTrack and + // refreshed from metadata). Used as the trackEnded uri on an unexpected death + // (R4-2) and on reconnect reconciliation (R4-3). + private currentUri = ""; + // At-most-one trackEnded per track. Set when we emit a track-end, cleared when + // a new track begins. Keeps the reconnect re-sync (R4-3) from double-emitting + // with a live not_playing/stopped event. + private endLatched = false; constructor(o: GoLibrespotBackendOptions) { super(); @@ -89,6 +101,11 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack } async start(): Promise { + // Fresh lifecycle (also covers a start() after a previous stop() on the same + // instance): clear the teardown flag and per-track end state. + this.stopping = false; + this.currentUri = ""; + this.endLatched = false; const spawn = this.deps.spawn ?? realSpawn; const execFileSync = this.deps.execFileSync ?? realExecFileSync; const existsSync = this.deps.existsSync ?? realExistsSync; @@ -157,6 +174,10 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack this.proc.on("exit", (code, signal) => { this.ready = false; this.log.warn({ code, signal }, "go-librespot exited"); + // R4-2: a stop()-initiated exit is expected — stay silent. Any other exit + // is the sidecar dying under us and must be recovered. + if (this.stopping) return; + this.onUnexpectedExit(code, signal); }); // 6. REST client, then poll GET / until the HTTP server answers. @@ -196,6 +217,70 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack } } + /** + * At-most-one track-end per track. The WS not_playing/stopped events, the + * unexpected-death degrade (R4-2) and the reconnect re-sync (R4-3) all funnel + * through here so a track can only advance the queue once. + */ + private emitTrackEnded(e: SpotifyTrackEndedEvent): void { + if (this.endLatched) return; + this.endLatched = true; + this.emit("trackEnded", e); + } + + /** + * R4-2: the go-librespot sidecar died without a stop() from us. Spotify + * auto-advance is driven ONLY by the WS trackEnded event (the player-side stall + * watchdog is disabled in external mode), so a silent death would stall the + * queue forever. Mirror the Rust I4 degrade-to-skip: + * (a) emit trackEnded{reason:"error"} so BotInstance advances the queue; + * (b) stop the WS reconnect loop so it stops hammering the now-dead API port; + * (c) surface via emitError so the controller tears down + relaunches a fresh + * backend on the next ensureStarted(). + * trackEnded MUST be emitted BEFORE emitError: the controller's error handler + * removeAllListeners() on teardown, so a trackEnded emitted after would be lost. + */ + private onUnexpectedExit(code: number | null, signal: NodeJS.Signals | null): void { + // (b) Kill the reconnect loop first — the API port is dead. + try { + this.events?.stop(); + } catch { + /* ignore */ + } + this.events = null; + // (a) Degrade-to-skip only if a track was actually loaded; a death during + // startup with nothing playing has no queue item to advance. + if (this.currentUri) { + this.emitTrackEnded({ uri: this.currentUri, reason: "error" }); + } + // (c) Surface so the controller rebuilds. + this.emitError( + new Error( + `go-librespot exited unexpectedly (code=${code ?? "null"}, signal=${signal ?? "null"})`, + ), + ); + } + + /** + * R4-3: the WS reconnected after a drop. go-librespot only pushes events (it is + * never polled after startup), so a not_playing/stopped emitted during the + * down window is lost and the queue would stall. Re-query GET /status and, if + * playback is no longer active (the track ended in the gap), emit trackEnded so + * the queue advances. Idempotent via the end-latch (no double-emit with a live + * event). Only fired on a real reconnect — never the initial connect. + */ + private async reconcileAfterReconnect(): Promise { + const rest = this.rest; + if (!rest) return; + const status = await rest.getStatus(); + // Couldn't read status — don't guess a track-end. + if (!status) return; + const active = status.track != null && !status.stopped; + if (!active) { + this.emitTrackEnded({ uri: this.currentUri, reason: "ended" }); + } + } + private async waitUntilReady(): Promise { const sleep = this.deps.sleep ?? ((ms: number) => new Promise((r) => setTimeout(r, ms))); const interval = this.deps.pollIntervalMs ?? 200; @@ -219,18 +304,26 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack durationMs: typeof d?.duration === "number" ? d.duration : 0, }; if (typeof d?.position === "number") this.positionMs = d.position; + // A new track is now playing: remember it (for R4-2/R4-3) and clear the + // per-track end-latch so its eventual end can advance the queue. + if (np.uri && np.uri !== this.currentUri) { + this.currentUri = np.uri; + this.endLatched = false; + } this.emit("metadata", np); }); ev.on("seek", (d: any) => { if (typeof d?.position === "number") this.positionMs = d.position; }); ev.on("not_playing", (d: any) => { - const e: SpotifyTrackEndedEvent = { uri: typeof d?.uri === "string" ? d.uri : "", reason: "ended" }; - this.emit("trackEnded", e); + this.emitTrackEnded({ uri: typeof d?.uri === "string" ? d.uri : "", reason: "ended" }); }); ev.on("stopped", (d: any) => { - const e: SpotifyTrackEndedEvent = { uri: typeof d?.uri === "string" ? d.uri : "", reason: "stopped" }; - this.emit("trackEnded", e); + this.emitTrackEnded({ uri: typeof d?.uri === "string" ? d.uri : "", reason: "stopped" }); + }); + // R4-3: reconcile any track-end missed while the WS was down. + ev.on("reconnected", () => { + void this.reconcileAfterReconnect(); }); } @@ -240,6 +333,10 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack async playTrack(uri: string): Promise { if (!this.rest) throw new Error("go-librespot backend not started"); + // Track what is loaded so an unexpected death (R4-2) / reconnect re-sync + // (R4-3) can advance the queue for the right uri, and reset the end-latch. + this.currentUri = uri; + this.endLatched = false; await this.rest.playTrack(uri); } @@ -267,6 +364,9 @@ export class GoLibrespotBackend extends EventEmitter implements SpotifyAudioBack } stop(): void { + // R4-2: mark this an INTENTIONAL teardown so the go-librespot 'exit' handler + // (fired by the SIGTERM below) stays silent instead of degrading-to-skip. + this.stopping = true; this.ready = false; try { this.events?.stop();