Skip to content

Harden disk collector: exclude network filesystems, bound statfs, df-compatible used_percent#40

Merged
LarsLaskowski merged 1 commit into
mainfrom
claude/issues-26-27-dngnvf
Jul 13, 2026
Merged

Harden disk collector: exclude network filesystems, bound statfs, df-compatible used_percent#40
LarsLaskowski merged 1 commit into
mainfrom
claude/issues-26-27-dngnvf

Conversation

@LarsLaskowski

@LarsLaskowski LarsLaskowski commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes the two disk-collector robustness findings from the 2026-07-13 project review:

#26 — a hung network filesystem blocked the entire metric loop. statfs(2) on an unreachable NFS/CIFS server blocks (indefinitely for hard mounts), and fastTick runs all collectors sequentially, so a single dead NAS mount froze every metric while the dashboard silently served a stale snapshot. Two complementary changes:

  • Network filesystem types (nfs, nfs4, cifs, smbfs, smb3, 9p, fuse.sshfs, davfs, afs, glusterfs, ceph, curlftpfs) are now in defaultExcludedFSTypes — local storage is what the dashboard is about; remote capacity belongs to the remote system's monitoring.
  • Defense in depth: each statfs call now runs in a goroutine behind a 2 s timeout (statfsWithTimeout). Since statfs cannot be canceled, a timed-out goroutine lingers until the syscall returns; to bound the pile-up, a timed-out mountpoint enters a 1-minute cool-down (badUntil) and is skipped instead of being retried every tick.

#27 — misleading disk-usage math and duplicate mountpoints.

  • used_percent now uses df's formula used / (used + Bavail) instead of used / total from Bfree. Bfree counts root-reserved blocks (typically 5 % on ext4) as free, so a full disk showed ~95 % — below the default disk_crit_percent: 95 — while df reported 100 % and services were already failing to write. total_bytes/used_bytes keep their raw meaning; only the percentage basis changes (value semantics, no JSON shape change, so no API version bump). Documented in docs/API.md.
  • Duplicate mountpoints in /proc/mounts (overmounts, some bind-mount setups) are deduplicated keeping the last entry — the filesystem actually visible at that path — preserving first-seen order. This also stops fastTick from writing two HistoryPoints with the same timestamp into one ring-buffer key per tick.

Note one intentional behavior change encoded in the updated TestDiskCollector_Collect: the existing fixture ends with an overlay overmount of /, so after keep-last dedup / resolves to the overlay entry and is excluded (previously it was reported as ext4 with values that statfs("/") actually took from the overlay).

Related Issue

Closes #26
Closes #27

Checklist

  • Tests added/updated for the change (go test ./... passes locally) — including go test ./... -race; new tests cover network-FS exclusion, the df percentage formula (Bfree > Bavail), zero-denominator guard, mountpoint dedup (later entry wins, one stat per unique mountpoint), and a hung statfs (returns within the timeout, healthy mounts still reported, exactly one attempt per cool-down window)
  • go vet ./... and golangci-lint run are clean (0 issues)
  • Documentation updated if this changes the REST API (docs/API.md) — added notes on df-compatible used_percent semantics, dedup, and network-FS exclusion under disks
  • No breaking change to /api/v1/... response shapes, or a new API version was introduced instead — JSON shape is unchanged; only the percentage's value basis changes to match df

Not verifiable without hardware: behavior against a real hard-mounted NFS share on a Pi (the hang was reproduced with an injected blocking statfsFunc, per the acceptance criteria in #26).

Fixes two robustness findings from the 2026-07-13 project review:

Issue #26 — a hung network filesystem froze the entire metric loop.
statfs(2) on an unreachable NFS/CIFS server blocks (indefinitely for
hard mounts), and fastTick runs all collectors sequentially, so one dead
NAS mount silently froze every metric. Network filesystem types (nfs,
nfs4, cifs, smb3, fuse.sshfs, 9p, ...) are now excluded by default —
remote capacity belongs to the remote system's monitoring — and as
defense in depth each statfs call runs behind a 2s timeout. A timed-out
mountpoint is skipped for a 1-minute cool-down so abandoned goroutines
(statfs cannot be canceled) do not pile up one per tick.

Issue #27 — used_percent was computed as used/total from Bfree, which
counts root-reserved blocks (5% on ext4) as free: the dashboard showed
~95% while df already reported 100% and services failed to write, under
the default disk_crit_percent threshold. The percentage now uses df's
formula, used / (used + Bavail). total_bytes and used_bytes keep their
raw meaning; only the percentage basis changes, so no API version bump.
Duplicate mountpoints in /proc/mounts (overmounts, bind-mount setups)
are also deduplicated, keeping the last entry — the filesystem actually
visible at the path — so history ring buffers no longer receive two
points per tick for the same mountpoint.
@LarsLaskowski
LarsLaskowski merged commit 0c667fe into main Jul 13, 2026
3 checks passed
@LarsLaskowski
LarsLaskowski deleted the claude/issues-26-27-dngnvf branch July 13, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants