Repository navigation
Conversation
Starosdev
requested changes
Sep 11, 2026
Starosdev
left a comment
Starosdev
left a comment
Owner
There was a problem hiding this comment.
@CaptainThrillsDev thanks for the SCSI support work. Request changes for two correctness issues:
-
webapp/backend/pkg/models/measurements/smart.go:1039creates SCSIpercentage_usedwithThreshold: -1. That disables threshold evaluation, socurrent_percent > 100remainsPasseddespite critical metadata. Use threshold100, matching NVMe, and add a regression test. -
webapp/backend/pkg/database/scrutiny_repository_device_self_tests.go:74-83sends SCSI self-tests through ATA’s 16-bit65536rollover logic. SCSI entries at>=65536hours get unknown effective ages. Add SCSI-specific absolute-hour handling and a>65535test.
README still says self-test history is ATA-only. Please update that text and link the PR to its tracking issue.
Validation: go test ./... passes. Focused reproductions confirmed both issues. Frontend checks remain unreported.
Author
|
Updated based on comments, by tracking issue, do you need an issue opened for the link in the README, or was the linked PR in the README ok like it is now? (https://github.com/Starosdev/scrutiny/pull/848/changes#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R576) |
Starosdev
requested changes
Sep 12, 2026
Starosdev
left a comment
Starosdev
left a comment
Owner
There was a problem hiding this comment.
Thanks for the follow-up. I completed a second pass on current head b80b0a0.
Both prior functional findings are fixed:
- SCSI/SAS endurance now uses a 100% threshold, with regression coverage.
- SCSI/SAS self-test hours now use absolute bounds, with coverage above ATA’s 16-bit rollover.
One documentation issue remains:
README.md:537says workload calculations use only ATA and NVMe counters, but this PR also adds SCSI/SAS support. Please include SCSI/SAS in this list or clarify that SCSI/SAS uses its separate gigabytes-processed path.
Self-test documentation is now aligned. The PR link in the self-test section is acceptable as background context, though it is not a substitute for a tracking issue.
go test ./... passes on this head. Requesting changes only for README alignment.
11 of 15 tasks
8 of 15 tasks
Starosdev
added a commit
that referenced
this pull request
Oct 28, 2026
## Summary Drives that support the SCSI Verify Error Counter log page (0x05) report the same seven counters as the read and write pages, but Scrutiny decoded only read and write, so the verify counters never reached the attribute table or its history. This is the first of three stacked PRs for #1024. It adds a `verify_*` attribute per counter, plus `verify_gigabytes_processed`. ## Type of Change - [ ] Bug fix (non-breaking change that fixes an issue) - [x] 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 #1024. The issue asks for five groups of SCSI/SAS fields; this PR delivers group 4 (verify error counters). It is not a complete fix for the issue, so it carries no closing keyword. Builds on the SCSI work in #848, and relates to #919 and #470. ## Changes Made - Decode the Verify Error Counter log page. The read, write and verify pages share one named `ScsiErrorCounters` type, replacing the two duplicated anonymous structs. - `Verify` is a pointer, so a drive that does not support the page gets no verify attributes at all, rather than seven permanently-zero ones that a value type could not tell apart from a drive reporting real all-zero counters. - Seven new attributes with display metadata: the six counters plus `verify_gigabytes_processed`. - Define the SCSI attribute set once. `ProcessScsiSmartInfo` (the legacy InfluxDB migration path) and `processScsiSmartInfoWithOverrides` (ingestion) now share one `buildScsiAttributes` helper instead of carrying two copies of the same map, which is what made adding a page mean editing two places. - Read and write attribute IDs, values and thresholds are unchanged. Verified by extracting every `(AttributeId, Threshold)` pair before and after and comparing. ### Deliberately not included - Replacement-risk weights are untouched. `ScsiReplacementRiskWeights` sums to 100, so adding a verify signal means rebalancing a table that moves the risk score of every SAS drive. That deserves its own change. - Verify counters are recorded without threshold evaluation in this PR. The follow-up PR turns them on together with the delta-based evaluation that makes a cumulative counter safe to judge. ## Testing - [ ] I have tested this locally - [x] I have added/updated tests that prove my fix/feature works - [ ] All existing tests pass To be explicit about the two unchecked boxes: the Go test suite was **not** run locally, by operator policy on this machine, so CI is the first run of it. What was checked locally: - `go build ./webapp/...` and `go vet` over the changed packages: clean. - `golangci-lint` v2.14.0, the version CI pins, run two ways: `--new-from-rev=origin/develop` from the real worktree path, and a full-module run cross-checked against this branch's diff line ranges. Both report zero findings on changed lines. This caught `fieldalignment` findings on the new structs, which are fixed here. - New tests: `TestFromCollectorSmartInfo_Scsi_Verify_ErrorCounters` (counter values, thresholds, metadata coverage for every emitted attribute ID) and `TestFromCollectorSmartInfo_Scsi_Verify_Absent` (a drive without the page gets no verify attributes and keeps its existing 16-attribute set). - New fixture `smart-scsi-sas-verify.json`: the existing Seagate `smart-scsi.json` plus a verify page. The field names and two of the values (`correction_algorithm_invocations` 7054, `gigabytes_processed` "0.000") come from the `smartctl-ultrastar-sas-samples.json` attachment on the issue; the three error counts are synthetic, chosen so the test exercises a nonzero recovered-error path. ## Checklist - [x] My code follows the project's code style - [x] I have performed a self-review of my code - [x] I have commented my code where necessary (particularly complex areas) - [x] I have updated the documentation if needed - [x] My changes generate no new warnings - [x] No console.log, debug statements, or commented-out code `TESTING.md`'s fixture inventory gains a row for the new fixture. The OpenAPI collector schema is updated in the third PR of the stack, in one place for all three pages. ## Additional Notes Review: required (merging to `develop` publishes the testing image through `deploy-testing.yml`, with no separate deploy step). Merge order matters: this PR is the base of `feat/1024-sas-phy-counters`, which is the base of `feat/1024-scsi-lifetime-counters`. Merge this one first. Why the verify counters are not thresholded yet, in one line: they are cumulative and commonly already nonzero on a healthy drive, so failing a drive on an absolute value would alarm existing SAS users on upgrade over errors that may be years old.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adding additional support for SCSI drives to read gigabytes processed (read/written) from information log sections. Fix a missing scsi_revision handler. Added selftest support for the top level sibling SCSI selftests keyed as scsi_self_test_N where N is the test number.
Type of Change
Changes Made
Testing
Used the workflow to build docker images to test web changes, unit tests added for associated changes
Checklist
Screenshots (if applicable)
Additional Notes
2 HP SAS SSD's (Samsung PM1643a OEM's) spurred on this change to get SAS/SCSI support updated within the app. This PR might be opinionated against those, as I have no additional SAS devices to work with (everything else on the expander/hba is SATA). Put together by Opus 4.8 & Sonnet 5 with some in the loop guidance, if it feels too vibe-slop'ed I can close and refine.