From fc82c8b47afe07341b72183e25c0bf44429c5e38 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 00:45:42 +0200 Subject: [PATCH] =?UTF-8?q?fix(r5):=20address=20the=20fourth=20/code-revie?= =?UTF-8?q?w=20high=20round=20=E2=80=94=206=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Feed memoizes onClose (like Playlists already did): the inline closure re-ran PlayerModal's keydown effect on every Feed render, which since the debounce landed also flushed the volume write and re-stole focus - the player reads its volume with readAccountMerged again — a partial stored object made the level undefined -> NaN -> a persisted null; applyVolume also refuses a non-finite level outright - that read moved into a lazy useState initializer (was running per render) - the two RSS except branches share one tail, so a counter can't drift again - run_ffmpeg tests cover the SIGTERM->SIGKILL escalation and the lingering- process path; dropped an assertion that only restated the fixture - storage.ts documents the actual { volume } shape (0 IS muted) --- backend/app/sync/runner.py | 36 ++++++++++++------------- backend/tests/test_run_ffmpeg.py | 33 +++++++++++++++++++++-- frontend/src/components/Feed.tsx | 9 +++++-- frontend/src/components/PlayerModal.tsx | 15 ++++++++--- frontend/src/lib/storage.ts | 5 ++-- 5 files changed, 70 insertions(+), 28 deletions(-) diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index a75b196..bb41b0e 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -74,28 +74,26 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str channel = futures[fut] try: entries = fut.result() - except RssError as exc: + except Exception as exc: # A failed poll is NOT an empty feed: skip the apply so `last_rss_at` stays where # it was. Stamping it here made a permanently-broken feed look freshly polled. - log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + # An RssError is an EXPECTED outcome (404, timeout) → one warning line; anything + # else is a bug on our side and earns a traceback. + if isinstance(exc, RssError): + log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + else: + log.exception("RSS fetch failed for channel %s", channel.id) failed += 1 - done += 1 - progress.report(done, total, "rss_poll") - continue - except Exception: - log.exception("RSS fetch failed for channel %s", channel.id) - failed += 1 - done += 1 - progress.report(done, total, "rss_poll") - continue - try: - new += apply_rss_feed(db, channel, entries) - except Exception: - # A write failure (read-only DB, failover) leaves the channel just as un-polled as - # an unreachable feed — count it, or a wholly broken run still reports "failed=0". - db.rollback() - failed += 1 - log.exception("RSS apply failed for channel %s", channel.id) + else: + try: + new += apply_rss_feed(db, channel, entries) + except Exception: + # A write failure (read-only DB, failover) leaves the channel just as un-polled + # as an unreachable feed — count it, or a wholly broken run reports "failed=0". + db.rollback() + failed += 1 + log.exception("RSS apply failed for channel %s", channel.id) + # One place that advances the counter, whatever happened above. done += 1 progress.report(done, total, "rss_poll") if failed: diff --git a/backend/tests/test_run_ffmpeg.py b/backend/tests/test_run_ffmpeg.py index 2ea5b21..5dec4dc 100644 --- a/backend/tests/test_run_ffmpeg.py +++ b/backend/tests/test_run_ffmpeg.py @@ -4,6 +4,7 @@ No ffmpeg here: a fake Popen stands in, so the reader threads, the 1s poll loop, and the bounded exit wait are all exercised on any machine. These are regression tests for a real deadlock: stderr was opened and never read, so a chatty ffmpeg filled the ~64KB pipe, blocked on its next write and never exited, and `proc.wait()` never returned.""" +import subprocess import time import pytest @@ -39,12 +40,16 @@ class _FakeStream: class _FakeProc: pid = 4242 - def __init__(self, stdout, stderr, returncode=0): + def __init__(self, stdout, stderr, returncode=0, wait_timeouts=0): self.stdout = stdout self.stderr = stderr self._rc = returncode + # How many of the first `wait()` calls raise TimeoutExpired — a process ignoring SIGTERM, + # or one that lingers after closing its pipes. + self._wait_timeouts = wait_timeouts self.terminated = False self.killed = False + self.waits = 0 def terminate(self): self.terminated = True @@ -57,6 +62,10 @@ class _FakeProc: self.terminate() def wait(self, timeout=None): + self.waits += 1 + if self._wait_timeouts > 0: + self._wait_timeouts -= 1 + raise subprocess.TimeoutExpired("ffmpeg", timeout or 0) return self._rc @@ -96,7 +105,7 @@ def test_a_nonzero_exit_raises_with_the_stderr_tail(fake_popen): ) with pytest.raises(RuntimeError, match="Invalid data found"): edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: False) - assert proc._rc == 2 + assert not proc.terminated # it exited on its own; nothing to kill def test_cancel_is_honoured_while_the_process_is_silent(fake_popen): @@ -123,6 +132,26 @@ def test_a_wedged_process_is_killed_by_the_stall_watchdog(fake_popen): assert proc.terminated # the worker thread is freed, not parked forever +def test_a_process_that_ignores_sigterm_is_killed(fake_popen): + # `_stop`'s escalation: terminate, wait 5s, then SIGKILL. Without it a cancel could park the + # worker thread on a process that simply refuses to go. + proc = fake_popen( + _FakeProc(_FakeStream(block=True), _FakeStream(block=True), wait_timeouts=1) + ) + with pytest.raises(edit.EditAborted): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: True) + assert proc.terminated and proc.killed + + +def test_a_process_lingering_after_eof_is_stopped_and_reported(fake_popen): + # Pipes closed but the process still around: the bounded wait must give up rather than block + # the worker thread on `proc.wait()` forever (the old code waited with no timeout at all). + proc = fake_popen(_FakeProc(_FakeStream([]), _FakeStream(), wait_timeouts=1)) + with pytest.raises(RuntimeError, match="did not exit"): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: False) + assert proc.terminated + + def test_stderr_is_drained_while_the_process_runs(fake_popen): # The deadlock case: a chatty ffmpeg that exits 0. EVERY stderr line must be consumed — with a # real pipe, an unread one fills at ~64KB and blocks ffmpeg's next write forever. diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index 0da24e0..ad1a3b5 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -137,6 +137,11 @@ export default function Feed({ (video: Video, startAt: number | null = null) => setActiveVideo({ video, startAt }), [], ); + // Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown/scroll-lock effect + // depends on `onClose`, so a fresh closure every Feed render re-subscribed those listeners, + // re-ran focusModal() under the open player, dropped its in-flight timers and flushed the + // debounced volume write on each pass. + const closePlayer = useCallback(() => setActiveVideo(null), []); const ytActive = !!ytSearch; // How many live-search results to gather (the count selector replaces a manual "load more"; @@ -484,7 +489,7 @@ export default function Feed({ video={activeVideo.video} startAt={activeVideo.startAt} queue={ytPlayerQueue} - onClose={() => setActiveVideo(null)} + onClose={closePlayer} onState={onState} /> @@ -739,7 +744,7 @@ export default function Feed({ video={activeVideo.video} startAt={activeVideo.startAt} queue={playerQueue} - onClose={() => setActiveVideo(null)} + onClose={closePlayer} onState={onState} /> diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 3368479..34f175b 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,7 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; -import { LS, readAccount, writeAccount } from "../lib/storage"; +import { LS, readAccountMerged, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -188,7 +188,14 @@ export default function PlayerModal({ // (the on-player flash has its own `volumeUi` state), the once-bound wheel/key listeners need a // ref anyway, and the hook's write lives inside a `setState` updater — which is both a second // write on every settle and not something React must run while the component is unmounting. - const volumeRef = useRef(readAccount(LS.ytPlayerPrefs, { volume: 100 }).volume); + // `readAccountMerged` (not readAccount) so a partial/older stored object still gets the default + // — a missing `volume` would otherwise travel as undefined into the arithmetic below. Read via + // a lazy `useState` initializer so it happens once, not on every render (the house pattern — + // see the frozen queue above). + const [storedVolume] = useState( + () => readAccountMerged(LS.ytPlayerPrefs, { volume: 100 }).volume, + ); + const volumeRef = useRef(storedVolume); const volSaveTimerRef = useRef(undefined); // When the last volume gesture happened — see nudgeVolume (a spin must not re-read the player). const lastNudgeAtRef = useRef(0); @@ -221,7 +228,9 @@ export default function PlayerModal({ /** Push a level (0–100) to the player and remember it; 0 also mutes (a player left at zero * volume but unmuted shows YouTube's speaker icon as ON, which reads as broken). */ const applyVolume = (level: number, flash = true) => { - const next = Math.max(0, Math.min(100, Math.round(level))); + // A non-finite level (a corrupt stored value, a player that answered NaN) must not become a + // clamped NaN: that would mute the player AND persist `null`, poisoning every later session. + const next = Number.isFinite(level) ? Math.max(0, Math.min(100, Math.round(level))) : 100; const p = playerRef.current; if (p && typeof p.setVolume === "function") { if (next > 0) p.unMute?.(); diff --git a/frontend/src/lib/storage.ts b/frontend/src/lib/storage.ts index fa98bb5..9ae2496 100644 --- a/frontend/src/lib/storage.ts +++ b/frontend/src/lib/storage.ts @@ -50,8 +50,9 @@ export const LS = { plexQ: "siftlode.plexQ", plexPlaylistLayout: "siftlode.plexPlaylistLayout", plexPlayerPrefs: "siftlode.plexPlayerPrefs", - // The YouTube modal player's volume/mute. A YT.Player instance is created per video and starts - // at full volume, so without this every next video is loud again (same idea as plexPlayerPrefs). + // The YouTube modal player's volume (`{ volume: 0–100 }`; 0 IS muted — there is no separate + // flag). A YT.Player instance is created per video and starts at full volume, so without this + // every next video is loud again (same idea as plexPlayerPrefs). ytPlayerPrefs: "siftlode.ytPlayerPrefs", notifHistory: "siftlode.notifications", notifSettings: "siftlode.notifSettings",