From f69725503f9d604c1c349f615336417c02c772f5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 3 Nov 2026 19:15:18 +0100 Subject: [PATCH] fix(plex): a finished HLS session outlives a throttled heartbeat --- backend/app/main.py | 2 +- backend/app/plex/stream.py | 24 ++++++++++++-------- backend/app/routes/plex/playback.py | 3 ++- backend/tests/test_hls_reaper.py | 35 +++++++++++++++++++++++------ 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/backend/app/main.py b/backend/app/main.py index f11aad8b..30905c8d 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -151,7 +151,7 @@ async def _reap_hls_sessions() -> None: somebody pressed play again or the process restarted. A FINISHED session is reaped on the short rule here, not kept for the full idle window: an open - player checkpoints every 10 s and that route touches the session, so a paused viewer is no longer + player keeps checkpointing and that route touches the session, so a paused viewer is no longer invisible to this pass (see `reap_idle`). Keeping every finished session for ten minutes instead held one whole segmented copy of a film per item on the download volume.""" while True: diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 50dcf8a0..1d36ffb0 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -1160,9 +1160,13 @@ def sweep_orphans(older_than: float | None = None) -> int: return dropped -# How long a session whose ffmpeg has FINISHED may go unfetched before a new session request reaps -# it (see reap_idle). -_FINISHED_IDLE_S = 30 +# The slowest heartbeat a live player sends: a browser cuts the timers of a hidden page that makes no +# sound to one a minute (measured on Chromium: a paused and a muted player, about a minute after +# hiding; a hidden player with sound keeps its 10 s). +_THROTTLED_HEARTBEAT_S = 60 +# How long a session whose ffmpeg has FINISHED may go without a signal before `reap_idle` takes it: +# the beat before the reaper looks, one lost on the way, and the time the next one takes to arrive. +_FINISHED_IDLE_S = 3 * _THROTTLED_HEARTBEAT_S def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int: @@ -1171,15 +1175,17 @@ def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int: A finished session is also what a PAUSED viewer has: a remux runs far faster than realtime, so ffmpeg is done a minute or two in and writes ENDLIST; hls.js then stops asking for the playlist, - and with its forward buffer full a paused player fetches nothing at all. With the 30 s rule that - killed a one-minute pause, and playback died ~30 s after resuming. + and with its forward buffer full a paused player fetches nothing at all. - What makes the short rule safe again is that fetching is no longer the only signal: a player - parked MID-ITEM checkpoints its position every 10 s, and that route calls `touch_session`, so a - viewer who is merely paused says so far more often than this rule asks. The heartbeat + What makes the short rule safe is that fetching is not the only signal: a player parked MID-ITEM + checkpoints its position and that route calls `touch_session`. That beat is every 10 s only while + the page is visible or making sound; a hidden, silent one sends it once a minute, which a 30 s + rule killed whenever the reaper happened to run more than 30 s after the beat (measured: a paused + 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 - scrubs back in anyway. Do not restate this as "every 10 s whether or not it is playing": + 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 one a whole segmented copy of the film on a volume shared with the download center.""" diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index 043dc26a..fed16d5b 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -323,7 +323,8 @@ def item_progress( `_should_push_resume`). `final` (sent on pause / seek / pagehide / unmount) pushes immediately and resets the throttle window, so Plex sees a settled position the instant the user pauses or seeks.""" it = item_or_404(db, rating_key) - # A player parked mid-item — playing or PAUSED — checkpoints every 10 s, so this is the signal + # A player parked mid-item — playing or PAUSED — keeps checkpointing (every 10 s, once a minute + # in a hidden silent page), so this is the signal # that such a viewer still wants their HLS session: the one thing `last_access` could not see, # since it moved only on a file fetch and a paused player fetches nothing (see # stream.reap_idle). It is NOT every open player: `usePlexProgress.flush` sends nothing once the diff --git a/backend/tests/test_hls_reaper.py b/backend/tests/test_hls_reaper.py index fb1ffe2b..c4b14368 100644 --- a/backend/tests/test_hls_reaper.py +++ b/backend/tests/test_hls_reaper.py @@ -5,11 +5,11 @@ POST /session: after the last playback, an abandoned session (its ffmpeg still w film, its directory a whole segmented copy of it) lived until somebody pressed play again. And it must not take a PAUSED viewer's session: a remux finishes long before the film does, and a -paused player with a full buffer fetches nothing, so "finished and unfetched for 30 s" does not mean +paused player with a full buffer fetches nothing, so "finished and unfetched for a while" does not mean "abandoned". An earlier fix bought that safety by keeping EVERY finished session for the full ten-minute idle window — one segmented copy of a film per item, on the volume the download center shares. Its successor replaced the blanket retention with the signal that already existed: a player -parked mid-item checkpoints its position every 10 s, paused or not, and that route says "still +parked mid-item keeps checkpointing its position, paused or not, and that route says "still here" (it stops once the item is finished, where reaping is correct). Both call sites are pinned here, because the bug each time was that only one of them moved: the @@ -81,6 +81,9 @@ class _Proc: return 0 +_PAST_THE_RULE = stream._FINISHED_IDLE_S + 5 + + class _Session: def __init__(self, tmp_path, name: str, finished: bool, idle_s: float): self.key = name @@ -98,11 +101,29 @@ def sessions(monkeypatch): def test_the_periodic_pass_reaps_a_finished_session_nobody_has_claimed(tmp_path, sessions): - sessions["12345"] = _Session(tmp_path, "done", finished=True, idle_s=45) + sessions["12345"] = _Session(tmp_path, "done", finished=True, idle_s=_PAST_THE_RULE) assert stream.reap_idle() == 1 assert sessions == {} +@pytest.mark.parametrize("idle_s", [65, 125]) +def test_a_heartbeat_throttled_to_once_a_minute_keeps_a_finished_session( + tmp_path, sessions, idle_s +): + """A hidden page that makes no sound gets its timers cut to one a minute (measured on a paused and + on a muted player, about a minute after hiding), so that is the viewer's heartbeat there: the + reaper can see a beat over a minute old, and two if one is lost on the way.""" + s = _Session(tmp_path, "hidden", finished=True, idle_s=idle_s) + sessions[(1, "12345")] = s + assert stream.reap_idle() == 0 + assert (1, "12345") in sessions and s.dir.exists() and not s.proc.killed + + +def test_an_abandoned_finished_session_is_still_gone_within_minutes(tmp_path, sessions): + sessions[(1, "12345")] = _Session(tmp_path, "gone", finished=True, idle_s=185) + assert stream.reap_idle() == 1 + + def test_the_periodic_pass_still_reaps_what_nobody_has_touched_for_the_idle_limit( tmp_path, sessions ): @@ -141,7 +162,7 @@ def test_every_reap_says_which_rule_took_the_session(tmp_path, sessions, caplog) def test_touching_a_session_keeps_it_through_the_short_rule(tmp_path, sessions): - s = _Session(tmp_path, "paused", finished=True, idle_s=90) + s = _Session(tmp_path, "paused", finished=True, idle_s=_PAST_THE_RULE) sessions[(1, "12345")] = s stream.touch_session(1, "12345") assert stream.reap_idle() == 0 @@ -184,7 +205,7 @@ def test_a_paused_viewers_checkpoint_keeps_their_session_alive( user = User(email="paused@example.com") db.add(user) db.commit() - s = _Session(tmp_path, "paused", finished=True, idle_s=90) + s = _Session(tmp_path, "paused", finished=True, idle_s=_PAST_THE_RULE) sessions[(user.id, "paused1")] = s resp = _client(db, user, override_dep).post( f"/api/plex/item/{it.rating_key}/progress", @@ -199,7 +220,7 @@ def test_starting_a_session_leaves_another_viewers_paused_one_alone( db, monkeypatch, tmp_path, sessions, override_dep ): """The opportunistic sweep this route used to run was not scoped to the key being started: it - walked the WHOLE registry with the 30 s finished rule, so pressing play on any item killed a + walked the WHOLE registry with the finished rule, so pressing play on any item killed a paused viewer's session — the very case the retention fix set out to protect.""" monkeypatch.setattr(state, "_configured_cache", True) monkeypatch.setattr(plex_paths, "local_media_path", lambda db_, fp: "/local/a.mkv") @@ -207,7 +228,7 @@ def test_starting_a_session_leaves_another_viewers_paused_one_alone( user = User(email="starter@example.com") db.add(user) db.commit() - paused = _Session(tmp_path, "paused", finished=True, idle_s=90) + paused = _Session(tmp_path, "paused", finished=True, idle_s=_PAST_THE_RULE) sessions[(999, "someone-else")] = paused def fake_start(db_, item, user_id, start, audio=None, aoff=0.0, multi=False, transcode=False):