diff --git a/README.md b/README.md index 8abfdbd..fc3401c 100644 --- a/README.md +++ b/README.md @@ -108,11 +108,15 @@ attempt at a Blu-ray-style subtitle as the thing to double-check here. unconfirmed (the ffmpeg direct-stream URL, the Playlists endpoint, the Emby-playstate sync endpoint) were all checked directly against Emby's own real OpenAPI spec. Two matched exactly. The third - playstate sync -- didn't: `PositionTicks`/`IsPaused` were being sent as a JSON body, -but Emby expects them as query parameters, alongside a required -`MediaSourceId` that wasn't being sent at all - meaning every progress -report was almost certainly failing outright. Fixed - see -docs/architecture.md's "Sync watch-party progress back to Emby" +- took two rounds to actually fix: `PositionTicks`/`IsPaused` were +being sent as a JSON body when Emby expects query parameters, alongside +a required `MediaSourceId` that wasn't being sent at all (round 1); a +real request logged after that shipped still failed, this time with +Emby's own "Value cannot be null (Parameter 'key')" - traced to a +missing `PlaySessionId` (round 2, from new logging added specifically +to catch exactly this). Both fixed, though round 2 hasn't yet been +confirmed against a real server the way round 1's own bad request was +- see docs/architecture.md's "Sync watch-party progress back to Emby" section for the full story. The one-shared-stream-per-party work is a different kind of unverified: its own Emby API calls were already live, confirmed code, just newly shared instead of per-viewer - what's untested is behavior diff --git a/docs/architecture.md b/docs/architecture.md index 93c015f..013221f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2023,26 +2023,53 @@ it" on its own, without cross-referencing `EmbyClient`'s own generic `[watchparty:emby]` failure log (which still fires too, independently, with the fuller error body). Safe to log the full path here, unlike `Logger.stream`'s own deliberately-partial logging - this query string -is only ever `MediaSourceId`/`PositionTicks`/`IsPaused`, never a token; -the real `embyToken` travels as a header, never in the URL, and the -tests confirm it never ends up in these lines either. +is only ever `MediaSourceId`/`PlaySessionId`/`PositionTicks`/`IsPaused`, +never a token; the real `embyToken` travels as a header, never in the +URL, and the tests confirm it never ends up in these lines either. + +**Round 2, caught immediately from that new logging**: a real request +logged after the fix above shipped still came back `400`, now `Value +cannot be null. (Parameter 'key')` - a bare .NET `Dictionary` null-key +exception, and *not* the earlier problem (the logged request's +`MediaSourceId` was present and well-formed). This exact message had +already burned this app once before, in `CatalogService +.negotiateStreamTarget`'s hand-built fallback URL (see its own +TODO(verify) comment) - diagnosed there as Emby indexing some internal +state by a parameter the request hadn't supplied. `PlaySessionId` was +the standout remaining candidate: it's how Emby/Jellyfin clients +normally correlate a progress report with the specific playback/ +transcode session it belongs to, and evidently something in Emby's real +handler keys a lookup on it even though the OpenAPI spec doesn't mark +it required. Now sent too, resolved the exact same way `MediaSourceId` +already was: `CatalogService.negotiateStreamTarget` already reads +`info.PlaySessionId` from the very same `PlaybackInfo` response (it +already used it in the hand-built fallback URL) - its return type +(`NegotiatedStreamTarget`) just needed to surface it, so +`PartyStreamService` could cache it alongside `mediaSourceId` and +expose both together from one accessor, +`getCachedPlaybackIds(partyId)`, replacing the round-1 `getCached +MediaSourceId`. Not independently confirmed against a real server yet +(no way to safely fire a real, state-mutating test call without one) - +flagged here the same way every other real-server-dependent guess in +this app is, but grounded in this app's own prior, confirmed diagnosis +of the identical error message rather than a fresh guess. Verified: full typecheck/test/build on both projects. Unit tests cover `getCurrentPlaybackState` (video reads the host, music reads the shared queue, both branches, plus the empty-queue null case), `setMemberEmbyToken`, `PlaystateService.reportProgress`'s corrected -query-string request shape (and that a thrown failure never -propagates), its own request/outcome logging (success, a non-ok -status, and a thrown error each produce a distinct, greppable line, -and the token itself never appears in one), and -`PartyStreamService.getCachedMediaSourceId`'s -cache-hit/cache-miss/dropped-on-failure behavior. `EmbyClient.fetch`'s -optional body-encoding capability stays generic plumbing (its own test -no longer claims PlaystateService as the reason it exists) - no -current call site needs it any more, but it costs nothing to keep for -whatever future caller does. No client changes at all - this is a pure -server-side background process with nothing for the browser to render -or react to. +query-string request shape including `PlaySessionId` (and that a thrown +failure never propagates), its own request/outcome logging (success, a +non-ok status, and a thrown error each produce a distinct, greppable +line, and the token itself never appears in one), and +`PartyStreamService.getCachedPlaybackIds`'s +cache-hit/cache-miss/dropped-on-failure behavior for both ids together. +`EmbyClient.fetch`'s optional body-encoding capability stays generic +plumbing (its own test no longer claims PlaystateService as the reason +it exists) - no current call site needs it any more, but it costs +nothing to keep for whatever future caller does. No client changes at +all - this is a pure server-side background process with nothing for +the browser to render or react to. ## One shared Emby stream per party (not one per viewer) diff --git a/server/src/index.ts b/server/src/index.ts index 595fb16..2b81c29 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -509,21 +509,21 @@ setInterval(() => PartyStreamService.reapIdleSessions(30 * 60 * 1000), 5 * 60 * // (periodic Progress reports only, no explicit start/stop). Best-effort: // PlaystateService.reportProgress swallows its own failures, so one bad // token or a transient Emby hiccup never stops the rest of this sweep. -// MediaSourceId comes from PartyStreamService's own cache, read-only -// (getCachedMediaSourceId never negotiates) - it's a required param on -// the real Progress endpoint (confirmed against Emby's OpenAPI spec, -// see PlaystateService.ts), and skipping a party entirely when nothing's -// cached yet (no one has hit play this session) is simpler and cheaper -// than this sweep ever negotiating a stream of its own just to get one. +// MediaSourceId/PlaySessionId come from PartyStreamService's own cache, +// read-only (getCachedPlaybackIds never negotiates) - both matter to the +// real Progress endpoint in practice (see PlaystateService.ts), and +// skipping a party entirely when nothing's cached yet (no one has hit +// play this session) is simpler and cheaper than this sweep ever +// negotiating a stream of its own just to get them. setInterval(() => { for (const party of PartyService.listActive()) { const state = PartyService.getCurrentPlaybackState(party); if (!state) continue; // nothing currently playing (e.g. an empty music queue) - const mediaSourceId = PartyStreamService.getCachedMediaSourceId(party.id); - if (!mediaSourceId) continue; // nothing negotiated yet this session - Progress requires it + const playbackIds = PartyStreamService.getCachedPlaybackIds(party.id); + if (!playbackIds) continue; // nothing negotiated yet this session - Progress needs it for (const member of party.members.values()) { if (!member.connection || !member.embyToken) continue; - PlaystateService.reportProgress(member.embyToken, member.userId, state.itemId, mediaSourceId, state.positionTicks, state.isPaused); + PlaystateService.reportProgress(member.embyToken, member.userId, state.itemId, playbackIds.mediaSourceId, playbackIds.playSessionId, state.positionTicks, state.isPaused); } } }, 20 * 1000); diff --git a/server/src/services/CatalogService.ts b/server/src/services/CatalogService.ts index c4ebe18..6d01d4b 100644 --- a/server/src/services/CatalogService.ts +++ b/server/src/services/CatalogService.ts @@ -17,6 +17,7 @@ const WATCH_PARTY_TYPES = new Set(['Movie', 'Episode', 'Video', 'MusicVideo', 'A export interface NegotiatedStreamTarget { targetPath: string; mediaSourceId: string; + playSessionId: string; } // Emby's /Views can return all sorts of things beyond actual media @@ -309,13 +310,21 @@ class CatalogServiceClass { * encoding option this app has no visibility into, sidesteps the * entire class of failure. * - * Returns `mediaSourceId` alongside the target path, not just the - * path - PartyStreamService caches both, and PlaystateService's own - * Progress report needs a real MediaSourceId too (a required query - * parameter on that endpoint, confirmed against Emby's real OpenAPI - * spec - see PlaystateService.ts). Resolving it here, once per - * negotiation, means the playstate sweep never has to make its own - * extra PlaybackInfo round trip just to get it. + * Returns `mediaSourceId`/`playSessionId` alongside the target path, + * not just the path - PartyStreamService caches all three, and + * PlaystateService's own Progress report needs both too: MediaSourceId + * is a required query parameter there (confirmed against Emby's real + * OpenAPI spec), and PlaySessionId - though only optional per that + * same spec - turned out to matter in practice too: a real report sent + * without one hit Emby's own "Value cannot be null (Parameter 'key')" + * (a bare .NET Dictionary null-key exception) - the exact error this + * app already ran into once before, in the stream-negotiation fallback + * URL above, for the same underlying reason (see that TODO(verify) + * comment) - so Emby's real Progress handler evidently keys some + * internal lookup (an active transcode/session entry) on it even + * though the spec doesn't mark it required. Resolving both here, once + * per negotiation, means the playstate sweep never has to make its own + * extra PlaybackInfo round trip just to get them. */ async negotiateStreamTarget(session: EmbySession, itemId: string, isAudio: boolean, audioStreamIndex: string, maxStreamingBitrate: string): Promise { // Applied to the negotiation itself, not just the final URL - a pick @@ -346,7 +355,7 @@ class CatalogServiceClass { const params = new URLSearchParams(urlQuery || ''); this.applyPlaybackOverrides(params, audioStreamIndex, maxStreamingBitrate); this.logStreamRequest(itemId, 'negotiated', params); - return { targetPath: `${urlPath}?${params}`, mediaSourceId: source.Id || '' }; + return { targetPath: `${urlPath}?${params}`, mediaSourceId: source.Id || '', playSessionId: info.PlaySessionId || '' }; } const params = new URLSearchParams({ @@ -358,7 +367,7 @@ class CatalogServiceClass { }); this.applyPlaybackOverrides(params, audioStreamIndex, maxStreamingBitrate); this.logStreamRequest(itemId, 'fallback', params); - return { targetPath: `/${isAudio ? 'Audio' : 'Videos'}/${itemId}/master.m3u8?${params}`, mediaSourceId: source.Id || '' }; + return { targetPath: `/${isAudio ? 'Audio' : 'Videos'}/${itemId}/master.m3u8?${params}`, mediaSourceId: source.Id || '', playSessionId: info.PlaySessionId || '' }; } /** diff --git a/server/src/services/PartyStreamService.ts b/server/src/services/PartyStreamService.ts index d132d86..a31a1f8 100644 --- a/server/src/services/PartyStreamService.ts +++ b/server/src/services/PartyStreamService.ts @@ -13,11 +13,13 @@ interface StreamSession { targetPath: string; /** Alongside targetPath, from the same negotiation - see * CatalogService.negotiateStreamTarget's own comment. Surfaced - * read-only via getCachedMediaSourceId() below for PlaystateService's - * own Progress report, which needs it too (a required query param on - * that endpoint) but must never trigger a negotiation of its own just - * to get it - see index.ts's periodic sweep. */ + * read-only, together, via getCachedPlaybackIds() below for + * PlaystateService's own Progress report, which needs both too (see + * that method's own comment for why) but must never trigger a + * negotiation of its own just to get them - see index.ts's periodic + * sweep. */ mediaSourceId: string; + playSessionId: string; lastAccessUtc: number; } @@ -65,23 +67,28 @@ class PartyStreamServiceClass { return existing.targetPath; } - const { targetPath, mediaSourceId } = await CatalogService.negotiateStreamTarget(session, itemId, isAudio, audioStreamIndex, maxStreamingBitrate); - this.sessions.set(party.id, { itemId, audioStreamIndex, maxStreamingBitrate, targetPath, mediaSourceId, lastAccessUtc: Date.now() }); + const { targetPath, mediaSourceId, playSessionId } = await CatalogService.negotiateStreamTarget(session, itemId, isAudio, audioStreamIndex, maxStreamingBitrate); + this.sessions.set(party.id, { itemId, audioStreamIndex, maxStreamingBitrate, targetPath, mediaSourceId, playSessionId, lastAccessUtc: Date.now() }); return targetPath; } /** - * Read-only: the current MediaSourceId behind this party's cached - * stream, or null if nothing has been negotiated yet this session - * (e.g. the party just started and no one has hit play). Deliberately - * does NOT call getOrNegotiate/touch lastAccessUtc - this is a passive - * lookup for index.ts's periodic playstate sweep, not real streaming - * traffic, and must never itself keep an otherwise-idle session alive - * (see reapIdleSessions) or trigger a fresh Emby negotiation on its - * own timer tick. + * Read-only: the MediaSourceId/PlaySessionId behind this party's + * cached stream, or null if nothing has been negotiated yet this + * session (e.g. the party just started and no one has hit play). + * Deliberately does NOT call getOrNegotiate/touch lastAccessUtc - this + * is a passive lookup for index.ts's periodic playstate sweep, not + * real streaming traffic, and must never itself keep an otherwise-idle + * session alive (see reapIdleSessions) or trigger a fresh Emby + * negotiation on its own timer tick. Returns both together, from the + * same cache entry, rather than two separate lookups - they always + * come from the same negotiation, and PlaystateService needs both at + * once anyway (see its own comment for why PlaySessionId turned out to + * matter too, not just MediaSourceId). */ - getCachedMediaSourceId(partyId: string): string | null { - return this.sessions.get(partyId)?.mediaSourceId ?? null; + getCachedPlaybackIds(partyId: string): { mediaSourceId: string; playSessionId: string } | null { + const session = this.sessions.get(partyId); + return session ? { mediaSourceId: session.mediaSourceId, playSessionId: session.playSessionId } : null; } /** GET /api/catalog/party-stream/:partyId/master.m3u8 - the caller diff --git a/server/src/services/PlaystateService.ts b/server/src/services/PlaystateService.ts index 51e5673..223f800 100644 --- a/server/src/services/PlaystateService.ts +++ b/server/src/services/PlaystateService.ts @@ -26,6 +26,24 @@ import { Logger } from '../Logger'; * (PlaylistIndex/Shuffle/SleepTimerMode/...), none of them these. A * missing required query parameter almost certainly meant every one of * these calls was failing outright. + * + * Round 2, from a real request logged after that fix shipped: still a + * 400, now `Value cannot be null. (Parameter 'key')` - a bare .NET + * `Dictionary` null-key exception, and *not* the earlier problem + * (MediaSourceId was present and well-formed in the logged request). + * This exact message already burned this app once before, in + * CatalogService.negotiateStreamTarget's hand-built fallback URL (see + * its own TODO(verify) comment) - diagnosed there as Emby's server + * indexing some internal state by a param this app hadn't supplied. + * `PlaySessionId` is the standout candidate: it's how Emby/Jellyfin + * clients normally correlate a progress report with the specific + * playback/transcode session it belongs to, and evidently something on + * Emby's side keys a lookup on it even though the OpenAPI spec doesn't + * mark it required. Now sent too - see PartyStreamService + * .getCachedPlaybackIds, which resolves it the exact same way + * MediaSourceId already was (both come from the one PlaybackInfo + * negotiation CatalogService.negotiateStreamTarget already does for the + * party's shared stream). */ class PlaystateServiceClass { /** @@ -33,10 +51,10 @@ class PlaystateServiceClass { * a 4xx/5xx) is caught and logged here, never thrown - this runs on a * timer with no request to fail, and one member's bad token or one * transient Emby hiccup must never stop the rest of the sweep, let - * alone touch real playback. `mediaSourceId` comes from - * PartyStreamService's own cache (see index.ts's sweep) - resolving a - * fresh one here would mean an extra PlaybackInfo round trip on every - * tick, for a value the party's already-negotiated stream already + * alone touch real playback. `mediaSourceId`/`playSessionId` come from + * PartyStreamService's own cache (see index.ts's sweep) - resolving + * fresh ones here would mean an extra PlaybackInfo round trip on every + * tick, for values the party's already-negotiated stream already * produced for free. * * Every call logs the exact outgoing request (method, path, query - @@ -46,9 +64,10 @@ class PlaystateServiceClass { * to answer "is this actually reaching Emby, and with what" without * digging through EmbyClient's own generic failure log. */ - async reportProgress(embyToken: string, userId: string, itemId: string, mediaSourceId: string, positionTicks: number, isPaused: boolean): Promise { + async reportProgress(embyToken: string, userId: string, itemId: string, mediaSourceId: string, playSessionId: string, positionTicks: number, isPaused: boolean): Promise { const params = new URLSearchParams({ MediaSourceId: mediaSourceId, + PlaySessionId: playSessionId, PositionTicks: String(Math.round(positionTicks)), IsPaused: String(isPaused) }); diff --git a/server/test/catalogService.test.ts b/server/test/catalogService.test.ts index 0b607d2..93a07dc 100644 --- a/server/test/catalogService.test.ts +++ b/server/test/catalogService.test.ts @@ -47,11 +47,12 @@ describe('CatalogService', () => { PlaySessionId: 'sess1' })); - const { targetPath, mediaSourceId } = await CatalogService.negotiateStreamTarget(session, 'item1', false, '2', ''); + const { targetPath, mediaSourceId, playSessionId } = await CatalogService.negotiateStreamTarget(session, 'item1', false, '2', ''); assert.ok(targetPath.includes('AudioStreamIndex=2')); assert.ok(!targetPath.includes('Subtitle'), 'this method never touches subtitle params at all'); assert.equal(mediaSourceId, 'src1', 'PlaystateService\'s own Progress report needs this too - see PartyStreamService'); + assert.equal(playSessionId, 'sess1', 'so does this - see PlaystateService\'s own "Value cannot be null" comment'); }); it('negotiateStreamTarget\'s hand-built fallback picks /Audio/ (and drops VideoCodec) when isAudio is true', async (t) => { @@ -66,10 +67,12 @@ describe('CatalogService', () => { it('negotiateStreamTarget\'s hand-built fallback picks /Videos/ (with VideoCodec) when isAudio is false', async (t) => { t.mock.method(EmbyClient, 'fetch', async () => jsonResponse({ MediaSources: [{ Id: 'src3' }], PlaySessionId: 'sess3' })); - const { targetPath } = await CatalogService.negotiateStreamTarget(session, 'movie1', false, '', ''); + const { targetPath, mediaSourceId, playSessionId } = await CatalogService.negotiateStreamTarget(session, 'movie1', false, '', ''); assert.ok(targetPath.startsWith('/Videos/movie1/master.m3u8')); assert.ok(targetPath.includes('VideoCodec=h264')); + assert.equal(mediaSourceId, 'src3'); + assert.equal(playSessionId, 'sess3', 'the hand-built fallback branch must surface this too, not just the negotiated-TranscodingUrl one'); }); it('negotiateStreamTarget applies a quality (MaxStreamingBitrate) pick the same way as an audio pick', async (t) => { diff --git a/server/test/partyStreamService.test.ts b/server/test/partyStreamService.test.ts index 45562b1..3cf0ef0 100644 --- a/server/test/partyStreamService.test.ts +++ b/server/test/partyStreamService.test.ts @@ -40,7 +40,7 @@ describe('PartyStreamService.getOrNegotiate', () => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: '/Videos/movie1/master.m3u8?PlaySessionId=sess1', mediaSourceId: 'src1' }; + return { targetPath: '/Videos/movie1/master.m3u8?PlaySessionId=sess1', mediaSourceId: 'src1', playSessionId: 'sess1' }; }); const party = PartyService.create('u-alice', 'Alice', 'movie1', 'Un Film', '/img/movie1', null, 'video'); @@ -49,7 +49,7 @@ describe('PartyStreamService.getOrNegotiate', () => { assert.equal(negotiateCount, 1, 'the second call must reuse the cached target, not negotiate again'); assert.equal(first, second); - assert.equal(PartyStreamService.getCachedMediaSourceId(party.id), 'src1', 'cached alongside the target - see PlaystateService'); + assert.deepEqual(PartyStreamService.getCachedPlaybackIds(party.id), { mediaSourceId: 'src1', playSessionId: 'sess1' }, 'cached alongside the target - see PlaystateService'); PartyStreamService.endSession(party.id); }); @@ -58,7 +58,7 @@ describe('PartyStreamService.getOrNegotiate', () => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: `/Videos/movie2/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}` }; + return { targetPath: `/Videos/movie2/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}`, playSessionId: `sess${negotiateCount}` }; }); const party = PartyService.create('u-alice', 'Alice', 'movie2', 'Un Film 2', '/img/movie2', null, 'video'); @@ -77,7 +77,7 @@ describe('PartyStreamService.getOrNegotiate', () => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: `/Videos/x/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}` }; + return { targetPath: `/Videos/x/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}`, playSessionId: `sess${negotiateCount}` }; }); const party = PartyService.create('u-alice', 'Alice', 'ep1', 'S01E01', '/img/ep1', [ @@ -96,7 +96,7 @@ describe('PartyStreamService.getOrNegotiate', () => { let capturedArgs: unknown[] = []; t.mock.method(CatalogService, 'negotiateStreamTarget', async (...args: unknown[]) => { capturedArgs = args; - return { targetPath: '/Audio/t1/master.m3u8', mediaSourceId: 'src-t1' }; + return { targetPath: '/Audio/t1/master.m3u8', mediaSourceId: 'src-t1', playSessionId: 'sess-t1' }; }); const party = PartyService.create('u-alice', 'Alice', 't1', 'Track One', '/img/t1', null, 'music'); @@ -123,7 +123,7 @@ describe('PartyStreamService failure recovery', () => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: `/Videos/movie3/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}` }; + return { targetPath: `/Videos/movie3/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}`, playSessionId: `sess${negotiateCount}` }; }); t.mock.method(EmbyClient, 'proxyResponse', async (_req: Request, res: Response) => { (res as unknown as { statusCode: number }).statusCode = 502; // simulate Emby having ended this session on its own @@ -131,7 +131,7 @@ describe('PartyStreamService failure recovery', () => { const party = PartyService.create('u-alice', 'Alice', 'movie3', 'Un Film 3', '/img/movie3', null, 'video'); await PartyStreamService.serveMasterPlaylist(fakeReq(), fakeRes(), party, FAKE_SESSION); - assert.equal(PartyStreamService.getCachedMediaSourceId(party.id), null, 'a failed response must drop the MediaSourceId too, not just the target path'); + assert.equal(PartyStreamService.getCachedPlaybackIds(party.id), null, 'a failed response must drop the MediaSourceId/PlaySessionId too, not just the target path'); await PartyStreamService.serveMasterPlaylist(fakeReq(), fakeRes(), party, FAKE_SESSION); assert.equal(negotiateCount, 2, 'a failed response must not be cached - the next request has to renegotiate'); @@ -143,7 +143,7 @@ describe('PartyStreamService failure recovery', () => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: '/Videos/movie4/master.m3u8', mediaSourceId: 'src-movie4' }; + return { targetPath: '/Videos/movie4/master.m3u8', mediaSourceId: 'src-movie4', playSessionId: 'sess-movie4' }; }); t.mock.method(EmbyClient, 'proxyResponse', async () => {}); // res.statusCode stays 200, the fakeRes() default @@ -162,15 +162,15 @@ describe('PartyStreamService lifecycle', () => { assert.doesNotThrow(() => PartyStreamService.endSession('no-such-party')); }); - it('getCachedMediaSourceId is null for a party that never negotiated a stream - never negotiates one itself', () => { - assert.equal(PartyStreamService.getCachedMediaSourceId('no-such-party'), null); + it('getCachedPlaybackIds is null for a party that never negotiated a stream - never negotiates one itself', () => { + assert.equal(PartyStreamService.getCachedPlaybackIds('no-such-party'), null); }); it('reapIdleSessions ends sessions past the idle cutoff, leaves a fresh one alone', async (t) => { let negotiateCount = 0; t.mock.method(CatalogService, 'negotiateStreamTarget', async () => { negotiateCount += 1; - return { targetPath: `/Videos/x/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}` }; + return { targetPath: `/Videos/x/master.m3u8?call=${negotiateCount}`, mediaSourceId: `src${negotiateCount}`, playSessionId: `sess${negotiateCount}` }; }); const party = PartyService.create('u-eve', 'Eve', 'movie5', 'Un Film 5', '/img/movie5', null, 'video'); diff --git a/server/test/playstateService.test.ts b/server/test/playstateService.test.ts index a2f404f..f1e1b53 100644 --- a/server/test/playstateService.test.ts +++ b/server/test/playstateService.test.ts @@ -19,15 +19,19 @@ describe('PlaystateService.reportProgress', () => { return new Response('{}', { status: 200 }); }); - await PlaystateService.reportProgress('tok-alice', 'u-alice', 'item1', 'src1', 12_345_678, false); + await PlaystateService.reportProgress('tok-alice', 'u-alice', 'item1', 'src1', 'sess1', 12_345_678, false); // Query params, not a JSON body - confirmed against Emby's real // OpenAPI spec (see PlaystateService.ts's own comment): the // original body-based guess was missing a required MediaSourceId - // entirely and would have failed outright. + // entirely and would have failed outright. PlaySessionId came next + // (round 2 - see the same comment): optional per the spec, but a + // real request without one still hit Emby's own "Value cannot be + // null (Parameter 'key')". const [path, query] = capturedPath.split('?'); assert.equal(path, '/Users/u-alice/PlayingItems/item1/Progress'); assert.equal(new URLSearchParams(query).get('MediaSourceId'), 'src1'); + assert.equal(new URLSearchParams(query).get('PlaySessionId'), 'sess1'); assert.equal(new URLSearchParams(query).get('PositionTicks'), '12345678'); assert.equal(new URLSearchParams(query).get('IsPaused'), 'false'); assert.equal(capturedToken, 'tok-alice'); @@ -41,7 +45,7 @@ describe('PlaystateService.reportProgress', () => { return new Response('{}', { status: 200 }); }); - await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 100.7, true); + await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 'sess1', 100.7, true); const query = capturedPath.split('?')[1]; assert.equal(new URLSearchParams(query).get('PositionTicks'), '101'); @@ -50,7 +54,7 @@ describe('PlaystateService.reportProgress', () => { it('swallows a failure instead of throwing - this runs on a timer with nothing to catch it', async (t) => { t.mock.method(EmbyClient, 'fetch', async () => { throw new Error('Emby unreachable'); }); - await assert.doesNotReject(() => PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 0, false)); + await assert.doesNotReject(() => PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 'sess1', 0, false)); }); // Logging: the whole point is being able to answer "what did this @@ -61,12 +65,13 @@ describe('PlaystateService.reportProgress', () => { t.mock.method(console, 'log', (line: string) => { logged.push(line); }); t.mock.method(EmbyClient, 'fetch', async () => new Response('{}', { status: 200 })); - await PlaystateService.reportProgress('super-secret-token', 'u-alice', 'item1', 'src1', 12_345_678, false); + await PlaystateService.reportProgress('super-secret-token', 'u-alice', 'item1', 'src1', 'sess1', 12_345_678, false); const requestLine = logged.find((l) => l.includes('[watchparty:playstate]') && l.includes('POST')); assert.ok(requestLine, 'the request itself must be logged'); assert.ok(requestLine!.includes('/Users/u-alice/PlayingItems/item1/Progress')); assert.ok(requestLine!.includes('MediaSourceId=src1')); + assert.ok(requestLine!.includes('PlaySessionId=sess1')); assert.ok(requestLine!.includes('PositionTicks=12345678')); assert.ok(requestLine!.includes('IsPaused=false')); assert.ok(!requestLine!.includes('super-secret-token'), 'the token travels as a header, never logged into the request line'); @@ -77,7 +82,7 @@ describe('PlaystateService.reportProgress', () => { t.mock.method(console, 'log', (line: string) => { logged.push(line); }); t.mock.method(EmbyClient, 'fetch', async () => new Response(null, { status: 204 })); // 204 is a null-body status - a non-null body here would throw - await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 0, false); + await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 'sess1', 0, false); const outcomeLine = logged.find((l) => l.includes('[watchparty:playstate]') && l.includes('204')); assert.ok(outcomeLine, 'the real status code must be logged'); @@ -85,12 +90,12 @@ describe('PlaystateService.reportProgress', () => { assert.ok(!outcomeLine!.includes('FAILED')); }); - it('logs a non-ok response as FAILED, with its real HTTP status - the exact case the original wrong request shape would have hit', async (t) => { + it('logs a non-ok response as FAILED, with its real HTTP status - the exact case both wrong request shapes hit for real', async (t) => { const logged: string[] = []; t.mock.method(console, 'log', (line: string) => { logged.push(line); }); - t.mock.method(EmbyClient, 'fetch', async () => new Response('bad request', { status: 400 })); + t.mock.method(EmbyClient, 'fetch', async () => new Response("Value cannot be null. (Parameter 'key')", { status: 400 })); - await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 0, false); + await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 'sess1', 0, false); const outcomeLine = logged.find((l) => l.includes('[watchparty:playstate]') && l.includes('400')); assert.ok(outcomeLine, 'the real status code must be logged'); @@ -102,7 +107,7 @@ describe('PlaystateService.reportProgress', () => { t.mock.method(console, 'log', (line: string) => { logged.push(line); }); t.mock.method(EmbyClient, 'fetch', async () => { throw new Error('Emby unreachable'); }); - await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 0, false); + await PlaystateService.reportProgress('tok', 'u1', 'item1', 'src1', 'sess1', 0, false); const outcomeLine = logged.find((l) => l.includes('[watchparty:playstate]') && l.includes('threw')); assert.ok(outcomeLine, 'a thrown error must still be logged, not just swallowed silently');