From a4924aee4db7f665944879f184002c7f7dd7f9ad Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 1 Sep 2026 14:05:30 +0200 Subject: [PATCH] =?UTF-8?q?fix(#3124):=20close=20AgentState=20=E2=80=94=20?= =?UTF-8?q?an=20unknown=20value=20is=20not=20a=20partial=20instruction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mara's call on the PR: "dont make the enum open, we will just add entries later". The catch-all variant is gone, and with it the per-agent inert path. What changes is where version skew lands, not whether it is handled. An unrecognised value used to be one agent this hive left alone; it is now a decode failure for the whole declaration, so a hive running older code converges *nothing* rather than obeying the agents it happened to understand. That fails closed instead of dangerous, and it is the right trade when both ends ship together — which is what "add entries later" assumes. The test moved with the property rather than being rewritten in place: `an_unknown_state_fails_the_whole_declaration` lives in swarm-queue-client, where the decode is, with a valid entry beside it as the control. `hive-c0re` keeps a coverage check that every state this build knows produces an action somewhere — asserting inertness there would be asserting something the type system no longer lets me build. --- hive-c0re/src/workers/wanted.rs | 44 +++++++++++-------------- swarm-queue-client/src/wanted.rs | 55 +++++++++++++------------------- 2 files changed, 41 insertions(+), 58 deletions(-) diff --git a/hive-c0re/src/workers/wanted.rs b/hive-c0re/src/workers/wanted.rs index 72fdcfc8..760a40fb 100644 --- a/hive-c0re/src/workers/wanted.rs +++ b/hive-c0re/src/workers/wanted.rs @@ -15,8 +15,9 @@ //! converging "to exactly this set" would tear down every agent the swarm has //! not adopted. //! -//! **An unrecognised state is inert**, so a controller that learns a new state -//! does not take agents down on hives that have not been updated yet. +//! **An unknown state is not a partial instruction.** [`AgentState`] is closed, +//! so a value this build cannot read fails the whole decode: the hive converges +//! nothing rather than obeying the agents it happened to understand. //! //! # What "diverged" is measured against //! @@ -25,9 +26,8 @@ //! reconcile's work; a loop reading `is_running` would insert a start DAG //! behind that reconcile's back on every boot. //! -//! One consequence, deliberate: a declaration outranks a local stop. An agent -//! this hive is declared to keep `Up` comes back at the next boot, because the -//! controller owns that decision. +//! One consequence, deliberate: a declaration outranks a local stop — an agent +//! declared `Up` returns at the next boot, because the controller owns that. use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; @@ -176,12 +176,6 @@ fn plan( Converge::Start => plan.start.push(agent.clone()), Converge::Stop => plan.stop.push(agent.clone()), Converge::Nothing => {} - Converge::Inert => { - tracing::info!( - %agent, state = ?decl.state, - "wanted state: state not recognised by this build; leaving the agent alone" - ); - } } } plan @@ -197,8 +191,6 @@ enum Converge { Stop, /// The hive already agrees with the declaration. Nothing, - /// A state this build does not recognise. - Inert, } /// The whole decision, pure: no queue, no store, no container. The three @@ -206,8 +198,6 @@ enum Converge { /// the behaviour rather than a model of it. fn decide(state: &AgentState, present: bool, intent: Option) -> Converge { match state { - // First, and whatever else is true of the agent. - AgentState::Unrecognised(_) => Converge::Inert, AgentState::Up if !present => Converge::Deploy, AgentState::Up => { if intent == Some(Wanted::Up) { @@ -237,10 +227,6 @@ mod tests { use crate::power::Wanted; use swarm_queue_client::wanted::{AgentState, AgentWanted, HiveWanted}; - fn unknown() -> AgentState { - AgentState::Unrecognised("paused".to_owned()) - } - fn declaration(agents: &[(&str, AgentState)]) -> HiveWanted { HiveWanted { agents: agents @@ -354,14 +340,20 @@ mod tests { assert_eq!(decide(&AgentState::Offline, false, None), Converge::Nothing); } - /// The version-skew arm: an unrecognised state is inert in every - /// combination, including the ones where a known state would act. + /// Version skew is handled one layer up, at the decode: `AgentState` is + /// closed, so an unknown value never reaches [`decide`] — it fails the + /// whole declaration in `swarm-queue-client`, which owns that test + /// (`an_unknown_state_fails_the_whole_declaration`). There is deliberately + /// no case for it here: a test asserting something unrepresentable would + /// pass forever without measuring anything. #[test] - fn an_unrecognised_state_is_inert_everywhere() { - for present in [true, false] { - for intent in [None, Some(Wanted::Up), Some(Wanted::Offline)] { - assert_eq!(decide(&unknown(), present, intent), Converge::Inert); - } + fn every_state_this_build_knows_is_covered_above() { + for state in [AgentState::Up, AgentState::Offline] { + let seen = [true, false].iter().any(|present| { + decide(&state, *present, None) != Converge::Nothing + || decide(&state, *present, Some(Wanted::Up)) != Converge::Nothing + }); + assert!(seen, "{state:?} produces no action in any combination"); } } } diff --git a/swarm-queue-client/src/wanted.rs b/swarm-queue-client/src/wanted.rs index 976f099e..60d8e114 100644 --- a/swarm-queue-client/src/wanted.rs +++ b/swarm-queue-client/src/wanted.rs @@ -56,14 +56,13 @@ pub struct AgentWanted { pub state: AgentState, } -/// What the controller wants an agent to be — an **open** enum. +/// What the controller wants an agent to be. /// -/// A value this build does not know deserialises into -/// [`AgentState::Unrecognised`], carried verbatim, rather than failing the -/// whole document or collapsing into a known variant. Both alternatives break -/// the same way: a closed enum forces "not `Up`" onto a state like `paused`, -/// so a controller that learns a new value would take agents down on every -/// hive that has not been updated yet. +/// **Closed on purpose.** A value this build does not know fails the whole +/// document's decode, so a hive running older code converges **nothing** rather +/// than part of a declaration it only half understands. Adding a state means +/// adding a variant here and shipping it to both ends — which is the intended +/// workflow, not an obstacle to route around with a catch-all variant. #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "snake_case")] pub enum AgentState { @@ -71,10 +70,6 @@ pub enum AgentState { Up, /// Exists on the hive and is not running. Offline, - /// Anything else. Kept as written so a hive can say what it declined to - /// act on instead of logging that something unspecified was skipped. - #[serde(untagged)] - Unrecognised(String), } /// Open the wanted-state bucket for writing, creating it if nothing has yet. @@ -129,32 +124,29 @@ mod tests { use super::{AgentState, HiveWanted}; #[test] - fn known_states_decode_and_an_unknown_one_stays_itself() { - let doc = r#"{"agents":{ - "a":{"state":"up"}, - "b":{"state":"offline"}, - "c":{"state":"paused"} - }}"#; + fn the_known_states_decode_and_round_trip() { + let doc = r#"{"agents":{"a":{"state":"up"},"b":{"state":"offline"}}}"#; let decoded: HiveWanted = serde_json::from_str(doc).expect("decodes"); assert_eq!(decoded.agents["a"].state, AgentState::Up); assert_eq!(decoded.agents["b"].state, AgentState::Offline); - assert_eq!( - decoded.agents["c"].state, - AgentState::Unrecognised("paused".to_owned()) - ); + assert_eq!(serde_json::to_string(&decoded).expect("serialises"), doc); } + /// The enum is closed, so a state this build does not know takes the + /// **whole document** down rather than one agent. That is the point: a hive + /// converges nothing rather than part of a declaration it half understands. + /// The `up` beside it is the control — a valid entry in the same document + /// does not rescue it, and this test would pass on a broken decoder without + /// the round-trip test above proving the happy path still works. #[test] - fn an_unrecognised_state_survives_a_round_trip() { - let doc = r#"{"agents":{"c":{"state":"paused"}}}"#; - let decoded: HiveWanted = serde_json::from_str(doc).expect("decodes"); - let reserialised = serde_json::to_string(&decoded).expect("serialises"); - assert_eq!(reserialised, r#"{"agents":{"c":{"state":"paused"}}}"#); + fn an_unknown_state_fails_the_whole_declaration() { + let doc = r#"{"agents":{"a":{"state":"up"},"c":{"state":"paused"}}}"#; + assert!(serde_json::from_str::(doc).is_err()); } - /// A field a newer controller adds must not cost an older hive the whole - /// document — that is the version skew the open enum exists for, one level - /// up. + /// Closed *values*, tolerant *fields*: a field a newer controller adds must + /// not cost an older hive the document, since adding one is not a semantic + /// a hive has to understand to obey the rest. #[test] fn unknown_fields_are_ignored() { let doc = r#"{"agents":{"a":{"state":"up","config_rev":"deadbeef"}},"epoch":3}"#; @@ -162,9 +154,8 @@ mod tests { assert_eq!(decoded.agents["a"].state, AgentState::Up); } - /// The control for the two above: openness is about *values and fields*, - /// not about accepting anything. A declaration missing the one required - /// field is an error, so a malformed document cannot read as "no agents". + /// A declaration missing the one required field is an error, so a malformed + /// document cannot read as "no agents". #[test] fn a_missing_state_is_an_error() { assert!(serde_json::from_str::(r#"{"agents":{"a":{}}}"#).is_err());