diff --git a/swarm-controller/src/wanted.rs b/swarm-controller/src/wanted.rs index 4d5def90..a80eaf47 100644 --- a/swarm-controller/src/wanted.rs +++ b/swarm-controller/src/wanted.rs @@ -127,6 +127,25 @@ fn apply( } .into()); } + // `Create` against a name that already has a *non-terminal* declaration + // is not a creation — it's the create-agent endpoint being asked to + // declare a name that turns out to already exist (a retried request, or + // an operator migrating a pre-existing agent into this bookkeeping for + // the first time). Creation is expected to be idempotent, so this is a + // no-op that leaves the existing declaration exactly as it stood rather + // than stomping it back to whatever state the create path passes — + // otherwise calling create against a name that is already `Up` would + // silently pause a running agent. `Destroyed` is excluded on purpose: + // that's the one case a fresh create *does* need to write over (see + // `Intent::Create`'s own doc comment). + if intent == Intent::Create + && let Some(existing) = declaration.agents.get(agent) + && existing.state != AgentState::Destroyed + { + let encoded = + serde_json::to_vec(&declaration).context("encoding the current declaration")?; + return Ok((declaration, encoded)); + } declaration .agents .insert(agent.to_owned(), AgentWanted { state }); @@ -201,6 +220,12 @@ impl WantedWriter { /// the time this runs the swarm has already built that agent's identity, /// repo and config from nothing. /// + /// Idempotent against any other existing declaration: an agent already + /// declared in a non-terminal state (an operator migrating a pre-existing + /// agent into this bookkeeping, or a retried request) is left exactly as + /// it stood — creation must not silently pause an agent already running + /// under some other wanted state. + /// /// A separate method rather than an `Intent` parameter on `set`: the /// caller that may overwrite a terminal state is the agent-creation job /// node and nothing else, and an argument every other caller has to pass @@ -239,6 +264,15 @@ impl WantedWriter { intent, )?; + // `apply`'s `Intent::Create` no-op (an already-declared, + // non-terminal agent) re-encodes the declaration unchanged — + // skip the network round-trip for it entirely rather than + // writing identical bytes back. + if entry.as_ref().map(|e| e.value.as_ref()) == Some(encoded.as_slice()) { + tracing::debug!(hive, agent, "declaration already current, no write needed"); + return Ok(declaration); + } + // `update` and `create` have separate error types, and only one // variant of each means "someone else got there first". Every // other failure returns immediately: retrying a disconnect or a @@ -402,6 +436,30 @@ mod tests { assert_eq!(declaration.agents["iris"].state, AgentState::Destroyed); } + /// mara: "this does not match the expectation that agent creation is + /// idempotent so pre existing agents can be migrated" — an agent already + /// declared in some other, non-terminal state (say an operator migrating + /// a pre-existing agent into this bookkeeping) must not be stomped back + /// to whatever state the create path passes. + #[test] + fn creating_an_agent_does_not_disturb_an_existing_non_terminal_declaration() { + let current = br#"{"agents":{"atlas":{"state":"up"}}}"#; + let (declaration, _) = + apply(Some(current), "atlas", AgentState::Paused, Intent::Create).unwrap(); + assert_eq!(declaration.agents["atlas"].state, AgentState::Up); + } + + /// The idempotent no-op only inspects the agent being created — it must + /// not touch any other agent's declaration on the same hive. + #[test] + fn creating_an_agent_that_already_exists_leaves_every_other_agent_alone() { + let current = br#"{"agents":{"atlas":{"state":"up"},"iris":{"state":"offline"}}}"#; + let (declaration, _) = + apply(Some(current), "atlas", AgentState::Paused, Intent::Create).unwrap(); + assert_eq!(declaration.agents["atlas"].state, AgentState::Up); + assert_eq!(declaration.agents["iris"].state, AgentState::Offline); + } + /// `Intent::Create` relaxes exactly one rule. An undecodable current /// value is still an error, for the same reason it is on a redeclare: /// writing over it discards every other agent on the hive.