Skip to content

fix(smart): detect manufacturer SMART failures for ATA and NVMe and keep them through recalculation - #970

Merged
Starosdev merged 5 commits into
betafrom
fix/965-verdict-correctness
Oct 13, 2026
Merged

Starosdev merged 5 commits into
betafrom
fix/965-verdict-correctness

Conversation

@Starosdev

@Starosdev Starosdev commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

Summary

WP-A (verdict correctness) for the 2.0.0 release. Manufacturer SMART failures that Scrutiny never flagged are now detected for ATA and NVMe, NVMe critical warnings are explained, and a device keeps its SMART-failure status when attribute overrides change.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

Related to #965

Changes Made

  • when_failed compared against FAILING_NOW / IN_THE_PAST, but smartctl JSON emits now / past (see testdata/smart-fail2.json:217), so attribute-level manufacturer threshold failures were never flagged. Constants now match smartctl, compared case-insensitively. now sets FailedSmart; past sets a warning.
  • NVMe critical_warning != 0 now sets the attribute and device FailedSmart status, not only FailedScrutiny, in both NVMe status paths (respecting ignore overrides). Status reasons decode bits 0-5 of the NVMe spec and report unknown bits.
  • NVMe percentage_used: warning at 90 or above; above 100 stays a Scrutiny failure as before (the spec does not treat 100 itself as failure).
  • ATA ingest now sets device FailedSmart when a non-ignored, non-transient attribute is FailedSmart, matching the NVMe path. failures.transient.ata (default [195]) is honored for FailedSmart at ingest and, for both FailedScrutiny and FailedSmart, during override recalculation, so the two agree.
  • recalculateStatusFromSmart (override changes) only propagated FailedScrutiny, so FailedSmart was dropped. It now keeps FailedSmart from non-ignored attributes and keeps the existing device bit (which carries smartctl's overall smart_status, not stored with the measurement) unless the only evidence was an attribute an override now ignores. persistRecalculatedDeviceStatus writes the exact recalculated bitset, so bits can be cleared as well as set.

Testing

  • I have tested this locally
  • I have added/updated tests that prove my fix/feature works
  • All existing tests pass

go build ./..., go vet ./..., go test ./... pass; GOOS=linux golangci-lint run --new-from-rev=origin/beta ./... (v2.14.0) 0 issues. New tests: real-fixture ATA when_failed (attribute and device status), NVMe critical_warning decoding and device status, percentage_used bands, notify filter with status_threshold=smart, status recalculation (keeps FailedSmart, ignored attribute drops it, smart_status bit kept, FailedScrutiny-only unchanged, transient 195 skipped), persistRecalculatedDeviceStatus against in-memory SQLite (no-op, clear one bit, reset), ATA ingest with an ignored or transient failing attribute, and warn-level notification for when_failed: "past". Removing the transient guard makes the transient ingest test fail.

Checklist

  • My code follows the project's code style
  • I have performed a self-review of my code
  • I have commented my code where necessary (particularly complex areas)
  • I have updated the documentation if needed
  • My changes generate no new warnings

Notes

  • No data migration: status is computed at ingest, so existing devices re-evaluate on their next collector run. Expect some devices to change from passed to failed after upgrade, with notifications; this belongs in the 2.0 release notes.
  • Known limit: a device whose FailedSmart bit came from smart_status and that also has an ignored FailedSmart attribute and no other one loses the bit on recalculation, because smart_status is not stored per measurement.
  • when_failed: "past" is permanent in a drive's history and now yields a warning; with warn-level notifications and repeat notifications enabled, such a drive notifies on every run. NVMe critical_warning temperature bit now counts as a SMART failure. Both belong in the 2.0 release notes.
  • Frontend has no mirror of the old constants; it only displays the new NVMe status reasons.

Review: not required (verdict logic on the beta integration branch; no production effect until a release, but an independent read-only review is being run per release coordination)

smartctl JSON emits "now" and "past" for ata_smart_attributes when_failed,
but the constants were FAILING_NOW and IN_THE_PAST, so no ATA attribute was
ever flagged FailedSmart or given the past-failure warning. Switch the
constants to the real values (comparison stays case-insensitive). The legacy
strings are never produced by smartctl and nothing stored or tested relies
on them, so they are not kept.

Drives with a "past" attribute now show WarningScrutiny and drives with a
"now" attribute show FailedSmart at attribute level. Stored data is not
changed; status is recomputed on the next ingest from the persisted raw
when_failed field.
A non-zero NVMe critical_warning now sets AttributeStatusFailedSmart as well
as FailedScrutiny, and the device status gets DeviceStatusFailedSmart, so
status_threshold=smart users and notifications see it. The status reason
decodes the NVMe spec bits (spare below threshold, temperature, reliability
degraded, read-only, volatile backup failed, PMR read-only) and flags unknown
bits.

percentage_used gains a warning band at 90 percent used. Failure stays at
values above 100 (the existing threshold): the NVMe spec says 100 means rated
endurance is consumed but is not itself a failure, and vendors may report up
to 255. 90 is conservative enough to give months of notice on typical
workloads without alerting on healthy mid-life drives.
…tion

recalculateStatusFromSmart rebuilt device status from Passed and only
propagated FailedScrutiny, so an override change dropped
DeviceStatusFailedSmart. It now keeps FailedSmart from any non-ignored
attribute with AttributeStatusFailedSmart, and keeps the device's existing
bit (which carries the smartctl overall smart_status, not stored with the
measurement) unless the only evidence for it was an attribute an override now
ignores. persistRecalculatedDeviceStatus writes the exact recalculated
bitset instead of only handling passed/failed transitions.

ATA ingest now also sets DeviceStatusFailedSmart when a non-ignored attribute
is FailedSmart (when_failed "now"), matching the NVMe path, so ingest and
recalculation agree.
@Starosdev Starosdev added bug Something isn't working backend Backend/Go related labels Oct 13, 2026
Ingest set DeviceStatusFailedSmart from an attribute outside the
failures.transient.ata guard, so a transient attribute (default 195) with
when_failed "now" failed the device. Gate it the same way as FailedScrutiny.
recalculateStatusFromSmart now takes the config and skips transient ATA
attributes for both failure kinds, so ingest and override recalculation agree.

Also fix a stale field comment on the NVMe warning band, and add tests for
persistRecalculatedDeviceStatus (clear one bit, unchanged, reset), transient
and ignored ATA ingest, transient recalculation, and warn-level notification
for a when_failed "past" attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backend/Go related bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant