fix: stop reporting restarts that never happened

.gitignore

@@ -2,4 +2,7 @@ target
 ring.db
 ring.db-shm
 ring.db-wal
-.config
+.config
+# Packer build artifacts: serial console dumps from the CH image build.
+# Regenerated on every build, hundreds of KB, no value in history.
+tests/e2e/cloud-hypervisor/packer/*.log

src/hypervisor/classifier.rs

@@ -166,6 +166,27 @@ pub(crate) fn classify_vm_start_error(
 /// polled by the reconcile loop, so setting the status without pushing the
 /// counter to the bound would retry a permanent failure on every tick forever —
 /// exactly the loop this classifier exists to stop.
+/// True when the scheduler already refuses to reconcile a deployment in this
+/// status, so no restart-budget marker is needed to stop the retries.
+///
+/// Derived from [`RECONCILED_STATUSES`] rather than listed by hand: the two
+/// must stay exact complements. A status ADDED to the reconcile filter while
+/// still reported as skipped here would be retried forever, with no budget to
+/// stop it — which is why this reads the same array the scheduler queries with
+/// instead of duplicating it.
+pub(crate) fn scheduler_skips_by_status(status: &DeploymentStatus) -> bool {
+    !crate::models::deployments::RECONCILED_STATUSES.contains(status)
+}
+
+/// Apply a VM start failure to a deployment: record the event, set the status,
+/// and decide what happens to the restart budget.
+///
+/// A transient failure bumps `restart_count` by one and retries within the
+/// budget. A terminal failure lands on its status immediately — and exhausts
+/// the budget only when the scheduler would otherwise keep reconciling that
+/// status. For the statuses it already skips, the counter is left alone: the
+/// deployment never restarted, and reporting that it did sends whoever reads
+/// the field looking for an instability that never happened.
 pub(crate) fn apply_vm_start_failure(
     deployment: &mut crate::models::deployments::Deployment,
     err: &RuntimeError,
@@ -179,7 +200,19 @@ pub(crate) fn apply_vm_start_failure(
 
     match status {
         Some(terminal) => {
-            deployment.restart_count = MAX_RESTART_COUNT;
+            // Exhausting the restart budget is how a terminal error stops the
+            // scheduler from retrying — but only for statuses the scheduler
+            // still reconciles (`ConfigError`, `ImagePullBackOff`,
+            // `CreateContainerError`, ...). A few terminal statuses are already
+            // excluded from its filter by status alone; for those the marker
+            // buys nothing and makes the field lie, so it is skipped.
+            //
+            // Concretely: a deployment refused by host-memory admission used to
+            // report 5 restarts having never started a single process, sending
+            // whoever read it looking for an instability that never existed.
+            if !scheduler_skips_by_status(&terminal) {
+                deployment.restart_count = MAX_RESTART_COUNT;
+            }
             deployment.status = terminal;
         }
         None => {
@@ -246,6 +279,76 @@ mod tests {
         );
     }
 
+    /// A deployment refused before anything ran must not claim restarts it
+    /// never made. `restart_count` is displayed by the CLI, the API and the
+    /// dashboard as a count of actual restarts; reporting 5 for a workload that
+    /// never started a process sends an operator hunting a phantom instability.
+    #[test]
+    fn admission_refusal_does_not_invent_restarts() {
+        let mut d = vm_deployment();
+        assert_eq!(d.restart_count, 0);
+
+        apply_vm_start_failure(
+            &mut d,
+            &RuntimeError::InsufficientResources("needs 4096 MiB but only 1800 MiB".into()),
+            "firecracker",
+            DeploymentStatus::CrashLoopBackOff,
+        );
+
+        assert_eq!(d.status, DeploymentStatus::InsufficientResources);
+        assert_eq!(
+            d.restart_count, 0,
+            "no VM was ever spawned, so no restart may be reported"
+        );
+    }
+
+    /// The counterpart: statuses the scheduler DOES keep reconciling still need
+    /// the exhausted budget, otherwise they would be retried forever.
+    #[test]
+    fn a_reconciled_terminal_status_still_exhausts_the_budget() {
+        let mut d = vm_deployment();
+
+        apply_vm_start_failure(
+            &mut d,
+            &RuntimeError::ConfigNotFound("missing-config".into()),
+            "firecracker",
+            DeploymentStatus::CrashLoopBackOff,
+        );
+
+        assert_eq!(d.status, DeploymentStatus::ConfigError);
+        assert_eq!(
+            d.restart_count,
+            crate::models::deployments::MAX_RESTART_COUNT,
+            "ConfigError is still reconciled, so the budget is what stops the retries"
+        );
+    }
+
+    /// The two sides must be exact complements. This no longer duplicates the
+    /// scheduler's list — both derive from `RECONCILED_STATUSES` — so the test
+    /// checks the property rather than a copy that could go stale.
+    #[test]
+    fn skipped_and_reconciled_statuses_are_complements() {
+        use crate::models::deployments::RECONCILED_STATUSES;
+
+        for status in DeploymentStatus::all() {
+            assert_eq!(
+                scheduler_skips_by_status(&status),
+                !RECONCILED_STATUSES.contains(&status),
+                "{status:?} is inconsistent between the reconcile filter and the skip check"
+            );
+        }
+
+        // Sanity: neither side is empty, which would make the assertion above
+        // vacuously true.
+        assert!(!RECONCILED_STATUSES.is_empty());
+        assert!(
+            DeploymentStatus::all()
+                .iter()
+                .any(scheduler_skips_by_status),
+            "no status is skipped — the marker would always be written"
+        );
+    }
+
     #[test]
     fn insufficient_resources_is_terminal() {
         assert_eq!(
@@ -381,12 +484,16 @@ mod tests {
         }
     }
 
-    /// The invariant that stops the infinite reboot loop: whichever branch is
-    /// taken, `restart_count` must move. Several terminal statuses are still
-    /// polled by the reconcile loop, so a terminal verdict that left the counter
-    /// at zero would retry a permanent failure on every tick, forever.
+    /// The invariant that stops the infinite reboot loop: a failure must either
+    /// move `restart_count` or land on a status the scheduler refuses to
+    /// reconcile. Leaving BOTH untouched would retry a permanent failure on
+    /// every tick, forever.
+    ///
+    /// The counter is not required on its own: several terminal statuses are
+    /// already excluded from the reconcile filter, and marking those would only
+    /// report restarts that never happened.
     #[test]
-    fn every_start_failure_moves_the_restart_counter() {
+    fn every_start_failure_either_counts_or_lands_outside_the_reconcile_filter() {
         let errors = [
             RuntimeError::FirmwareNotFound("f".into()),
             RuntimeError::ImageNotFound("i".into()),
@@ -404,17 +511,23 @@ mod tests {
                 DeploymentStatus::CrashLoopBackOff,
             );
             assert!(
-                deployment.restart_count > 0,
-                "counter stayed at zero for {:?} — the deployment would retry forever",
-                err
+                deployment.restart_count > 0 || scheduler_skips_by_status(&deployment.status),
+                "{:?} left the counter at zero AND landed on {:?}, which the scheduler \
+                 still reconciles — the deployment would retry forever",
+                err,
+                deployment.status
             );
         }
     }
 
-    /// A permanent failure lands on its status and exhausts the budget at once,
-    /// so the very next tick is terminal instead of the fifth.
+    /// A permanent failure lands on its terminal status immediately, so the very
+    /// next tick is terminal instead of the fifth.
+    ///
+    /// `Failed` is outside the scheduler's reconcile filter, so the status alone
+    /// stops the retries and the restart budget is left untouched — the
+    /// deployment never restarted, and must not claim it did.
     #[test]
-    fn terminal_start_failure_jumps_to_the_bound() {
+    fn terminal_start_failure_lands_on_its_status_at_once() {
         let mut deployment = vm_deployment();
         apply_vm_start_failure(
             &mut deployment,
@@ -423,11 +536,11 @@ mod tests {
             DeploymentStatus::CrashLoopBackOff,
         );
 
+        assert_eq!(deployment.status, DeploymentStatus::Failed);
         assert_eq!(
-            deployment.restart_count,
-            crate::models::deployments::MAX_RESTART_COUNT
+            deployment.restart_count, 0,
+            "the VM never started, so no restart may be reported"
         );
-        assert_eq!(deployment.status, DeploymentStatus::Failed);
     }
 
     /// A transient failure keeps its one-per-tick budget and converges to the

src/models/deployments.rs

@@ -7,6 +7,26 @@ use std::str::FromStr;
 
 pub(crate) const MAX_RESTART_COUNT: u32 = 5;
 
+/// Statuses the scheduler keeps reconciling on every tick.
+///
+/// Single source of truth: `scheduler::schedule` builds its DB filter from this
+/// list, and `classifier::scheduler_skips_by_status` is its complement. Keeping
+/// them derived from one array is what stops the two drifting apart — a status
+/// added here while still reported as "skipped" would be retried forever with
+/// no restart budget to stop it.
+pub(crate) const RECONCILED_STATUSES: &[DeploymentStatus] = &[
+    DeploymentStatus::Pending,
+    DeploymentStatus::Creating,
+    DeploymentStatus::Running,
+    DeploymentStatus::Deleted,
+    DeploymentStatus::CreateContainerError,
+    DeploymentStatus::ImagePullBackOff,
+    DeploymentStatus::NetworkError,
+    DeploymentStatus::ConfigError,
+    DeploymentStatus::FileSystemError,
+    DeploymentStatus::Error,
+];
+
 /// All variants serialize to snake_case, both on the wire (serde JSON) and
 /// in the SQLite `deployment.status` column (Display). Before this change,
 /// lifecycle states were lowercase (`running`, …) while error states were

src/runtime/containerd/lifecycle.rs

@@ -854,7 +854,21 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment
     // do recover, so this runtime keeps them inside the retry budget.
     let terminal = crate::hypervisor::classifier::classify_create_error(&err).is_terminal()
         && !matches!(err, RuntimeError::InstanceCreationFailed(_));
-    if increment_restart {
+    // `InsufficientResources` is excluded from the scheduler's reconcile filter
+    // by status alone, so it needs no counter marker — and writing one would
+    // report restarts that never happened (no container was ever created). The
+    // other terminal statuses (`ImagePullBackOff`, `CreateContainerError`) ARE
+    // still reconciled, so for those the exhausted budget remains what stops
+    // the retries.
+    let status_alone_stops_reconciliation =
+        match crate::hypervisor::classifier::classify_create_error(&err) {
+            crate::hypervisor::classifier::Disposition::Terminal(ref s) => {
+                crate::hypervisor::classifier::scheduler_skips_by_status(s)
+            }
+            crate::hypervisor::classifier::Disposition::Retry => false,
+        };
+
+    if increment_restart && !status_alone_stops_reconciliation {
         if terminal {
             deployment.restart_count = MAX_RESTART_COUNT;
         } else {

src/runtime/docker/lifecycle.rs

@@ -179,8 +179,21 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment
     // the restart bound: the deployment converges to its terminal state on the
     // next tick instead of five ticks from now. Transient errors still bump by
     // one and retry within the budget, exactly as before.
-    let terminal = crate::hypervisor::classifier::classify_create_error(&err).is_terminal();
-    if increment_restart {
+    //
+    // One exception: a few terminal statuses are already excluded from the
+    // scheduler's reconcile filter, so the budget marker buys nothing there and
+    // would report restarts that never happened — a host-memory refusal is
+    // checked before the image pull, so nothing was ever created.
+    let disposition = crate::hypervisor::classifier::classify_create_error(&err);
+    let terminal = disposition.is_terminal();
+    let marker_needed = match &disposition {
+        crate::hypervisor::classifier::Disposition::Terminal(status) => {
+            !crate::hypervisor::classifier::scheduler_skips_by_status(status)
+        }
+        crate::hypervisor::classifier::Disposition::Retry => true,
+    };
+
+    if increment_restart && marker_needed {
         if terminal {
             deployment.restart_count = MAX_RESTART_COUNT;
         } else {

src/scheduler/scheduler.rs

@@ -1439,18 +1439,10 @@ pub(crate) async fn schedule(
         let mut filters = HashMap::new();
         filters.insert(
             String::from("status"),
-            vec![
-                DeploymentStatus::Pending.to_string(),
-                DeploymentStatus::Creating.to_string(),
-                DeploymentStatus::Running.to_string(),
-                DeploymentStatus::Deleted.to_string(),
-                DeploymentStatus::CreateContainerError.to_string(),
-                DeploymentStatus::ImagePullBackOff.to_string(),
-                DeploymentStatus::NetworkError.to_string(),
-                DeploymentStatus::ConfigError.to_string(),
-                DeploymentStatus::FileSystemError.to_string(),
-                DeploymentStatus::Error.to_string(),
-            ],
+            crate::models::deployments::RECONCILED_STATUSES
+                .iter()
+                .map(|s| s.to_string())
+                .collect(),
         );
         let mut list_deployments = match deployments::find_all(&pool, filters).await {
             Ok(list) => list,

tests/e2e/firecracker/t16_insufficient_memory.sh

@@ -80,16 +80,13 @@ log "no VM was spawned and no rootfs was copied"
 # Terminal, not transient: the RAM is not coming back, so retrying would only
 # spam events.
 #
-# `restart_count` reads 5 here, which does NOT mean five boot attempts were
-# made: `apply_vm_start_failure` assigns MAX_RESTART_COUNT outright for any
-# terminal VM-start error (src/hypervisor/classifier.rs), so the counter is
-# used as a "do not reconcile again" marker rather than a tally. Cloud
-# Hypervisor goes through the same function, so both runtimes behave alike —
-# t23's "restart_count must stay 0" comment is simply stale.
+# `restart_count` stays 0: the admission check refused the boot before any
+# process existed, so there is nothing to report. It used to read 5 — the
+# terminal path assigned MAX_RESTART_COUNT as a "stop reconciling" marker,
+# which was redundant (the scheduler already skips this status) and made the
+# field claim restarts that never happened.
 #
-# What matters for this test is that the count is FROZEN, i.e. nothing is
-# retrying underneath. Asserting the exact value would pin an implementation
-# detail of that marker.
+# What matters here is that the count is FROZEN: nothing is retrying underneath.
 COUNT_ONE=$("$RING_BIN" deployment inspect "$DEPLOYMENT_ID" --output json 2>/dev/null | jq -r '.restart_count // 0')
 sleep 12
 COUNT_TWO=$("$RING_BIN" deployment inspect "$DEPLOYMENT_ID" --output json 2>/dev/null | jq -r '.restart_count // 0')