M9: a refused batch's fixes in linear time (#1063) (#1522)

The server's fixes for a refused batch walked every deleted layer's whole subtree again and
compared each document-level fix with all the earlier ones, so a batch deleting a deep chain took
quadratic time on the hub thread. Each subtree walk now stops at a layer already restored, and
document-level fixes are looked up and sent once per target. The fixes are unchanged.

A refused batch deleting a 20,000-deep chain plus 40,000 comment puts: 27.2 s before, 0.14 s after
(debug build).

Fixes #1063

crates/collab/src/server.rs

@@ -557,6 +557,7 @@ impl Server {
         let doc = self.state.document();
         let mut props: BTreeMap<NodeId, BTreeSet<Prop>> = BTreeMap::new();
         let mut gone: BTreeSet<NodeId> = BTreeSet::new();
+        let (mut asked, mut sent) = (BTreeSet::new(), BTreeSet::new());
         let mut out = Vec::new();
         for c in changes {
             match c {
@@ -569,9 +570,11 @@ impl Server {
                 Change::Delete { id } => {
                     gone.insert(*id);
                 }
+                // The same target has the same fix: look each one up once, and send each fix once.
                 other => {
-                    if let Some(fix) = docprops::current(doc, other)?
-                        && !out.contains(&fix)
+                    if Target::of(other).is_some_and(|t| asked.insert(t))
+                        && let Some(fix) = docprops::current(doc, other)?
+                        && Target::of(&fix).is_some_and(|t| sent.insert(t))
                     {
                         out.push(fix);
                     }
@@ -589,19 +592,21 @@ impl Server {
                 nodes.extend(t.write(self.state.value(&t)?.as_ref()));
             }
         }
-        let mut restored = std::collections::BTreeSet::new();
+        // Each deleted node's subtree, pre-order, every node once: a restored node's whole subtree
+        // went out with it, so the walk stops there (deleting a deep chain stays linear).
+        let mut restored = BTreeSet::new();
         for id in gone {
-            match doc.node(id) {
-                Some(_) => {
-                    for d in doc.subtree(id) {
-                        if restored.insert(d)
-                            && let Some(n) = doc.node(d)
-                        {
-                            nodes.push(create_change(n, self.state.key(d))?);
-                        }
-                    }
+            if !doc.contains(id) {
+                nodes.push(Change::Delete { id });
+                continue;
+            }
+            let mut stack = vec![id];
+            while let Some(d) = stack.pop() {
+                let Some(n) = doc.node(d) else { continue };
+                if restored.insert(d) {
+                    nodes.push(create_change(n, self.state.key(d))?);
+                    stack.extend(n.children.iter().rev());
                 }
-                None => nodes.push(Change::Delete { id }),
             }
         }
         nodes.extend(out);

crates/collab/src/server_tests.rs

@@ -2,7 +2,7 @@
 
 use figcraft_doc::{Document, Node, NodeId, NodeKind};
 
-use crate::change::{Change, Json, Prop};
+use crate::change::{Change, Entry, Json, Prop};
 use crate::message::{ClientMsg, Hello, PROTOCOL, Presence, ServerMsg};
 use crate::replica::Mode;
 use crate::{Client, Limits, NoRecompute, Outgoing, Server};
@@ -217,6 +217,43 @@ fn fixes_roll_a_rejected_client_back_to_the_server_state() {
     assert_eq!(b.view(), s.replica());
 }
 
+#[test]
+fn a_refused_deep_delete_restores_each_node_once_in_linear_time() {
+    const DEPTH: usize = 20_000;
+    let (mut d, _, frame) = doc();
+    let mut chain = vec![frame];
+    while chain.len() < DEPTH {
+        let parent = *chain.last().unwrap();
+        chain.push(d.create(parent, NodeKind::Frame, "F", 0.0, 0.0, 1.0, 1.0).unwrap());
+    }
+    let mut s = Server::new(d, "tok", Limits::default());
+    let a = join(&mut s);
+    // Every node of the chain deleted, plus a rename of a node nobody has: refused whole.
+    let missing = NodeId(u64::MAX >> 1);
+    let mut changes: Vec<Change> = chain.iter().map(|&id| Change::Delete { id }).collect();
+    changes.push(Change::Set { id: missing, prop: Prop::Name, value: Json("x".into()) });
+    // As many comment threads put, each twice: they go back to not existing, once each.
+    let put = |c: u64| Change::Put { entry: Entry::Comment(c), value: Json(serde_json::json!({})) };
+    changes.extend((1..=DEPTH as u64).chain(1..=DEPTH as u64).map(put));
+    let started = std::time::Instant::now();
+    let (_, fixes) = rejected(&mut s, a.id(), changes);
+    let took = started.elapsed();
+    assert_eq!(fixes.first(), Some(&Change::Delete { id: missing }));
+    let restored: Vec<NodeId> = fixes[1..=DEPTH]
+        .iter()
+        .map(|c| match c {
+            Change::Create { id, .. } => *id,
+            other => panic!("not a creation: {other:?}"),
+        })
+        .collect();
+    assert_eq!(restored, chain, "each deleted node comes back once, parents first");
+    let removed: Vec<Change> = (1..=DEPTH as u64).map(|c| Change::Remove { entry: Entry::Comment(c) }).collect();
+    assert_eq!(fixes[DEPTH + 1..], removed[..], "each entry once, in batch order");
+    // Walking every deleted node's subtree again and comparing each fix with all the earlier
+    // ones took 27 s in a debug build; once each takes 0.1 s.
+    assert!(took < std::time::Duration::from_secs(10), "{took:?}");
+}
+
 #[test]
 fn remote_batches_apply_under_pending_ones() {
     let (d, _, frame) = doc();