diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 962a8dac..37e7c743 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -795,7 +795,6 @@ fn declaration_target( 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 = 409, description = "the agent is already declared destroyed, a terminal state the request tries to move it off of (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), ), @@ -813,14 +812,6 @@ 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, but not a malformed - // request — the request is well-formed, it just conflicts with the - // target resource's current (terminal) state, which per RFC 9110 is - // what 409 exists for, not 400. Everything else here is the - // pre-existing catch-all. - if let Some(terminal) = e.downcast_ref::() { - return error_problem(axum::http::StatusCode::CONFLICT, &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 77e0ca04..4e25d80a 100644 --- a/swarm-controller/src/wanted.rs +++ b/swarm-controller/src/wanted.rs @@ -13,41 +13,10 @@ //! 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 { @@ -81,19 +50,6 @@ 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 }); @@ -262,39 +218,4 @@ 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); - } }