diff --git a/crates/server/src/api/declare.rs b/crates/server/src/api/declare.rs index b3c5204..d326bf9 100644 --- a/crates/server/src/api/declare.rs +++ b/crates/server/src/api/declare.rs @@ -271,31 +271,38 @@ async fn do_sync(st: &Shared, application: &str, bodies: Vec) -> ApiResul // Reap, and ONLY inside this application. The flat version of this reaped // everything the cell held, so two teams syncing against one deployment - // would delete each other's graphs — including from the durable store. Done - // AFTER the declares, so a sync that fails half way removes nothing. + // would delete each other's graphs — including from the durable store. + // + // A partial declaration is not an authoritative inventory. One malformed + // body must not turn `ok: false` into a successful deletion of every valid + // target the caller omitted, so a sync that refused anything removes + // nothing. Successfully applied documents stay applied and the caller can + // repair/retry the list without recovering deleted configuration first. let mut removed = Vec::new(); - for rt in st.registry.of_app(application) { - if declared.contains(&rt.doc.graph) { - continue; - } - // A sync reaps what it could have DECLARED, and nothing else. v1 - // exempted graph nodes from a target sync because a target list does not - // name them and reaping one would tear down half a topology; there are - // no node-targets any more, so the same rule is spelt as "a sync of - // targets does not delete a graph". A one-node graph IS a target and is - // fair game. - if rt.plan.nodes.len() > 1 { - continue; - } - let name = rt.doc.graph.clone(); - if let Err(e) = crate::store::forget(&st.queen, application, &name).await { - tracing::warn!(graph = %name, error = %e, "sync: not reaped, the stored document could not be removed"); - refused.push(json!({ "target": name, "error": format!("not reaped: {e}") })); - continue; + if refused.is_empty() { + for rt in st.registry.of_app(application) { + if declared.contains(&rt.doc.graph) { + continue; + } + // A sync reaps what it could have DECLARED, and nothing else. v1 + // exempted graph nodes from a target sync because a target list does not + // name them and reaping one would tear down half a topology; there are + // no node-targets any more, so the same rule is spelt as "a sync of + // targets does not delete a graph". A one-node graph IS a target and is + // fair game. + if rt.plan.nodes.len() > 1 { + continue; + } + let name = rt.doc.graph.clone(); + if let Err(e) = crate::store::forget(&st.queen, application, &name).await { + tracing::warn!(graph = %name, error = %e, "sync: not reaped, the stored document could not be removed"); + refused.push(json!({ "target": name, "error": format!("not reaped: {e}") })); + continue; + } + crate::supervisor::stop(&rt).await; + st.registry.remove(application, &name); + removed.push(name); } - crate::supervisor::stop(&rt).await; - st.registry.remove(application, &name); - removed.push(name); } ok(json!({ diff --git a/crates/server/tests/live.rs b/crates/server/tests/live.rs index 776e494..6d6ef22 100644 --- a/crates/server/tests/live.rs +++ b/crates/server/tests/live.rs @@ -2144,6 +2144,75 @@ async fn one_owner_per_ingress_queue() { h.cleanup("second").await; } +/// A sync that rejects one document is not a complete inventory and may not +/// delete an omitted target. +/// +/// Returning `ok: false` after removing valid configuration is a destructive +/// partial success: a typo in one replacement document would turn into an +/// outage in an unrelated target before the caller could correct and retry it. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[ignore = "needs a broker: set GATE_TEST_QUEEN_URL and run with --include-ignored"] +async fn a_partially_refused_sync_reaps_nothing() { + let Some(h) = harness("sync-refusal").await else { + return; + }; + let out = egress_of("sync-refusal", &h.application); + // Two targets, so the list that follows is PARTIALLY valid and one target is + // omitted from it. A sync naming nothing valid would prove much less. + for name in ["keep", "drop"] { + let (status, body) = h + .put_graph(name, one_node(&format!("{out}.{name}"), wide("b"))) + .await; + assert_eq!(status, 200, "initial declare of {name}: {body}"); + } + + let mut valid = one_node(&format!("{out}.keep"), wide("b")); + valid["application"] = json!(h.application); + valid["graph"] = json!("keep"); + let (status, result) = h + .send( + reqwest::Method::PUT, + &format!("/v1/apps/{}/targets", h.application), + Some(json!([ + valid, + { + "application": h.application, + "graph": "broken", + "version": 1, + "nodes": {}, + "paths": [] + } + ])), + ) + .await; + assert_eq!(status, 200, "sync response: {result}"); + assert_eq!(result["ok"], json!(false), "the invalid graph must fail"); + assert_eq!( + result["applied"], + json!(["keep"]), + "a valid document in the list still applies: {result}" + ); + assert_eq!( + result["removed"], + json!([]), + "a partial sync may reap nothing" + ); + + // `drop` is the one the caller left out. A refusal anywhere in the list is + // what makes the list unfit to authorise a deletion. + for name in ["keep", "drop"] { + let (status, view) = h.get_graph(name).await; + assert_eq!( + status, 200, + "`{name}` was deleted by a refused sync: {view}" + ); + assert!(view["running"].as_bool().unwrap_or(false), "{name}: {view}"); + } + + h.cleanup("keep").await; + h.cleanup("drop").await; +} + /// The routes that are gone say where to go instead. /// /// A 404 would read as "wrong URL" and send somebody hunting; a 410 with the