peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(plex): an HLS session's file names are one vocabulary, and a name it cannot produce fails at once

Browse Source 
Round 5 added a guard so a request for a segment beyond the produced region
answers instead of polling out the full timeout — the route is a SYNC one, so
every such wait parks a threadpool worker and, through `current_user`, a pooled
DB connection. Measured against real ffmpeg, the guard could not fire where it
mattered.

`-hls_segment_filename seg_%v_%d.ts` substitutes the VARIANT first: a dual-audio
session writes seg_0_0.ts (video) and seg_a0_0.ts / seg_a1_0.ts (the `name:aN`
renditions). The regex read those as sequence-first, so every video segment
parsed as segment 0 and every audio segment did not parse at all — the guard was
inert on every multi-audio item, which is most of this library. `_seg()` carried
the same inversion: the warm-up waited 8 s for `seg_1_0.ts`, a name ffmpeg never
writes, on every multi-audio session start AND every seek-restart.

The layout is now one constant plus one parser (`segment_name` /
`_SEG_NAME_RE`), with a test pinning them to each other, instead of three
comments that could drift — and they had drifted, all three ways.

The guard is also a positive rule now. It was a blocklist, so anything that was
not a single-audio segment name was waved through: `GET …/anything.m3u8` still
parked a worker for 30 s, while the route's comment claimed a flood of hopeless
names was what it prevented. A session produces its playlists and its segments
and nothing else. `.vtt` leaves `_HLS_CT` with it — ffmpeg runs with `-sn` and
subtitles are a separate route, so that extension could only ever name a file
that will not arrive.

Also here:
- A 5000-digit segment index reached the client as a 500: `int()` on an
  unbounded digit run raises past 4300 digits and only OSError was caught. The
  name is bounded on the route and the index in the regex — the same bug class
  the subtitle routes were bounded for.
- A session torn down between the wait and the read is a 404, not a 500. `_kill`
  rmtrees the directory and no lock spans those two steps; the player's recovery
  is keyed on the STATUS, so a 500 skipped the reclaim and ended playback with
  "couldn't play" instead of restarting at the playhead. The trigger is ordinary
  — `loadSession` awaits the new session before destroying the old hls instance.
- Two comments claimed an open player checkpoints every 10 s "whether or not it
  is playing", which is the stated reason the 30 s finished-idle reap is safe.
  `flush` sends nothing once the finish latch is up, or before a position is
  known. The states it does cover — playing or paused mid-item — are the ones
  the rule needs; the comments now say that instead of more.
- The route's tests leave the DB lane: the route reads nothing from the database
  since the auto-start was removed, and there those four regressions SKIP on a
  machine with no Postgres while pytest prints green.

14 tests, mutation-checked: the old parse, the old warm-up name, the blocklist,
both bounds off the absurd name, the unguarded read, and the route dropping
`proc` — each red on the test that names it.
This commit is contained in:
peter
2026-09-12 05:49:38 +02:00
parent 1f81684b6e
commit 7f17b17c13
3 changed files with 222 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/plex/stream.py
+51 -23
View File
@@ -56,6 +56,10 @@ _SEG_SECONDS = 6
# worse than the old zero-buffer behaviour).
_WARMUP_SEGMENTS = 3
_WARMUP_SEG_TIMEOUT_S = 8.0
# The HLS variant ffmpeg gives the VIDEO rendition of a multi-audio session. `-var_stream_map` names
# each audio rendition (`name:a0`, `a1`, …) but leaves the video one unnamed, so ffmpeg falls back to
# its index for that one. See `segment_name` for why this is a constant and not a comment.
_VIDEO_VARIANT = "0"
_SESSION_IDLE_S = 600 # reap a session with no access for this long
# A session directory is named `{rating_key}_{int(start_s)}_{tag}_{aoff:+.2f}` (see start_session);
# before multi-audio it was just `{rating_key}_{int(start_s)}`. `sweep_orphans` deletes ONLY names
@@ -503,7 +507,9 @@ def _ffmpeg_cmd(
if audio_langs:
# MULTI-RENDITION: map EVERY audio track as an alternate HLS rendition in one group, so hls.js
# switches audio CLIENT-SIDE on the same timeline (no session restart, no drift) — the premium
# audio-switch path. Produces master.m3u8 + stream_%v.m3u8 (v0=video, v1..=audio) + seg_%v_%d.ts.
# audio-switch path. Produces master.m3u8 + one stream_%v.m3u8 and seg_%v_*.ts per variant,
# where %v is the video rendition's index ("0") and each audio rendition's `name:` (a0, a1,
# …) — see `_SEG_NAME_RE`, which is the one place that layout is written down.
args += ["-map", "0:v:0"]
for i in range(len(audio_langs)):
args += ["-map", f"{ai}:a:{i}?"]
@@ -630,8 +636,7 @@ def start_session(
directory = _HLS_ROOT / f"{key}_{int(start_s)}_{tag}_{aoff:+.2f}"
def _seg(i: int) -> Path:
# multi-audio: the video rendition is variant v0 → seg_{i}_0.ts; single-audio → seg_{i}.ts
return directory / (f"seg_{i}_0.ts" if is_multi else f"seg_{i}.ts")
return directory / segment_name(i, is_multi)
def _spawn(
hw: bool, tonemap: bool = False, *, replace_only: "HlsSession | None" = None
@@ -758,32 +763,51 @@ def current_session(key: str) -> HlsSession | None:
return s
# A segment file, single-audio (`seg_12.ts`) or one rendition of a multi-audio session
# (`seg_12_0.ts`). ffmpeg writes them strictly in order, which is what makes `segment_may_arrive`
# exact rather than a heuristic.
_SEG_NAME_RE = re.compile(r"^seg_(\d+)(_\d+)?\.ts$")
# The names ffmpeg writes into a session directory, in ONE place. Getting this wrong is invisible —
# every path that reads a name falls back to "wait and see" — so it is a constant plus a matching
# parser, not three comments that can drift (they did: deep-review round 6 found the regex, `_seg`
# and the comment above `_ffmpeg_cmd` all disagreeing).
#
# MEASURED (ffmpeg 8.1.1, this repo's own argv): `-hls_segment_filename seg_%v_%d.ts` substitutes
# `%v` with the VARIANT and `%d` with the SEQUENCE — variant FIRST. A dual-audio session writes
# seg_0_0.ts… (video, the unnamed variant) plus seg_a0_0.ts… / seg_a1_0.ts… (the `name:aN` audio
# renditions), and the playlists beside them are master.m3u8 + stream_0/a0/a1.m3u8. Single-audio
# uses `seg_%d.ts` + index.m3u8, with no variant field at all.
_SEG_NAME_RE = re.compile(r"^seg_(?:([0-9a-z]{1,4})_)?(\d{1,9})\.ts$")
_PLAYLIST_NAME_RE = re.compile(r"^(?:master|index|stream_[0-9a-z]{1,4})\.m3u8$")
def segment_may_arrive(directory: Path, filename: str) -> bool:
def segment_name(seq: int, is_multi: bool) -> str:
"""The file ffmpeg writes for the `seq`-th VIDEO segment of a session."""
return f"seg_{_VIDEO_VARIANT}_{seq}.ts" if is_multi else f"seg_{seq}.ts"
def artifact_may_arrive(directory: Path, filename: str) -> bool:
"""Whether waiting for `filename` can possibly end in a file.
ffmpeg produces segments sequentially, so a segment whose PREDECESSOR does not exist yet is not
the next one — it is somewhere beyond the generated region, and no amount of waiting will
conjure it. Saying so at once matters because the route that waits is a SYNC one: it holds a
thread off the ~40-slot pool, and a pooled DB connection with it, for the whole timeout. An
ordinary seek past the produced region was enough to park one for 20 s, and forty concurrent
requests for a name that will never exist parked the whole API (deep-review round 5).
Positive rule, not a blocklist: a session produces segments and playlists and NOTHING else, so
any other name is refused at once. ffmpeg also produces segments strictly in order, so a segment
whose PREDECESSOR does not exist yet is not the next one — it is beyond the generated region,
and no amount of waiting will conjure it. Playlists are the one thing that must still be waited
for: they are rewritten continuously and have no ordering to read.
Anything that is not a segment (a playlist, a WebVTT track) is left to the timeout: those are
rewritten continuously and have no ordering to read."""
Saying no at once matters because the route that waits is a SYNC one: it holds a thread off the
~40-slot pool, and a pooled DB connection with it, for the whole timeout. An ordinary seek past
the produced region was enough to park one for 20 s, and forty concurrent requests for a name
that will never exist parked the whole API (deep-review round 5). Round 5's guard read only
single-audio segment names, so both halves of that were still fully reachable: every multi-audio
name fell through, and so did any `.m3u8`/`.vtt` name at all."""
if _PLAYLIST_NAME_RE.match(filename):
return True
m = _SEG_NAME_RE.match(filename)
if m is None:
return False
variant, seq = m.group(1), int(m.group(2))
if seq == 0:
return True
idx = int(m.group(1))
if idx == 0:
return True
prev = f"seg_{variant}_{seq - 1}.ts" if variant else f"seg_{seq - 1}.ts"
try:
return (directory / f"seg_{idx - 1}{m.group(2) or ''}.ts").exists()
return (directory / prev).exists()
except OSError:
return False
@@ -873,9 +897,13 @@ def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int:
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 (deep-review round 4).
What makes the short rule safe again is that fetching is no longer the only signal: an open
player 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 (round 5). Round 4's blanket
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 (round 5). 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": round 6
found that claim in two comments and not in the code. Round 4's 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."""
now = time.time()
backend/app/routes/plex/playback.py
+40 -11
View File
@@ -323,9 +323,13 @@ 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)
# An open player checkpoints every 10 s whether or not it is playing, so this is the signal that a
# PAUSED 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).
# A player parked mid-item — playing or PAUSED — checkpoints every 10 s, 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
# 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.
plex_stream.touch_session(str(rating_key))
position = max(0, int(body.position_seconds))
duration = int(body.duration_seconds)
@@ -622,11 +626,18 @@ def stream_file(
return FileResponse(str(src)) # Starlette honours the Range header (206)
# What a session directory can contain, and nothing else. No `.vtt`: ffmpeg runs with `-sn` and
# subtitles are independent tracks served by GET /subtitle, so a WebVTT name here could only ever be
# a name that will not arrive — and accepting it cost a 20 s parked worker per request.
_HLS_CT = {
".m3u8": "application/vnd.apple.mpegurl",
".ts": "video/mp2t",
".vtt": "text/vtt",
}
# A real artifact name is ~20 characters. The bound is what keeps an absurd one out of the parser:
# `int()` on an unbounded digit run raises ValueError past 4300 digits (CPython's
# `sys.get_int_max_str_digits`), which reached the client as a 500 — the same shape of bug the
# subtitle routes were bounded for (`_RatingKey`, above).
_HLS_NAME_MAX = 128
@router.get("/stream/{rating_key}/hls/{session_id}/{filename}")
@@ -650,17 +661,24 @@ def stream_hls_file(
auto-start could only ever begin at 0, silently throwing away the viewer's position."""
# Filename must be a plain hls artifact — no path separators, only the expected extensions.
ext = "." + filename.rsplit(".", 1)[-1] if "." in filename else ""
if ext not in _HLS_CT or "/" in filename or "\\" in filename or ".." in filename:
if (
len(filename) > _HLS_NAME_MAX
or ext not in _HLS_CT
or "/" in filename
or "\\" in filename
or ".." in filename
):
raise HTTPException(status_code=404, detail="Not found")
s = plex_stream.current_session(str(rating_key))
if s is None or s.id != session_id:
raise HTTPException(status_code=404, detail="No active playback session")
path = s.dir / filename
# A segment beyond the generated region can never arrive: ffmpeg writes them in order, so if the
# predecessor is not there this one is not next. Answering now instead of waiting it out is what
# keeps a seek past the produced region — or a deliberate flood of hopeless names — from parking
# threadpool workers and their DB connections (see `segment_may_arrive`).
if not plex_stream.segment_may_arrive(s.dir, filename):
# Only a name this session can actually produce is worth waiting for: the playlists, and a
# segment whose predecessor is already on disk (ffmpeg writes them in order, so if it is not
# there this one is not next). Answering now instead of waiting it out is what keeps a seek past
# the produced region — or a deliberate flood of hopeless names — from parking threadpool
# workers and their DB connections (see `artifact_may_arrive`).
if not plex_stream.artifact_may_arrive(s.dir, filename):
raise HTTPException(status_code=404, detail="Not available yet")
timeout = 30 if ext == ".m3u8" else 20
# `proc` so a session whose ffmpeg has already exited answers at once instead of polling out the
@@ -669,8 +687,19 @@ def stream_hls_file(
# past the produced region was enough to do it (round 5).
if not plex_stream.wait_for(path, timeout, proc=s.proc):
raise HTTPException(status_code=404, detail="Not available yet")
try:
content = path.read_bytes()
except OSError:
# The session was torn down between the wait and the read — `_kill` rmtrees the whole
# directory, 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
# 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 HTTPException(status_code=404, detail="No active playback session") from None
return Response(
content=path.read_bytes(),
content=content,
media_type=_HLS_CT[ext],
headers={"Cache-Control": "no-store"},
)