#1444: accept imported entries from unissued spaces; reserve only the top half (NO_SESSION_SPACE)

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) {

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<Vec<(Entry, bool)>, Error> {
         let before = self.state.document();
         let mut touched: BTreeSet<Entry> = 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())));
             }