Stop writing the alert to the sign twice on every refresh

[mjaksn]

Handing the sign back so a picture write lands means putting the alert on again
afterwards, and the sign restarts an alert when it takes one. Nothing told the
callers that had happened, so they re-asserted it straight after and restarted
it a second time for nothing: once per refresh interval, for as long as the
alert was up, and again on every reboot.

SlotRegistry.refresh and SlotRegistry.reboot now report whether they put the
alert back, and the alert is re-asserted only when they did not. That case is
real and is the one this must not break: a service with no icons never hands the
sign back, so nothing else would put the alert on a sign that was power cycled
mid-alert.

Two smaller things found while measuring this one.

An alert with no message, which only a state file written by an older version
can hold, was written back to the priority file. The body is empty but the
formatting bytes around it are not, so the sign reads a blank priority message
and holds the display dark. At startup that was two extra things a person sees,
a dark sign and then the release, for an alert about to be thrown away. It is
now released and forgotten wherever it is found rather than only by the startup
path, which a sign unreachable at startup could stop reaching.

And a reconnect re-pushed the slots without re-asserting the alert, so a link
that came back in front of a sign that had lost one left the priority file empty
until the next tick of the timer. Both the timer and the reconnect hook now go
through one _refresh_and_reassert. That one has a cost rather than a saving:
a reconnect where nothing was actually lost now writes the alert once where it
wrote nothing before. It is the right trade, but it is a write added in a change
about removing them.

What this does not fix

A refresh while an alert is up still costs a release, one blank per picture
file, and the alert restarting. That is not redundancy, it is the price of the
hand-back on a re-push that assumes the worst: refresh forgets what the sign
holds and rewrites everything, so every picture goes out every interval whether
or not the sign already has it.

docs/protocol-notes.md under "Reading state back" has the way out, and PR #77
measured the half that was missing: a J reply says plainly whether a picture
file was ever written. Asking would turn the blind re-push into a comparison and
take the common case to no writes at all. That is a change to what refresh
promises and it wants the sign to validate, so it is not in here.

Checking it

tests/test_pictures.py::TestNotWritingTheAlertTwice is the new work. Each test
was run against the code before the change: the central one fails with
assert 2 == 1, the two writes being the identical alert A0 ESC SP b FIRE
sent twice, and the empty-alert one leaves the alert recorded as active with the
sign showing the rotation.

reboot returned how many slots it restored, which only tests read; it returns
the report instead, and the count is still in its log line and in list_slots.

[Copilot]

🟡 Changes recommended

Concurrent refresh/reassert operations and stale empty alerts still require fixes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request prevents duplicate alert writes during refresh/reboot, restores alerts after reconnects, and handles legacy empty alerts.

Changes:

• Reports alert restoration from refresh and reboot.
• Centralizes refresh and conditional reassertion.
• Adds regression coverage and updates documentation.

File summaries

File	Summary
tests/test_registry.py	Updates reboot expectations.
tests/test_pictures.py	Adds alert-write regression coverage.
readerboard/services/registry.py	Reports alert restoration status.
readerboard/services/alerts.py	Tracks put-backs and handles empty alerts.
readerboard/api/routes.py	Avoids duplicate reboot reassertion.
readerboard/api/app.py	Shares refresh/reassert logic with reconnects.
CHANGELOG.md	Documents the behavior change.
AGENTS.md	Records refresh/reassert behavior.

Review details

Suppressed comments (1)

readerboard/api/app.py:118

• _refresh_and_reassert is invoked by both the periodic loop and the reconnect hook, but only registry.refresh() is serialized. If one invocation is waiting for the registry lock while the other finishes its refresh, both can return False for a no-picture configuration and then call alerts.reassert(); that method force-writes the priority file, so the alert can still be restarted twice. Serialize the whole refresh-and-reassert operation (or otherwise make the conditional reassert atomic with the refresh) so concurrent callers share one repair.

    if not await registry.refresh():
        await alerts.reassert()

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Reconnects can reassert already-expired TTL alerts before the sweep handles them.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

readerboard/api/app.py:118

• This new reconnect path can run after an alert's expires_at but before the next _sweep_loop pass. AlertService.reassert() does not check _has_expired(), so a reconnect in that window force-writes an already-expired TTL alert back to the sign; have the reassert path release or skip expired alerts before writing them.

        await alerts.reassert()

readerboard/api/app.py:265

• The new reconnect registration is not exercised by the added tests: they call _refresh_and_reassert directly, but no test triggers SignController._connect after registration. A regression in this partial or in the hook wiring would therefore still pass while the documented reconnect repair is absent; add an app/controller integration test that reconnects with an active alert and verifies the alert is restored.

        controller.on_reconnect(
            functools.partial(_refresh_and_reassert, registry, alerts)
        )

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite