fix(api): a paused viewer keeps its HLS session, a refused subtitle publish backs off, and a NUL is a 422 everywhere
Deep-review round 4, backend half. - The periodic HLS reaper (round 3) applied the "finished and unfetched for 30 s" rule that only a NEW session request may apply. A remux finishes long before the film, and a paused player with a full buffer fetches nothing, so a one-minute pause killed the session and playback died ~30 s after resuming. The periodic pass is `reap_abandoned` now (the long idle limit, finished or not). - A subtitle `publish` that could not keep its output (full / read-only cache volume) returned quietly, so every poll queued the window and the whole-container pass again. It reports failure and the pass is backed off like a failed read; a cached VTT that cannot be READ is answered `unavailable` (`cache_unreadable`) instead of scheduling reads that can never land. - The subtitle routes bound `rating_key` / `sub_ord` (they become a cache file name: a 250-digit one was ENAMETOOLONG, which pathlib's exists() re-raises -> a 500), and the cache helpers no longer throw on such a name. - One app-wide handler turns psycopg's "text fields cannot contain NUL" DataError into a 422: round 3 guarded only the by-rating_key lookup, and `contains=`, `contains_group=`, reorder/bulk key lists and the collection search still 500'd. - The hand-written `db.get(...) is None -> 404 "Unknown ..."` siblings use `get_or_404` with a `what` (channel / video / user), so the Hungarian UI no longer shows English for a gone row.
This commit is contained in:
+20
-2
@@ -24,6 +24,7 @@ log = logging.getLogger("siftlode.app")
|
||||
from fastapi.middleware.cors import CORSMiddleware
|
||||
from fastapi.responses import FileResponse, HTMLResponse, JSONResponse
|
||||
from fastapi.staticfiles import StaticFiles
|
||||
from sqlalchemy.exc import DataError
|
||||
from starlette.middleware.sessions import SessionMiddleware
|
||||
|
||||
from app import state
|
||||
@@ -109,11 +110,13 @@ async def _reap_hls_sessions() -> None:
|
||||
"""The periodic reaper plex/stream.py's docstring promises. `reap_idle` used to run only inside
|
||||
the NEXT session request, so after the last playback of the day an abandoned session — its
|
||||
ffmpeg still working through the film, its directory a whole segmented copy of it — lived until
|
||||
somebody pressed play again or the process restarted."""
|
||||
somebody pressed play again or the process restarted.
|
||||
|
||||
`reap_abandoned`, not `reap_idle`: a finished session is also a paused viewer's."""
|
||||
while True:
|
||||
await asyncio.sleep(_HLS_REAP_EVERY_S)
|
||||
try:
|
||||
await asyncio.to_thread(plex_stream.reap_idle)
|
||||
await asyncio.to_thread(plex_stream.reap_abandoned)
|
||||
except Exception:
|
||||
log.exception("HLS session reap failed")
|
||||
|
||||
@@ -147,6 +150,21 @@ async def _validation_error(request: Request, exc: RequestValidationError) -> JS
|
||||
)
|
||||
|
||||
|
||||
@app.exception_handler(DataError)
|
||||
async def _nul_in_text(request: Request, exc: DataError) -> JSONResponse:
|
||||
"""A NUL byte in a string the request sent is a 422, on every route at once. Postgres cannot hold
|
||||
NUL in text, so the driver refuses to even send the query — and a `%00` in a query parameter or
|
||||
an escaped NUL in a JSON body (a playlist's `contains=`, a reorder's key list, a collection
|
||||
search) was a 500 with a traceback wherever it reached a comparison. Guarding each site did not
|
||||
hold: round 3 guarded the by-rating_key lookup and missed the rest. Any other DataError is left
|
||||
alone — re-raised, it stays the 500 it always was."""
|
||||
if "NUL (0x00)" not in str(exc.orig):
|
||||
raise exc
|
||||
return JSONResponse(
|
||||
status_code=422, content={"detail": {"code": "invalid_input", "reason": "nul"}}
|
||||
)
|
||||
|
||||
|
||||
app.add_middleware(
|
||||
SessionMiddleware,
|
||||
secret_key=settings.secret_key,
|
||||
|
||||
@@ -13,8 +13,9 @@ def by_rating_key(db: Session, model: type[_M], rating_key: str) -> _M | None:
|
||||
tables (PlexItem / PlexShow / PlexSeason / PlexCollection); the return type is that model, so call
|
||||
sites keep their `Model | None` static type.
|
||||
|
||||
A key with a NUL in it (`%00` in a path segment) names no row — and Postgres refuses NUL in text,
|
||||
so asking would be a DataError, i.e. a 500, from every route that looks an item up."""
|
||||
A key with a NUL in it (`%00` in a path segment) names no row — a 404, like any unknown key.
|
||||
Postgres refuses NUL in text, so asking would be a DataError; the app-wide handler turns the
|
||||
places that DO ask (lists and filters compared to rating_key) into a 422 (see main.py)."""
|
||||
if "\x00" in str(rating_key):
|
||||
return None
|
||||
return db.execute(select(model).where(model.rating_key == str(rating_key))).scalars().first()
|
||||
|
||||
@@ -820,14 +820,36 @@ def sweep_orphans(older_than: float | None = None) -> int:
|
||||
return dropped
|
||||
|
||||
|
||||
def reap_idle() -> int:
|
||||
# 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
|
||||
|
||||
|
||||
def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int:
|
||||
"""Kill sessions nobody has fetched from for `_SESSION_IDLE_S` — or, once their ffmpeg has
|
||||
finished, for `finished_idle_s`.
|
||||
|
||||
The short rule is only safe where a NEW session is being requested (the default, POST /session):
|
||||
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. The periodic reaper
|
||||
therefore uses `reap_abandoned` — with the 30 s rule, a one-minute pause killed the session and
|
||||
playback died ~30 s after resuming (deep-review round 4). A viewer paused for longer than
|
||||
`_SESSION_IDLE_S` still loses it; the player restarts one at the playhead when a fetch 404s."""
|
||||
now = time.time()
|
||||
dropped = 0
|
||||
with _lock:
|
||||
for key, s in list(_sessions.items()):
|
||||
done = s.proc.poll() is not None
|
||||
if now - s.last_access > _SESSION_IDLE_S or (done and now - s.last_access > 30):
|
||||
idle = now - s.last_access
|
||||
if idle > _SESSION_IDLE_S or (done and idle > finished_idle_s):
|
||||
_kill(s)
|
||||
_sessions.pop(key, None)
|
||||
dropped += 1
|
||||
return dropped
|
||||
|
||||
|
||||
def reap_abandoned() -> int:
|
||||
"""The periodic reaper's pass: only what nobody has fetched from for `_SESSION_IDLE_S`, finished
|
||||
or not (see reap_idle for why a finished session is not yet an abandoned one)."""
|
||||
return reap_idle(finished_idle_s=_SESSION_IDLE_S)
|
||||
|
||||
@@ -207,7 +207,17 @@ def read_meta(key: str) -> tuple[list[list[float]], bool]:
|
||||
raw = json.loads(_meta_path(key).read_text(encoding="utf-8"))
|
||||
return [[float(a), float(b)] for a, b in raw.get("ranges", [])], bool(raw.get("complete"))
|
||||
except (OSError, ValueError, TypeError):
|
||||
return ([[0.0, _FULL_END_S]], True) if _vtt_path(key).exists() else ([], False)
|
||||
return ([[0.0, _FULL_END_S]], True) if _exists(_vtt_path(key)) else ([], False)
|
||||
|
||||
|
||||
def _exists(p: Path) -> bool:
|
||||
"""`Path.exists` that answers False for a path the OS refuses outright: pathlib swallows only
|
||||
ENOENT-like errors, so an over-long name (ENAMETOOLONG, from an absurd rating_key or ordinal)
|
||||
escaped as a 500 from the status route."""
|
||||
try:
|
||||
return p.exists()
|
||||
except (OSError, ValueError):
|
||||
return False
|
||||
|
||||
|
||||
def _write_meta(key: str, ranges: list[list[float]], complete: bool) -> None:
|
||||
@@ -305,8 +315,11 @@ def _last_cue_end(vtt: str) -> float:
|
||||
return end
|
||||
|
||||
|
||||
def publish(key: str, vtt: str, covered: tuple[float, float], complete: bool) -> None:
|
||||
"""Merge a freshly extracted region into the cached track and widen its covered ranges.
|
||||
def publish(key: str, vtt: str, covered: tuple[float, float], complete: bool) -> bool:
|
||||
"""Merge a freshly extracted region into the cached track and widen its covered ranges. False
|
||||
when the region did not land (the cache is unwritable, or the VTT there cannot be read) — the
|
||||
caller must then record a FAILED pass: returning quietly let the next poll schedule the same read
|
||||
again, a whole-container pass every time, while the player said "preparing" for good.
|
||||
|
||||
A read-merge-write, so it runs under `_PUBLISH_LOCK`: two jobs publish into the same keys (a
|
||||
window and the full pass, and every embedded track of the file rides each of them), and without
|
||||
@@ -319,7 +332,7 @@ def publish(key: str, vtt: str, covered: tuple[float, float], complete: bool) ->
|
||||
# There, but unreadable (`cache_read` swallows the OSError). Merging into "" would replace
|
||||
# a whole track with this one region while its sidecar still claimed all of it.
|
||||
log.warning("subtitle cache %s unreadable; not publishing into it", key)
|
||||
return
|
||||
return False
|
||||
old_ranges, was_complete = read_meta(key)
|
||||
if old is None:
|
||||
# A sidecar with no VTT behind it claims cues that are not there, so it is not merged
|
||||
@@ -330,8 +343,10 @@ def publish(key: str, vtt: str, covered: tuple[float, float], complete: bool) ->
|
||||
old_ranges, was_complete = [], False
|
||||
_write_meta(key, [], False)
|
||||
if not cache_write(key, merge_vtt(old or "", vtt)):
|
||||
return # the region never landed, so the sidecar must not claim it
|
||||
log.warning("subtitle cache %s could not be written", key)
|
||||
return False # the region never landed, so the sidecar must not claim it
|
||||
_write_meta(key, merge_ranges(old_ranges + [list(covered)]), complete or was_complete)
|
||||
return True
|
||||
|
||||
|
||||
# --- background extraction ----------------------------------------------------------------------
|
||||
@@ -531,6 +546,12 @@ def ensure(rating_key: str, sub_ord: int, at: float = 0.0) -> dict:
|
||||
gen = generation(key)
|
||||
if complete and have:
|
||||
return {"state": "ready", "code": None, "gen": gen}
|
||||
if not have and _exists(_vtt_path(key)):
|
||||
# On disk but unreadable (permissions, a bad volume): nothing can serve it, and no extraction
|
||||
# can help — `publish` refuses to merge into it, and a job whose sidecar covers the position
|
||||
# skips as already done — so scheduling only kept the player on "preparing" for good. `gen` 0:
|
||||
# there is nothing a <track> could fetch.
|
||||
return {"state": "unavailable", "code": "cache_unreadable", "gen": 0}
|
||||
code = _recent_failure(key)
|
||||
if code and not covered:
|
||||
return {
|
||||
@@ -616,7 +637,7 @@ def _already_done(key: str, window_start: float | None, at: float) -> bool:
|
||||
"""A queued job looks again before it reads anything. While it waited, another job may have
|
||||
covered it: every embedded track rides each pass over its file, so a window for one language also
|
||||
publishes the others."""
|
||||
if not _vtt_path(key).exists():
|
||||
if not _exists(_vtt_path(key)):
|
||||
return False
|
||||
ranges, complete = read_meta(key)
|
||||
return complete or (window_start is not None and covers(ranges, at))
|
||||
@@ -696,7 +717,8 @@ def _extract_and_publish(rating_key: str, sub_ord: int, window_start: float | No
|
||||
# identity is per ITEM (the embedded fan-out shares it), so a sidecar picked while an
|
||||
# embedded track's full pass was running sat behind that pass for minutes.
|
||||
vtt = _sidecar_vtt(db, want)
|
||||
publish(cache_key(rating_key, sub_ord), vtt, (0.0, _FULL_END_S), True)
|
||||
if not publish(cache_key(rating_key, sub_ord), vtt, (0.0, _FULL_END_S), True):
|
||||
raise SubtitleUnavailable("extract_failed")
|
||||
return
|
||||
if want.get("index") is None:
|
||||
raise SubtitleUnavailable("not_fetchable")
|
||||
@@ -721,13 +743,16 @@ def _extract_and_publish(rating_key: str, sub_ord: int, window_start: float | No
|
||||
# the track read "ready" for good while its subtitles stopped wherever ffmpeg did (the cache
|
||||
# has no eviction, so nothing would ever have extracted it again).
|
||||
covered_end = end if clean else start + _last_cue_end(vtt)
|
||||
publish(
|
||||
landed = publish(
|
||||
key,
|
||||
shift_vtt(vtt, -start), # zero-based window clock → absolute cue times
|
||||
(start, covered_end),
|
||||
clean and length is None,
|
||||
)
|
||||
if not clean:
|
||||
if not landed:
|
||||
# A read that could not be kept is a failed read (see publish) — backed off like one.
|
||||
_pass_failed(rating_key, [key], window_start, (start, end))
|
||||
elif not clean:
|
||||
_pass_failed(rating_key, [key], window_start, (covered_end, end))
|
||||
|
||||
|
||||
|
||||
@@ -26,7 +26,7 @@ from app.models import (
|
||||
User,
|
||||
Video,
|
||||
)
|
||||
from app.routes._common import owned_or_404
|
||||
from app.routes._common import get_or_404, owned_or_404
|
||||
from app.sync.explore import explore_ingest_page
|
||||
from app.sync.runner import backfill_channel_now, run_recent_backfill
|
||||
from app.sync.subscriptions import apply_channel_details
|
||||
@@ -663,8 +663,7 @@ def block_channel(
|
||||
"""Block a channel for this user: its videos won't appear in their live search (and won't be
|
||||
ingested for them) and are hidden from their feed / explore. Idempotent. The now-hidden
|
||||
un-kept search/explore videos are reclaimed by the discovery-cleanup job in due course."""
|
||||
if db.get(Channel, channel_id) is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown channel")
|
||||
get_or_404(db, Channel, channel_id, what="channel")
|
||||
if not _is_blocked(db, user, channel_id):
|
||||
db.add(BlockedChannel(user_id=user.id, channel_id=channel_id))
|
||||
# Stop any active exploration of it so it can be cleaned up.
|
||||
|
||||
@@ -38,6 +38,7 @@ from app.models import (
|
||||
MediaAsset,
|
||||
User,
|
||||
)
|
||||
from app.routes._common import get_or_404
|
||||
from app.security import hash_password
|
||||
from app.userscope import is_messageable_user, messageable_clauses
|
||||
from app.utils import iso
|
||||
@@ -1091,8 +1092,7 @@ def admin_set_quota(
|
||||
admin: User = Depends(admin_user),
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
if db.get(User, user_id) is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown user")
|
||||
get_or_404(db, User, user_id, what="user")
|
||||
cur = quota.resolve(db, user_id)
|
||||
row = db.get(DownloadQuota, user_id)
|
||||
if row is None:
|
||||
|
||||
@@ -29,6 +29,7 @@ from app.models import (
|
||||
Video,
|
||||
VideoState,
|
||||
)
|
||||
from app.routes._common import get_or_404
|
||||
from app.sync.videos import parse_iso8601_duration
|
||||
from app.utils import iso
|
||||
from app.youtube.client import YouTubeClient, YouTubeError
|
||||
@@ -235,8 +236,7 @@ def set_video_state(
|
||||
status = body.status
|
||||
if status not in VALID_STATES:
|
||||
raise HTTPException(status_code=400, detail=f"status must be one of {VALID_STATES}")
|
||||
if db.get(Video, video_id) is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown video")
|
||||
get_or_404(db, Video, video_id, what="video")
|
||||
|
||||
now = datetime.now(timezone.utc)
|
||||
|
||||
@@ -332,8 +332,7 @@ def set_video_progress(
|
||||
if position < 0:
|
||||
position = 0
|
||||
|
||||
if db.get(Video, video_id) is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown video")
|
||||
get_or_404(db, Video, video_id, what="video")
|
||||
|
||||
# Decide whether this position is worth keeping (else clear it).
|
||||
near_end = duration > 0 and position > duration - FINISH_MARGIN_SECONDS
|
||||
|
||||
@@ -23,7 +23,7 @@ from app.sync.playlists import (
|
||||
sync_user_playlists,
|
||||
)
|
||||
from app.ratelimit import RateLimiter
|
||||
from app.routes._common import not_found, owned_or_404
|
||||
from app.routes._common import get_or_404, owned_or_404
|
||||
from app.youtube.client import YouTubeClient, YouTubeError
|
||||
|
||||
router = APIRouter(prefix="/api/playlists", tags=["playlists"])
|
||||
@@ -292,8 +292,7 @@ def add_watch_later(
|
||||
) -> dict:
|
||||
"""Bookmark shortcut: add a video to the built-in Watch later playlist."""
|
||||
video_id = body.video_id
|
||||
if db.get(Video, video_id) is None:
|
||||
raise not_found("video")
|
||||
get_or_404(db, Video, video_id, what="video")
|
||||
pl = _watch_later(db, user)
|
||||
_add_item(db, pl, video_id, mark_dirty=False)
|
||||
return {"saved": True, "playlist_id": pl.id}
|
||||
@@ -513,8 +512,7 @@ def add_item(
|
||||
) -> dict:
|
||||
pl = _own_playlist(db, user, playlist_id)
|
||||
video_id = body.video_id
|
||||
if db.get(Video, video_id) is None:
|
||||
raise not_found("video")
|
||||
get_or_404(db, Video, video_id, what="video")
|
||||
_add_item(db, pl, video_id, mark_dirty=True)
|
||||
return {"playlist_id": pl.id, "video_id": video_id}
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ from fastapi import (
|
||||
BackgroundTasks,
|
||||
Depends,
|
||||
HTTPException,
|
||||
Path,
|
||||
Query,
|
||||
Request,
|
||||
Response,
|
||||
@@ -95,12 +96,19 @@ def image(rating_key: str, request: Request, variant: str = "thumb") -> Response
|
||||
# poster, not for a route that starts reads of the media library: a suspended or "log out
|
||||
# everywhere"-revoked cookie kept scheduling them. They are not bursty (one poll a second, one fetch
|
||||
# per track), so the DB user-load costs nothing that matters.
|
||||
#
|
||||
# Both path parameters are bounded, the same way on both routes: they become a cache FILE name
|
||||
# (`sub_<rating_key>_<ord>`), and an absurd one — a 250-digit key or ordinal — outgrew the OS's name
|
||||
# limit and turned the status poll into a 500. A Plex rating_key is a short number; the bound only
|
||||
# has to be generous.
|
||||
_RatingKey = Path(max_length=32)
|
||||
_SubOrd = Path(ge=0, le=9999)
|
||||
|
||||
|
||||
@router.get("/subtitle/{rating_key}/{sub_ord}/status")
|
||||
def subtitle_status(
|
||||
rating_key: str,
|
||||
sub_ord: int,
|
||||
rating_key: str = _RatingKey,
|
||||
sub_ord: int = _SubOrd,
|
||||
user: User = Depends(current_user),
|
||||
# Bounded both ways: a non-finite position used to reach the scheduler's arithmetic and 500, and
|
||||
# an absurd one reached ffmpeg's `-ss`, which refused it.
|
||||
@@ -117,8 +125,8 @@ def subtitle_status(
|
||||
|
||||
@router.get("/subtitle/{rating_key}/{sub_ord}")
|
||||
def subtitle(
|
||||
rating_key: str,
|
||||
sub_ord: int,
|
||||
rating_key: str = _RatingKey,
|
||||
sub_ord: int = _SubOrd,
|
||||
user: User = Depends(current_user),
|
||||
# Finite, like every float this router takes: `-inf` crashed the shift (a 500), `nan` stacked
|
||||
# every cue at 0. It can legitimately be negative (a subtitle nudged later than the session start).
|
||||
|
||||
Reference in New Issue
Block a user