From d89d887a9b45d0c95491c8ec6fcd12cddf1a1f5e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 05:50:11 +0200 Subject: [PATCH] fix(plex): the reclaim budget bounds what it says it bounds, and a refused finish is not performed anyway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MAX_SESSION_RECLAIMS` was cleared by the first fragment of any session, and a reclaimed session essentially always delivers one — `start_session` does not return until its first segment exists. So the budget only ever bit where a session parsed its manifest and then died before a single fragment, which is not either case its own comment names. Measured: 40 loss cycles produced 41 sessions and never an error. A transcode falling behind realtime, or two viewers of one rating_key taking turns, respawned ffmpeg indefinitely — the round-5 behaviour, still there behind the guard added to stop it. It counts within a rolling window now. Two losses inside two minutes is a session that keeps being lost; one an hour apart is not. And the window is reset per item, because the player stays mounted across a queue advance — otherwise a budget spent on one episode would deny the next its first legitimate reclaim. Separately: `finish`'s rollback re-performed the write it was taking back. On the play-to-the-end path the position it restores IS the end of the media, and the backend reads a position within its finish margin as a finish — so the branch named "take the finish back" set watched, cleared the position and scrobbled to Plex through a second route, while the caller's error toast was already on screen saying the mark had failed. There is no resume point at the end of the media to restore; the cache drop is the whole of the rollback there. The credits-skip caller captures a mid-episode position and still restores it, which is the case that needs it. 7 tests, mutation-checked: the FRAG_LOADED reset, the missing window, and the unguarded rollback each red on the test that names them. --- .../components/PlexPlayer.hls.dom.test.tsx | 43 +++++++++++++++---- .../PlexPlayer.session.dom.test.tsx | 37 +++++++++++++--- frontend/src/components/PlexPlayer.tsx | 30 +++++++++---- frontend/src/lib/plexPlayerHooks.ts | 17 +++++++- 4 files changed, 102 insertions(+), 25 deletions(-) diff --git a/frontend/src/components/PlexPlayer.hls.dom.test.tsx b/frontend/src/components/PlexPlayer.hls.dom.test.tsx index abc0dae7..cc78a1fd 100644 --- a/frontend/src/components/PlexPlayer.hls.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.hls.dom.test.tsx @@ -174,16 +174,19 @@ describe("an HLS session the server no longer has", () => { expect(session).toHaveBeenCalledTimes(3); }); - it("starts the budget over once a fragment actually loads", async () => { - // A session lost once an hour is not a session that cannot be served; the only proof a reclaim - // worked is a fragment arriving. + it("keeps giving up even though every reclaimed session plays a fragment first", async () => { + // The budget used to be cleared by the first FRAG_LOADED of any session — and a reclaimed + // session essentially always delivers one, because `start_session` does not return until its + // first segment exists. So it bounded NEITHER case its own comment named: measured at 40 loss + // cycles, the player opened 41 sessions and never showed an error (deep-review round 6). A + // fragment is not proof the session can be served; surviving the window is. const session = mount(); - for (let i = 0; i < 4; i++) { + const v = () => document.querySelector("video") as Media; + for (let i = 0; i < 3; i++) { await waitFor(() => expect(hlsInstances).toHaveLength(i + 1)); act(() => hlsInstances[i]!.emit(Hls.Events.MANIFEST_PARSED)); - const v = document.querySelector("video") as Media; - v._t = 700; - act(() => void v.dispatchEvent(new Event("timeupdate"))); + v()._t = 700; + act(() => void v().dispatchEvent(new Event("timeupdate"))); act(() => hlsInstances[i]!.emit(Hls.Events.FRAG_LOADED, { frag: { type: "main", duration: 6, stats: { total: 1000 } }, @@ -191,8 +194,30 @@ describe("an HLS session the server no longer has", () => { ); act(() => hlsInstances[i]!.emit(Hls.Events.ERROR, lost404)); } - expect(screen.queryByText("plex.player.errGeneric")).toBeNull(); - expect(session).toHaveBeenCalledTimes(5); + await screen.findByText("plex.player.errGeneric"); + expect(session).toHaveBeenCalledTimes(3); + }); + + it("starts the budget over for a loss long after the last one", async () => { + // The other half: a session reaped once an hour is not a session that cannot be served. The + // window is what separates the two, so a spent budget must not follow the viewer around. + vi.useFakeTimers({ shouldAdvanceTime: true, toFake: ["Date"] }); + try { + const session = mount(); + const v = () => document.querySelector("video") as Media; + for (let i = 0; i < 4; i++) { + await waitFor(() => expect(hlsInstances).toHaveLength(i + 1)); + act(() => hlsInstances[i]!.emit(Hls.Events.MANIFEST_PARSED)); + v()._t = 700; + act(() => void v().dispatchEvent(new Event("timeupdate"))); + act(() => hlsInstances[i]!.emit(Hls.Events.ERROR, lost404)); + act(() => void vi.setSystemTime(Date.now() + 10 * 60_000)); + } + expect(screen.queryByText("plex.player.errGeneric")).toBeNull(); + expect(session).toHaveBeenCalledTimes(5); + } finally { + vi.useRealTimers(); + } }); it("is a failure, not a restart, when the session never played", async () => { diff --git a/frontend/src/components/PlexPlayer.session.dom.test.tsx b/frontend/src/components/PlexPlayer.session.dom.test.tsx index fd37d9c8..f1ac961f 100644 --- a/frontend/src/components/PlexPlayer.session.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.session.dom.test.tsx @@ -371,14 +371,39 @@ describe("leaving an item that was played through", () => { endMedia(); await screen.findByText("Title B"); await tick(); - // The position the viewer actually reached is reported, not suppressed by a finish that the - // server never confirmed… - expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([ - ["A", 2995, 3000, true], - ]); - // …and the cached detail is not left claiming watched: only the server is honest now. + // The cached detail is not left claiming watched: only the server is honest now. const cached = qc.getQueryData(qk.plexItem("A", "en")); expect(cached?.status).not.toBe("watched"); + // And the rollback does NOT write the position back. 2995 of 3000 is inside the backend's + // finish margin, so that checkpoint would have set watched / position 0 and scrobbled to Plex + // — performing, through a second route, the very write that was just refused, while the error + // toast said it had failed (round 6). There is no resume point at the end of the media to + // restore; the cache drop is the whole of the rollback here. + expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([]); + }); + + it("does restore the position when the refused finish came from the credits skip", async () => { + // The other caller of the same helper captures a MID-episode position, and there the rollback + // is the point: without it the viewer loses the stretch they watched. One helper, two callers, + // and only the near-end one must hold back. + prefs({ upNext: false, autoSkipCredits: true, autoSkipDelay: 0 }); + const withCredits = { + ...item("A"), + markers: [{ type: "credits", start_s: 2000, end_s: 2990 }], + } as unknown as PlexItemDetail; + const { qc, progress } = mount({ + details: { A: withCredits, B: item("B") }, + sessions: { A: () => Promise.resolve(direct("A")), B: () => new Promise(() => {}) }, + queue: ["A", "B"], + }); + vi.spyOn(api, "plexSetState").mockRejectedValue(new Error("offline")); + await playingAt("/media/A", 2010); // inside the credits → auto-skip advances and marks watched + await screen.findByText("Title B"); + await tick(); + expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([ + ["A", 2010, 3000, true], + ]); + expect(qc.getQueryData(qk.plexItem("A", "en"))?.status).not.toBe("watched"); }); it("records a second viewing when the viewer plays on from the finished frame", async () => { diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 6e815cc8..d566e960 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -75,10 +75,16 @@ import { pushLayer, removeLayer, anyLayerAbove, escapeHandled } from "../lib/lay // The rich info overlay (poster/cast/ratings) reuses the same component as the card's info page. const PlexInfo = lazy(() => import("./PlexInfo")); -// Consecutive reclaims of a 404'd HLS session 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 the ERROR handler). +// Reclaims of a 404'd 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 the ERROR handler). const MAX_SESSION_RECLAIMS = 2; +// A ROLLING window, because "consecutive" could not bound anything: the budget used to be cleared by +// the first fragment of any session, and a reclaimed session essentially always delivers one — +// `start_session` does not return until its first segment exists. So both cases the ERROR handler +// names restarted ffmpeg forever anyway (measured: 40 loss cycles → 41 sessions, never an error). +// Two losses inside two minutes is a session that keeps being lost; one an hour apart is not. +const RECLAIM_WINDOW_MS = 120_000; // The Plex module's rich, full-page player. Plays the LOCAL file: direct-playable files stream raw // (native