swarm-controller: refuse a state transition off destroyed

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<AgentDeclaration>),
-        (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::<wanted::TerminalStateError>() {
+            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,

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::<super::TerminalStateError>().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);
+    }
 }