fix(plex): a restart that fails to start still deletes the session it moved aside

backend/app/plex/stream.py

@@ -820,74 +820,78 @@ def start_session(
         # platform without fork (the API always runs in a Linux container in prod) it's simply omitted.
         preexec = _sw_child_preexec if (transcode and not hw and os.name == "posix") else None
         doomed: list[tuple[HlsSession, Path | None]] = []
-        with _lock:
+        # Finished even when the new session fails to start (no space, no ffmpeg, a DB error): what
-            if replace_only is not None and _sessions.get(reg) is not replace_only:
+        # was moved aside under the lock is otherwise left for a restart's sweep.
-                return None  # superseded/culled while we waited out of lock — don't clobber the newer one
+        try:
-            old = _sessions.pop(reg, None)
+            with _lock:
-            if old is not None:
+                if replace_only is not None and _sessions.get(reg) is not replace_only:
-                doomed.append(_detach(old))
+                    return None  # superseded/culled while we waited out of lock — don't clobber the newer one
-            # An ffmpeg still exiting from THIS path (the old session, or one stopped a moment ago)
+                old = _sessions.pop(reg, None)
-            # would write into the directory about to be made; see `_dying`.
+                if old is not None:
-            prev = _dying.pop(directory, None)
+                    doomed.append(_detach(old))
-            if prev is not None:
+                # An ffmpeg still exiting from THIS path (the old session, or one stopped a moment ago)
-                _await_exit(key, prev)
+                # would write into the directory about to be made; see `_dying`.
-            shutil.rmtree(directory, ignore_errors=True)
+                prev = _dying.pop(directory, None)
-            directory.mkdir(parents=True, exist_ok=True)
+                if prev is not None:
-            proc = subprocess.Popen(
+                    _await_exit(key, prev)
-                _ffmpeg_cmd(
+                shutil.rmtree(directory, ignore_errors=True)
-                    src,
+                directory.mkdir(parents=True, exist_ok=True)
-                    start_s,
+                proc = subprocess.Popen(
+                    _ffmpeg_cmd(
+                        src,
+                        start_s,
+                        directory,
+                        audio_ord,
+                        aoff,
+                        audio_langs if is_multi else None,
+                        transcode=transcode,
+                        hw=hw,
+                        tonemap=tonemap,
+                        preset=preset,
+                        crf=crf,
+                        max_height=max_height,
+                        loudnorm=loudnorm,
+                    ),
+                    stdout=subprocess.DEVNULL,
+                    stderr=subprocess.DEVNULL,
+                    preexec_fn=preexec,
+                )
+                s = HlsSession(
+                    key,
                     directory,
-                    audio_ord,
+                    proc,
-                    aoff,
+                    start_s,
-                    audio_langs if is_multi else None,
+                    variants,
                     transcode=transcode,
                     hw=hw,
                     tonemap=tonemap,
-                    preset=preset,
+                    user_id=user_id,
-                    crf=crf,
+                )
-                    max_height=max_height,
+                _sessions[reg] = s
-                    loudnorm=loudnorm,
+                # The one line that says which pipeline a session actually got. Everything downstream
-                ),
+                # — the cap, the HW→SW fallback, the subtitle offset — is about this choice, and until
-                stdout=subprocess.DEVNULL,
+                # now it was only inferable from the ffmpeg command nobody keeps.
-                stderr=subprocess.DEVNULL,
+                log.info(
-                preexec_fn=preexec,
+                    "an HLS session started",
-            )
+                    extra=logs.event(
-            s = HlsSession(
+                        "plex.stream.session_started",
-                key,
+                        key=key,
-                directory,
+                        transcode=transcode,
-                proc,
+                        hw=hw,
-                start_s,
+                        tonemap=tonemap,
-                variants,
+                        variants=len(variants or ()),
-                transcode=transcode,
+                        start_s=round(start_s, 1),
-                hw=hw,
+                    ),
-                tonemap=tonemap,
+                )
-                user_id=user_id,
+                # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be
-            )
+                # capped to zero.
-            _sessions[reg] = s
+                doomed += _enforce_caps(
-            # The one line that says which pipeline a session actually got. Everything downstream
+                    user_id,
-            # — the cap, the HW→SW fallback, the subtitle offset — is about this choice, and until
+                    max(1, sysconfig.get_int(db, "plex_max_transcodes")),
-            # now it was only inferable from the ffmpeg command nobody keeps.
+                    max(1, sysconfig.get_int(db, "plex_max_sessions")),
-            log.info(
+                )
-                "an HLS session started",
+        finally:
-                extra=logs.event(
+            _finish(doomed)
-                    "plex.stream.session_started",
-                    key=key,
-                    transcode=transcode,
-                    hw=hw,
-                    tonemap=tonemap,
-                    variants=len(variants or ()),
-                    start_s=round(start_s, 1),
-                ),
-            )
-            # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be
-            # capped to zero.
-            doomed += _enforce_caps(
-                user_id,
-                max(1, sysconfig.get_int(db, "plex_max_transcodes")),
-                max(1, sysconfig.get_int(db, "plex_max_sessions")),
-            )
-        _finish(doomed)
         return s
 
     s = _spawn(*_select_encode(db, item, transcode, src))

backend/tests/test_plex_stream.py

@@ -1058,3 +1058,63 @@ class TestSessionsPerViewer:
             self._start(monkeypatch, user_id=uid)
         assert len(stream._sessions) == 3 and (1, "12345") not in stream._sessions
         assert oldest.proc.killed and not oldest.transcode
+
+
+class TestTeardownFinishes:
+    """A session taken down by a cap or replaced by its own viewer is only renamed aside under the
+    lock; it is deleted after. The rename alone already moves the directory and `terminate()` alone
+    already marks the proc killed — so what is pinned here is the tomb: a whole segmented copy of the
+    film that only a restart's sweep would otherwise reclaim."""
+
+    capturing_env = TestTranscodeStartSession.capturing_env
+
+    @staticmethod
+    def _left_behind(root):
+        return sorted(p.name for p in root.iterdir() if stream._TOMB in p.name) + sorted(
+            str(p) for p in stream._dying
+        )
+
+    def test_a_culled_session_is_deleted_not_only_moved_aside(self, capturing_env, monkeypatch):
+        monkeypatch.setattr(stream, "_dying", {})
+        first = TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="1")
+        first.last_access -= 60
+        TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="2")
+        TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="3")
+        assert first.proc.killed and not first.dir.exists()
+        assert self._left_behind(capturing_env) == []
+
+    def test_a_viewers_restart_deletes_the_session_it_replaces(self, capturing_env, monkeypatch):
+        monkeypatch.setattr(stream, "_dying", {})
+        monkeypatch.setattr(
+            stream, "_select_encode", lambda db, item, transcode, src: (False, False)
+        )
+        monkeypatch.setattr(
+            stream, "_probe_first_pts", lambda seg: None
+        )  # no real segment to probe
+        old = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=0.0)
+        new = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=600.0)
+        assert old.proc.killed and new.dir.exists() and not old.dir.exists()
+        assert self._left_behind(capturing_env) == []
+
+    def test_a_restart_that_fails_to_start_still_deletes_what_it_replaced(
+        self, capturing_env, monkeypatch
+    ):
+        # The new ffmpeg cannot start (a full disk, no ffmpeg, no memory): the old session is already
+        # moved aside, and nothing else in this process would ever delete it.
+        monkeypatch.setattr(stream, "_dying", {})
+        monkeypatch.setattr(
+            stream, "_select_encode", lambda db, item, transcode, src: (False, False)
+        )
+        monkeypatch.setattr(
+            stream, "_probe_first_pts", lambda seg: None
+        )  # no real segment to probe
+        old = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=0.0)
+
+        def no_ffmpeg(*args, **kwargs):
+            raise OSError(28, "No space left on device")
+
+        monkeypatch.setattr(stream.subprocess, "Popen", no_ffmpeg)
+        with pytest.raises(OSError):
+            stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=600.0)
+        assert old.proc.killed and not old.dir.exists()
+        assert self._left_behind(capturing_env) == []