From b9fe770ab13c4f89594de582d7baedbaa8c6755e Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Fri, 3 Jul 2026 01:09:00 +0800 Subject: [PATCH] feat(spotify): retry/backoff on Connect commands (device-latency/flakiness watchdog) [S4.6] Co-Authored-By: Claude Opus 4.8 (1M context) --- src/music/spotify/connect-api.test.ts | 95 +++++++++++++++++++++++- src/music/spotify/connect-api.ts | 102 ++++++++++++++++++-------- src/music/spotify/controller.ts | 5 +- 3 files changed, 169 insertions(+), 33 deletions(-) diff --git a/src/music/spotify/connect-api.test.ts b/src/music/spotify/connect-api.test.ts index 8aaee85..cf2c4b6 100644 --- a/src/music/spotify/connect-api.test.ts +++ b/src/music/spotify/connect-api.test.ts @@ -179,6 +179,11 @@ describe("SpotifyConnectApi mutating calls", () => { * rejection that crashes the backend. */ describe("SpotifyConnectApi C3.6 — mutating calls are resilient (no throw)", () => { + // S4.6: transient statuses (404/429) now retry with backoff; inject a no-op + // sleep so these swallow-guarantee tests stay instant (no real timers). The + // no-throw/swallow contract asserted here is unchanged. + const noSleep = async () => {}; + function rejectingHttp(status: number) { const err: any = new Error(`http ${status}`); err.response = { status }; @@ -186,12 +191,18 @@ describe("SpotifyConnectApi C3.6 — mutating calls are resilient (no throw)", ( } it("play() does NOT throw on a 404 (no active device)", async () => { - const api = new SpotifyConnectApi(token(), { http: rejectingHttp(404) }); + const api = new SpotifyConnectApi(token(), { + http: rejectingHttp(404), + sleep: noSleep, + }); await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); }); it("play() does NOT throw on a 429 (rate-limited)", async () => { - const api = new SpotifyConnectApi(token(), { http: rejectingHttp(429) }); + const api = new SpotifyConnectApi(token(), { + http: rejectingHttp(429), + sleep: noSleep, + }); await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); }); @@ -201,7 +212,10 @@ describe("SpotifyConnectApi C3.6 — mutating calls are resilient (no throw)", ( }); it("pause/resume/seek do NOT throw on a rejection", async () => { - const api = new SpotifyConnectApi(token(), { http: rejectingHttp(404) }); + const api = new SpotifyConnectApi(token(), { + http: rejectingHttp(404), + sleep: noSleep, + }); await expect(api.pause("dev-1")).resolves.toBeUndefined(); await expect(api.resume("dev-1")).resolves.toBeUndefined(); await expect(api.seek(1000, "dev-1")).resolves.toBeUndefined(); @@ -214,6 +228,81 @@ describe("SpotifyConnectApi C3.6 — mutating calls are resilient (no throw)", ( }); }); +/** + * Task S4.6: bounded retry/backoff on the mutating Connect commands + * (spec §4.3/§13 recovery/watchdog). Transient statuses {404,429,500,502,503} + * retry up to MAX_ATTEMPTS (3) with exponential backoff; non-transient statuses + * are NOT retried. Every path still preserves C3.6 (swallow, never throw). A + * no-op injected `sleep` keeps the tests instant (no real timers). + */ +describe("SpotifyConnectApi S4.6 — retry/backoff on mutating commands", () => { + const MAX_ATTEMPTS = 3; + const noSleep = async () => {}; + + function rejectStatus(status: number, headers?: Record) { + const err: any = new Error(`http ${status}`); + err.response = { status, headers }; + return err; + } + + it("play() retries a transient 404 then succeeds (2 calls, no throw)", async () => { + const put = vi + .fn() + .mockRejectedValueOnce(rejectStatus(404)) + .mockResolvedValueOnce({ status: 200, data: {} }); + const http = makeHttp({ put }); + const api = new SpotifyConnectApi(token(), { http, sleep: noSleep }); + await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); + expect(put).toHaveBeenCalledTimes(2); + }); + + it("play() exhausts on a persistent 500 (MAX_ATTEMPTS calls, swallowed, warns once)", async () => { + const put = vi.fn().mockRejectedValue(rejectStatus(500)); + const http = makeHttp({ put }); + const warn = vi.fn(); + const logger = { warn } as any; + const api = new SpotifyConnectApi(token(), { http, sleep: noSleep, logger }); + await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); + expect(put).toHaveBeenCalledTimes(MAX_ATTEMPTS); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("does NOT retry a non-transient 403 (exactly ONE call, no throw)", async () => { + const put = vi.fn().mockRejectedValue(rejectStatus(403)); + const http = makeHttp({ put }); + const api = new SpotifyConnectApi(token(), { http, sleep: noSleep }); + await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); + expect(put).toHaveBeenCalledTimes(1); + }); + + it("429 honors a CAPPED Retry-After then succeeds (2 calls, bounded sleep)", async () => { + const put = vi + .fn() + .mockRejectedValueOnce(rejectStatus(429, { "retry-after": "1" })) + .mockResolvedValueOnce({ status: 200, data: {} }); + const http = makeHttp({ put }); + const sleep = vi.fn<(ms: number) => Promise>(async () => {}); + const api = new SpotifyConnectApi(token(), { http, sleep }); + await expect(api.play("dev-1", "spotify:track:abc")).resolves.toBeUndefined(); + expect(put).toHaveBeenCalledTimes(2); + expect(sleep).toHaveBeenCalledTimes(1); + const delay = sleep.mock.calls[0][0]; + expect(delay).toBe(1000); + expect(delay).toBeLessThanOrEqual(2000); + }); + + it("transfer() shares the retry path — 404 then success (2 calls)", async () => { + const put = vi + .fn() + .mockRejectedValueOnce(rejectStatus(404)) + .mockResolvedValueOnce({ status: 200, data: {} }); + const http = makeHttp({ put }); + const api = new SpotifyConnectApi(token(), { http, sleep: noSleep }); + await expect(api.transfer("dev-1", true)).resolves.toBeUndefined(); + expect(put).toHaveBeenCalledTimes(2); + }); +}); + describe("SpotifyConnectApi.getPlaybackState", () => { it("GETs /v1/me/player and maps is_playing/progress/item", async () => { const http = makeHttp({ diff --git a/src/music/spotify/connect-api.ts b/src/music/spotify/connect-api.ts index 6a05a22..56cae27 100644 --- a/src/music/spotify/connect-api.ts +++ b/src/music/spotify/connect-api.ts @@ -2,6 +2,18 @@ import axios, { type AxiosInstance } from "axios"; const API_BASE = "https://api.spotify.com"; +/** + * Task S4.6 (spec §4.3/§13 recovery/watchdog): transient Connect-command + * failures that warrant a bounded retry with exponential backoff. 404 is the + * device-visibility latency case (device not yet enumerated), 429 is rate-limit, + * 5xx are Spotify-side flakiness. Everything else (401/403/network) is NOT + * retried — it is swallowed immediately (C3.6). + */ +const TRANSIENT = new Set([404, 429, 500, 502, 503]); +const MAX_ATTEMPTS = 3; +const BASE_DELAY_MS = 150; +const MAX_DELAY_MS = 2_000; + export interface SpotifyDevice { id: string; name: string; @@ -32,10 +44,21 @@ export interface PlaybackState { export class SpotifyConnectApi { private getToken: () => Promise; private http: AxiosInstance; + private sleep: (ms: number) => Promise; + private logger?: import("pino").Logger; - constructor(getToken: () => Promise, deps?: { http?: AxiosInstance }) { + constructor( + getToken: () => Promise, + deps?: { + http?: AxiosInstance; + sleep?: (ms: number) => Promise; + logger?: import("pino").Logger; + }, + ) { this.getToken = getToken; this.http = deps?.http ?? axios.create({ baseURL: API_BASE, timeout: 15_000 }); + this.sleep = deps?.sleep ?? ((ms) => new Promise((r) => setTimeout(r, ms))); + this.logger = deps?.logger; } /** Bearer auth headers, or null when no valid user token is available. */ @@ -45,6 +68,37 @@ export class SpotifyConnectApi { return { Authorization: `Bearer ${token}` }; } + /** + * S4.6 recovery/watchdog: run a mutating PUT with bounded retry + exponential + * backoff on TRANSIENT statuses only. On exhaustion OR a non-transient error + * it SWALLOWS and warns once — preserving C3.6 (a mutating command NEVER + * rejects up the queue-advance path). Tokens are never logged. + */ + private async mutateWithRetry(put: () => Promise): Promise { + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { + try { + await put(); + return; + } catch (err: any) { + const status = err?.response?.status; + if (!TRANSIENT.has(status) || attempt === MAX_ATTEMPTS) { + // C3.6: never reject up the queue path — swallow, but surface once. + this.logger?.warn( + { status }, + "Spotify Connect command failed (exhausted/non-retryable)", + ); + return; + } + let delay = Math.min(BASE_DELAY_MS * 2 ** (attempt - 1), MAX_DELAY_MS); + if (status === 429) { + const ra = Number(err?.response?.headers?.["retry-after"]); + if (Number.isFinite(ra) && ra > 0) delay = Math.min(ra * 1000, MAX_DELAY_MS); + } + await this.sleep(delay); + } + } + } + async getDevices(): Promise { const headers = await this.authHeaders(); if (!headers) return []; @@ -70,51 +124,43 @@ export class SpotifyConnectApi { async transfer(deviceId: string, play = false): Promise { const headers = await this.authHeaders(); if (!headers) return; - try { - await this.http.put("/v1/me/player", { device_ids: [deviceId], play }, { headers }); - } catch { - // C3.6: swallow (e.g. 403/404/429) — never reject up the queue path. - } + await this.mutateWithRetry(() => + this.http.put("/v1/me/player", { device_ids: [deviceId], play }, { headers }), + ); } async play(deviceId: string, trackUri: string): Promise { const headers = await this.authHeaders(); if (!headers) return; - try { - await this.http.put( + await this.mutateWithRetry(() => + this.http.put( "/v1/me/player/play", { uris: [trackUri] }, { headers, params: { device_id: deviceId } }, - ); - } catch { - // C3.6: swallow — the backend treats a failed play as "couldn't play". - } + ), + ); } async pause(deviceId?: string): Promise { const headers = await this.authHeaders(); if (!headers) return; - try { - await this.http.put("/v1/me/player/pause", undefined, { + await this.mutateWithRetry(() => + this.http.put("/v1/me/player/pause", undefined, { headers, params: deviceId ? { device_id: deviceId } : undefined, - }); - } catch { - // C3.6: swallow. - } + }), + ); } async resume(deviceId?: string): Promise { const headers = await this.authHeaders(); if (!headers) return; - try { - await this.http.put("/v1/me/player/play", undefined, { + await this.mutateWithRetry(() => + this.http.put("/v1/me/player/play", undefined, { headers, params: deviceId ? { device_id: deviceId } : undefined, - }); - } catch { - // C3.6: swallow. - } + }), + ); } async seek(ms: number, deviceId?: string): Promise { @@ -122,11 +168,9 @@ export class SpotifyConnectApi { if (!headers) return; const params: Record = { position_ms: ms }; if (deviceId) params.device_id = deviceId; - try { - await this.http.put("/v1/me/player/seek", undefined, { headers, params }); - } catch { - // C3.6: swallow. - } + await this.mutateWithRetry(() => + this.http.put("/v1/me/player/seek", undefined, { headers, params }), + ); } async getPlaybackState(): Promise { diff --git a/src/music/spotify/controller.ts b/src/music/spotify/controller.ts index 496c0e9..3b2f937 100644 --- a/src/music/spotify/controller.ts +++ b/src/music/spotify/controller.ts @@ -128,7 +128,10 @@ export class SpotifyController extends EventEmitter { ), }); this.connect = - o.connect ?? new SpotifyConnectApi(() => this.oauth.getAccessToken()); + o.connect ?? + new SpotifyConnectApi(() => this.oauth.getAccessToken(), { + logger: this.logger, + }); } /** Shared OAuth client (web router + Rust backend reuse this instance). */