Skip to content

feat(schedule): show which asset is on screen right now - #3308

Open
mickzijdel wants to merge 12 commits into
Screenly:masterfrom
mickzijdel:feat/highlight-now-playing
Open

feat(schedule): show which asset is on screen right now#3308
mickzijdel wants to merge 12 commits into
Screenly:masterfrom
mickzijdel:feat/highlight-now-playing

Conversation

@mickzijdel

Copy link
Copy Markdown
Contributor

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. Then src/anthias_viewer/__init__.py for the four call sites, and app/consumers.py for 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 as cec:available and the SMART fact — and the render just reads a key.

Two commits, each green on its own:

  1. The fact and the highlight, arriving on the table's existing 5s poll.
  2. The push, so it arrives in milliseconds instead. Purely an optimisation: if Redis is unreachable, the subscription drops or the socket closes, the task ends quietly and the poll goes on keeping the table correct.

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 stop would 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. blank retires the fact outright, because unlike stop it 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 SET carries get=True and the publish only fires when the value moved. The SET itself stays unconditional, since it is what refreshes the TTL.

New design tokens. --color-success is fine as a fill, but its ink partner --color-success-on-wash belongs to the translucent wash and lightens for dark mode, stranding dark text at 1.85:1 there. Adds --color-success-fill / --color-on-success as a stable pair — the split --color-danger already draws — and registers them in the contrast harness and the design-system page so this can't recur silently.

Asset.is_now_playing is 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:

  • /ws is unauthenticated (AllowedHostsOriginValidator only). 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.
  • If CI shows a red upload test in 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

  • I have performed a self-review of my own code.
  • New and existing unit tests pass locally and on CI with my changes.
  • I have done an end-to-end test for Raspberry Pi devices.
  • I have tested my changes for x86 devices.
  • I added a documentation for the changes I have made (when necessary).

mickzijdel and others added 2 commits August 20, 2026 10:12
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
@mickzijdel
mickzijdel requested a review from a team as a code owner August 20, 2026 09:45
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@18c03b7). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3308   +/-   ##
=========================================
  Coverage          ?   90.49%           
=========================================
  Files             ?       87           
  Lines             ?    10058           
  Branches          ?     1109           
=========================================
  Hits              ?     9102           
  Misses            ?      704           
  Partials          ?      252           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mickzijdel and others added 3 commits August 20, 2026 11:23
…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 vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.duration really would be wrong in both directions, and refresh() re-asserting _believed rather 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 through page_context.assets(), so there is no view that renders _asset_row.html with the highlight silently missing.
  • SET ... GET needs Redis >= 6.2. Dockerfile.redis.j2 installs redis-server from 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-success on --color-success-fill is #0e4a30 on #34d399 = 5.35:1, and neither --color-green-500 nor --color-green-900 is redeclared in theme-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:

  1. 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.
  2. self.send() from a bare create_task breaks Channels' send serialisation — a now-playing frame can interleave with asset_update.
  3. The nudge has no rate limit. The 5s poll used to be the ceiling on table renders; it no longer is.
  4. The /ws disclosure 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.

Comment thread src/anthias_server/app/consumers.py Outdated
# exactly this socket's. Note that vendor.ts opens /ws on every
# page, not just the schedule page, so this is one Redis
# subscription per open tab.
self._now_playing_task = asyncio.create_task(self._watch_now_playing())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Redis connection per open tab, on an unauthenticated endpoint.

connect_to_redis_async() builds a fresh client — and therefore a fresh connection pool — on every call, so this is one Redis connection plus one asyncio task per WebSocket. Your own comment notes vendor.ts opens /ws on every page, so that is already one per tab rather than one per schedule page.

The part that makes it more than a housekeeping concern: /ws has no auth, and AllowedHostsOriginValidator is a no-op under the default ALLOWED_HOSTS=['*'] (asgi.py says so explicitly). WebSocket handshakes are not subject to CORS, so any page the operator happens to have open — not just something already on the LAN — can open sockets in a loop and pin a Redis connection and an event-loop task each, on a board that may have 512 MB.

Suggested shape: one process-wide subscriber task that re-broadcasts onto the existing WS_GROUP via group_send with a new handler type, exactly as notify_asset_update already does. That makes it one Redis subscription per server process regardless of tab count, reuses the fan-out path that is already tested, and fixes the concurrency issue below as a side effect.

Comment thread src/anthias_server/app/consumers.py Outdated
data = message.get('data')
if not isinstance(data, str):
continue
await self.send(text_data=data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending from an independent task breaks Channels' serialisation guarantee.

AsyncWebsocketConsumer runs its handlers one at a time in the consumer's own dispatch loop, which is why asset_update never has to think about a concurrent send. This task sits outside that loop, so a now-playing frame can land while asset_update is mid-send — an upload finishing during a rotation is enough. Concurrent websocket.send on a single ASGI connection is not something uvicorn's ws implementations promise to serialise for you.

It will be rare, and the symptom would be an ugly one to debug from a field report. Routing through the channel layer (see the comment on connect) removes the concurrency rather than making it safe.

Comment thread src/anthias_server/app/consumers.py Outdated
if not isinstance(data, str):
continue
await self.send(text_data=data)
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but worth weighing against the module's own logging philosophy: except Exception at DEBUG means a genuine defect here — a renamed redis-py API, a pubsub() signature change, aclose() going away — silently disables the feature with nothing in the journal at the default level. The tests mock the client end to end, so they would not catch it either.

now_playing already carries a _warn_once latch built for exactly this trade-off (warn on the first failure per process, DEBUG after). Reusing it here would keep the quiet-by-default behaviour for the expected "no Redis" case while still surfacing the unexpected one.

Comment thread src/anthias_server/app/consumers.py Outdated
# CancelledError only: the task body already swallows Exception
# on both its paths, so a broader suppress would be dead code
# that also swallowed a cancellation aimed at disconnect itself.
with contextlib.suppress(asyncio.CancelledError, TimeoutError):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the comment says a broader suppress "would also swallow a cancellation aimed at disconnect itself" — but suppressing asyncio.CancelledError here does exactly that. If the server is shutting down and disconnect() is itself cancelled, wait_for raises CancelledError, this swallows it, and disconnect returns as though nothing happened.

task.cancel() followed by await asyncio.wait({task}, timeout=self.NOW_PLAYING_TEARDOWN_S) gives you the same bounded wait without absorbing a cancellation meant for the caller.

# turns one into a full table re-render, and a single-asset
# playlist would otherwise pay that on every loop for no news.
# ``get=True`` returns the previous value (Redis >= 6.2).
previous = client.set(NOW_PLAYING_KEY, asset_id, ex=TTL_S, get=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dedup handles the single-asset playlist, but nothing caps the rate when the asset genuinely changes.

clamp_duration floors at 0, so a playlist of short- or zero-duration images rotates as fast as the display loop turns. Every one of those rotations is a distinct value, so it clears the previous != asset_id check, publishes, and each open tab turns that into a full _asset_table.html fetch — Asset.objects.all(), a Redis read, and a template render, per tab, per rotation.

Before this change the 5s poll was a hard ceiling on how often that could happen. It no longer is, and the direction of the regression is toward the weakest hardware. The PR body's framing — the push "only decides whether the operator sees it land with the picture or up to 5s later" — is right about the benefit but understates the cost at the fast end.

A floor on announcement frequency here (say, no more than one publish per second, keeping the unconditional SET for the TTL) would preserve the felt-instant case that motivates the feature while restoring a bound. Debouncing the htmx trigger in vendor.ts would work too, but server-side is cheaper and covers every client.

#: nudges open browsers so the highlight arrives with the picture
#: rather than up to 5s later. Its own channel, not the viewer command
#: bus: the audience here is browsers, not the viewer.
NOW_PLAYING_CHANNEL = 'anthias.now_playing'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the /ws disclosure you flagged in the PR body — worth restating with the reach it actually has, because the note reads as narrower than the exposure.

With the default ALLOWED_HOSTS=['*'], AllowedHostsOriginValidator gates nothing, and WebSocket handshakes are not CORS-gated. So this is not only readable by something already sitting on the LAN: any website the operator visits while their device is reachable can open ws://<device>/ws in the background and record a continuous feed of asset UUIDs and rotation timing. That is a fair bit more than "an unauthenticated listener".

The cheap part: the browser does not use the payload at all — vendor.ts fires refresh-assets on any message and ignores the body. Publishing an empty frame (or a fixed sentinel) on this channel would remove the disclosure outright and cost nothing, and the poll already carries the actual id over the authenticated HTTP path. Worth doing here rather than deferring, since "flagging rather than fixing" is what turns into a permanent property.

return redis.Redis(host='redis', decode_responses=True, port=6379, db=0)


def connect_to_redis_async() -> 'redis.asyncio.Redis':

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things follow from this being a factory rather than a cached client, and they are only visible at the call site:

  • Every call is a new connection pool. The one caller today invokes it per WebSocket (see consumers.py), so pool-per-tab is the real behaviour.
  • No socket timeout is set, which the NOW_PLAYING_TEARDOWN_S comment in consumers.py already identifies as the reason teardown can stall against a wedged Redis. Setting socket_timeout / socket_connect_timeout here would make that bound unnecessary rather than worked around.

If the fan-out moves to a single process-wide subscriber, a module-level lazily-created client would be the natural shape and both points go away.

mickzijdel and others added 7 commits August 26, 2026 18:25
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>
@mickzijdel
mickzijdel force-pushed the feat/highlight-now-playing branch from b918890 to 1a8aa42 Compare August 26, 2026 18:49
@sonarqubecloud

Copy link
Copy Markdown

@mickzijdel

Copy link
Copy Markdown
Contributor Author

I addressed all 7. 1, 2 and 4 shared a root cause, so they're fixed in one go.

# Fix
1 _watch_now_playing is one module-level task re-broadcasting via group_send, your suggested shape. One Redis connection per server process. Holders tracked by channel name in a set, released in a finally, since group_discard raises when the layer's Redis is down and Channels lets that escape.
2 Gone with that. Every frame goes through group_send, so nothing sends from outside the dispatch loop.
3 Uses the warn-once latch. I pulled it into anthias_common/warn_once.py rather than paste a fourth copy, one instance per module so each line keeps its own logger name.
4 Block deleted rather than reworked. The task outlives sockets now, so there's nothing left to bound.
5 1s floor on announcements, SET still unconditional so the liveness TTL keeps refreshing. You're right that min_value=0 on duration means nothing throttles rotation.
6 Payload dropped, '*' sentinel instead. See below.
7 Both timeouts set. See below.

On 6: Narrowed but not closed. notify_asset_update still carries real ids over the same socket on every write, and the frame still goes out the instant the viewer rotates, so timing leaks even with nothing in it. Closing it would mean implementing auth on the websocket route.

On 7: socket_connect_timeout=5 and socket_timeout=5. Safe on a pub/sub client under the pinned redis-py 8.1.0, which passes math.inf for blocking reads as an explicit opt-out from socket_timeout, so a read waiting for the next rotation is untouched and reconnect/AUTH/HELLO get bounded. That same opt-out is why neither timeout spots a half-open socket, and health_check_interval can't do it alone: it only fires from check_health, which runs when parse_response is re-entered, so a blocking listen() never PINGs. The subscriber reads with get_message(timeout=30) in a loop instead, which is a ceiling on one read rather than a delay.

Tests. You were right that they mock the client end to end. tests/test_now_playing_integration.py drives the real client and a real RedisChannelLayer, including the SET ... GET version dependency you checked Dockerfile.redis.j2 by hand for. Mutation-checked rather than trusted green: forward the real payload and the disclosure assertion fails, subscribe to the wrong channel and both bridge tests fail.

Just to flag: the warn_once extraction touches undervoltage.py and storage_health.py to extract a shared function that is also used by the new code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Anthias Webpage - requesting highlighting the actual asset being played

2 participants