From df2997e9f3ba68776c85d5966cf8625067ad3792 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 3 Nov 2026 19:20:22 +0100 Subject: [PATCH] fix(plex): a lost HLS session is reclaimed at the first answer The route answers 410 for a session it no longer has and keeps 404 for a segment not there yet. hls.js retries even a 4xx on a stream with one video variant, so the player waited about half a minute before asking again, and minutes in a throttled page. --- backend/app/plex/stream.py | 2 +- backend/app/routes/plex/playback.py | 14 +++-- backend/tests/test_plex_hls_file_route.py | 18 +++--- frontend/src/lib/hlsEngine.test.ts | 72 ++++++++++++++++++++++ frontend/src/lib/hlsEngine.ts | 73 ++++++++++++++--------- 5 files changed, 135 insertions(+), 44 deletions(-) diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 1d36ffb0..e491773b 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -1184,7 +1184,7 @@ def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int: session gone that way, and one that lived on the same cadence because its phase was luckier). So the rule is sized to the throttled beat (`_THROTTLED_HEARTBEAT_S`). The heartbeat stops once the item is finished or before a position is known (`usePlexProgress.flush`) — both - states where reaping is correct, and where the player's 404 → reclaim recovers a viewer who + states where reaping is correct, and where the player's 410 → reclaim recovers a viewer who scrubs back in anyway. Do not restate this as "whether or not it is playing": a review found that claim in two comments and not in the code. An earlier blanket `reap_abandoned` bought the same safety by keeping every finished session for ten minutes — each diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index fed16d5b..47477427 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -330,7 +330,7 @@ def item_progress( # stream.reap_idle). It is NOT every open player: `usePlexProgress.flush` sends nothing once the # finish latch is up, or before the first position is known. Those are the states in which # reaping is the right answer anyway, and a viewer who scrubs back into a reaped session gets - # the same 404 → reclaim the player already handles. + # the same 410 → reclaim the player already handles. plex_stream.touch_session(user.id, str(rating_key)) position = max(0, int(body.position_seconds)) duration = int(body.duration_seconds) @@ -610,7 +610,9 @@ def stream_hls_file( is per (key, viewer, offset, tag) — so resolving by the key alone handed the old player the new session's `seg_12.ts`, produced from a different offset under `-copyts`: wrong bytes with a 200, which the player has no way to notice. A request for a - session that is no longer the registered one is a 404, and the player reclaims at the playhead. + session that is no longer the registered one is a 410, and the player reclaims at the playhead. + 410, not 404: a 404 here means "that segment is not there yet", worth hls.js's retries, and the + player can only read the status (a gone session retried for half a minute behind that 404). That reclaim is also why a request with no live session is no longer auto-started here: the auto-start could only ever begin at 0, silently throwing away the viewer's position.""" @@ -626,7 +628,7 @@ def stream_hls_file( raise not_found("file") s = plex_stream.current_session(user.id, str(rating_key)) if s is None or s.id != session_id: - raise refused(404, "session_gone") + raise refused(410, "session_gone") path = s.dir / filename # Only a name THIS session can actually produce is worth waiting for: its own playlists, and a # segment of one of its own renditions whose predecessor is already on disk (ffmpeg writes them @@ -649,13 +651,13 @@ def stream_hls_file( except OSError: # The session was torn down between the wait and the read — teardown moves the whole # directory aside and deletes it, and nothing holds `_lock` across those two steps. It has - # to present as the same 404 a gone session does: the player's recovery is keyed on the - # STATUS (`code === 404`), so + # to present as the same 410 a gone session does: the player's recovery is keyed on the + # STATUS, so # a 500 here skipped the reclaim and ended playback with "couldn't play" instead of # restarting at the playhead. Ordinary trigger: `loadSession` awaits the new session — which # kills and rmtrees the old one — before destroying the old hls instance, so the outgoing # instance's in-flight segment request races the rmtree on every seek-restart. - raise refused(404, "session_gone") from None + raise refused(410, "session_gone") from None return Response( content=content, media_type=_HLS_CT[ext], diff --git a/backend/tests/test_plex_hls_file_route.py b/backend/tests/test_plex_hls_file_route.py index bf58d4b4..c60ff471 100644 --- a/backend/tests/test_plex_hls_file_route.py +++ b/backend/tests/test_plex_hls_file_route.py @@ -88,10 +88,10 @@ def test_another_viewers_session_of_the_same_item_is_not_this_viewers(client, tm (d / "index.m3u8").write_bytes(b"#EXTM3U") sessions[(7, "m1")] = _Session("sid-a", d) r = client.get("/api/plex/stream/m1/hls/sid-a/index.m3u8") - assert r.status_code == 404, r.text + assert r.status_code == 410, r.text -def test_a_segment_of_a_replaced_session_is_a_404_not_the_new_sessions_bytes( +def test_a_segment_of_a_replaced_session_is_gone_not_the_new_sessions_bytes( client, tmp_path, sessions ): """Viewer A is at 0:00; viewer B seeks to 25:00 and their session takes the registry slot. A's @@ -102,15 +102,15 @@ def test_a_segment_of_a_replaced_session_is_a_404_not_the_new_sessions_bytes( (theirs / "seg_12.ts").write_bytes(b"theirs") sessions[(VIEWER, "m1")] = _Session("sid-b", theirs) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_12.ts") - assert r.status_code == 404, r.text + assert r.status_code == 410, r.text assert b"theirs" not in r.content -def test_a_playlist_with_no_live_session_is_a_404_not_a_silent_restart_at_zero( +def test_a_playlist_with_no_live_session_is_gone_not_a_silent_restart_at_zero( client, monkeypatch, sessions ): """The entry playlist used to auto-start a session when none was registered — always at 0.0, - silently throwing away the viewer's position. A 404 sends the player through its own reclaim, + silently throwing away the viewer's position. A 410 sends the player through its own reclaim, which restarts at the playhead.""" started: list[float] = [] monkeypatch.setattr( @@ -119,7 +119,7 @@ def test_a_playlist_with_no_live_session_is_a_404_not_a_silent_restart_at_zero( lambda *a, **k: started.append(a[3] if len(a) > 3 else 0.0), ) r = client.get("/api/plex/stream/m1/hls/sid-gone/index.m3u8") - assert r.status_code == 404, r.text + assert r.status_code == 410, r.text assert started == [] @@ -235,12 +235,12 @@ def test_an_absurd_segment_index_is_a_404_not_a_500(client, tmp_path, sessions, assert r.status_code == 404, r.text -def test_a_session_torn_down_during_the_read_is_a_404_not_a_500( +def test_a_session_torn_down_during_the_read_is_gone_not_a_500( client, monkeypatch, tmp_path, sessions ): """Teardown moves the whole directory aside and nothing holds `_lock` across the wait and the read, so the file can vanish in between — on every seek-restart, since `loadSession` awaits the new - session before destroying the old hls instance. The player's recovery is keyed on the 404 + session before destroying the old hls instance. The player's recovery is keyed on the STATUS, so a 500 here skipped the reclaim and ended playback with "couldn't play".""" d = tmp_path / "m1_0_0_+0.00" d.mkdir() @@ -249,7 +249,7 @@ def test_a_session_torn_down_during_the_read_is_a_404_not_a_500( # The file existed when the wait checked, and is gone by the time the route reads it. monkeypatch.setattr(plex_stream, "wait_for", lambda *a, **k: True) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_12.ts") - assert r.status_code == 404, r.text + assert r.status_code == 410, r.text def test_a_dead_producer_is_not_waited_out(client, tmp_path, sessions): diff --git a/frontend/src/lib/hlsEngine.test.ts b/frontend/src/lib/hlsEngine.test.ts index 703933ba..25b3feb2 100644 --- a/frontend/src/lib/hlsEngine.test.ts +++ b/frontend/src/lib/hlsEngine.test.ts @@ -65,6 +65,27 @@ class FakeHls implements HlsPort { lost404() { this.lost(404); } + /** A request answered "this session is gone". hls.js only reports it (not fatal) and retries for + * half a minute: with one video variant it has nowhere else to go. */ + gone() { + this.handlers.onError({ + fatal: false, + network: true, + status: 410, + type: "networkError", + details: "fragLoadError", + }); + } + /** hls.js reports a failed request it will retry by itself. */ + retrying(status: number) { + this.handlers.onError({ + fatal: false, + network: true, + status, + type: "networkError", + details: "fragLoadError", + }); + } /** hls.js gave up on a playlist or fragment with this answer (null: no answer at all). */ lost(status: number | null) { this.handlers.onError({ @@ -418,6 +439,57 @@ describe("a session the server no longer has (404)", () => { }); }); +describe("a session the server says is gone (410)", () => { + async function playing() { + const s = setup(); + s.engine.start("A", 600); + await s.asks[0]!.resolve(hls("A", 600)); + s.hlses[0]!.handlers.onManifestParsed(); + s.media.fire("canplay"); + s.state.position = 1299; + return s; + } + + it("is reclaimed at the first answer, not after hls.js's half minute of retries", async () => { + const { asks, hlses } = await playing(); + hlses[0]!.gone(); + expect(asks[1]!.startAt).toBe(1299); + }); + + it("keeps the running picture until the new session answers", async () => { + const { engine, hlses } = await playing(); + expect(engine.getSnapshot().ready).toBe(true); + hlses[0]!.gone(); + expect(engine.getSnapshot().ready).toBe(true); + }); + + it("asks once, whatever the same instance still reports", async () => { + const { asks, hlses } = await playing(); + hlses[0]!.gone(); + hlses[0]!.gone(); + hlses[0]!.lost(410); + expect(asks).toHaveLength(2); + }); + + it(`shares the reclaim budget, and says so after ${MAX_SESSION_RECLAIMS}`, async () => { + const { engine, asks, hlses } = setup(); + engine.start("A", 600); + for (let i = 0; i <= MAX_SESSION_RECLAIMS; i++) { + await asks[i]!.resolve(hls("A", 600)); + hlses[i]!.gone(); + } + expect(asks).toHaveLength(MAX_SESSION_RECLAIMS + 1); + expect(engine.getSnapshot().error).toBe("generic"); + }); + + it("leaves a segment that is merely not there yet to hls.js's retries", async () => { + const { engine, asks, hlses } = await playing(); + hlses[0]!.retrying(404); + expect(asks).toHaveLength(1); + expect(engine.getSnapshot().error).toBeNull(); + }); +}); + describe("a server that is restarting (a deploy)", () => { // The gateway answers 502 while the app restarts, and nothing at all may answer (status 0): the // stream is not broken, the server is away. The buffer keeps playing meanwhile. diff --git a/frontend/src/lib/hlsEngine.ts b/frontend/src/lib/hlsEngine.ts index 64aa7305..99f14a70 100644 --- a/frontend/src/lib/hlsEngine.ts +++ b/frontend/src/lib/hlsEngine.ts @@ -14,7 +14,7 @@ import type { HlsLevelInfo } from "./plexStats"; // once: twenty presses of ← at key-repeat pace past the buffer were twenty requests, twenty ffmpeg // spawns, and five seconds before the picture moved again. -// Reclaims of a 404'd HLS session, within `RECLAIM_WINDOW_MS`, before the player stops and says so. +// Reclaims of a lost HLS session, within `RECLAIM_WINDOW_MS`, before the player stops and says so. // Two covers the real case (the session was reaped, once) without letting a server that cannot serve // the stream keep the viewer on a spinner (see `onHlsError`). export const MAX_SESSION_RECLAIMS = 2; @@ -33,6 +33,8 @@ export const RECLAIM_WINDOW_MS = 120_000; export const AWAY_WINDOW_MS = 180_000; const AWAY_RETRY_MS = [2_000, 4_000, 8_000, 15_000]; const SERVER_AWAY = new Set([0, 502, 503, 504]); +// The server's answer for a session it no longer has; its 404 means a segment not there yet. +const SESSION_GONE = 410; /** Why a session could not play, in the three buckets the player has a sentence for. */ type EngineError = "transcode" | "notFound" | "generic"; @@ -183,7 +185,7 @@ export class HlsEngine { // the way back to the first item then sent a second request beside the one still out for it. private readonly inFlight = new Map(); private queued: Request | null = null; - // How often a session the server answered 404 for may be reclaimed (see `onHlsError`). + // How often a session the server lost may be reclaimed (see `onHlsError`). private reclaims = { n: 0, at: 0 }; // Since when the server has been away, and how often it was asked since (see `serverAway`). private away: { since: number; tries: number } | null = null; @@ -459,42 +461,57 @@ export class HlsEngine { }; } - // A fatal HLS error (the session died, the file became unreadable) would otherwise leave the - // spinner up forever — it is said instead. + // A fatal HLS error (the file became unreadable) would otherwise leave the spinner up forever — it + // is said instead. // - // Except a 404: the session is gone, not broken. Either the server reaped it (a pause longer than - // its idle limit, a deploy) — hls.js never retries a 4xx, so this used to end playback with - // "couldn't play" after the buffer ran out — or it was superseded before it ever played (two - // requests for one item: the server's LAST spawn wins). The server answers a superseded session - // exactly so the client asks again; the reclaim is that asking. + // Except a session the server no longer has (410): gone, not broken. Either the server reaped it + // or restarted, or it was superseded before it ever played (two requests for one item: the + // server's LAST spawn wins), and the reclaim is the asking again that answer wants. It does not + // wait for hls.js to give up: with one video variant it has nothing to switch to, so it retries + // even a 4xx, six times over about half a minute (measured: seven requests, then the reclaim), and + // in a throttled page once a minute (four minutes). Quiet, since the buffer is still playing. // - // Bounded by the budget, not by whether the session got as far as playing: that belongs to ONE - // session, and every restart builds a fresh one, so it could never bound a loop. The server also answers - // 404 for "that segment is not there YET" — indistinguishable on the wire — so a transcode falling - // behind realtime restarted ffmpeg forever behind a spinner that never became an error. + // And except a fatal 404: a segment still not there after those retries, so a transcode has fallen + // behind, and asking again is all that is left. + // + // Both are bounded by the budget, not by whether the session got as far as playing: that belongs to + // ONE session, and every restart builds a fresh one, so it could never bound a loop. A transcode + // falling behind realtime restarted ffmpeg forever behind a spinner that never became an error. private onHlsError(req: Request, media: HTMLMediaElement, e: HlsFailure): void { this.deps.onHlsError?.(e); - if (!e.fatal || !this.current(req)) return; + if (!this.current(req)) return; + if (e.network && e.status === SESSION_GONE) { + this.reclaim(req, media, this.snapshot.ready); + return; + } + if (!e.fatal) return; if (e.network && SERVER_AWAY.has(e.status ?? 0) && this.serverAway(req)) return; if (e.network && e.status === 404) { - const now = this.deps.now(); - // A new window when the last loss was long enough ago. The anchor moves with EVERY loss: left - // at the window's start, the window tumbled instead of rolling, and any loss period longer - // than half the window reset the budget on every second loss (measured: 200 losses 70 s apart - // → 200 reclaims, never an error). - if (now - this.reclaims.at > RECLAIM_WINDOW_MS) this.reclaims.n = 0; - this.reclaims.at = now; - if (this.reclaims.n < MAX_SESSION_RECLAIMS) { - this.reclaims.n += 1; - // The server dropped it already; nothing to stop. - this.attachedSession = null; - this.start(req.id, this.playhead(), this.wantsPlay(media)); - return; - } + this.reclaim(req, media, false); + return; } this.set({ error: "generic" }); } + /** Ask for `req`'s item again where the viewer is, while the budget lasts; then say it cannot play. */ + private reclaim(req: Request, media: HTMLMediaElement, quiet: boolean): void { + const now = this.deps.now(); + // A new window when the last loss was long enough ago. The anchor moves with EVERY loss: left + // at the window's start, the window tumbled instead of rolling, and any loss period longer + // than half the window reset the budget on every second loss (measured: 200 losses 70 s apart + // → 200 reclaims, never an error). + if (now - this.reclaims.at > RECLAIM_WINDOW_MS) this.reclaims.n = 0; + this.reclaims.at = now; + if (this.reclaims.n >= MAX_SESSION_RECLAIMS) { + this.set({ error: "generic" }); + return; + } + this.reclaims.n += 1; + // The server dropped it already; nothing to stop. + this.attachedSession = null; + this.start(req.id, this.playhead(), this.wantsPlay(media), quiet); + } + /** The server is away (see `AWAY_WINDOW_MS`): ask for `req`'s item again where the viewer is, * after a delay that grows with each try. False once it has been away too long — the caller then * says it cannot play. A restart the viewer asks for meanwhile (a seek, another item) replaces the