Warning when SMART reports unreadable sectors. Parse Seagate drive hours correctly.

controller/lib/smart.py

@@ -2,6 +2,7 @@
 SMART disk health monitoring for WROLPi Controller.
 """
 
+import re
 import subprocess
 from typing import Optional
 
@@ -57,21 +58,47 @@ def get_all_smart_status() -> list[dict]:
 
 def _get_device_smart(device) -> dict:
     """Extract SMART data from a pySMART device."""
+    pending = _get_attribute(device, "Current_Pending_Sector")
+    reallocated = _get_attribute(device, "Reallocated_Sector_Ct")
+    uncorrectable = _get_attribute(device, "Offline_Uncorrectable")
     return {
         "device": device.name,
         "path": f"/dev/{device.name}",
         "model": device.model,
         "serial": device.serial,
         "capacity": device.capacity,
-        "assessment": device.assessment,  # PASS, FAIL, WARN
+        "assessment": device.assessment,  # PASS, FAIL, WARN (pySMART's view)
+        "health": _derive_health(device.assessment, pending, uncorrectable),
         "temperature": _get_temperature(device),
         "power_on_hours": _get_attribute(device, "Power_On_Hours"),
-        "reallocated_sectors": _get_attribute(device, "Reallocated_Sector_Ct"),
-        "pending_sectors": _get_attribute(device, "Current_Pending_Sector"),
+        "reallocated_sectors": reallocated,
+        "pending_sectors": pending,
+        "offline_uncorrectable": uncorrectable,
         "smart_enabled": device.smart_enabled,
     }
 
 
+def _derive_health(assessment, pending_sectors, offline_uncorrectable) -> Optional[str]:
+    """Derive the overall health verdict shown in the navbar and UIs.
+
+    pySMART's ``assessment`` only reflects the drive's own pass/fail
+    self-assessment, which is driven by attributes flagged FAILING_NOW.  A
+    drive can be quietly losing data to unreadable sectors while still
+    self-assessing PASS, so we downgrade to WARN when sectors are pending
+    reallocation or already uncorrectable.
+
+    ``Reallocated_Sector_Ct`` is intentionally NOT a trigger: a small,
+    stable reallocated count is common and benign, and warning on it would
+    cry wolf.  ``assessment`` of None (unsupported drive) passes through
+    unchanged.
+    """
+    if (assessment or "").upper() == "FAIL":
+        return "FAIL"
+    if (pending_sectors or 0) > 0 or (offline_uncorrectable or 0) > 0:
+        return "WARN"
+    return assessment
+
+
 def _get_temperature(device) -> Optional[int]:
     """Get drive temperature.
 
@@ -90,19 +117,34 @@ def _get_temperature(device) -> Optional[int]:
 
 
 def _get_attribute(device, name: str) -> Optional[int]:
-    """Get a specific SMART attribute value."""
+    """Get a specific SMART attribute value as an integer."""
     if not hasattr(device, 'attributes') or device.attributes is None:
         return None
 
     for attr in device.attributes:
         if attr and attr.name == name:
-            try:
-                return int(attr.raw)
-            except (ValueError, TypeError):
-                return None
+            return _parse_raw_int(attr.raw)
     return None
 
 
+def _parse_raw_int(raw) -> Optional[int]:
+    """Parse the integer value out of a SMART attribute's raw field.
+
+    smartctl/pySMART decorate some raw values in vendor-specific ways that
+    plain int() cannot handle: Seagate reports Power_On_Hours as
+    ``'50287h+00m+00.000s'`` and Temperature_Celsius as ``'53 (0 17 0 0 0)'``.
+    Extract the leading integer so these are not silently discarded.
+    """
+    try:
+        return int(raw)
+    except (ValueError, TypeError):
+        pass
+    if raw is None:
+        return None
+    match = re.match(r'\s*(-?\d+)', str(raw))
+    return int(match.group(1)) if match else None
+
+
 def _scan_devices() -> list[tuple[str, Optional[str]]]:
     """List SMART-capable devices via ``smartctl --scan``.
 

controller/templates/index.html

@@ -1785,8 +1785,8 @@
             let html = '<div class="health-grid">';
 
             for (const drive of drives) {
-                const assessmentClass = getHealthClass(drive.assessment);
-                const assessmentText = drive.assessment || 'Unknown';
+                const assessmentClass = getHealthClass(drive.health || drive.assessment);
+                const assessmentText = drive.health || drive.assessment || 'Unknown';
                 const tempText = drive.temperature !== null ? `${drive.temperature}°C` : '--';
 
                 html += `
@@ -1833,7 +1833,7 @@
 
         title.textContent = `${drive.device} - Health Details`;
 
-        const assessmentClass = getHealthClass(drive.assessment);
+        const assessmentClass = getHealthClass(drive.health || drive.assessment);
         const formatHours = (hours) => {
             if (hours === null || hours === undefined) return '--';
             const days = Math.floor(hours / 24);
@@ -1877,6 +1877,10 @@
                     <td style="color: #888;">Pending Sectors</td>
                     <td class="${drive.pending_sectors > 0 ? 'health-warn' : ''}">${drive.pending_sectors !== null ? drive.pending_sectors : '--'}</td>
                 </tr>
+                <tr>
+                    <td style="color: #888;">Uncorrectable Sectors</td>
+                    <td class="${drive.offline_uncorrectable > 0 ? 'health-warn' : ''}">${drive.offline_uncorrectable !== null && drive.offline_uncorrectable !== undefined ? drive.offline_uncorrectable : '--'}</td>
+                </tr>
                 <tr>
                     <td style="color: #888;">SMART Enabled</td>
                     <td>${drive.smart_enabled ? 'Yes' : 'No'}</td>

controller/test/test_smart.py

@@ -4,6 +4,8 @@ Unit tests for controller.lib.smart module.
 
 from unittest import mock
 
+import pytest
+
 from controller.lib.smart import (
     is_smart_available,
     get_all_smart_status,
@@ -11,12 +13,22 @@ from controller.lib.smart import (
     _get_device_smart,
     _get_temperature,
     _get_attribute,
+    _parse_raw_int,
+    _derive_health,
     _drive_is_asleep,
     HDD_HIGH_TEMPERATURE,
     HDD_CRITICAL_TEMPERATURE,
 )
 
 
+def _attr(name, raw):
+    a = mock.Mock()
+    a.name = name
+    a.raw = raw
+    a.when_failed = ''
+    return a
+
+
 class TestIsSmartAvailable:
     """Tests for is_smart_available function."""
 
@@ -90,8 +102,31 @@ class TestGetDeviceSmart:
         assert result["serial"] == "S1234"
         assert result["capacity"] == "500 GB"
         assert result["assessment"] == "PASS"
+        assert result["health"] == "PASS"
         assert result["smart_enabled"] is True
 
+    def test_warns_on_pending_sectors_despite_pass(self):
+        """A drive self-assessing PASS with pending sectors must surface a
+        WARN health while leaving the raw assessment untouched."""
+        mock_device = mock.Mock()
+        mock_device.name = "sdc"
+        mock_device.model = "WDC WD140EDGZ"
+        mock_device.serial = "9LJ0BM0G"
+        mock_device.capacity = "14.0 TB"
+        mock_device.assessment = "PASS"
+        mock_device.smart_enabled = True
+        mock_device.attributes = [
+            _attr("Current_Pending_Sector", "80"),
+            _attr("Offline_Uncorrectable", "51"),
+            _attr("Reallocated_Sector_Ct", "1"),
+        ]
+
+        result = _get_device_smart(mock_device)
+        assert result["assessment"] == "PASS"
+        assert result["health"] == "WARN"
+        assert result["pending_sectors"] == 80
+        assert result["offline_uncorrectable"] == 51
+
 
 class TestGetTemperature:
     """Tests for _get_temperature function."""
@@ -208,6 +243,48 @@ class TestGetAttribute:
         result = _get_attribute(mock_device, "Power_On_Hours")
         assert result is None
 
+    def test_parses_seagate_power_on_hours_format(self):
+        """Seagate reports Power_On_Hours as 'NNNNNh+00m+00.000s'; the leading
+        integer must be extracted rather than discarded to None."""
+        mock_device = mock.Mock()
+        mock_device.attributes = [_attr("Power_On_Hours", "50287h+00m+00.000s")]
+
+        result = _get_attribute(mock_device, "Power_On_Hours")
+        assert result == 50287
+
+
+class TestParseRawInt:
+    """Tests for _parse_raw_int — vendor-specific raw value parsing."""
+
+    @pytest.mark.parametrize("raw, expected", [
+        ("80", 80),                       # plain string integer
+        (22225, 22225),                   # already an int
+        ("50287h+00m+00.000s", 50287),    # Seagate Power_On_Hours
+        ("56 (Min/Max 17/66)", 56),       # WD temperature decoration
+        ("53 (0 17 0 0 0)", 53),          # Seagate temperature tuple
+        (None, None),                     # missing value
+        ("invalid", None),                # no leading integer
+    ])
+    def test_parse_raw_int(self, raw, expected):
+        assert _parse_raw_int(raw) == expected
+
+
+class TestDeriveHealth:
+    """Tests for _derive_health — the verdict shown in the navbar/UIs."""
+
+    @pytest.mark.parametrize("assessment, pending, uncorrectable, expected", [
+        ("PASS", 0, 0, "PASS"),       # clean drive stays PASS
+        ("FAIL", 0, 0, "FAIL"),       # self-assessed failure stays FAIL
+        ("PASS", 80, 0, "WARN"),      # pending sectors downgrade PASS -> WARN
+        ("PASS", 0, 51, "WARN"),      # uncorrectable sectors downgrade -> WARN
+        ("PASS", 0, 0, "PASS"),       # reallocated is not a trigger (clean here)
+        ("FAIL", 80, 51, "FAIL"),     # FAIL takes priority over sector WARN
+        (None, None, None, None),     # unsupported drive passes through unknown
+        (None, 5, 0, "WARN"),         # unknown assessment + pending -> WARN
+    ])
+    def test_derive_health(self, assessment, pending, uncorrectable, expected):
+        assert _derive_health(assessment, pending, uncorrectable) == expected
+
 
 class TestDriveIsAsleep:
     """Tests for _drive_is_asleep function (smartctl -n standby)."""