fix(sync): preserve targets when any declaration is refused

crates/server/src/api/declare.rs

@@ -271,31 +271,38 @@ async fn do_sync(st: &Shared, application: &str, bodies: Vec<Value>) -> 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!({

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