fix(r5): address the fourth /code-review high round — 6 findings
- 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)
This commit is contained in:
+17
-19
@@ -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:
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user