From 8a83bc0e9f3d9f4c696462f27206f1d95a4c1084 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 03:47:42 +0200 Subject: [PATCH] fix(plex): a finished item is finished when the server says so, and a lost session is reclaimed a bounded number of times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 5, over round 4's own fixes. - Round 4 added a `finish` latch so a played-through item records no checkpoint at the credits. On a NATURAL play-through it was raised a beat too late: per the HTML spec the browser sets `paused` and fires 'pause' BEFORE 'ended', so the pause checkpoint went out while the latch was still down and raced the watched mark — both requests read "not watched yet" from their own snapshot, so Plex counted one watch twice, and where Plex's reported duration runs more than the finish margin past the real media the checkpoint wrote a resume position AFTER the scrobble. The pause that ends the media no longer flushes. The regression test that was meant to cover this modelled the end as a bare 'ended' event — a browser that does not exist — and is now the real order. - The watched mark was fired and forgotten (`.catch(() => {})`) while the latch patched the cached detail to watched / position 0 regardless, and round 4's once-per-id start guard meant the corrected detail arriving later was never used: an episode whose mark failed reopened at 0:00 with a Watched badge the server disagreed with. `finish` now takes the in-flight write and takes itself back if the server refuses it — the real position is reported, the lying cache entry is dropped, and the failure is said out loud instead of swallowed. Both callers (the natural end and the credits auto-skip) hand it over. - The reclaim of a 404'd HLS session had no bound. `parsed` cannot provide one — it belongs to one hls instance and every restart builds a fresh one — and the server answers 404 both for "the session is gone" and for "that segment is not there yet", so a transcode falling behind realtime, or two viewers of one item taking turns, respawned ffmpeg forever behind a spinner that never became an error. Two consecutive reclaims, reset by the first fragment that actually loads, then the honest failure. Five regression tests, each mutation-checked red on the assertion it is named after. --- .../components/PlexPlayer.hls.dom.test.tsx | 42 ++++++++++++ .../PlexPlayer.session.dom.test.tsx | 67 ++++++++++++++++++- frontend/src/components/PlexPlayer.tsx | 50 +++++++++++--- frontend/src/lib/plexPlayerHooks.ts | 55 ++++++++++++--- 4 files changed, 194 insertions(+), 20 deletions(-) diff --git a/frontend/src/components/PlexPlayer.hls.dom.test.tsx b/frontend/src/components/PlexPlayer.hls.dom.test.tsx index 795a8869..abc0dae7 100644 --- a/frontend/src/components/PlexPlayer.hls.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.hls.dom.test.tsx @@ -153,6 +153,48 @@ describe("an HLS session the server no longer has", () => { expect(screen.queryByText("plex.player.errGeneric")).toBeNull(); }); + it("gives up and says so when the reclaimed session keeps 404ing", async () => { + // `parsed` cannot bound this: it lives on ONE hls instance and every restart builds a fresh one, + // so a session that parses its manifest and then 404s re-arms the guard every cycle. The server + // also answers 404 for "that segment is not there YET" (a transcode falling behind realtime), + // which is indistinguishable on the wire — so the player respawned ffmpeg forever behind a + // spinner that never became an error (round 5). + const session = mount(); + const v = () => document.querySelector("video") as Media; + // The first session plus the two it is allowed to reclaim; the third 404 has nothing left. + for (let i = 0; i < 3; 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)); + } + await screen.findByText("plex.player.errGeneric"); + // Two reclaims, then the honest answer — not a fourth, fifth, hundredth session. + 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. + const session = mount(); + for (let i = 0; i < 4; 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"))); + act(() => + hlsInstances[i]!.emit(Hls.Events.FRAG_LOADED, { + frag: { type: "main", duration: 6, stats: { total: 1000 } }, + }), + ); + act(() => hlsInstances[i]!.emit(Hls.Events.ERROR, lost404)); + } + expect(screen.queryByText("plex.player.errGeneric")).toBeNull(); + expect(session).toHaveBeenCalledTimes(5); + }); + it("is a failure, not a restart, when the session never played", async () => { // Otherwise a session that 404s from the start would restart itself in a loop. const session = mount(); diff --git a/frontend/src/components/PlexPlayer.session.dom.test.tsx b/frontend/src/components/PlexPlayer.session.dom.test.tsx index b33b0e86..fd37d9c8 100644 --- a/frontend/src/components/PlexPlayer.session.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.session.dom.test.tsx @@ -296,6 +296,17 @@ describe("leaving an item that was played through", () => { const prefs = (p: object) => localStorage.setItem("siftlode.plexPlayerPrefs.7", JSON.stringify({ subLang: "en", ...p })); + // The end of the media, in the order the HTML spec mandates: the browser sets `paused`, fires + // 'pause', and only THEN fires 'ended'. Dispatching a bare 'ended' models a browser that does not + // exist, and hid the fact that the pause checkpoint goes out before the finish latch is up + // (deep-review round 5). + const endMedia = () => { + const el = video(); + Object.defineProperty(el, "ended", { configurable: true, get: () => true }); + act(() => void el.dispatchEvent(new Event("pause"))); + act(() => void el.dispatchEvent(new Event("ended"))); + }; + it("settles it as watched when it ended, so Prev does not reopen it at its last seconds", async () => { prefs({ upNext: false }); const { qc, progress } = mount({ @@ -305,7 +316,7 @@ describe("leaving an item that was played through", () => { }); vi.spyOn(api, "plexSetState").mockResolvedValue(undefined as never); await playingAt("/media/A", 2995); - act(() => void video().dispatchEvent(new Event("ended"))); + endMedia(); await screen.findByText("Title B"); await tick(); const cached = qc.getQueryData(qk.plexItem("A", "en")); @@ -344,6 +355,60 @@ describe("leaving an item that was played through", () => { const cached = qc.getQueryData(qk.plexItem("A", "en")); expect([cached?.status, cached?.position_seconds]).toEqual(["watched", 0]); }); + it("takes the finish back when the server refuses the watched mark", async () => { + // The mark was fired and forgotten (`.catch(() => {})`) while the cache was patched to + // watched / position 0 regardless, and the once-per-id start guard meant the corrected detail + // arriving later was never used: the episode reopened at 0:00 with a Watched badge the server + // disagreed with (round 5). + prefs({ upNext: false }); + const { qc, progress } = mount({ + details: { A: item("A"), 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", 2995); + 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. + const cached = qc.getQueryData(qk.plexItem("A", "en")); + expect(cached?.status).not.toBe("watched"); + }); + + it("records a second viewing when the viewer plays on from the finished frame", async () => { + // `finished` is a latch that makes every checkpoint a no-op; the only thing that lowers it is + // the video's own 'play' event. Nothing tested that, so dropping the listener would have + // discarded a whole rewatch with the gate green (round 5). + prefs({ upNext: true }); + const { progress } = mount({ + details: { A: item("A"), B: item("B") }, + sessions: { A: () => Promise.resolve(direct("A")), B: () => new Promise(() => {}) }, + queue: ["A", "B"], + }); + vi.spyOn(api, "plexSetState").mockResolvedValue(undefined as never); + await playingAt("/media/A", 2995); + endMedia(); // the up-next card is up; the item is finished + await tick(); + progress.mockClear(); + + // The viewer cancels the card and rewinds: playing again takes the finish back. + const el = video() as Media; + Object.defineProperty(el, "ended", { configurable: true, get: () => false }); + act(() => void el.dispatchEvent(new Event("play"))); + el._t = 2400; + act(() => void el.dispatchEvent(new Event("timeupdate"))); + act(() => void el.dispatchEvent(new Event("pause"))); + await tick(); + expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([ + ["A", 2400, 3000, true], + ]); + }); }); describe("restarting the session before playback has ticked", () => { diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index a35b9023..6e815cc8 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -27,6 +27,7 @@ import { VolumeX, } from "lucide-react"; import { api, HttpError } from "../lib/api"; +import { notifyActionError } from "../lib/actionErrors"; import { audioOrdForLang, clamp01, @@ -74,6 +75,11 @@ 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). +const MAX_SESSION_RECLAIMS = 2; + // The Plex module's rich, full-page player. Plays the LOCAL file: direct-playable files stream raw // (native