Skip to content

fix(notify): don't re-notify a past-only SMART failure when its value moves - #1144

Closed
RyoKusnadi wants to merge 1 commit into
Starosdev:developfrom
RyoKusnadi:fix/skip-value-change-for-past-failures
Closed

RyoKusnadi wants to merge 1 commit into
Starosdev:developfrom
RyoKusnadi:fix/skip-value-change-for-past-failures

Conversation

@RyoKusnadi

Copy link
Copy Markdown

Summary

With repeat notifications off, a drive whose only problem is a when_failed: "past" attribute still sends a SMART notification on every collector run. It now notifies once, when the attribute first shows up.

How it happens: attribute 190 (Airflow_Temperature_Cel) is 100 minus the temperature, so its value moves a point or two between runs. past becomes a WarningScrutiny status (since #1126), which qualifies at notify level Warn. hasQualifyingFailure then sees "same status, different value" and calls it a change, every time.

Steps to reproduce: Warn level, Repeat off, a drive with attribute 190 when_failed: "past". Submit twice with the value going 72 then 71.
Expected: one notification. Actual: one per submission.

Cause: hasQualifyingFailure in notify.go compares the evaluated value. For a past mark the value says nothing about the drive's state.

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

Fixes #1143 (the "notify once" outcome it lists as expected). Whether past should notify at all is left open.

Changes Made

  • smart_ata_attribute.go: new FailedOnlyInThePast(), true when past is the attribute's only status and only reason.
  • notify.go: skip the value comparison for such an attribute. A new or escalated status still notifies, and an attribute with its own warning keeps the value check.

Scope: a value-based warning on a moving attribute (like temperature) can still re-notify; that is existing behaviour and left alone. The same applies to the beta branch (#1129), which gets this through the next sync.

Testing

  • I have tested this locally
  • I have added/updated tests that prove my fix/feature works
  • All existing tests pass (except one local-only failure, below)

New: TestShouldNotify_NoRepeat_AtaPastFailureValueMovesIsNotChange and ..._AtaPastFailureWithOtherWarningValueChanged. Before the fix the first one fails ("Should be false"); after, both pass.

Run: go vet and go test on ./pkg/notify/ ./pkg/models/measurements/ ./pkg/web/handler/. TestScriptStreamsOutputAndPreservesEnvironment fails on my Mac (same as noted in #1126); the rest pass. Not run: full suite, a real drive. golangci-lint can't load the repo (vendor/modules.txt out of sync), so it was skipped.

Environment: fix/skip-value-change-for-past-failures @ e1900f7, go1.27.1, darwin/arm64

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 (no docs describe this)
  • My changes generate no new warnings
  • No console.log, debug statements, or commented-out code

… moves

With repeat notifications off, an ATA attribute whose only problem is
when_failed "past" notified on every run because attributes like 190
(100 minus temperature) change value between runs. Skip the value-change
check for such attributes; a new or escalated status still notifies.

Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
@RyoKusnadi

Copy link
Copy Markdown
Author

Closing this one, sorry for the noise.

@RyoKusnadi RyoKusnadi closed this Nov 8, 2026
@RyoKusnadi
RyoKusnadi deleted the fix/skip-value-change-for-past-failures branch November 8, 2026 09:44
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.

1 participant