feat(schedule): show which asset is on screen right now - #3308
mickzijdel wants to merge 15 commits into
Conversation
The Schedule Overview could not answer the one question an operator asks while standing in front of the screen: which of these is playing? With shuffle on, play order says nothing about it, so the only way to tell today is to go and look at the TV (Screenly#3177). The viewer already knows — scheduler.current_asset_id — but only answered on request, over the blocking BLPOP round trip behind /api/v1/viewer_current_asset. The table re-renders every 5s for every open browser, so asking per render would wake the display loop on each poll. It publishes the id to Redis instead, the same shape as cec:available and the SMART fact, and the render just reads a key. The TTL is liveness, not content: a 3-minute window kept alive by a refresher on a 1-minute tick, matching the display-resolution fact. Deriving it from the asset's own duration was the first instinct and was wrong in both directions — durations run to a year, so a viewer that died mid-rotation would have gone on claiming a row for months, while a viewer paused with `stop` would have dropped the highlight off a picture still on the screen. Tying the fact to "the viewer said something recently" gets both right without either knowing about the other. `blank` retires the fact outright, because unlike `stop` it leaves nothing on screen to point at. The named row gets a tinted background and a solid "Playing now" chip. Nothing is highlighted when the viewer hasn't reported, so a stopped viewer or an unreachable Redis shows no highlight rather than a stale guess. The chip needed a token the design system didn't have. --color-success is safe as a fill, but its ink partner --color-success-on-wash belongs to the translucent wash and lightens for dark mode, which strands dark text at 1.85:1 there. Adds --color-success-fill / --color-on-success as a stable pair, exactly the split --color-danger already draws, and puts them in the contrast harness so the next person can't repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiKQ8i8nJRJudQxSVtdkCi
…e poll The highlight was correct but late: the table asks the server for a fresh render every 5s, so an operator stepping through assets with Next watched the screen change and the page catch up a beat later. The viewer now announces each change on a pub/sub channel of its own, and the WebSocket consumer subscribes for the life of a socket and nudges its browser — the same "something changed, re-fetch the table" frame the consumer already sends on writes, which vendor.ts turns into an htmx refresh. Measured at 1-12ms from the viewer's publish to the frame leaving the consumer, against a real Redis. Only actual changes are announced. Every announcement costs every open browser a full table render, and a single-asset playlist rotates forever with no news to report, so the SET carries `get=True` and the publish only fires when the value moved. The SET itself stays unconditional because it is what refreshes the liveness TTL. Nothing here is load-bearing: no Redis, a dropped subscription or a closed socket ends the task quietly and the 5s poll goes on keeping the table correct. The subscription is per-connection, so it dies with its socket — note that vendor.ts opens /ws on every page, not just the schedule page, so it is one Redis subscription per open tab. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiKQ8i8nJRJudQxSVtdkCi
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3308 +/- ##
=========================================
Coverage ? 90.47%
=========================================
Files ? 87
Lines ? 10058
Branches ? 1109
=========================================
Hits ? 9100
Misses ? 705
Partials ? 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…the key Review found the liveness TTL didn't deliver the one property it was added for. refresh() was a bare EXPIRE, so it extended whatever sat in Redis regardless of who put it there or whether this process had ever displayed anything. A viewer that restarts starts ticking before wait_for_server and the splash, so it inherited its dead predecessor's claim and renewed it for the whole ~60-120s boot while its own screen showed the splash page. A viewer crash-looping faster than the TTL — the Sentry ANTHIAS-3 class this file already documents — renewed it forever, which is exactly the stale claim the TTL was supposed to end. The comparison to the display-resolution reporter was what hid it: that one re-derives its value every tick, so it can only assert something currently true. This one extended a value it never re-derived. Now it re-asserts the module's own memory of what it last put on screen, and does nothing at all until this process has put something there. Two things fall out of using SET rather than EXPIRE. The fact now survives Redis losing it — an unclean restart inside the fsync window, a flushed volume, an eviction — where before a pinned hour-long dashboard would have gone unhighlighted until it finally rotated, the very case the refresher exists to serve. And clear() retires it for good instead of for one tick. Also closes a race between the two threads. blank_display() runs on the subscriber thread and retires the fact, but a rotation already past its own check on the main thread could re-create it microseconds later, and with the loop then parked on loop_is_stopped nothing would ever retire it again — a highlight pinned to a black screen. The refresher tick now reconciles that, and boot clears whatever a previous process left behind rather than waiting for it to expire. Verified against a real Redis: an inherited key's TTL is left to decay (100s stayed 100s), this process's own is renewed to 180, a flushed Redis is repopulated on the next tick, and a cleared fact stays gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiKQ8i8nJRJudQxSVtdkCi
Review found disconnect() gated group_discard behind the subscription task's teardown. That teardown closes a Redis connection, and connect_to_redis_async sets no socket timeout, so redis-py's close wraps its wait in async_timeout(None) — a half-open socket to a wedged Redis stalls it with no ceiling. The channel name would then stay in ws_server and every later notify_asset_update would fan out to a dead channel. group_discard now runs first and unconditionally, and the wait for the task is capped. Narrows the suppress to CancelledError and TimeoutError. The Exception arm was dead code — the task body catches Exception on both its paths, so nothing but a BaseException can escape it — and it also swallowed a cancellation aimed at disconnect() itself, making the ASGI server's own teardown timeout unable to interrupt it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiKQ8i8nJRJudQxSVtdkCi
…ptions The warn-once latch never reset, so the first Redis blip after a container start silenced that call site at DEBUG for the life of the process — days to weeks for anthias-server. Since each key latches a whole `except Exception`, a genuinely different fault afterwards (a WRONGTYPE from a key something else wrote, a decode failure) would never be seen. It now re-arms on the next success, which is warn-once-per-outage rather than warn-once-ever. The sibling helpers in undervoltage and storage_health latch one narrow branch each, so they don't have this problem to solve. Two descriptions also drifted from the code during the TTL rework: a comment in test_viewer.py still said the TTL came from the clamped duration, which stopped being true when it became a liveness window, and CLAUDE.md named the pub/sub channel where it meant the key the server reads. Both now say what the code does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiKQ8i8nJRJudQxSVtdkCi
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Reviewed this as untrusted input, including a pass for anything hostile hiding in the diff. Nothing malicious found. I checked out the branch, ran the targeted suites (41 new/adjacent tests, 516 in test_viewer.py + test_template_views.py + api/tests/test_assets.py) and ruff check / ruff format --check — all green here too.
One note on process rather than code: this PR edits CLAUDE.md and .claude/skills/anthias-viewer/SKILL.md, which steer how agents behave in this repo. I read both hunks in full — they are factual and match the code, no injected directives — but instruction files arriving from a fork deserve a deliberate human read rather than being skimmed as docs.
What holds up
The reasoning in the module docstring is right where it counts, and I verified the parts that were checkable rather than taking them on faith:
- The TTL-as-liveness argument. Deriving it from
Asset.durationreally would be wrong in both directions, andrefresh()re-asserting_believedrather than blind-EXPIREing is the correct call — a crash-looping viewer renewing its dead predecessor's claim forever is exactly the failure the TTL exists to end. - No surface left unflagged. All three render paths (
views.py:209,:221,:1494) go throughpage_context.assets(), so there is no view that renders_asset_row.htmlwith the highlight silently missing. SET ... GETneeds Redis >= 6.2.Dockerfile.redis.j2installsredis-serverfrom Debian trixie, so this is fine — worth having confirmed, because the failure mode would have been a silently dead feature (warn once, then DEBUG forever).- The contrast claim.
--color-on-successon--color-success-fillis#0e4a30on#34d399= 5.35:1, and neither--color-green-500nor--color-green-900is redeclared intheme-dark.css, so the pair really is stable across both themes.
What I'd want changed
The substantive findings are all in the push half (commit 2), and the first two share one fix. Details inline; summarised here:
- One Redis connection and one pub/sub subscription per open browser tab, on an endpoint that is unauthenticated and (with the default
ALLOWED_HOSTS=['*']) not origin-gated either. self.send()from a barecreate_taskbreaks Channels' send serialisation — a now-playing frame can interleave withasset_update.- The nudge has no rate limit. The 5s poll used to be the ceiling on table renders; it no longer is.
- The
/wsdisclosure you flagged reaches further than the note says — and costs nothing to fix, since the client ignores the payload.
All four are in the layer the PR body itself describes as "purely an optimisation". Routing the fan-out through the existing channel layer instead of a per-socket subscription addresses 1, 2 and 4 at once and reuses the notify_asset_update path that is already there. Commit 1 (the fact and the highlight) I have no reservations about.
now_playing added the third copy of this helper, and the review asks for a fourth caller in consumers.py. That settles the "sibling modules with no dependency" argument the second copy was justified with. WarnOnce is an instance per module rather than one shared set, because both alternatives bite: the line keeps its own module's logger name, so the journal still says which subsystem noticed, and the keys stay namespaced. undervoltage and storage_health both latch on 'no_boot_id' and neither may silence the other. The optional exception argument and the re-arm-on-success behaviour come from the now_playing copy. The other two never passed an exception, so their output is byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dedup stops a single-asset playlist announcing forever, but nothing capped the rate when the asset genuinely changes. duration may be 0 (the v2 serializer says min_value=0), so a playlist of zero-duration assets rotates as fast as the display loop turns, and every rotation is a distinct value that clears the dedup. Each announcement costs every open browser a full _asset_table.html render: Asset.objects.all(), a Redis read and a template render, per tab, per rotation. The 5s poll used to be a hard ceiling on that and stopped being one when the push landed, with the regression pointing at the weakest hardware in the fleet. The SET stays unconditional, because it is what refreshes the liveness TTL. Only the announcement is gated, and a dropped one costs at most one poll interval of staleness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review asked for a socket timeout on connect_to_redis_async, and it was right: nothing bounded the initial dial or the SUBSCRIBE. Both timeouts are safe on a pub/sub client under the pinned redis-py 8.1.0. PubSub.parse_response hands read_response math.inf for a blocking read, which that version documents as the per-read opt-out from socket_timeout, so a read legitimately waiting for the next rotation is never cut short. Reconnect, AUTH, HELLO and resubscribe do not pass math.inf, so they stay bounded, which is the half that was actually missing. The same opt-out is why socket_timeout cannot answer the other half of the finding: it cannot notice a half-open socket under a blocking read. Nor can health_check_interval on its own, because it only fires from PubSub.check_health, which runs when parse_response is re-entered. Detecting a wedged subscription is the caller's job; the docstring says so and the next commit does it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The subscription lived on the consumer instance, so every open socket got its own. connect_to_redis_async builds a fresh client, and so a fresh pool, per call; vendor.ts opens /ws on every page, not just the schedule page; and /ws has no auth, with AllowedHostsOriginValidator a no-op under the default ALLOWED_HOSTS=['*']. So anything the operator's browser could reach was able to pin a Redis connection and an event-loop task per socket it opened. On a 512 MB board that is not housekeeping. Moving the subscription to one process-wide task that re-broadcasts via group_send answers four of the review's findings at once: - One Redis connection for the server, regardless of tab count. - No send from outside the consumer's dispatch loop, so a now-playing frame can no longer interleave with an in-flight asset_update. Channels serialises handlers; a bare create_task was outside that. - The bridge drops the payload and re-sends the '*' sentinel the write paths already use. vendor.ts fires htmx refresh-assets on any message and never reads the body, so the id bought the browser nothing. This narrows the disclosure rather than closing it, and the comment says so: notify_asset_update still carries real ids on every write, and the frame's timing still marks each rotation. Closing it means auth on /ws, which is a bigger change than this PR. - The bounded-teardown block in disconnect() goes away with the per-socket task, and with it a suppress(CancelledError) whose comment argued against exactly what it did. A refcount stops the task when the last socket closes, so an idle server holds no subscription and shutdown leaves nothing pending. The release sits in a finally, because group_discard raises when the channel layer's Redis is unreachable and Channels lets that escape; skipping the release would ratchet the count up for good and strand the subscription with no sockets behind it. The task is cancelled rather than awaited, because a cancelled task is not an unretrieved exception and so costs no asyncio ERROR log (and no Sentry event); the reference is kept, since the loop holds only a weak one. acquire() restarts a task that is done or already cancelling, so a server that outlives a Redis outage retries on the next browser connect instead of staying poll-only, and a browser arriving mid-teardown doesn't inherit a task on its way out. Reads use get_message with an explicit timeout rather than listen(), which is what gives health_check_interval anything to do: PubSub.check_health only runs when parse_response is re-entered, so a blocking listen() never PINGs and a half-open socket would wedge the push for the life of the process. Failures go through a warn-once latch instead of a DEBUG line. "No Redis" is expected and stays one line, but a renamed redis-py API would otherwise disable the push with nothing in the journal at the default level, and the tests mock the client end to end so they would not catch it either. The latch is this module's own, so the line is filed under the server's logger rather than the viewer-side module's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several blocks had outgrown what they guard: the now-playing module docstring restated in prose what the function docstrings already say, the tailwind token comment re-derived a contrast figure that test_design_tokens.py now enforces, and the refresher's race note ran seven lines for a three-line body. The reasoning the review specifically checked and endorsed is untouched: the TTL is liveness not content, and refresh() re-asserts what this process displayed rather than EXPIREing whatever is in Redis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pre-empting the objection the process-wide subscriber invites: it replaced a per-socket lifetime, which is correct by construction, with bookkeeping that can desync. A set of channel names removes that class outright. A double release, or a connect whose disconnect never ran, is idempotent instead of leaving the arithmetic permanently off, and there is no counter to clamp at zero. The read timeout goes from 1s to 30s. It was never a delay: get_message returns the moment a message lands, so the timeout is only a ceiling on one read, and its single job is to re-enter parse_response often enough for PubSub.check_health to fire. Matching the client's health_check_interval probes an idle connection about twice a minute instead of waking the event loop sixty times, which matters on a Pi 1. now_playing's latch is private again, now that the consumer has its own and nothing outside the module keys into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unit suites mock the Redis client wholesale, which the review called out and which then proved itself: swapping listen() for get_message(timeout=...) broke every mocked test on a missing attribute rather than on behaviour. That is a suite reporting on itself instead of on the code, and it leaves two real failure modes uncovered. - SET ... GET needs Redis >= 6.2. On an older server the write raises, the warn-once latch swallows it, and the highlight is dead for the life of the process with one line in the journal. A reviewer checked Dockerfile.redis.j2 by hand to rule this out; now the suite does. - The subscriber is coupled to redis-py's pub/sub API and to the channel layer's message shape, and nothing pinned either. So these four drive the real client and the real RedisChannelLayer: publish/read/refresh round trips including the TTL, and the whole bridge end to end, a viewer-side publish coming out as an asset_update on the group with the sentinel rather than the asset id. The two bridge scenarios run on a private loop in their own thread. Not asyncio.run() on the calling thread, and not pytest-asyncio, anyio's plugin or asgiref.async_to_sync either: the Playwright sync API keeps a loop running on the thread pytest calls tests on, for the life of the session, and all of those want to drive a loop there too. CI runs the whole integration suite in one process, so anything that passes when this file runs alone has proved nothing. Verified by mutation, not just by passing: forwarding the real payload instead of the sentinel fails the disclosure assertion, and subscribing to the wrong channel fails both bridge tests. Both checked against the full integration suite in the test container. They need the Docker stack, where 'redis' resolves, and skip anywhere else rather than failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b918890 to
1a8aa42
Compare
|
I addressed all 7. 1, 2 and 4 shared a root cause, so they're fixed in one go.
On 6: Narrowed but not closed. On 7: Tests. You were right that they mock the client end to end. Just to flag: the |
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Re-reviewed at 1a8aa420 against the seven findings from the 2026-08-24 pass. All seven are genuinely addressed — I checked each against the code rather than against the summary. Three small things are left, none blocking; two are comment-only and inline below.
| # | Finding | Status |
|---|---|---|
| 1 | Redis connection + task per open tab | Fixed. _watch_now_playing is one module-level task, held by a set of channel names. test_one_subscription_no_matter_how_many_tabs asserts connect.call_count == 1 across three tabs. |
| 2 | self.send() from a bare create_task |
Fixed. Every frame goes group_send → asset_update, inside the consumer's own dispatch loop. |
| 3 | except Exception at DEBUG hides a real defect |
Fixed. consumers._warn is the module's own WarnOnce, re-armed via worked('subscription'); the test pins exactly one WARNING across two failures. |
| 4 | suppress(CancelledError) in disconnect() |
Resolved by deletion. The per-socket teardown block is gone. |
| 5 | No ceiling on the browser nudge | Fixed. MIN_ANNOUNCE_INTERVAL_S = 1.0 gates only _announce; the SET stays unconditional. Both the floor and the release-after-the-floor are tested. |
| 6 | /ws disclosure reaches further than the note said |
Narrowed as agreed. The id never leaves the server, pinned by a unit test and a mutation-checked integration test. Timing still leaks, which the code says out loud. |
| 7 | No socket timeout on connect_to_redis_async |
Fixed, and the reasoning holds. |
What I verified rather than took on faith
The redis-py argument behind #7. This was the one that could have been subtly wrong — the math.inf opt-out is usually cited for the sync client, and this is the async one — so I read redis-py 8.1.0's source:
asyncio/client.py,PubSub.parse_response:read_timeout = math.inf if block else timeout, with a comment naming it the per-read opt-out fromsocket_timeout.asyncio/connection.py,read_response:if timeout == math.inf: read_timeout = None, elsetimeout if timeout is not None else self.socket_timeout.
So the claim holds on the async path too: socket_connect_timeout=5 / socket_timeout=5 bound the dial, SUBSCRIBE and the retry layer's reconnect/AUTH/HELLO, and never a read waiting for the next rotation.
The detail neither the PR body nor the commit message mentions, and which matters more for this design: with an explicit numeric timeout a timed-out read returns None rather than raising (except asyncio.TimeoutError: if timeout is not None: return None). That is what stops get_message(timeout=30) from killing the task every 30s on an idle device, and test_the_subscription_survives_an_idle_stretch is pinning exactly that. Worth a line in the docstring, since the whole loop depends on it. check_health() does run at the top of parse_response, so pairing the 30s read ceiling with health_check_interval=30 gives the PING somewhere to fire.
client.aclose() alone is sufficient cleanup. No pool is passed in, so auto_close_connection_pool defaults to True and aclose() disconnects the pool including the pub/sub's in-use connection. No connection leak per subscriber restart.
The contrast pair. --color-green-500 (#34d399) and --color-green-900 (#0e4a30) are each declared exactly once, in palette.css, and never redeclared per theme — the pair really is stable in both, and the INK_ON_FILL entry keeps it that way.
The integration tests' private-loop-per-thread trick is sound. channels-redis 4.3.0's RedisChannelLayer caches per event loop (self._layers[loop] plus a wrapped loop.close), so a fresh loop per test can't inherit connections from a closed one.
No gap on unblank. start_loop parks while loop_is_stopped, so unblank re-enters asset_loop within 0.1s and republishes — there is no window where the screen is live but the fact has been retired.
All 11 checks green on this head, including the test_v1_endpoints.py upload test that flakes under xdist (#3307).
What's left
Two comment-only tidies (inline) and one behavioural note (inline on _acquire_now_playing_watcher). None of them blocks: fix the first two and this is good to merge. The third and the residual /ws timing leak both belong in follow-up issues — the leak in particular can't be closed without auth on /ws, and notify_asset_update already emits real asset ids over that same unauthenticated socket on every write, so this PR isn't what introduces the exposure.
The rework that swapped the counter for a set of channel names spliced its replacement into the middle of an existing sentence, so the one comment explaining why the subscriber is process-wide at all lost the half that said what a per-socket subscription would cost: "claim a Redis connection and a" then jumped straight to "Per *process*". The line also ran to 117 characters in a 79-column file, which nothing caught because E501 is not in the enabled ruff set. Three test docstrings and the viewer skill still described the counter the same rework removed, so they argued for the design it replaced. The skill matters most of the three, since that file is what a later agent reads instead of the code. Also records why the read loop can use a timeout at all: an expired read returns None rather than raising, because read_response only turns a timeout into TimeoutError when it came from socket_timeout. Without that an idle device would kill the task every 30s, so the whole loop rests on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed those; will continue with the other PRs later as I got a bit swamped! |
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Follow-up review at 39363bf8, continuing the 24 Aug and 1 Sep threads. This round re-checked the comment fixes and then took the branch to real hardware — a Pi 5 (arm64) and an x86 board, both running the PR's changed Python, templates and rebuilt CSS over release 353426a.
The comment cleanups from 1 Sep all landed, and the earlier seven findings remain addressed: one subscriber across three real browser tabs on each board, group_send for every frame, the warn-once logger, release in finally, the one-second announcement floor with the unconditional TTL write, the '*' sentinel in place of the UUID, and the async Redis timeouts. All 11 CI checks are green.
One new finding, and it does block merge — details inline on now_playing.py. In short: _believed is only updated after the Redis SET returns, so a write that raises leaves the refresher re-asserting the previous asset every 60s, and the 5s poll reads the same wrong key. It self-corrects only on the next rotation, which for a long-duration asset can be an hour away and while playback is paused is never. Reproduced on the x86 board with a real SET failure: the viewer's own /api/v1/viewer_current_asset reported the new asset while Redis and the browser chip both showed the old one, and the wrong key's TTL climbed from 123s to 165s after recovery. CI does not cover this case.
Everything else on hardware came out the way the PR says it should:
| Check | Result |
|---|---|
| Four-asset rotation, both boards | All three tabs matched all four assets; TTL 179s at sampled transitions |
| Push latency, x86 | 346 / 396 / 581 ms from rotation to highlight |
| Poll fallback, x86 | 4.65s with WebSocket creation disabled — degrades exactly as described |
| Paused liveness, Pi 5 | Highlight held 193.8s on a paused asset |
| Dead viewer, Pi 5 | Key expired and the browser cleared it 181.8s after the viewer stopped |
| Stop / blank / unblank | stop keeps the fact, blank retires it, unblank restores it |
| API contract, x86 | is_now_playing absent from v1, v1.1, v1.2 and v2 responses |
| Selection and themes | Chip survives row selection; rgb(52, 211, 153) on rgb(14, 74, 48) in both themes |
These were headless integrated tests of the software path, not verification of physical HDMI output, and there is no armhf, Pi 4/eglfs or Rock Pi coverage in this run. Both boards have been restored to their original images, playlists and Redis config.
The two accepted follow-ups — automatic push recovery after a Redis outage for already-open tabs, and authentication on /ws for the residual timing disclosure — are tracked separately and are not expected in this PR.
One housekeeping note: the review threads from the earlier rounds are still marked unresolved on GitHub even where the code has been fixed, so those flags no longer reflect the state of the branch.
_believed was assigned after the SET returned, inside the try. A SET that raised left playback moving on to the new asset while this process kept believing the previous one, and refresh() then re-asserted that stale id every tick with a full TTL once Redis was back. The 5s poll reads the same key, so it could not repair it; only the next publish could, and while playback is stopped that never comes. The refresher runs regardless of loop_is_stopped by design, so the wrong row was pinned until the viewer process restarted. Reproduced by the reviewer on an x86 board by denying only SET: the viewer's own /api/v1/viewer_current_asset reported the new asset while Redis and the Playing-now chip showed the old one, and the wrong key's TTL climbed from 123s to 165s after recovery. The assignment moves before the write and outside the try, which is the order clear() already used and what the module docstring promised: refresh() re-asserts what this process last put on screen, not what Redis last accepted. test_publish_swallows_redis_errors drove exactly this path and asserted nothing, which is why CI was green. It now checks the belief advanced, and a new test pins the full sequence: A publishes, B's SET fails, Redis recovers, refresh() must write B. Both failed before the move. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
_watch_now_playing ended on any exception and was only restarted from _acquire_now_playing_watcher, i.e. on a new WebSocket connect(). The browser's socket terminates at uvicorn rather than at Redis, so a blip never closes it and vendor.ts never reconnects — nothing fired the restart. Every tab that was open across a `docker compose restart redis` stayed poll-only until someone reloaded the page, and the same went for a failed group_send, since the channel layer points at the same Redis. Correctness was never at stake; the 5s poll is the documented fallback. What was wrong is that the restart trigger was uncorrelated with Redis coming back, so "a server that outlives an outage retries" read stronger than it behaved. The attempt is now its own coroutine and the task loops over it with a backoff. Bounded in the delay rather than in the number of attempts: the task only exists while a browser holds a socket open, so an attempt ceiling would reinstate the same bug for any outage that outlasted it. 1s doubling to a 60s cap, so a device that has been unreachable all night costs one dial a minute. Recovery has to be told apart from a flap. A Redis that accepts SUBSCRIBE and drops the connection a second later is the same fault continuing, so the backoff reset and the warn-once re-arm both wait on the subscription holding for 60s. Without that gate a flapping server would warn once a second — the journal-flooding warn_once exists to prevent (Screenly#3268). An outage is one WARNING either way. Note that redis-py absorbs a bare connection drop: PubSub.on_connect re-issues SUBSCRIBE, so CLIENT KILL against a live server never reaches the task. Only an outage longer than that retry budget does, which is why the integration test puts a TCP hop in front of Redis and closes it rather than killing the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
|
Heads up: this PR currently has merge conflicts with Flagged by an automated PR-hygiene sweep — no action needed beyond the rebase, and no reply expected. |
2 participants
Issues Fixed
Closes #3177.
Description
The Schedule Overview couldn't answer the question an operator asks while standing in front of the screen: which of these is playing? With shuffle on, play order says nothing about it, so the only way to tell was to go and look at the TV.
Start with
src/anthias_common/now_playing.py— it is the whole protocol in ~140 lines, and its module docstring carries the reasoning the rest of the diff follows from. Thensrc/anthias_viewer/__init__.pyfor the four call sites, andapp/consumers.pyfor the push half.The viewer already knows the answer (
scheduler.current_asset_id) but only answered on request, over the blocking BLPOP behind/api/v1/viewer_current_asset. The table re-renders every 5s per open browser, so asking per render would wake the display loop on every poll. It publishes the id to Redis instead — the same shape ascec:availableand the SMART fact — and the render just reads a key.Two commits, each green on its own:
The TTL is liveness, not content. Deriving it from the asset's own duration was the first instinct and was wrong in both directions: durations run to a year, so a viewer that died mid-rotation would keep claiming a row for months, while a viewer paused with
stopwould drop the highlight off a picture still on screen. It is now a fixed 180s window refreshed on a 60s tick, matching the display-resolution fact.blankretires the fact outright, because unlikestopit leaves nothing on screen to point at.Announcements are deduped. Each one costs every open browser a full table render, and a single-asset playlist rotates forever with no news, so the
SETcarriesget=Trueand the publish only fires when the value moved. TheSETitself stays unconditional, since it is what refreshes the TTL.New design tokens.
--color-successis fine as a fill, but its ink partner--color-success-on-washbelongs to the translucent wash and lightens for dark mode, stranding dark text at 1.85:1 there. Adds--color-success-fill/--color-on-successas a stable pair — the split--color-dangeralready draws — and registers them in the contrast harness and the design-system page so this can't recur silently.Asset.is_now_playingis a transient attribute, not a column: no migration, and a new test asserts it stays out of all four API versions' responses.Two notes for reviewers:
/wsis unauthenticated (AllowedHostsOriginValidatoronly). It already emitted asset ids on writes, but it now carries a continuous feed of asset UUIDs and rotation timing to any unauthenticated listener. Not a media path —/anthias_assets/is gated to the Docker bridge CIDR — so this is disclosure of what is on screen and when it changes. Flagging rather than fixing here.api/tests/test_v1_endpoints.py, that is the pre-existing xdist flake filed as Test suite flakes under pytest -n auto: cleanup_asset_dir wipes an asset directory shared by all xdist workers #3307, not this change.Checklist