fix(r5): address the seventh /code-review high round — 7 findings
Five of them sit where round 6 turned F from a Fullscreen API call into a CSS maximise: the guards around it still asked `isFullscreen`, which that pivot made permanently false. - Arrow-key volume was dead in the maximised view: the scroll-deference checked only `isFullscreenRef`, so the (now hidden but still scrollable) card kept swallowing Up/Down. Both flags now count as "the video fills the screen". Measured: 87px of scroll room on the focused card, ArrowDown moves 80 -> 75 and scrolls nothing. - The maximised view had no way out once the pointer touched the video. Reaching YouTube's controls means clicking into the cross-origin iframe, which takes keyboard focus with it, and the re-arm paths (stage mouseleave, window focus) need the pointer to leave the browser entirely while the stage fills it. So the exit now lives on screen, in our DOM: a button that un-maximises AND pulls focus back, re-arming every shortcut. - WebKit fires only `webkitfullscreenchange`/`webkitFullscreenElement`, so on Safari/iPadOS YouTube's own fullscreen never registered — no overlay yield, no focus reclaim. One `fullscreenEl()` reader, both event spellings. - The stage claimed `z-index: 9999`, the glass tuner's reserved level, for a job that only needs to beat the modal chrome it is nested in (its siblings top out at z-menu). Dropped to rail-level; nothing escapes the modal's stacking context either way. - `inset: 0` already sizes a fixed layer — the inherited `100vw`/`100vh` over-constrained it, which is how a classic scrollbar gets a horizontal overflow and a mobile URL bar hides the bottom edge (YouTube's control bar). - The shortcut hint still promised "F: fullscreen" in both locales. Backend, both "the fix stopped one layer short": - The startup HLS sweep was awaited inside lifespan, so uvicorn still served nothing until it finished — a thread doesn't help when nothing is listening yet. Fire-and-forget task instead, cancelled on shutdown. Measured with 200 planted segments: "Application startup complete" now precedes "swept 1". - An RSS poll where every channel failed returned normally, so the scheduler recorded `ok` with the outage buried in the summary string. `failed == total` now raises: a run that reached nothing is a failed run. Suites: backend 147 -> 150.
This commit is contained in:
+22
-6
@@ -70,20 +70,36 @@ async def lifespan(app: FastAPI):
|
||||
url,
|
||||
)
|
||||
# No HLS session survives a restart (they live in this process), so anything still on disk is
|
||||
# a crashed run's segments — sweep them or they accumulate forever. Off the event loop: the
|
||||
# first sweep after a long uptime can be gigabytes of `.ts` files, and blocking here delays
|
||||
# serving (a 502 + a failed post-deploy health probe).
|
||||
swept = await asyncio.to_thread(plex_stream.sweep_orphans)
|
||||
if swept:
|
||||
log.info("swept %d orphaned HLS segment directories", swept)
|
||||
# a crashed run's segments — sweep them or they accumulate forever. FIRE AND FORGET, not
|
||||
# awaited: uvicorn does not serve a single request until lifespan startup returns, so awaiting
|
||||
# the sweep would hold the port shut for as long as the rmtree runs (gigabytes of `.ts` after a
|
||||
# long uptime) — the 502 + failed post-deploy health probe this is meant to avoid. A thread
|
||||
# alone doesn't buy that; not waiting does. The sweep only touches directories no live session
|
||||
# owns, so racing it against early playback is safe.
|
||||
sweep = asyncio.create_task(_sweep_hls_orphans())
|
||||
start_scheduler()
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
log.info("Siftlode shutting down")
|
||||
sweep.cancel()
|
||||
shutdown_scheduler()
|
||||
|
||||
|
||||
async def _sweep_hls_orphans() -> None:
|
||||
"""Background half of the startup HLS sweep (see lifespan). Owns its own errors: a failure here
|
||||
must never take the app down, and a cancelled shutdown is not one."""
|
||||
try:
|
||||
swept = await asyncio.to_thread(plex_stream.sweep_orphans)
|
||||
except asyncio.CancelledError:
|
||||
raise
|
||||
except Exception:
|
||||
log.exception("HLS orphan sweep failed")
|
||||
return
|
||||
if swept:
|
||||
log.info("swept %d orphaned HLS segment directories", swept)
|
||||
|
||||
|
||||
app = FastAPI(title=settings.app_name, lifespan=lifespan)
|
||||
|
||||
app.add_middleware(
|
||||
|
||||
@@ -52,9 +52,10 @@ def sync_subscriptions(
|
||||
def sync_rss(
|
||||
user: User = Depends(require_human), db: Session = Depends(get_db)
|
||||
) -> dict:
|
||||
# A partial failure comes back in `failed_feeds` so a manual poll reports it instead of an
|
||||
# innocent-looking "0 new"; a TOTAL outage raises out of run_rss_poll and surfaces as a 500,
|
||||
# which is the honest answer for a poll that reached nothing.
|
||||
result = run_rss_poll(db, _user_channels(db, user))
|
||||
# `failed` = feeds that could not be read at all (see run_rss_poll); surfaced so a manual poll
|
||||
# reports an outage instead of an innocent-looking "0 new".
|
||||
return {"new_videos": result["new"], "failed_feeds": result["failed"]}
|
||||
|
||||
|
||||
|
||||
@@ -56,7 +56,11 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str
|
||||
|
||||
`failed` counts a channel whose feed could not be READ *or* whose rows could not be APPLIED —
|
||||
both leave the channel un-polled, and counting only the first would move the blind spot rather
|
||||
than remove it."""
|
||||
than remove it.
|
||||
|
||||
Raises RuntimeError when NOTHING could be polled (`failed == total`, total > 0): a run that
|
||||
achieved literally nothing is a failed run, and only an exception turns the scheduler card red
|
||||
— a returned count leaves it green with the outage buried in the summary line."""
|
||||
if channels is None:
|
||||
channels = db.execute(select(Channel)).scalars().all()
|
||||
total = len(channels)
|
||||
@@ -99,6 +103,12 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str
|
||||
progress.report(done, total, "rss_poll")
|
||||
if failed:
|
||||
log.warning("RSS poll: %d/%d channels could not be polled", failed, total)
|
||||
if total and failed == total:
|
||||
# Nothing was polled at all — an egress outage (the tinyproxy-after-reboot incident), a
|
||||
# dead DNS, a read-only database. Returning normally would record the run as `ok` with the
|
||||
# failure hidden inside the result string, i.e. a green pill for a total outage; the count
|
||||
# in the message keeps the summary just as informative.
|
||||
raise RuntimeError(f"RSS poll failed for all {total} channels")
|
||||
return {"new": new, "failed": failed}
|
||||
|
||||
|
||||
|
||||
@@ -106,34 +106,71 @@ class TestRssPollResult:
|
||||
Counting only the fetch failures would move the "a broken run looks like a quiet hour" blind
|
||||
spot rather than remove it."""
|
||||
|
||||
def _run(self, monkeypatch, *, fetch, apply_):
|
||||
def _run(self, monkeypatch, *, fetch, apply_, channels=("UC1", "UC2")):
|
||||
monkeypatch.setattr(runner, "fetch_channel_feed", fetch)
|
||||
monkeypatch.setattr(runner, "apply_rss_feed", apply_)
|
||||
return runner.run_rss_poll(_StubDb(), [_StubChannel("UC1"), _StubChannel("UC2")])
|
||||
return runner.run_rss_poll(_StubDb(), [_StubChannel(c) for c in channels])
|
||||
|
||||
def test_a_healthy_run_reports_no_failures(self, monkeypatch):
|
||||
out = self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=lambda db, ch, e: 1)
|
||||
assert out == {"new": 2, "failed": 0}
|
||||
|
||||
def test_unreadable_feeds_are_counted(self, monkeypatch):
|
||||
def boom(cid):
|
||||
raise rss.RssError("404")
|
||||
# One dead channel among healthy ones: counted, but the run itself is still a run.
|
||||
def fetch(cid):
|
||||
if cid == "UC_dead":
|
||||
raise rss.RssError("404")
|
||||
return [{"id": "v"}]
|
||||
|
||||
out = self._run(monkeypatch, fetch=boom, apply_=lambda db, ch, e: 1)
|
||||
assert out == {"new": 0, "failed": 2}
|
||||
out = self._run(
|
||||
monkeypatch, fetch=fetch, apply_=lambda db, ch, e: 1, channels=("UC1", "UC_dead")
|
||||
)
|
||||
assert out == {"new": 1, "failed": 1}
|
||||
|
||||
def test_apply_failures_are_counted_too(self, monkeypatch):
|
||||
# A read-only database: every feed reads fine, nothing can be stored.
|
||||
def boom(db, ch, entries):
|
||||
raise RuntimeError("read-only transaction")
|
||||
# A per-row write failure (a constraint clash on one channel) while the rest store fine.
|
||||
def apply_(db, ch, entries):
|
||||
if ch.id == "UC_bad":
|
||||
raise RuntimeError("constraint")
|
||||
return 1
|
||||
|
||||
out = self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=boom)
|
||||
assert out == {"new": 0, "failed": 2}, "a run where nothing was stored must not read as ok"
|
||||
out = self._run(
|
||||
monkeypatch,
|
||||
fetch=lambda cid: [{"id": "v"}],
|
||||
apply_=apply_,
|
||||
channels=("UC1", "UC_bad"),
|
||||
)
|
||||
assert out == {"new": 1, "failed": 1}, "an unstored channel must not read as ok"
|
||||
|
||||
def test_a_quiet_hour_is_distinguishable_from_an_outage(self, monkeypatch):
|
||||
quiet = self._run(monkeypatch, fetch=lambda cid: [], apply_=lambda db, ch, e: 0)
|
||||
assert quiet == {"new": 0, "failed": 0} # same "new", different "failed" — that's the point
|
||||
|
||||
def test_a_total_outage_raises_instead_of_returning_ok(self, monkeypatch):
|
||||
# Every feed unreachable (the tinyproxy-lost-the-boot-race incident). Returning a result
|
||||
# here recorded the scheduler run as "ok", with the outage only visible inside the summary
|
||||
# text — a green pill for a run that did nothing at all.
|
||||
def boom(cid):
|
||||
raise rss.RssError("Connection refused")
|
||||
|
||||
with pytest.raises(RuntimeError, match="all 2 channels"):
|
||||
self._run(monkeypatch, fetch=boom, apply_=lambda db, ch, e: 1)
|
||||
|
||||
def test_a_read_only_database_is_a_total_outage_too(self, monkeypatch):
|
||||
# Every feed reads fine, nothing can be stored: just as un-polled, just as red.
|
||||
def boom(db, ch, entries):
|
||||
raise RuntimeError("read-only transaction")
|
||||
|
||||
with pytest.raises(RuntimeError, match="all 2 channels"):
|
||||
self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=boom)
|
||||
|
||||
def test_no_channels_at_all_is_not_an_outage(self, monkeypatch):
|
||||
# A fresh instance with nothing subscribed must not report an error every poll.
|
||||
out = self._run(
|
||||
monkeypatch, fetch=lambda cid: [], apply_=lambda db, ch, e: 0, channels=()
|
||||
)
|
||||
assert out == {"new": 0, "failed": 0}
|
||||
|
||||
|
||||
class TestRssErrorVsEmpty:
|
||||
"""A failed poll must RAISE (so the caller leaves `last_rss_at` alone); only a genuinely
|
||||
|
||||
Reference in New Issue
Block a user