diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 37e7c743..6ebe3032 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -794,7 +794,7 @@ fn declaration_target( request_body = SetAgentStateRequest, responses( (status = 200, description = "the declaration as now published", body = Vec), - (status = 400, description = "a name is not an identifier, the hive is not in this swarm, or the state is unknown (problem+json)", body = String), + (status = 400, description = "a name is not an identifier, the hive is not in this swarm, the state is unknown, or the agent is already declared destroyed (terminal — problem+json)", body = String), (status = 503, description = "no swarm queue is wired up (problem+json)", body = String), (status = 500, description = "the declaration could not be published (problem+json)", body = String), ), @@ -812,6 +812,12 @@ async fn set_agent_state( .into_string(); let declaration = writer.set(&hive, &agent, req.state).await.map_err(|e| { + // A `TerminalStateError` is the caller's mistake (asking to move an + // agent off a state its own doc comment says is terminal), not a + // server fault — everything else here is the pre-existing catch-all. + if let Some(terminal) = e.downcast_ref::() { + return error_problem(axum::http::StatusCode::BAD_REQUEST, &terminal.to_string()); + } tracing::warn!(hive = %hive, agent = %agent, error = %format!("{e:#}"), "declaring agent state failed"); error_problem( axum::http::StatusCode::INTERNAL_SERVER_ERROR, diff --git a/swarm-controller/src/wanted.rs b/swarm-controller/src/wanted.rs index 4e25d80a..77e0ca04 100644 --- a/swarm-controller/src/wanted.rs +++ b/swarm-controller/src/wanted.rs @@ -13,10 +13,41 @@ //! declaration to reconcile against, because the current value can be read //! back from the queue whenever it is needed. +use std::fmt; + use anyhow::{Context, Result}; use async_nats::jetstream::kv::{CreateErrorKind, UpdateErrorKind}; use swarm_queue_client::wanted::{AgentState, AgentWanted, HiveWanted}; +/// The one failure out of [`apply`] that is the caller's mistake, not a +/// server fault — worth a distinct type so `set_agent_state` can tell it +/// apart from every other error this module raises (an undecodable +/// declaration, a lost write) and answer 400 instead of 500. +/// +/// Carried through as a plain `anyhow::Error` like every other error here +/// (this crate has no typed-error convention to join), and recovered at the +/// HTTP boundary with `downcast_ref` — the shape every other "kind of error +/// that came from underneath" check in this module already uses +/// ([`Wrote`]'s `UpdateErrorKind`/`CreateErrorKind` matching), just inspecting +/// an `anyhow::Error` instead of a client library's own error enum. +#[derive(Debug)] +pub struct TerminalStateError { + agent: String, +} + +impl fmt::Display for TerminalStateError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{} is already declared destroyed — terminal, per AgentState::Destroyed's own doc \ + comment: no transition brings it back short of a fresh deploy", + self.agent + ) + } +} + +impl std::error::Error for TerminalStateError {} + /// The three outcomes of one write attempt, which the two KV verbs report /// through separate error types. enum Wrote { @@ -50,6 +81,19 @@ fn apply(current: Option<&[u8]>, agent: &str, state: AgentState) -> Result<(Hive .context("the hive's current declaration is not decodable")?, None => HiveWanted::default(), }; + // Redeclaring the same terminal state is a no-op, not a transition — a + // client that already sees the agent as destroyed and asks again should + // not be punished for it. Anything else moving off `Destroyed` is the + // one transition this module exists to refuse. + if let Some(existing) = declaration.agents.get(agent) + && existing.state == AgentState::Destroyed + && state != AgentState::Destroyed + { + return Err(TerminalStateError { + agent: agent.to_owned(), + } + .into()); + } declaration .agents .insert(agent.to_owned(), AgentWanted { state }); @@ -218,4 +262,39 @@ mod tests { assert_eq!(again.agents["atlas"].state, AgentState::Offline); assert_eq!(again.agents["iris"].state, AgentState::Up); } + + /// The bug this module exists to fix: nothing used to stop a caller from + /// declaring a destroyed agent back to `Up`/`Offline`, silently + /// un-terminaling a state whose own doc comment says that is impossible. + #[test] + fn moving_a_destroyed_agent_to_any_other_state_is_refused() { + let current = br#"{"agents":{"atlas":{"state":"destroyed"}}}"#; + for state in [AgentState::Up, AgentState::Offline] { + let err = apply(Some(current), "atlas", state).unwrap_err(); + assert!( + err.downcast_ref::().is_some(), + "expected a TerminalStateError for {state:?}, got: {err}" + ); + } + } + + /// Redeclaring `Destroyed` while already `Destroyed` is a no-op, not a + /// transition — a client that already sees the terminal state and asks + /// again should get success back, not an error for stating a fact. + #[test] + fn redeclaring_destroyed_as_destroyed_is_not_a_transition() { + let current = br#"{"agents":{"atlas":{"state":"destroyed"}}}"#; + let (declaration, _) = apply(Some(current), "atlas", AgentState::Destroyed).unwrap(); + assert_eq!(declaration.agents["atlas"].state, AgentState::Destroyed); + } + + /// The refusal is per-agent — a destroyed agent on the same hive as a + /// live one must not block declaring the live one. + #[test] + fn a_destroyed_agent_does_not_block_declaring_a_different_one() { + let current = br#"{"agents":{"atlas":{"state":"destroyed"},"iris":{"state":"up"}}}"#; + let (declaration, _) = apply(Some(current), "iris", AgentState::Offline).unwrap(); + assert_eq!(declaration.agents["atlas"].state, AgentState::Destroyed); + assert_eq!(declaration.agents["iris"].state, AgentState::Offline); + } }