diff --git a/hive-c0re/src/stores/power.rs b/hive-c0re/src/stores/power.rs index e34beb3d..d19fbbeb 100644 --- a/hive-c0re/src/stores/power.rs +++ b/hive-c0re/src/stores/power.rs @@ -109,6 +109,13 @@ impl PowerStore { /// Read an agent's intent. `None` when the agent has no row yet /// (callers seed from observed state via [`Self::get_or_seed`]). + /// + /// A row that exists but holds an unparseable value is an **error**, not + /// `None`. The two used to collapse, and the damage was not that a caller + /// read the wrong value: [`Self::get_or_seed`] takes `None` to mean "never + /// seeded" and writes what the container is *currently doing*, so a + /// corrupted row silently overwrote itself with the observed state this + /// store exists to overrule. pub fn get(&self, agent: &str) -> Result> { let conn = self.conn.lock().expect("agent_power mutex poisoned"); let row: Option = conn @@ -119,7 +126,12 @@ impl PowerStore { ) .optional() .context("select agent_power")?; - Ok(row.and_then(|s| Wanted::parse(&s))) + match row { + None => Ok(None), + Some(stored) => Wanted::parse(&stored).map(Some).with_context(|| { + format!("agent_power row for {agent} holds an unknown intent {stored:?}") + }), + } } /// Write an agent's intent (last-writer-wins, synchronous at @@ -178,6 +190,60 @@ mod tests { ); } + /// Write a value `set` cannot produce, the way corruption would. + fn write_raw(store: &PowerStore, agent: &str, wanted: &str) { + let conn = store.conn.lock().expect("agent_power mutex poisoned"); + conn.execute( + "INSERT INTO agent_power (agent, wanted, updated_at) VALUES (?1, ?2, 0)", + params![agent, wanted], + ) + .expect("insert raw row"); + } + + /// A row that exists but does not parse is an error, not an absent one. + /// The two rows beside it are the control: this must not pass by breaking + /// `get` for everything. + #[test] + fn an_unparseable_row_is_an_error_not_an_absent_one() { + let store = PowerStore::open_in_memory().expect("open"); + store.set("valid", Wanted::Up).expect("set"); + write_raw(&store, "corrupt", "sideways"); + + assert_eq!(store.get("valid").expect("valid reads"), Some(Wanted::Up)); + assert_eq!(store.get("missing").expect("absent reads"), None); + + let err = store + .get("corrupt") + .expect_err("corrupt must not read as absent"); + assert!( + format!("{err:#}").contains("sideways"), + "the error should name the offending value: {err:#}" + ); + } + + /// The damage the error arm exists to prevent: `get_or_seed` treats + /// `None` as "never seeded" and writes observed state, so a corrupt row + /// used to overwrite itself with whatever the container was doing. + #[test] + fn a_corrupt_row_is_not_reseeded_from_observed_state() { + let store = PowerStore::open_in_memory().expect("open"); + write_raw(&store, "corrupt", "sideways"); + + assert!(store.get_or_seed("corrupt", true).is_err()); + + let stored: String = store + .conn + .lock() + .expect("agent_power mutex poisoned") + .query_row( + "SELECT wanted FROM agent_power WHERE agent = ?1", + params!["corrupt"], + |r| r.get(0), + ) + .expect("row still there"); + assert_eq!(stored, "sideways", "the corrupt row must be left alone"); + } + #[test] fn get_set_roundtrip_and_seed() { let store = PowerStore::open_in_memory().expect("open");