Skip to content

feat(server): warn in the UI when storage is failing - #3286

Merged
vpetersson merged 16 commits into
Screenly:masterfrom
vpetersson-bot:feat/sd-card-health
Aug 16, 2026
Merged

feat(server): warn in the UI when storage is failing#3286
vpetersson merged 16 commits into
Screenly:masterfrom
vpetersson-bot:feat/sd-card-health

Conversation

@vpetersson-bot

Copy link
Copy Markdown
Contributor

Issues Fixed

No tracking issue. Follow-up to the under-voltage warning (PR 3285), which covered the first of the two failures a Raspberry Pi actually dies of. This covers the second. Both previously showed up only as unexplained behaviour: the screen keeps playing while every change the operator makes silently fails to stick.

Description

SD cards have no health register, so unlike under-voltage there is no single sensor to read. The verdict is assembled from four container-readable sources, so Balena fleets are covered without a host agent:

  • ext4 superblock error counters. Durable: they survive reboots and are cleared only by fsck.
  • A write check — write, fsync, POSIX_FADV_DONTNEED, read back, compare. Needed because the counters go quiet once a filesystem stops: nothing is going wrong any more because nothing is being written.
  • eMMC life_time / pre_eol_info where the board has them.
  • Card identity from the MMC/SD registers.

The device is resolved from /proc/self/mountinfo rather than assumed, so the bind mount, Balena's volume and an x86 SSD all work with no per-platform case.

SMART extends this to x86/arm64, where a SATA/NVMe device reports wear nowhere in sysfs. Reading it needs an ioctl, so anthias-viewer (the one container privileged everywhere, including both Balena templates) samples it and publishes a TTL'd Redis fact the server reads — the same division of labour as HDMI-CEC. It folds into the existing wear_pct/pre_eol fields, so the UI and API need no separate SSD branch.

Six banner states, because the fixes differ: a full card, bad blocks, wear and a dying card need different advice, and the noun follows the hardware so an x86 player isn't told to replace a memory card. The under-voltage banner is generalised into a shared .device-alert block so the two stack cleanly, power first, without repeating each other.

Also: storage on /api/v2/info, a System Info card, and a storage.txt section in the debug bundle.

Validated on hardware, which changed the code five times

Driving this against a real ext4 broken with the kernel's trigger_fs_error, and against the full testbed fleet, found bugs no unit test would have:

  • A stopped filesystem is spelled emergency_ro, not ro, on kernels 6.18 and 7.0. Writes failed with EROFS while both the ro option and statvfs's ST_RDONLY stayed clear. read_only was returning False on a filesystem refusing every write.
  • privileged: true on a container does not mean the process is privileged. start_viewer.sh drops to the unprivileged viewer user, so smartctl returned "Permission denied" and every board would have reported its drive unsupported. Fixed with one NOPASSWD sudoers rule scoped to that binary.
  • Bad blocks are not wear. The x86 testbed's SSD has zero wear, a PASSED self-assessment, and 8 reallocated/pending sectors. The wear copy would have claimed it had used "most of" its rated writes.
  • 0 is falsy in a Django template, so a genuine 0% wear reading rendered as "unknown".
  • SD and eMMC manufacturer IDs are different namespaces. Vendor tables now come from mmc-utils, which keeps two tables for exactly that reason.

Regression sweep across all seven boards (x86, pi5, pi4, pi3-64, pi3a-32, pi2, rock4): every board reports ok, no false positives, write check 1.2-16.6 ms, nothing left behind, smartctl correctly absent on the SBCs. The Rock Pi 4 exercised the eMMC wear path.

The docstring's claim that Raspberry Pi OS mounts root errors=remount-ro was also wrong and is corrected: every Pi reports Errors behavior: Continue, so a dying Pi card limps rather than stopping. The Rock Pi 4 is the opposite. Both endings are real, which is why both signals are read.

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

End-to-end on x86: viewer collects SMART, publishes to Redis, server folds it in, /api/v2/info returns the real drive's figures and the page renders the right banner. Pi coverage is the fleet sweep above plus the emergency_ro test on a loopback filesystem, never a testbed rootfs.

Ops finding: the x86 testbed's SSD has 4 reallocated + 4 pending sectors at 2226 power-on hours. testbed-qa already documents EIO wedging sshd on that board; this is the same drive. Worth replacing before it takes the board out mid-burn-in.

🤖 Generated with Claude Code

vpetersson-bot and others added 9 commits August 15, 2026 11:53
The second of the two failures a Raspberry Pi actually dies of, after
under-voltage. Both previously showed up only as unexplained
behaviour: a screen that keeps playing while every change the operator
makes silently fails to stick.

- Assemble a verdict from ext4's superblock error counters, a
  periodic write-and-read-back check, and the eMMC wear registers;
  SD cards have no health register, so no single sensor exists
- Resolve the device from /proc/self/mountinfo rather than assuming
  mmcblk0p2, so bind mounts, Balena volumes and x86 SSDs all work
- Read only container-visible sysfs, so Balena fleets are covered
  without a host agent; dmesg stays in collect_debug.sh
- Detect the read-only remount specifically: error counters go quiet
  once ext4 stops writing, which is exactly the failure that matters
- Sample counters every 60s and pay for a write check every 15min,
  on a daemon thread so a slow fsync can't tie up a worker slot
- Six banner states with distinct copy: a full card, a worn eMMC and
  a dying SD card need different fixes, and the noun follows the
  hardware so x86 players aren't told to replace a memory card
- Generalize the under-voltage banner into a shared .device-alert so
  the two stack cleanly, power first, without repeating each other
- Expose storage on /api/v2/info and add a System Info card
- collect_debug.sh captures ext4 counters, MMC registers and the
  kernel's I/O errors as fallbacks for an unanswerable stack

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by driving the detector against a real loopback ext4 broken with
/sys/fs/ext4/<dev>/trigger_fs_error, rather than the synthetic sysfs
trees the unit tests use.

- newer kernels stop the filesystem with an internal emergency flag
  instead of SB_RDONLY, so every write returned EROFS while the mount
  options read `rw,relatime` and statvfs's ST_RDONLY stayed clear;
  matching only 'ro' missed the exact failure this exists to catch
- an empty sysfs attribute now reads back as None, so an ext4 that has
  never errored reports last_error_function as null rather than ""

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the storage warning to the boards that don't boot from a card.
sysfs gives a SATA/NVMe device no wear signal at all, so the figure
eMMC keeps in a register lives behind a SMART ioctl instead.

- collect in anthias-viewer, the one container privileged everywhere
  including both Balena templates, and publish a TTL'd Redis fact the
  server reads during a page render; same division of labour as CEC
- fold into the existing wear_pct/pre_eol fields rather than adding a
  parallel path, so an SSD and a compute module's eMMC render through
  one code path and the UI and API need no third branch
- trust NVMe percentage_used outright and mark ATA wear inexact: the
  normalized-counts-down convention is near-universal but not a spec
- carry the verdict on the well-defined signals instead — the overall
  self-assessment, NVMe's spare threshold, reallocated/pending sectors
- ignore a fact naming a different disk; a box with a boot SSD and a
  data drive would otherwise report one drive's wear against the other
- smartmontools only on x86/arm64/pi5, never on the SD-card boards
  where it is image weight that can never return an answer
- collect_debug.sh grows a host-side raw SMART section

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unknown

Both found on the x86 testbed's real drive: zero wear, a PASSED overall
self-assessment, and 4 reallocated + 4 pending sectors.

- that drive would have been described as having used "most of" the
  writes it was built for, sending the operator to look at write
  volume while the drive fails to read blocks it already wrote; bad
  blocks now get their own state, icon and copy, and state a count
- `{% if wear_pct %}` treated a genuine 0% as missing, because 0 is
  falsy in a Django template; both templates now test for None

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found on the x86 testbed. `privileged: true` on anthias-viewer does not
mean the sampler runs privileged: bin/start_viewer.sh drops to the
unprivileged `viewer` user, so smartctl returned "open device:
/dev/sda failed: Permission denied" and every board reported its drive
as unsupported.

- two barriers, not one: /dev/sda is brw-rw---- root:disk, and the ATA
  passthrough behind SMART needs CAP_SYS_RAWIO
- one NOPASSWD sudoers rule scoped to the single binary clears both;
  adding viewer to the disk group plus setcap would need
  cap_dac_override and land closer to root
- sudo -n so an image without the rule fails fast instead of hanging a
  sampler thread on a password prompt; direct call kept when already root
- rule ships only on the boards that ship smartmontools

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regression sweep across all six boards (x86, pi5, pi4, pi3-64, pi2,
pi3a-32, rock4). Every board reported `ok` — no false positives — and
the write check passed everywhere (fsync 2.6-16.6 ms).

- eMMC manufacturer IDs are JEDEC-assigned and a DIFFERENT namespace
  from the SD list; sharing one table would have rendered a JEDEC 0x03
  part as "SanDisk". Split, and kept short so unknown IDs stay hex
- the docstring claimed Raspberry Pi OS mounts root errors=remount-ro.
  It does not: every Pi reports "Errors behavior: Continue" and passes
  no errors= option, so a dying Pi card limps rather than stopping,
  and the counters are what rise. The Rock Pi 4 is the opposite. Both
  endings are real, which is why both signals are read

Also confirms on the Pi's 6.18 kernel that a stopped filesystem is
spelled emergency_ro, same as the dev host — so the earlier read-only
fix was load-bearing on the Pi fleet too, not just x86.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nowns

Answers "surely 0x88 is a vendor id" — it is, but nobody publishes what
it maps to.

- replace the hand-written tables with mmc-utils' sd_database and
  mmc_database, transcribed verbatim (including the entries upstream
  itself leaves unnamed). It keeps two tables for the same reason we
  do: 0x03 is SanDisk on SD but Toshiba on eMMC
- that corrects several guesses of mine: 0x27 was Phison, upstream
  says Delkin/Phison; 0x1b was Samsung, upstream says
  Transcend/Samsung; 0x9c was unsourced entirely and is gone
- an unlisted id now yields manufacturer=None instead of the string
  "Unknown (0x88)", which put a placeholder where the UI expects a
  company name. The raw id is exposed as manufacturer_id and shown in
  the technical detail, since that is what a support engineer searches

There is no authoritative public registry: JEDEC assigns eMMC ids and
does not publish them freely, and the SD Association publishes nothing.
mmc-utils and the kernel's CID_MANFID_* defines are the closest thing,
they agree where they overlap, and neither lists 0x88.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Rock Pi 4's CID dump shows life_time 0x01, and DEVICE_LIFE_TIME_EST
0x01 means "0-10% used", not "10% used". We report the band's upper
bound, which is right for the 80% warning threshold but made the UI
claim "about 10% of the rated write life used" on a device that may
have used none of it.

- media now carries wear_is_exact, false for an eMMC band and for an
  ATA vendor attribute, true only for NVMe's defined percentage_used
- both templates say "up to N%" unless the figure is exact

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merged under-voltage PR gained four commits after I branched, two
of which fixed flaws in a latch this module copied the design of. Both
were present here.

- a latch with no boot id was trusted: comparing None to a stored None
  matches, so a device that cannot read its boot id would treat last
  week's latch as current and never reset, warning about a card that
  had already been replaced. Discarded outright now, and not persisted
- the latch was rewritten on every sample. Redis persists to the card,
  so at a 60s cadence that is ~1,400 appendonly-fsynced writes a day
  onto a healthy device — self-defeating in a feature whose whole
  point is avoiding card wear. A write that changes nothing is skipped

The third upstream fix (poll POLLERR handling) does not apply: this
watcher samples rather than polling. The fourth, _parse_iso stamping a
naive timestamp as UTC, is shared code and already covers storage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

This PR adds end-to-end “storage health” diagnostics to Anthias, surfacing impending storage failure (ext4 error counters, write/read-back checks, eMMC wear, and viewer-sampled SMART) in the web UI, /api/v2/info, and the debug bundle, so operators get actionable warnings before changes start silently failing to persist.

Changes:

  • Introduces storage-health detection (+ latch) and SMART collection, with a Celery watcher thread updating Redis state and the viewer publishing SMART facts.
  • Adds UI surfaces: stacked device-alert banners across all pages and a new Storage Health card on System Info.
  • Extends /api/v2/info and the debug bundle to include storage health, backed by substantial new test coverage.

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tools/image_builder/utils.py Installs smartmontools only on boards that can boot from SMART-capable storage.
tests/test_template_views.py Adds banner + System Info rendering tests and stubs storage health for determinism.
tests/test_storage_watcher.py Adds unit tests for the new storage-health watcher thread.
tests/test_storage_health.py Adds extensive tests for storage probing, write-checking, latching, and SMART merge behavior.
tests/test_smart.py Adds tests for SMART parsing, smartctl execution behavior, and Redis publish/read.
src/anthias_viewer/init.py Adds viewer-side SMART sampling loop that publishes TTL’d Redis facts.
src/anthias_server/lib/storage_watcher.py Adds Celery-worker thread to keep storage-health latch current and log transitions.
src/anthias_server/celery_tasks.py Starts the storage watcher on Celery worker_ready.
src/anthias_server/app/templates/system_info.html Adds a new Storage Health stat card and unifies “verdict” styling.
src/anthias_server/app/templates/_storage_warning.html Adds a new storage warning banner rendered on every page.
src/anthias_server/app/templates/_power_warning.html Refactors power banner to use shared “device-alert” styling/container.
src/anthias_server/app/templates/_layout.html Switches to a shared _device_alerts.html include above <main>.
src/anthias_server/app/templates/_device_alerts.html New wrapper template stacking power + storage banners.
src/anthias_server/app/static/sass/_styles.scss Introduces .device-alert component styles and shared .status-verdict styles.
src/anthias_server/app/page_context.py Adds storage warning/card context generation and state shaping for templates.
src/anthias_server/api/views/v2.py Adds storage to /api/v2/info plus OpenAPI schema description.
src/anthias_server/api/tests/test_info_endpoints.py Adds API tests covering the new storage payload and failure modes.
src/anthias_common/storage_health.py New core storage-health module: mount resolution, ext4 counters, write-check, latch, SMART merge.
src/anthias_common/smart.py New SMART module: viewer-side sampling via smartctl + normalization + TTL’d Redis publishing.
docker/Dockerfile.viewer.j2 Adds a sudoers rule enabling the unprivileged viewer to run smartctl as root.
bin/collect_debug.sh Adds storage.txt (ext4 counters, MMC identity/wear, SMART, kernel storage errors) to debug bundles.

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

Comment thread src/anthias_viewer/__init__.py Outdated
Comment thread src/anthias_server/lib/storage_watcher.py
Comment thread src/anthias_server/app/templates/system_info.html Outdated
Comment thread docker/Dockerfile.viewer.j2 Outdated
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.77698% with 78 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@0bf0790). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/anthias_common/storage_health.py 88.15% 32 Missing and 13 partials ⚠️
src/anthias_common/smart.py 88.96% 10 Missing and 6 partials ⚠️
src/anthias_server/lib/storage_watcher.py 85.96% 4 Missing and 4 partials ⚠️
src/anthias_server/app/page_context.py 90.19% 4 Missing and 1 partial ⚠️
src/anthias_server/celery_tasks.py 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3286   +/-   ##
=========================================
  Coverage          ?   90.29%           
=========================================
  Files             ?       84           
  Lines             ?     9920           
  Branches          ?     1097           
=========================================
  Hits              ?     8957           
  Misses            ?      709           
  Partials          ?      254           

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

Balena audit. The resin-data volume starts empty on a first boot, and
ENOENT from the write check was classified as REASON_ERROR, which
lands in the failure path — so a brand-new healthy device could render
"this player can't save anything".

- ENOENT/ENOTDIR now map to REASON_MISSING and set write_ok to None
  rather than False, keeping the whole verdict out of the failure path
  instead of relying on every consumer to special-case a reason string
- not latched as a write failure either, so it cannot stick

Celery's wait_for_migrations closes the window in practice, since the
database lives in that directory. That is an ordering coupling worth
not depending on silently.

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

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 20 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/anthias_viewer/init.py:2668

  • SMART sampling resolves the backing disk via storage_health.probe(), but calling it with no data_dir uses storage_health.default_data_dir() (HOME-based). In the viewer process HOME is for the viewer user, so this can resolve the wrong mount (often the container root/overlay) and either skip SMART or report the wrong device. Pass the same config dir the server uses so both containers agree on the backing disk.
                facts = storage_health.probe()
                disk = facts.get('disk')
                # Only ever a real answer on SATA/NVMe. SD and eMMC
                # have their own registers and smartctl has nothing to
                # say about them, so most of the fleet skips this.
                if disk and facts['media']['kind'] == 'disk':
                    smart.publish(r, smart.collect(f'/dev/{disk}'))

src/anthias_server/api/views/v2.py:1149

  • The OpenAPI schema for /api/v2/info.storage.media says wear fields are only populated on eMMC, but storage_health also populates wear_pct/pre_eol from SMART for SATA/NVMe disks (kind == 'disk'). This makes the schema description misleading for API consumers.
                                    'What the device is. `kind` is '
                                    '`sd`, `emmc`, `disk` or '
                                    '`unknown`; the wear fields are '
                                    'populated on eMMC only, since '
                                    'SD cards do not report health.'

tests/test_template_views.py:3742

  • This autouse fixture patches get_state() to always return the same healthy dict instance. page_context._storage_card() mutates the dict it receives (adds/overwrites keys like warn/kind/parsed timestamps), so returning a shared instance can leak state between tests and make ordering matter. Return a fresh copy on each call.
    with mock.patch(
        'anthias_server.app.page_context.storage_health.get_state',
        return_value=healthy,
    ):
        yield

src/anthias_server/lib/storage_watcher.py:90

  • When storage status is STATUS_WEAR, this warning logs wear=%s%% directly from state['media']['wear_pct']. For SMART-driven wear warnings, wear_pct can legitimately be None (e.g., failed self-assessment or bad sectors without a vendor wear attribute), which would log as wear=None% and be confusing. Format wear as "unknown" when wear_pct is None.
    elif status == storage_health.STATUS_WEAR:
        logger.warning(
            'Storage on %s is nearing end of life (wear=%s%%, pre_eol=%s).',
            state.get('device'),
            state.get('media', {}).get('wear_pct'),

src/anthias_server/api/views/v2.py:1179

  • The /api/v2/info OpenAPI schema for storage.media is missing the wear_is_exact field, but anthias_common.storage_health.read_media_info() always returns it (True for NVMe, False otherwise). This makes the documented response shape incomplete.
                                    'wear_pct': {'type': ['integer', 'null']},
                                    'pre_eol': {'type': ['string', 'null']},

All nine verified against the code before fixing; none were false
positives.

- HIGH: ENOSPC latched write_failed_since_boot, so a full card that
  the operator then emptied flipped from "run out of space" to
  "replace the memory card" and stayed there until reboot — punishing
  them for following the advice. The latch now skips ENOSPC, which the
  module separates from hardware faults everywhere else
- NVMe media_errors set pre_eol but were not counted as bad blocks, so
  an NVMe drive got the wear copy and a "used N% of its writes" line:
  the mismatch _storage_kind exists to prevent, on the other bus
- "since it last restarted" rendered ext4's lifetime superblock
  counter; it now uses the per-boot delta the sentence claims
- errors_this_boot is bounded to the startup window it exists for.
  boot_time is time.time() - uptime, so a clock restored behind real
  time by fake-hwclock dragged errors from previous boots inside it
- a short os.write near ENOSPC read back as corruption, reporting a
  full filesystem as a card handing back bad data
- any NVMe critical_warning bit meant end-of-life, so a drive that had
  once run hot was told to plan a replacement; only the bits that mean
  it now count
- ATA vendor wear can no longer raise a warning alone. It counts down
  from 100 by convention, not spec, and an inverted attribute reads as
  worn out when new — smart.py's docstring already claimed the
  well-defined signals carry the verdict; now they do
- the healthy card claimed "the last write test passed" when none had
  run (notably with Redis down) and "no storage errors" on a
  filesystem with no counter
- drive serials reached a bundle documented as safe to post publicly;
  scrub_pii now redacts them, keeping model and firmware
- dropped a .device-alerts gap rule that specificity made dead code

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

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 20 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/anthias_viewer/init.py:2668

  • storage_health.probe() defaults to $HOME/.anthias, but the viewer runs as the viewer user and $HOME may not be the same directory the server uses for Anthias data/config. That can make the viewer resolve and publish SMART for the wrong backing device (e.g., the container rootfs) and disagree with the server/UI about which disk is being described. Pass the same config dir the server uses when probing.
                facts = storage_health.probe()
                disk = facts.get('disk')
                # Only ever a real answer on SATA/NVMe. SD and eMMC
                # have their own registers and smartctl has nothing to
                # say about them, so most of the fleet skips this.
                if disk and facts['media']['kind'] == 'disk':
                    smart.publish(r, smart.collect(f'/dev/{disk}'))

src/anthias_server/app/templates/system_info.html:186

  • The failing-storage System Info card says “Replace it…”, which is incorrect/unclear for non-replaceable storage (e.g. soldered eMMC) and also ambiguous for x86 (“it” could be the player). Since the banner at the top already gives the correct hardware-specific action, this sentence should just refer the operator to that warning.
          {% if storage.errors_new %}{{ storage.errors_new|intcomma }} storage
          error{{ storage.errors_new|pluralize }}{% else %}A storage error{% endif %}
          since this player last restarted.
          {% endif %}
          Replace it and see the warning at the top of the page.
        </p>

src/anthias_server/lib/storage_watcher.py:92

  • This log message renders wear=None% when the status is wear but the underlying signal is pre_eol (or other SMART evidence) without a numeric wear percentage. That produces a misleading/garbled message in support logs; prefer logging wear=unknown (or similar) when no percentage is available.
            'Storage on %s is nearing end of life (wear=%s%%, pre_eol=%s).',
            state.get('device'),
            state.get('media', {}).get('wear_pct'),
            state.get('media', {}).get('pre_eol'),
        )

- viewer: pass settings.get_configdir() to the SMART probe instead of
  letting it fall back to $HOME. Copilot's premise was wrong -- HOME is
  /data in every compose template and survives start_viewer.sh's
  `sudo -E -u viewer`, confirmed on the x86 testbed -- but resolving
  the disk the server reports on should not hinge on an env var
  surviving a shell script, and celery already passes it explicitly
- storage_watcher: STATUS_UNKNOWN fell into the else branch and logged
  "healthy again", stating the opposite of what happened
- system_info: `{% if fsync_ms %}` hid a real 0.0 ms reading. Swept the
  rest of the template for the same falsy-zero class rather than
  patching the one line reported: power_on_hours and temperature_c had
  it too (0 C is a temperature). The sector counters stay falsy-tested
  on purpose, where zero really does mean "none"
- sudoers: pinned to the exact argument list rather than the bare
  binary, so the grant is "read three SMART pages off a block device"
  instead of "run smartctl as root however you like" -- it can also
  write (-t starts self-tests, -s toggles SMART). Verified on the
  testbed that the read still works as the viewer user and that
  `-t short` is now denied

Added a test asserting the sudoers rule and smart._argv() agree: sudo
matches arguments literally and a mismatch denies silently, which would
look exactly like a disk with no SMART.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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 20 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/anthias_server/lib/storage_watcher.py:126

  • last_write_check is updated whenever write_check is true, even if storage_health.record_check() didn’t actually run the write check (it skips writes when the filesystem can’t be resolved). If supported temporarily flips false, this stamps the interval anyway and can delay the next real write check after recovery.
                # Stamped after the call, not before: on a card that
                # is struggling the check itself can take a while,
                # and the interval should be a gap between checks
                # rather than a deadline they overrun.
                last_write_check = time.monotonic()

tests/test_template_views.py:3742

  • The autouse _healthy_storage fixture returns the same healthy dict instance for every call to storage_health.get_state. page_context._storage_card() mutates the returned dict (adds warn, kind, parses timestamps, etc.), so later requests/tests can observe cross-test state and become order-dependent/flaky.
    with mock.patch(
        'anthias_server.app.page_context.storage_health.get_state',
        return_value=healthy,
    ):
        yield

The re-review reported no new comments but listed two suppressed ones.
Both hold up.

- the watcher stamped last_write_check whenever it asked for a write
  check, but record_check only performs one when the filesystem
  resolved. A transient unresolvable mount therefore consumed the
  interval and deferred the next real check by up to 15 minutes past
  recovery. Gated on the same condition record_check applies
- _storage_card mutated the dict get_state returned. Harmless in
  production, where each call builds a fresh one, but the function
  cannot know that and _parse_iso is destructive on a second
  application — it returns None for an already-parsed datetime, so
  re-decorating the same dict silently blanks every timestamp. It now
  copies before mutating, and the test doubles use side_effect so they
  behave like the real function instead of handing out one shared
  instance

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vpetersson-bot and others added 3 commits August 15, 2026 17:31
The quality gate passed, so none of these surfaced as PR comments —
they only exist on the SonarCloud dashboard.

- split smart.parse into per-protocol helpers (cognitive complexity 30
  vs 15 allowed): NVMe and ATA are two dialects that had accumulated in
  one function, and separating them makes the "NVMe is specified, ATA
  is convention" distinction structural rather than a comment
- extract _fold_write_check from record_check (18 vs 15)
- collect_debug.sh: `[` -> `[[` in the new functions. Sonar is right and
  the file already preferred `[[` 16:6, so this was inconsistent with
  its own conventions, not just the rule
- explicit `return 0` on the three report-producing functions
- watcher tests: ruff's SIM117 wants the context managers combined and
  Sonar's S5778 then counts them as multiple throwing invocations. The
  callable form of pytest.raises satisfies both rather than suppressing
  either, and a named function replaces the lambda S9081 flagged (it
  has to block, so return_value cannot express it)

One deliberate NOSONAR, per the repo's own guidance on genuine false
positives: S7677 wants the "no ext4 filesystems" line on stderr, but
that is report content section() captures into storage.txt. Redirecting
it would drop it from the bundle and leave an empty section reading as
"could not check" instead of "nothing to report".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Swapping pytest.raises to its callable form traded a CODE_SMELL
(S5778) for four BUG-severity S9000s, which dropped Reliability on New
Code to C and failed the gate. I changed the form without checking the
rule types, which made things worse rather than better.

Three rules were pulling against each other: S9000 wants the context
manager form, S5778 wants a single throwing invocation in that scope,
and ruff's SIM117 forbids nesting the two `with`s. monkeypatch settles
all three -- no context managers to combine, so there is one `with`
holding one call.

Also finishes what that commit left:
- extract _derive_pre_eol; parse was still at 17 against the 15 allowed
- explicit return in storage_kernel_log, missed the first time
- NOSONAR moved onto the flagged line. As a preceding comment block it
  was simply ignored, so the S7677 false positive survived

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last four S5778s survived because the block still held two calls,
not one: mock.MagicMock() as well as _watch_loop. That is exactly the
rule's point -- with both inside, you cannot tell which one threw.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@vpetersson
vpetersson merged commit ddc1845 into Screenly:master Aug 16, 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