From d50c09176f805ecb0367971b5dfae0522ff7fdf9 Mon Sep 17 00:00:00 2001 From: Yvan Wang <131545713+BootstrapperSBL@users.noreply.github.com> Date: Sat, 15 Aug 2026 02:13:33 +0800 Subject: [PATCH] fix(agent): honor explicit SMART_DEVICES type hint instead of scan-detected type (#2102) When SMART_DEVICES specifies an explicit type (e.g. /dev/sda:scsi), the agent resolved the device type correctly but smartctlArgs dropped the -d flag for scsi/ata (the #1345 scan-misdetection workaround), so smartctl re-detected the wrong type (sat) and collection failed on USB drives whose bridge does not support SAT passthrough. Mark types that come from an explicit SMART_DEVICES hint and always pass them through via -d, while still letting scan-detected scsi/ata auto-detect as before. Adds regression tests for the arg building, the full parse -> merge -> args path, and flag preservation across rescans. Fixes #2072 --- agent/smart.go | 20 ++++++++++-- agent/smart_test.go | 75 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/agent/smart.go b/agent/smart.go index a8c0c52e..0c052f21 100644 --- a/agent/smart.go +++ b/agent/smart.go @@ -55,6 +55,11 @@ type DeviceInfo struct { typeVerified bool // parserType holds the parser type (nvme, sat, scsi) that last succeeded. parserType string + // explicitType reports whether Type came from an explicit ":type" hint in + // SMART_DEVICES. Such a type is a deliberate user override and must always be + // passed to smartctl via -d, even for scsi/ata where a scan-detected type is + // otherwise left off (see smartctlArgs and issue #1345). + explicitType bool } // deviceKey is a composite key for a device, used to identify a device uniquely. @@ -251,8 +256,9 @@ func (sm *SmartManager) parseConfiguredDevices(config string) ([]*DeviceInfo, er } devices = append(devices, &DeviceInfo{ - Name: name, - Type: devType, + Name: name, + Type: devType, + explicitType: devType != "", }) } @@ -558,7 +564,9 @@ func (sm *SmartManager) smartctlArgs(deviceInfo *DeviceInfo, includeStandby bool deviceType = strings.ToLower(deviceInfo.Type) parserType = strings.ToLower(deviceInfo.parserType) // types sometimes misidentified in scan; see github.com/henrygd/beszel/issues/1345 - if deviceType != "" && deviceType != "scsi" && deviceType != "ata" { + // An explicit SMART_DEVICES ":type" hint is a deliberate override, so always + // pass it through; otherwise scsi/ata are left off so smartctl can auto-detect. + if deviceType != "" && (deviceInfo.explicitType || (deviceType != "scsi" && deviceType != "ata")) { args = append(args, "-d", deviceInfo.Type) } } @@ -663,6 +671,9 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo target.Type = prev.Type target.typeVerified = true target.parserType = prev.parserType + if prev.explicitType { + target.explicitType = true + } } // applyConfiguredMetadata updates a matched device with any configured @@ -676,6 +687,9 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo existingDev.typeVerified = false existingDev.parserType = normalizeParserType(newType) } + if configuredDev.explicitType { + existingDev.explicitType = true + } if configuredDev.InfoName != "" { existingDev.InfoName = configuredDev.InfoName } diff --git a/agent/smart_test.go b/agent/smart_test.go index f66f6b16..65ad4ac5 100644 --- a/agent/smart_test.go +++ b/agent/smart_test.go @@ -392,6 +392,81 @@ func TestSmartctlArgs(t *testing.T) { ) } +// TestSmartctlArgsExplicitType verifies that an explicit SMART_DEVICES type hint +// is always passed to smartctl via -d, while a scan-detected scsi/ata type is +// still left off so smartctl can auto-detect it (see issue #1345). +func TestSmartctlArgsExplicitType(t *testing.T) { + sm := &SmartManager{} + + // Scan-detected scsi: -d is intentionally omitted. + scanScsi := &DeviceInfo{Name: "/dev/sda", Type: "scsi"} + assert.Equal(t, + []string{"-a", "--json=c", "/dev/sda"}, + sm.smartctlArgs(scanScsi, false), + ) + + // Explicit scsi from SMART_DEVICES: -d scsi must be passed. + explicitScsi := &DeviceInfo{Name: "/dev/sda", Type: "scsi", explicitType: true} + assert.Equal(t, + []string{"-d", "scsi", "-a", "--json=c", "/dev/sda"}, + sm.smartctlArgs(explicitScsi, false), + ) + + // Explicit ata from SMART_DEVICES: -d ata must be passed (devstat still added). + explicitAta := &DeviceInfo{Name: "/dev/sdb", Type: "ata", explicitType: true} + assert.Equal(t, + []string{"-d", "ata", "-a", "--json=c", "-l", "devstat", "/dev/sdb"}, + sm.smartctlArgs(explicitAta, false), + ) +} + +// TestSmartDevicesExplicitTypeFlowsToSmartctlArgs is a regression test for +// issue #2072: an explicit SMART_DEVICES type (e.g. /dev/sda:scsi) must win over +// a wrong scan-detected type (sat) and be handed to smartctl as -d scsi. +func TestSmartDevicesExplicitTypeFlowsToSmartctlArgs(t *testing.T) { + sm := &SmartManager{} + + configured, err := sm.parseConfiguredDevices("/dev/sda:scsi") + require.NoError(t, err) + require.Len(t, configured, 1) + assert.True(t, configured[0].explicitType) + + // smartctl --scan misreports this USB drive as sat, which fails on it. + scanned := []*DeviceInfo{ + {Name: "/dev/sda", Type: "sat", Protocol: "ATA"}, + } + + merged := mergeDeviceLists(nil, scanned, configured) + require.Len(t, merged, 1) + + device := merged[0] + assert.Equal(t, "scsi", device.Type, "configured type should win over scan-detected sat") + assert.True(t, device.explicitType, "explicit hint must survive the merge") + + assert.Equal(t, + []string{"-d", "scsi", "-a", "--json=c", "/dev/sda"}, + sm.smartctlArgs(device, false), + "explicit scsi type must be passed to smartctl, not dropped", + ) +} + +// TestMergeDeviceListsPreservesExplicitTypeAcrossRescan ensures a verified, +// explicitly-typed device keeps its explicit flag when a later scan re-reports +// it with a different auto-detected type. +func TestMergeDeviceListsPreservesExplicitTypeAcrossRescan(t *testing.T) { + existing := []*DeviceInfo{ + {Name: "/dev/sda", Type: "scsi", parserType: "scsi", typeVerified: true, explicitType: true}, + } + scanned := []*DeviceInfo{ + {Name: "/dev/sda", Type: "sat"}, + } + + merged := mergeDeviceLists(existing, scanned, nil) + require.Len(t, merged, 1) + assert.Equal(t, "scsi", merged[0].Type) + assert.True(t, merged[0].explicitType, "explicit type flag should survive a rescan") +} + func TestResolveRefreshError(t *testing.T) { scanErr := errors.New("scan failed") collectErr := errors.New("collect failed")