From 2d02f289ec849d26c1ad4a461c3a3a956e70fcb6 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Wed, 28 Oct 2026 02:16:00 +0100 Subject: [PATCH] Keep session joins working when a document has out-of-range ids Fixes #1067 Co-Authored-By: Claude Opus 5.5 --- crates/collab/src/entry_ids_tests.rs | 31 ++++++++++++++++++++++++++++ crates/collab/src/server.rs | 16 +++++++++----- crates/doc/src/ids.rs | 5 +++-- crates/doc/src/ids_tests.rs | 4 ++++ 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/crates/collab/src/entry_ids_tests.rs b/crates/collab/src/entry_ids_tests.rs index db58bb2f..976d2cb6 100644 --- a/crates/collab/src/entry_ids_tests.rs +++ b/crates/collab/src/entry_ids_tests.rs @@ -92,6 +92,37 @@ fn client_ids_skip_spaces_used_by_styles_and_comments() { assert_eq!(join(&mut server).id(), 10); } +#[test] +fn an_id_in_the_top_space_does_not_block_joins() { + let mut d = Document::new("a"); + d.styles.push(style(format!("{STYLE_ID_PREFIX}{}", u64::MAX), "Out of range")); + let c = comment(u64::from(u32::MAX) << 32, &d); + d.comments.push(c); + let mut server = Server::new(d, "", Limits::default()); + let a = join(&mut server); + assert!(a.id() > 0 && a.id() < u32::MAX, "{}", a.id()); + // After a reload the session still takes newcomers and keeps the document as it was. + let state = server.state().unwrap(); + let mut restored = Server::restore(state.clone(), "", Limits::default()).unwrap(); + let b = join(&mut restored); + assert!(b.id() > a.id() && b.id() < u32::MAX, "{}", b.id()); + assert_eq!(restored.document().styles.len(), 1); + assert_eq!(restored.document().comments.len(), 1); + // A state saved while the counter was stuck at the top recovers too. + let stuck = crate::SessionState { next_client: u32::MAX, ..state }; + let mut restored = Server::restore(stuck, "", Limits::default()).unwrap(); + let c = join(&mut restored); + assert!(c.id() > a.id() && c.id() < u32::MAX, "{}", c.id()); + // A new entry may not take an id from a space nobody was given yet. + for (k, space) in [c.id() + 1, c.id() + 1000, u32::MAX].into_iter().enumerate() { + let id = format!("{STYLE_ID_PREFIX}{}", (u64::from(space) << 32) | 1); + let ServerMsg::Reject { reason, .. } = send(&mut restored, c.id(), 1 + k as u64, vec![put_style(id.clone())]) else { + panic!("accepted {id}") + }; + assert!(reason.contains("id space"), "{reason}"); + } +} + /// Each client makes `rounds` styles, collections (with an extra mode), variables and comments /// before seeing anyone else's. fn create_everything(doc: &mut Document, who: usize, round: usize) { diff --git a/crates/collab/src/server.rs b/crates/collab/src/server.rs index 52f112ad..54089af6 100644 --- a/crates/collab/src/server.rs +++ b/crates/collab/src/server.rs @@ -153,7 +153,13 @@ impl Server { server.state = replica; server.session = state.session; server.seq = state.seq; - server.next_client = server.next_client.max(state.next_client); + // A counter stuck at the top (saved while an out-of-range id in the document blocked + // every join) carries on above the ids actually handed out. + let saved = match state.next_client { + u32::MAX => state.issued.keys().chain(state.log.iter().map(|a| &a.author)).max().map_or(0, |c| c.saturating_add(1)), + n => n, + }; + server.next_client = server.next_client.max(saved); server.issued = state.issued.into_iter().filter(|(id, _)| *id < server.next_client).collect(); server.deleted = state.deleted; server.removed = state.removed; @@ -481,9 +487,9 @@ impl Server { /// The document-level entries (styles, variables, collections, comments) a batch adds or /// removes, each with whether it is there afterwards. A new one is refused when its id lies in - /// the space of another client this session gave an id to (unless it was removed before: undo - /// puts entries back); ids from before the session (space 0, earlier sessions, other files) - /// are fine. + /// the space of another client this session gave an id to, or of one it hasn't given out yet + /// (unless it was removed before: undo puts entries back); ids from before the session (space + /// 0, earlier sessions, other files) are fine. fn check_entries(&self, from: u32, changes: &[Change], after: &Document) -> Result, Error> { let before = self.state.document(); let mut touched: BTreeSet = BTreeSet::new(); @@ -510,7 +516,7 @@ impl Server { } if now && !self.removed.contains(&entry) - && let Some(space) = entry.id_space().filter(|s| *s != from && self.issued.contains_key(s)) + && let Some(space) = entry.id_space().filter(|s| *s != from && (self.issued.contains_key(s) || *s >= self.next_client)) { return Err(Error::Invalid(format!("new {} isn't in client {from}'s id space (it is client {space}'s)", entry.describe()))); } diff --git a/crates/doc/src/ids.rs b/crates/doc/src/ids.rs index 024946bb..7e244fbb 100644 --- a/crates/doc/src/ids.rs +++ b/crates/doc/src/ids.rs @@ -128,7 +128,8 @@ impl Document { /// The highest client id space any id in the document lies in (layers, comments, styles, /// variables, collections, modes): a session hands out client ids above it, so ids made in - /// earlier sessions are never made again. + /// earlier sessions are never made again. Space `u32::MAX` doesn't count: numbers too big for + /// any client's space land there ([`space_of`]) and no session hands that client id out. pub fn highest_id_space(&self) -> u32 { let nodes = self.ids().map(|id| id.0); let comments = self.comments.iter().map(|c| c.id); @@ -136,6 +137,6 @@ impl Document { let vars = self.variables.variables.iter().filter_map(|v| id_number(&v.id, VARIABLE_ID_PREFIX)); let colls = self.variables.collections.iter().filter_map(|c| id_number(&c.id, COLLECTION_ID_PREFIX)); let modes = self.variables.collections.iter().flat_map(|c| c.modes.iter().filter_map(|m| mode_number(&m.id))); - nodes.chain(comments).chain(styles).chain(vars).chain(colls).chain(modes).map(space_of).max().unwrap_or(0) + nodes.chain(comments).chain(styles).chain(vars).chain(colls).chain(modes).map(space_of).filter(|s| *s != u32::MAX).max().unwrap_or(0) } } diff --git a/crates/doc/src/ids_tests.rs b/crates/doc/src/ids_tests.rs index 4f489886..7c6a50e0 100644 --- a/crates/doc/src/ids_tests.rs +++ b/crates/doc/src/ids_tests.rs @@ -103,6 +103,10 @@ fn the_highest_space_counts_every_kind_of_id() { assert_eq!(d.highest_id_space(), 9); d.comments.push(comment(11u64 << 32, d.first_page().unwrap())); assert_eq!(d.highest_id_space(), 11); + // Numbers past every client's space (they saturate to `u32::MAX`) are no session's. + d.styles.push(style(&format!("S:{}", u64::MAX))); + d.comments.push(comment(u64::from(u32::MAX) << 32, d.first_page().unwrap())); + assert_eq!(d.highest_id_space(), 11); // Other forms are space 0. assert_eq!(id_space("lib:S:99999999999", "S:"), 0); assert_eq!(id_space("S:+5", "S:"), 0);