diff --git a/frontend/src/components/PlexPlayer.hls.dom.test.tsx b/frontend/src/components/PlexPlayer.hls.dom.test.tsx index cc78a1fd..1e83fb9e 100644 --- a/frontend/src/components/PlexPlayer.hls.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.hls.dom.test.tsx @@ -220,6 +220,34 @@ describe("an HLS session the server no longer has", () => { } }); + it("gives up on a session lost over and over on a SLOW cadence too", async () => { + // The window's anchor used to be written only where the window RESET, so it marked the start of + // the window instead of the last loss — a tumbling window, not the rolling one the constant is + // named for. Any cadence slower than half the window then cleared the budget on every second + // loss and the bound never bit: a transcode falling ~70s behind realtime — the case the ERROR + // handler itself names — reclaimed forever (measured: 200 losses 70s apart → 200 reclaims, + // never an error). The two cases above space their losses 0s and 10min apart, i.e. both sit + // outside the 60–120s band where the defect lived (deep-review round 7). + vi.useFakeTimers({ shouldAdvanceTime: true, toFake: ["Date"] }); + try { + const session = mount(); + const v = () => document.querySelector("video") as Media; + for (let i = 0; i < 6 && !screen.queryByText("plex.player.errGeneric"); 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() + 70_000)); + } + await screen.findByText("plex.player.errGeneric"); + // Two reclaims, each within two minutes of the loss before it — then the honest answer. + expect(session).toHaveBeenCalledTimes(3); + } finally { + vi.useRealTimers(); + } + }); + 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 f1ac961f..5593d358 100644 --- a/frontend/src/components/PlexPlayer.session.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.session.dom.test.tsx @@ -382,6 +382,44 @@ describe("leaving an item that was played through", () => { expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([]); }); + it("performs the refused mark through no OTHER route while it stays on the finished item", async () => { + // Round 6 guarded the explicit rollback write, but the same catch lowered the play-through + // latch — and that latch is the only gate on the 10s interval, the page-hide beacon and + // `settle`. All three post `absRef`, which on this path IS the end of the media, and the + // backend reads a position within `_FINISH_MARGIN_S` of the duration as a finish: within ten + // seconds of the toast saying the mark had failed, the item was watched, its position cleared + // and a scrobble sent to Plex (deep-review round 7). `upNext` is ON here — the default — so the + // player STAYS on the finished item and those routes are live; the round-6 test above turns it + // off, which tears the effect down before any of them can fire. + prefs({ upNext: true }); + const beacon = vi.spyOn(api, "plexProgressBeacon").mockImplementation(() => {}); + const { qc, progress, unmount } = 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(); // the up-next card is up, the item is finished, and the mark comes back refused + await tick(); + progress.mockClear(); + + // The viewer closes the tab while the failed-mark toast is still up. + act(() => void window.dispatchEvent(new Event("pagehide"))); + expect(beacon).not.toHaveBeenCalled(); + + // The refusal dropped the cached detail — and the open player's own observer refetches it, so + // by the time the viewer leaves there is an entry again. `settle` must not re-assert watched + // onto it: only the server is honest about this item now. + act(() => void qc.setQueryData(qk.plexItem("A", "en"), item("A"))); + + // And leaving the player: `settle` must neither post the end position nor re-assert watched. + unmount(); + await tick(); + expect(progress.mock.calls.filter(([id]) => id === "A")).toEqual([]); + expect(qc.getQueryData(qk.plexItem("A", "en"))?.status).not.toBe("watched"); + }); + 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, diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index d566e960..0b3ed07a 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -435,9 +435,15 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { if (parsed && data.type === Hls.ErrorTypes.NETWORK_ERROR && data.response?.code === 404) { const now = Date.now(); // A new window: the last loss was long enough ago that this is a fresh incident, not the - // same session being lost over and over. - if (now - restartsRef.current.at > RECLAIM_WINDOW_MS) - restartsRef.current = { n: 0, at: now }; + // same session being lost over and over. The anchor moves with EVERY loss, because with + // it left at the window's start the window tumbled instead of rolling: any loss period + // longer than half the window satisfied the reset on every second loss, so the budget + // never reached its bound and the reclaim loop it exists to end ran forever (measured: + // 200 losses 70s apart → 200 reclaims, never an error; 5s apart → 2 and an error, which + // is why the committed test, spacing its cases 0s and 10min apart, could not see it — + // deep-review round 7). + if (now - restartsRef.current.at > RECLAIM_WINDOW_MS) restartsRef.current.n = 0; + restartsRef.current.at = now; if (restartsRef.current.n < MAX_SESSION_RECLAIMS) { restartsRef.current.n += 1; loadSession(playhead(absRef, loadStartRef), !video.paused); diff --git a/frontend/src/lib/plexFinishMargin.contract.test.ts b/frontend/src/lib/plexFinishMargin.contract.test.ts new file mode 100644 index 00000000..c773e626 --- /dev/null +++ b/frontend/src/lib/plexFinishMargin.contract.test.ts @@ -0,0 +1,39 @@ +import { readFileSync } from "node:fs"; +import { expect, it } from "vitest"; + +// One invariant, two languages, no shared wire format — so a test is the only thing that can hold +// them together. +// +// The backend finishes an item when a progress checkpoint lands within `_FINISH_MARGIN_S` of the +// duration: watched, position cleared, scrobbled to Plex. The player's refused-finish rollback must +// therefore never post a position inside that window, or the branch named "take the finish back" +// performs, through the backend, the very write that was just refused (rounds 6 and 7 both landed +// on this). The frontend mirrors the number so it knows where the window starts. +// +// Both were 30 and exactly complementary, but nothing related them, so a backend-only change would +// have re-opened the defect silently — a rule stated in a comment and not enforced in code, the one +// thing this repo's house rules single out. The direction matters: the frontend's margin must be at +// LEAST the backend's. Larger is merely conservative (a rollback skipped where it could have run); +// smaller is the bug. +// +// Both values are read from source rather than imported: `plexPlayerHooks` pulls in the i18n module +// (and with it `localStorage`), and booting half the app to read one integer would make this test +// about the module graph instead of about the two numbers. +function constantIn(path: string, pattern: RegExp): number { + const source = readFileSync(new URL(path, import.meta.url), "utf8"); + const m = pattern.exec(source); + // If this fails the constant was renamed or moved — find it and re-point this test, because the + // invariant it guards did not go away. + expect(m, `${pattern} not found in ${path}`).not.toBeNull(); + return Number(m![1]); +} + +it("never lets the player's finish margin fall below the backend's", () => { + const backend = constantIn( + "../../../backend/app/routes/plex/playback.py", + /^_FINISH_MARGIN_S\s*=\s*(\d+)/m, + ); + const frontend = constantIn("./plexPlayerHooks.ts", /^const FINISH_MARGIN_S\s*=\s*(\d+)/m); + expect(backend).toBeGreaterThan(0); + expect(frontend).toBeGreaterThanOrEqual(backend); +}); diff --git a/frontend/src/lib/plexPlayerHooks.ts b/frontend/src/lib/plexPlayerHooks.ts index 01ccfda3..90020698 100644 --- a/frontend/src/lib/plexPlayerHooks.ts +++ b/frontend/src/lib/plexPlayerHooks.ts @@ -23,9 +23,16 @@ import { PLEX_SCALE, normalizeVolumeIn, stepVolumeIn } from "./playerVolume"; // How close to the end the backend counts as "finished" (`_FINISH_MARGIN_S` in routes/plex/playback): // a progress checkpoint at or past `duration - this` marks the item watched and clears the position. -// Mirrored here because `finish`'s rollback must not accidentally drive that branch. It is a -// one-way safety margin, not a shared contract — if the two ever diverge, the worst case is a -// rollback that is skipped or a resume point set a few seconds from the end. +// Mirrored here because `finish`'s rollback must not accidentally drive that branch. +// +// Drift is NOT symmetric, which an earlier version of this comment had backwards. If THIS value +// grows, the rollback is skipped where it need not be — a lost resume point, annoying. If the +// BACKEND's grows (its own comment invites that tuning by contrasting it with the feed's 10 s), +// this guard starts admitting a rollback the backend reads as a finish, which marks the item +// watched, clears the position and scrobbles to Plex — re-performing the very write that was just +// refused, i.e. exactly the defect this constant was introduced to close. So the invariant is +// one-directional and it is PINNED, not described: `plexFinishMargin.contract.test.ts` reads the +// Python and fails if this value ever drops below it (round 7). const FINISH_MARGIN_S = 30; // A live wall clock for the top bar, re-rendered every 15s (minute resolution is enough for the @@ -111,6 +118,16 @@ export function usePlexProgress(opts: { const finishRef = useRef<(confirmed?: Promise) => void>(() => {}); useEffect(() => { let finished = false; + // Raised when the caller's watched write came back REFUSED. The play-through latch itself stays + // up, because lowering it re-armed three other routes to the same write: the 10s interval, the + // page-hide beacon and `settle` all post `absRef`, which on this path IS the end of the media, + // and the backend reads a position within `_FINISH_MARGIN_S` of the duration as a finish. So the + // branch named "take the finish back" marked the item watched, cleared the position and + // scrobbled to Plex within ten seconds — the refused write performed through a third route, + // under an error toast saying it had failed. Round 6 closed only the explicit rollback write; + // this closes the other three (deep-review round 7). What the refusal does take back is the + // CLAIM: the cache must not say watched, and `settle` must not re-assert it on the way out. + let finishRefused = false; // `final` = a settled checkpoint (pause / page-hide / leaving the player) vs. the periodic tick. // Only final checkpoints get mirrored to a linked Plex account (backend-side debounce), so Plex // sees the resume position when the user actually stops — not a timeline call every 10 seconds. @@ -157,6 +174,7 @@ export function usePlexProgress(opts: { window.addEventListener("pagehide", onHide); const onPlayAgain = () => { finished = false; + finishRefused = false; }; video?.addEventListener("play", onPlayAgain); // Back (unmount) persists the position server-side via flush(), but reopening the same item in @@ -170,9 +188,14 @@ export function usePlexProgress(opts: { // Match the language-keyed detail query (E5 S6) so the optimistic resume patch lands. const key = qk.plexItem(id, plexLang()); if (finished) { - qc.setQueryData(key, (old) => - old ? { ...old, status: "watched", position_seconds: 0 } : old, - ); + // A refused finish claims nothing: the server has the item unwatched at the position the + // rollback restored, and the catch below has already dropped this entry, so the next open + // fetches the truth. Re-patching "watched / 0" here would put back the very lie the + // refusal handler removed. + if (!finishRefused) + qc.setQueryData(key, (old) => + old ? { ...old, status: "watched", position_seconds: 0 } : old, + ); return; } flush(true); @@ -185,14 +208,15 @@ export function usePlexProgress(opts: { finishRef.current = (confirmed?: Promise) => { finished = true; // The latch has to go up BEFORE the media pauses, so it cannot wait for the server. If the - // watched write is then refused, the latch is taken back: the cached detail said watched / - // position 0 while the server still had the item unwatched at the last checkpoint, 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). + // watched write is then refused, what is taken back is the CLAIM, not the latch (see + // `finishRefused`): the cached detail said watched / position 0 while the server still had + // the item unwatched at the last checkpoint, 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). const pos = Math.floor(absRef.current); const dur = durationRef.current; void confirmed?.catch(() => { - finished = false; + finishRefused = true; // Restore the resume point the finish replaced — but ONLY where there is one to restore. // On the play-to-the-end path `pos` IS the end of the media, and the backend reads a // position within `_FINISH_MARGIN_S` of the duration as a finish: it would set watched,