Skip to content

fix collector error handling - #1032

Draft
kaysond wants to merge 3 commits into
masterfrom
collector_error_handling
Draft

fix collector error handling#1032
kaysond wants to merge 3 commits into
masterfrom
collector_error_handling

Conversation

@kaysond

@kaysond kaysond commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Closes #944
Closes #945

Builds on the existing wip commit — master is merged in and the work is finished in commits on
top, so the diff of what changed since wip stays reviewable.

#944 — stop storing smartctl's failure output

smartctl.exit_status is now a typed bitmask
(webapp/backend/pkg/models/collector/smartctl_exit_status.go) matching
smartctl.h,
used by both collector.SmartInfo and models.Scan.

The bits split cleanly in two:

  • bits 0-2 (FAILCMD, FAILDEV, FAILSMART) mean smartctl could not produce usable output —
    SmartctlExitStatus.IsFatal()
  • bits 3-7 describe problems with the disk, which is exactly the data worth keeping

Two independent guards, because either side can be older than the other:

  • the collector does not publish a result whose exit status is fatal
  • UploadDeviceMetrics rejects such a payload with 400 before touching either database

LogSmartctlExitCode also became LogSmartctlExitStatus and now logs every set bit. The old
version was an if / else if chain, so exit status 0x18 only ever logged "failing disk" and
swallowed the pre-fail bit.

#942 (-n standby)

Exit status 2 is shared: it means both "device open failed" and "device is in a low power mode and
-n asked me to exit". When FAILDEV is the only bit set and the configured
metrics_smart_args contain -n/--nocheck, the drive is simply asleep — the collector logs that
at info level and moves on without publishing and without notifying.

That is deliberately conservative: a drive that has genuinely gone away also disappears from
smartctl --scan, so it never reaches Collect() in the first place.

#945 — notifications for collector failures

New route POST /api/collector/error, handled by handler.CollectorError. The collector reports:

  • smartctl --scan failures (no device attached to the report)
  • per-device smartctl --info failures — previously these were swallowed with //ignore errors.
    and the device silently vanished from the dashboard, because a device that fails --info never
    gets a ScrutinyUUID
  • smartctl --xall failures that are fatal per the above

Detect.Start() is split into Scan() and Info(), and Info() returns a
[]detect.DeviceInfoError so each failing device is reported with its own error rather than one
shared message.

New setting metrics.notify_collector_errors, defaulting to true (opt-out, as the issue asked),
with a migration (m20260725120000) and a "Notify - Collector Errors" control in the settings dialog.

Differences from the wip commit

  • POST /api/device/:scrutiny_uuid/smart had been dropped from server.go — restored. Without
    it the collector cannot upload anything.
  • The lo.Filter predicate kept devices where ScrutinyUUID.IsNil() was true, i.e. exactly the
    devices that were meant to be dropped. Resolved in favour of the loop that landed on master in
    fix(collector): keep only devices with non-nil ScrutinyUUID #991.
  • Two endpoints (/api/collector_scan_error and /api/device/:scrutiny_uuid/collector_error)
    collapsed into one. The device-scoped one had to invent a UUID via
    GenerateScrutinyUUID("", "", deviceName) for the --info failure case, which produces an ID that
    matches no real device; the single endpoint carries the device name and an optional UUID instead.
  • SmartctlExitStatus lives in webapp/backend/pkg/models/collector rather than
    collector/pkg/models, so the dependency keeps pointing collector → webapp models (the direction
    detect.go already uses) instead of adding the reverse edge.
  • Per-flag HasFailCmd()/HasFailDev()/… replaced by Has(flags), IsFatal() and
    Descriptions().

Tests

  • smartctl_exit_status_test.go — which exit codes are fatal, and that all set bits get reported
  • TestHasPowerModeCheck — the -n/--nocheck forms smartctl accepts
  • TestUploadDeviceMetricsRoute_RejectsFatalSmartctlExitStatus — uploads good data, then the same
    payload with exit status 2, and asserts 400 plus that the stored summary is byte-for-byte unchanged
  • TestCollectorErrorRoute — posts a collector error and asserts a notification actually reaches a
    webhook, with the right failure type and message

AI disclosure

Per AI_POLICY.md: the commits after wip were written by Claude Code (Opus 5).
It did the merge conflict resolution, the implementation, and the checks below, all inside containers
(golang:1.25-trixie with an influxdb:2.8 sidecar, node:24-trixie with headless Chromium);
nothing was installed on the host.

Automated verification performed:

  • go build ./... and go vet clean; cross-compiled for windows/amd64, darwin/arm64 and
    freebsd/amd64 so the per-OS detect files all still compile
  • go test ./... against a live influxdb — all packages pass, including the
    webapp/backend/pkg/web integration suite
  • npm run build:prod and npx ng test --watch=false --browsers=ChromeHeadless --code-coverage
  • gofmt clean

That is the extent of the verification behind this PR as opened. It has not been run against real
hardware, and in particular the -n standby path has not been exercised against a drive that is
actually asleep.

kaysond added 3 commits April 23, 2026 13:16
…dling

# Conflicts:
#	collector/pkg/collector/metrics.go
#	webapp/backend/pkg/models/collector/smart.go
smartctl.exit_status becomes a typed bitmask matching smartctl.h. Bits 0-2
(FAILCMD, FAILDEV, FAILSMART) mean smartctl could not produce usable output, so
the collector stops publishing those results and the backend rejects them before
writing to either database. The remaining bits describe problems with the disk
itself, which is data worth keeping.

Exit status 2 is ambiguous: it also means the drive was in a low power mode and
`-n` asked smartctl to exit. When that bit is the only one set and the configured
smart args contain -n/--nocheck, the drive is simply asleep, so it is skipped
without a notification.

Adds POST /api/collector/error so the collector can report scan failures, per
device `smartctl --info` failures (previously swallowed, which made the device
silently disappear) and fatal `smartctl --xall` failures. The backend turns those
into notifications, controlled by a new opt-out metrics.notify_collector_errors
setting.

LogSmartctlExitCode was an if/else if chain, so only the lowest set bit was ever
logged. It now reports every set bit.

Closes #944
Closes #945
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.

Add notifications for collector failures Backend will add invalid data to the database if smartctl returns a json-formatted error

1 participant