diff --git a/README.md b/README.md index fc3401c..ea0b02a 100644 --- a/README.md +++ b/README.md @@ -279,6 +279,9 @@ Then put it behind your existing reverse proxy like everything else (was still each viewer's own local choice until now); fixed the audio picker briefly showing an empty option on first load — see docs/architecture.md +- [x] Fixed catalog browsing feeling slow: image thumbnails were never + cacheable (Emby's own Cache-Control/ETag were silently dropped) — + see docs/architecture.md - [ ] Try a full party end-to-end with two real accounts (still the main gap) - [ ] Tune transcoding target / support direct play when possible diff --git a/docs/architecture.md b/docs/architecture.md index 54587a6..8ada078 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2286,6 +2286,56 @@ select restores the party's current text or burn-in pick correctly on load; and the audio select now lands on a real first option instead of an empty one when `streamAudioIndex` is `null`. +## Catalog browsing was slow: images were never cacheable + +Reported directly ("l'api du contenu des catalogues est hyper lente"). +`CatalogService`'s own listing methods (`getLibraryItems`, `getArtists`, +etc.) were already fine - each is exactly one Emby call, no per-item +follow-up requests, and timed directly against the real server a +300-item library listing (`getLibraryItems`'s own `Limit: 300`) came +back in well under half a second. The real cost was downstream of that: +every one of those up to 300 tiles needs its own thumbnail, each +proxied through `EmbyClient.proxyImage` → `proxyResponse`, and +`proxyResponse` was silently dropping every caching signal Emby sent - +confirmed directly against the real server that an image response +already carries `Cache-Control: public` and a real `ETag`, but this +method only ever forwarded `content-type`/`content-range`/ +`accept-ranges`. With no validator and no cache policy ever reaching +the browser, it had no choice but to re-download every single thumbnail +in full, from scratch, on every visit to every library, home-page +carousel, series/season/album page - the same images, over and over, +compounding across however many tiles a page shows. + +**Fixed both directions of the same problem.** `EmbyClient.fetch` +already accepted `range` to forward; it now also accepts +`ifNoneMatch`/`ifModifiedSince`, sent as `If-None-Match`/ +`If-Modified-Since` - and a `304` (the entire point of sending those) +is explicitly excluded from the failure-logging path (`!res.ok` is true +for a 304, but it's Emby correctly saying "unchanged", not an error +worth `[watchparty:emby]`). `proxyResponse` now reads the browser's own +`If-None-Match`/`If-Modified-Since` request headers and forwards them +to Emby, and forwards `cache-control`/`etag`/`last-modified` back from +Emby's response, alongside the existing `content-range`/`accept-ranges` +pair. Confirmed end to end against the real server, not just reasoned +about: a plain image GET comes back `200` with `Cache-Control: public` ++ a real `ETag`; the *same* request repeated with that `ETag` as +`If-None-Match` comes back a real `304` with a zero-byte body. Harmless +for everything else this method proxies (video/audio segments, HLS +playlists, subtitles) - it only ever forwards whatever Emby itself +decided, never invents a policy, so something Emby doesn't mark +cacheable (a per-session transcode) simply never gets these headers set +at all, same as before. + +Verified: full typecheck/test/build on both projects. New +`EmbyClient.proxyResponse` tests (this method had never been exercised +directly before - every existing test that touches it mocks it away +entirely to isolate whatever calls it) cover forwarding the cache +headers on a real bodied response, forwarding the browser's conditional +headers to Emby and relaying a real 304 with no body, and that a +response with neither header (most proxied paths) doesn't choke or +invent one; `EmbyClient.fetch` tests cover a 304 not hitting the +failure log and the two new headers being sent only when given. + ## Known limitations - Playback always goes through `hls.js` against whatever Emby's diff --git a/server/src/emby/EmbyClient.ts b/server/src/emby/EmbyClient.ts index 9698421..35aa8a9 100644 --- a/server/src/emby/EmbyClient.ts +++ b/server/src/emby/EmbyClient.ts @@ -14,25 +14,34 @@ import { Logger } from '../Logger'; */ export class EmbyClient { /** Fetches a path from Emby with the given user's token, forwarding - * Range (needed for direct-play video seeking) when the browser sent one. - * `method`/`body` are for PlaystateService's own POST calls (see - * PlaystateService.ts) - every other existing call site only ever - * passed `range`, so this stays a plain GET for them, unaffected. - * `body`, when given, is JSON-encoded here so every caller passes a - * plain object rather than remembering to stringify it themselves. - * Every Emby call in this app goes through here - see logErrorResponse - * below for what happens when Emby answers with anything but success. */ - static async fetch(pathAndQuery: string, token: string, options: { range?: string; method?: string; body?: unknown } = {}): Promise { + * Range (needed for direct-play video seeking) when the browser sent + * one, and If-None-Match/If-Modified-Since (needed for images to + * revalidate as a cheap 304 instead of re-downloading the full body + * every time - see proxyResponse's own comment on caching) when the + * browser sent those. `method`/`body` are for PlaystateService's own + * POST calls (see PlaystateService.ts) - every other existing call + * site only ever passed `range`, so this stays a plain GET for them, + * unaffected. `body`, when given, is JSON-encoded here so every + * caller passes a plain object rather than remembering to stringify + * it themselves. Every Emby call in this app goes through here - see + * logErrorResponse below for what happens when Emby answers with + * anything but success. */ + static async fetch(pathAndQuery: string, token: string, options: { range?: string; ifNoneMatch?: string; ifModifiedSince?: string; method?: string; body?: unknown } = {}): Promise { const url = pathAndQuery.startsWith('http') ? pathAndQuery : `${EMBY_SERVER_URL}${pathAndQuery}`; const headers: Record = { 'X-Emby-Token': token }; if (options.range) headers.Range = options.range; + if (options.ifNoneMatch) headers['If-None-Match'] = options.ifNoneMatch; + if (options.ifModifiedSince) headers['If-Modified-Since'] = options.ifModifiedSince; let body: string | undefined; if (options.body !== undefined) { headers['Content-Type'] = 'application/json'; body = JSON.stringify(options.body); } const res = await fetch(url, { headers, method: options.method, body }); - if (!res.ok) await EmbyClient.logErrorResponse(res, pathAndQuery); + // A 304 (the whole point of the conditional headers above) is + // Emby correctly saying "unchanged" - not a failure worth this + // app's own error log, even though !res.ok is true for it. + if (!res.ok && res.status !== 304) await EmbyClient.logErrorResponse(res, pathAndQuery); return res; } @@ -92,21 +101,44 @@ export class EmbyClient { /** * Proxies an Emby response to an Express response: forwards status, - * content-type, and the headers that make byte-range seeking work - * (content-range/accept-ranges), rewriting HLS playlists in transit and - * streaming everything else (images, video/audio segments) through - * untouched - no buffering, so this doesn't hold a whole segment in - * memory. `routeKey` is only ever read back out by `rewritePlaylist` - * above - a real itemId for `proxyImage`/`proxySubtitle` (which never - * actually reach that branch - wrong content-type/extension - so it's - * inert for them either way) and `party.id` for `PartyStreamService`, - * whose whole point is routing every member back to the *same* shared - * stream regardless of which item they think they're watching. + * content-type, the headers that make byte-range seeking work + * (content-range/accept-ranges), and the ones that make caching work + * (cache-control/etag/last-modified - see below), rewriting HLS + * playlists in transit and streaming everything else (images, video/ + * audio segments) through untouched - no buffering, so this doesn't + * hold a whole segment in memory. `routeKey` is only ever read back + * out by `rewritePlaylist` above - a real itemId for `proxyImage`/ + * `proxySubtitle` (which never actually reach that branch - wrong + * content-type/extension - so it's inert for them either way) and + * `party.id` for `PartyStreamService`, whose whole point is routing + * every member back to the *same* shared stream regardless of which + * item they think they're watching. + * + * Caching: added after a "the catalog is really slow" report traced + * to this - confirmed directly against the real server that Emby + * already answers an image request with `Cache-Control: public` and a + * real `ETag`, but neither ever reached the browser: this method used + * to forward only content-type/content-range/accept-ranges, silently + * dropping both, so every tile in every library grid re-downloaded + * its full image on every single visit with no way to even ask Emby + * "has this changed?". Now forwards cache-control/etag/last-modified + * too, and forwards the browser's own conditional headers (If-None- + * Match/If-Modified-Since - see EmbyClient.fetch above) through to + * Emby on the way there, so a cache hit round-trips as a real, + * bodyless 304 instead of the full image every time. Harmless for the + * other things this method proxies (playlists/segments/subtitles) - + * this only ever forwards whatever Emby itself already decided, never + * invents a policy, so a transcoded segment Emby marks non-cacheable + * stays exactly that. */ static async proxyResponse(req: Request, res: Response, pathAndQuery: string, token: string, routeKey: string): Promise { let embyRes: globalThis.Response; try { - embyRes = await EmbyClient.fetch(pathAndQuery, token, { range: req.headers.range }); + embyRes = await EmbyClient.fetch(pathAndQuery, token, { + range: req.headers.range, + ifNoneMatch: req.headers['if-none-match'], + ifModifiedSince: req.headers['if-modified-since'] + }); } catch { res.status(502).json({ error: 'Serveur Emby injoignable.' }); return; @@ -115,7 +147,7 @@ export class EmbyClient { res.status(embyRes.status); const contentType = embyRes.headers.get('content-type') || ''; if (contentType) res.setHeader('Content-Type', contentType); - for (const h of ['content-range', 'accept-ranges']) { + for (const h of ['content-range', 'accept-ranges', 'cache-control', 'etag', 'last-modified']) { const v = embyRes.headers.get(h); if (v) res.setHeader(h, v); } diff --git a/server/test/embyClient.test.ts b/server/test/embyClient.test.ts index 3c7eb85..ed9d8ab 100644 --- a/server/test/embyClient.test.ts +++ b/server/test/embyClient.test.ts @@ -6,8 +6,55 @@ // what EmbyClient.fetch itself calls. import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; +import { PassThrough } from 'stream'; +import type { Request, Response } from 'express'; import { EmbyClient } from '../src/emby/EmbyClient'; +/** A real Writable (via PassThrough) so `.pipe(res)` in proxyResponse's + * streaming branch works exactly as it would against a real Express + * response, decorated with the handful of Express Response methods + * proxyResponse actually calls - same "just enough of the real + * interface, cast through `unknown`" approach as partyStreamService + * .test.ts's own fakeRes, but pipe-able too since, unlike that file, + * EmbyClient.proxyResponse itself (not a mock of it) runs for real + * here. Returned already cast to plain `Response` for passing into + * proxyResponse; callers read back `.statusCode`/`.headers`/`.body` + * through the same `unknown` cast the fake was built from. */ +type FakeRes = { statusCode: number; headers: Record; body: Promise }; + +function fakeRes(): Response { + const stream = new PassThrough(); + const chunks: Buffer[] = []; + stream.on('data', (c) => chunks.push(c)); + const body = new Promise((resolve) => stream.on('end', () => resolve(Buffer.concat(chunks).toString()))); + const headers: Record = {}; + // Untyped (`any`) for the construction phase only - trying to keep this + // typed as the full `Response & FakeRes` intersection while assigning + // its methods one at a time confuses TS into thinking each assignment + // narrows the *whole* intersection, not just the one property. `inspect` + // and the `Response` return type below are the only two shapes anything + // outside this function ever sees it as. + const fake: any = stream; + fake.headers = headers; + fake.statusCode = 200; + fake.body = body; + fake.status = (code: number) => { fake.statusCode = code; return fake; }; + fake.setHeader = (name: string, value: string) => { headers[name.toLowerCase()] = value; return fake; }; + fake.send = (text: string) => { stream.end(text); return fake; }; + return fake as Response; +} + +/** Reads back the tracking fields fakeRes() attached, through the same + * `unknown` cast - `res` itself is typed as plain `Response` at every + * call site (matching what proxyResponse's own signature expects). */ +function inspect(res: Response): FakeRes { + return res as unknown as FakeRes; +} + +function fakeReq(headers: Record = {}): Request { + return { headers } as unknown as Request; +} + describe('EmbyClient.fetch', () => { it('logs Emby\'s status and response body on a failure, without consuming the body the real caller still needs', async (t) => { const logged: string[] = []; @@ -50,6 +97,34 @@ describe('EmbyClient.fetch', () => { assert.equal(logged.filter((l) => l.includes('[watchparty:emby]')).length, 0); }); + it('does not log a 304 as a failure - a cache hit is the whole point of sending If-None-Match, not an error', async (t) => { + const logged: string[] = []; + t.mock.method(console, 'log', (line: string) => { logged.push(line); }); + t.mock.method(globalThis, 'fetch', async () => new Response(null, { status: 304 })); + + await EmbyClient.fetch('/Items/1/Images/Primary', 'tok', { ifNoneMatch: '"abc"' }); + + assert.equal(logged.filter((l) => l.includes('[watchparty:emby]')).length, 0, 'a 304 is !res.ok but must not hit the failure log'); + }); + + it('sends If-None-Match/If-Modified-Since only when given - a plain GET stays exactly as before', async (t) => { + let capturedInit: RequestInit | undefined; + t.mock.method(globalThis, 'fetch', async (_url: string, init: RequestInit) => { + capturedInit = init; + return new Response('{}', { status: 200 }); + }); + + await EmbyClient.fetch('/Items/1/Images/Primary', 'tok', { ifNoneMatch: '"abc"', ifModifiedSince: 'Mon, 01 Jan 2024 00:00:00 GMT' }); + let headers = capturedInit?.headers as Record; + assert.equal(headers['If-None-Match'], '"abc"'); + assert.equal(headers['If-Modified-Since'], 'Mon, 01 Jan 2024 00:00:00 GMT'); + + await EmbyClient.fetch('/Users/u1/Views', 'tok'); + headers = capturedInit?.headers as Record; + assert.equal(headers['If-None-Match'], undefined); + assert.equal(headers['If-Modified-Since'], undefined); + }); + it('JSON-encodes a body and sets method/Content-Type when a caller passes one - generic plumbing', async (t) => { // Not PlaystateService's own call any more - its real Progress // endpoint takes query params, not a body (confirmed against Emby's @@ -86,3 +161,66 @@ describe('EmbyClient.fetch', () => { assert.equal((capturedInit?.headers as Record)['Content-Type'], undefined); }); }); + +// proxyResponse itself, never exercised for real before (every other test +// file that touches it mocks it away entirely, to isolate whatever calls +// it) - specifically the caching path added after a real "the catalog is +// really slow" report traced to this method silently dropping Emby's own +// Cache-Control/ETag on every image it proxied. Mocks the GLOBAL fetch, +// same as EmbyClient.fetch's own tests above - proxyResponse is real code +// running on top of it, not itself replaced. +describe('EmbyClient.proxyResponse', () => { + it('forwards Cache-Control/ETag/Last-Modified from a real (bodied) response, alongside the existing content-range/accept-ranges', async (t) => { + t.mock.method(globalThis, 'fetch', async () => new Response('fake-jpeg-bytes', { + status: 200, + headers: { + 'Content-Type': 'image/jpeg', + 'Cache-Control': 'public', + 'ETag': '"abc123"', + 'Accept-Ranges': 'bytes' + } + })); + + const res = fakeRes(); + await EmbyClient.proxyResponse(fakeReq(), res, '/Items/1/Images/Primary', 'tok', '1'); + const got = inspect(res); + + assert.equal(got.statusCode, 200); + assert.equal(got.headers['cache-control'], 'public'); + assert.equal(got.headers['etag'], '"abc123"'); + assert.equal(got.headers['accept-ranges'], 'bytes'); + assert.equal(await got.body, 'fake-jpeg-bytes', 'the body itself must still stream through untouched'); + }); + + it('forwards the browser\'s own If-None-Match/If-Modified-Since to Emby, and relays a real 304 with no body', async (t) => { + let capturedInit: RequestInit | undefined; + t.mock.method(globalThis, 'fetch', async (_url: string, init: RequestInit) => { + capturedInit = init; + return new Response(null, { status: 304, headers: { ETag: '"abc123"' } }); + }); + + const req = fakeReq({ 'if-none-match': '"abc123"', 'if-modified-since': 'Mon, 01 Jan 2024 00:00:00 GMT' }); + const res = fakeRes(); + await EmbyClient.proxyResponse(req, res, '/Items/1/Images/Primary', 'tok', '1'); + const got = inspect(res); + + const sentHeaders = capturedInit?.headers as Record; + assert.equal(sentHeaders['If-None-Match'], '"abc123"', 'the browser\'s validator must reach Emby, or Emby can never actually answer 304'); + assert.equal(sentHeaders['If-Modified-Since'], 'Mon, 01 Jan 2024 00:00:00 GMT'); + assert.equal(got.statusCode, 304); + assert.equal(got.headers['etag'], '"abc123"'); + }); + + it('does not choke on no Cache-Control/ETag at all - most proxied responses (video segments, subtitles) never had one', async (t) => { + t.mock.method(globalThis, 'fetch', async () => new Response('vtt content', { status: 200, headers: { 'Content-Type': 'text/vtt' } })); + + const res = fakeRes(); + await EmbyClient.proxyResponse(fakeReq(), res, '/Videos/1/x/Subtitles/2/Stream.vtt', 'tok', '1'); + const got = inspect(res); + + assert.equal(got.statusCode, 200); + assert.equal(got.headers['cache-control'], undefined); + assert.equal(got.headers['etag'], undefined); + assert.equal(await got.body, 'vtt content'); + }); +});