diff --git a/crates/collab/src/entry_ids_tests.rs b/crates/collab/src/entry_ids_tests.rs index 976d2cb6..72a147ae 100644 --- a/crates/collab/src/entry_ids_tests.rs +++ b/crates/collab/src/entry_ids_tests.rs @@ -6,6 +6,7 @@ use figcraft_doc::{Collection, Comment, Document, Message, Mode, Style, StyleKin use crate::change::{Change, Entry, Json}; use crate::message::{ClientMsg, ServerMsg}; +use crate::server::NO_SESSION_SPACE; use crate::sim::Sim; use crate::{Client, Limits, NoRecompute, Outgoing, Server}; @@ -113,8 +114,8 @@ fn an_id_in_the_top_space_does_not_block_joins() { 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() { + // A new entry may not take an id from the reserved top half of the spaces. + for (k, space) in [NO_SESSION_SPACE, 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}") @@ -123,6 +124,22 @@ fn an_id_in_the_top_space_does_not_block_joins() { } } +#[test] +fn styles_imported_from_another_session_are_accepted_and_joins_go_on() { + let mut server = Server::new(Document::new("a"), "", Limits::default()); + let a = join(&mut server); + // `file.import` copies styles with their ids, made by clients this session hasn't given out + // (up to the highest space a member may use). + for (k, space) in [a.id() + 1, a.id() + 1000, NO_SESSION_SPACE - 1].into_iter().enumerate() { + let id = format!("{STYLE_ID_PREFIX}{}", (u64::from(space) << 32) | 1); + assert!(matches!(send(&mut server, a.id(), 1 + k as u64, vec![put_style(id.clone())]), ServerMsg::Changes(_)), "refused {id}"); + } + // After a restart client ids start above them, and there are plenty left. + let mut restored = Server::restore(server.state().unwrap(), "", Limits::default()).unwrap(); + assert_eq!(join(&mut restored).id(), NO_SESSION_SPACE); + assert_eq!(join(&mut Server::new(restored.document().clone(), "", Limits::default())).id(), NO_SESSION_SPACE); +} + /// 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 c45b0ba6..d8aab594 100644 --- a/crates/collab/src/server.rs +++ b/crates/collab/src/server.rs @@ -25,6 +25,12 @@ pub enum Outgoing { /// Why a viewer's batch is refused. pub const VIEW_ONLY: &str = "you can only view this file (your link is view-only)"; +/// A new style, variable, collection or comment may not take an id in a space at or above this +/// that isn't its creator's. Client ids start above every space in the document after a restart, +/// so no member can use them up: at least this many always remain. Lower spaces stay open, for +/// entries imported from other files' sessions. +pub const NO_SESSION_SPACE: u32 = 1 << 31; + /// What changed a server's lasting state, in order: what a store appends to its log between /// snapshots ([`Server::journaling`]) and replays over the last snapshot after a restart /// ([`Server::replay`]). Presence and who is connected aren't in it. @@ -506,9 +512,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, 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. + /// the space of another client this session gave an id to, or in a space at or above + /// [`NO_SESSION_SPACE`] (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(); @@ -535,7 +541,7 @@ impl Server { } if now && !self.removed.contains(&entry) - && let Some(space) = entry.id_space().filter(|s| *s != from && (self.issued.contains_key(s) || *s >= self.next_client)) + && let Some(space) = entry.id_space().filter(|s| *s != from && (self.issued.contains_key(s) || *s >= NO_SESSION_SPACE)) { return Err(Error::Invalid(format!("new {} isn't in client {from}'s id space (it is client {space}'s)", entry.describe()))); }