fix(sku): fix v5 SKU upgrade regressions that wedged pre-existing machines

crates/api-core/src/tests/sku.rs

@@ -150,7 +150,10 @@ pub mod tests {
           "storage": [
             {
               "model": "Dell Ent NVMe CM6 RI 1.92TB",
-              "count": 1
+              "count": 1,
+              "min_size_mb": 1800000,
+              "max_size_mb": 2000000,
+              "pci_patterns": ["/devices/pci0000:00/0000:64:00.0/nvme/nvme0/nvme0n1"]
             }
           ],
           "tpm":

crates/api-db/src/sku.rs

@@ -26,8 +26,9 @@ use model::machine::Machine;
 use model::machine::capabilities::{MachineCapabilitiesSet, MachineCapabilityInfiniband};
 use model::machine::machine_search_config::MachineSearchConfig;
 use model::sku::{
-    Sku, SkuComponentChassis, SkuComponentCpu, SkuComponentGpu, SkuComponentInfinibandDevices,
-    SkuComponentMemory, SkuComponentStorage, SkuComponentTpm, SkuComponents, diff_skus,
+    SKU_VERSION_WITH_DRIVE_LOCATION, Sku, SkuComponentChassis, SkuComponentCpu, SkuComponentGpu,
+    SkuComponentInfinibandDevices, SkuComponentMemory, SkuComponentStorage, SkuComponentTpm,
+    SkuComponents, diff_skus,
 };
 use sqlx::PgConnection;
 
@@ -80,10 +81,16 @@ pub async fn find_matching_with_exclusion(
     Ok(None)
 }
 
-/// Reject a SKU whose storage components carry an uncompilable PCI location
-/// pattern, so an invalid regex is caught at authoring time rather than at
-/// validation time.
-fn validate_storage_pci_patterns(sku: &Sku) -> Result<(), DatabaseError> {
+/// Validate storage components of an expected SKU being persisted.
+///
+/// Rejects uncompilable PCI patterns (caught at authoring time rather than at
+/// validation time), and rejects v5 storage entries that carry no constraints
+/// at all (no size bounds, no PCI patterns). Such entries would silently accept
+/// any drive at any location, providing weaker guarantees than v4 model
+/// matching. This most commonly occurs when a v5 SKU is auto-generated from
+/// hardware_info that predates the size_mb/pci_path fields; those SKUs should
+/// not be persisted until the hardware has been re-enumerated.
+fn validate_storage_for_create(sku: &Sku) -> Result<(), DatabaseError> {
     for storage in &sku.components.storage {
         for pattern in &storage.pci_patterns {
             regex::Regex::new(pattern).map_err(|err| {
@@ -92,6 +99,25 @@ fn validate_storage_pci_patterns(sku: &Sku) -> Result<(), DatabaseError> {
                 ))
             })?;
         }
+        if let (Some(min), Some(max)) = (storage.min_size_mb, storage.max_size_mb)
+            && min > max
+        {
+            return Err(DatabaseError::InvalidArgument(format!(
+                "storage entry (model {:?}) has min_size_mb ({min}) greater than max_size_mb ({max})",
+                storage.model
+            )));
+        }
+        if sku.schema_version >= SKU_VERSION_WITH_DRIVE_LOCATION
+            && storage.pci_patterns.is_empty()
+            && storage.min_size_mb.is_none()
+            && storage.max_size_mb.is_none()
+        {
+            return Err(DatabaseError::InvalidArgument(format!(
+                "v5 storage entry (model {:?}) has no size bounds or PCI patterns; \
+                 re-enumerate the machine's hardware before creating this SKU",
+                storage.model
+            )));
+        }
     }
     Ok(())
 }
@@ -110,7 +136,7 @@ pub async fn create(txn: &mut PgConnection, sku: &Sku) -> Result<(), DatabaseErr
         ));
     }
 
-    validate_storage_pci_patterns(sku)?;
+    validate_storage_for_create(sku)?;
 
     let mut inner_txn = Transaction::begin_inner(txn).await?;
 
@@ -241,7 +267,7 @@ pub async fn replace(txn: &mut PgConnection, sku: &Sku) -> Result<Sku, DatabaseE
         ));
     }
 
-    validate_storage_pci_patterns(sku)?;
+    validate_storage_for_create(sku)?;
 
     let mut inner_txn = Transaction::begin_inner(txn).await?;
 
@@ -776,29 +802,28 @@ pub async fn generate_sku_from_machine_at_version_5(
     // authored from this can then widen the size range or replace the literal
     // path with a regex. Drives are ordered by path for deterministic output.
     //
-    // Both fields are required: a missing size or PCI path would produce an
-    // unconstrained storage entry that validates any drive at any location or
-    // capacity. Since a generated SKU can be persisted as an expected SKU,
-    // reject generation rather than silently authoring a permissive v5 SKU.
+    // size_mb and pci_path may be absent on hardware_info records that predate
+    // the v5 fields (discovered before PR #3717). Rather than failing generation
+    // and wedging the machine, we include the drive with whatever fields are
+    // present. A drive without a path will not match any location-constrained
+    // expected group (which is correct — the mismatch is reported as a diff),
+    // and a drive without a size satisfies only unconstrained size ranges. The
+    // machine self-heals once hardware re-enumeration populates the new fields.
     let mut storage: Vec<SkuComponentStorage> = hardware_info
         .nvme_devices
         .iter()
-        .map(|nvme| {
-            let (Some(size_mb), Some(pci_path)) = (nvme.size_mb, nvme.pci_path.as_ref()) else {
-                return Err(DatabaseError::InvalidArgument(format!(
-                    "generate sku (v5): nvme drive (model {:?}, serial {:?}) is missing size or PCI path",
-                    nvme.model, nvme.serial
-                )));
-            };
-            Ok(SkuComponentStorage {
-                model: nvme.model.clone(),
-                count: 1,
-                min_size_mb: Some(size_mb),
-                max_size_mb: Some(size_mb),
-                pci_patterns: vec![pci_path.clone()],
-            })
+        .map(|nvme| SkuComponentStorage {
+            model: nvme.model.clone(),
+            count: 1,
+            min_size_mb: nvme.size_mb,
+            max_size_mb: nvme.size_mb,
+            pci_patterns: nvme
+                .pci_path
+                .as_ref()
+                .map(|p| vec![p.clone()])
+                .unwrap_or_default(),
         })
-        .collect::<Result<_, _>>()?;
+        .collect();
     storage.sort();
     sku.components.storage = storage;
 

crates/api-model/src/sku.rs

@@ -403,12 +403,15 @@ pub fn diff_skus(actual_sku: &Sku, expected_sku: &Sku) -> Vec<String> {
 /// Legacy (schema version < 5) storage comparison: match discovered storage to
 /// expected storage by model and compare counts.
 fn diff_storage_by_model(actual_sku: &Sku, expected_sku: &Sku, diffs: &mut Vec<String>) {
-    let mut actual_storage: HashMap<String, SkuComponentStorage> = actual_sku
-        .components
-        .storage
-        .iter()
-        .map(|s| (s.model.clone(), s.clone()))
-        .collect();
+    // v5 actual SKUs have one entry per drive; aggregate by model so the count
+    // comparison against a v4 expected SKU (grouped by model) is correct.
+    let mut actual_storage: HashMap<String, SkuComponentStorage> = HashMap::new();
+    for s in &actual_sku.components.storage {
+        actual_storage
+            .entry(s.model.clone())
+            .and_modify(|e| e.count += s.count)
+            .or_insert_with(|| s.clone());
+    }
 
     for es in &expected_sku.components.storage {
         if let Some(actual_storage) = actual_storage.remove(&es.model) {
@@ -973,4 +976,21 @@ mod tests {
             "got {diffs:?}"
         );
     }
+
+    #[test]
+    fn v5_actual_matches_v4_expected_by_model() {
+        // A v5 actual SKU (one entry per drive, count=1 each) must match a v4
+        // expected SKU (grouped by model) correctly. The regression: HashMap
+        // collection clobbered duplicate models, leaving count=1 instead of 2.
+        let actual_v5 = vec![drive(PATH_A, 3_840_000), drive(PATH_B, 3_840_000)];
+        let expected_v4 = vec![SkuComponentStorage {
+            model: "nvme".to_string(),
+            count: 2,
+            min_size_mb: None,
+            max_size_mb: None,
+            pci_patterns: Vec::new(),
+        }];
+        let diffs = diff_skus(&sku(5, actual_v5), &sku(4, expected_v4));
+        assert!(diffs.is_empty(), "expected no diffs, got {diffs:?}");
+    }
 }