fc65dcaf161fb15a62bd19ebffe2442e2d84bece
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 CoffeeKnyte
|
fc65dcaf16 |
fix(playback): make session liveness server-observed
Client progress reports could keep a zero-byte phantom session alive and, worse, make it outrank a genuinely-serving stream when the enforcer picked over-cap victims. `streammonitor.LiveLocalSessions` substituted `Session.LastActivityAt` for a zero `LastServedAt`, and `LastActivityAt` is advanced by `UpdateProgress` and by the realtime WebSocket hello/ack/result handlers. Since `streamenforcer.selectVictims` keeps the `limit` most-recently-served streams, a progress-only phantom sorted ahead of a real stream and the real one was trimmed instead. Reaping had the same root cause: `sessionIsInactiveLocked` keyed idleness on `LastActivityAt`, so a client that kept pinging held a session open forever. Per decision A5 (Option C), client progress is now UI metadata only and never feeds enforcement or reaping: - `LiveLocalSessions` projects `LastServedAt` verbatim, emitting an empty timestamp when the session has never served, so it sorts as the stalest over-cap victim. - `sessionIsInactiveLocked` measures idleness from `LastServedAt`, falling back only to `StartedAt`. - A configurable never-served window (`DefaultUnservedSessionGrace`, 2m, via `SetUnservedSessionGrace`) keeps a legitimately slow start from being reaped before its first byte, without granting a phantom unbounded life. It is a separate knob rather than a hardcoded floor so it cannot silently override `SetLivenessGracePeriods`. In-flight transports remain exempt, so direct-play and remux long pours and per-segment HLS serves are unaffected. Paused sessions with an open realtime/WebSocket connection are exempt from reaping. That preserves the issue #243 fix (reaping a paused transcode froze clients) while staying within Option C: an open, ping-checked connection is server-observed, unlike a client's reported progress, and the session still consumes one of the user's cap slots. Part 1 of 3 for the Batch 4 liveness/replica work. Part of #305 |
||
|
CoffeeKnyte
|
e5bf0155ad |
fix(playback): make revocation state converge and cutoffs credential-accurate
Five defects in revocation state and credential semantics. Lands after the tracker-lifecycle batch on purpose: raising the over-cap TTL is only safe once the count feeding it is trustworthy. #13 -- a longer old revocation suppressed a newer cutoff. applyLocal kept or replaced the WHOLE record by expiry, so when the existing revocation expired later the new one was dropped entirely, including its newer RevokedAt. The durable upsert did the same, with a comment documenting it as intentional. RevokedAt is the user-kill CUTOFF, so this left a credential issued between the two cutoffs valid -- a second admin kill after a user re-authenticates silently failed to cut them. The two fields now merge independently: ExpiresAt stays monotonic, RevokedAt advances to the later value, and reason follows the newer cutoff. Both superseded comments are replaced rather than left contradicting the code. Session-kind revocation still ignores RevokedAt, so the enforcer's re-revoke cannot weaken a session kill. Also fixed while here: Redis received the merged record but pub/sub published the raw input one, so under pub/sub-only delivery (Redis down) an edge got the newer short record without the older long expiry and lost monotonicity. Both now carry the merged record. #7 + M1 -- Postgres could indefinitely block the urgent Redis kill. RevokeWithWarnings held the global opMu across all propagation, stripped the caller's deadline with WithoutCancel, and did the durable Postgres upsert BEFORE Redis, on a pool with no statement timeout. The local kill still applied, so playback on that process was fine -- but edge propagation, pub/sub, the admin response and every later revoke/unrevoke stalled behind the lock. Redis and pub/sub now go first, and the detached context is bounded. WithoutCancel is kept deliberately: propagation must outlive an aborted admin request. opMu scope is deliberately NOT narrowed. mirrorToRedis is an unconditional SET with no atomic merge, so same-process serialization is what stops an older value overwriting a newer one; narrowing the lock would also let an unrevoke interleave with a revoke's propagation. Bounding the context caps how long the lock can be held, which is the actual reported harm. The remaining cross-replica race -- two central replicas racing the same SET -- is documented, not half-fixed; it needs A6's shared picture. A2 / #6 -- a missed unrevoke got resurrected. In-memory tombstones already existed, but being process-local they did not survive a restart or reach a replica that missed the pub/sub event, so maintain's durable self-heal re-Upserted the surviving entry and the ban returned. Tombstones are now durable, via two nullable columns on stream_revocations rather than a second table: a tombstone is a state of the same key, and it needs its own expiry horizon separate from the revocation's. The upsert rejects a stale replica's write while a tombstone is live but lets a genuinely newer revocation clear it, and warm paths apply tombstones BEFORE revocations so an un-banned key cannot be restored as a live kill. Tombstones are pruned on the same sweep, so the table cannot grow without bound. A1 / #3 -- over-cap kills reopened after 5 minutes while the token stayed reconstructable for 24h. The TTL now derives from playback.MaxTokenTTL rather than duplicating 24h, behind a validated setting. Critically, the enforcer uses a revoke-if-absent path rather than re-revoking. Expiry is monotonic and the enforcer re-evaluates every 30s, so a plain long TTL would slide expiry forward by another full lifetime on every pass -- making a wrong kill effectively permanent for as long as any stale record persisted, with only an explicit unrevoke to recover it. Admin Revoke keeps its monotonic behaviour; only the enforcer's own repeat kill is non-extending. The setting is documented as affecting future revocations only, since monotonic expiry means it cannot shorten one already issued. A3 / #5 -- the user cutoff compared against a fresh time.Now() taken at request entry, so a request from a pre-cutoff login could look post-cutoff and escape the kill. The credential time is now the access token's iat. Two deliberate choices worth stating. API-key credentials carry no issue time, so they pass the zero time and, per IsRevoked's documented contract, are never matched by a user cutoff: a user kill provably cannot cut an API-key-owned pour. That is an accepted, logged, documented hole -- and strictly better than time.Now(), which actively defeats the cutoff. And jellycompat uses the compat session's CreatedAt rather than the bridged Silo token's iat, because that token refreshes without a new Jellyfin login, so its iat would advance on refresh and let a refreshed credential slip past a cutoff. Stream tokens are now bound to their route: a token whose SessionID does not match the URL's session_id is rejected with 403 instead of being silently ignored, matching the reconstruction helper that already refused a different session. Per-login logout cuts remain out of scope -- they need per-login identity in the stream credential. S5 (the (sessionID, userID, startedAt) clump) is rejected as ceremony now that A3 is the iat option rather than the generation model. #12 (closing an RSS feed does not cut its current pour) is deferred: it needs a namespaced revocation id that cannot collide with real session ids, that id threaded onto public feed requests, and protection against a new feed inheriting an old tombstone. Part of #305. |
||
|
CoffeeKnyte
|
ecb4555eec |
fix(playback): make edge tracking lifecycle-safe and session identity canonical
Five defects that all produced a WRONG over-cap count, which is why they land before the revocation batch: decision A1 raises the over-cap revocation TTL from 5m to ~24h, removing the self-healing that currently limits the damage of a miscount. A false positive after A1 blocks a legitimate stream for a day, so the count has to be trustworthy first. #1 -- overlapping edge requests deleted a live stream. Tracker.sessions was a set and Remove tore down all state plus the Redis key, while both proxy pour handlers deferred removal unconditionally. Two overlapping Range GETs on one session id -- ordinary seek behaviour -- meant the first to finish deleted the record while the second was still pouring, and later AddBytes calls were then dropped because AddBytes ignores bytes for a session with no live record. The stream went invisible to authoritative monitoring while still serving. Track now returns a Lease that the request-scoped caller releases exactly once; teardown happens when the last live lease is released. A plain refcount would have been wrong: Track(A) -> Remove -> Track(B) -> Release(A) decrements B, and clamping at zero does not help because the count legitimately belongs to B. That is not hypothetical -- the transcode node deliberately replaces sessions under the same id so a quality switch does not orphan ffmpeg, and it calls unconditional Remove from its reaper and stop paths. So each generation carries an epoch, Remove and Cleanup bump it, and a release from a superseded generation is a logged no-op. Lease identity is a set rather than a counter, which makes a duplicate release detectable instead of silently destructive. The transcode node keeps using Remove: its Track calls are not request-scoped and are correctly owned by session lifecycle. "Every Track needs a paired Release" is true only of the request-scoped callers. #8 -- async transcode tracking could leave a permanent ghost. The tracking write ran as a bare goroutine with a WithoutCancel context, so if stop won the race the delayed Track recreated the record after cleanup -- and because it landed in sessions, Snapshot treated it as live until Remove and it NEVER idle-expired. A permanent phantom inflating its owner's count, able to trigger false over-cap kills of that user's real streams. The write now takes the per-session lifecycle lock that stop and reap already hold, and re-checks session pointer identity before writing, so a stopped or replaced generation cannot resurrect a record. Pointer identity rather than id equality is what makes same-id replacement safe. The write stays off the request path -- the API server and the playback client are blocked on the 202. #9 + M3 -- protocol-v3 counted one stream twice. The stream token carries a transport id distinct from the logical session id, and the node tracked under the transport id while the API/proxy record used the logical one, so mergeStreams saw two streams. M3 was the reason this had not yet bitten: the v3 fresh-start caller sent no owner attribution at all, so the transport record landed under user 0, which the enforcer skips -- silently exempting the stream from the cap entirely. Fresh v3 starts now carry the logical session id and full owner attribution (both were already in scope at the call site), and merging is keyed on logical identity where present via one shared helper used by both merge functions, which had already drifted apart once. The enforcer view resolves SessionID to the logical id so a kill targets the real session rather than a replaceable transport generation. The raw admin view keeps the transport id and exposes logical_session_id as an additive omitempty field, advertised on the node-sessions capability endpoint, so the v1 response shape is unchanged. GAP-15 -- edge transcode liveness was request-observed. touchTranscodeSession fired before proxying, so hammering dead segment URLs advanced LastServedAt with zero bytes served. Visibility and liveness are now separate operations: EnsureEphemeral makes a session visible without claiming bytes were served, and served-byte liveness advances only from a 2xx/206 upstream response. Previously the proxy metered every upstream body regardless of status, so a node 404's error body counted as served bytes -- moving the touch later would not have fixed it. S4 -- LiveLocalSessions moved from the HTTP handlers package to streammonitor, which owns monitoring. A background enforcer importing api/handlers was backwards. Pure move; its existing mapping assertions moved with it. The LastActivityAt fallback inside it is left as-is -- decision A5 removes it in the liveness batch. Verified with go test -race across nodesessions, proxy and transcodenode; the overlap regression test was confirmed to fail under the old unconditional teardown. Part of #305. |
||
|
CoffeeKnyte
|
0217cce2df |
feat(playback): stream kill switch + async over-cap enforcer
Add the enforcement layer on top of server-observed monitoring: a revocation kill switch that stops any stream within ~120s and keeps it dead, plus an async over-cap enforcer that drives kills off the live monitoring picture — entirely off the per-segment hot path and with no client-protocol change. - internal/streamrevoke: the central kill list. IsRevoked is a pure in-memory lookup safe on the request hot path; a Redis pub/sub + poll mirror keeps edge caches current, and a Postgres durable mirror lets kills survive a server restart AND a Redis flush so a restart-resilient stream cannot be reconstructed and re-served after being killed. A user revocation is a cutoff (kills tokens minted before it, spares post-reauth tokens), not a 24h ban. - internal/streamenforcer: async over-cap brain — reads the monitoring snapshot and per-user limits, selects victims, and collapses every reason (exceeded limit, admin terminate, abuse) to the same action: write a revocation. - Edge + native + jellycompat enforcement: proxy refuses revoked sessions on every request and cuts long direct-play/remux pours mid-stream; the transcode node guards both serve and the reconstruct path so a killed session is never re-spawned after a node restart; jellycompat serve surfaces close their kill-switch coverage holes. - streamtoken.IssuedTime exposes the token iat the user-kill cutoff compares against; token IssuedTime + revocation guards wire through router, downloads, and admin terminate-by-id (with admin-list dedupe). - Restore sendfile zero-copy on direct-play/remux byte counting so the monitor's served-byte accounting does not cost the sendfile fast path. - migrations/sql: stream_revocations durable table. Part of the stream monitoring & kill-switch epic. |