fix(#3124): close AgentState — an unknown value is not a partial instruction
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.
This commit is contained in:
parent
37f3c63eeb
commit
a4924aee4d
2 changed files with 41 additions and 58 deletions
|
|
@ -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::<HiveWanted>(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::<HiveWanted>(r#"{"agents":{"a":{}}}"#).is_err());
|
||||
|
|
|
|||
Loading…
Reference in a new issue