Skip to content

feat(server): warn in the UI when the device is under-voltage - #3285

Merged
vpetersson merged 7 commits into
Screenly:masterfrom
vpetersson-bot:feat/undervoltage-warning
Aug 15, 2026
Merged

feat(server): warn in the UI when the device is under-voltage#3285
vpetersson merged 7 commits into
Screenly:masterfrom
vpetersson-bot:feat/undervoltage-warning

Conversation

@vpetersson-bot

@vpetersson-bot vpetersson-bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Surfaces under-voltage in the web UI instead of leaving it to show up as
unexplained display glitches.

Prompted by this forum thread,
where stray characters along the edge of the screen turned out to be
kernel console messages triggered by a marginal power supply. The
accepted answer there suggests redirecting stderr to /dev/null, which
cannot work: those messages are kernel printk output, not something a
userspace process emits. Nothing here suppresses anything.

Why not vcgencmd get_throttled

It is no longer a reliable source, and not only because it is missing
from the server image.

The raspberrypi-hwmon driver polls the same firmware property every
2 seconds and clears the sticky bits as it goes (it sends
value = 0xffff). On every current Raspberry Pi OS and balenaOS Pi
kernel, the "has occurred since boot" bits (16-19) are wiped moments
after they are set, so anything reading them sees a field the kernel
just zeroed. The kernel owns this state now.

So detection reads the kernel's own view instead:
/sys/class/hwmon/hwmonN/in0_lcrit_alarm on the hwmon device named
rpi_volt.

Balena

The sysfs read is what makes one code path work everywhere. It needs no
host agent, no /dev/vcio mapping, no libraspberrypi-bin and no
compose changes, so Balena fleets (which ship no anthias_host_agent)
are covered by exactly the same code as docker-compose installs.

Since-boot history

Because the driver keeps clearing the firmware bits, "has this happened
since boot" is not available from the hardware at all. Anthias latches
it in Redis, keyed on /proc/sys/kernel/random/boot_id so it resets on
reboot rather than persisting through the Redis volume and warning
about a brown-out that was fixed weeks ago.

Detection is event-driven, not sampled. The driver calls
hwmon_notify_event() on every change, raising POLLPRI on the
attribute, so the watcher blocks in poll() and is woken on each
transition. That matters because the attribute only reflects a
2-second window: a marginal supply produces bursts of short dips, and
any sanely-spaced sampler would walk straight past them. The poll also
carries an hourly timeout, but that is only a backstop to re-sync if an
event were ever missed. It costs no CPU while idle and runs in the
celery worker, a single long-lived process on both compose and Balena.

Nothing user-facing depends on that cadence either: get_state() reads
the live attribute on every page render and writes back on
disagreement, so what an operator sees is current to the request.

What an operator sees

A persistent banner on every page (not just System Info, because
someone whose screen is glitching opens the Schedule page to look at
their content), plus a Power supply card on System Info. Red while it
is happening, amber once power recovers but the supply has already
proven it cannot hold up.

Copy is plain language and leads on using the official Raspberry Pi
power supply, stated as the fix rather than one option among four. Safe
to name outright, since the banner only ever renders when the
rpi_volt sensor exists. Sensor and kernel detail stay on System Info
behind a disclosure.

Boards with no sensor report "Not monitored" rather than implying a
healthy supply, and the API exposes supported for the same reason: it
is the only field separating "we checked and it is fine" from "we
cannot check".

Writes

A healthy device never writes to Redis at all. Observations that change
nothing are skipped, which matters because Redis runs --appendonly yes and every SET is fsynced to the SD card. Writing constantly to
the card would be poor manners in a feature whose entire purpose is
avoiding card corruption.

An unreadable latch is treated as unknown, not empty, so a transient
Redis error cannot overwrite a real brown-out record and tell the
operator the supply is fine. Device-level faults (an unreadable boot
id, a Redis that will not answer) are logged once per boot rather than
on every wake-up and page render.

Also in here

  • under_voltage on /api/v2/info, reading the same state the UI
    renders from so the two surfaces cannot drift.
  • collect_debug.sh now captures /api/v2/info into api-info.json
    rather than re-deriving device state in shell, so the bundle tracks
    the API as more diagnostics move into the UI. Raw sysfs and the
    kernel ring buffer stay as fallbacks for the case that prompts most
    debug bundles: a stack too broken to answer HTTP. vcgencmd get_throttled is kept but relabelled, since bits 0-3 and the thermal
    readings are still valid. Its timeout is 90s, above the ~80s
    get_node_ip() can block waiting on the host agent, because a
    missing host agent is itself a fault the bundle exists to diagnose.

Testing

1819 unit tests, ruff check + ruff format --check, and mypy
across 181 files all clean.

Validated on all seven testbeds

Deployed to every board in the fleet, with the in-container blob md5s
asserted equal to the commit's blobs on each one. Every board runs
image bada3fb-<board>, an ancestor of this branch's base with zero
intervening changes to any overlaid file, so nothing unrelated rides
along.

Board Arch hwmon supported Card Watcher
Pi 2 B armhf hwmon1 true OK started
Pi 3 A+ (512 MB) armhf hwmon1 true OK started
Pi 3 B+ arm64 hwmon1 true OK started
Pi 4 B arm64 hwmon1 true OK started
Pi 5 B arm64 hwmon2 true OK started
Rock Pi 4B arm64 none false Not monitored start() -> False
x86_64 x86 none false Not monitored not started

Kernel 6.18.34 on every Pi. On all seven the banner was correctly
absent while healthy, and no board logged a spurious
Under-voltage alarm cleared. after boot.

  • The sensor exists on every Pi, including the Pi 5. That was an
    open question when this was written.
  • Pi 5 puts rpi_volt on hwmon2, not hwmon1 (rp1_adc takes
    hwmon1). Matching on the device name rather than a fixed index
    is what makes Pi 5 work; a hardcoded hwmon1 would have worked on
    six boards out of seven and silently read an ADC on the newest one.
  • The two non-Pi boards degrade correctly. Rock Pi 4 and x86 both
    report supported: false, render "Not monitored" rather than
    implying a healthy supply, and start no watcher thread at all.
  • A real kernel notification is processed correctly. Verified on a
    Pi 5 by driving the actual _watch_loop against a cgroup.events
    attribute, which goes through the same kernfs_generic_poll() path
    as the hwmon alarm but can be triggered on demand. With the poll
    timeout set to an hour so only a genuine event could wake it, the
    loop observed the event and logged no failure. A negative control
    with the previous code dropped the event and logged
    Under-voltage watcher failed.
  • poll() genuinely blocks. On Pi 4 and Pi 5 it sat the full
    3000 ms and returned no events after the initial read, confirming
    the "first read clears the latched POLLPRI" assumption. Had that
    been wrong the watcher would have spun a core at 100% on every Pi.
  • Watcher liveness and cost (Pi 4): poisoning the Redis latch to
    active: true saw the watcher correct it on its next wake-up while
    preserving count and history. Celery's whole MainProcess burned
    0.120 s over 40 s (0.3% of one core) including beat, so the watcher
    itself is below that.
  • The event-count semantics verified on-device, driving the real
    _watch_loop against a controllable file: a brown-out held across
    ~6 wake-ups counted 1, recovery kept seen_since_boot, and a
    second distinct dip counted 2.
  • The unknown-boot-id handling verified on-device. With
    get_boot_id() forced to None, the live reading stayed accurate,
    nothing was written to Redis, and the explanatory warning logged. A
    planted boot_id: null latch carrying seen_since_boot: true, count: 7 was correctly discarded to false / 0 rather than trusted.
  • Both banner states rendered on-device, red for a live alarm and
    amber for a recovered one, on the Schedule page rather than only on
    System Info.

The 512 MB Pi 3 A+ and the 1 GB Rock Pi 4 are both known-fragile under
memory pressure. The Rock Pi 4 sat at 0 MB swap free throughout, so it
got a server-only restart to avoid adding pressure and its watcher path
was exercised with a direct start() probe instead. Neither board was
destabilised (Rock Pi 4 ended at 109 MB available, unchanged).

All seven boards were restored to their pinned images, the overlay
confirmed gone from both containers, the test latch deleted, and the
locks released.

The seven-board sweep ran at ba5b9621. Everything after it is the
review round below, which changed the poll-mask handling (separately
verified on hardware, above), the Redis write discipline, log
throttling, and the re-sample constant. The per-board sensor and
rendering results are unaffected by those.

What review and hardware testing changed

Four defects that unit tests alone did not surface:

  1. A healthy Pi logged Under-voltage alarm cleared. after every
    boot
    , because the first reading of a pass differs from the initial
    None. It reads as though an alarm had occurred, in exactly the log
    a support engineer greps. Found on the Pi 4.
  2. A stored boot_id: null compared equal to a current unknown
    None
    , so a device that could not read its boot id would treat an
    arbitrarily old latch as current and never reset the warning again.
  3. POLLERR was treated as fatal. kernfs_generic_poll() returns
    DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI on every genuine change
    notification, so a real brown-out arrives as POLLPRI|POLLERR and
    was raising, backing off 60s and dropping the transition. Measured
    as mask 0xa on a Pi 5. The tests missed it because the fake poller
    scripted a bare POLLPRI, a mask the kernel never returns.
  4. A transient Redis GET failure erased history, by falling back
    to an empty state that was then persisted.

Not covered

A genuine brown-out was not induced: that needs a deliberately weak
supply on a board someone can physically reach, and forcing one on a
shared testbed risks the SD-card corruption this feature warns about.
The notification mechanism is verified end to end on hardware (see
the cgroup.events test above, same kernel path); what is untested is
the specific firmware condition that triggers it, which rests on the
raspberrypi-hwmon driver calling hwmon_notify_event(). The hourly
re-sample, plus the live read on every page render, both cover the case
where an event is missed.

🤖 Generated with Claude Code

Surfaces power-supply problems that previously only showed up as
unexplained display glitches and stray text on screen.

- Read the kernel's rpi_volt hwmon sensor instead of vcgencmd: the
  raspberrypi-hwmon driver clears the firmware's sticky bits every
  2s, so get_throttled's since-boot flags are not a durable record
- Works unchanged on Balena, which ships no host agent: sysfs reads
  the same from inside an unprivileged container
- Latch the since-boot history in Redis, keyed on boot_id so it
  resets on reboot rather than persisting via the Redis volume
- Watch the sensor with poll()/POLLPRI in the celery worker, since
  a sampler would miss the short dips a marginal supply produces
- Show a persistent banner on every page plus a System Info card,
  in plain language, leading on the official Raspberry Pi supply
- Expose under_voltage on /api/v2/info
- collect_debug.sh now captures /api/v2/info rather than deriving
  device state in shell, keeping raw sysfs and the kernel ring
  buffer as fallbacks for when the stack cannot answer

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 15, 2026 06:15
@vpetersson-bot
vpetersson-bot requested a review from a team as a code owner August 15, 2026 06:15
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.66667% with 37 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@1f0e704). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/anthias_common/undervoltage.py 87.91% 14 Missing and 4 partials ⚠️
src/anthias_server/app/page_context.py 74.28% 9 Missing ⚠️
src/anthias_server/lib/undervoltage_watcher.py 91.54% 3 Missing and 3 partials ⚠️
src/anthias_server/celery_tasks.py 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3285   +/-   ##
=========================================
  Coverage          ?   90.39%           
=========================================
  Files             ?       81           
  Lines             ?     9228           
  Branches          ?      989           
=========================================
  Hits              ?     8342           
  Misses            ?      656           
  Partials          ?      230           

☔ 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds kernel-hwmon based under-voltage detection (via rpi_volt sysfs), latches “seen since boot” in Redis, and surfaces the state consistently across the web UI, /api/v2/info, and debug bundles—so operators get an actionable warning instead of unexplained display glitches.

Changes:

  • Introduces anthias_common.undervoltage (sysfs detection + Redis latch) and a Celery-side background watcher to keep the latch current.
  • Adds an always-on-page under-voltage banner plus a System Info “Power supply” card with plain-language guidance and technical detail behind a disclosure.
  • Exposes under_voltage on /api/v2/info and updates collect_debug.sh to snapshot the info endpoint (with sysfs/dmesg fallbacks).

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_undervoltage.py Unit tests for hwmon discovery, alarm reads, and Redis latch behavior
tests/test_template_views.py Template-level tests for the global banner and System Info power card rendering
src/anthias_server/lib/undervoltage_watcher.py New poll()-based watcher thread for under-voltage transitions and latch refresh
src/anthias_server/celery_tasks.py Starts the watcher on Celery worker_ready
src/anthias_server/app/templates/system_info.html Adds “Power supply” card + technical details disclosure
src/anthias_server/app/templates/_power_warning.html New global under-voltage banner partial
src/anthias_server/app/templates/_layout.html Includes the power warning partial above <main>
src/anthias_server/app/static/sass/_styles.scss New design tokens + styles for the banner and power card
src/anthias_server/app/page_context.py Adds navbar power_warning context and System Info power context
src/anthias_server/api/views/v2.py Adds under_voltage to /api/v2/info (+ schema)
src/anthias_server/api/tests/test_info_endpoints.py Tests for the new under_voltage info field
src/anthias_common/undervoltage.py New sysfs-based under-voltage reader + Redis latch implementation
bin/collect_debug.sh Captures /api/v2/info snapshot and adds under-voltage sysfs/dmesg fallbacks
Suppressed comments (1)

src/anthias_common/undervoltage.py:225

  • record_observation() increments count on every call where active=True, even if the latch was already active. Because the watcher also resamples on a timeout, a sustained under-voltage condition will inflate count over time (e.g., +1 every RESAMPLE_INTERVAL_S) even though no new “dip” occurred. If count is meant to represent the number of under-voltage events/dips (as the UI copy implies), it should only increment on a rising edge (previously inactive → active), while last_seen can still be refreshed whenever active is true.
        if not state['seen_since_boot']:
            state['seen_since_boot'] = True
            state['first_seen'] = now
        state['last_seen'] = now
        state['count'] = state['count'] + 1

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

Comment thread src/anthias_server/lib/undervoltage_watcher.py Outdated
Comment thread src/anthias_common/undervoltage.py
Follow-up from local review of the under-voltage warning.

- count was incremented on every truthy reading, so the watcher's
  30s resample rendered "power dropped too low 21 times" for one
  brown-out that never went away; it is now edge-triggered off the
  latch, which also stops a worker restart recounting
- treat POLLERR/POLLHUP and an empty sysfs read as errors: a removed
  hwmon device made poll() return instantly forever, and an empty
  read was mapped to "healthy", clearing a live warning
- persist a live alarm seen only by a page render, so the banner no
  longer disappears on recovery when the watcher is down
- guard the stored count coercion; a non-numeric value raised before
  the write that would overwrite it, killing the feature for good
- cache the hwmon path scan, which ran on every page render
- add tests for the watcher, which had none

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 06:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/anthias_server/lib/undervoltage_watcher.py:121

  • The watcher logs "Under-voltage alarm cleared" on the first read (and after any exception) because previous starts as None and active != previous is true even when the device is simply healthy. This creates misleading/noisy logs; only log transitions after an initial baseline has been established.
                    if active != previous:
                        _log_transition(active, state)
                        previous = active

src/anthias_common/undervoltage.py:276

  • If the kernel boot id can't be read (get_boot_id() returns None), the latch is persisted with boot_id=None and will then survive reboots (because stored.get('boot_id') == boot_id stays true). That contradicts the intended "reset on reboot" behavior; consider degrading to a non-persisted, live-only state when boot id is unavailable.
    if boot_id is None:
        boot_id = get_boot_id()

    state = _load_latch(redis_client, boot_id)

Found on a real Pi 4: the first reading of each watch pass differs
from the initial None, so a device that never browned out logged
"Under-voltage alarm cleared." about 30s after every boot. It reads
as though an alarm had happened, in the one log a support engineer
greps. A device that starts up already in alarm is still logged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 06:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/anthias_common/undervoltage.py:276

  • record_observation() currently persists a latch even when boot_id is None (by writing 'boot_id': None). With the _load_latch() fix above this stored value becomes dead data; it’s also safer to avoid writing a latch at all when the boot id is unknown, since we can’t guarantee it will reset on reboot. Return a best-effort in-memory state instead.
    if boot_id is None:
        boot_id = get_boot_id()

    state = _load_latch(redis_client, boot_id)

src/anthias_common/undervoltage.py:218

  • If boot_id cannot be read (e.g. /proc unavailable), _load_latch() will accept any previously-stored payload whose boot_id is also None, which makes the latch persist across reboots (contrary to the “reset on reboot” behavior described in the module docstring). Treat an unknown boot id as “cannot trust persisted latch” and always return an empty state.

This issue also appears on line 273 of the same file.

def _load_latch(redis_client: Any, boot_id: str | None) -> dict[str, Any]:
    """Read the stored latch, discarding it if it predates this boot.

    Redis persists to a volume, so without the boot-id check a
    device that browned out last week would still be showing the

From Copilot review. A stored `boot_id: null` compares equal to a
current unknown `None`, so a device that could not read its boot id
would treat an old latch as current and the warning would never
reset again.

- discard a stored latch outright when the boot id is unknown
- skip persisting in that case, and log why
- correct the watcher comment: the fd is held for the life of the
  inner loop, and only reopened on the recovery path

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 06:56
@vpetersson-bot

Copy link
Copy Markdown
Contributor Author

Third point from the review (the suppressed one on record_observation and boot_id) was real and is fixed in ba5b962.

The trap is sharper than just dead data: a stored boot_id: null compares equal to a current unknown None, so _load_latch would treat an arbitrarily old latch as belonging to the current boot. On a device that could not read /proc/sys/kernel/random/boot_id, the warning would latch on and never reset again, across any number of reboots and power-supply swaps.

Fixed on both sides so they cannot disagree:

  • _load_latch discards a stored latch outright when the boot id is unknown
  • record_observation skips the write in that case and logs why, returning best-effort live state

Degrading to "live readings only" is the safe direction here: it can under-report history, never invent it. Two regression tests cover the discard and the skipped write.

All three points now addressed. Full gate green: 1812 tests, ruff check + ruff format --check, mypy across 181 files.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_undervoltage_watcher.py:176

  • Avoid using # type: ignore[method-assign] here by using a small FakePoll subclass that overrides poll() for this test. This keeps mypy clean without suppressions and avoids runtime monkeypatching of a method.
        poller = FakePoll([])
        poller.poll = flip  # type: ignore[method-assign]

@vpetersson-bot

Copy link
Copy Markdown
Contributor Author

Re-validated on the Pi 4 testbed against the current head ba5b9621, so the device-testing claim now covers the boot_id fix rather than the commit before it. In-container blob md5s asserted equal to the commit's blobs.

  • unknown boot id: live reading stayed accurate (active: true), nothing persisted, explanatory warning logged
  • planted boot_id: null latch carrying seen_since_boot: true, count: 7 was discarded to false / 0 rather than trusted
  • count semantics unchanged: brown-out held across ~6 resamples counted 1, a second distinct dip counted 2
  • healthy boot logs only the Watching ... line, no spurious alarm cleared
  • /api/v2/info, the System Info card and the amber banner all rendered correctly end to end

Testbed restored to bada3fb-pi4-64, test latch deleted, lock released.

@vpetersson-bot

Copy link
Copy Markdown
Contributor Author

Now validated on all seven testbeds at ba5b9621, with in-container blob md5s asserted equal to that commit's blobs on every board.

Board Arch hwmon supported Card Watcher
Pi 2 B armhf hwmon1 true OK started
Pi 3 A+ (512 MB) armhf hwmon1 true OK started
Pi 3 B+ arm64 hwmon1 true OK started
Pi 4 B arm64 hwmon1 true OK started
Pi 5 B arm64 hwmon2 true OK started
Rock Pi 4B arm64 none false Not monitored start() -> False
x86_64 x86 none false Not monitored not started

Banner correctly absent while healthy on all seven, and no board logged a spurious Under-voltage alarm cleared. past the 30s resample.

Two things this fleet run adds over the single-board run:

  • Pi 5 is the only board where rpi_volt is not hwmon1 (rp1_adc takes that slot). Every other Pi, armhf and arm64, is on hwmon1. A hardcoded index would have worked on six boards out of seven and silently read the ADC on the newest one.
  • Both non-Pi boards start no watcher thread at all, so the feature costs nothing on hardware that cannot report.

The 512 MB Pi 3 A+ and the 1 GB Rock Pi 4 are known-fragile. The Rock Pi 4 sat at 0 MB swap free throughout, so it got a server-only restart to avoid adding pressure and its watcher path was exercised with a direct start() probe; it ended at 109 MB available, unchanged. Neither board was destabilised.

All seven restored to their pinned bada3fb-<board> images, overlay confirmed gone from both containers, test latch deleted, locks released.

Second review pass. The POLLERR guard added last round broke the
event-driven path outright: kernfs_generic_poll() returns
DEFAULT_POLLMASK|EPOLLERR|EPOLLPRI on every genuine change
notification, so a real brown-out arrives as POLLPRI|POLLERR and was
raising, logging a stack trace, sleeping 60s and dropping the
transition. Measured as mask 0xa on a Pi 5 (kernel 6.18) and
confirmed with a negative control: the old guard drops a real kernel
event, the new one processes it.

- believe POLLERR only when POLLPRI is absent, the standard sysfs idiom
- tests now script the mask the kernel really returns; the previous
  bare-POLLPRI mask is what let this through
- skip the Redis write when nothing changed; the 30s re-sample was
  ~2,880 appendonly-fsynced writes a day on a healthy device
- treat an unreadable latch as unknown, not empty, so a transient
  Redis error can no longer erase a real brown-out record
- log the missing-boot-id warning once per process, not every 30s
- raise collect_debug's API timeout above get_node_ip()'s ~80s wait,
  so the snapshot is captured on hosts with no running host agent

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 11:16
@vpetersson-bot

Copy link
Copy Markdown
Contributor Author

Second review pass found a serious one that the hardware run had missed, because that run never induced a real sensor event.

The POLLERR guard broke the event-driven path outright. kernfs_generic_poll() returns DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI whenever the attribute's event counter advances, so POLLERR rides along with every genuine sysfs notification. The guard I added last round therefore raised on every real brown-out: stack trace logged, 60s backoff, transition dropped. The watcher would have degraded to a blind 60-second sampler, strictly worse than not using poll() at all.

Measured on a Pi 5 (kernel 6.18) with a cgroup.events notification, which goes through the same kernfs path: mask 0xa = POLLPRI|POLLERR. Then drove the real _watch_loop against that attribute with a 3600s resample so only a genuine kernel event could wake it, with a negative control:

guard observations after a real kernel event watcher failed lines
old (POLLERR fatal) +0 (event dropped) 1
new (POLLERR only without POLLPRI) +1 (processed) 0

The tests missed it because FakePoll scripted a bare POLLPRI, a mask the kernel never actually returns. They now use the real one, plus a regression test for this exact case and its converse (bare POLLERR must still reach the backoff path).

Four smaller fixes in the same commit:

  • Unconditional Redis SET on every observation. With the 30s re-sample that is ~2,880 appendonly-fsynced writes a day onto the SD card of a healthy device, which is poor manners in a feature about preventing card corruption. Now skipped when nothing changed, so a healthy device never even creates the key.
  • A transient Redis GET failure erased history. _load_latch fell back to an empty state which was then persisted, wiping seen_since_boot/count and telling the operator the supply is fine. Load failure is now distinguished from "no latch" and never written back.
  • Missing-boot-id warning logged every 30s; now once per process.
  • collect_debug.sh used --max-time 10 against /api/v2/info, which can block ~80s inside get_node_ip() waiting on the host agent. A missing host agent is itself a common fault the bundle diagnoses, so the snapshot was dropped on exactly the broken devices that need it. Raised to 90s.

1818 tests, ruff, mypy all clean.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/anthias_server/app/page_context.py:50

  • _parse_iso() claims to return an aware datetime, but datetime.fromisoformat() will successfully parse a naive ISO string (no offset) and return a naive datetime. Passing a naive datetime into Django’s humanize/naturaltime (with USE_TZ enabled) can raise offset-naive/offset-aware comparison errors during template rendering. Consider rejecting naive timestamps (or making them explicitly UTC) so template rendering can’t break on a malformed/stale latch value.
    if not isinstance(value, str):
        return None
    try:
        return datetime.fromisoformat(value)
    except ValueError:
        return None

src/anthias_server/app/page_context.py:72

  • _power_warning() fully suppresses exceptions from undervoltage.get_state() with no logging. If this path starts failing in production, the banner will silently disappear (potentially hiding real under-voltage) and operators/support will have no signal that the diagnostic itself is broken. Logging once per process keeps the “never break rendering” property without losing debuggability.
    try:
        state = undervoltage.get_state(_redis)
    except Exception:
        # Never let a diagnostic break page rendering.
        return None

Every state change arrives as a kernel POLLPRI notification on both
edges, so the poll timeout is only a backstop for a missed event. A
30s timeout woke the worker and re-read sysfs to learn nothing ~2,880
times a day on a device that was behaving.

Nothing user-facing depends on the cadence: get_state() reads the
live attribute on every page render and writes back on disagreement,
so the UI is current to the request, not to the last poll.

- RESAMPLE_INTERVAL_S 30 -> 3600
- route the unreadable-latch warning through the same once-per-boot
  throttle as the missing-boot-id one, since page renders hit it too

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 11:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/anthias_common/undervoltage.py:339

  • record_observation() updates the Redis latch via a non-atomic read/modify/write (GET in _load_latch → local mutation → SET). Because both the watcher thread and page renders can call this concurrently (and the server may run multiple workers), two callers can observe the same inactive→active edge and each increment count, inflating the occurrence counter and potentially clobbering first_seen/last_seen.

To keep count reliably edge-triggered under concurrency, consider making the latch update atomic (e.g., WATCH/MULTI with retry, or a Lua script that reads the JSON, applies the transition rule, and writes back in one round trip; alternatively store active/count in separate Redis keys and use INCR on a successful inactive→active transition).

    state, readable = _load_latch(redis_client, boot_id)
    prior = dict(state)
    was_active = state['active']
    state['active'] = active

    if active:
        now = _now_iso()
        if not state['seen_since_boot']:
            state['seen_since_boot'] = True
            state['first_seen'] = now
        state['last_seen'] = now
        if not was_active:
            state['count'] = state['count'] + 1

tests/test_undervoltage_watcher.py:188

  • This test assigns to poller.poll and suppresses the type error with # type: ignore[method-assign]. The repo guidelines discourage type: ignore when a small refactor can avoid it; here the override can be expressed via a tiny FakePoll subclass, keeping mypy clean without suppressions.
        poller = FakePoll([])
        poller.poll = flip  # type: ignore[method-assign]
        with (

From the low-confidence review comments.

- _parse_iso() stamped naive values as UTC. We only ever write aware
  strings, but fromisoformat accepts a naive one and naturaltime then
  compares it to a naive LOCAL now: a dip one minute ago rendered as
  "3 hours from now" on a US-Eastern player. Not the TypeError the
  review predicted, Django's humanize handles naive values, just a
  nonsense relative time in the banner.
- _power_warning() swallowed every exception silently, so a broken
  probe was indistinguishable from a healthy supply and would have
  hidden a real under-voltage with no signal. Logged once per process.
- replace a `type: ignore[method-assign]` in the tests with a FakePoll
  subclass, per the repo's no-suppressions rule.
- tests pinning the concurrency behaviour: two callers observing the
  same edge converge on one increment rather than stacking, and
  seen_since_boot is never downgraded by a stale writer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 15, 2026 11:38
@vpetersson-bot

Copy link
Copy Markdown
Contributor Author

Went through the four suppressed (low-confidence) review comments. Three were real and are fixed in 812a7e1; one I believe is incorrect, with a test now pinning the behaviour.

Fixed — naive latch timestamp (page_context.py:50). Real, but not for the stated reason. The prediction was a TypeError from naturaltime taking down template rendering. Django's humanize actually handles naive values by comparing against a naive now(), so there is no crash: I removed the fix and the test still passed, which is what sent me looking. The real effect is a wrong relative time. Measured with TZ=America/New_York, a dip one minute ago:

aware value -> a minute ago
naive value -> 3 hours from now

So a stale or hand-edited latch would put "most recently 3 hours from now" in the banner on any non-UTC device. Naive values are now stamped UTC (which is what the writer emits), explicit offsets are preserved, and there's a test that fails without the fix.

Fixed — silent exception swallow (page_context.py:72). Agreed, and the failure direction is the bad one: returning None renders no banner, so a broken probe looks exactly like a healthy supply and would hide a real under-voltage with no signal anywhere. Now logged once per process, keeping the never-break-rendering property.

Fixed — type: ignore[method-assign] in the tests. Straight violation of the repo's no-suppressions rule, and mine. Replaced with a small FlippingPoll(FakePoll) subclass.

Not fixed — non-atomic latch update (undervoltage.py:339). The race is real, but the stated consequence is not: it cannot inflate count. Both callers compute count + 1 from the same value they each read, so two callers racing on one edge both write n+1, not n+2. get_state's write-back also re-reads inside record_observation rather than reusing its earlier load, which narrows the window further. seen_since_boot only ever moves up within a boot, so a stale writer cannot clear a warning either. Two tests now pin both properties. A Lua script or WATCH/MULTI would add real complexity to defend a counter that is already convergent, so I would rather not take it on without a demonstrated failure.

1823 tests, ruff, mypy all clean.

@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

@vpetersson
vpetersson merged commit 0bf0790 into Screenly:master Aug 15, 2026
11 checks passed
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.

3 participants