[reconfigurator] Fix race in chicken switch loader (#8998)

jgallagher · web-flow · commit 1ee118395ed9 · 2025-09-05T10:44:21.000-04:00
The chicken switch loading task exposes a watch channel that downstream consumers can use to read the current value. It initializes that channel to the `::default()` value of chicken switches and then reads the most recent value from the db when it's activated, but that introduces a race where consumers can see an incorrect value: if there is a chicken switch value in the db, they can see the `::default()` _before_ the first activation, allowing them to ignore the actual value in the db in favor of whatever the default set is. This PR closes the race window by setting the initial watch channel value to `NotYetLoaded`, and then replacing that during the first activation. This fell out of trying to fix up tests on #8980, but is a legit bug and easily separable from that work. So here it is!
diff --git a/nexus/src/app/background/tasks/blueprint_planner.rs b/nexus/src/app/background/tasks/blueprint_planner.rs
@@ -4,6 +4,7 @@
 
 //! Background task for automatic update planning.
 
+use super::chicken_switches::ReconfiguratorChickenSwitchesLoaderState;
 use crate::app::background::BackgroundTask;
 use chrono::Utc;
 use futures::future::BoxFuture;
@@ -13,7 +14,6 @@ use nexus_db_queries::db::DataStore;
 use nexus_reconfigurator_planning::planner::Planner;
 use nexus_reconfigurator_planning::planner::PlannerRng;
 use nexus_reconfigurator_preparation::PlanningInputFromDb;
-use nexus_types::deployment::ReconfiguratorChickenSwitchesView;
 use nexus_types::deployment::{Blueprint, BlueprintTarget};
 use nexus_types::internal_api::background::BlueprintPlannerStatus;
 use omicron_common::api::external::LookupType;
@@ -26,7 +26,7 @@ use tokio::sync::watch::{self, Receiver, Sender};
 /// Background task that runs the update planner.
 pub struct BlueprintPlanner {
     datastore: Arc<DataStore>,
-    rx_chicken_switches: Receiver<ReconfiguratorChickenSwitchesView>,
+    rx_chicken_switches: Receiver<ReconfiguratorChickenSwitchesLoaderState>,
     rx_inventory: Receiver<Option<CollectionUuid>>,
     rx_blueprint: Receiver<Option<Arc<(BlueprintTarget, Blueprint)>>>,
     tx_blueprint: Sender<Option<Arc<(BlueprintTarget, Blueprint)>>>,
@@ -35,7 +35,7 @@ pub struct BlueprintPlanner {
 impl BlueprintPlanner {
     pub fn new(
         datastore: Arc<DataStore>,
-        rx_chicken_switches: Receiver<ReconfiguratorChickenSwitchesView>,
+        rx_chicken_switches: Receiver<ReconfiguratorChickenSwitchesLoaderState>,
         rx_inventory: Receiver<Option<CollectionUuid>>,
         rx_blueprint: Receiver<Option<Arc<(BlueprintTarget, Blueprint)>>>,
     ) -> Self {
@@ -59,7 +59,21 @@ impl BlueprintPlanner {
     /// If it is different from the current target blueprint,
     /// save it and make it the current target.
     pub async fn plan(&mut self, opctx: &OpContext) -> BlueprintPlannerStatus {
-        let switches = self.rx_chicken_switches.borrow_and_update().clone();
+        // Refuse to run if we haven't had a chance to load the chicken switches
+        // from the database yet. (There might not be any in the db, which is
+        // fine! But the loading task needs to have a chance to check.)
+        let switches = match &*self.rx_chicken_switches.borrow_and_update() {
+            ReconfiguratorChickenSwitchesLoaderState::NotYetLoaded => {
+                debug!(
+                    opctx.log,
+                    "chicken switches not yet loaded; doing nothing"
+                );
+                return BlueprintPlannerStatus::Disabled;
+            }
+            ReconfiguratorChickenSwitchesLoaderState::Loaded(switches) => {
+                switches.clone()
+            }
+        };
         if !switches.switches.planner_enabled {
             debug!(&opctx.log, "blueprint planning disabled, doing nothing");
             return BlueprintPlannerStatus::Disabled;
@@ -281,7 +295,7 @@ mod test {
     use nexus_test_utils_macros::nexus_test;
     use nexus_types::deployment::{
         PendingMgsUpdates, PlannerChickenSwitches,
-        ReconfiguratorChickenSwitches,
+        ReconfiguratorChickenSwitches, ReconfiguratorChickenSwitchesView,
     };
     use omicron_uuid_kinds::OmicronZoneUuid;
 
@@ -326,14 +340,16 @@ mod test {
 
         // Enable the planner
         let (_tx, chicken_switches_collector_rx) =
-            watch::channel(ReconfiguratorChickenSwitchesView {
-                version: 1,
-                switches: ReconfiguratorChickenSwitches {
-                    planner_enabled: true,
-                    planner_switches: PlannerChickenSwitches::default(),
+            watch::channel(ReconfiguratorChickenSwitchesLoaderState::Loaded(
+                ReconfiguratorChickenSwitchesView {
+                    version: 1,
+                    switches: ReconfiguratorChickenSwitches {
+                        planner_enabled: true,
+                        planner_switches: PlannerChickenSwitches::default(),
+                    },
+                    time_modified: now_db_precision(),
                 },
-                time_modified: now_db_precision(),
-            });
+            ));
 
         // Finally, spin up the planner background task.
         let mut planner = BlueprintPlanner::new(
diff --git a/nexus/src/app/background/tasks/chicken_switches.rs b/nexus/src/app/background/tasks/chicken_switches.rs
@@ -15,24 +15,32 @@ use serde_json::json;
 use std::sync::Arc;
 use tokio::sync::watch;
 
+/// Enum that allows downstream tasks to know whether this task has had a chance
+/// to read the current chicken switches from the database.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum ReconfiguratorChickenSwitchesLoaderState {
+    NotYetLoaded,
+    Loaded(ReconfiguratorChickenSwitchesView),
+}
+
 /// Background task that tracks reconfigurator chicken switches from the DB
 pub struct ChickenSwitchesLoader {
     datastore: Arc<DataStore>,
-    tx: watch::Sender<ReconfiguratorChickenSwitchesView>,
-    rx: watch::Receiver<ReconfiguratorChickenSwitchesView>,
+    tx: watch::Sender<ReconfiguratorChickenSwitchesLoaderState>,
 }
 
 impl ChickenSwitchesLoader {
     pub fn new(datastore: Arc<DataStore>) -> Self {
-        let (tx, rx) =
-            watch::channel(ReconfiguratorChickenSwitchesView::default());
-        Self { datastore, tx, rx }
+        let (tx, _rx) = watch::channel(
+            ReconfiguratorChickenSwitchesLoaderState::NotYetLoaded,
+        );
+        Self { datastore, tx }
     }
 
     pub fn watcher(
         &self,
-    ) -> watch::Receiver<ReconfiguratorChickenSwitchesView> {
-        self.rx.clone()
+    ) -> watch::Receiver<ReconfiguratorChickenSwitchesLoaderState> {
+        self.tx.subscribe()
     }
 }
 
@@ -55,15 +63,21 @@ impl BackgroundTask for ChickenSwitchesLoader {
                     json!({ "error": message })
                 }
                 Ok(switches) => {
-                    let switches = switches.unwrap_or_default();
+                    let switches =
+                        ReconfiguratorChickenSwitchesLoaderState::Loaded(
+                            switches.unwrap_or_default(),
+                        );
                     let updated = self.tx.send_if_modified(|s| {
                         if *s != switches {
-                            *s = switches;
+                            *s = switches.clone();
                             return true;
                         }
                         false
                     });
-                    debug!(opctx.log, "chicken switches load complete");
+                    debug!(
+                        opctx.log, "chicken switches load complete";
+                        "switches" => ?switches,
+                    );
                     json!({ "chicken_switches_updated": updated })
                 }
             }
@@ -94,14 +108,35 @@ mod test {
         );
 
         let mut task = ChickenSwitchesLoader::new(datastore.clone());
+
+        // Initial state should be `NotYetLoaded`.
+        let mut rx = task.watcher();
+        assert_eq!(
+            *rx.borrow_and_update(),
+            ReconfiguratorChickenSwitchesLoaderState::NotYetLoaded
+        );
+
+        // We haven't inserted anything into the DB, so the initial activation
+        // should populate the channel with our default values.
+        let default_switches = ReconfiguratorChickenSwitchesView::default();
         let out = task.activate(&opctx).await;
-        assert_eq!(out["chicken_switches_updated"], false);
+        assert_eq!(out["chicken_switches_updated"], true);
+        assert!(rx.has_changed().unwrap());
+        assert_eq!(
+            *rx.borrow_and_update(),
+            ReconfiguratorChickenSwitchesLoaderState::Loaded(
+                default_switches.clone()
+            )
+        );
+
+        // Insert an initial set of switches.
+        let expected_switches = ReconfiguratorChickenSwitches {
+            planner_enabled: !default_switches.switches.planner_enabled,
+            planner_switches: PlannerChickenSwitches::default(),
+        };
         let switches = ReconfiguratorChickenSwitchesParam {
             version: 1,
-            switches: ReconfiguratorChickenSwitches {
-                planner_enabled: true,
-                planner_switches: PlannerChickenSwitches::default(),
-            },
+            switches: expected_switches,
         };
         datastore
             .reconfigurator_chicken_switches_insert_latest_version(
@@ -111,14 +146,31 @@ mod test {
             .unwrap();
         let out = task.activate(&opctx).await;
         assert_eq!(out["chicken_switches_updated"], true);
+        assert!(rx.has_changed().unwrap());
+        {
+            let view = match rx.borrow_and_update().clone() {
+                ReconfiguratorChickenSwitchesLoaderState::NotYetLoaded => {
+                    panic!("unexpected value")
+                }
+                ReconfiguratorChickenSwitchesLoaderState::Loaded(view) => view,
+            };
+            assert_eq!(view.version, 1);
+            assert_eq!(view.switches, expected_switches);
+        }
+
+        // Activating again should not change things.
         let out = task.activate(&opctx).await;
         assert_eq!(out["chicken_switches_updated"], false);
+        assert!(!rx.has_changed().unwrap());
+
+        // Insert a new version.
+        let expected_switches = ReconfiguratorChickenSwitches {
+            planner_enabled: !expected_switches.planner_enabled,
+            planner_switches: PlannerChickenSwitches::default(),
+        };
         let switches = ReconfiguratorChickenSwitchesParam {
             version: 2,
-            switches: ReconfiguratorChickenSwitches {
-                planner_enabled: false,
-                planner_switches: PlannerChickenSwitches::default(),
-            },
+            switches: expected_switches,
         };
         datastore
             .reconfigurator_chicken_switches_insert_latest_version(
@@ -128,5 +180,16 @@ mod test {
             .unwrap();
         let out = task.activate(&opctx).await;
         assert_eq!(out["chicken_switches_updated"], true);
+        assert!(rx.has_changed().unwrap());
+        {
+            let view = match rx.borrow_and_update().clone() {
+                ReconfiguratorChickenSwitchesLoaderState::NotYetLoaded => {
+                    panic!("unexpected value")
+                }
+                ReconfiguratorChickenSwitchesLoaderState::Loaded(view) => view,
+            };
+            assert_eq!(view.version, 2);
+            assert_eq!(view.switches, expected_switches);
+        }
     }
 }
