From b2c6a4d969ab8e2893d7fb3a46cd877b41f2865f Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 12:47:32 +0800 Subject: [PATCH 01/10] fix: reject imports that reuse local op ids with different content (#1118) Importing history from a client that shared a peer id trimmed the "known" part by version vector and applied the tail on top of different local history: it panicked in state apply (poisoning the doc, aborting on drop) or silently scrambled text. Compare the overlap with local history before anything is mutated and return UsedOpID; also reject changes that skip counters of their peer instead of tripping a DAG assertion. Rationale: context/import-peer-id-reuse.md Co-Authored-By: Claude Opus 5.5 --- .changeset/import-reused-peer-id.md | 5 + AGENTS.md | 3 + context/import-peer-id-reuse.md | 90 ++++ .../examples/import_known_history_perf.rs | 150 ++++++ crates/loro-internal/AGENTS.md | 3 + crates/loro-internal/src/arena.rs | 9 + .../src/encoding/fast_snapshot.rs | 3 +- crates/loro-internal/src/loro.rs | 10 +- crates/loro-internal/src/oplog.rs | 2 + .../loro-internal/src/oplog/change_store.rs | 50 +- .../loro-internal/src/oplog/known_history.rs | 384 +++++++++++++++ .../tests/import_reused_peer_id.test.ts | 76 +++ crates/loro/tests/import_reused_peer_id.rs | 463 ++++++++++++++++++ crates/loro/tests/movable_list_invalid_ops.rs | 4 +- docs/encoding.md | 15 +- 15 files changed, 1222 insertions(+), 45 deletions(-) create mode 100644 .changeset/import-reused-peer-id.md create mode 100644 context/import-peer-id-reuse.md create mode 100644 crates/examples/examples/import_known_history_perf.rs create mode 100644 crates/loro-internal/src/oplog/known_history.rs create mode 100644 crates/loro-wasm/tests/import_reused_peer_id.test.ts create mode 100644 crates/loro/tests/import_reused_peer_id.rs diff --git a/.changeset/import-reused-peer-id.md b/.changeset/import-reused-peer-id.md new file mode 100644 index 000000000..8ced6f676 --- /dev/null +++ b/.changeset/import-reused-peer-id.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Importing updates or a snapshot that reuse op ids the document already has for different content (two clients that shared a peer id) now throws a "ID ... has been used" error and leaves the document unchanged and usable. Previously the import trapped the WASM instance (`RuntimeError: unreachable`) or silently scrambled text. Imports with a change that skips counters of its peer also throw instead of trapping. diff --git a/AGENTS.md b/AGENTS.md index 708a805f9..0d59af1de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,9 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. [context/mergeable-containers.md](context/mergeable-containers.md). - `import_batch` force-detach, batch-wide rollback scope, and the never-exit-detached invariant: [context/import-batch-atomicity.md](context/import-batch-atomicity.md). +- Imports that reuse local op ids (a shared peer id): the known-history check, + what counts as equal, and what it cannot catch: + [context/import-peer-id-reuse.md](context/import-peer-id-reuse.md). - Movable-list `Move`/`Set` import validation, and why a move of a deleted element is applied rather than rejected: [context/movable-list-op-validation.md](context/movable-list-op-validation.md). diff --git a/context/import-peer-id-reuse.md b/context/import-peer-id-reuse.md new file mode 100644 index 000000000..638f8f612 --- /dev/null +++ b/context/import-peer-id-reuse.md @@ -0,0 +1,90 @@ +# Imports That Reuse Local Op Ids + +Verified against code 2026-09-30. + +An import skips the part of each change the doc already has by version vector and +applies the rest on top of the local history. When two clients shared a peer id +(loro-dev/loro#1118), the "known" part is different history. The rest was then +applied to a state it was not written against. It either panicked inside +`RichtextState::apply_diff` while the doc locks were held, poisoning them (a later +`Drop` then aborted the process), or it returned `Ok` with scrambled text. Since +loro-dev/loro#1160, `RichtextState::validate_diff` rejects the out-of-range +case after the fact. The check below rejects both cases before anything changes, +and names the reused id. + +## The check + +`OpLog::check_and_trim_known_part_of_changes` (`crates/loro-internal/src/oplog/known_history.rs`) +runs in `LoroDoc::import_changes_and_apply_delta_to_state_if_needed` right after +decode, before the preflight and before anything is mutated. That function is the +single entry for binary updates, snapshots imported into a non-empty doc, JSON +updates, and each blob of `import_batch`. An error takes the existing early-exit +path, which rolls back only the arena (decode may have registered containers). + +1. **Content.** If the import brings anything new, every imported change that + overlaps the local version vector is compared with the local changes over the + overlap. A mismatch returns `LoroError::UsedOpID { id }` with the first + differing id. +2. **Trim.** The known parts are dropped, as the decoders used to do. +3. **Skipped counters.** A change whose deps are present but whose peer's + previous counter is not is rejected with `DecodeError`. The main pass of + `import_changes_to_oplog` would apply it and trip the counter assertion in + `AppDag::update_version_on_new_change` under the op log lock. The check replays + that pass on counter ends only. Parked pending changes already wait for their + own predecessor (`remote_change_apply_state`). + +The decoders (`ChangeStore::decode_block_bytes`, `decode_snapshot_for_updates`) +therefore no longer trim. A snapshot with nothing new still returns no changes, +without cloning them. + +## What "equal" means + +Both sides can split and merge the same history differently, so the comparison +is per atom range, not per change: + +- **Deps** are compared at every counter where either side starts a change. Inside + a change the deps are implicitly `p@c-1`, and changes only merge when that holds, + so `deps_at` is canonical. Lamport and timestamp are not compared: the import + recomputes lamports from deps, and a merged change keeps only its first timestamp. +- **Ops** are cut at every op boundary of either side and compared by meaning + (`op_eq`): list insert values through the arena (offsets differ), text by bytes + (`unicode_start` is an arena offset), and a one-atom delete ignores the sign of + `signed_len`, because slicing a reversed delete down to one atom yields `-1` + where a forward one yields `1`. +- **JSON imports** use `ImportedValues::Lossy`: JSON text does not round-trip every + value (`NaN` becomes `null`, binary may come back as a list), so value payloads + are not compared there. Containers, positions, text, keys, ids and deps still are. +- History below the shallow root, or any local change `get_change` cannot find, + is not compared. The import then behaves as it did before this check. + +## What it does not catch + +- **Delta sync.** If the conflicting prefix is not in the import (the sender + exported from the receiver's version vector), nothing can be compared: the + receiver's vv already claims those ids. A tail position past the end of a + sequence is then rejected by `validate_diff` and rolled back (loro-dev/loro#1160). + A tail that happens to land on valid positions is applied and scrambles the + state. Detecting that needs content hashes in the version vector. +- **Imports with nothing new.** A conflicting duplicate that brings nothing new + is a no-op and is not reported. This keeps a no-op re-import as cheap as before. +- **Same-peer changes that are concurrent but contiguous** (for example `p@2` + with deps `[]` on top of a local `p@0..=1`). They do not crash, and are still + accepted. +- A peer id reused by a local commit after importing is outside import. + +## Cost + +Imports that don't overlap local history, or overlap only a little, cost the same +as before. An import that re-sends all known history *and* brings new changes +pays for comparing the overlap. On a 10k-change doc that was about +15% (55 โ†’ 64 ms) +for both "all updates + 1 change" and "snapshot + 1 change". Probe: +`crates/examples/examples/import_known_history_perf.rs`. + +## Tests + +- `crates/loro/tests/import_reused_peer_id.rs`: the #1118 repro through + updates, snapshot, `import_batch` and JSON; a same-length conflict that used to + scramble; a conflicting known change followed by a new one; skipped counters; + and re-imports that must still succeed (piecewise vs merged change stores, every + op kind including `NaN` and one-atom reversed deletes, shallow docs). +- `crates/loro-wasm/tests/import_reused_peer_id.test.ts`. diff --git a/crates/examples/examples/import_known_history_perf.rs b/crates/examples/examples/import_known_history_perf.rs new file mode 100644 index 000000000..6677ab2f4 --- /dev/null +++ b/crates/examples/examples/import_known_history_perf.rs @@ -0,0 +1,150 @@ +//! Ad-hoc perf probe for imports that overlap history the doc already has +//! (`OpLog::check_and_trim_known_part_of_changes`, loro-dev/loro#1118). +//! +//! Run on two revisions and compare. Not part of CI. +use loro::{ExportMode, LoroDoc, VersionVector}; +use std::time::{Duration, Instant}; + +/// Two peers editing text, a list and a map, syncing every few commits so the +/// history has many changes with cross-peer deps. +fn make_history(commits: usize) -> (LoroDoc, LoroDoc) { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + for i in 0..commits { + let doc = if i % 2 == 0 { &a } else { &b }; + let text = doc.get_text("text"); + let len = text.len_unicode(); + text.insert((i * 7) % (len + 1), "hello ").unwrap(); + if len > 20 && i % 3 == 0 { + text.delete((i * 5) % (len - 10), 3).unwrap(); + } + doc.get_list("list").push(i as i64).unwrap(); + doc.get_map("map") + .insert(&format!("k{}", i % 100), i as i64) + .unwrap(); + doc.commit(); + if i % 4 == 3 { + a.import(&b.export(ExportMode::updates(&a.oplog_vv())).unwrap()) + .unwrap(); + b.import(&a.export(ExportMode::updates(&b.oplog_vv())).unwrap()) + .unwrap(); + } + } + a.import(&b.export(ExportMode::updates(&a.oplog_vv())).unwrap()) + .unwrap(); + (a, b) +} + +fn time(rounds: usize, mut f: impl FnMut() -> T) -> Duration { + let mut best = Duration::MAX; + for _ in 0..rounds { + let start = Instant::now(); + std::hint::black_box(f()); + best = best.min(start.elapsed()); + } + best +} + +fn main() { + let commits: usize = std::env::var("COMMITS") + .ok() + .and_then(|x| x.parse().ok()) + .unwrap_or(20_000); + let rounds = 7; + let (source, _) = make_history(commits); + let base_snapshot = source.export(ExportMode::Snapshot).unwrap(); + let base_vv = source.oplog_vv(); + let fresh_base = || { + let doc = LoroDoc::new(); + doc.import(&base_snapshot).unwrap(); + doc + }; + + // One more change on top, from a third peer. + let next = LoroDoc::new(); + next.import(&base_snapshot).unwrap(); + next.set_peer_id(3).unwrap(); + next.get_text("text").insert(0, "new").unwrap(); + next.commit(); + let all_plus_one = next.export(ExportMode::all_updates()).unwrap(); + let snapshot_plus_one = next.export(ExportMode::Snapshot).unwrap(); + let only_new = next.export(ExportMode::updates(&base_vv)).unwrap(); + let all_known = source.export(ExportMode::all_updates()).unwrap(); + + // The last 20 commits' worth of history re-sent with the new change. + let mut resend_from = base_vv.clone(); + for (_, counter) in resend_from.iter_mut() { + *counter = (*counter - 30).max(0); + } + let resend_plus_one = next.export(ExportMode::updates(&resend_from)).unwrap(); + + // Many small non-overlapping updates, imported one by one. + let (inc_src, _) = make_history(2_000); + let mut blobs = Vec::new(); + let mut vv = VersionVector::default(); + { + let replay = LoroDoc::new(); + let json = inc_src.export_json_updates(&Default::default(), &inc_src.oplog_vv()); + for change in json.changes { + replay + .import_json_updates(json_schema_with(&json.peers, change)) + .unwrap(); + blobs.push(replay.export(ExportMode::updates(&vv)).unwrap()); + vv = replay.oplog_vv(); + } + } + + let load = std::fs::read_to_string("/proc/loadavg").ok().or_else(|| { + std::process::Command::new("sysctl") + .args(["-n", "vm.loadavg"]) + .output() + .ok() + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + }); + println!( + "commits={commits} changes={} loadavg={}", + source.len_changes(), + load.unwrap_or_default() + ); + + let report = + |name: &str, d: Duration| println!("{name:<40} {:>10.3} ms", d.as_secs_f64() * 1e3); + report( + "incremental: 2k small blobs one by one", + time(rounds, || { + let doc = LoroDoc::new(); + for b in &blobs { + doc.import(b).unwrap(); + } + doc + }), + ); + // Times the import alone, into a doc freshly loaded from the base snapshot. + let run = |name: &str, bytes: &[u8]| { + let mut best = Duration::MAX; + for _ in 0..rounds { + let doc = fresh_base(); + let start = Instant::now(); + doc.import(bytes).unwrap(); + best = best.min(start.elapsed()); + } + report(name, best); + }; + run("only new change (no overlap)", &only_new); + run("re-sent tail + new change", &resend_plus_one); + run("all updates, all known (no-op)", &all_known); + run("all updates + new change", &all_plus_one); + run("snapshot, all known (no-op)", &base_snapshot); + run("snapshot + new change", &snapshot_plus_one); +} + +fn json_schema_with(peers: &Option>, change: loro::JsonChange) -> loro::JsonSchema { + loro::JsonSchema { + schema_version: 1, + start_version: Default::default(), + peers: peers.clone(), + changes: vec![change], + } +} diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index a54380f90..e68036673 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -52,6 +52,9 @@ over graceful degradation. - Movable-list `Move`/`Set` element validation on import, plus change-store rollback records and KV block-range decoding: [../../context/movable-list-op-validation.md](../../context/movable-list-op-validation.md). +- Imported changes that reuse local op ids are checked against local history + before anything is applied (`src/oplog/known_history.rs`): + [../../context/import-peer-id-reuse.md](../../context/import-peer-id-reuse.md). - `import_batch` force-detach, its batch-wide rollback scope, and the never-exit-detached invariant: [../../context/import-batch-atomicity.md](../../context/import-batch-atomicity.md). diff --git a/crates/loro-internal/src/arena.rs b/crates/loro-internal/src/arena.rs index 03c92fc4d..4faa19afa 100644 --- a/crates/loro-internal/src/arena.rs +++ b/crates/loro-internal/src/arena.rs @@ -630,6 +630,15 @@ impl SharedArena { (self.inner.values.lock()[range]).to_vec() } + /// Whether the values in two ranges are equal, without cloning them. + pub(crate) fn value_slices_eq(&self, a: Range, b: Range) -> bool { + let values = self.inner.values.lock(); + match (values.get(a), values.get(b)) { + (Some(a), Some(b)) => a == b, + _ => false, + } + } + /// Borrow the values in `range` without cloning them (unlike /// [`Self::get_values`], which clones into a fresh `Vec`). #[inline] diff --git a/crates/loro-internal/src/encoding/fast_snapshot.rs b/crates/loro-internal/src/encoding/fast_snapshot.rs index 727770ab6..cf0542160 100644 --- a/crates/loro-internal/src/encoding/fast_snapshot.rs +++ b/crates/loro-internal/src/encoding/fast_snapshot.rs @@ -383,7 +383,6 @@ pub(crate) fn encode_updates_in_range( pub(crate) fn decode_updates(oplog: &mut OpLog, body: Bytes) -> Result, LoroError> { let mut reader: &[u8] = body.as_ref(); let mut index = 0; - let self_vv = oplog.vv(); let mut changes = Vec::new(); while !reader.is_empty() { let old_reader_len = reader.len(); @@ -400,7 +399,7 @@ pub(crate) fn decode_updates(oplog: &mut OpLog, body: Bytes) -> Result self.import_changes_and_apply_delta_to_state_if_needed( |oplog| encoding::decode_oplog_changes(oplog, parsed), + ImportedValues::Exact, origin, ), EncodeMode::Auto => { @@ -764,11 +766,14 @@ impl LoroDoc { pub(crate) fn import_changes_and_apply_delta_to_state_if_needed( &self, decode_changes: impl FnOnce(&mut OpLog) -> Result, LoroError>, + values: ImportedValues, origin: InternalString, ) -> Result { let mut oplog = self.oplog.lock(); let arena_checkpoint = oplog.arena.checkpoint_for_rollback(); - let changes = match decode_changes(&mut oplog) { + let changes = match decode_changes(&mut oplog) + .and_then(|changes| oplog.check_and_trim_known_part_of_changes(changes, values)) + { Ok(changes) => changes, Err(e) => { oplog.rollback_arena(arena_checkpoint, &self.state.lock()); @@ -950,6 +955,7 @@ impl LoroDoc { self.with_barrier(|| { let result = self.import_changes_and_apply_delta_to_state_if_needed( |oplog| crate::encoding::json_schema::decode_json_changes(json, &oplog.arena), + ImportedValues::Lossy, Default::default(), ); self.emit_events(); diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index d07beaa21..663f8cd51 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -1,4 +1,6 @@ mod change_store; +mod known_history; +pub(crate) use known_history::ImportedValues; pub(crate) mod loro_dag; mod pending_changes; diff --git a/crates/loro-internal/src/oplog/change_store.rs b/crates/loro-internal/src/oplog/change_store.rs index 7edd054a7..0b023fc1e 100644 --- a/crates/loro-internal/src/oplog/change_store.rs +++ b/crates/loro-internal/src/oplog/change_store.rs @@ -358,6 +358,12 @@ impl ChangeStore { self.external_kv.lock().export_all() } + /// Decode the changes of a snapshot imported into a non-empty doc. + /// + /// Changes this doc already has are kept, because + /// `OpLog::check_and_trim_known_part_of_changes` compares them with the local + /// history before trimming them. When nothing is new, return no changes and + /// skip the clones. pub(crate) fn decode_snapshot_for_updates( bytes: Bytes, arena: &SharedArena, @@ -365,46 +371,22 @@ impl ChangeStore { ) -> Result, LoroError> { let change_store = ChangeStore::new_mem(arena, Arc::new(AtomicI64::new(0))); let _ = change_store.import_all(bytes)?; - let mut changes = Vec::new(); + let mut has_new = false; change_store.visit_all_changes(&mut |c| { - let cnt_threshold = self_vv.get(&c.id.peer).copied().unwrap_or(0); - if c.id.counter >= cnt_threshold { - changes.push(c.clone()); - return; - } - - let change_end = c.ctr_end(); - if change_end > cnt_threshold { - changes.push(c.slice((cnt_threshold - c.id.counter) as usize, c.atom_len())); - } + has_new |= c.ctr_end() > self_vv.get(&c.id.peer).copied().unwrap_or(0); }); + let mut changes = Vec::new(); + if has_new { + change_store.visit_all_changes(&mut |c| changes.push(c.clone())); + } Ok(changes) } - pub(crate) fn decode_block_bytes( - bytes: Bytes, - arena: &SharedArena, - self_vv: &VersionVector, - ) -> LoroResult> { - let mut ans = ChangesBlockBytes::new(bytes).parse(arena)?; - if ans.is_empty() { - return Ok(ans); - } - - let start = self_vv.get(&ans[0].peer()).copied().unwrap_or(0); - ans.retain_mut(|c| { - if c.id.counter >= start { - true - } else if c.ctr_end() > start { - *c = c.slice((start - c.id.counter) as usize, c.atom_len()); - true - } else { - false - } - }); - - Ok(ans) + /// Decode an update block. Changes the doc already has are kept; see + /// [`Self::decode_snapshot_for_updates`]. + pub(crate) fn decode_block_bytes(bytes: Bytes, arena: &SharedArena) -> LoroResult> { + ChangesBlockBytes::new(bytes).parse(arena) } /// Rolls back the store and the arena (to `arena`, the checkpoint taken when the import diff --git a/crates/loro-internal/src/oplog/known_history.rs b/crates/loro-internal/src/oplog/known_history.rs new file mode 100644 index 000000000..a5ad2fd09 --- /dev/null +++ b/crates/loro-internal/src/oplog/known_history.rs @@ -0,0 +1,384 @@ +//! Imported changes that reuse op ids the doc already has. +//! +//! An import skips the part of each change the doc already has (by version vector) +//! and applies the rest on top of the local history. When two clients shared a +//! peer id, the "known" part is different history, and the rest is then applied to +//! a state it was not written against: it panicked inside state apply (poisoning +//! the doc) or silently scrambled it (loro-dev/loro#1118). See +//! `context/import-peer-id-reuse.md`. + +use loro_common::{Counter, HasCounterSpan, LoroError, LoroResult, PeerID, ID}; +use rle::{HasLength, Sliceable}; +use rustc_hash::FxHashMap; + +use crate::{ + arena::SharedArena, + change::Change, + container::list::list_op::InnerListOp, + op::{InnerContent, Op}, + version::Frontiers, + OpLog, +}; + +#[cfg(feature = "counter")] +use crate::op::FutureInnerContent; + +/// How faithfully the import format carries op values. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ImportedValues { + /// Binary encodings round-trip every value. + Exact, + /// JSON updates may have gone through JSON text, which does not round-trip + /// every value (`NaN` becomes `null`, binary may come back as a list). Values + /// are then not compared; everything else still is. + Lossy, +} + +impl OpLog { + /// Check the decoded `changes` against the history this doc already has for the + /// same op ids, then drop the parts it already has. + /// + /// Returns [`LoroError::UsedOpID`] with the first conflicting id when an imported + /// change differs from the local one (deps or op content), and + /// [`LoroError::DecodeError`] when a change skips counters of its peer (see + /// [`Self::check_changes_do_not_skip_counters`]). It reads the op log only, so a + /// caller can return the error before mutating anything. + /// + /// The comparison runs only when the import brings something new: that is what + /// would be applied on top of a conflicting prefix. An import made only of + /// known changes is a no-op either way and stays as cheap as before. + pub(crate) fn check_and_trim_known_part_of_changes( + &self, + mut changes: Vec, + values: ImportedValues, + ) -> LoroResult> { + let vv = self.vv(); + let known_end = |c: &Change| vv.get(&c.id.peer).copied().unwrap_or(0); + if changes.iter().any(|c| c.ctr_end() > known_end(c)) { + for change in changes.iter() { + if change.id.counter < known_end(change) { + self.check_known_part_of_change(change, known_end(change), values)?; + } + } + } + + changes.retain_mut(|c| { + let end = known_end(c); + if c.id.counter >= end { + true + } else if c.ctr_end() > end { + *c = c.slice((end - c.id.counter) as usize, c.atom_len()); + true + } else { + false + } + }); + self.check_changes_do_not_skip_counters(&changes)?; + Ok(changes) + } + + /// A change of peer `p` starting at counter `c > 0` is written after `p@c-1`, so + /// `p@c-1` is in the causal past of its deps. When all its deps are present but + /// `p@c-1` is not, the change skips counters and is invalid. The main pass of + /// `import_changes_to_oplog` applies any change whose deps are present and + /// would hit the counter assertion in `AppDag::update_version_on_new_change` + /// under the op log lock. (Changes parked as pending already wait for `p@c-1`, + /// see `remote_change_apply_state`.) + /// + /// This replays that pass on counter ends only: changes are visited in the same + /// order, and a change counts as applied when its deps are. + fn check_changes_do_not_skip_counters(&self, changes: &[Change]) -> LoroResult<()> { + let vv = self.vv(); + let mut ends: FxHashMap = FxHashMap::default(); + for change in changes { + let end_of = |ends: &FxHashMap, peer: &PeerID| { + ends.get(peer) + .or_else(|| vv.get(peer)) + .copied() + .unwrap_or(0) + }; + let peer = change.id.peer; + let end = end_of(&ends, &peer); + if change.ctr_end() <= end || self.dag.import_deps_before_shallow_root(&change.deps) { + continue; + } + + if !change + .deps + .iter() + .all(|dep| dep.counter < end_of(&ends, &dep.peer)) + { + // Parked as pending. + continue; + } + + if change.id.counter > end { + return Err(LoroError::DecodeError( + format!( + "Invalid change {}: its deps are present but {} is not, so it skips counters of its peer", + change.id, + ID::new(peer, change.id.counter - 1), + ) + .into_boxed_str(), + )); + } + + ends.insert(peer, change.ctr_end()); + } + + Ok(()) + } + + fn check_known_part_of_change( + &self, + change: &Change, + known_end: Counter, + values: ImportedValues, + ) -> LoroResult<()> { + let peer = change.id.peer; + // History before the shallow root is not stored, so it cannot be compared. + let shallow_start = self.shallow_since_vv().get(&peer).copied().unwrap_or(0); + let end = change.ctr_end().min(known_end); + let mut ctr = change.id.counter.max(shallow_start); + while ctr < end { + // Missing local history cannot be compared; keep the old behavior for it. + let Some(local) = self.change_store.get_change(ID::new(peer, ctr)) else { + break; + }; + let seg_end = end.min(local.ctr_end()); + if seg_end <= ctr { + break; + } + + if deps_at(change, ctr) != deps_at(&local, ctr) { + return Err(LoroError::UsedOpID { + id: ID::new(peer, ctr), + }); + } + + if let Some(bad) = first_mismatch(&self.arena, values, change, &local, ctr, seg_end) { + return Err(LoroError::UsedOpID { + id: ID::new(peer, bad), + }); + } + + ctr = seg_end; + } + + Ok(()) + } +} + +/// The deps of the op at `ctr`. Inside a change every op depends on the previous +/// one, and changes only merge when that holds, so this does not depend on how +/// either side split its history into changes. +fn deps_at(change: &Change, ctr: Counter) -> Frontiers { + if ctr == change.id.counter { + change.deps.clone() + } else { + Frontiers::from_id(ID::new(change.id.peer, ctr - 1)) + } +} + +/// Walk the ops of `a` and `b` over `[start, end)` and return the first counter at +/// which they differ. Each side may split or merge the ops differently, so both +/// are cut at every boundary of either side before comparing. +fn first_mismatch( + arena: &SharedArena, + values: ImportedValues, + a: &Change, + b: &Change, + start: Counter, + end: Counter, +) -> Option { + let mut a_ops = OpCursor::new(a, start); + let mut b_ops = OpCursor::new(b, start); + let mut ctr = start; + while ctr < end { + let (Some(a_op), Some(b_op)) = (a_ops.op_at(ctr), b_ops.op_at(ctr)) else { + return Some(ctr); + }; + let len = (end - ctr) + .min(a_op.ctr_end() - ctr) + .min(b_op.ctr_end() - ctr); + let a_slice = slice_op(a_op, ctr, len); + let b_slice = slice_op(b_op, ctr, len); + if !op_eq(arena, values, &a_slice, &b_slice) { + // Error path only: find the exact atom. + let offset = (0..len) + .find(|&i| { + !op_eq( + arena, + values, + &slice_op(a_op, ctr + i, 1), + &slice_op(b_op, ctr + i, 1), + ) + }) + .unwrap_or(0); + return Some(ctr + offset); + } + ctr += len; + } + + None +} + +struct OpCursor<'a> { + ops: &'a [Op], + index: usize, +} + +impl<'a> OpCursor<'a> { + fn new(change: &'a Change, start: Counter) -> Self { + let ops = change.ops.vec().as_slice(); + let index = ops.partition_point(|op| op.ctr_end() <= start); + Self { ops, index } + } + + /// The op containing `ctr`. Counters only move forward. + fn op_at(&mut self, ctr: Counter) -> Option<&'a Op> { + while let Some(op) = self.ops.get(self.index) { + if op.ctr_end() > ctr { + return (op.counter <= ctr).then_some(op); + } + self.index += 1; + } + None + } +} + +fn slice_op(op: &Op, ctr: Counter, len: Counter) -> std::borrow::Cow<'_, Op> { + let from = (ctr - op.counter) as usize; + let to = from + len as usize; + if from == 0 && to == op.atom_len() { + std::borrow::Cow::Borrowed(op) + } else { + std::borrow::Cow::Owned(op.slice(from, to)) + } +} + +/// Whether two ops with the same id range are the same op. Compares what the op +/// means, not how it is stored: arena offsets and the direction of a one-atom +/// delete are representation details. +fn op_eq(arena: &SharedArena, values: ImportedValues, a: &Op, b: &Op) -> bool { + if a.container != b.container || a.atom_len() != b.atom_len() { + return false; + } + + let exact = values == ImportedValues::Exact; + + match (&a.content, &b.content) { + (InnerContent::List(a), InnerContent::List(b)) => list_op_eq(arena, exact, a, b), + (InnerContent::Map(a), InnerContent::Map(b)) => { + a.key == b.key + && a.value.is_some() == b.value.is_some() + && (!exact || a.value == b.value) + } + (InnerContent::Tree(a), InnerContent::Tree(b)) => a == b, + (InnerContent::Future(a), InnerContent::Future(b)) => match (a, b) { + #[cfg(feature = "counter")] + (FutureInnerContent::Counter(a), FutureInnerContent::Counter(b)) => { + !exact || a == b || (a.is_nan() && b.is_nan()) + } + ( + crate::op::FutureInnerContent::Unknown { + prop: a_prop, + value: a_value, + }, + crate::op::FutureInnerContent::Unknown { + prop: b_prop, + value: b_value, + }, + ) => a_prop == b_prop && (!exact || a_value == b_value), + #[allow(unreachable_patterns)] + _ => false, + }, + _ => false, + } +} + +fn list_op_eq(arena: &SharedArena, exact: bool, a: &InnerListOp, b: &InnerListOp) -> bool { + match (a, b) { + ( + InnerListOp::Insert { + slice: a_slice, + pos: a_pos, + }, + InnerListOp::Insert { + slice: b_slice, + pos: b_pos, + }, + ) => { + a_pos == b_pos + && (!exact || arena.value_slices_eq(a_slice.to_range(), b_slice.to_range())) + } + ( + InnerListOp::InsertText { + slice: a_slice, + unicode_len: a_len, + pos: a_pos, + .. + }, + InnerListOp::InsertText { + slice: b_slice, + unicode_len: b_len, + pos: b_pos, + .. + }, + ) => a_pos == b_pos && a_len == b_len && a_slice[..] == b_slice[..], + (InnerListOp::Delete(a), InnerListOp::Delete(b)) => { + // A one-atom delete has no direction: `signed_len` 1 and -1 delete the + // same element, and which one is stored depends on how it was sliced. + a.id_start == b.id_start + && a.span.pos == b.span.pos + && (a.span.signed_len == b.span.signed_len + || (a.span.signed_len.abs() == 1 && b.span.signed_len.abs() == 1)) + } + ( + InnerListOp::Move { + from: a_from, + elem_id: a_elem, + to: a_to, + }, + InnerListOp::Move { + from: b_from, + elem_id: b_elem, + to: b_to, + }, + ) => a_from == b_from && a_elem == b_elem && a_to == b_to, + ( + InnerListOp::Set { + elem_id: a_elem, + value: a_value, + }, + InnerListOp::Set { + elem_id: b_elem, + value: b_value, + }, + ) => a_elem == b_elem && (!exact || a_value == b_value), + ( + InnerListOp::StyleStart { + start: a_start, + end: a_end, + key: a_key, + value: a_value, + info: a_info, + }, + InnerListOp::StyleStart { + start: b_start, + end: b_end, + key: b_key, + value: b_value, + info: b_info, + }, + ) => { + a_start == b_start + && a_end == b_end + && a_key == b_key + && (!exact || a_value == b_value) + && a_info == b_info + } + (InnerListOp::StyleEnd, InnerListOp::StyleEnd) => true, + _ => false, + } +} diff --git a/crates/loro-wasm/tests/import_reused_peer_id.test.ts b/crates/loro-wasm/tests/import_reused_peer_id.test.ts new file mode 100644 index 000000000..aafee4a3e --- /dev/null +++ b/crates/loro-wasm/tests/import_reused_peer_id.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// Two docs that shared a peer id wrote different ops under the same ids. Importing +// one into the other used to trap the WASM instance ("RuntimeError: unreachable") +// or scramble the text. It must throw and leave the doc unchanged and usable. +// See loro-dev/loro#1118 and context/import-peer-id-reuse.md. + +function history(): LoroDoc { + const doc = new LoroDoc(); + doc.setPeerId(7); + doc.getMap("m").set("k", 0); + doc.getText("t").insert(0, "a"); + doc.commit(); // 7@0, 7@1 + return doc; +} + +function conflicting(text: string): LoroDoc { + const doc = new LoroDoc(); + doc.setPeerId(7); + doc.getText("t").insert(0, text); + doc.commit(); + return doc; +} + +function expectRejected(doc: LoroDoc, bytes: Uint8Array) { + const value = doc.toJSON(); + const version = doc.version().toJSON(); + expect(() => doc.import(bytes)).toThrow(/has been used/); + expect(doc.toJSON()).toEqual(value); + expect(doc.version().toJSON()).toEqual(version); + + // Still usable: local edits and legitimate remote updates apply. + doc.getText("after").insert(0, "local"); + doc.commit(); + const remote = new LoroDoc(); + remote.setPeerId(1000); + remote.import(doc.export({ mode: "snapshot" })); + remote.getMap("remote").set("k", "v"); + remote.commit(); + doc.import(remote.export({ mode: "update", from: doc.version() })); + expect(doc.toJSON()).toEqual(remote.toJSON()); +} + +describe("importing history that reuses local op ids", () => { + it("throws instead of trapping (issue repro)", () => { + const other = conflicting("xyz"); // 7@0..=2 + expectRejected(history(), other.export({ mode: "update" })); + expectRejected(history(), other.export({ mode: "snapshot" })); + }); + + it("throws instead of scrambling the text", () => { + const doc = new LoroDoc(); + doc.setPeerId(7); + doc.getText("t").insert(0, "hello"); + doc.commit(); + expectRejected(doc, conflicting("hellO world").export({ mode: "update" })); + expect(doc.getText("t").toString()).toBe("hello"); + }); + + it("still accepts overlapping re-imports of the same history", () => { + const source = new LoroDoc(); + source.setPeerId(1); + const text = source.getText("t"); + text.insert(0, "hello world"); + source.commit(); + const target = new LoroDoc(); + target.import(source.export({ mode: "update" })); + text.delete(0, 6); + text.insert(0, "hi "); + source.commit(); + target.import(source.export({ mode: "update" })); + target.import(source.export({ mode: "snapshot" })); + expect(target.toJSON()).toEqual(source.toJSON()); + }); +}); diff --git a/crates/loro/tests/import_reused_peer_id.rs b/crates/loro/tests/import_reused_peer_id.rs new file mode 100644 index 000000000..796492a70 --- /dev/null +++ b/crates/loro/tests/import_reused_peer_id.rs @@ -0,0 +1,463 @@ +//! Importing history whose op ids are already used locally by *different* ops +//! (two clients sharing a peer id) must fail with an `Err` before anything is +//! applied, instead of panicking, poisoning the doc, or scrambling its state. +//! See loro-dev/loro#1118. + +use loro::{ + ExportMode, LoroDoc, LoroError, LoroResult, LoroValue, ToJson, TreeParentId, VersionVector, ID, +}; +use pretty_assertions::assert_eq; + +struct Before { + value: serde_json::Value, + vv: VersionVector, + frontiers: loro::Frontiers, +} + +fn snapshot_of(doc: &LoroDoc) -> Before { + Before { + value: doc.get_deep_value().to_json_value(), + vv: doc.oplog_vv(), + frontiers: doc.oplog_frontiers(), + } +} + +fn assert_unchanged(doc: &LoroDoc, before: &Before) { + assert_eq!(doc.get_deep_value().to_json_value(), before.value); + assert_eq!(doc.oplog_vv(), before.vv); + assert_eq!(doc.oplog_frontiers(), before.frontiers); + assert_eq!(doc.state_frontiers(), before.frontiers); +} + +fn assert_used_op_id(err: LoroError, expected: ID) { + match err { + LoroError::UsedOpID { id } => assert_eq!(id, expected), + other => panic!("expected UsedOpID({expected}), got {other:?}"), + } +} + +/// After a rejected import the doc keeps working: it accepts local edits and +/// legitimate remote updates, and it can be exported and dropped. +fn assert_still_usable(doc: &LoroDoc) { + doc.get_text("after").insert(0, "local").unwrap(); + doc.commit(); + + let remote = LoroDoc::new(); + remote.set_peer_id(1000).unwrap(); + remote + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + remote.get_map("remote").insert("k", "v").unwrap(); + remote.commit(); + doc.import(&remote.export(ExportMode::updates(&doc.oplog_vv())).unwrap()) + .unwrap(); + assert_eq!( + doc.get_deep_value().to_json_value(), + remote.get_deep_value().to_json_value() + ); + + let copy = LoroDoc::new(); + copy.import(&doc.export(ExportMode::Snapshot).unwrap()) + .unwrap(); + assert_eq!( + copy.get_deep_value().to_json_value(), + doc.get_deep_value().to_json_value() + ); +} + +/// The reproduction from loro-dev/loro#1118: `history` and `other` both wrote +/// 7@0..=1, and `other` went on to 7@2. The import used to trim the known part of +/// `other`'s change and apply its tail (an insert at position 2) on top of +/// `history`'s text, which has length 1. +#[test] +fn import_overlapping_change_with_different_content_is_rejected() -> LoroResult<()> { + let history = LoroDoc::new(); + history.set_peer_id(7)?; + history.get_map("m").insert("k", 0)?; + history.get_text("t").insert(0, "a")?; + history.commit(); // 7@0, 7@1 + + let other = LoroDoc::new(); + other.set_peer_id(7)?; + other.get_text("t").insert(0, "xyz")?; + other.commit(); // 7@0..=2 + + let before = snapshot_of(&history); + for update in [ + other.export(ExportMode::all_updates())?, + other.export(ExportMode::Snapshot)?, + ] { + let err = history.import(&update).unwrap_err(); + assert_used_op_id(err, ID::new(7, 0)); + assert_unchanged(&history, &before); + + let err = history.import_batch(&[update]).unwrap_err(); + assert_used_op_id(err, ID::new(7, 0)); + assert_unchanged(&history, &before); + assert!(!history.is_detached()); + } + + let json = other.export_json_updates(&Default::default(), &other.oplog_vv()); + let err = history.import_json_updates(json).unwrap_err(); + assert_used_op_id(err, ID::new(7, 0)); + assert_unchanged(&history, &before); + + assert_still_usable(&history); + Ok(()) +} + +/// Same shape, but the shifted tail lands on a valid position, so the import used +/// to succeed and silently scramble the text. +#[test] +fn import_overlapping_change_with_same_shape_is_rejected() -> LoroResult<()> { + let history = LoroDoc::new(); + history.set_peer_id(7)?; + history.get_text("t").insert(0, "hello")?; + history.commit(); // 7@0..=4 + + let other = LoroDoc::new(); + other.set_peer_id(7)?; + other.get_text("t").insert(0, "hellO world")?; + other.commit(); // 7@0..=10 + + let before = snapshot_of(&history); + let err = history + .import(&other.export(ExportMode::all_updates())?) + .unwrap_err(); + assert_used_op_id(err, ID::new(7, 4)); + assert_unchanged(&history, &before); + assert_still_usable(&history); + Ok(()) +} + +/// The conflicting part is a separate change that is entirely known locally, and +/// the new part starts exactly at the local counter end, so there is no partially +/// overlapping change to trim. +#[test] +fn import_known_conflicting_change_followed_by_new_change_is_rejected() -> LoroResult<()> { + let history = LoroDoc::new(); + history.set_peer_id(7)?; + history.get_text("t").insert(0, "ab")?; + history.commit(); // 7@0..=1 + + let other = LoroDoc::new(); + other.set_peer_id(7)?; + other.get_text("t").insert(0, "xy")?; + other.commit(); // 7@0..=1 + // Put a change from another peer in between so 7@2 starts a new change. + let helper = LoroDoc::new(); + helper.set_peer_id(9)?; + helper.import(&other.export(ExportMode::all_updates())?)?; + helper.get_map("m").insert("k", 1)?; + helper.commit(); + other.import(&helper.export(ExportMode::all_updates())?)?; + other.get_text("t").insert(2, "z")?; + other.commit(); // 7@2, deps [7@1, 9@0] + + let before = snapshot_of(&history); + let err = history + .import(&other.export(ExportMode::all_updates())?) + .unwrap_err(); + assert_used_op_id(err, ID::new(7, 0)); + assert_unchanged(&history, &before); + assert_still_usable(&history); + Ok(()) +} + +/// A peer-id collision in history the import does not build on (nothing new is +/// imported) is not detected, and the import stays a no-op. +#[test] +fn import_with_only_known_conflicting_changes_is_a_no_op() -> LoroResult<()> { + let history = LoroDoc::new(); + history.set_peer_id(7)?; + history.get_text("t").insert(0, "ab")?; + history.commit(); + + let other = LoroDoc::new(); + other.set_peer_id(7)?; + other.get_text("t").insert(0, "xy")?; + other.commit(); + + let before = snapshot_of(&history); + let status = history.import(&other.export(ExportMode::all_updates())?)?; + assert!(status.success.is_empty()); + assert!(status.pending.is_none()); + assert_unchanged(&history, &before); + Ok(()) +} + +/// Builds a doc with every kind of op, syncing `replica` piecewise along the way so +/// its change store splits and merges the history differently from `source`'s. +fn build_source_and_piecewise_replica() -> LoroResult<(LoroDoc, LoroDoc)> { + let source = LoroDoc::new(); + source.set_peer_id(1)?; + let replica = LoroDoc::new(); + replica.set_peer_id(2)?; + let sync = |from: &LoroDoc, to: &LoroDoc| -> LoroResult<()> { + to.import(&from.export(ExportMode::updates(&to.oplog_vv()))?)?; + Ok(()) + }; + + let text = source.get_text("text"); + text.insert(0, "hello wรถrld ๐Ÿ˜€")?; + source.commit(); + sync(&source, &replica)?; + text.delete(3, 4)?; // forward + for i in (0..3).rev() { + text.delete(i, 1)?; // backward, single-atom deletes + } + text.mark(0..2, "bold", true)?; + source.commit(); + sync(&source, &replica)?; + + let list = source.get_list("list"); + list.push(1)?; + list.push("two")?; + list.push(LoroValue::Double(f64::NAN))?; + list.delete(0, 1)?; + let map = source.get_map("map"); + map.insert("nan", f64::NAN)?; + map.insert("bin", vec![1u8, 2, 3])?; + map.delete("bin")?; + source.commit(); + sync(&source, &replica)?; + + let mov = source.get_movable_list("mov"); + mov.push("a")?; + mov.push("b")?; + mov.push("c")?; + mov.mov(0, 2)?; + mov.set(1, "B")?; + mov.delete(0, 1)?; + let tree = source.get_tree("tree"); + let root = tree.create(TreeParentId::Root)?; + let child = tree.create(root)?; + tree.mov(child, TreeParentId::Root)?; + tree.delete(root)?; + let counter = source.get_counter("counter"); + counter.increment(1.5)?; + counter.decrement(0.5)?; + source.commit(); + + // Changes from the replica's own peer in between, then more from `source`. + sync(&source, &replica)?; + replica.get_text("text").insert(0, ">")?; + replica.commit(); + sync(&replica, &source)?; + text.insert(1, "more")?; + text.delete(0, 2)?; + source.commit(); + Ok((source, replica)) +} + +/// Re-importing history the doc already has must still succeed, whatever the +/// export format and however the change store split or merged it. +#[test] +fn reimporting_known_history_with_new_changes_still_succeeds() -> LoroResult<()> { + let (source, replica) = build_source_and_piecewise_replica()?; + let expected = source.get_deep_value().to_json_value(); + let replica_vv = replica.oplog_vv(); + assert_ne!(replica_vv, source.oplog_vv()); + + let full_updates = source.export(ExportMode::all_updates())?; + let snapshot = source.export(ExportMode::Snapshot)?; + let json = source.export_json_updates(&Default::default(), &source.oplog_vv()); + let json_updates = serde_json::to_string(&json).unwrap(); + + for (name, import) in [ + ( + "updates", + Box::new(|doc: &LoroDoc| doc.import(&full_updates).map(|_| ())) + as Box LoroResult<()>>, + ), + ( + "snapshot", + Box::new(|doc: &LoroDoc| doc.import(&snapshot).map(|_| ())), + ), + ( + "batch", + Box::new(|doc: &LoroDoc| { + doc.import_batch(&[full_updates.clone(), snapshot.clone()]) + .map(|_| ()) + }), + ), + ( + "json", + Box::new(|doc: &LoroDoc| doc.import_json_updates(json_updates.as_str()).map(|_| ())), + ), + ] { + let doc = LoroDoc::new(); + doc.import(&replica.export(ExportMode::Snapshot)?)?; + import(&doc).unwrap_or_else(|e| panic!("{name}: {e:?}")); + assert_eq!(doc.get_deep_value().to_json_value(), expected, "{name}"); + + // The replica itself, whose change store holds the history split up. + let doc = LoroDoc::new(); + doc.import(&replica.export(ExportMode::all_updates())?)?; + doc.import(&replica.export(ExportMode::updates(&doc.oplog_vv()))?)?; + import(&doc).unwrap_or_else(|e| panic!("{name} (updates base): {e:?}")); + assert_eq!(doc.get_deep_value().to_json_value(), expected, "{name}"); + } + + // And in the other direction: the merged history into the piecewise one. + replica.import(&full_updates)?; + assert_eq!(replica.get_deep_value().to_json_value(), expected); + Ok(()) +} + +/// Two exports of the same peer's history that were cut at different points, +/// imported so that every imported change straddles the local counter end. +#[test] +fn reimporting_overlapping_ranges_of_the_same_history_succeeds() -> LoroResult<()> { + let source = LoroDoc::new(); + source.set_peer_id(1)?; + let text = source.get_text("text"); + let mut versions = vec![source.oplog_vv()]; + for i in 0..20 { + text.insert(text.len_unicode(), &format!("{i},"))?; + if i % 3 == 0 { + text.delete(0, 1)?; + } + source.commit(); + versions.push(source.oplog_vv()); + } + + let target = LoroDoc::new(); + for window in versions.windows(3) { + // Export [v_i, v_{i+2}) while the target already has up to v_{i+1}. + let update = source.export(ExportMode::updates(&window[0]))?; + target.import(&update)?; + } + assert_eq!( + target.get_deep_value().to_json_value(), + source.get_deep_value().to_json_value() + ); + Ok(()) +} + +/// The issue also noted that dropping the doc aborted the process when the +/// failed import panicked inside `catch_unwind`. With an `Err` there is no panic; +/// make sure dropping inside the closure is fine. +#[test] +fn rejected_import_inside_catch_unwind_can_drop_the_doc() { + let result = std::panic::catch_unwind(|| { + let history = LoroDoc::new(); + history.set_peer_id(7).unwrap(); + history.get_map("m").insert("k", 0).unwrap(); + history.get_text("t").insert(0, "a").unwrap(); + history.commit(); + + let other = LoroDoc::new(); + other.set_peer_id(7).unwrap(); + other.get_text("t").insert(0, "xyz").unwrap(); + other.commit(); + + let r = history.import(&other.export(ExportMode::all_updates()).unwrap()); + drop(history); + r.is_err() + }); + assert_eq!(result.ok(), Some(true)); +} + +/// JSON updates for `doc`'s last change, moved to `peer` at `counter` with `deps`. +fn forged_json_change(doc: &LoroDoc, peer: u64, counter: i32, deps: Vec) -> loro::JsonSchema { + let mut json = doc.export_json_updates(&Default::default(), &doc.oplog_vv()); + json.changes.drain(..json.changes.len() - 1); + let change = json.changes.last_mut().unwrap(); + let shift = counter - change.id.counter; + change.id.counter = counter; + change.deps = deps; + for op in change.ops.iter_mut() { + op.counter += shift; + } + json.peers = Some(vec![peer]); + change.id.peer = 0; + json +} + +/// A change whose deps are all present but whose own peer's previous counter is +/// not skips counters. It used to trip a DAG assertion under the op log lock, +/// poisoning the doc (and aborting when it was dropped). +#[test] +fn import_change_that_skips_counters_is_rejected() -> LoroResult<()> { + let writer = LoroDoc::new(); + writer.set_peer_id(8)?; + writer.get_text("t").insert(0, "xyz")?; + writer.commit(); + + // Into a doc that has none of peer 7, and into one that has 7@0..=1. + for local_len in [0, 2] { + let doc = LoroDoc::new(); + doc.set_peer_id(7)?; + if local_len > 0 { + doc.get_text("t").insert(0, &"a".repeat(local_len))?; + doc.commit(); + } + let before = snapshot_of(&doc); + let err = doc + .import_json_updates(forged_json_change(&writer, 7, 5, vec![])) + .unwrap_err(); + assert!( + matches!(&err, LoroError::DecodeError(msg) if msg.contains("skips counters")), + "{err:?}" + ); + assert_unchanged(&doc, &before); + doc.set_peer_id(100)?; + assert_still_usable(&doc); + } + + // The deps are satisfied by an earlier change of the same import. + let helper = LoroDoc::new(); + helper.set_peer_id(9)?; + helper.get_map("m").insert("k", 1)?; + helper.commit(); + let mut json = helper.export_json_updates(&Default::default(), &helper.oplog_vv()); + let forged = forged_json_change(&writer, 7, 5, vec![ID::new(1, 0)]); + // Peer index 1 is peer 7, index 0 is peer 9. + json.peers = Some(vec![9, 7]); + let mut change = forged.changes.into_iter().next().unwrap(); + change.id.peer = 1; + change.deps = vec![ID::new(0, 0)]; + json.changes.push(change); + + let doc = LoroDoc::new(); + let before = snapshot_of(&doc); + let err = doc.import_json_updates(json).unwrap_err(); + assert!(matches!(err, LoroError::DecodeError(_)), "{err:?}"); + assert_unchanged(&doc, &before); + assert_still_usable(&doc); + Ok(()) +} + +/// Known history below a shallow root cannot be compared and must not be +/// rejected for that. +#[test] +fn reimporting_into_a_shallow_doc_still_succeeds() -> LoroResult<()> { + let source = LoroDoc::new(); + source.set_peer_id(1)?; + let text = source.get_text("text"); + for i in 0..10 { + text.insert(0, &format!("{i}"))?; + source.commit(); + } + let shallow = LoroDoc::new(); + shallow.import(&source.export(ExportMode::shallow_snapshot(&source.oplog_frontiers()))?)?; + text.insert(0, "new")?; + source.commit(); + + shallow.import(&source.export(ExportMode::updates(&shallow.shallow_since_vv().to_vv()))?)?; + assert_eq!( + shallow.get_deep_value().to_json_value(), + source.get_deep_value().to_json_value() + ); + + // The full history, which straddles the shallow root, plus one more change. + text.insert(0, "more")?; + source.commit(); + shallow.import(&source.export(ExportMode::all_updates())?)?; + assert_eq!( + shallow.get_deep_value().to_json_value(), + source.get_deep_value().to_json_value() + ); + Ok(()) +} diff --git a/crates/loro/tests/movable_list_invalid_ops.rs b/crates/loro/tests/movable_list_invalid_ops.rs index 1e7e6515d..991082d62 100644 --- a/crates/loro/tests/movable_list_invalid_ops.rs +++ b/crates/loro/tests/movable_list_invalid_ops.rs @@ -426,8 +426,10 @@ fn move_of_element_deleted_before_shallow_root_is_rejected() { /// (and no validation) was opened. With `two_peers`, `C1` is by peer 2 and `C2` by /// peer 3. fn forged_after_map_change(container: &str, forged_op: Value, two_peers: bool) -> String { + // Peer 2 is taken by `base_with_concurrent_edit`; reusing its `2@0` for other + // content is rejected as `UsedOpID` before the movable-list check runs. let (peers, c1_id, c2_id, c2_deps, c2_counter) = if two_peers { - (json!(["1", "2", "3"]), "0@1", "0@2", "0@1", 0) + (json!(["1", "4", "3"]), "0@1", "0@2", "0@1", 0) } else { (json!(["1"]), "4@0", "5@0", "4@0", 5) }; diff --git a/docs/encoding.md b/docs/encoding.md index 5d52cc82c..138b013df 100644 --- a/docs/encoding.md +++ b/docs/encoding.md @@ -543,11 +543,13 @@ The updates APIs differ only in which changes are selected: shallow root), while `UpdatesInRange` emits the normalized requested ID spans. Both produce the exact body above. -On import, each decoded block is filtered against the receiver's current +On import, the decoded changes are filtered against the receiver's current VersionVector. A change entirely before the receiver's exclusive counter end is -dropped; a change crossing that boundary is sliced at the boundary. The -remaining changes from every block are then sorted by starting Lamport value -before application. Block order in the byte stream is therefore not application +dropped; a change crossing that boundary is sliced at the boundary. Before +that, when the import brings anything new, the part of each change the receiver +already has is compared with its local history, and a mismatch (a reused peer +id) fails the import with `UsedOpID`. The changes from every block are sorted by +starting Lamport value before application. Block order in the byte stream is therefore not application order. These selection and ordering rules are API behavior around the wire format; they do not add fields to it. @@ -555,8 +557,9 @@ Selection: [`ChangeStore::export_blocks_from`](../crates/loro-internal/src/oplog/change_store.rs#L543-L577), [`export_blocks_in_range`](../crates/loro-internal/src/oplog/change_store.rs#L203-L233). Import filtering and ordering: -[`ChangeStore::decode_block_bytes`](../crates/loro-internal/src/oplog/change_store.rs#L278-L300), -[`fast_snapshot.rs::decode_updates`](../crates/loro-internal/src/encoding/fast_snapshot.rs#L360-L388). +[`fast_snapshot.rs::decode_updates`](../crates/loro-internal/src/encoding/fast_snapshot.rs), +[`OpLog::check_and_trim_known_part_of_changes`](../crates/loro-internal/src/oplog/known_history.rs) +(see [`context/import-peer-id-reuse.md`](../context/import-peer-id-reuse.md)). ## 7. Postcard change block From ae1a9bdf617ec3e40f56b789eb98e60e00e43e90 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:04:16 +0800 Subject: [PATCH 02/10] fix: drop subscriber callbacks outside the subscriber set lock (#1162) Dropping a subscriber's callback runs arbitrary code; when it owns another Subscription or an UndoManager on the same doc, their drop re-entered the set's mutex on the same thread and deadlocked. The unsubscribe closure, retain's dropped-subscriber sweep, and remove() now move the removed subscribers out and drop them after releasing the lock. Co-Authored-By: Claude Opus 5.5 --- .changeset/subscription-drop-deadlock.md | 5 ++ .../loro-internal/src/utils/subscription.rs | 18 ++++- .../loro/tests/subscription_drop_reentrant.rs | 79 +++++++++++++++++++ 3 files changed, 99 insertions(+), 3 deletions(-) create mode 100644 .changeset/subscription-drop-deadlock.md create mode 100644 crates/loro/tests/subscription_drop_reentrant.rs diff --git a/.changeset/subscription-drop-deadlock.md b/.changeset/subscription-drop-deadlock.md new file mode 100644 index 000000000..fc10d46fb --- /dev/null +++ b/.changeset/subscription-drop-deadlock.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix a deadlock when a subscription is dropped and its callback owned another subscription or an `UndoManager` on the same document: subscriber callbacks are now dropped after the subscriber set's lock is released. diff --git a/crates/loro-internal/src/utils/subscription.rs b/crates/loro-internal/src/utils/subscription.rs index 3fc19b1ff..7f58f5235 100644 --- a/crates/loro-internal/src/utils/subscription.rs +++ b/crates/loro-internal/src/utils/subscription.rs @@ -323,10 +323,15 @@ where }; if let Either::Left(subscribers) = subscribers { - subscribers.remove(&subscriber_id); + let removed = subscribers.remove(&subscriber_id); if subscribers.is_empty() { lock.subscribers.remove(&emitter_key); } + // Drop the subscriber only after releasing the lock: its callback + // may own another `Subscription` (or an `UndoManager`) on this set, + // whose drop takes this lock again (loro-dev/loro#1162). + drop(lock); + drop(removed); return; } @@ -374,7 +379,10 @@ where let subscribers = lock.subscribers.remove(emitter); // A subscriber parked mid-emit belongs to the emitter being // removed; it never became visible, so it goes with it. - lock.pending_subscribers.remove(emitter); + let parked = lock.pending_subscribers.remove(emitter); + // Drop callbacks outside the lock; see the unsubscribe closure in `insert`. + drop(lock); + drop(parked); subscribers .and_then(|x| x.left().map(|s| s.into_values())) .into_iter() @@ -453,9 +461,10 @@ where } // Remove any dropped subscriptions that were dropped while invoking the callback. + let mut removed = Vec::new(); for (dropped_emitter, dropped_subscription_id) in mem::take(&mut lock.dropped_subscribers) { if *emitter == dropped_emitter { - subscribers.remove(&dropped_subscription_id); + removed.extend(subscribers.remove(&dropped_subscription_id)); } else { lock.dropped_subscribers .insert((dropped_emitter, dropped_subscription_id)); @@ -464,6 +473,9 @@ where lock.subscribers .insert(emitter.clone(), Either::Left(subscribers)); + // Drop callbacks outside the lock; see the unsubscribe closure in `insert`. + drop(lock); + drop(removed); Ok(()) } diff --git a/crates/loro/tests/subscription_drop_reentrant.rs b/crates/loro/tests/subscription_drop_reentrant.rs new file mode 100644 index 000000000..1f64f8016 --- /dev/null +++ b/crates/loro/tests/subscription_drop_reentrant.rs @@ -0,0 +1,79 @@ +//! Dropping a subscriber's callback runs arbitrary code. When that callback owns +//! another `Subscription` or an `UndoManager` on the same doc, their drop takes the +//! subscriber set's lock again, so the set must not hold it while dropping +//! callbacks. See loro-dev/loro#1162. + +use loro::{ContainerTrait, LoroDoc, UndoManager}; +use std::sync::{mpsc, Arc, Mutex}; +use std::time::Duration; + +fn returns_within_5s(f: impl FnOnce() + Send + 'static) -> bool { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + f(); + let _ = tx.send(()); + }); + rx.recv_timeout(Duration::from_secs(5)).is_ok() +} + +#[test] +fn dropping_a_callback_that_owns_another_subscription_does_not_deadlock() { + let doc = LoroDoc::new(); + let inner = doc.subscribe_root(Arc::new(|_| {})); + let outer = doc.subscribe_root(Arc::new(move |_| { + let _owned = &inner; + })); + assert!(returns_within_5s(move || drop(outer))); +} + +#[test] +fn dropping_a_callback_that_owns_an_undo_manager_does_not_deadlock() { + let doc = LoroDoc::new(); + let undo = Mutex::new(UndoManager::new(&doc)); + let outer = doc.subscribe_root(Arc::new(move |_| { + let _owned = &undo; + })); + assert!(returns_within_5s(move || drop(outer))); +} + +/// A container subscription whose callback owns a root subscription. +#[test] +fn dropping_a_container_callback_that_owns_a_root_subscription_does_not_deadlock() { + let doc = LoroDoc::new(); + let text = doc.get_text("t"); + let inner = doc.subscribe_root(Arc::new(|_| {})); + let outer = doc.subscribe( + &text.id(), + Arc::new(move |_| { + let _owned = &inner; + }), + ); + assert!(returns_within_5s(move || drop(outer))); +} + +/// The subscription is dropped while its own callback runs, so `retain` removes it +/// after the emit, together with the subscription its callback owns. +#[test] +fn unsubscribing_during_emit_drops_owned_subscription_without_deadlock() { + assert!(returns_within_5s(|| { + let doc = LoroDoc::new(); + let inner = doc.subscribe_root(Arc::new(|_| {})); + let slot: Arc>> = Arc::new(Mutex::new(None)); + let slot_in_cb = slot.clone(); + let outer = doc.subscribe_root(Arc::new(move |_| { + let _owned = &inner; + // Drop our own subscription mid-emit. + slot_in_cb.lock().unwrap().take(); + })); + *slot.lock().unwrap() = Some(outer); + doc.get_text("t").insert(0, "a").unwrap(); + doc.commit(); + // The doc still works and emits to new subscribers. + let hits = Arc::new(Mutex::new(0)); + let hits_in_cb = hits.clone(); + let _sub = doc.subscribe_root(Arc::new(move |_| *hits_in_cb.lock().unwrap() += 1)); + doc.get_text("t").insert(0, "b").unwrap(); + doc.commit(); + assert_eq!(*hits.lock().unwrap(), 1); + })); +} From 11e8518a41d6778fd7cfb9e1f2d95c193e48092f Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:11:26 +0800 Subject: [PATCH 03/10] fix: export several non-contiguous spans of one peer in updates_in_range (#1155) export_blocks_in_range inserted every selected change into one temporary change store, which requires each peer's counters to be contiguous, so two non-contiguous spans of one peer panicked under the op log lock. Merge the ranges per peer and put the k-th range of every peer into the k-th store; the blocks of all stores are written one after another. Co-Authored-By: Claude Opus 5.5 --- .changeset/export-updates-in-range-spans.md | 5 + .../loro-internal/src/oplog/change_store.rs | 64 ++++++--- .../tests/export_updates_in_range.test.ts | 37 +++++ crates/loro/tests/export_updates_in_range.rs | 126 ++++++++++++++++++ 4 files changed, 212 insertions(+), 20 deletions(-) create mode 100644 .changeset/export-updates-in-range-spans.md create mode 100644 crates/loro-wasm/tests/export_updates_in_range.test.ts create mode 100644 crates/loro/tests/export_updates_in_range.rs diff --git a/.changeset/export-updates-in-range-spans.md b/.changeset/export-updates-in-range-spans.md new file mode 100644 index 000000000..80bc384b1 --- /dev/null +++ b/.changeset/export-updates-in-range-spans.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +`export({ mode: "updates-in-range" })` no longer panics (a `RuntimeError: unreachable` trap in WASM) when the spans hold several non-contiguous or overlapping ranges of one peer; all the ranges are exported. diff --git a/crates/loro-internal/src/oplog/change_store.rs b/crates/loro-internal/src/oplog/change_store.rs index 0b023fc1e..12c354157 100644 --- a/crates/loro-internal/src/oplog/change_store.rs +++ b/crates/loro-internal/src/oplog/change_store.rs @@ -23,7 +23,7 @@ use loro_common::{ use loro_kv_store::{mem_store::MemKvConfig, MemKvStore}; use once_cell::sync::OnceCell; use rle::{HasLength, Mergable, RlePush, RleVec, Sliceable}; -use rustc_hash::FxHashSet; +use rustc_hash::{FxHashMap, FxHashSet}; use std::sync::atomic::AtomicI64; #[cfg(test)] use std::sync::atomic::AtomicUsize; @@ -308,35 +308,59 @@ impl ChangeStore { } pub(super) fn export_blocks_in_range(&self, spans: &[IdSpan], w: &mut W) { - let new_store = ChangeStore::new_mem(&self.arena, self.merge_interval.clone()); + // A change store needs each peer's counters to be contiguous, so merge the + // ranges of each peer and put the k-th range of every peer into the k-th + // store. The blocks of all stores are written one after another, which the + // update decoder reads the same way (loro-dev/loro#1155). + let mut ranges: FxHashMap> = FxHashMap::default(); for span in spans { let mut span = *span; span.normalize_(); - if span.counter.end <= 0 { - continue; + let start = span.counter.start.max(0); + let end = span.counter.end.max(0); + if start < end { + ranges.entry(span.peer).or_default().push((start, end)); } + } - span.counter.start = span.counter.start.max(0); - span.counter.end = span.counter.end.max(0); - if span.counter.start >= span.counter.end { - continue; + let mut layers: Vec> = Vec::new(); + for (peer, mut peer_ranges) in ranges { + peer_ranges.sort_unstable(); + let mut merged: Vec<(Counter, Counter)> = Vec::with_capacity(peer_ranges.len()); + for (start, end) in peer_ranges { + match merged.last_mut() { + Some(last) if start <= last.1 => last.1 = last.1.max(end), + _ => merged.push((start, end)), + } } - - // PERF: this can be optimized by reusing the current encoded blocks - // In the current method, it needs to parse and re-encode the blocks - for c in self.iter_changes(span) { - let start = ((span.counter.start - c.id.counter).max(0) as usize).min(c.atom_len()); - let end = ((span.counter.end - c.id.counter).max(0) as usize).min(c.atom_len()); - if start == end { - continue; + for (i, (start, end)) in merged.into_iter().enumerate() { + if layers.len() <= i { + layers.push(Vec::new()); } - - let ch = c.slice(start, end); - new_store.insert_change(ch, false, false); + layers[i].push(IdSpan::new(peer, start, end)); } } - encode_blocks_in_store(new_store, &self.arena, w); + for layer in layers { + let new_store = ChangeStore::new_mem(&self.arena, self.merge_interval.clone()); + for span in layer { + // PERF: this can be optimized by reusing the current encoded blocks + // In the current method, it needs to parse and re-encode the blocks + for c in self.iter_changes(span) { + let start = + ((span.counter.start - c.id.counter).max(0) as usize).min(c.atom_len()); + let end = ((span.counter.end - c.id.counter).max(0) as usize).min(c.atom_len()); + if start == end { + continue; + } + + let ch = c.slice(start, end); + new_store.insert_change(ch, false, false); + } + } + + encode_blocks_in_store(new_store, &self.arena, w); + } } fn encode_from( diff --git a/crates/loro-wasm/tests/export_updates_in_range.test.ts b/crates/loro-wasm/tests/export_updates_in_range.test.ts new file mode 100644 index 000000000..594914ebd --- /dev/null +++ b/crates/loro-wasm/tests/export_updates_in_range.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// Several non-contiguous spans of one peer used to panic ("counter should be +// continuous") and trap the WASM instance. See loro-dev/loro#1155. +describe("export updates-in-range", () => { + it("exports several non-contiguous spans of one peer", () => { + const doc = new LoroDoc(); + doc.setPeerId(1); + const text = doc.getText("t"); + for (let i = 0; i < 10; i++) { + text.insert(i, String(i)); + doc.commit(); + } + const bytes = doc.export({ + mode: "updates-in-range", + spans: [ + { id: { peer: "1", counter: 0 }, len: 2 }, + { id: { peer: "1", counter: 5 }, len: 2 }, + ], + }); + + const target = new LoroDoc(); + target.import( + doc.export({ + mode: "updates-in-range", + spans: [{ id: { peer: "1", counter: 0 }, len: 5 }], + }), + ); + target.import(bytes); + expect(target.getText("t").toString()).toBe("0123456"); + // The source doc is still usable. + text.insert(0, "x"); + doc.commit(); + expect(doc.getText("t").toString()).toBe("x0123456789"); + }); +}); diff --git a/crates/loro/tests/export_updates_in_range.rs b/crates/loro/tests/export_updates_in_range.rs new file mode 100644 index 000000000..68142e51e --- /dev/null +++ b/crates/loro/tests/export_updates_in_range.rs @@ -0,0 +1,126 @@ +//! `ExportMode::updates_in_range` with several spans of one peer, which used to +//! panic with "counter should be continuous" while holding the op log lock. +//! See loro-dev/loro#1155. + +use loro::{ExportMode, IdSpan, LoroDoc, ToJson, VersionVector}; +use pretty_assertions::assert_eq; + +fn source() -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let text = doc.get_text("t"); + for i in 0..10 { + text.insert(i, "a").unwrap(); + doc.commit(); + } + let other = LoroDoc::new(); + other.set_peer_id(2).unwrap(); + other + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + for i in 0..6 { + other.get_map("m").insert(&format!("k{i}"), i).unwrap(); + other.commit(); + } + doc.import(&other.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + doc +} + +fn vv(entries: &[(u64, i32)]) -> VersionVector { + let mut vv = VersionVector::new(); + for &(peer, end) in entries { + vv.insert(peer, end); + } + vv +} + +#[test] +fn issue_repro_non_contiguous_spans_of_one_peer() { + let doc = source(); + let bytes = doc + .export(ExportMode::updates_in_range(vec![ + IdSpan::new(1, 0, 2), + IdSpan::new(1, 5, 7), + ])) + .unwrap(); + + // A doc that has 1@0..5 gets both ranges; a fresh one gets the first and parks + // the second until 1@2..5 arrive. + let has_prefix = LoroDoc::new(); + has_prefix + .import( + &doc.export(ExportMode::updates_in_range(vec![IdSpan::new(1, 0, 5)])) + .unwrap(), + ) + .unwrap(); + has_prefix.import(&bytes).unwrap(); + assert_eq!(has_prefix.oplog_vv(), vv(&[(1, 7)])); + + let fresh = LoroDoc::new(); + let status = fresh.import(&bytes).unwrap(); + assert_eq!(fresh.oplog_vv(), vv(&[(1, 2)])); + assert!(status.pending.is_some()); + fresh + .import( + &doc.export(ExportMode::updates_in_range(vec![IdSpan::new(1, 2, 5)])) + .unwrap(), + ) + .unwrap(); + assert_eq!(fresh.oplog_vv(), vv(&[(1, 7)])); +} + +#[test] +fn overlapping_unordered_spans_of_several_peers() { + let doc = source(); + let bytes = doc + .export(ExportMode::updates_in_range(vec![ + IdSpan::new(1, 6, 10), + IdSpan::new(2, 3, 6), + IdSpan::new(1, 3, 7), + IdSpan::new(1, 0, 3), + IdSpan::new(2, 0, 1), + IdSpan::new(2, 1, 2), + ])) + .unwrap(); + let fresh = LoroDoc::new(); + fresh.import(&bytes).unwrap(); + // 1@0..10 as one range, 2@0..2 and 2@3..6 (2@2 missing, so 2@3.. stays pending). + assert_eq!(fresh.oplog_vv(), vv(&[(1, 10), (2, 2)])); + fresh + .import( + &doc.export(ExportMode::updates_in_range(vec![IdSpan::new(2, 2, 3)])) + .unwrap(), + ) + .unwrap(); + assert_eq!(fresh.oplog_vv(), doc.oplog_vv()); + assert_eq!( + fresh.get_deep_value().to_json_value(), + doc.get_deep_value().to_json_value() + ); +} + +#[test] +fn many_small_spans_round_trip() { + let doc = source(); + let spans: Vec = (0..10) + .step_by(2) + .map(|i| IdSpan::new(1, i, i + 1)) + .collect(); + let odd: Vec = (1..10) + .step_by(2) + .map(|i| IdSpan::new(1, i, i + 1)) + .collect(); + let fresh = LoroDoc::new(); + fresh + .import(&doc.export(ExportMode::updates_in_range(spans)).unwrap()) + .unwrap(); + fresh + .import(&doc.export(ExportMode::updates_in_range(odd)).unwrap()) + .unwrap(); + assert_eq!(fresh.oplog_vv(), vv(&[(1, 10)])); + assert_eq!( + fresh.get_text("t").to_string(), + doc.get_text("t").to_string() + ); +} From e148f0ecd0537e0bd85b0c266afc7f676226745f Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:20:38 +0800 Subject: [PATCH 04/10] fix: accept a style start without its end in snapshots (#1165) A version addressed with op-level frontiers can hold a mark's StyleStart but not its StyleEnd. checkout keeps the lone start anchor, but the fast state decoder rejected it, and the first read of a fork or snapshot panicked under the doc locks. Decode it the way checkout keeps it (no style range until the end arrives), relax the debug-only pair checks accordingly, leave it alone in dead-style redaction, and only advance a shallow root past a StyleStart when the StyleEnd exists in the op log. Rationale: context/shallow-snapshot-style-redaction.md Co-Authored-By: Claude Opus 5.5 --- .changeset/unclosed-style-mark-snapshot.md | 5 + context/shallow-snapshot-style-redaction.md | 12 +- .../src/container/richtext/richtext_state.rs | 29 ++-- .../src/encoding/shallow_snapshot.rs | 19 +-- .../loro-internal/src/state/richtext_state.rs | 20 +-- .../tests/unclosed_style_mark.test.ts | 43 ++++++ .../tests/snapshot_unclosed_style_mark.rs | 131 ++++++++++++++++++ 7 files changed, 232 insertions(+), 27 deletions(-) create mode 100644 .changeset/unclosed-style-mark-snapshot.md create mode 100644 crates/loro-wasm/tests/unclosed_style_mark.test.ts create mode 100644 crates/loro/tests/snapshot_unclosed_style_mark.rs diff --git a/.changeset/unclosed-style-mark-snapshot.md b/.changeset/unclosed-style-mark-snapshot.md new file mode 100644 index 000000000..3f4f79142 --- /dev/null +++ b/.changeset/unclosed-style-mark-snapshot.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix a panic (a `RuntimeError: unreachable` trap in WASM) when reading a document forked or snapshotted at a version that holds a mark's start but not its end ("unclosed style mark"). Such a start anchor is now decoded like `checkout` keeps it: it styles nothing until its end arrives. A shallow snapshot of a document whose history ends at such a start no longer fails with `FrontiersNotFound`. diff --git a/context/shallow-snapshot-style-redaction.md b/context/shallow-snapshot-style-redaction.md index 076b210c1..b04188fd3 100644 --- a/context/shallow-snapshot-style-redaction.md +++ b/context/shallow-snapshot-style-redaction.md @@ -1,6 +1,6 @@ # Shallow Snapshot Dead-Style Redaction -Verified against code 2026-08-11 +Verified against code 2026-09-30 Shallow snapshot export nulls out the values of rich-text style pairs that are "dead" at the shallow root โ€” pairs whose anchors no longer enclose any text. @@ -65,6 +65,16 @@ a content-redaction mechanism). redacted version on import, which matches what whitelist redaction would have produced. +## Unclosed pairs + +A version can hold a mark's `StyleStart` (counter `c`) without its `StyleEnd` +(`c + 1`) when it is addressed with op-level frontiers (loro-dev/loro#1165). The +state keeps such a start anchor with no style range, as `checkout` already did, +and the style applies to nothing until the end arrives. The fast state decoder +accepts it, redaction leaves its value alone, and `calc_shallow_doc_start` +advances a root on a `StyleStart` to `c + 1` only when that op exists in the op +log. Tests: `crates/loro/tests/snapshot_unclosed_style_mark.rs`. + ## What still ships - The style key, info flags, and anchor ids (needed for entity indexing and diff --git a/crates/loro-internal/src/container/richtext/richtext_state.rs b/crates/loro-internal/src/container/richtext/richtext_state.rs index 60c1720ce..038550525 100644 --- a/crates/loro-internal/src/container/richtext/richtext_state.rs +++ b/crates/loro-internal/src/container/richtext/richtext_state.rs @@ -2706,12 +2706,27 @@ impl RichtextState { return; } + // A start anchor whose end is not in the state (a version that holds a + // mark's `StyleStart` but not its `StyleEnd`, loro-dev/loro#1165) styles + // nothing, so it has no style range. + let closed: FxHashSet<&Arc> = self + .iter_chunk() + .filter_map(|c| match c { + RichtextStateChunk::Style { + style, + anchor_type: AnchorType::End, + } => Some(style), + _ => None, + }) + .collect(); let mut entity_index_to_style_anchor: FxHashMap = FxHashMap::default(); let mut index = 0; for c in self.iter_chunk() { - if matches!(c, RichtextStateChunk::Style { .. }) { - entity_index_to_style_anchor.insert(index, c); + if let RichtextStateChunk::Style { style, .. } = c { + if closed.contains(style) { + entity_index_to_style_anchor.insert(index, c); + } } index += c.length() @@ -2752,7 +2767,7 @@ impl RichtextState { ); } - /// Allow StyleAnchors to appear in pairs, so that there won't be unmatched single StyleAnchors. + /// Every end anchor must follow its start anchor. A start anchor may lack its end. pub(crate) fn check_style_anchors_appear_in_pairs(&self) { if !cfg!(any(debug_assertions, test)) { return; @@ -2772,11 +2787,9 @@ impl RichtextState { }, } } - assert!( - start_ops.is_empty(), - "Only has start anchors {:#?}", - &start_ops - ); + // A start anchor without its end is allowed: a version can hold a mark's + // `StyleStart` op but not its `StyleEnd` op (loro-dev/loro#1165). + let _ = start_ops; } /// Iter style ranges in the given range in entity index diff --git a/crates/loro-internal/src/encoding/shallow_snapshot.rs b/crates/loro-internal/src/encoding/shallow_snapshot.rs index 816c5bf93..b46ebb1ed 100644 --- a/crates/loro-internal/src/encoding/shallow_snapshot.rs +++ b/crates/loro-internal/src/encoding/shallow_snapshot.rs @@ -944,14 +944,17 @@ fn calc_shallow_doc_start( let mut processed = false; if let Some(op) = oplog.get_op_that_includes(id) { if let crate::op::InnerContent::List(InnerListOp::StyleStart { .. }) = &op.content { - // StyleStart and StyleEnd operations must be kept together in the GC snapshot. - // Splitting them could lead to an weird document state that cannot be - // properly encoded. To ensure they stay together, we advance the frontier by - // one step to include both operations. - - // > Id.counter + 1 is guaranteed to be the StyleEnd Op - ans.push(id.inc(1)); - processed = true; + // StyleStart and StyleEnd operations are kept together in the GC snapshot + // when both exist, so we advance the frontier by one step to include both + // operations. (Id.counter + 1 is guaranteed to be the StyleEnd Op.) + // + // When this doc's history ends at the StyleStart, the end op does not + // exist here; the root keeps the lone start anchor, which the state + // encoding supports (loro-dev/loro#1165). + if oplog.vv().includes_id(id.inc(1)) { + ans.push(id.inc(1)); + processed = true; + } } } diff --git a/crates/loro-internal/src/state/richtext_state.rs b/crates/loro-internal/src/state/richtext_state.rs index e753cffc1..bad789c92 100644 --- a/crates/loro-internal/src/state/richtext_state.rs +++ b/crates/loro-internal/src/state/richtext_state.rs @@ -1412,9 +1412,9 @@ mod snapshot { } } - if !open.is_empty() { - return Err(state_decode_error(format!("{CTX}: unclosed style mark"))); - } + // Pairs still open are a version that holds a mark's `StyleStart` but not its + // `StyleEnd` (loro-dev/loro#1165). They style nothing yet, but the end may + // still arrive, so leave their values alone. if start_count != marks.len() { return Err(state_decode_error(format!("{CTX}: unused style mark"))); } @@ -1638,11 +1638,10 @@ mod snapshot { "Decode richtext state failed: unused style mark", )); } - if !id_to_style.is_empty() { - return Err(state_decode_error( - "Decode richtext state failed: unclosed style mark", - )); - } + // A `StyleStart` without its `StyleEnd` is valid: a version can include the + // start op but not the end op (their counters are consecutive), and + // `checkout` keeps the lone start anchor the same way. The style applies + // to nothing until the end anchor arrives (loro-dev/loro#1165). text.state = LazyLoad::Src(loader); // NOTE: We need to ensure the invariance that the version id is always increased when the richtext state is changed // This is used to avoid the version_id to be the same as the previous zero version @@ -1835,13 +1834,14 @@ mod snapshot { #[test] fn rejects_malformed_span_structures() { - // Start without a matching end. + // Start without a matching end is valid (loro-dev/loro#1165): a version can + // hold a mark's StyleStart but not its StyleEnd. It is left untouched. let unclosed = payload( "", vec![span(10, 0)], vec![mark(LoroValue::Bool(true), richtext::ExpandType::After)], ); - assert!(redact_dead_style_values(&unclosed, None).is_err()); + assert!(redact_dead_style_values(&unclosed, None).unwrap().is_none()); // End without a start. (An empty mark list short-circuits before // validation, so give it one mark to reach the scan.) diff --git a/crates/loro-wasm/tests/unclosed_style_mark.test.ts b/crates/loro-wasm/tests/unclosed_style_mark.test.ts new file mode 100644 index 000000000..18ff79944 --- /dev/null +++ b/crates/loro-wasm/tests/unclosed_style_mark.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// A version with a mark's StyleStart but not its StyleEnd (op-level frontiers) +// used to trap on the first read of a fork or snapshot of it ("unclosed style +// mark"). See loro-dev/loro#1165. +function source(): LoroDoc { + const doc = new LoroDoc(); + doc.setPeerId(1); + doc.configTextStyle({ bold: { expand: "after" } }); + const text = doc.getText("t"); + text.insert(0, "abc"); // 0@1..2@1 + text.mark({ start: 1, end: 3 }, "bold", true); // StyleStart 3@1, StyleEnd 4@1 + text.insert(3, "Z"); // 5@1 + doc.commit(); + return doc; +} + +const midMark = [{ peer: "1" as const, counter: 3 }]; + +describe("versions with an unclosed style mark", () => { + it("forkAt is readable and matches checkout", () => { + const doc = source(); + const checkedOut = doc.fork(); + checkedOut.checkout(midMark); + const expected = checkedOut.getText("t").toDelta(); + + const fork = doc.forkAt(midMark); + expect(fork.getText("t").toDelta()).toEqual(expected); + fork.import(doc.export({ mode: "update" })); + expect(fork.getText("t").toDelta()).toEqual(doc.getText("t").toDelta()); + }); + + it("a snapshot of the checked-out version imports and reads", () => { + const doc = source(); + doc.checkout(midMark); + const expected = doc.getText("t").toDelta(); + const bytes = doc.fork().export({ mode: "snapshot" }); + const other = new LoroDoc(); + other.import(bytes); + expect(other.getText("t").toDelta()).toEqual(expected); + }); +}); diff --git a/crates/loro/tests/snapshot_unclosed_style_mark.rs b/crates/loro/tests/snapshot_unclosed_style_mark.rs new file mode 100644 index 000000000..93e05dd7e --- /dev/null +++ b/crates/loro/tests/snapshot_unclosed_style_mark.rs @@ -0,0 +1,131 @@ +//! A version can hold a mark's `StyleStart` op but not its `StyleEnd` (op-level +//! frontiers: the two ops have consecutive counters). `checkout` handles it; a +//! snapshot of it used to panic on the first read with "unclosed style mark". +//! See loro-dev/loro#1165. + +use loro::{ExpandType, ExportMode, Frontiers, LoroDoc, StyleConfig, StyleConfigMap, ToJson, ID}; +use pretty_assertions::assert_eq; + +fn source() -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let mut styles = StyleConfigMap::new(); + styles.insert( + "bold".into(), + StyleConfig { + expand: ExpandType::After, + }, + ); + doc.config_text_style(styles); + let text = doc.get_text("t"); + text.insert(0, "abc").unwrap(); // 0@1..=2@1 + text.mark(1..3, "bold", true).unwrap(); // StyleStart 3@1, StyleEnd 4@1 + text.insert(3, "Z").unwrap(); // 5@1 + doc.commit(); + doc +} + +fn mid_mark() -> Frontiers { + Frontiers::from_id(ID::new(1, 3)) +} + +fn checkout_delta(doc: &LoroDoc, f: &Frontiers) -> serde_json::Value { + let tmp = doc.fork(); + tmp.checkout(f).unwrap(); + tmp.get_text("t").get_richtext_value().to_json_value() +} + +#[test] +fn fork_at_version_with_unclosed_mark_is_readable() { + let doc = source(); + let expected = checkout_delta(&doc, &mid_mark()); + let fork = doc.fork_at(&mid_mark()).unwrap(); + assert_eq!( + fork.get_text("t").get_richtext_value().to_json_value(), + expected + ); + assert_eq!(fork.get_text("t").to_string(), "abc"); + + // The fork keeps working: edits, and the rest of the history closes the mark. + fork.get_text("t").insert(0, ">").unwrap(); + fork.commit(); + fork.import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + doc.import(&fork.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!( + fork.get_text("t").get_richtext_value().to_json_value(), + doc.get_text("t").get_richtext_value().to_json_value() + ); +} + +#[test] +fn snapshot_of_version_with_unclosed_mark_imports_and_reads() { + let doc = source(); + let expected = checkout_delta(&doc, &mid_mark()); + let checked_out = doc.fork(); + checked_out.checkout(&mid_mark()).unwrap(); + let bytes = checked_out.fork().export(ExportMode::Snapshot).unwrap(); + + let other = LoroDoc::new(); + other.import(&bytes).unwrap(); + assert_eq!( + other.get_text("t").get_richtext_value().to_json_value(), + expected + ); + + // Re-exporting (including a shallow snapshot, which redacts dead styles) works. + let again = LoroDoc::new(); + again + .import(&other.export(ExportMode::Snapshot).unwrap()) + .unwrap(); + assert_eq!( + again.get_text("t").get_richtext_value().to_json_value(), + expected + ); + let shallow = LoroDoc::new(); + shallow + .import( + &other + .export(ExportMode::shallow_snapshot(&other.oplog_frontiers())) + .unwrap(), + ) + .unwrap(); + assert_eq!( + shallow.get_text("t").get_richtext_value().to_json_value(), + expected + ); +} + +/// A shallow snapshot of a doc whose history ends at the `StyleStart` used to fail +/// with `FrontiersNotFound`: the root was moved to the missing `StyleEnd`. +#[test] +fn shallow_snapshot_of_doc_ending_at_style_start() { + let doc = source(); + let expected = checkout_delta(&doc, &mid_mark()); + let fork = doc.fork_at(&mid_mark()).unwrap(); + let shallow = LoroDoc::new(); + shallow + .import( + &fork + .export(ExportMode::shallow_snapshot(&fork.oplog_frontiers())) + .unwrap(), + ) + .unwrap(); + assert_eq!( + shallow.get_text("t").get_richtext_value().to_json_value(), + expected + ); + + // The rest of the history closes the mark on top of the shallow root. + shallow + .import( + &doc.export(ExportMode::updates(&shallow.oplog_vv())) + .unwrap(), + ) + .unwrap(); + assert_eq!( + shallow.get_text("t").get_richtext_value().to_json_value(), + doc.get_text("t").get_richtext_value().to_json_value() + ); +} From 207c81f0af5c9f85589c78dd9a7bd3a9b29f2681 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:44:06 +0800 Subject: [PATCH 05/10] docs: migration advice for legacy text delete start_ids (#1149) Deletes written by WASM loro-crdt <= 1.16.3 can name the wrong characters in start_id. The ops are immutable and the affected readers (shallow-snapshot placeholders, cursors on deleted text) have nothing but start_id, so the history is not repaired in code. Document which paths stay correct and how to avoid the wrong ones, and pin that advice with a test on a forged legacy history (plus the still-divergent shallow root before the delete). Co-Authored-By: Claude Opus 5.5 --- context/richtext-insert-positions.md | 22 +++- .../loro/tests/legacy_text_delete_start_id.rs | 102 ++++++++++++++++++ 2 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 crates/loro/tests/legacy_text_delete_start_id.rs diff --git a/context/richtext-insert-positions.md b/context/richtext-insert-positions.md index ce3813369..33710ade5 100644 --- a/context/richtext-insert-positions.md +++ b/context/richtext-insert-positions.md @@ -1,6 +1,6 @@ # Rich-Text Insert Positions and Delete IDs -Verified against code 2026-09-28. +Verified against code 2026-09-30. ## Where local text goes next to style anchors @@ -65,3 +65,23 @@ real IDs from it (`CrdtRope::delete`), so a shallow import can end with other text than a full import, and a cursor on a deleted character that no delete op names does not resolve. Histories that already contain such deletes are tracked in loro-dev/loro#1149. + +### Existing histories (loro-dev/loro#1149) + +Not repaired in code: the op bytes are immutable history, and the readers that go +wrong have nothing but `start_id` to go on. A shallow import's placeholders stand +for trimmed history whose IDs are unknown, and a cursor on a character that no +delete names has no op to follow. Deriving IDs from positions there would mean +seeding the shallow tracker from the root state's per-character IDs, a large +change to diff calculation for a narrow legacy case. Advice for affected +documents (edited with WASM `loro-crdt` โ‰ค 1.16.3, astral characters, deletes +spanning several insert runs): + +- Full imports, full snapshots, and checkouts replay deletes by position and are + correct. +- Take shallow snapshots at the latest frontiers, or at any version after the + last delete written by an affected build. The root state then comes from a + positional replay, and the bad `start_id`s lie before the root. +- Re-create cursors on text that such a delete removed (`getCursor` after + loading) instead of resolving stored ones, or store positions for these + documents. diff --git a/crates/loro/tests/legacy_text_delete_start_id.rs b/crates/loro/tests/legacy_text_delete_start_id.rs new file mode 100644 index 000000000..5f5114cb8 --- /dev/null +++ b/crates/loro/tests/legacy_text_delete_start_id.rs @@ -0,0 +1,102 @@ +//! Deletes written by WASM `loro-crdt` <= 1.16.3 can record a `start_id` that names +//! the wrong characters around astral characters (loro-dev/loro#1149). The ops +//! can't be repaired; this pins the documented migration advice +//! (`context/richtext-insert-positions.md`): full imports and shallow snapshots +//! rooted after the bad delete give the right text. + +use loro::{ExportMode, JsonOpContent, JsonTextOp, LoroDoc, ID}; + +/// The issue's edits, with the delete rewritten into the single op an affected +/// build wrote: `{pos: 0, len: 3, start_id: 0@7}` names "a๐Ÿ˜€b" (IDs 0..=2), but +/// "a๐Ÿ˜€x" (IDs 0, 1, 3) was deleted. +fn legacy_history() -> LoroDoc { + let writer = LoroDoc::new(); + writer.set_peer_id(7).unwrap(); + let text = writer.get_text("t"); + text.insert(0, "a๐Ÿ˜€b").unwrap(); + writer.commit(); + text.insert(2, "x").unwrap(); // "a๐Ÿ˜€xb" + writer.commit(); + text.delete(0, 3).unwrap(); // deletes "a๐Ÿ˜€x" + writer.commit(); + text.insert(1, "!").unwrap(); + writer.commit(); + + let mut json = writer.export_json_updates(&Default::default(), &writer.oplog_vv()); + for change in json.changes.iter_mut() { + let deletes: Vec = change + .ops + .iter() + .enumerate() + .filter(|(_, op)| matches!(op.content, JsonOpContent::Text(JsonTextOp::Delete { .. }))) + .map(|(i, _)| i) + .collect(); + if deletes.is_empty() { + continue; + } + let counter = deletes + .iter() + .map(|&i| change.ops[i].counter) + .min() + .unwrap(); + let mut op = change.ops[deletes[0]].clone(); + op.counter = counter; + op.content = JsonOpContent::Text(JsonTextOp::Delete { + pos: 0, + len: 3, + start_id: ID::new(0, 0), // peer index 0 is peer 7 + }); + // The deletes are adjacent and cover the same counters as the one op. + for &i in deletes[1..].iter().rev() { + change.ops.remove(i); + } + change.ops[deletes[0]] = op; + } + let doc = LoroDoc::new(); + doc.import_json_updates(json).unwrap(); + doc +} + +#[test] +fn full_import_and_shallow_snapshot_after_the_delete_are_right() { + let full = legacy_history(); + assert_eq!(full.get_text("t").to_string(), "b!"); + + let copy = LoroDoc::new(); + copy.import(&full.export(ExportMode::Snapshot).unwrap()) + .unwrap(); + assert_eq!(copy.get_text("t").to_string(), "b!"); + + let shallow = LoroDoc::new(); + shallow + .import( + &full + .export(ExportMode::shallow_snapshot(&full.oplog_frontiers())) + .unwrap(), + ) + .unwrap(); + assert_eq!(shallow.get_text("t").to_string(), "b!"); +} + +/// The forged history does reproduce loro-dev/loro#1149: a shallow snapshot rooted +/// before the delete labels the deleted placeholders with the bad `start_id` and +/// ends with other text. This documents the open part of the issue; update it if +/// the shallow replay stops trusting `start_id`. +#[test] +fn shallow_snapshot_rooted_before_the_delete_still_diverges() { + let full = legacy_history(); + let before_delete = full.vv_to_frontiers(&{ + let mut vv = loro::VersionVector::new(); + vv.insert(7, 5); // "a๐Ÿ˜€b" and "x", before the delete + vv + }); + let shallow = LoroDoc::new(); + shallow + .import( + &full + .export(ExportMode::shallow_snapshot(&before_delete)) + .unwrap(), + ) + .unwrap(); + assert_eq!(shallow.get_text("t").to_string(), "x!"); +} From 2ced4e6c74875fe5bbeb3d3508f4589d952e494f Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:45:26 +0800 Subject: [PATCH 06/10] fix: show empty root containers touched by imported history (#1156) A root is visible when the state store has it. The writing peer creates it with its first local op, but an import only did so when the root's net diff was non-empty, so text inserted then deleted showed {"t": ""} live and {} in a replay. After an attached import, and after a checkout to the latest version, create the state of every non-mergeable root the diff calculator touched. Getting a root handle also makes it visible, by design; that half of the issue's random workload is an API decision and is documented, not changed. Rationale: context/root-container-visibility.md Co-Authored-By: Claude Opus 5.5 --- .changeset/empty-root-replay.md | 5 + AGENTS.md | 3 + context/root-container-visibility.md | 38 +++ crates/loro-internal/AGENTS.md | 2 + crates/loro-internal/src/diff_calc.rs | 5 + crates/loro-internal/src/loro.rs | 30 +- .../tests/root_visibility_replay.test.ts | 21 ++ crates/loro/tests/root_visibility_replay.rs | 310 ++++++++++++++++++ 8 files changed, 411 insertions(+), 3 deletions(-) create mode 100644 .changeset/empty-root-replay.md create mode 100644 context/root-container-visibility.md create mode 100644 crates/loro-wasm/tests/root_visibility_replay.test.ts create mode 100644 crates/loro/tests/root_visibility_replay.rs diff --git a/.changeset/empty-root-replay.md b/.changeset/empty-root-replay.md new file mode 100644 index 000000000..dbc0ff612 --- /dev/null +++ b/.changeset/empty-root-replay.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +A root container touched by a document's history is now part of the value after importing that history, even when its ops add up to an empty value (for example text inserted and then deleted). Previously the writing peer showed `{"t": ""}` while a replay of the same history showed `{}`. diff --git a/AGENTS.md b/AGENTS.md index 0d59af1de..d56ce261d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,9 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. [context/mergeable-containers.md](context/mergeable-containers.md). - `import_batch` force-detach, batch-wide rollback scope, and the never-exit-detached invariant: [context/import-batch-atomicity.md](context/import-batch-atomicity.md). +- Which root containers are visible (history-touched vs handle-acquired), and + why replays materialize empty roots: + [context/root-container-visibility.md](context/root-container-visibility.md). - Imports that reuse local op ids (a shared peer id): the known-history check, what counts as equal, and what it cannot catch: [context/import-peer-id-reuse.md](context/import-peer-id-reuse.md). diff --git a/context/root-container-visibility.md b/context/root-container-visibility.md new file mode 100644 index 000000000..60192c15a --- /dev/null +++ b/context/root-container-visibility.md @@ -0,0 +1,38 @@ +# Root Container Visibility + +Verified against code 2026-09-30. + +`DocState::get_deep_value` (`crates/loro-internal/src/state.rs`) lists the root +containers that have an entry in the state store (`preferred_root_containers`), so +a root can be visible with an empty value (`{"t": ""}`). `Configure`'s +`hide_empty_root_containers` (off by default) hides those. + +## What makes a root visible + +- **Any op in the history touches it.** The writing peer creates the root's state + with its first local op. An import used to create it only when the root's net + diff was non-empty, so text inserted and then deleted showed `{"t": ""}` on the + writer and `{}` in a replay of the same history (loro-dev/loro#1156). + `materialize_touched_roots` (`src/loro.rs`) now creates the state of every + non-mergeable root that the import's `DiffCalculator` touched + (`DiffCalculator::touched_containers`). The same happens when a checkout reaches + the op log's latest version (`attach`, the end of `import_batch`), where every + range the persistent calculator has seen lies in the target's history. Parked + pending changes already materialize their roots + (`pending_root_containers_to_materialize`). +- **Getting a handle.** `doc.get_text("t")` (and the other `get_*` root getters) + creates the root's state without any op, by design, and snapshots keep it. So + the value does not depend on the history alone: a peer that only read a root + shows it, and a replay of its history does not. Changing that is an API + decision; it is the remaining part of the random workload in #1156. + +## Not covered + +Checking out an *older* version keeps every root the state already has, including +roots first touched after that version. A fresh replay up to that version would +not show them. + +## Tests + +`crates/loro/tests/root_visibility_replay.rs`, +`crates/loro-wasm/tests/root_visibility_replay.test.ts`. diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index e68036673..7537b75bd 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -52,6 +52,8 @@ over graceful degradation. - Movable-list `Move`/`Set` element validation on import, plus change-store rollback records and KV block-range decoding: [../../context/movable-list-op-validation.md](../../context/movable-list-op-validation.md). +- Root container visibility after import/checkout (`materialize_touched_roots`): + [../../context/root-container-visibility.md](../../context/root-container-visibility.md). - Imported changes that reuse local op ids are checked against local history before anything is applied (`src/oplog/known_history.rs`): [../../context/import-peer-id-reuse.md](../../context/import-peer-id-reuse.md). diff --git a/crates/loro-internal/src/diff_calc.rs b/crates/loro-internal/src/diff_calc.rs index f7b135904..825ddcf19 100644 --- a/crates/loro-internal/src/diff_calc.rs +++ b/crates/loro-internal/src/diff_calc.rs @@ -144,6 +144,11 @@ impl DiffCalculator { } } + /// The containers touched by the ranges this calculator has computed diffs for. + pub(crate) fn touched_containers(&self) -> impl Iterator + '_ { + self.calculators.keys().copied() + } + #[allow(unused)] pub(crate) fn get_calc(&self, container: ContainerIdx) -> Option<&ContainerDiffCalculator> { self.calculators.get(&container).map(|(_, c)| c) diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 15a4d7978..b2bac74a4 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -855,7 +855,7 @@ impl LoroDoc { return Err(e); } } - let mut diff = DiffCalculator::new(false); + let mut calc = DiffCalculator::new(false); // Applying may have unlocked pending changes; the isolated fast path is only valid // when exactly the candidate batch was appended on top of the old version. let isolated_batch = isolated_batch.filter(|(component_vv, _)| { @@ -872,7 +872,7 @@ impl LoroDoc { #[cfg(test)] ISOLATED_FAST_PATH_HITS.with(|hits| hits.set(hits.get() + 1)); let component_frontiers = oplog.dag.vv_to_frontiers(&component_vv); - let (diff, _) = diff.calc_diff_internal( + let (diff, _) = calc.calc_diff_internal( &oplog, &VersionVector::default(), &Frontiers::default(), @@ -882,7 +882,7 @@ impl LoroDoc { ); (diff, DiffMode::Import) } else { - diff.calc_diff_internal( + calc.calc_diff_internal( &oplog, &old_vv, &old_frontiers, @@ -915,6 +915,7 @@ impl LoroDoc { panic!("state apply returned Err for import without rollback guard: {e}"); } + materialize_touched_roots(&mut state, &calc, &oplog.arena); } if result.has_deps_before_shallow_root { @@ -2388,6 +2389,12 @@ impl LoroDoc { }, diff_mode, )?; + // Only at the latest version: every root the calculator has seen is then in + // the history of the target version. Checking out an older version keeps + // whatever roots the state already had. + if after == oplog.vv() { + materialize_touched_roots(&mut state, &calc, &oplog.arena); + } Ok(()) } @@ -2832,6 +2839,23 @@ impl LoroDoc { } } +/// A root container touched by any op in a doc's history is part of its value, even +/// when the ops add up to an empty value: the peer that wrote them created the root's +/// state with its first local op. A replay whose net diff for the root is empty never +/// creates that state, so create it here (loro-dev/loro#1156). +/// +/// `calc` must have computed diffs whose ranges all lie in the history of the state's +/// new version. +fn materialize_touched_roots(state: &mut DocState, calc: &DiffCalculator, arena: &SharedArena) { + for idx in calc.touched_containers() { + if let Some(id) = arena.idx_to_id(idx) { + if id.is_root() && !id.is_mergeable() && !state.store.contains_id(&id) { + state.ensure_container(&id); + } + } + } +} + fn pending_root_containers_to_materialize(oplog: &OpLog, changes: &[Change]) -> Vec { let mut roots = FxHashSet::default(); for change in changes { diff --git a/crates/loro-wasm/tests/root_visibility_replay.test.ts b/crates/loro-wasm/tests/root_visibility_replay.test.ts new file mode 100644 index 000000000..c62c86062 --- /dev/null +++ b/crates/loro-wasm/tests/root_visibility_replay.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// A root touched by history is visible even when its ops add up to an empty +// value, both for the peer that wrote them and for a replay of that history. +// See loro-dev/loro#1156. +describe("empty root containers", () => { + it("a replay shows the same empty root as the peer", () => { + const a = new LoroDoc(); + a.setPeerId(1); + a.getText("t").insert(0, "ab"); + a.commit(); + a.getText("t").delete(0, 2); + a.commit(); + expect(a.toJSON()).toEqual({ t: "" }); + + const replay = new LoroDoc(); + replay.import(a.export({ mode: "update" })); + expect(replay.toJSON()).toEqual({ t: "" }); + }); +}); diff --git a/crates/loro/tests/root_visibility_replay.rs b/crates/loro/tests/root_visibility_replay.rs new file mode 100644 index 000000000..6bc9e8414 --- /dev/null +++ b/crates/loro/tests/root_visibility_replay.rs @@ -0,0 +1,310 @@ +//! Whether a root container is part of a doc's value must depend on its history +//! only: a root touched by any op is visible, even when its ops add up to an empty +//! value. A peer and a fresh replay of its own history used to disagree for such +//! roots. See loro-dev/loro#1156. + +use loro::{ExportMode, LoroDoc, LoroMap, LoroText, ToJson, UndoManager, ValueOrContainer}; +use pretty_assertions::assert_eq; +use rand::{rngs::StdRng, Rng, SeedableRng}; +use serde_json::json; + +fn replay(doc: &LoroDoc) -> LoroDoc { + let replay = LoroDoc::new(); + replay + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + replay +} + +fn value(doc: &LoroDoc) -> serde_json::Value { + doc.get_deep_value().to_json_value() +} + +fn inserted_then_deleted() -> LoroDoc { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + a.get_text("t").insert(0, "ab").unwrap(); + a.commit(); + a.get_text("t").delete(0, 2).unwrap(); + a.commit(); + a +} + +#[test] +fn issue_repro_empty_root_text_is_visible_after_replay() { + let a = inserted_then_deleted(); + assert_eq!(value(&a), json!({"t": ""})); + assert_eq!(value(&replay(&a)), json!({"t": ""})); +} + +#[test] +fn every_import_path_agrees() { + let a = inserted_then_deleted(); + let expected = value(&a); + let updates = a.export(ExportMode::all_updates()).unwrap(); + + // One import, a snapshot, a batch, JSON, and a detached import then attach. + let snapshot = LoroDoc::new(); + snapshot + .import(&a.export(ExportMode::Snapshot).unwrap()) + .unwrap(); + assert_eq!(value(&snapshot), expected, "snapshot"); + + let batch = LoroDoc::new(); + let first = LoroDoc::new(); + first.set_peer_id(9).unwrap(); + first.get_map("other").insert("k", 1).unwrap(); + first.commit(); + batch + .import_batch(&[ + updates.clone(), + first.export(ExportMode::all_updates()).unwrap(), + ]) + .unwrap(); + let mut with_other = expected.clone(); + with_other["other"] = json!({"k": 1}); + assert_eq!(value(&batch), with_other, "import_batch"); + + let from_json = LoroDoc::new(); + from_json + .import_json_updates(a.export_json_updates(&Default::default(), &a.oplog_vv())) + .unwrap(); + assert_eq!(value(&from_json), expected, "json"); + + let detached = LoroDoc::new(); + detached.detach(); + detached.import(&updates).unwrap(); + detached.attach(); + assert_eq!(value(&detached), expected, "detached import + attach"); + + // Into a doc that already has content of its own. + let busy = LoroDoc::new(); + busy.set_peer_id(5).unwrap(); + busy.get_list("l").push(1).unwrap(); + busy.commit(); + busy.import(&updates).unwrap(); + let mut with_list = expected.clone(); + with_list["l"] = json!([1]); + assert_eq!(value(&busy), with_list, "non-empty target"); +} + +#[test] +fn root_map_and_list_that_end_empty_are_visible() { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + a.get_map("m").insert("k", 1).unwrap(); + a.get_list("l").push(1).unwrap(); + a.get_movable_list("ml").push(1).unwrap(); + a.commit(); + a.get_map("m").delete("k").unwrap(); + a.get_list("l").delete(0, 1).unwrap(); + a.get_movable_list("ml").delete(0, 1).unwrap(); + a.commit(); + assert_eq!(value(&replay(&a)), value(&a)); +} + +/// The insert and the delete arrive in separate imports, or the insert is undone. +#[test] +fn separate_imports_and_undo_agree_with_replay() { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let mut undo = UndoManager::new(&a); + a.get_text("t").insert(0, "ab").unwrap(); + a.commit(); + undo.undo().unwrap(); + assert_eq!(value(&a), json!({"t": ""})); + assert_eq!(value(&replay(&a)), value(&a)); + + let src = inserted_then_deleted(); + let b = LoroDoc::new(); + let insert_only = loro::IdSpan::new(1, 0, 2); + b.import( + &src.export(ExportMode::updates_in_range(vec![insert_only])) + .unwrap(), + ) + .unwrap(); + b.import(&src.export(ExportMode::updates(&b.oplog_vv())).unwrap()) + .unwrap(); + assert_eq!(value(&b), json!({"t": ""})); + assert_eq!(value(&b), value(&replay(&src))); +} + +// A trimmed copy of the random workload from the issue: several peers editing a +// movable list, a text and a map, with undo/redo, syncs, snapshots, restarts and +// batches. Every non-shallow peer must equal a fresh replay of its own history. +// +// Getting a root handle (`doc.get_text("t")`) makes the root visible by design, +// with or without ops, and the workload reads roots to pick positions. So every +// peer first writes one op to each root: the roots it reads are then also in its +// history, and undo can still bring them back to empty values. This is a guard +// against new mismatches; the targeted tests above are the reproductions. + +struct Peer { + doc: LoroDoc, + undo: UndoManager, + shallow: bool, +} + +fn new_peer(doc: LoroDoc, shallow: bool) -> Peer { + let undo = UndoManager::new(&doc); + Peer { doc, undo, shallow } +} + +fn step(rng: &mut StdRng, peers: &mut [Peer], i: usize) { + let n = peers.len(); + let p = rng.gen_range(0..n); + let q = (p + rng.gen_range(1..n)) % n; + let kind = rng.gen_range(0..16); + let d = &peers[p].doc; + let l = d.get_movable_list("l"); + let len = l.len(); + match kind { + 0 | 1 => { + l.insert(rng.gen_range(0..=len), i as i64).unwrap(); + } + 2 if len > 1 => { + l.mov(rng.gen_range(0..len), rng.gen_range(0..len)).unwrap(); + } + 3 if len > 0 => { + l.set(rng.gen_range(0..len), i as i64).unwrap(); + } + 4 if len > 0 => { + l.delete(rng.gen_range(0..len), 1).unwrap(); + } + 5 => { + if rng.gen_bool(0.5) { + let m = l + .insert_container(rng.gen_range(0..=len), LoroMap::new()) + .unwrap(); + m.insert("v", i as i64).unwrap(); + } else { + let t = l + .insert_container(rng.gen_range(0..=len), LoroText::new()) + .unwrap(); + t.insert(0, "hi").unwrap(); + } + } + 6 if len > 0 => { + if let Some(ValueOrContainer::Container(c)) = l.get(rng.gen_range(0..len)) { + match c { + loro::Container::Map(m) => { + m.insert("v", i as i64).unwrap(); + } + loro::Container::Text(t) => { + let tl = t.len_unicode(); + t.insert(rng.gen_range(0..=tl), "x").unwrap(); + } + _ => {} + } + } + } + 7 => { + let t = d.get_text("t"); + let tl = t.len_unicode(); + t.insert(rng.gen_range(0..=tl), "ab").unwrap(); + } + 8 => { + d.get_map("m") + .insert(&format!("k{}", rng.gen_range(0..4)), i as i64) + .unwrap(); + } + 9 => { + let _ = peers[p].undo.undo(); + } + 10 => { + let _ = peers[p].undo.redo(); + } + 11 | 12 => { + let u = peers[p] + .doc + .export(ExportMode::updates(&peers[q].doc.oplog_vv())) + .unwrap(); + let _ = peers[q].doc.import(&u); + } + 13 => { + let s = peers[p].doc.export(ExportMode::Snapshot).unwrap(); + let _ = peers[q].doc.import(&s); + } + 14 => { + let peer = peers[q].doc.peer_id(); + let shallow = rng.gen_bool(0.5); + let bytes = if shallow { + peers[q] + .doc + .export(ExportMode::shallow_snapshot( + &peers[q].doc.oplog_frontiers(), + )) + .unwrap() + } else { + peers[q].doc.export(ExportMode::Snapshot).unwrap() + }; + let doc = LoroDoc::new(); + doc.import(&bytes).unwrap(); + doc.set_peer_id(peer).unwrap(); + let was_shallow = peers[q].shallow; + peers[q] = new_peer(doc, shallow || was_shallow); + } + 15 => { + let r = (q + 1) % n; + let a = peers[p] + .doc + .export(ExportMode::updates(&peers[q].doc.oplog_vv())) + .unwrap(); + let b = peers[r] + .doc + .export(ExportMode::updates(&peers[q].doc.oplog_vv())) + .unwrap(); + let _ = peers[q].doc.import_batch(&[b, a]); + } + _ => {} + } + peers[p].doc.commit(); +} + +fn mismatches(seed: u64, steps: usize) -> Vec { + let mut rng = StdRng::seed_from_u64(seed); + let n = rng.gen_range(3..=5); + let mut peers: Vec = (0..n) + .map(|i| { + let d = LoroDoc::new(); + d.set_peer_id(i as u64 + 1).unwrap(); + let peer = new_peer(d, false); + peer.doc.get_movable_list("l").push(-1).unwrap(); + peer.doc.get_text("t").insert(0, "s").unwrap(); + peer.doc.get_map("m").insert("init", -1).unwrap(); + peer.doc.commit(); + peer + }) + .collect(); + for i in 0..steps { + step(&mut rng, &mut peers, i); + } + let mut out = Vec::new(); + for (i, p) in peers.iter().enumerate() { + p.doc.commit(); + if p.shallow || p.doc.is_shallow() { + continue; + } + let replayed = replay(&p.doc); + if replayed.get_deep_value() != p.doc.get_deep_value() { + out.push(format!( + "seed {seed} peer {i}: state={} replay={}", + value(&p.doc), + value(&replayed) + )); + } + } + out +} + +#[test] +fn random_peers_agree_with_replay_of_their_history() { + // Includes the seeds the issue reported for the root-map variant. + let mut seeds: Vec = (0..120).collect(); + seeds.extend([426, 509, 591, 748]); + let bad: Vec = seeds + .into_iter() + .flat_map(|seed| mismatches(seed, 120)) + .collect(); + assert!(bad.is_empty(), "{}", bad.join("\n")); +} From 4de63a99c2bbc0900b61a275f231d477596a04fc Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:46:44 +0800 Subject: [PATCH 07/10] fix: keep arena indices when a failed import is rolled back (#1164) A failed import's arena rollback freed the indices of containers registered during the import, while DocState store entries, the dead-container cache and handlers on other threads still referred to them. The next container registered at a freed index inherited the old state. The rollback now keeps every index and drops only what the failed import may have told the arena (parent links of new normal containers, cached depths), plus values and text; every rollback path also clears the dead-container cache. Rationale: context/failed-import-arena-indices.md. Co-Authored-By: Claude Opus 5.5 --- .changeset/failed-import-state-rollback.md | 5 + AGENTS.md | 3 + context/arena-parent-links.md | 40 +- context/dead-container-cache.md | 7 +- context/failed-import-arena-indices.md | 96 +++++ context/import-batch-atomicity.md | 5 +- context/movable-list-op-validation.md | 4 +- .../examples/failed_import_rollback_perf.rs | 103 +++++ crates/loro-internal/AGENTS.md | 4 +- crates/loro-internal/src/arena.rs | 51 +-- .../src/encoding/fast_snapshot.rs | 4 +- crates/loro-internal/src/loro.rs | 20 +- crates/loro-internal/src/oplog.rs | 27 +- .../loro-internal/src/oplog/change_store.rs | 11 +- crates/loro-internal/src/state/AGENTS.md | 3 + .../src/state/dead_containers_cache.rs | 8 + .../failed_import_state_rollback.test.ts | 86 ++++ .../tests/failed_import_state_rollback.rs | 370 ++++++++++++++++++ 18 files changed, 773 insertions(+), 74 deletions(-) create mode 100644 .changeset/failed-import-state-rollback.md create mode 100644 context/failed-import-arena-indices.md create mode 100644 crates/examples/examples/failed_import_rollback_perf.rs create mode 100644 crates/loro-wasm/tests/failed_import_state_rollback.test.ts create mode 100644 crates/loro/tests/failed_import_state_rollback.rs diff --git a/.changeset/failed-import-state-rollback.md b/.changeset/failed-import-state-rollback.md new file mode 100644 index 000000000..0d86b6df6 --- /dev/null +++ b/.changeset/failed-import-state-rollback.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +After an import failed and was rolled back, containers created later could show state left over from containers the failed import had touched (for example a new root map showing a tree node's metadata), read as deleted, or trip an internal assertion when the same bad update was imported again. A failed import now leaves the document's containers unchanged. diff --git a/AGENTS.md b/AGENTS.md index d56ce261d..53791cd03 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,6 +66,9 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. - How the arena learns a container's parent (parsed changes, state KV, the op log's creator resolver), its locking (including `ChangeStore`'s lock order), and import rollback: [context/arena-parent-links.md](context/arena-parent-links.md). +- Why a failed import's rollback keeps arena indices (DocState, caches and handlers + are keyed by them) and what it drops instead: + [context/failed-import-arena-indices.md](context/failed-import-arena-indices.md). - Which deletions `DocState::is_deleted` caches, and how tree/movable-list children are revived: [context/dead-container-cache.md](context/dead-container-cache.md). - Movable-list `apply_diff`/undo: delete + re-insert of a child becomes a move, diff --git a/context/arena-parent-links.md b/context/arena-parent-links.md index b0306264c..3652eeeaa 100644 --- a/context/arena-parent-links.md +++ b/context/arena-parent-links.md @@ -1,6 +1,6 @@ # Arena Parent Links -Verified against code 2026-09-29. +Verified against code 2026-09-30. `SharedArena` (`crates/loro-internal/src/arena.rs`) stores each container's parent. Liveness (`DocState::is_deleted`), paths (`DocState::get_path`, @@ -130,9 +130,13 @@ that writes the KV store, or a live container would read as deleted. Measured A failed import can parse old change blocks while it computes its diff (including through the creator resolver) or validates movable-list ops -(`OpLog::resolve_movable_list_elem`), which registers containers and allocates -their values. The arena rollback (`SharedArena::rollback`) drops registrations -and values made during the import, so `ChangeStore::rollback_import`, after +(`OpLog::resolve_movable_list_elem`), which registers containers with their +parent links and allocates their values. The arena rollback +(`SharedArena::rollback`) keeps every container index, but drops the parent +links of normal containers registered during the import and truncates the +values and text allocated during it (see +[failed-import-arena-indices.md](failed-import-arena-indices.md) for why indices +are kept). So `ChangeStore::rollback_import`, after truncating and evicting blocks (see [movable-list-op-validation.md](movable-list-op-validation.md)), drops the parsed changes of the kept blocks that were parsed since the import began; the @@ -142,7 +146,7 @@ block. A block parsed before the checkpoint can only refer to what was there then, so it keeps its parsed changes (`SharedArenaRollback::keeps`). Dropping them all made every failed import an O(blocks) pass and the next history read a full reparse. Blocks without bytes were built from changes inserted before the -import, so their containers were registered then. Before 2026-09-28 the parsed +import, so their containers were registered and linked then. Before 2026-09-28 the parsed ops kept indices and value slices past the truncated arena: exporting the history hit `unreachable!` in the JSON encoder, and a later checkout panicked on a missing value (loro-dev/loro#1161). @@ -164,18 +168,18 @@ and runs under the state lock: `Absent`, so a resolver that reached it before the swap does not register the discarded history. - Under the state lock: `OpLog::rollback_import`, `rollback_arena`, and - `reset_to_empty_for_failed_snapshot_import` take `&DocState` as proof. A - state-locked query (`has_container`, `is_deleted`, `get_path_to_container`) - can register containers through the resolver and then use their indices - before it returns; the rollback must not free them in between. Without the - state lock, a loom model hit `get_depth` on a freed index. - -Only the state-locked queries are covered. A handler created on another thread -during the import keeps its index, and `DocState` is not rolled back: its store -and dead-container cache keep entries at the freed indices, and the next -container registered at one inherits them. `get_path_to_container` answers -`None` if its registration is already gone. See loro-dev/loro#1164 -(pre-existing). + `reset_to_empty_for_failed_snapshot_import` take `&mut DocState`, as proof + and to clear the dead-container cache. A state-locked query (`has_container`, + `is_deleted`, `get_path_to_container`) can register containers through the + resolver and then walk their links before it returns; the rollback must not + drop them in between. Without the state lock, a loom model hit `get_depth` on + an index the rollback freed (it freed indices before loro-dev/loro#1164). + +A handler created on another thread during the import keeps its index and its +container, and so do `DocState`'s store entries and caches. If the rollback +dropped the link of a container that a thread is using without the state lock, +the next lookup resolves it again; `get_path_to_container` answers `None` if +the link is gone at that moment. ## Testing pitfall @@ -192,6 +196,8 @@ the real path. thread stress of queries against history readers (`UNREGISTERED_PARENT_THREAD_TRIALS`), and a random comparison of every tree meta against a full-history import (`UNREGISTERED_PARENT_SEEDS=0..300`). +- `crates/loro/tests/failed_import_state_rollback.rs`: state, handlers, and + retries after a failed import (loro-dev/loro#1164). - `crates/loro/tests/unregistered_container_parent.rs` `queries_race_with_failing_imports`: queries against imports that fail at decode or depend on history before a shallow root, swept across the import's diff --git a/context/dead-container-cache.md b/context/dead-container-cache.md index 5e42fd44a..2582d5749 100644 --- a/context/dead-container-cache.md +++ b/context/dead-container-cache.md @@ -1,6 +1,6 @@ # Dead Container Cache -Verified against code 2026-09-28. +Verified against code 2026-09-30. `DocState::is_deleted` (`crates/loro-internal/src/state/dead_containers_cache.rs`) answers whether a container is unreachable from a root: it walks the arena @@ -49,6 +49,11 @@ walk and assert that no cached entry on the chain contradicts the result. - `DocState::apply_local_op` clears `revivable` before a tree `Move`. A local op cannot address a deleted movable-list element. - Resets replace the cache. +- A failed import's rollback clears both sets + (`DocState::forget_parent_link_caches_after_failed_import`, called by the + `OpLog` rollback methods): an answer found through a parent link the + rollback drops is not true of the kept history. See + [failed-import-arena-indices.md](failed-import-arena-indices.md). `clear_revivable` is O(1) when the set is empty, so documents that never query deleted containers pay nothing. diff --git a/context/failed-import-arena-indices.md b/context/failed-import-arena-indices.md new file mode 100644 index 000000000..1d012c919 --- /dev/null +++ b/context/failed-import-arena-indices.md @@ -0,0 +1,96 @@ +# Failed Imports Keep Their Arena Indices + +Verified against code 2026-09-30. + +A failed import rolls the op log back (`OpLog::rollback_import`, `rollback_arena`, +`reset_to_empty_for_failed_snapshot_import`), including the arena +(`SharedArena::rollback`, always called through the change store; see +[arena-parent-links.md](arena-parent-links.md#import-rollback)). The arena rollback +keeps every container index. It used to truncate the containers registered since the +checkpoint and hand their indices to the next registrations (loro-dev/loro#1164). + +## Why indices are not freed + +Much outside the op log is keyed by `ContainerIdx` and is not rolled back: + +- `DocState`'s store (`InnerStore` wrappers and its decoded-value cache). A failed + import loads wrappers from KV while it validates (`validate_diff_batch`, + `needs_checkout_diff` โ†’ `store.get_container`). +- `DocState::dead_containers_cache` and `alive_containers_cache`. +- Handlers (`BasicHandler` holds the index), which another thread can create while + the import runs: `get_map("x")`, tree metas, `get_container`. + +When an index was reused, the new container inherited the old one's state. The issue's +repro shows a new root map `fresh` with the text of a tree meta that the failed import +had loaded at that index. Retrying the bad update tripped a type assertion in +`InnerStore` (a Map index holding Text state). With other threads, containers created +later read as deleted, and a container whose index was reused could become its own +ancestor and overflow the stack. + +The other direction the issue suggested, rolling `DocState` back at indices at or above +the checkpoint, does not cover handlers. A handler another thread created during the +import would still write to whatever container got its index next. Keeping the +indices makes every index-keyed structure stay valid without tracking each one. + +Keeping them is safe because a failed import changes no container state: +`DocState::apply_diff` validates the whole batch before it mutates anything, and +decode or DAG failures return before state is touched. A wrapper loaded while +validating holds that container's state at the kept version. Nothing is shown for a +root that the failed import registered but that has no store entry: +`preferred_root_containers` and `existing_retention_roots` filter by +`store.contains_id`. + +## What the rollback still drops + +- **Parent links** of normal containers registered since the checkpoint, and links from + a normal container to such a container. They may come from the rolled-back changes, + and a container whose only creating op was rolled back must not look alive + (`has_container` and `is_deleted` answer as if it was never registered). Links that + are true for the kept history come back through the resolvers: the state KV, or the + op log's creator resolver, which parses the block again because + `ChangeStore::rollback_arena` dropped the parsed changes of blocks parsed since the + checkpoint (`SharedArenaRollback::keeps`). Root links are kept, since a root's ID + determines them (none for a top-level root, the encoded parent for a mergeable root). +- **Depth cache.** When anything was registered since the checkpoint, every cached depth + is reset: one could have been computed through a dropped link, including for the + descendants of an older container. Kept links give the same depth again. +- **Values and text** allocated since the checkpoint are truncated. Only parsed changes + refer to them, and those are dropped with the blocks above. +- **Dead-container cache.** `DocState::forget_parent_link_caches_after_failed_import` + clears it on every rollback path (the `OpLog` rollback methods take `&mut DocState` + and call it). A deletion cached through a link the rollback drops is not true of the + kept history. It is a cache, so clearing only costs recomputation. + +Local transactions cannot register containers during an import: `import`, +`import_json_updates`, and `import_batch` hold the transaction lock +(`with_barrier` / `BatchImportGuard`), and `checkout` needs the op log lock. + +## Costs and limits + +- Successful imports are unchanged; `checkpoint_for_rollback` reads two fewer lengths. + `crates/examples/examples/failed_import_rollback_perf.rs` measured the same times + before and after for 5k/20k small updates one by one, a large update into a loaded doc, + and a failed large import (2026-09-30). +- A failed import leaves its container IDs registered. Retrying the same update + registers nothing new; different failing updates grow the arena by the containers + they name, like handlers for IDs that do not exist. +- A link the failed import set on a container registered *before* the checkpoint, to a + parent also registered before it, is kept (pre-existing; it needs a journal of + `set_parent` calls during the import). This takes a container that was registered + without a parent, for example a handler for an ID not received yet, whose creating op + is in the failed import. + +## Tests + +`crates/loro/tests/failed_import_state_rollback.rs`: the issue's repro verbatim, new +roots and children after two failures (plus snapshot and update round trips), retrying +the same bad update through `import` and `import_batch`, the valid update arriving after +the forged one failed, and a thread that creates root handlers and reads a tree meta while +the import fails twice (`FAILED_IMPORT_THREAD_TRIALS`, default 8). Every one except +the valid-update test failed before the fix. The threaded one failed in 10 of 10 +single-trial runs. + +`crates/loro-wasm/tests/failed_import_state_rollback.test.ts`: the same shape through +the JS API. It revives 40 nodes because which diffs the state validates before the +rejected one follows `FxHashMap` order, which differs on wasm32: with one or eight +nodes the old code happened to validate the text first and the test passed. diff --git a/context/import-batch-atomicity.md b/context/import-batch-atomicity.md index 2183d98e5..41e9d8c0d 100644 --- a/context/import-batch-atomicity.md +++ b/context/import-batch-atomicity.md @@ -54,8 +54,9 @@ Consequences to keep in mind: rare within a single blob (the lamport-ordered main pass applies deps first), routine across a batch, where later blobs unlock what earlier blobs parked. Undoing individual mutations then depends on getting their relative order right, and getting it wrong - resurrects scope-local changes whose `ContainerIdx` registrations the arena rollback - already truncated (dangling indices โ€” a corruption vector on the next unlock). + resurrects scope-local changes whose value slices and parent links the arena rollback + already dropped (before loro-dev/loro#1164 it also freed their `ContainerIdx` + registrations โ€” a corruption vector on the next unlock). Per-slot snapshots are order-independent by construction. The batch must also *re-park* pre-batch pending changes it unlocked, or they are silently dropped and the doc diverges when their deps arrive; the same snapshot handles both directions. Note that diff --git a/context/movable-list-op-validation.md b/context/movable-list-op-validation.md index 9e5c454f5..92217d7c5 100644 --- a/context/movable-list-op-validation.md +++ b/context/movable-list-op-validation.md @@ -151,8 +151,8 @@ from the element in `DocState`. Detection and fallback: the same change then panicked with "counter should be continuous". - `ChangeStore::rollback_import` therefore evicts the flushed blocks of every peer the rollback touched. They reload from KV on demand. - - The lookup's parse also registers containers that the arena rollback forgets, - so rollback then drops the parsed changes of the blocks parsed since the + - The lookup's parse also registers container links that the arena rollback + drops, so rollback then drops the parsed changes of the blocks parsed since the import began ([arena-parent-links.md](arena-parent-links.md#import-rollback)). - **Cheap rollback records.** `ChangeStoreRollback` keeps a `BlockShape` for each unflushed pre-scope block an import appended to: change count, the last change's diff --git a/crates/examples/examples/failed_import_rollback_perf.rs b/crates/examples/examples/failed_import_rollback_perf.rs new file mode 100644 index 000000000..fd5ab1488 --- /dev/null +++ b/crates/examples/examples/failed_import_rollback_perf.rs @@ -0,0 +1,103 @@ +//! Ad-hoc perf probe for import rollback (loro-dev/loro#1164): successful imports, which the +//! rollback must not slow down, and the cost of a failed one. +//! +//! Run on two revisions and compare. Not part of CI. +use loro::{ExportMode, LoroDoc, LoroMap}; +use std::time::{Duration, Instant}; + +fn best(rounds: usize, mut f: impl FnMut() -> Duration) -> Duration { + (0..rounds).map(|_| f()).min().unwrap() +} + +fn edit(doc: &LoroDoc, i: usize) { + let text = doc.get_text("text"); + let len = text.len_unicode(); + text.insert((i * 7) % (len + 1), "hello ").unwrap(); + doc.get_list("list").push(i as i64).unwrap(); + let child = doc + .get_map("map") + .insert_container(&format!("k{}", i % 500), LoroMap::new()) + .unwrap(); + child.insert("v", i as i64).unwrap(); + doc.commit(); +} + +fn load(snapshot: &[u8]) -> LoroDoc { + let doc = LoroDoc::new(); + doc.import(snapshot).unwrap(); + let _ = doc.get_deep_value(); + doc +} + +fn main() { + let n: usize = std::env::var("N") + .ok() + .and_then(|x| x.parse().ok()) + .unwrap_or(5_000); + let rounds = 5; + + // Small updates imported one by one. + let src = LoroDoc::new(); + src.set_peer_id(1).unwrap(); + let mut updates = Vec::with_capacity(n); + for i in 0..n { + let vv = src.oplog_vv(); + edit(&src, i); + updates.push(src.export(ExportMode::updates(&vv)).unwrap()); + } + let small = best(rounds, || { + let doc = LoroDoc::new(); + let start = Instant::now(); + for u in &updates { + doc.import(u).unwrap(); + } + start.elapsed() + }); + + // A large update into a doc that has state. + let snapshot = src.export(ExportMode::Snapshot).unwrap(); + let base_vv = src.oplog_vv(); + let other = load(&snapshot); + other.set_peer_id(2).unwrap(); + for i in 0..n { + edit(&other, i); + } + let large_update = other.export(ExportMode::updates(&base_vv)).unwrap(); + let large = best(rounds, || { + let doc = load(&snapshot); + let start = Instant::now(); + doc.import(&large_update).unwrap(); + start.elapsed() + }); + + // A failed import: the large update plus a list insert forged out of bounds. + other.get_list("list").insert(0, -1).unwrap(); + other.commit(); + let mut json = + serde_json::to_value(other.export_json_updates(&base_vv, &other.oplog_vv())).unwrap(); + let last = json["changes"].as_array_mut().unwrap().last_mut().unwrap()["ops"] + .as_array_mut() + .unwrap() + .last_mut() + .unwrap(); + last["content"]["pos"] = 1_000_000_000u64.into(); + let carrier = load(&snapshot); + carrier.detach(); + carrier + .import_json_updates(serde_json::to_string(&json).unwrap()) + .unwrap(); + let bad = carrier.export(ExportMode::updates(&base_vv)).unwrap(); + let doc = load(&snapshot); + let failed = best(rounds, || { + let start = Instant::now(); + assert!(doc.import(&bad).is_err()); + start.elapsed() + }); + + println!( + "n={n} small_updates_one_by_one={:.2}ms large_update_into_state={:.2}ms failed_import={:.2}ms", + small.as_secs_f64() * 1e3, + large.as_secs_f64() * 1e3, + failed.as_secs_f64() * 1e3, + ); +} diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index 7537b75bd..29732c2af 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -48,7 +48,9 @@ over graceful degradation. - `tests/mergeable_container/` and `tests/mergeable_cid_encoding.rs`: focused mergeable container regression tests. - `src/tests/import_atomicity.rs`: import rollback and malformed-input - regressions. + regressions. A failed import keeps its arena indices and drops only parent + links, values, and the dead-container cache: + [../../context/failed-import-arena-indices.md](../../context/failed-import-arena-indices.md). - Movable-list `Move`/`Set` element validation on import, plus change-store rollback records and KV block-range decoding: [../../context/movable-list-op-validation.md](../../context/movable-list-op-validation.md). diff --git a/crates/loro-internal/src/arena.rs b/crates/loro-internal/src/arena.rs index 4faa19afa..074eb50ad 100644 --- a/crates/loro-internal/src/arena.rs +++ b/crates/loro-internal/src/arena.rs @@ -104,8 +104,6 @@ pub struct SharedArena { pub(crate) struct SharedArenaRollback { container_len: usize, - root_len: usize, - top_level_root_len: usize, values_len: usize, str: StrArenaCheckpoint, } @@ -331,15 +329,11 @@ impl SharedArena { pub(crate) fn checkpoint_for_rollback(&self) -> SharedArenaRollback { let containers = self.inner.containers.read(); let container_len = containers.container_idx_to_id.len(); - let root_len = containers.root_c_idx.len(); - let top_level_root_len = containers.top_level_root_c_idx.len(); drop(containers); let values_len = self.inner.values.lock().len(); let str = self.inner.str.lock().checkpoint(); SharedArenaRollback { container_len, - root_len, - top_level_root_len, values_len, str, } @@ -348,27 +342,34 @@ impl SharedArena { /// Call it through the op log's change store (`ChangeStore::rollback_arena` and friends), /// which does it under the lock the creator resolver parses under and drops the parsed /// changes that may refer to what this removes. See `context/arena-parent-links.md`. + /// + /// Container indices are never freed: a container registered since the checkpoint keeps + /// its index. `DocState` (store entries, caches) and handlers on other threads may hold + /// it, and a new container that reused it would inherit their state (loro-dev/loro#1164). + /// What the failed import may have told the arena about those containers is dropped: the + /// parent links of normal containers registered since the checkpoint, and links to them. + /// The resolvers find the links again from the kept history or the state. Roots keep their + /// links, which their IDs determine. Values and text are truncated; only parsed changes + /// refer to them. See `context/failed-import-arena-indices.md`. pub(crate) fn rollback(&self, checkpoint: SharedArenaRollback) { - let mut containers = self.inner.containers.write(); - let removed_ids = containers - .container_idx_to_id - .split_off(checkpoint.container_len); - for id in removed_ids { - containers.container_id_to_idx.remove(&id); + { + let mut guard = self.inner.containers.write(); + let containers = &mut *guard; + let container_len = checkpoint.container_len; + if containers.container_idx_to_id.len() > container_len { + let ids = &containers.container_idx_to_id; + containers.parents.retain(|child, parent| { + let child_is_new = (child.to_index() as usize) >= container_len; + let parent_is_new = + parent.is_some_and(|p| (p.to_index() as usize) >= container_len); + !((child_is_new || parent_is_new) && ids[child.to_index() as usize].is_normal()) + }); + // A cached depth may have been computed through a dropped link, also for the + // descendants of a container registered before the checkpoint. Roots and + // links that are kept give the same depth again. + containers.depth.fill(None); + } } - containers.depth.truncate(checkpoint.container_len); - containers.root_c_idx.truncate(checkpoint.root_len); - containers - .top_level_root_c_idx - .truncate(checkpoint.top_level_root_len); - containers.parents.retain(|child, parent| { - let child_is_kept = (child.to_index() as usize) < checkpoint.container_len; - let parent_is_kept = parent - .map(|p| (p.to_index() as usize) < checkpoint.container_len) - .unwrap_or(true); - child_is_kept && parent_is_kept - }); - drop(containers); self.inner.values.lock().truncate(checkpoint.values_len); self.inner.str.lock().rollback(checkpoint.str); diff --git a/crates/loro-internal/src/encoding/fast_snapshot.rs b/crates/loro-internal/src/encoding/fast_snapshot.rs index cf0542160..526fe2c1f 100644 --- a/crates/loro-internal/src/encoding/fast_snapshot.rs +++ b/crates/loro-internal/src/encoding/fast_snapshot.rs @@ -251,7 +251,7 @@ pub(crate) fn decode_snapshot_inner( if let Err(e) = decode_result { state.reset_to_empty_for_failed_snapshot_import(); - oplog.reset_to_empty_for_failed_snapshot_import(arena_checkpoint, &state); + oplog.reset_to_empty_for_failed_snapshot_import(arena_checkpoint, &mut state); return Err(e); } drop(state); @@ -263,7 +263,7 @@ pub(crate) fn decode_snapshot_inner( let mut oplog = doc.oplog().lock(); let mut state = doc.app_state().lock(); state.reset_to_empty_for_failed_snapshot_import(); - oplog.reset_to_empty_for_failed_snapshot_import(arena_checkpoint, &state); + oplog.reset_to_empty_for_failed_snapshot_import(arena_checkpoint, &mut state); return Err(e); } debug_assert_eq!(doc.state_frontiers(), doc.oplog_frontiers()); diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index b2bac74a4..571b5b679 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -725,7 +725,7 @@ impl LoroDoc { }, diff_mode, ) { - oplog.rollback_owned_import(owns_rollback, &state); + oplog.rollback_owned_import(owns_rollback, &mut state); return Err(e); } } @@ -743,7 +743,7 @@ impl LoroDoc { if keep_prefix { oplog.commit_owned_import_rollback(owns_rollback); } else { - oplog.rollback_owned_import(owns_rollback, &self.state.lock()); + oplog.rollback_owned_import(owns_rollback, &mut self.state.lock()); } Err(e) } @@ -755,7 +755,7 @@ impl LoroDoc { Ok(result) } Err(e) => { - oplog.rollback_owned_import(owns_rollback, &self.state.lock()); + oplog.rollback_owned_import(owns_rollback, &mut self.state.lock()); Err(e) } } @@ -776,7 +776,7 @@ impl LoroDoc { { Ok(changes) => changes, Err(e) => { - oplog.rollback_arena(arena_checkpoint, &self.state.lock()); + oplog.rollback_arena(arena_checkpoint, &mut self.state.lock()); return Err(e); } }; @@ -785,7 +785,7 @@ impl LoroDoc { if preflight.has_deps_before_shallow_root && (self.is_detached() || !preflight.applies_to_dag) { - oplog.rollback_arena(arena_checkpoint, &self.state.lock()); + oplog.rollback_arena(arena_checkpoint, &mut self.state.lock()); return Err(LoroError::ImportUpdatesThatDependsOnOutdatedVersion); } @@ -800,7 +800,7 @@ impl LoroDoc { let result = encoding::apply_decoded_changes_to_oplog(&mut oplog, changes); if owns_rollback { if let Err(e) = oplog.validate_movable_list_elem_refs_in_import_scope() { - oplog.rollback_import(&self.state.lock()); + oplog.rollback_import(&mut self.state.lock()); return Err(e); } oplog.commit_import_rollback(); @@ -816,7 +816,7 @@ impl LoroDoc { let pending_root_containers = pending_root_containers_to_materialize(&oplog, &changes); let result = encoding::apply_decoded_changes_to_oplog(&mut oplog, changes); if result.has_deps_before_shallow_root { - oplog.rollback_arena(arena_checkpoint, &self.state.lock()); + oplog.rollback_arena(arena_checkpoint, &mut self.state.lock()); return Err(LoroError::ImportUpdatesThatDependsOnOutdatedVersion); } @@ -851,7 +851,7 @@ impl LoroDoc { // pending changes hold movable-list ops, so other imports skip the scan. if rollback_enabled { if let Err(e) = oplog.validate_movable_list_elem_refs_in_import_scope() { - oplog.rollback_import(&self.state.lock()); + oplog.rollback_import(&mut self.state.lock()); return Err(e); } } @@ -909,7 +909,7 @@ impl LoroDoc { diff_mode, ) { if rollback_enabled { - oplog.rollback_import(&state); + oplog.rollback_import(&mut state); return Err(e); } @@ -3246,7 +3246,7 @@ impl BatchImportGuard<'_> { // still at its pre-batch version; undoing the batch in the `OpLog` // makes the two agree again, which is what lets the doc stay attached. tracing::warn!("import_batch cannot reattach, rolling the batch back: {e}"); - doc.oplog.lock().rollback_import(&doc.state.lock()); + doc.oplog.lock().rollback_import(&mut doc.state.lock()); // The shared diff calculator cached ranges against the rolled-back // history; drop that cache instead of reusing stale entries. *doc.diff_calculator.lock() = DiffCalculator::new(true); diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index 663f8cd51..9e8513036 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -360,7 +360,7 @@ impl OpLog { /// Roll back an import rollback scope this caller owns (see [`Self::rollback_import`]). /// No-op when `owns` is false โ€” the scope then belongs to an outer owner such as /// `import_batch`. - pub(crate) fn rollback_owned_import(&mut self, owns: bool, state: &DocState) { + pub(crate) fn rollback_owned_import(&mut self, owns: bool, state: &mut DocState) { if owns { self.rollback_import(state); } @@ -426,14 +426,17 @@ impl OpLog { /// Undo the open import rollback scope, including the arena. /// - /// `_state` shows that the caller holds the state lock. A reader under that lock can + /// `state` shows that the caller holds the state lock. A reader under that lock can /// register containers through the creator resolver, which runs without the op log lock, - /// and use their indices before it returns; the arena rollback must not free them in - /// between. See `context/arena-parent-links.md`. - pub(crate) fn rollback_import(&mut self, _state: &DocState) { + /// and resolve their parent links before it returns; the arena rollback must not drop + /// those links in between. See `context/arena-parent-links.md`. The state's caches that + /// depend on parent links are cleared as well + /// (`context/failed-import-arena-indices.md`). + pub(crate) fn rollback_import(&mut self, state: &mut DocState) { let Some(rollback) = self.import_rollback.take() else { return; }; + state.forget_parent_link_caches_after_failed_import(); // Also rolls back the arena; see `ChangeStore::rollback_import`. self.change_store @@ -446,17 +449,23 @@ impl OpLog { /// Rolls the arena back to a checkpoint taken before an import that failed before its /// import rollback scope began. See [`ChangeStore::rollback_arena`], and - /// [`Self::rollback_import`] for `_state`. - pub(crate) fn rollback_arena(&self, arena_checkpoint: SharedArenaRollback, _state: &DocState) { + /// [`Self::rollback_import`] for `state`. + pub(crate) fn rollback_arena( + &self, + arena_checkpoint: SharedArenaRollback, + state: &mut DocState, + ) { + state.forget_parent_link_caches_after_failed_import(); self.change_store.rollback_arena(arena_checkpoint); } - /// See [`Self::rollback_import`] for `_state`. + /// See [`Self::rollback_import`] for `state`. pub(crate) fn reset_to_empty_for_failed_snapshot_import( &mut self, arena_checkpoint: SharedArenaRollback, - _state: &DocState, + state: &mut DocState, ) { + state.forget_parent_link_caches_after_failed_import(); let arena = self.arena.clone(); let configure = self.configure.clone(); // Also rolls back the arena; see `ChangeStore::retire`. diff --git a/crates/loro-internal/src/oplog/change_store.rs b/crates/loro-internal/src/oplog/change_store.rs index 12c354157..e52c79e67 100644 --- a/crates/loro-internal/src/oplog/change_store.rs +++ b/crates/loro-internal/src/oplog/change_store.rs @@ -483,11 +483,12 @@ impl ChangeStore { /// the parsed changes of the cached blocks that were parsed since. Every arena rollback must /// go through here (or [`Self::rollback_import`] / [`Self::retire`]). /// - /// Parsing a block registers the containers its ops use and allocates their values, and - /// the arena rollback drops what was registered or allocated after the checkpoint, so a - /// block parsed in between may hold indices and value slices that no longer exist or that - /// new registrations reuse. Such a block keeps only its bytes, so the next access parses and - /// registers again. A block parsed before the checkpoint can only refer to what was there + /// Parsing a block registers the containers its ops use with their parent links and + /// allocates their values. The arena rollback truncates the values and text allocated after + /// the checkpoint and drops the parent links of containers registered after it (their + /// indices stay; see `SharedArena::rollback`), so a block parsed in between may hold value + /// slices that no longer exist, and parsing it again is what registers those links again. + /// Such a block keeps only its bytes, so the next access parses and registers again. A block parsed before the checkpoint can only refer to what was there /// then (its `parsed_extent`), and keeps its parsed changes. A block without bytes was built /// in memory from changes inserted before the import, whose containers were registered /// then. diff --git a/crates/loro-internal/src/state/AGENTS.md b/crates/loro-internal/src/state/AGENTS.md index d6c42ebbb..7572efb81 100644 --- a/crates/loro-internal/src/state/AGENTS.md +++ b/crates/loro-internal/src/state/AGENTS.md @@ -29,6 +29,9 @@ before changing mergeable child behavior. final; tree and movable-list ones are dropped whenever a move could revive them; mergeable ones are never cached: [../../../../context/dead-container-cache.md](../../../../context/dead-container-cache.md). + A failed import keeps its arena indices, so store entries and caches keyed + by `ContainerIdx` stay valid; it clears this cache: + [../../../../context/failed-import-arena-indices.md](../../../../context/failed-import-arena-indices.md). - `unknown_state.rs` and `../diff_calc/unknown.rs`: forward compatibility for unknown container types. They are never created locally; the `apply_diff` pre-check and undo rules are in diff --git a/crates/loro-internal/src/state/dead_containers_cache.rs b/crates/loro-internal/src/state/dead_containers_cache.rs index d3382fa7a..e2ccaa448 100644 --- a/crates/loro-internal/src/state/dead_containers_cache.rs +++ b/crates/loro-internal/src/state/dead_containers_cache.rs @@ -36,6 +36,14 @@ impl DeadContainersCache { } impl DocState { + /// Called when a failed import is rolled back. The import may have registered parent links + /// that the arena rollback drops, and a deletion found through one of them while it ran + /// (e.g. by a query on another thread) is not true of the kept history. + /// See `context/failed-import-arena-indices.md`. + pub(crate) fn forget_parent_link_caches_after_failed_import(&mut self) { + self.dead_containers_cache.clear(); + } + pub(crate) fn is_deleted(&mut self, idx: ContainerIdx) -> bool { #[cfg(not(debug_assertions))] { diff --git a/crates/loro-wasm/tests/failed_import_state_rollback.test.ts b/crates/loro-wasm/tests/failed_import_state_rollback.test.ts new file mode 100644 index 000000000..903f6a682 --- /dev/null +++ b/crates/loro-wasm/tests/failed_import_state_rollback.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc, LoroMap, LoroText, TreeID } from "../bundler/index"; + +// A failed import must not leave state behind for containers created later: a new root +// map used to show the text of a tree node's metadata that the failed import had loaded +// while it validated. See loro-dev/loro#1164 and context/failed-import-arena-indices.md. + +const NODES = 40; + +function snapshotAndBadUpdate(): { snap: Uint8Array; bad: Uint8Array } { + // Peer 2 creates nodes under `p` and puts a text in their metas; peer 1 deletes `p` + // concurrently. + const a = new LoroDoc(); + a.setPeerId(1); + const p = a.getTree("tree").createNode(); + a.commit(); + const b = new LoroDoc(); + b.setPeerId(2); + b.import(a.export({ mode: "update" })); + const nodes: TreeID[] = []; + for (let i = 0; i < NODES; i++) { + const c = b.getTree("tree").getNodeByID(p.id)!.createNode(); + c.data.setContainer("t", new LoroText()).insert(0, `z${i}`); + nodes.push(c.id); + } + b.commit(); + a.getTree("tree").delete(p.id); + a.commit(); + a.import(b.export({ mode: "update" })); + a.getMap("m").set("k", 1); + a.commit(); + const snap = a.export({ mode: "snapshot" }); + const vv = a.oplogVersion(); + + // Peer 4 revives the nodes and edits their metas, then a text insert forged out of + // bounds, which the state rejects. Which diffs the state validates before it rejects + // the text depends on hash order (it differs between wasm32 and native), so there are + // many metas. + const e = new LoroDoc(); + e.setPeerId(4); + e.import(snap); + for (const id of nodes) { + e.getTree("tree").move(id, undefined); + e.getTree("tree").getNodeByID(id)!.data.set("r", 1); + } + e.getText("text").insert(0, "a"); + e.commit(); + const json = e.exportJsonUpdates(vv, e.oplogVersion()); + const ops = json.changes[json.changes.length - 1].ops; + (ops[ops.length - 1].content as { pos: number }).pos = 1000; + const carrier = new LoroDoc(); + carrier.import(snap); + carrier.detach(); + carrier.importJsonUpdates(json); + const bad = carrier.export({ mode: "update", from: vv }); + return { snap, bad }; +} + +function load(bytes: Uint8Array): LoroDoc { + const doc = new LoroDoc(); + doc.import(bytes); + doc.setPeerId(99); + return doc; +} + +describe("failed import state rollback", () => { + it("containers created after a failed import get their own state", () => { + const { snap, bad } = snapshotAndBadUpdate(); + const doc = load(snap); + const reference = load(snap); + expect(() => doc.import(bad)).toThrow(); + expect(() => doc.import(bad)).toThrow(); + for (const d of [doc, reference]) { + for (let i = 0; i < 2 * NODES; i++) { + const child = d.getMap(`fresh${i}`).setContainer("c", new LoroMap()); + child.set("v", i); + d.commit(); + expect(child.isDeleted()).toBe(false); + } + } + expect(doc.toJSON()).toEqual(reference.toJSON()); + expect(load(doc.export({ mode: "snapshot" })).toJSON()).toEqual( + reference.toJSON(), + ); + }); +}); diff --git a/crates/loro/tests/failed_import_state_rollback.rs b/crates/loro/tests/failed_import_state_rollback.rs new file mode 100644 index 000000000..4b841499c --- /dev/null +++ b/crates/loro/tests/failed_import_state_rollback.rs @@ -0,0 +1,370 @@ +//! A failed import must not leave `DocState` entries, handlers, or caches pointing at arena +//! indices that its rollback handed out to other containers (loro-dev/loro#1164). See +//! `context/failed-import-arena-indices.md`. + +use loro::{ContainerTrait, ExportMode, LoroDoc, LoroMap, LoroText, ToJson, TreeParentId}; + +fn import(bytes: &[u8]) -> LoroDoc { + let doc = LoroDoc::new(); + doc.import(bytes).unwrap(); + doc.set_peer_id(99).unwrap(); + doc +} + +/// A snapshot where `c` (created by peer 2 under `p`, which peer 1 deleted concurrently) has a +/// text in its meta, and an update that revives `c`, edits its meta, and ends with a list insert +/// forged out of bounds, which the state rejects. Also returns the update before it was forged. +fn snapshot_bad_and_good_update() -> (Vec, Vec, Vec) { + // Peer 2 creates `c` under `p` and puts a text in its meta; peer 1 deletes `p` concurrently. + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let p = a.get_tree("tree").create(TreeParentId::Root).unwrap(); + a.commit(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + let c = b.get_tree("tree").create(p).unwrap(); + let meta = b.get_tree("tree").get_meta(c).unwrap(); + meta.insert_container("t", LoroText::new()) + .unwrap() + .insert(0, "z") + .unwrap(); + b.commit(); + a.get_tree("tree").delete(p).unwrap(); + a.commit(); + a.import(&b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + a.get_map("m").insert("k", 1).unwrap(); + a.commit(); + let snap = a.export(ExportMode::Snapshot).unwrap(); + let vv = a.oplog_vv(); + + // Peer 4 revives `c` and edits its meta, then a list insert forged out of bounds, which + // the state rejects. + let e = LoroDoc::new(); + e.set_peer_id(4).unwrap(); + e.import(&snap).unwrap(); + e.get_tree("tree").mov(c, TreeParentId::Root).unwrap(); + e.get_tree("tree") + .get_meta(c) + .unwrap() + .insert("r", 1) + .unwrap(); + e.get_list("list").insert(0, 1).unwrap(); + e.get_list("list").insert(1, 2).unwrap(); + e.commit(); + let mut json = serde_json::to_value(e.export_json_updates(&vv, &e.oplog_vv())).unwrap(); + let last = json["changes"].as_array_mut().unwrap().last_mut().unwrap()["ops"] + .as_array_mut() + .unwrap() + .last_mut() + .unwrap(); + last["content"]["pos"] = 1000.into(); + let carrier = LoroDoc::new(); + carrier.import(&snap).unwrap(); + carrier.detach(); + carrier + .import_json_updates(serde_json::to_string(&json).unwrap()) + .unwrap(); + let bad = carrier.export(ExportMode::updates(&vv)).unwrap(); + let good = e.export(ExportMode::updates(&vv)).unwrap(); + (snap, bad, good) +} + +/// [`snapshot_bad_and_good_update`] without the update before it was forged. +fn snapshot_and_bad_update() -> (Vec, Vec) { + let (snap, bad, _) = snapshot_bad_and_good_update(); + (snap, bad) +} + +/// The repro from loro-dev/loro#1164, verbatim. +#[test] +fn failed_import_leaves_state_at_freed_indices() { + // Peer 2 creates `c` under `p` and puts a text in its meta; peer 1 deletes `p` concurrently. + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let p = a.get_tree("tree").create(TreeParentId::Root).unwrap(); + a.commit(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + let c = b.get_tree("tree").create(p).unwrap(); + let meta = b.get_tree("tree").get_meta(c).unwrap(); + meta.insert_container("t", LoroText::new()) + .unwrap() + .insert(0, "z") + .unwrap(); + b.commit(); + a.get_tree("tree").delete(p).unwrap(); + a.commit(); + a.import(&b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + a.get_map("m").insert("k", 1).unwrap(); + a.commit(); + let snap = a.export(ExportMode::Snapshot).unwrap(); + let vv = a.oplog_vv(); + + // Peer 4 revives `c` and edits its meta, then a list insert forged out of bounds, which + // the state rejects. + let e = LoroDoc::new(); + e.set_peer_id(4).unwrap(); + e.import(&snap).unwrap(); + e.get_tree("tree").mov(c, TreeParentId::Root).unwrap(); + e.get_tree("tree") + .get_meta(c) + .unwrap() + .insert("r", 1) + .unwrap(); + e.get_list("list").insert(0, 1).unwrap(); + e.get_list("list").insert(1, 2).unwrap(); + e.commit(); + let mut json = serde_json::to_value(e.export_json_updates(&vv, &e.oplog_vv())).unwrap(); + let last = json["changes"].as_array_mut().unwrap().last_mut().unwrap()["ops"] + .as_array_mut() + .unwrap() + .last_mut() + .unwrap(); + last["content"]["pos"] = 1000.into(); + let carrier = LoroDoc::new(); + carrier.import(&snap).unwrap(); + carrier.detach(); + carrier + .import_json_updates(serde_json::to_string(&json).unwrap()) + .unwrap(); + let bad = carrier.export(ExportMode::updates(&vv)).unwrap(); + + let doc = import(&snap); + let reference = import(&snap); + assert!(doc.import(&bad).is_err()); + for d in [&doc, &reference] { + d.get_map("fresh") + .insert_container("c0", LoroMap::new()) + .unwrap() + .insert("v", 1) + .unwrap(); + d.commit(); + } + // Fails: `fresh` also shows `"t": "z"` from `c`'s meta. + assert_eq!( + doc.get_deep_value().to_json_value(), + reference.get_deep_value().to_json_value() + ); +} + +/// After a failed import, new containers (roots and children) get their own state, the doc +/// round-trips through a snapshot, and every later edit matches a doc that never saw the import. +#[test] +fn containers_created_after_a_failed_import_match_a_clean_doc() { + let (snap, bad) = snapshot_and_bad_update(); + let doc = import(&snap); + let reference = import(&snap); + assert!(doc.import(&bad).is_err()); + assert!(doc.import(&bad).is_err()); + for d in [&doc, &reference] { + for i in 0..10 { + let root = d.get_map(format!("fresh{i}").as_str()); + let child = root.insert_container("c", LoroMap::new()).unwrap(); + child.insert("v", i).unwrap(); + child + .insert_container("t", LoroText::new()) + .unwrap() + .insert(0, "x") + .unwrap(); + d.get_list("list").push(i).unwrap(); + d.commit(); + assert!(!child.is_deleted()); + assert!(!root.is_deleted()); + } + } + let expected = reference.get_deep_value().to_json_value(); + assert_eq!(doc.get_deep_value().to_json_value(), expected); + assert_eq!( + import(&doc.export(ExportMode::Snapshot).unwrap()) + .get_deep_value() + .to_json_value(), + expected + ); + assert_eq!( + import(&doc.export(ExportMode::all_updates()).unwrap()) + .get_deep_value() + .to_json_value(), + expected + ); + doc.check_state_correctness_slow(); +} + +/// Retrying the same invalid update is rejected again, and leaves the doc unchanged. +#[test] +fn retrying_a_failed_import_is_rejected_again() { + let (snap, bad) = snapshot_and_bad_update(); + let doc = import(&snap); + let reference = import(&snap); + for i in 0..5 { + assert!(doc.import(&bad).is_err()); + assert_eq!( + doc.get_deep_value().to_json_value(), + reference.get_deep_value().to_json_value() + ); + assert_eq!(doc.oplog_vv(), reference.oplog_vv()); + // Register a new container between the attempts. + for d in [&doc, &reference] { + let _ = d.get_map(format!("between{i}").as_str()); + } + } + // Also through `import_batch`. + assert!(doc.import_batch(&[bad.clone()]).is_err()); + assert!(doc.import(&bad).is_err()); + assert_eq!( + doc.get_deep_value().to_json_value(), + reference.get_deep_value().to_json_value() + ); + doc.check_state_correctness_slow(); +} + +/// The containers the failed import registered (the revived node's meta, the new list) work +/// when the valid version of the update arrives later. +#[test] +fn valid_update_after_a_failed_import_matches_a_clean_doc() { + let (snap, bad, good) = snapshot_bad_and_good_update(); + let doc = import(&snap); + let reference = import(&snap); + assert!(doc.import(&bad).is_err()); + // Queries between the attempts that touch the containers of the failed import. + let _ = doc.get_deep_value(); + assert!(doc.get_list("list").is_empty()); + doc.import(&good).unwrap(); + reference.import(&good).unwrap(); + let expected = reference.get_deep_value().to_json_value(); + assert_eq!(doc.get_deep_value().to_json_value(), expected); + for d in [&doc, &reference] { + d.get_map("fresh") + .insert_container("c0", LoroMap::new()) + .unwrap() + .insert("v", 1) + .unwrap(); + d.commit(); + } + assert_eq!( + doc.get_deep_value().to_json_value(), + reference.get_deep_value().to_json_value() + ); + doc.check_state_correctness_slow(); +} + +/// Another thread registers containers (root handlers, tree metas) while an import fails. Its +/// handlers keep working after the rollback, and containers created afterwards get their own +/// state. Before loro-dev/loro#1164 the rollback freed the indices those handlers held. +/// +/// Timing-dependent: each trial overlaps the queries with two failing imports. The number of +/// trials is `FAILED_IMPORT_THREAD_TRIALS` (default 8). +#[test] +fn handlers_created_during_a_failed_import_keep_their_containers() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::{Arc, Barrier}; + + let (snap, _) = snapshot_and_bad_update(); + let base = import(&snap); + let c = base + .get_tree("tree") + .get_nodes(true) + .iter() + .find(|n| matches!(n.parent, TreeParentId::Node(_))) + .map(|n| n.id) + .unwrap(); + let vv = base.oplog_vv(); + // A bad update that registers many containers before the state rejects it. + let e = import(&snap); + e.set_peer_id(4).unwrap(); + e.get_tree("tree").mov(c, TreeParentId::Root).unwrap(); + e.get_tree("tree") + .get_meta(c) + .unwrap() + .insert("r", 1) + .unwrap(); + for i in 0..300 { + let child = e + .get_map("new") + .insert_container(&format!("n{i}"), LoroMap::new()) + .unwrap(); + child.insert("v", i).unwrap(); + e.commit(); + } + e.get_list("list").insert(0, 1).unwrap(); + e.get_list("list").insert(1, 2).unwrap(); + e.commit(); + let mut json = serde_json::to_value(e.export_json_updates(&vv, &e.oplog_vv())).unwrap(); + let last = json["changes"].as_array_mut().unwrap().last_mut().unwrap()["ops"] + .as_array_mut() + .unwrap() + .last_mut() + .unwrap(); + last["content"]["pos"] = 1000.into(); + let carrier = import(&snap); + carrier.detach(); + carrier + .import_json_updates(serde_json::to_string(&json).unwrap()) + .unwrap(); + let bad = carrier.export(ExportMode::updates(&vv)).unwrap(); + + let trials: usize = std::env::var("FAILED_IMPORT_THREAD_TRIALS") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(8); + for trial in 0..trials { + let doc = import(&snap); + let barrier = Arc::new(Barrier::new(2)); + let done = Arc::new(AtomicBool::new(false)); + let querier = { + let doc = doc.clone(); + let barrier = barrier.clone(); + let done = done.clone(); + std::thread::spawn(move || { + barrier.wait(); + let mut handlers = Vec::new(); + let mut n = 0; + while !done.load(Ordering::Acquire) || n < 4 { + let meta = doc.get_tree("tree").get_meta(c).unwrap(); + assert!(meta.is_deleted()); + handlers.push((n, doc.get_map(format!("fresh{n}").as_str()))); + n += 1; + std::thread::yield_now(); + } + handlers + }) + }; + barrier.wait(); + assert!(doc.import(&bad).is_err()); + assert!(doc.import(&bad).is_err()); + done.store(true, Ordering::Release); + let handlers = querier.join().unwrap(); + + let reference = import(&snap); + for (n, map) in handlers.iter() { + map.insert("v", *n).unwrap(); + reference + .get_map(format!("fresh{n}").as_str()) + .insert("v", *n) + .unwrap(); + } + for d in [&doc, &reference] { + for i in 0..10 { + let child = d + .get_map("after") + .insert_container(&format!("c{i}"), LoroMap::new()) + .unwrap(); + child.insert("v", i).unwrap(); + assert!(!child.is_deleted()); + } + d.commit(); + } + assert_eq!( + doc.get_deep_value().to_json_value(), + reference.get_deep_value().to_json_value(), + "trial {trial}" + ); + assert!(doc.get_tree("tree").get_meta(c).unwrap().is_deleted()); + doc.check_state_correctness_slow(); + } +} From 8c20916b4daa433fbfb4cf9ae6a4b7a4f30d7b51 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 13:47:28 +0800 Subject: [PATCH 08/10] fix: emit tree events for revived subtrees on import (#1157) In the Linear and ImportGreaterUpdates diff modes the tree diff is the raw ops, and TreeState::apply_diff_and_convert labeled a move that brought a node out of a deleted subtree as a single Create. The nodes below it got no Create, so a tree rebuilt from events lost them. In Linear mode a move of an alive node under a deleted one also emitted no Delete, and in ImportGreaterUpdates mode Create ops skipped the cycle check that apply_diff applies. Raw Create/Move ops are now classified by whether the target is alive before and after the op, and a revived node is followed by a Create for each node of its subtree. Checkout-mode diffs are unchanged. Rationale and the local-event gap: context/tree-events.md. Co-Authored-By: Claude Opus 5.5 --- .changeset/tree-revival-create-event.md | 5 + AGENTS.md | 2 + context/CONTEXT-GAPS.md | 1 + context/tree-events.md | 80 +++ crates/loro-internal/AGENTS.md | 2 + crates/loro-internal/src/state/AGENTS.md | 2 + crates/loro-internal/src/state/tree_state.rs | 248 +++++++--- .../tests/tree_revival_events.test.ts | 142 ++++++ crates/loro/tests/tree_revival_events.rs | 463 ++++++++++++++++++ 9 files changed, 870 insertions(+), 75 deletions(-) create mode 100644 .changeset/tree-revival-create-event.md create mode 100644 context/tree-events.md create mode 100644 crates/loro-wasm/tests/tree_revival_events.test.ts create mode 100644 crates/loro/tests/tree_revival_events.rs diff --git a/.changeset/tree-revival-create-event.md b/.changeset/tree-revival-create-event.md new file mode 100644 index 000000000..f930bcd3b --- /dev/null +++ b/.changeset/tree-revival-create-event.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix missing tree events on import. When an imported move brought a node back out of a deleted subtree, only the node got a `create` event, not the nodes below it, so a tree rebuilt from events lost them. Importing a move of a node under a deleted node also emitted no `delete` event in some cases. This happened when the import had no concurrency with the local document, for example on a peer that only receives updates. diff --git a/AGENTS.md b/AGENTS.md index 53791cd03..ba0179465 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,6 +40,8 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. and delete `start_id`s: [context/richtext-insert-positions.md](context/richtext-insert-positions.md). - Tree diff cache transitions and why their lamport window comes from the two versions, not the replay base: [context/tree-checkout-window.md](context/tree-checkout-window.md). +- Tree event invariants (revived subtrees get `Create`s) and the two event + paths, diff-calculator vs raw ops: [context/tree-events.md](context/tree-events.md). - loro.js tree diff item order, index counting, apply resolution, and batch dry-run: [context/loro-js-tree-diff.md](context/loro-js-tree-diff.md). - User-facing Loro usage, sync, editor integration, and performance guidance: diff --git a/context/CONTEXT-GAPS.md b/context/CONTEXT-GAPS.md index 9b92d88f0..e7a08807a 100644 --- a/context/CONTEXT-GAPS.md +++ b/context/CONTEXT-GAPS.md @@ -5,3 +5,4 @@ fix the docs in that change. Format: `YYYY-MM-DD | | | why it was hard | suggested home` +2026-09-30 | Does `fuzz::actor::assert_value_eq` / `Actor::check_tracker` fail on a mismatch? | No: since #953 it only `tracing::warn!`s (`crates/fuzz/src/actor.rs`), so tracker and value checks in the fuzz crate (including `crates/fuzz/tests/checkout_path.rs`, which `context/tree-checkout-window.md` describes as comparing values) fail only when the tracker panics | a randomized repro of loro-dev/loro#1157 passed on the old code | `context/tree-checkout-window.md` Tests section, or make it assert diff --git a/context/tree-events.md b/context/tree-events.md new file mode 100644 index 000000000..ffa103f8f --- /dev/null +++ b/context/tree-events.md @@ -0,0 +1,80 @@ +# Tree Events + +Verified against code 2026-09-30. + +Tree events (`TreeExternalDiff::{Create, Move, Delete}`) describe only the +alive tree, the nodes reachable from the root. Applied in order, the events of +one batch must turn the alive tree before it into the alive tree after it: + +- `Create` for every node that becomes alive, including each node below a + node that leaves a deleted subtree (the subtree comes back with it); +- `Delete` only for the root of a subtree that stops being alive: it stands + for the whole subtree, with no `Delete` for the descendants; +- no event for changes inside deleted subtrees; +- every `index` / `old_index` is valid when the events are applied in order. + +`trigger_on_new_container` (`crates/loro-internal/src/state.rs`) sends the +metadata map of each `Create` target as a new container, so a revived node's +metadata arrives in map events right after the tree events. + +## Two ways to build the events + +`TreeState::apply_diff_and_convert` +(`crates/loro-internal/src/state/tree_state.rs`) gets a diff whose shape +depends on the mode returned by `TreeDiffCalculator::calculate_diff` +(`crates/loro-internal/src/diff_calc/tree.rs`): + +- `DiffMode::Checkout` (checkouts and concurrent imports): the calculator + classifies each item against its own cache (`TreeDeltaItem::new`: + `Create`, `Move`, `Delete`, `MoveInDelete`, `UnCreate`) and follows every + `Create` with its current subtree (`TreeCacheForDiff::push_children_creation`). + The state only converts items. +- `DiffMode::Linear` and `DiffMode::ImportGreaterUpdates`: the items are the + raw ops, sorted by lamport. `TreeInternalDiff::Create` / `Move` only name + the op type, not what happens to the alive tree. The state classifies each + op by whether the target is alive before and after it + (`TreeState::apply_raw_move_and_convert`) and, when a node that existed + becomes alive, pushes a `Create` for its subtree from the state's children + lists (`TreeState::push_subtree_creation`). A `Create` or `Move` that leaves + the node alive where it was produces no event. + +Before 2026-09-30 the raw path treated a `Move` that revived a node as a +single `Create` and a `Create` op as a `Create` whenever its parent was +alive, so (loro-dev/loro#1157): + +- the nodes below a node moved out of a deleted subtree got no `Create`; +- in `Linear` mode a move of an alive node under a deleted one emitted + nothing instead of `Delete`; +- in `ImportGreaterUpdates` mode a `Create` op skipped the cycle check that + `apply_diff` applies, so the state could differ with and without a + subscriber. + +The raw path only runs when the op log has no concurrency with the current +state (`Linear`), or when the new ops all follow the state's version +(`ImportGreaterUpdates`), for example a peer that only receives updates. Two- +or three-peer repros that sync both ways usually take the `Checkout` path. + +## Local events are different + +Local tree ops build their events from `EventHint::Tree` in +`crates/loro-internal/src/handler/tree.rs` and do not go through +`apply_diff_and_convert`. The local API allows creating under, moving into and +moving out of deleted nodes, and those hints still use the shapes of alive +nodes (`Create` under a deleted parent, `Move` with a deleted `old_parent`). +The undo manager inverts those events, so changing them is a separate change. + +## Tests + +- `crates/loro/tests/tree_revival_events.rs`: a mirror rebuilt only from + events (children lists and metadata) is compared with the state after + imports: the repro blobs from the issue, `Linear` and + `ImportGreaterUpdates` revivals, a move into a deleted subtree, an undone + delete, and `random_sync_events_rebuild_the_tree` (random tree edits on + three peers plus two observers that only import; wider sweeps with + `TREE_EVENT_SEEDS=0..3000 cargo test --release -p loro --test tree_revival_events`). + Local edits reset the mirror (see above). +- `crates/loro-wasm/tests/tree_revival_events.test.ts`: the two `Linear` + cases through `loro-crdt`. +- The fuzz `Actor` tracker (`crates/fuzz/src/actor.rs`) does not catch wrong + events on its own: `assert_value_eq` only logs a mismatch, so only a panic + inside the tracker fails a run. diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index 29732c2af..e23176236 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -37,6 +37,8 @@ over graceful degradation. - `src/diff_calc/`: diff calculation when moving between versions. The tree calculator's cache transitions: [../../context/tree-checkout-window.md](../../context/tree-checkout-window.md). + Which tree diff modes carry raw ops, and how tree events are built for each: + [../../context/tree-events.md](../../context/tree-events.md). - `src/container/richtext/`: text state with style anchors. Where local inserts go next to anchors, the insert cursor cache, and delete `start_id`s: [../../context/richtext-insert-positions.md](../../context/richtext-insert-positions.md). diff --git a/crates/loro-internal/src/state/AGENTS.md b/crates/loro-internal/src/state/AGENTS.md index 7572efb81..c2106865d 100644 --- a/crates/loro-internal/src/state/AGENTS.md +++ b/crates/loro-internal/src/state/AGENTS.md @@ -21,6 +21,8 @@ before changing mergeable child behavior. shallow-snapshot export; read [../../../../context/shallow-snapshot-style-redaction.md](../../../../context/shallow-snapshot-style-redaction.md) before changing the richtext snapshot codec or that pass. + `tree_state.rs` builds tree events two ways depending on the diff mode: + [../../../../context/tree-events.md](../../../../context/tree-events.md). `movable_list_state.rs` import validation and the Checkout-mode fallback for moves of deleted elements, plus the List/Text `validate_diff` bounds checks: [../../../../context/movable-list-op-validation.md](../../../../context/movable-list-op-validation.md). diff --git a/crates/loro-internal/src/state/tree_state.rs b/crates/loro-internal/src/state/tree_state.rs index 0402492e5..918fb8d1f 100644 --- a/crates/loro-internal/src/state/tree_state.rs +++ b/crates/loro-internal/src/state/tree_state.rs @@ -1041,6 +1041,128 @@ pub(crate) enum FractionalIndexGenResult { NotConfigured, } +impl TreeState { + /// Applies a raw `Create` or `Move` op (`Linear` and `ImportGreaterUpdates` + /// modes) and pushes the events that turn the alive tree before it into + /// the alive tree after it: + /// + /// - alive before and after: `Move`, unless nothing visible changed (e.g. a + /// `Create` op for a child that the revival of its parent already + /// brought back where it is); + /// - alive before only: `Delete`, which covers the subtree; + /// - alive after only: `Create` for the node and for every node below it, + /// because a node that leaves a deleted subtree brings its subtree back; + /// - neither: nothing. + /// + /// With `with_check`, an op that would create a cycle is skipped, as in + /// `apply_diff`. + fn apply_raw_move_and_convert( + &mut self, + target: TreeID, + parent: TreeParentId, + last_move_op: IdFull, + position: &FractionalIndex, + with_check: bool, + ans: &mut Vec, + ) { + let old = self + .trees + .get(&target) + .map(|node| (node.parent, node.position.as_ref() == Some(position))); + let old_index = match &old { + Some(_) if !self.is_node_deleted(&target).unwrap() => { + Some(self.get_index_by_tree_id(&target).unwrap()) + } + _ => None, + }; + let result = self.mov( + target, + parent, + last_move_op, + Some(position.clone()), + with_check, + ); + if with_check { + if result.is_err() { + return; + } + } else { + result.unwrap(); + } + + let alive = !self.is_node_deleted(&target).unwrap(); + match (old, old_index) { + (Some((old_parent, same_position)), Some(old_index)) => { + if !alive { + ans.push(TreeDiffItem { + target, + action: TreeExternalDiff::Delete { + old_parent, + old_index, + }, + }); + return; + } + let index = self.get_index_by_tree_id(&target).unwrap(); + if old_parent == parent && old_index == index && same_position { + return; + } + ans.push(TreeDiffItem { + target, + action: TreeExternalDiff::Move { + parent, + index, + position: position.clone(), + old_parent, + old_index, + }, + }); + } + (old, _) => { + if !alive { + return; + } + ans.push(TreeDiffItem { + target, + action: TreeExternalDiff::Create { + parent, + index: self.get_index_by_tree_id(&target).unwrap(), + position: position.clone(), + }, + }); + // A node that did not exist has no children yet. + if old.is_some() { + self.push_subtree_creation(target, ans); + } + } + } + } + + /// Pushes a `Create` for every node below `root`: parents before their + /// children and siblings in order, so each `index` is valid when the + /// events are applied in sequence. + fn push_subtree_creation(&self, root: TreeID, ans: &mut Vec) { + let mut stack = vec![root]; + while let Some(node) = stack.pop() { + let parent = TreeParentId::Node(node); + let Some(children) = self.children.get(&parent) else { + continue; + }; + for (index, (position, &child)) in children.iter().enumerate() { + ans.push(TreeDiffItem { + target: child, + action: TreeExternalDiff::Create { + parent, + index, + position: position.position.clone(), + }, + }); + stack.push(child); + } + } + } +} + impl ContainerState for TreeState { fn container_idx(&self) -> crate::container::idx::ContainerIdx { self.idx @@ -1058,6 +1180,12 @@ impl ContainerState for TreeState { ctx: DiffApplyContext, ) -> Diff { let need_check = !matches!(ctx.mode, DiffMode::Checkout | DiffMode::Linear); + // In `Checkout` mode the items come from `TreeDiffCalculator`, which + // has already classified them against its cache and follows every + // `Create` with the node's subtree. In `Linear` and + // `ImportGreaterUpdates` modes they are the raw ops, so the events are + // derived from the state. See `context/tree-events.md`. + let raw_ops = !matches!(ctx.mode, DiffMode::Checkout); let mut ans = vec![]; if let InternalDiff::Tree(tree) = &diff { // assert never cause cycle move @@ -1066,6 +1194,19 @@ impl ContainerState for TreeState { let target = diff.target; // create associated metadata container match &diff.action { + TreeInternalDiff::Create { parent, position } + | TreeInternalDiff::Move { parent, position } + if raw_ops => + { + self.apply_raw_move_and_convert( + target, + *parent, + last_move_op, + position, + need_check, + &mut ans, + ); + } TreeInternalDiff::Create { parent, position } => { self.mov(target, *parent, last_move_op, Some(position.clone()), false) .unwrap(); @@ -1092,84 +1233,41 @@ impl ContainerState for TreeState { // If this is some, the node is still alive at the moment let old_index = self.get_index_by_tree_id(&target); let was_alive = !self.is_node_deleted(&target).unwrap(); - if need_check { - if self - .mov(target, *parent, last_move_op, Some(position.clone()), true) - .is_ok() - { - if self.is_node_deleted(&target).unwrap() { - if was_alive { - // delete event - ans.push(TreeDiffItem { - target, - action: TreeExternalDiff::Delete { - old_parent, - old_index: old_index.unwrap(), - }, - }); - } - // Otherwise, it's a normal move inside deleted nodes, no event is needed - } else if was_alive { - // normal move - ans.push(TreeDiffItem { - target, - action: TreeExternalDiff::Move { - parent: *parent, - index: self.get_index_by_tree_id(&target).unwrap(), - position: position.clone(), - old_parent, - old_index: old_index.unwrap(), - }, - }); - } else { - // create event - ans.push(TreeDiffItem { - target, - action: TreeExternalDiff::Create { - parent: *parent, - index: self.get_index_by_tree_id(&target).unwrap(), - position: position.clone(), - }, - }); - } - } - } else { - self.mov(target, *parent, last_move_op, Some(position.clone()), false) - .unwrap(); - - if let TreeParentId::Node(p) = parent { - // reuse diff cache, at this time,it's "move in deleted" - if self.is_node_deleted(p).unwrap() { - continue; - } + self.mov(target, *parent, last_move_op, Some(position.clone()), false) + .unwrap(); + + if let TreeParentId::Node(p) = parent { + // reuse diff cache, at this time,it's "move in deleted" + if self.is_node_deleted(p).unwrap() { + continue; } + } - let index = self.get_index_by_tree_id(&target).unwrap(); - match was_alive { - true => { - ans.push(TreeDiffItem { - target, - action: TreeExternalDiff::Move { - parent: *parent, - index, - position: position.clone(), - old_parent, - old_index: old_index.unwrap(), - }, - }); - } - false => { - ans.push(TreeDiffItem { - target, - action: TreeExternalDiff::Create { - parent: *parent, - index, - position: position.clone(), - }, - }); - } + let index = self.get_index_by_tree_id(&target).unwrap(); + match was_alive { + true => { + ans.push(TreeDiffItem { + target, + action: TreeExternalDiff::Move { + parent: *parent, + index, + position: position.clone(), + old_parent, + old_index: old_index.unwrap(), + }, + }); + } + false => { + ans.push(TreeDiffItem { + target, + action: TreeExternalDiff::Create { + parent: *parent, + index, + position: position.clone(), + }, + }); } - }; + } } TreeInternalDiff::Delete { parent, position } => { let mut send_event = true; diff --git a/crates/loro-wasm/tests/tree_revival_events.test.ts b/crates/loro-wasm/tests/tree_revival_events.test.ts new file mode 100644 index 000000000..6d79af498 --- /dev/null +++ b/crates/loro-wasm/tests/tree_revival_events.test.ts @@ -0,0 +1,142 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc, LoroTree, TreeDiffItem, TreeID } from "../bundler/index"; + +// Tree events must turn the tree before an import into the tree after it, +// including the nodes below a node revived from a deleted subtree +// (loro-dev/loro#1157). See context/tree-events.md. + +const ROOT = "root"; + +/** Children lists of the alive tree, rebuilt only from events. */ +class Mirror { + children = new Map(); + + static fromTree(tree: LoroTree): Mirror { + const m = new Mirror(); + const walk = (key: string, ids: TreeID[]) => { + if (ids.length > 0) m.children.set(key, ids); + for (const id of ids) { + walk( + id, + tree + .getNodeByID(id)! + .children() + ?.map((n) => n.id) ?? [], + ); + } + }; + walk( + ROOT, + tree.roots().map((n) => n.id), + ); + return m; + } + + alive(id: TreeID): boolean { + for (const ids of this.children.values()) { + if (ids.includes(id)) return true; + } + return false; + } + + remove(target: TreeID, parent: TreeID | undefined, index: number) { + const key = parent ?? ROOT; + const ids = this.children.get(key); + expect(ids?.[index], `${target}: old index`).toBe(target); + ids!.splice(index, 1); + if (ids!.length === 0) this.children.delete(key); + } + + insert(target: TreeID, parent: TreeID | undefined, index: number) { + if (parent !== undefined) { + expect(this.alive(parent), `${target}: parent ${parent} alive`).toBe( + true, + ); + } + const key = parent ?? ROOT; + const ids = this.children.get(key) ?? []; + expect(index).toBeLessThanOrEqual(ids.length); + ids.splice(index, 0, target); + this.children.set(key, ids); + } + + apply(item: TreeDiffItem) { + if (item.action === "create") { + expect(this.alive(item.target), `${item.target}: created twice`).toBe( + false, + ); + this.insert(item.target, item.parent, item.index); + } else if (item.action === "move") { + this.remove(item.target, item.oldParent, item.oldIndex); + this.insert(item.target, item.parent, item.index); + } else { + this.remove(item.target, item.oldParent, item.oldIndex); + // The whole subtree goes with it. + const stack: string[] = [item.target]; + while (stack.length > 0) { + const key = stack.pop()!; + const ids = this.children.get(key); + if (ids) { + this.children.delete(key); + stack.push(...ids); + } + } + } + } +} + +/** Imports `updates` into a copy of `base` and checks the events. */ +function checkImportEvents(base: LoroDoc, updates: Uint8Array) { + const doc = new LoroDoc(); + doc.import(base.export({ mode: "update" })); + const tree = doc.getTree("tree"); + const mirror = Mirror.fromTree(tree); + const items: TreeDiffItem[] = []; + doc.subscribe((e) => { + for (const ev of e.events) { + if (ev.diff.type === "tree") items.push(...ev.diff.diff); + } + }); + doc.import(updates); + for (const item of items) mirror.apply(item); + expect(mirror.children).toEqual(Mirror.fromTree(tree).children); +} + +function docWithSubtree() { + const doc = new LoroDoc(); + doc.setPeerId(1n); + const tree = doc.getTree("tree"); + const p = tree.createNode(); + const x = p.createNode(); + const y1 = x.createNode(); + x.createNode(); + y1.createNode(); + doc.commit(); + return { doc, tree, p, x, y1 }; +} + +describe("tree events on import", () => { + it("create every node below a node moved out of a deleted subtree", () => { + const { doc, tree, p, x } = docWithSubtree(); + const base = doc.fork(); + const v0 = doc.oplogVersion(); + tree.delete(p.id); + doc.commit(); + tree.move(x.id, undefined); + doc.commit(); + checkImportEvents(base, doc.export({ mode: "update", from: v0 })); + }); + + it("delete a node moved under a deleted one", () => { + const { doc, tree, p, y1 } = docWithSubtree(); + const q = tree.createNode(); + q.createNode(); + doc.commit(); + const base = doc.fork(); + const v0 = doc.oplogVersion(); + tree.delete(p.id); + tree.move(q.id, y1.id); + doc.commit(); + checkImportEvents(base, doc.export({ mode: "update", from: v0 })); + }); +}); diff --git a/crates/loro/tests/tree_revival_events.rs b/crates/loro/tests/tree_revival_events.rs new file mode 100644 index 000000000..d3db42433 --- /dev/null +++ b/crates/loro/tests/tree_revival_events.rs @@ -0,0 +1,463 @@ +//! Tree events must turn the tree before an import into the tree after it: +//! every node that becomes alive gets a `Create`, including the descendants of +//! a node revived from a deleted subtree (loro-dev/loro#1157). + +use base64::Engine; +use loro::{ + event::Diff, ContainerID, ContainerTrait, ExportMode, LoroDoc, LoroTree, LoroValue, + TreeExternalDiff, TreeID, TreeParentId, ValueOrContainer, +}; +use rand::{rngs::StdRng, seq::SliceRandom, Rng, SeedableRng}; +use rustc_hash::FxHashMap; +use std::sync::{Arc, Mutex}; + +/// The alive part of a tree (children lists and scalar metadata), rebuilt +/// only from events. +#[derive(Default, Debug, Clone, PartialEq)] +struct Mirror { + children: FxHashMap>, + meta: FxHashMap>, +} + +impl Mirror { + fn from_tree(tree: &LoroTree) -> Self { + let mut m = Mirror::default(); + let mut stack = vec![TreeParentId::Root]; + while let Some(p) = stack.pop() { + let children = tree.children(p).unwrap_or_default(); + for c in children.iter() { + stack.push(TreeParentId::Node(*c)); + let meta = tree.get_meta(*c).unwrap().get_value(); + let meta = meta.into_map().unwrap(); + m.meta.insert( + *c, + meta.iter().map(|(k, v)| (k.clone(), v.clone())).collect(), + ); + } + if !children.is_empty() { + m.children.insert(p, children); + } + } + m + } + + fn apply_meta(&mut self, container: &ContainerID, updated: &[(String, Option)]) { + let Some((_, meta)) = self + .meta + .iter_mut() + .find(|(id, _)| &id.associated_meta_container() == container) + else { + // Not an alive node's metadata. + return; + }; + for (k, v) in updated { + match v { + Some(v) => meta.insert(k.clone(), v.clone()), + None => meta.remove(k), + }; + } + } + + fn contains(&self, id: &TreeID) -> bool { + self.children.values().any(|c| c.contains(id)) + } + + fn remove(&mut self, target: TreeID, parent: TreeParentId, index: usize) { + let list = self + .children + .get_mut(&parent) + .unwrap_or_else(|| panic!("{target}: parent {parent:?} has no children in the mirror")); + assert_eq!(list.get(index), Some(&target), "{target}: wrong old_index"); + list.remove(index); + if list.is_empty() { + self.children.remove(&parent); + } + } + + fn insert(&mut self, target: TreeID, parent: TreeParentId, index: usize) { + if let TreeParentId::Node(p) = parent { + assert!( + self.contains(&p), + "{target}: parent {p} is not alive in the mirror" + ); + } + let list = self.children.entry(parent).or_default(); + assert!(index <= list.len(), "{target}: index {index} out of bounds"); + list.insert(index, target); + } + + fn apply(&mut self, target: TreeID, action: &TreeExternalDiff) { + match action { + TreeExternalDiff::Create { parent, index, .. } => { + assert!(!self.contains(&target), "{target}: created twice"); + self.insert(target, *parent, *index); + // The metadata arrives as map events. + self.meta.insert(target, FxHashMap::default()); + } + TreeExternalDiff::Move { + parent, + index, + old_parent, + old_index, + .. + } => { + self.remove(target, *old_parent, *old_index); + self.insert(target, *parent, *index); + } + TreeExternalDiff::Delete { + old_parent, + old_index, + } => { + self.remove(target, *old_parent, *old_index); + // The whole subtree goes with it. + let mut stack = vec![target]; + while let Some(n) = stack.pop() { + self.meta.remove(&n); + if let Some(c) = self.children.remove(&TreeParentId::Node(n)) { + stack.extend(c); + } + } + } + } + } +} + +#[derive(Debug)] +enum Recorded { + Tree(TreeID, TreeExternalDiff), + Map(ContainerID, Vec<(String, Option)>), +} + +/// Replays the tree events of a document into a [`Mirror`]. +/// +/// The subscriber only records the events; they are applied on the test +/// thread, so a bad event fails the test instead of panicking under the +/// document's lock. +struct EventMirror { + mirror: Mirror, + events: Arc>>, + _sub: loro::Subscription, +} + +/// Subscribes to `doc`; the mirror starts from the current state of `tree`. +fn mirror_events(doc: &LoroDoc, tree: &LoroTree) -> EventMirror { + let events = Arc::new(Mutex::new(Vec::new())); + let recorded = events.clone(); + let tid = tree.id(); + let sub = doc.subscribe_root(Arc::new(move |e| { + let mut recorded = recorded.lock().unwrap(); + for c in e.events.iter() { + match &c.diff { + Diff::Tree(t) if c.target == &tid => { + recorded.extend( + t.diff + .iter() + .map(|i| Recorded::Tree(i.target, i.action.clone())), + ); + } + Diff::Map(m) => { + let updated = m + .updated + .iter() + .map(|(k, v)| { + let v = v.as_ref().map(|v| match v { + ValueOrContainer::Value(v) => v.clone(), + ValueOrContainer::Container(c) => LoroValue::Container(c.id()), + }); + (k.to_string(), v) + }) + .collect(); + recorded.push(Recorded::Map(c.target.clone(), updated)); + } + _ => {} + } + } + })); + EventMirror { + mirror: Mirror::from_tree(tree), + events, + _sub: sub, + } +} + +fn assert_mirror_matches(m: &mut EventMirror, tree: &LoroTree) { + let events = std::mem::take(&mut *m.events.lock().unwrap()); + for e in events.iter() { + match e { + Recorded::Tree(target, action) => m.mirror.apply(*target, action), + Recorded::Map(container, updated) => m.mirror.apply_meta(container, updated), + } + } + let expected = Mirror::from_tree(tree); + for (p, children) in expected.children.iter() { + assert_eq!( + m.mirror.children.get(p), + Some(children), + "children of {p:?} differ between the events and the state; events: {events:?}" + ); + } + assert_eq!(m.mirror, expected, "events: {events:?}"); +} + +const BASE: &str = "bG9ybwAAAAAAAAAAAAAAAACHcG0ABEEAAQABARABAgAAAAAAAAABAQAAAAAABQEAAAEABgEEAQAAAggBawVidXJzdAAOAQQCAQACAQACAQsCAQEAAwPRB0wAAQEBARsC6QMAAAAAAAACAAAAAAAAAAEBAQEBAQAAAAAFAQAAAQAGAQQBAAACCAFrBWJ1cnN0AA4BBAIBAAIBAAIBCwIBAQADA9oG"; +const REST: &str = "bG9ybwAAAAAAAAAAAAAAACh/hBkABL8BAAYADAI6BQEAAAAAAAAA6QMAAAAAAAD//////////wMAAAAAAAAA6gMAAAAAAAABAQEDAAIDAwQBAgGgAAEAAAYBAAEABAAVBAQBBAAEBAAAAQQEAQMABgQBAAAIFgJrMQFrBW1saXN0BHRyZWUFYnVyc3QMAQICBAAGAgGAAoGAHAEEBwEABgIDAQAGBgAFAgEABwQLAxALBBACDAEAHwcBAwIDKAECAAAC/////wcDmgQABAAAAQIABQEAAQJRAAIAAgEQAQMAAAAAAAAAAQEAAAAAAAUBAAABAAsCBAEAAAIEAQEABA0BawVidXJzdARsaXN0AA8BBAMDAAICBAACBAsCBAEACAOzAwcBA4MGWwECAgIBEQHpAwAAAAAAAAABAQAAAAAABQEAAAEACwIEAQAAAgQBAwAEDQFrBWJ1cnN0BHRyZWUJAQICAQADAQGAEAEEAwMAAgIEAAMDCxACBAEABwOXBQACAAFoAAMEAwEbAuoDAAAAAAAA6QMAAAAAAAABAQEBAQEEAAAABQEAAAEAEAMEAQQABAQAAAAABAEAAAYQAXYBawVtbGlzdAVidXJzdAASAQQEAQAEAgQEAAECAgYLAgYBAAkHAQkAAyMDvQJTAAECAQEbAusDAAAAAAAAAwAAAAAAAAABAQEBAQECAAAABQEAAAEABgEEAQMAAAUEdHJlZQkBAgIBAAMBAYAOAQQCAQACAQACARACAQEABAAAAAFMAAEDAQEbAuwDAAAAAAAA6wMAAAAAAAABAQEBAQEAAAAABQEAAAEABgEEAQAAAggBawVidXJzdAAOAQQCAQACAQACAQsCAQEAAwOLAmMAAgQCASMD7QMAAAAAAADrAwAAAAAAAOwDAAAAAAAAAQEBAQIBAAAAAAUBAAABAAsCBAAAAQAEAAMAAAQDc3ViCQECAgEAAwEBgBABBAMDAAICBAADAwsQAgQBAAYJAwABAAF5AAMEAwEjA+8DAAAAAAAA6QMAAAAAAAD//////////wEBAQEBAQQAAAAFAQAAAQAQAwQAAAEEBAEAAAQEAQMABhACazIBawVidXJzdAR0cmVlABQBBAQBAAQCBAUAAgEEBAsBEAIGAQAQA7wEA9ABAQIAAAL/////B5sBAAUHBQE3BfADAAAAAAAA6QMAAAAAAADvAwAAAAAAAOoDAAAAAAAAAwAAAAAAAAABAQMFAgMEAQQCT4AAAAUBAAABAAsCBAEDAAIEAQAABA0BawR0cmVlBWJ1cnN0DAECAgQABgIBgAKBgBUBBAYFAAIBBAACCgAFAxALBhACCgEAGQAAAAABAgPPAwACAQABAgADAAAAAAAAAAFhAAMDAwEbAvEDAAAAAAAA6wMAAAAAAAABAQEBAQEAAAAABQEAAAEACwIEAQAAAgQBBAAEDgFrBWJ1cnN0BW1saXN0ABABBAQFAAIBAgYAAgYLAgYBAAsDqAQHAQPwBAPRAbkBAAcHBwEwBPIDAAAAAAAA8QMAAAAAAADtAwAAAAAAAOoDAAAAAAAAAQEDBQECAwEEAp9QQAAABQEAAAEAGgUEAQMABAQBAAAGBAEFAAgEAQQACgQAAAAKHQFrAXYEdHJlZQVidXJzdAdjb3VudGVyBW1saXN0DQECAgQABwICgYACgoAdAQQJAQAEAgcDAgQCBQoAAwIABwcQCwMQBgsCDgEAFgAAAAEDhAHeBwADAQEDEgcBCQAD4gdaAAMFAwEjA/MDAAAAAAAA6QMAAAAAAADqAwAAAAAAAAEBAQECAQAAAAAFAQAAAQALAgQAAAEEBAACAAAEA3R4dAARAQQDAwACAgQAAwMLBQMDAQIABQkCAmFihwEAAw4DAS4E9AMAAAAAAADyAwAAAAAAAP//////////8wMAAAAAAAABAQIDAQMBDAGdgAAABQEAAAEACwIEAQMAAAQBAQACCgR0cmVlBGxpc3QJAQICAQADAQGAFAEEBAUAAgEEBQACAQQFEAsQAgYBABUAAAAAAQMHAQP1AgEAAAAC/////wdcAAICAgEbAvUDAAAAAAAAAwAAAAAAAAABAQEBAQECAAAABQEAAAEACwIEAQUAAAQBAQACDQdjb3VudGVyBGxpc3QAEAEEAwMAAgIEAAMDAwsCBAEAB+sFBwEDrQZjAAIHAgEjA/cDAAAAAAAA6QMAAAAAAADqAwAAAAAAAAEBAQECAQQAAAAFAQAAAQALAgQAAAEEBAADAAAEA3N1YgkBAgIBAAMBAYAQAQQDAwACAgQAAwMLEAIEAQAGCQMAAQABswEABQwFAT8G+AMAAAAAAADqAwAAAAAAAOkDAAAAAAAA9wMAAAAAAAD1AwAAAAAAAAEAAAAAAAAAAQEDBQMEBQECAlDAAAAFAQAAAQAQAwQBAAACBAEEAAQEAQMABhMBawVidXJzdAVtbGlzdAR0cmVlCgECAgEABAECgoAbAQQGCQACAQAEBgUAAgEEAAcDCwkECwEQAgoBCwEDAgECAgEAAgECDwOdBgPKBQP6BgAEAAACAmIAAgMCARsC+QMAAAAAAADrAwAAAAAAAAEBAQEBAQAAAAAFAQAAAQALAgQAAAEABAEDAAIIAmsyBHRyZWUJAQICAQADAQGAEAEEAwMAAgIEAAMDCxACBAEACQPVAgABAAABAJIBAAcRBwE2BfoDAAAAAAAA7QMAAAAAAAD///////////kDAAAAAAAA9AMAAAAAAAABAQIDAwQBAgGgAAAABQEAAAEAGgUEAAABAgQAAgAABAAAAwIEAAIABgQAAwEABAN0eHQAGAEEBAEACAICCgAGCQsFCwUQBgkBAgECAQAUCQICYWIJAgJhYgEBAAAC/////wc="; + +/// The repro from loro-dev/loro#1157: `0@1008` is created under a parent that +/// another peer already deleted, gets a child `3@1008`, and is then moved to +/// the root. Importing the rest of the history emitted `Create` for `0@1008` +/// but not for `3@1008`. +#[test] +fn imported_revival_emits_create_for_every_child() { + let b64 = base64::engine::general_purpose::STANDARD; + let base = b64.decode(BASE).unwrap(); + let rest = b64.decode(REST).unwrap(); + + let doc = LoroDoc::new(); + doc.import(&base).unwrap(); + let tree = doc.get_tree("tree"); + let mut mirror = mirror_events(&doc, &tree); + doc.import(&rest).unwrap(); + assert_mirror_matches(&mut mirror, &tree); + + // A fresh document importing everything at once. + let fresh = LoroDoc::new(); + let tree = fresh.get_tree("tree"); + let mut mirror = mirror_events(&fresh, &tree); + fresh.import(&base).unwrap(); + fresh.import(&rest).unwrap(); + assert_mirror_matches(&mut mirror, &tree); + + // Checking out every version of the history keeps the mirror in sync. + let all = fresh.export(ExportMode::all_updates()).unwrap(); + let doc = LoroDoc::new(); + doc.import(&all).unwrap(); + let tree = doc.get_tree("tree"); + let mut mirror = mirror_events(&doc, &tree); + let vv = doc.oplog_vv(); + let mut versions = vec![loro::Frontiers::default()]; + for (peer, end) in vv.iter() { + for counter in 0..*end { + versions.push(loro::Frontiers::from_id(loro::ID::new(*peer, counter))); + } + } + for f in versions.iter() { + doc.checkout(f).unwrap(); + assert_mirror_matches(&mut mirror, &tree); + } + doc.checkout_to_latest(); + assert_mirror_matches(&mut mirror, &tree); +} + +fn doc_with_peer(peer: u64) -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(peer).unwrap(); + doc +} + +/// `p > x > (y1 > z, y2)`, committed by peer 1. +fn doc_with_subtree() -> (LoroDoc, [TreeID; 5]) { + let doc = doc_with_peer(1); + let tree = doc.get_tree("tree"); + let p = tree.create(TreeParentId::Root).unwrap(); + let x = tree.create(p).unwrap(); + let y1 = tree.create(x).unwrap(); + let y2 = tree.create(x).unwrap(); + let z = tree.create(y1).unwrap(); + doc.commit(); + (doc, [p, x, y1, y2, z]) +} + +/// Imports `updates` into a copy of `base` one by one and checks that the +/// events bring a mirror of the tree to the imported state. +fn check_import_events(base: &LoroDoc, updates: &[Vec]) { + let doc = LoroDoc::new(); + doc.import(&base.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + let tree = doc.get_tree("tree"); + let mut mirror = mirror_events(&doc, &tree); + for u in updates { + doc.import(u).unwrap(); + assert_mirror_matches(&mut mirror, &tree); + } +} + +/// A linear update (`DiffMode::Linear`) that moves `x` out of the deleted `p` +/// revives `x` and its whole subtree. +#[test] +fn linear_import_of_move_out_of_deleted_subtree_creates_descendants() { + let (doc, [p, x, ..]) = doc_with_subtree(); + let base = doc.fork(); + let tree = doc.get_tree("tree"); + let v0 = doc.oplog_vv(); + tree.delete(p).unwrap(); + doc.commit(); + let v1 = doc.oplog_vv(); + tree.mov(x, TreeParentId::Root).unwrap(); + doc.commit(); + // Both in one import, and one change at a time. + check_import_events(&base, &[doc.export(ExportMode::updates(&v0)).unwrap()]); + check_import_events( + &base, + &[ + doc.export(ExportMode::updates_till(&v1)).unwrap(), + doc.export(ExportMode::updates(&v1)).unwrap(), + ], + ); +} + +/// Moving an alive node under a deleted one deletes it; a linear import used to +/// emit nothing for it. +#[test] +fn linear_import_of_move_into_deleted_subtree_deletes_node() { + let (doc, [p, _, y1, ..]) = doc_with_subtree(); + let tree = doc.get_tree("tree"); + let q = tree.create(TreeParentId::Root).unwrap(); + let r = tree.create(q).unwrap(); + doc.commit(); + let base = doc.fork(); + let v0 = doc.oplog_vv(); + tree.delete(p).unwrap(); + tree.mov(q, y1).unwrap(); + // Moves inside the deleted subtree emit nothing, then `r` comes back. + tree.mov(q, p).unwrap(); + tree.mov(r, TreeParentId::Root).unwrap(); + doc.commit(); + check_import_events(&base, &[doc.export(ExportMode::updates(&v0)).unwrap()]); +} + +/// `DiffMode::ImportGreaterUpdates`: one peer deletes `p` while another moves +/// `x` out of it with a greater lamport. Applied in lamport order, the move +/// revives `x` and its subtree. +#[test] +fn concurrent_delete_and_move_out_creates_descendants() { + let (doc, [p, x, ..]) = doc_with_subtree(); + let v0 = doc.oplog_vv(); + + let a = doc.fork(); + a.set_peer_id(2).unwrap(); + a.get_tree("tree").delete(p).unwrap(); + a.commit(); + + let b = doc.fork(); + b.set_peer_id(3).unwrap(); + // Raise the lamport of the move above the delete's. + b.get_map("m").insert("k", 1).unwrap(); + b.get_tree("tree").mov(x, TreeParentId::Root).unwrap(); + b.commit(); + + let merged = doc.fork(); + merged + .import(&a.export(ExportMode::updates(&v0)).unwrap()) + .unwrap(); + merged + .import(&b.export(ExportMode::updates(&v0)).unwrap()) + .unwrap(); + let tree = merged.get_tree("tree"); + assert!(!tree.is_node_deleted(&x).unwrap()); + check_import_events(&doc, &[merged.export(ExportMode::updates(&v0)).unwrap()]); +} + +/// Undoing a delete re-creates the node and then each of its descendants, one +/// op each. Importing that must create every node exactly once. +#[test] +fn import_of_undone_delete_creates_each_node_once() { + let (doc, [p, ..]) = doc_with_subtree(); + let tree = doc.get_tree("tree"); + tree.get_meta(p).unwrap().insert("k", 1).unwrap(); + doc.commit(); + let mut undo = loro::UndoManager::new(&doc); + let base = doc.fork(); + let v0 = doc.oplog_vv(); + tree.delete(p).unwrap(); + doc.commit(); + let v1 = doc.oplog_vv(); + assert!(undo.undo().unwrap()); + doc.commit(); + // The undo brings the subtree back, possibly under new ids. + assert_eq!(tree.get_nodes(false).len(), 5); + check_import_events(&base, &[doc.export(ExportMode::updates(&v0)).unwrap()]); + check_import_events( + &base, + &[ + doc.export(ExportMode::updates_till(&v1)).unwrap(), + doc.export(ExportMode::updates(&v1)).unwrap(), + ], + ); +} + +fn random_parent(nodes: &[TreeID], rng: &mut StdRng) -> TreeParentId { + match nodes.choose(rng) { + Some(n) if rng.gen_bool(0.7) => TreeParentId::Node(*n), + _ => TreeParentId::Root, + } +} + +/// Three peers make random tree edits (including creates under, moves into +/// and moves out of deleted subtrees, and metadata edits) and sync at random +/// with two observers that only import. The observers' imports have no local +/// concurrency, so they take the `Linear` and `ImportGreaterUpdates` modes. +/// After every import the events must rebuild the document's tree. +fn random_sync(seed: u64) { + const EDITORS: usize = 3; + const DOCS: usize = 5; + let mut rng = StdRng::seed_from_u64(seed); + let docs: Vec = (1..=DOCS as u64).map(doc_with_peer).collect(); + let mut mirrors: Vec = docs + .iter() + .map(|d| mirror_events(d, &d.get_tree("tree"))) + .collect(); + for _ in 0..150 { + if rng.gen_bool(0.4) { + let i = rng.gen_range(0..DOCS); + let j = rng.gen_range(0..DOCS); + if i == j { + continue; + } + let update = docs[j] + .export(ExportMode::updates(&docs[i].oplog_vv())) + .unwrap(); + docs[i].import(&update).unwrap(); + assert_mirror_matches(&mut mirrors[i], &docs[i].get_tree("tree")); + } else { + let i = rng.gen_range(0..EDITORS); + let tree = docs[i].get_tree("tree"); + // Includes deleted nodes. + let nodes = tree.nodes(); + let target = nodes.choose(&mut rng).copied(); + // Errors are expected, e.g. moving a node below itself. + let _ = match (rng.gen_range(0..10), target) { + (0..=2, _) | (_, None) => tree.create(random_parent(&nodes, &mut rng)).map(drop), + (3..=5, Some(t)) => tree.mov(t, random_parent(&nodes, &mut rng)), + (6, Some(t)) => tree.delete(t), + (_, Some(t)) => tree.get_meta(t).and_then(|m| { + m.insert( + &format!("k{}", rng.gen_range(0..3)), + rng.gen_range(0..100i64), + ) + }), + }; + docs[i].commit(); + // Local events on deleted subtrees still use the shapes of live + // ones (a create under a deleted parent emits `Create`, a move out + // of one emits `Move`), so start the mirror over from the state. + mirrors[i].events.lock().unwrap().clear(); + mirrors[i].mirror = Mirror::from_tree(&tree); + } + } +} + +#[test] +fn random_sync_events_rebuild_the_tree() { + let seeds: Vec = match std::env::var("TREE_EVENT_SEEDS") { + Ok(range) => { + let (start, end) = range.split_once("..").expect("TREE_EVENT_SEEDS=start..end"); + (start.parse().unwrap()..end.parse().unwrap()).collect() + } + Err(_) => (0..100).collect(), + }; + let failed: Vec = seeds + .into_iter() + .filter(|&seed| std::panic::catch_unwind(|| random_sync(seed)).is_err()) + .collect(); + assert!(failed.is_empty(), "failed seeds: {failed:?}"); +} From ccdcee356245e72a0423d3b97af2b13fbb9e7083 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 14:04:34 +0800 Subject: [PATCH 09/10] fix: make apply_diff/revert_to all-or-nothing (#1154) A batch that fails is rolled back: its ops are taken out of the transaction (uncommitted edits before it stay), undone from the state by a checkout through a temporary change under an import rollback scope, and removed from the DAG. No event is emitted. A stale tree diff naming a missing parent now returns an error instead of panicking. Rationale, reused op ids and the multi-thread fallback: context/apply-diff-atomicity.md. Co-Authored-By: Claude Opus 5.5 --- .changeset/apply-diff-atomic.md | 5 + AGENTS.md | 2 + context/apply-diff-atomicity.md | 119 +++ context/unknown-containers.md | 15 +- crates/loro-internal/AGENTS.md | 2 + crates/loro-internal/src/arena.rs | 24 + crates/loro-internal/src/handler/tree.rs | 9 +- crates/loro-internal/src/loro.rs | 192 +++- crates/loro-internal/src/oplog.rs | 17 + .../loro-internal/src/oplog/change_store.rs | 25 +- crates/loro-internal/src/oplog/loro_dag.rs | 16 + crates/loro-internal/src/state.rs | 109 +- .../src/state/container_store.rs | 12 + .../src/state/container_store/inner_store.rs | 48 +- .../loro-internal/src/state/counter_state.rs | 11 + crates/loro-internal/src/txn.rs | 137 +++ .../tests/apply_diff_atomicity.test.ts | 106 ++ crates/loro/tests/apply_diff_atomicity.rs | 980 ++++++++++++++++++ 18 files changed, 1806 insertions(+), 23 deletions(-) create mode 100644 .changeset/apply-diff-atomic.md create mode 100644 context/apply-diff-atomicity.md create mode 100644 crates/loro-wasm/tests/apply_diff_atomicity.test.ts create mode 100644 crates/loro/tests/apply_diff_atomicity.rs diff --git a/.changeset/apply-diff-atomic.md b/.changeset/apply-diff-atomic.md new file mode 100644 index 000000000..ba2eae263 --- /dev/null +++ b/.changeset/apply-diff-atomic.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +`applyDiff` and `revertTo` are now all or nothing: when they throw (for example on an out-of-range position, a diff made for another version of the document, or a diff type that doesn't match its container), the document, its history and its subscribers are left as they were. Uncommitted edits made before the call are kept, still uncommitted. Applying a diff that names a tree parent the document doesn't have now throws instead of trapping. diff --git a/AGENTS.md b/AGENTS.md index ba0179465..11af923fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,8 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. - Imports that reuse local op ids (a shared peer id): the known-history check, what counts as equal, and what it cannot catch: [context/import-peer-id-reuse.md](context/import-peer-id-reuse.md). +- `apply_diff`/`revert_to` rollback of a failed batch, reused op ids, other + threads: [context/apply-diff-atomicity.md](context/apply-diff-atomicity.md). - Movable-list `Move`/`Set` import validation, and why a move of a deleted element is applied rather than rejected: [context/movable-list-op-validation.md](context/movable-list-op-validation.md). diff --git a/context/apply-diff-atomicity.md b/context/apply-diff-atomicity.md new file mode 100644 index 000000000..bfec1d78a --- /dev/null +++ b/context/apply-diff-atomicity.md @@ -0,0 +1,119 @@ +# `apply_diff` / `revert_to` Atomicity + +Verified against code 2026-09-30 (loro-dev/loro#1154). + +`LoroDoc::apply_diff` and `revert_to` (and loro-crdt `applyDiff` / `revertTo`) +are all or nothing. When they return `Err`, the doc's state, history, version +and subscribers are as before the call. Undo/redo (`undo_internal_with`) still +calls `LoroDoc::_apply_diff` directly and keeps what applied. + +## Why a rollback, not a pre-check + +`_apply_diff` (`crates/loro-internal/src/loro.rs`) turns each entry of a batch +into local ops. Entries fail for many reasons: positions out of range +(`OutOfBound`), a diff made for another version of the doc, a diff type that +doesn't match the container, a missing container, a tree parent the doc +doesn't have. Checking the whole batch first would mean predicting what every +handler's `apply_diff` does while the loop recreates containers and remaps ids +(the unknown-container pre-check in +[unknown-containers.md](unknown-containers.md) already needs that for one +error). Local ops can instead be undone, because nothing outside the doc has +seen them until the transaction commits. + +A stale tree diff used to panic on a parent the doc doesn't have +(`TreeHandler::create_at_with_target_for_apply_diff` unwrapped +`is_node_deleted`); it returns that error now, and the batch is rolled back. + +## How it works + +`LoroDoc::apply_diff_all_or_nothing` runs the batch in the current transaction, +after any uncommitted edits, which must survive a rollback. Committing them first would have been simpler, but it +turns N `apply_diff` calls before one `commit` into N commits (N events, N undo +steps). + +1. `begin_apply_diff_scope` marks the transaction + (`Transaction::begin_rollback_scope`: the next counter and lamport, the + first-appearance flag) and starts `DocState::begin_local_rollback`. +2. On success the marks are dropped. The ops stay in the transaction as before. +3. On failure, `rollback_apply_diff_scope`: + - `Transaction::roll_back_scope` takes the transaction's ops out and keeps + the ones before the batch (slicing an op the batch's first op was merged + into), restores the event hints the batch touched (their count and last + hint, recorded on first touch, since the batch's first hint may have been + merged into it) and the counters; + - resets the DAG's local version to before the transaction + (`AppDag::discard_pending_txn`, with the DAG frontiers the transaction + saved when it began), opens an import rollback scope, and inserts all the + transaction's ops as a (non-local) change; + - checks the state out from the batch's last op back to the op before the + batch (the middle of that change) with a fresh `DiffCalculator::new(false)` + (the doc's persistent one never sees the change), with the event recorder + taken out so nothing is recorded; + - rolls the op log back with `OpLog::rollback_import_keeping_arena` (change + store, DAG, pending changes, history cache) and applies the kept ops to the + DAG's version again (`update_version_on_new_local_op`); + - finishes with `DocState::finish_local_rollback`. + +The transaction goes on: its options, `on_commit` and kept ops are untouched, +and the next commit reports only the kept ops. + +The checkout costs a multiple of the batch (measured 2x for a text batch, 6x +for a list and movable-list batch); freeing the history cache makes the next +checkout rebuild it. Only the failure path pays either. A successful call pays +two more transaction and state locks (about 60 ns) and a hash lookup per op. + +## Reused op ids + +The next local ops reuse the counters and lamports of the discarded ops, so +anything keyed by them must not survive: + +- Container states: the checkout restores every container the ops touched. + `InnerStore` records the containers that got new (not loaded) state during + the scope (`created_journal`), and `finish_local_rollback` removes them, so + the store is as before, including which roots `get_deep_value` shows. A + container created by the batch gets fresh state when an op creates it again. +- Counters: the checkout subtracts the increments, which may not give back the + same float, so their old values are recorded on the first op and set back. +- Parent links: `SharedArena::forget_parents_of_discarded_ops` drops the links + of normal containers whose id is at or after the batch's first op. Without a + link such a container reads as deleted without being cached as deleted + (`dead-container-cache.md`), and the next op that creates it links it again. + The registrations stay: other code may hold their indices. +- The arena is not rolled back. The checkout may register old containers and + create state for them; freeing those indices would leave state entries behind + (loro-dev/loro#1164). The discarded ops' values stay allocated, so the next + op's values are not adjacent to the previous op's and the two are stored as + separate ops instead of one merged op. +- `dead_containers_cache` and `alive_containers_cache` are cleared. + +## Other threads + +The handlers lock the transaction per op, so another thread can edit or commit +while a batch runs. Rolling back would then drop that thread's edits, so the +batch is left partly applied (the behavior before #1154) and a warning is +logged: + +- another thread's op in the scoped transaction sets `foreign_ops` (checked in + `Transaction::apply_local_op` by a thread-local address); +- a commit, import, checkout or export on another thread commits the scoped + transaction, so the transaction found at failure is not the scoped one; +- a batch that starts while another thread's batch runs in the same + transaction gets no scope. + +Scope ids also key the `DocState` journal, so a stale scope cannot end a newer +one's journal. + +## Tests + +- `crates/loro/tests/apply_diff_atomicity.rs`: the #1154 repro, type mismatch, + missing container, detached editing, shallow docs, events, `UndoManager`, + pending edits (still uncommitted, their commit message, ops and event hints + the batch merged into, the single event of the next commit), counters, + containers and tree nodes created by the failed batch and created again, + twins that make the same edits without the failed call (random stale + `doc.diff(a, b)` batches, including mergeable children, and a fixed case for + existing containers), `revert_to`, and another thread editing during failing + batches. + `LORO_APPLY_DIFF_ATOMICITY_SEEDS` runs more than the default 120 seeds. +- `crates/loro-wasm/tests/apply_diff_atomicity.test.ts`: `applyDiff` throws and + leaves the doc, events and undo stack unchanged. diff --git a/context/unknown-containers.md b/context/unknown-containers.md index 9fab2b20a..60d954685 100644 --- a/context/unknown-containers.md +++ b/context/unknown-containers.md @@ -1,6 +1,6 @@ # Unknown Container Types -Verified against code 2026-09-28 (after #1134, #1152, #1153; third review of #1142). +Verified against code 2026-09-30 (after #1134, #1152, #1153; third review of #1142; #1154). A container whose type this version doesn't know (`ContainerType::Unknown(k)`, written by a newer Loro) can be imported, exported, checked out, moved in a @@ -24,11 +24,14 @@ All of these return `LoroError::ArgErr` built by ## Why `_apply_diff` checks before applying -Local ops have no rollback: an op changes the state, the oplog DAG's local -version and the arena as it is applied, and `_apply_diff` keeps applying the -rest of a batch after an error (#1154). So an unknown container has to be found -before the first entry is applied. The loop changes its own inputs as it runs, -so the check predicts that from the whole batch: +`_apply_diff` keeps applying the rest of a batch after an error. `apply_diff` +and `revert_to` now roll a failed batch back +([apply-diff-atomicity.md](apply-diff-atomicity.md), #1154), but undo/redo +calls `_apply_diff` without a rollback and keeps what applied. So an unknown +container has to be found before the first entry is applied; for `apply_diff` +the check is also the cheaper rejection and keeps its debug assertions on the +loop. The loop changes its own inputs as it runs, so the check predicts that +from the whole batch: - *fresh*: container values of Map/List diffs, the metas of created or moved tree nodes, and the mergeable children of those are recreated under new ids diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index e23176236..c7805ec51 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -9,6 +9,8 @@ over graceful degradation. - `src/loro.rs`: document-level orchestration for commit, import/export, checkout, barriers, state/oplog coordination, and event emission. + `apply_diff`/`revert_to` roll a failed batch back: + [../../context/apply-diff-atomicity.md](../../context/apply-diff-atomicity.md). - `src/encoding.rs`: public/internal `ExportMode`, binary header parsing, checksum verification, `EncodeMode` dispatch, import metadata, and the bridge from decoded changes into `OpLog`. diff --git a/crates/loro-internal/src/arena.rs b/crates/loro-internal/src/arena.rs index 074eb50ad..9d6863af3 100644 --- a/crates/loro-internal/src/arena.rs +++ b/crates/loro-internal/src/arena.rs @@ -462,6 +462,30 @@ impl SharedArena { self.inner.containers.write().set_parent(child, parent); } + /// Forget the parent links of the normal containers whose creating op is `peer`'s op at + /// `start_counter` or later, after such ops were discarded (a rolled back local + /// transaction). The next op at that id may create the container under another parent, + /// and until then the container has no parent, so it reads as deleted without being + /// cached as deleted. The registrations stay: other code may hold their indices. The + /// caller removes their state. See `context/apply-diff-atomicity.md`. + pub(crate) fn forget_parents_of_discarded_ops(&self, peer: PeerID, start_counter: Counter) { + let mut containers = self.inner.containers.write(); + let discarded: Vec = containers + .container_idx_to_id + .iter() + .enumerate() + .filter(|(_, id)| { + matches!(id, ContainerID::Normal { peer: p, counter, .. } + if *p == peer && *counter >= start_counter) + }) + .map(|(i, id)| ContainerIdx::from_index_and_type(i as u32, id.container_type())) + .collect(); + for idx in discarded { + containers.parents.remove(&idx); + containers.depth[idx.to_index() as usize] = None; + } + } + pub fn log_hierarchy(&self) { if cfg!(debug_assertions) { let containers = self.inner.containers.read(); diff --git a/crates/loro-internal/src/handler/tree.rs b/crates/loro-internal/src/handler/tree.rs index bf44e4dae..361cf9005 100644 --- a/crates/loro-internal/src/handler/tree.rs +++ b/crates/loro-internal/src/handler/tree.rs @@ -480,9 +480,12 @@ impl TreeHandler { } } - let with_event = !parent - .tree_id() - .is_some_and(|p| self.is_node_deleted(&p).unwrap()); + // A diff made for another version of the doc can name a parent this doc + // doesn't have: an error, which `apply_diff` rolls back. + let with_event = match parent.tree_id() { + Some(p) => !self.is_node_deleted(&p)?, + None => true, + }; if !with_event { return Ok(false); } diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 571b5b679..a4be184ad 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -1376,7 +1376,7 @@ impl LoroDoc { let diff = self.diff_events(&f, target, true)?; // This doc kept the state of the mergeable children the target re-activates, so apply // their actual changes rather than the full states `diff` reports. - self._apply_diff( + self.apply_diff_all_or_nothing( DiffBatch::from_changes(diff), &mut Default::default(), false, @@ -1469,9 +1469,192 @@ impl LoroDoc { /// A mergeable child that the batch re-activates is aligned with this doc's hidden state /// only if `diff.full_state` is set (batches from [`LoroDoc::diff`]); otherwise its entry /// is applied as an increment. See [`DiffBatch::full_state`]. + /// + /// All or nothing: when it returns `Err`, the doc, its history and its subscribers are as + /// before the call, and uncommitted edits made before it are kept. See + /// `context/apply-diff-atomicity.md`. pub fn apply_diff(&self, diff: DiffBatch) -> LoroResult<()> { let align = diff.full_state; - self._apply_diff(diff, &mut Default::default(), true, align) + self.apply_diff_all_or_nothing(diff, &mut Default::default(), true, align) + } + + /// [`Self::_apply_diff`], rolled back when it fails (loro-dev/loro#1154). Undo uses + /// `_apply_diff` directly and keeps what applied. See `context/apply-diff-atomicity.md`. + pub(crate) fn apply_diff_all_or_nothing( + &self, + diff: DiffBatch, + container_remap: &mut FxHashMap, + skip_unreachable: bool, + align_revived_mergeable: bool, + ) -> LoroResult<()> { + if !self.can_edit() { + return Err(LoroError::EditWhenDetached); + } + + let scope = self.begin_apply_diff_scope(); + let result = self._apply_diff( + diff, + container_remap, + skip_unreachable, + align_revived_mergeable, + ); + if let Some(scope) = scope { + if result.is_ok() { + self.end_apply_diff_scope(scope); + } else { + self.rollback_apply_diff_scope(scope); + } + } + result + } + + /// Marks where the batch starts in the transaction it will run in. + /// + /// Returns `None`, and the batch runs without a rollback, when there is no transaction + /// to run it in (the batch then fails before it applies anything) or another thread's + /// batch runs in it. + fn begin_apply_diff_scope(&self) -> Option { + static NEXT_SCOPE_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + + let mut guard = self.txn.lock(); + if guard.is_none() && !cfg!(target_arch = "wasm32") && self.can_edit() { + // Like the handlers' `with_txn`, which the batch's first op would call + drop(guard); + self.start_auto_commit(); + guard = self.txn.lock(); + } + let txn = guard.as_mut()?; + if txn.has_rollback_scope() { + return None; + } + let id = NEXT_SCOPE_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + txn.begin_rollback_scope(id); + self.state.lock().begin_local_rollback(id); + Some(id) + } + + fn end_apply_diff_scope(&self, scope: u64) { + let mut guard = self.txn.lock(); + if let Some(txn) = guard.as_mut() { + txn.end_rollback_scope(scope); + } + self.state.lock().end_local_rollback(scope); + } + + /// Undo the ops of a failed batch, keeping the ops the transaction held before it. + /// Emits nothing. + /// + /// The transaction's ops are inserted into the op log as a change, under an import + /// rollback scope, so that the diff calculator can compute the checkout from the batch's + /// last op back to the version before the batch; then the scope is rolled back and the + /// ops before the batch are put back into the DAG's version as pending local ops. + fn rollback_apply_diff_scope(&self, scope: u64) { + let mut guard = self.txn.lock(); + let Some(txn) = guard + .as_mut() + .filter(|txn| txn.rollback_scope_is_intact(scope)) + else { + // Another thread committed the transaction or applied ops in it while the batch + // ran. Undoing it would drop their edits, so the batch stays partly applied. + warn!("apply_diff failed while another thread edited the doc; not rolled back"); + if let Some(txn) = guard.as_mut() { + txn.end_rollback_scope(scope); + } + self.state.lock().end_local_rollback(scope); + return; + }; + + let batch = txn.roll_back_scope(scope); + let mut oplog = self.oplog.lock(); + let mut state = self.state.lock(); + if batch.batch_start < batch.batch_end { + let peer = batch.peer; + let txn_ops_before = (batch.batch_start - batch.txn_start_counter) as usize; + let before_batch = if txn_ops_before == 0 { + batch.deps.clone() + } else { + Frontiers::from_id(ID::new(peer, batch.batch_start - 1)) + }; + #[cfg(debug_assertions)] + { + // Only the txn's ops changed the DAG's version since it began: anything else + // (an import, a checkout) commits the txn first. + let mut expected = batch.dag_frontiers_at_start.clone(); + expected.update_frontiers_on_new_change( + ID::new(peer, batch.batch_end - 1), + &batch.deps, + ); + assert_eq!(&expected, oplog.dag.frontiers()); + assert_eq!(oplog.dag.vv().get(&peer), Some(&batch.batch_end)); + } + oplog.dag.discard_pending_txn( + batch.dag_frontiers_at_start, + peer, + batch.txn_start_counter, + ); + oplog.begin_import_rollback(); + oplog.insert_new_change( + Change { + id: ID::new(peer, batch.txn_start_counter), + lamport: batch.txn_start_lamport, + deps: batch.deps.clone(), + timestamp: batch.timestamp, + commit_msg: None, + ops: batch.ops, + }, + false, + ); + + let batch_last = state.frontiers.clone(); + debug_assert_eq!( + batch_last, + Frontiers::from_id(ID::new(peer, batch.batch_end - 1)) + ); + let from = oplog + .dag + .frontiers_to_vv(&batch_last) + .expect("the batch's ops are in the DAG"); + let to = oplog + .dag + .frontiers_to_vv(&before_batch) + .expect("the version before the batch is in the DAG"); + let (diff, diff_mode) = DiffCalculator::new(false).calc_diff_internal( + &oplog, + &from, + &batch_last, + &to, + &before_batch, + None, + ); + // `DocState::apply_diff` refuses to run inside a transaction, and records nothing + // without its event recorder. + state.abort_txn(); + let recorder = state.take_event_recorder(); + state + .apply_diff( + InternalDocDiff { + origin: Default::default(), + by: EventTriggerKind::Checkout, + diff: Cow::Owned(diff), + new_version: Cow::Owned(before_batch), + }, + diff_mode, + ) + .expect("undoing the ops of a failed apply_diff from the state"); + state.restore_event_recorder(recorder); + state.resume_txn(); + oplog.rollback_import_keeping_arena(&state); + if txn_ops_before > 0 { + oplog.dag.update_version_on_new_local_op( + &batch.deps, + ID::new(peer, batch.txn_start_counter), + batch.txn_start_lamport, + txn_ops_before, + ); + oplog.refresh_visible_op_count(); + } + } + state.finish_local_rollback(scope, batch.peer, batch.batch_start); } /// Apply a diff to the current state. @@ -1512,8 +1695,9 @@ impl LoroDoc { } else { FullStatePlan::default() }; - // There is no rollback for the local ops applied below, so reject the - // diff before touching the doc if it needs an unknown container. + // Undo applies its diff without a rollback (`apply_diff` and + // `revert_to` roll back a failed batch), so reject the diff before + // touching the doc if it needs an unknown container. let predicted_skips = self.check_apply_diff_creates_no_unknown_container( &diff, align_revived_mergeable.then_some(&plan), diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index 9e8513036..d5d5f78b5 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -447,6 +447,23 @@ impl OpLog { self.refresh_visible_op_count(); } + /// [`Self::rollback_import`] without the arena rollback, for a scope that only inserted a + /// discarded local transaction (`LoroDoc::rollback_apply_diff_scope`): its containers were + /// registered by the transaction, and undoing it from the state may register old + /// containers and create state for them. See `context/apply-diff-atomicity.md`. + pub(crate) fn rollback_import_keeping_arena(&mut self, _state: &DocState) { + let Some(rollback) = self.import_rollback.take() else { + return; + }; + + self.change_store + .rollback_import_keeping_arena(rollback.change_store); + self.dag.rollback_import(); + rollback.pending.rollback(&mut self.pending_changes); + self.history_cache.lock().free_all(); + self.refresh_visible_op_count(); + } + /// Rolls the arena back to a checkpoint taken before an import that failed before its /// import rollback scope began. See [`ChangeStore::rollback_arena`], and /// [`Self::rollback_import`] for `state`. diff --git a/crates/loro-internal/src/oplog/change_store.rs b/crates/loro-internal/src/oplog/change_store.rs index e52c79e67..a53ff72c6 100644 --- a/crates/loro-internal/src/oplog/change_store.rs +++ b/crates/loro-internal/src/oplog/change_store.rs @@ -419,10 +419,29 @@ impl ChangeStore { &self, rollback: ChangeStoreRollback, arena: SharedArenaRollback, + ) { + let mut inner = self.inner.lock(); + Self::rollback_changes_in(&self.arena, &mut inner, rollback); + self.rollback_arena_in(&mut inner, arena); + } + + /// [`Self::rollback_import`] without rolling the arena back: everything registered in it + /// since the scope began stays registered. For a scope that only held changes whose + /// containers were registered before it (a rolled back local transaction), where freeing + /// the registrations made while undoing it from the state would leave state entries at + /// freed indices. + pub(crate) fn rollback_import_keeping_arena(&self, rollback: ChangeStoreRollback) { + let mut inner = self.inner.lock(); + Self::rollback_changes_in(&self.arena, &mut inner, rollback); + } + + fn rollback_changes_in( + arena: &SharedArena, + inner: &mut ChangeStoreInner, + rollback: ChangeStoreRollback, ) { // The name set may already include names from changes this rollback removes. That is // fine: stale names only make `old_history_may_touch_root_names` conservatively true. - let mut inner = self.inner.lock(); let mut touched_peers = FxHashSet::default(); inner.mem_parsed_kv.retain(|id, _| { let old_end = rollback.old_vv.get(&id.peer).copied().unwrap_or(0); @@ -448,7 +467,7 @@ impl ChangeStore { let changes = Arc::make_mut( block .content - .changes_mut(&self.arena) + .changes_mut(arena) .expect("an unflushed block always holds parsed changes"), ); changes.truncate(shape.n_changes); @@ -475,8 +494,6 @@ impl ChangeStore { .mem_parsed_kv .retain(|id, block| !block.flushed || !touched_peers.contains(&id.peer)); } - - self.rollback_arena_in(&mut inner, arena); } /// Rolls the arena back to `arena`, a checkpoint taken before a failed import, and drops diff --git a/crates/loro-internal/src/oplog/loro_dag.rs b/crates/loro-internal/src/oplog/loro_dag.rs index 8f289504b..4c7684d49 100644 --- a/crates/loro-internal/src/oplog/loro_dag.rs +++ b/crates/loro-internal/src/oplog/loro_dag.rs @@ -922,6 +922,22 @@ impl AppDag { } } + /// Undo [`Self::update_version_on_new_local_op`] for every op of a + /// transaction that is discarded instead of committed: `frontiers` and + /// `start_counter` are the DAG's frontiers and `peer`'s end before its + /// first op. + pub(crate) fn discard_pending_txn( + &mut self, + frontiers: Frontiers, + peer: PeerID, + start_counter: Counter, + ) { + self.pending_txn_node = None; + self.frontiers = frontiers; + // An end of 0 removes the peer. + self.vv.set_last(ID::new(peer, start_counter - 1)); + } + pub(crate) fn latest_vv_contains_peer(&self, peer: PeerID) -> bool { self.vv.contains_key(&peer) && *self.vv.get(&peer).unwrap() > 0 } diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index d483858d1..23943d003 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -178,6 +178,20 @@ pub struct DocState { /// ([`crate::undo::DiffBatch::from_changes`]: revert, undo). Containers that keep their /// state are then not revived at all, which skips building full states nobody reads. record_changes_only: bool, + /// Set while a local transaction that may be rolled back runs (`apply_diff`, + /// `revert_to`). See [`Self::begin_local_rollback`]. + local_rollback: Option, +} + +/// What undoing a local transaction's ops by checkout cannot restore exactly, +/// recorded as the ops are applied. See `context/apply-diff-atomicity.md`. +struct LocalRollbackJournal { + /// The `apply_diff` scope that began it. + scope: u64, + /// The value of each counter before the transaction's first op on it: + /// subtracting the increments again may not give back the same float. + #[cfg(feature = "counter")] + counters: FxHashMap, } struct AliveContainersCache { @@ -504,6 +518,7 @@ impl DocState { dead_containers_cache: Default::default(), alive_containers_cache: None, record_changes_only: false, + local_rollback: None, }, crate::lock::LockKind::DocState, )) @@ -530,6 +545,7 @@ impl DocState { dead_containers_cache: Default::default(), alive_containers_cache: None, record_changes_only: false, + local_rollback: None, })) } @@ -1022,6 +1038,13 @@ impl DocState { if self.in_txn { self.changed_idx_in_txn.insert(op.container); } + #[cfg(feature = "counter")] + if let (Some(journal), State::CounterState(counter)) = (&mut self.local_rollback, &*state) { + journal + .counters + .entry(op.container) + .or_insert_with(|| counter.value()); + } // A local move can take a node out of a deleted subtree. Movable-list // elements cannot be revived locally: a deleted one has no index. if let RawOpContent::Tree(tree_op) = &raw_op.content { @@ -1033,6 +1056,84 @@ impl DocState { Ok(()) } + /// Start recording what [`Self::finish_local_rollback`] needs to undo the + /// local ops applied from now on: the containers that get new state and the + /// counters' values. + /// + /// `scope` identifies the caller. A journal of another scope is replaced: two + /// scopes never share a transaction, so that scope's transaction was + /// committed by another thread and cannot be rolled back anyway. + pub(crate) fn begin_local_rollback(&mut self, scope: u64) { + self.store.begin_created_journal(); + self.local_rollback = Some(LocalRollbackJournal { + scope, + #[cfg(feature = "counter")] + counters: Default::default(), + }); + } + + /// Stop recording for `scope`, keeping the ops. + pub(crate) fn end_local_rollback(&mut self, scope: u64) { + if self + .local_rollback + .as_ref() + .is_some_and(|j| j.scope == scope) + { + self.store.take_created_journal(); + self.local_rollback = None; + } + } + + /// Takes the event recorder out, so that a state change the caller discards + /// is not recorded; put it back with [`Self::restore_event_recorder`]. + pub(crate) fn take_event_recorder(&mut self) -> EventRecorder { + std::mem::take(&mut self.event_recorder) + } + + pub(crate) fn restore_event_recorder(&mut self, recorder: EventRecorder) { + self.event_recorder = recorder; + } + + /// Finish rolling back the local ops applied since + /// [`Self::begin_local_rollback`], after the caller undid them by checking + /// the state out at the version before them. `peer` and `start_counter` are + /// the id of the first op. Restores what the checkout cannot: + /// + /// - containers that got state since (new or never loaded before) lose it, + /// so the store is the same as before, including which roots show up; + /// - counters get their exact old values; + /// - containers the ops created lose their parent link, since the next ops + /// reuse their ids (`SharedArena::forget_parents_of_discarded_ops`); + /// - the liveness caches, which may hold answers for the state in between. + pub(crate) fn finish_local_rollback( + &mut self, + scope: u64, + peer: PeerID, + start_counter: crate::id::Counter, + ) { + let created = self.store.take_created_journal(); + let journal = self.local_rollback.take(); + assert!( + journal.as_ref().is_some_and(|j| j.scope == scope), + "the journal of a rolled back apply_diff was replaced" + ); + #[cfg(feature = "counter")] + if let Some(journal) = journal { + for (idx, value) in journal.counters { + if let Some(State::CounterState(counter)) = self.store.get_container_mut(idx) { + counter.set_value(value); + } + } + } + #[cfg(not(feature = "counter"))] + let _ = journal; + self.store.remove_created(&created); + self.arena + .forget_parents_of_discarded_ops(peer, start_counter); + self.dead_containers_cache.clear(); + self.alive_containers_cache = None; + } + pub(crate) fn start_txn(&mut self, origin: InternalString, trigger: EventTriggerKind) { self.pre_txn(origin, trigger); self.in_txn = true; @@ -1042,6 +1143,12 @@ impl DocState { self.in_txn = false; } + /// Undo [`Self::abort_txn`] for a transaction that goes on + /// (`LoroDoc::rollback_apply_diff_scope`). + pub(crate) fn resume_txn(&mut self) { + self.in_txn = true; + } + pub fn iter_and_decode_all(&mut self) -> impl Iterator { self.store.iter_and_decode_all() } @@ -2736,7 +2843,7 @@ fn trigger_on_new_container( } #[derive(Default, Clone)] -struct EventRecorder { +pub(crate) struct EventRecorder { recording_diff: bool, // A batch of diffs will be converted to a event when // they cannot be merged with the next diff. diff --git a/crates/loro-internal/src/state/container_store.rs b/crates/loro-internal/src/state/container_store.rs index 6d900b6b2..687054b27 100644 --- a/crates/loro-internal/src/state/container_store.rs +++ b/crates/loro-internal/src/state/container_store.rs @@ -316,6 +316,18 @@ impl ContainerStore { LoadAllFlag } + pub(super) fn begin_created_journal(&mut self) { + self.store.begin_created_journal(); + } + + pub(super) fn take_created_journal(&mut self) -> Vec { + self.store.take_created_journal() + } + + pub(super) fn remove_created(&mut self, created: &[ContainerIdx]) { + self.store.remove_created(created); + } + pub(super) fn get_or_create_mut(&mut self, idx: ContainerIdx) -> &mut State { self.store .get_or_insert_with(idx, || { diff --git a/crates/loro-internal/src/state/container_store/inner_store.rs b/crates/loro-internal/src/state/container_store/inner_store.rs index a6bfce712..ecd2eb701 100644 --- a/crates/loro-internal/src/state/container_store/inner_store.rs +++ b/crates/loro-internal/src/state/container_store/inner_store.rs @@ -61,6 +61,10 @@ pub(crate) struct InnerStore { /// longer implies `store` contains every entry in `kv`, so `load_all()` /// must re-scan `kv` instead of short-circuiting. evicted_since_full_load: bool, + /// While `Some`, the containers given new (not loaded) state are recorded, so + /// a rolled back local transaction can remove them again + /// (`DocState::begin_local_rollback`). + created_journal: Option>, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -132,11 +136,15 @@ impl InnerStore { if self.get_entry_mut(idx).is_none() { let id = self.arena.get_container_id(idx).unwrap(); let key = id.to_bytes(); - let container = self - .kv - .get(&key) - .map(ContainerWrapper::new_from_bytes) - .unwrap_or_else(f); + let container = match self.kv.get(&key) { + Some(bytes) => ContainerWrapper::new_from_bytes(bytes), + None => { + if let Some(journal) = &mut self.created_journal { + journal.push(idx); + } + f() + } + }; Self::insert_entry(&mut self.store, idx, container); } @@ -160,10 +168,39 @@ impl InnerStore { return; } + if let Some(journal) = &mut self.created_journal { + journal.push(idx); + } let c = f(); Self::insert_entry(&mut self.store, idx, c); } + /// Start recording the containers that get new state. See `created_journal`. + pub(crate) fn begin_created_journal(&mut self) { + self.created_journal = Some(Vec::new()); + } + + /// Stop recording and return the containers that got new state since + /// [`Self::begin_created_journal`]. + pub(crate) fn take_created_journal(&mut self) -> Vec { + self.created_journal.take().unwrap_or_default() + } + + /// Drop the state of containers that got new state while a rolled back + /// local transaction ran. They had no state and no KV entry before it (the + /// KV store is only written by `flush`), so this restores the store. + pub(crate) fn remove_created(&mut self, created: &[ContainerIdx]) { + for &idx in created { + debug_assert!(self + .arena + .get_container_id(idx) + .is_none_or(|id| self.kv.get(&id.to_bytes()).is_none())); + if let Some(slot) = self.store.get_mut(Self::slot(idx)) { + *slot = None; + } + } + } + pub(crate) fn get_mut(&mut self, idx: ContainerIdx) -> Option<&mut ContainerWrapper> { if self.get_entry_mut(idx).is_none() { let id = self.arena.get_container_id(idx).unwrap(); @@ -659,6 +696,7 @@ impl InnerStore { config, value_cache_queue: VecDeque::new(), evicted_since_full_load: false, + created_journal: None, } } diff --git a/crates/loro-internal/src/state/counter_state.rs b/crates/loro-internal/src/state/counter_state.rs index 332b2f52d..861f39022 100644 --- a/crates/loro-internal/src/state/counter_state.rs +++ b/crates/loro-internal/src/state/counter_state.rs @@ -18,6 +18,17 @@ pub struct CounterState { value: f64, } +impl CounterState { + pub(crate) fn value(&self) -> f64 { + self.value + } + + /// For rolling back local ops (`DocState::finish_local_rollback`). + pub(crate) fn set_value(&mut self, value: f64) { + self.value = value; + } +} + impl CounterState { pub(crate) fn new(idx: ContainerIdx) -> Self { Self { idx, value: 0. } diff --git a/crates/loro-internal/src/txn.rs b/crates/loro-internal/src/txn.rs index 62a92d762..1bfc58790 100644 --- a/crates/loro-internal/src/txn.rs +++ b/crates/loro-internal/src/txn.rs @@ -166,6 +166,52 @@ pub struct Transaction { msg: Option>, latest_timestamp: Timestamp, pub(super) is_peer_first_appearance: bool, + /// The DAG's frontiers when the txn began, before its first op. + dag_frontiers_at_start: Frontiers, + /// Set while `LoroDoc::apply_diff` or `revert_to` runs a batch in this txn, + /// which is rolled back if the batch fails (context/apply-diff-atomicity.md). + rollback_scope: Option, +} + +/// See [`Transaction::begin_rollback_scope`]. +#[derive(Debug)] +struct RollbackScope { + id: u64, + thread: usize, + /// Another thread applied an op in this txn while the batch ran. + foreign_ops: bool, + /// The txn's next counter and lamport when the batch began. + counter: Counter, + lamport: Lamport, + is_peer_first_appearance: bool, + /// For each container the batch gave an event hint to: how many hints it + /// had before, and the last one, which the batch's first hint may have been + /// merged into. + event_hints: FxHashMap)>, +} + +/// Identifies the current thread for [`RollbackScope`]: the address of a +/// thread local is unique among the live threads. +fn current_thread_mark() -> usize { + thread_local!(static MARK: u8 = const { 0 }); + MARK.with(|m| m as *const u8 as usize) +} + +/// What [`Transaction::roll_back_scope`] took out of a txn. +pub(crate) struct RolledBackBatch { + pub(crate) peer: PeerID, + /// The txn's first op, its lamport, and the version it was applied on. + pub(crate) txn_start_counter: Counter, + pub(crate) txn_start_lamport: Lamport, + pub(crate) deps: Frontiers, + /// The DAG's frontiers before the txn's first op. + pub(crate) dag_frontiers_at_start: Frontiers, + /// The batch's ops are `batch_start..batch_end`; the txn keeps the ops before. + pub(crate) batch_start: Counter, + pub(crate) batch_end: Counter, + /// Every op of the txn, including the batch's. + pub(crate) ops: RleVec<[Op; 1]>, + pub(crate) timestamp: Timestamp, } impl std::fmt::Debug for Transaction { @@ -350,6 +396,7 @@ impl Transaction { let peer = state_lock.peer.load(std::sync::atomic::Ordering::Relaxed); let next_counter = oplog_lock.next_id(peer).counter; let next_lamport = oplog_lock.dag.frontiers_to_next_lamport(&frontiers); + let dag_frontiers_at_start = oplog_lock.dag.frontiers().clone(); let latest_timestamp = oplog_lock.get_greatest_timestamp(&frontiers); if let Err(err) = oplog_lock.check_change_greater_than_last_peer_id(peer, next_counter, &frontiers) @@ -377,6 +424,8 @@ impl Transaction { msg: None, latest_timestamp, is_peer_first_appearance: false, + dag_frontiers_at_start, + rollback_scope: None, }) } @@ -532,6 +581,82 @@ impl Transaction { Ok(None) } + /// Mark the start of a batch that `LoroDoc` rolls back if it fails + /// ([`Self::roll_back_scope`]). The txn may already hold ops; they are kept. + pub(crate) fn begin_rollback_scope(&mut self, id: u64) { + debug_assert!(self.rollback_scope.is_none()); + self.rollback_scope = Some(RollbackScope { + id, + thread: current_thread_mark(), + foreign_ops: false, + counter: self.next_counter, + lamport: self.next_lamport, + is_peer_first_appearance: self.is_peer_first_appearance, + event_hints: FxHashMap::default(), + }); + } + + pub(crate) fn has_rollback_scope(&self) -> bool { + self.rollback_scope.is_some() + } + + pub(crate) fn end_rollback_scope(&mut self, id: u64) { + if self.rollback_scope.as_ref().is_some_and(|s| s.id == id) { + self.rollback_scope = None; + } + } + + /// Whether this is the txn scope `id` began in and every op in it came from + /// the thread that began it. + pub(crate) fn rollback_scope_is_intact(&self, id: u64) -> bool { + self.rollback_scope + .as_ref() + .is_some_and(|s| s.id == id && !s.foreign_ops) + } + + /// Take the ops of scope `id` (which must be intact) out of the txn: it + /// keeps its ops from before the scope, and its counters and event hints are + /// as they were then. The ops stay applied to the state and the DAG's + /// version; the caller undoes them. + pub(crate) fn roll_back_scope(&mut self, id: u64) -> RolledBackBatch { + assert!(self.rollback_scope_is_intact(id)); + let scope = self.rollback_scope.take().unwrap(); + let ops = take(&mut self.local_ops); + for op in ops.iter() { + let end = op.counter + op.atom_len() as Counter; + if end <= scope.counter { + self.local_ops.push(op.clone()); + } else if op.counter < scope.counter { + self.local_ops + .push(op.slice(0, (scope.counter - op.counter) as usize)); + } + } + for (container, (len, last)) in scope.event_hints { + if len == 0 { + self.event_hints.remove(&container); + continue; + } + let hints = self.event_hints.get_mut(&container).unwrap(); + hints.truncate(len); + *hints.last_mut().unwrap() = last.unwrap(); + } + let batch_end = self.next_counter; + self.next_counter = scope.counter; + self.next_lamport = scope.lamport; + self.is_peer_first_appearance = scope.is_peer_first_appearance; + RolledBackBatch { + peer: self.peer, + txn_start_counter: self.start_counter, + txn_start_lamport: self.start_lamport, + deps: self.frontiers.clone(), + dag_frontiers_at_start: self.dag_frontiers_at_start.clone(), + batch_start: scope.counter, + batch_end, + ops, + timestamp: self.latest_timestamp, + } + } + fn take_options(&self) -> CommitOptions { let mut options = CommitOptions::new(); if !self.origin.is_empty() { @@ -580,6 +705,12 @@ impl Transaction { return Err(LoroError::UnmatchedContext { expected, found }); } + if let Some(scope) = &mut self.rollback_scope { + if scope.thread != current_thread_mark() { + scope.foreign_ops = true; + } + } + let len = content.content_len(); assert!(len > 0); let raw_op = RawOp { @@ -639,6 +770,12 @@ impl Transaction { ); let container_hints = self.event_hints.entry(container).or_default(); + if let Some(scope) = &mut self.rollback_scope { + scope + .event_hints + .entry(container) + .or_insert_with(|| (container_hints.len(), container_hints.last().cloned())); + } match container_hints.last_mut() { Some(last) if last.can_merge(&event) => { diff --git a/crates/loro-wasm/tests/apply_diff_atomicity.test.ts b/crates/loro-wasm/tests/apply_diff_atomicity.test.ts new file mode 100644 index 000000000..b599bf9ef --- /dev/null +++ b/crates/loro-wasm/tests/apply_diff_atomicity.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from "vitest"; +import { ContainerID, LoroDoc, UndoManager } from "../bundler/index"; + +// `applyDiff` is all or nothing: a batch with an invalid entry throws and +// leaves the doc, its history and its subscribers as they were +// (loro-dev/loro#1154). + +function failingBatch(): [ContainerID, unknown][] { + return [ + // Applied first, so partial application would be visible + ["cid:root-text:Text", { type: "text", diff: [{ insert: "partial" }] }], + ["cid:root-list:List", { type: "list", diff: [{ delete: 5 }] }], + ]; +} + +function snapshot(doc: LoroDoc) { + return { + value: doc.toJSON(), + version: doc.version().toJSON(), + frontiers: doc.frontiers(), + changes: doc.changeCount(), + ops: doc.opCount(), + }; +} + +describe("applyDiff atomicity", () => { + it("throws and leaves the doc unchanged when an entry is out of bounds", () => { + const doc = new LoroDoc(); + doc.setPeerId(1); + doc.getList("list").push(1); + doc.commit(); + const before = snapshot(doc); + + expect(() => doc.applyDiff(failingBatch() as never)).toThrow(); + doc.commit(); + expect(snapshot(doc)).toStrictEqual(before); + + // Still usable; the next ops reuse the rolled back batch's op ids. + doc.getText("text").insert(0, "ok"); + doc.commit(); + expect(doc.toJSON()).toStrictEqual({ list: [1], text: "ok" }); + const replay = new LoroDoc(); + replay.import(doc.export({ mode: "update" })); + expect(replay.toJSON()).toStrictEqual(doc.toJSON()); + }); + + it("emits no event and adds no undo step for a failed batch", async () => { + const doc = new LoroDoc(); + const undo = new UndoManager(doc, {}); + doc.getList("list").push(1); + doc.commit(); + let events = 0; + doc.subscribe(() => { + events += 1; + }); + let updates = 0; + doc.subscribeLocalUpdates(() => { + updates += 1; + }); + + expect(() => doc.applyDiff(failingBatch() as never)).toThrow(); + doc.commit(); + await Promise.resolve(); + expect(events).toBe(0); + expect(updates).toBe(0); + // The only undo step is the push. + expect(undo.undo()).toBe(true); + expect(doc.toJSON()).toStrictEqual({ list: [] }); + expect(undo.canUndo()).toBe(false); + }); + + it("keeps uncommitted edits made before a failed batch", () => { + const doc = new LoroDoc(); + doc.getList("list").push(1); + doc.commit(); + doc.getText("mine").insert(0, "abc"); + expect(() => doc.applyDiff(failingBatch() as never)).toThrow(); + doc.commit(); + expect(doc.toJSON()).toStrictEqual({ list: [1], mine: "abc" }); + }); + + it("rolls back a stale diff made for another version", () => { + const source = new LoroDoc(); + source.setPeerId(1); + const text = source.getText("text"); + text.insert(0, "hello world"); + source.commit(); + const v1 = source.frontiers(); + const map = source.getMap("map"); + map.set("k", 1); + text.delete(0, 6); + source.getList("list").insert(0, "x"); + source.commit(); + const diff = source.diff(v1, source.frontiers(), false); + + // Not at v1: the text is too short for the delete. + const target = new LoroDoc(); + target.getText("text").insert(0, "hi"); + target.getList("list").push(1); + target.commit(); + const before = snapshot(target); + expect(() => target.applyDiff(diff)).toThrow(); + target.commit(); + expect(snapshot(target)).toStrictEqual(before); + }); +}); diff --git a/crates/loro/tests/apply_diff_atomicity.rs b/crates/loro/tests/apply_diff_atomicity.rs new file mode 100644 index 000000000..0f64255d6 --- /dev/null +++ b/crates/loro/tests/apply_diff_atomicity.rs @@ -0,0 +1,980 @@ +//! `apply_diff` and `revert_to` are all-or-nothing: a batch that fails leaves +//! the document, its history and its subscribers as they were +//! (loro-dev/loro#1154). See `context/apply-diff-atomicity.md`. + +use std::sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, +}; + +use loro::{ + event::{Diff, DiffBatch, ListDiffItem, MapDelta}, + ContainerID, ContainerTrait, ContainerType, ExportMode, Frontiers, LoroCounter, LoroDoc, + LoroList, LoroMap, LoroResult, LoroText, LoroValue, TextDelta, ToJson, TreeParentId, + UndoManager, ValueOrContainer, VersionVector, +}; +use pretty_assertions::assert_eq; +use rand::{rngs::StdRng, Rng, SeedableRng}; +use serde_json::Value; + +/// Everything a failed `apply_diff` must leave untouched. +#[derive(Debug, PartialEq)] +struct Observed { + value: Value, + vv: VersionVector, + oplog_frontiers: Frontiers, + state_frontiers: Frontiers, + len_ops: usize, + len_changes: usize, +} + +fn observe(doc: &LoroDoc) -> Observed { + Observed { + value: doc.get_deep_value().to_json_value(), + vv: doc.oplog_vv(), + oplog_frontiers: doc.oplog_frontiers(), + state_frontiers: doc.state_frontiers(), + len_ops: doc.len_ops(), + len_changes: doc.len_changes(), + } +} + +/// The deep value without empty root containers: reading a root through a +/// handler materializes it, a replay only shows roots with content. +fn value_without_empty_roots(doc: &LoroDoc) -> Value { + let mut value = doc.get_deep_value().to_json_value(); + value.as_object_mut().unwrap().retain(|_, v| match v { + Value::String(s) => !s.is_empty(), + Value::Array(a) => !a.is_empty(), + Value::Object(o) => !o.is_empty(), + Value::Number(n) => n.as_f64() != Some(0.0), + _ => true, + }); + value +} + +/// The doc agrees with a fresh replay of its own history and of its snapshot. +fn assert_consistent_with_replay(doc: &LoroDoc) { + let value = value_without_empty_roots(doc); + let from_updates = LoroDoc::new(); + from_updates + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(value_without_empty_roots(&from_updates), value); + let from_snapshot = LoroDoc::new(); + from_snapshot + .import(&doc.export(ExportMode::Snapshot).unwrap()) + .unwrap(); + assert_eq!(value_without_empty_roots(&from_snapshot), value); +} + +fn text_insert(s: &str) -> Diff<'static> { + Diff::Text(vec![TextDelta::Insert { + insert: s.into(), + attributes: None, + }]) +} + +fn map_diff(key: &'static str, value: ValueOrContainer) -> Diff<'static> { + let mut updated = rustc_hash::FxHashMap::default(); + updated.insert(key.into(), Some(value)); + Diff::Map(MapDelta { updated }) +} + +fn root(name: &str, kind: ContainerType) -> ContainerID { + ContainerID::new_root(name, kind) +} + +/// A batch whose first entries apply and whose last entry is out of bounds. +fn batch_failing_at_the_end() -> DiffBatch { + let mut batch = DiffBatch::default(); + batch + .push(root("text", ContainerType::Text), text_insert("partial")) + .unwrap(); + batch + .push( + root("list", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 5 }]), + ) + .unwrap(); + batch +} + +#[test] +fn issue_1154_out_of_bound_entry_rolls_back_the_batch() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + let before = observe(&doc); + + let r = doc.apply_diff(batch_failing_at_the_end()); + assert!( + matches!(r, Err(loro::LoroError::OutOfBound { .. })), + "{r:?}" + ); + doc.commit(); + assert_eq!(observe(&doc), before); + + // The doc is still usable, and reuses the op ids of the rolled back batch. + doc.get_text("text").insert(0, "ok").unwrap(); + doc.commit(); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": [1], "text": "ok"}) + ); + assert_consistent_with_replay(&doc); +} + +#[test] +fn diff_type_mismatch_rolls_back_the_batch() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.get_map("map").insert("k", 1).unwrap(); + doc.commit(); + let before = observe(&doc); + + let mut batch = DiffBatch::default(); + batch + .push( + root("map", ContainerType::Map), + map_diff("k", ValueOrContainer::Value(2.into())), + ) + .unwrap(); + batch + .push(root("text", ContainerType::Text), text_insert("x")) + .unwrap(); + // A text diff for a list container. + batch + .push(root("list", ContainerType::List), text_insert("y")) + .unwrap(); + assert!(doc.apply_diff(batch).is_err()); + doc.commit(); + assert_eq!(observe(&doc), before); +} + +#[test] +fn missing_container_rolls_back_the_batch() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + let before = observe(&doc); + + let mut batch = DiffBatch::default(); + batch + .push(root("text", ContainerType::Text), text_insert("x")) + .unwrap(); + batch + .push( + ContainerID::new_normal(loro::ID::new(42, 7), ContainerType::Text), + text_insert("y"), + ) + .unwrap(); + let r = doc.apply_diff(batch); + assert!( + matches!(r, Err(loro::LoroError::ContainersNotFound { .. })), + "{r:?}" + ); + doc.commit(); + assert_eq!(observe(&doc), before); +} + +/// Children created by the failed batch (map/list/movable-list containers, +/// tree nodes and their metadata) must not leave state, parent links or +/// liveness answers behind at the op ids the next edits reuse. +#[test] +fn created_containers_are_discarded_and_their_ids_reused_cleanly() { + let source = LoroDoc::new(); + source.set_peer_id(1).unwrap(); + let map = source.get_map("map"); + let child = map.insert_container("child", LoroMap::new()).unwrap(); + child.insert("a", 1).unwrap(); + let text = map.insert_container("text", LoroText::new()).unwrap(); + text.insert(0, "hello").unwrap(); + let list = source.get_list("list"); + let inner = list.insert_container(0, LoroList::new()).unwrap(); + inner.push("x").unwrap(); + let mlist = source.get_movable_list("mlist"); + let counter = mlist.insert_container(0, LoroCounter::new()).unwrap(); + counter.increment(2.5).unwrap(); + let tree = source.get_tree("tree"); + let node = tree.create(TreeParentId::Root).unwrap(); + tree.get_meta(node).unwrap().insert("name", "n").unwrap(); + let node2 = tree.create(node).unwrap(); + tree.get_meta(node2).unwrap().insert("name", "m").unwrap(); + source.get_counter("counter").increment(0.2).unwrap(); + source.commit(); + let full = source + .diff(&Frontiers::default(), &source.oplog_frontiers()) + .unwrap(); + + let doc = LoroDoc::new(); + doc.set_peer_id(2).unwrap(); + doc.get_list("list").push(1).unwrap(); + doc.get_counter("counter").increment(0.1).unwrap(); + doc.commit(); + let before = observe(&doc); + + let mut failing = full.clone(); + failing + .push( + root("zzz", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 1 }]), + ) + .unwrap(); + assert!(doc.apply_diff(failing).is_err()); + doc.commit(); + assert_eq!(observe(&doc), before); + // Restored exactly, not by subtracting the increment again. + assert_eq!(doc.get_counter("counter").get_value(), 0.1); + + // The same batch again, without the failing entry: it creates containers + // and tree nodes at the op ids the rolled back batch used. + doc.apply_diff(full).unwrap(); + doc.commit(); + let value = doc.get_deep_value().to_json_value(); + let expected = source.get_deep_value().to_json_value(); + assert_eq!(value["map"], expected["map"]); + assert_eq!(value["mlist"], expected["mlist"]); + assert_eq!(doc.get_tree("tree").nodes().len(), 2); + for n in doc.get_tree("tree").nodes() { + assert!(!doc.get_tree("tree").is_node_deleted(&n).unwrap()); + let meta = doc.get_tree("tree").get_meta(n).unwrap(); + assert!(!meta.is_deleted()); + meta.insert("edited", true).unwrap(); + } + let ValueOrContainer::Container(loro::Container::Map(child)) = + doc.get_map("map").get("child").unwrap() + else { + panic!() + }; + assert!(!child.is_deleted()); + child.insert("b", 2).unwrap(); + doc.commit(); + assert_consistent_with_replay(&doc); +} + +#[test] +fn uncommitted_edits_survive_a_failed_apply_diff() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + doc.set_next_commit_message("mine"); + doc.get_text("mine").insert(0, "abc").unwrap(); + doc.get_list("list").push(2).unwrap(); + + assert!(doc.apply_diff(batch_failing_at_the_end()).is_err()); + doc.commit(); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": [1, 2], "mine": "abc"}) + ); + let last = doc + .get_change(doc.oplog_frontiers().as_single().unwrap()) + .unwrap(); + assert_eq!(last.message(), "mine"); + assert_consistent_with_replay(&doc); +} + +#[test] +fn uncommitted_edits_and_a_successful_apply_diff_keep_the_commit_options() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + doc.set_next_commit_message("mine"); + doc.get_text("mine").insert(0, "abc").unwrap(); + let mut batch = DiffBatch::default(); + batch + .push(root("text", ContainerType::Text), text_insert("diff")) + .unwrap(); + doc.apply_diff(batch).unwrap(); + doc.commit(); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": [1], "mine": "abc", "text": "diff"}) + ); + let last = doc + .get_change(doc.oplog_frontiers().as_single().unwrap()) + .unwrap(); + assert_eq!(last.message(), "mine"); +} + +#[test] +fn failed_apply_diff_emits_no_event() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + let events = Arc::new(AtomicUsize::new(0)); + let events_in_cb = events.clone(); + let _sub = doc.subscribe_root(Arc::new(move |_| { + events_in_cb.fetch_add(1, Ordering::SeqCst); + })); + let updates = Arc::new(AtomicUsize::new(0)); + let updates_in_cb = updates.clone(); + let _sub2 = doc.subscribe_local_update(Box::new(move |_| { + updates_in_cb.fetch_add(1, Ordering::SeqCst); + true + })); + + assert!(doc.apply_diff(batch_failing_at_the_end()).is_err()); + doc.commit(); + assert_eq!(events.load(Ordering::SeqCst), 0); + assert_eq!(updates.load(Ordering::SeqCst), 0); + + // A later edit is reported as usual. + doc.get_text("text").insert(0, "x").unwrap(); + doc.commit(); + assert_eq!(events.load(Ordering::SeqCst), 1); + assert_eq!(updates.load(Ordering::SeqCst), 1); +} + +#[test] +fn undo_manager_does_not_see_a_failed_apply_diff() { + let doc = LoroDoc::new(); + let mut undo = UndoManager::new(&doc); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + assert_eq!(undo.undo_count(), 1); + + assert!(doc.apply_diff(batch_failing_at_the_end()).is_err()); + doc.commit(); + assert_eq!(undo.undo_count(), 1); + assert!(undo.undo().unwrap()); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": []}) + ); +} + +#[test] +fn failed_apply_diff_in_detached_editing_mode_keeps_the_checked_out_version() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + let v1 = doc.oplog_frontiers(); + doc.get_list("list").push(2).unwrap(); + doc.commit(); + doc.set_detached_editing(true); + doc.checkout(&v1).unwrap(); + doc.get_text("text").insert(0, "draft").unwrap(); + doc.commit(); + let before = observe(&doc); + + let mut batch = DiffBatch::default(); + batch + .push(root("text", ContainerType::Text), text_insert("partial")) + .unwrap(); + batch + .push( + root("list", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 5 }]), + ) + .unwrap(); + assert!(doc.apply_diff(batch).is_err()); + doc.commit(); + assert!(doc.is_detached()); + assert_eq!(observe(&doc), before); + doc.get_text("text").insert(0, ">").unwrap(); + doc.commit(); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": [1], "text": ">draft"}) + ); +} + +#[test] +fn failed_apply_diff_on_a_shallow_doc() { + let source = LoroDoc::new(); + source.get_list("list").push(1).unwrap(); + source.get_text("text").insert(0, "abc").unwrap(); + source.commit(); + source.get_list("list").push(2).unwrap(); + source.commit(); + let doc = LoroDoc::new(); + doc.import( + &source + .export(ExportMode::shallow_snapshot(&source.oplog_frontiers())) + .unwrap(), + ) + .unwrap(); + let before = observe(&doc); + + let mut batch = DiffBatch::default(); + batch + .push( + root("text", ContainerType::Text), + Diff::Text(vec![ + TextDelta::Retain { + retain: 1, + attributes: None, + }, + TextDelta::Delete { delete: 1 }, + ]), + ) + .unwrap(); + batch + .push( + root("list", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 5 }]), + ) + .unwrap(); + assert!(doc.apply_diff(batch).is_err()); + doc.commit(); + assert_eq!(observe(&doc), before); + doc.get_text("text").insert(0, "x").unwrap(); + doc.commit(); + assert_eq!(doc.get_text("text").to_string(), "xabc"); +} + +// --------------------------------------------------------------------------- +// Random stale diffs: `doc.diff(a, b)` applied to a doc that is not at `a`. +// --------------------------------------------------------------------------- + +/// Sorted, so that the same random choices pick the same node in two docs +/// whose tree states were built in a different order. +fn alive_nodes(tree: &loro::LoroTree) -> Vec { + let mut alive: Vec<_> = tree + .nodes() + .into_iter() + .filter(|n| !tree.is_node_deleted(n).unwrap()) + .collect(); + alive.sort(); + alive +} + +fn random_edit(doc: &LoroDoc, rng: &mut StdRng) -> LoroResult<()> { + let text = doc.get_text("text"); + let list = doc.get_list("list"); + let map = doc.get_map("map"); + let mlist = doc.get_movable_list("mlist"); + let tree = doc.get_tree("tree"); + match rng.gen_range(0..14) { + 0 | 1 => { + let pos = rng.gen_range(0..=text.len_unicode()); + text.insert(pos, ["a", "bc", "def", "๐Ÿ™‚"][rng.gen_range(0..4)])?; + } + 2 => { + let len = text.len_unicode(); + if len > 0 { + let pos = rng.gen_range(0..len); + let n = rng.gen_range(1..=(len - pos).min(3)); + text.delete(pos, n)?; + } + } + 3 => { + let pos = rng.gen_range(0..=list.len()); + if rng.gen_bool(0.3) { + let child = list.insert_container(pos, LoroText::new())?; + child.insert(0, "c")?; + } else { + list.insert(pos, rng.gen_range(0..100))?; + } + } + 4 => { + if !list.is_empty() { + list.delete(rng.gen_range(0..list.len()), 1)?; + } + } + 5 => { + let key = ["a", "b", "c", "d"][rng.gen_range(0..4)]; + if rng.gen_bool(0.3) { + let child = map.insert_container(key, LoroMap::new())?; + child.insert("x", rng.gen_range(0..10))?; + } else if rng.gen_bool(0.2) { + map.delete(key)?; + } else { + map.insert(key, rng.gen_range(0..100))?; + } + } + 6 => { + let pos = rng.gen_range(0..=mlist.len()); + if rng.gen_bool(0.3) { + let child = mlist.insert_container(pos, LoroCounter::new())?; + child.increment(1.0)?; + } else { + mlist.insert(pos, rng.gen_range(0..100))?; + } + } + 7 => { + let len = mlist.len(); + if len > 1 { + mlist.mov(rng.gen_range(0..len), rng.gen_range(0..len))?; + } else if len == 1 && rng.gen_bool(0.5) { + mlist.delete(0, 1)?; + } + } + 8 => { + let len = mlist.len(); + if len > 0 { + mlist.set(rng.gen_range(0..len), rng.gen_range(0..100))?; + } + } + 9 => { + let alive = alive_nodes(&tree); + let parent = if alive.is_empty() || rng.gen_bool(0.3) { + TreeParentId::Root + } else { + TreeParentId::Node(alive[rng.gen_range(0..alive.len())]) + }; + let node = tree.create(parent)?; + tree.get_meta(node)?.insert("v", rng.gen_range(0..10))?; + } + 10 => { + let alive = alive_nodes(&tree); + if !alive.is_empty() { + let target = alive[rng.gen_range(0..alive.len())]; + if rng.gen_bool(0.4) { + tree.delete(target)?; + } else { + let parent = alive[rng.gen_range(0..alive.len())]; + // Cyclic moves are rejected; that is fine here. + let _ = tree.mov(target, parent); + } + } + } + 12 => { + // Mergeable children: deleting the key hides the child's state, and + // ensuring it again (here or through a diff) brings the state back. + let key = ["m1", "m2"][rng.gen_range(0..2)]; + match rng.gen_range(0..4) { + 0 => map.delete(key)?, + 1 => { + let t = map.ensure_mergeable_text(key)?; + t.insert(0, "t")?; + } + 2 => { + let m = map.ensure_mergeable_map(key)?; + m.insert("k", rng.gen_range(0..10))?; + } + _ => { + let c = map.ensure_mergeable_counter(key)?; + c.increment(1.0)?; + } + } + } + _ => { + // Integers: float sums depend on the order they are added in. + doc.get_counter("counter") + .increment(rng.gen_range(0..10) as f64)?; + } + } + Ok(()) +} + +/// Builds a doc with a random history of `n` commits from two peers and +/// returns it with the frontiers after each commit. +fn random_history(rng: &mut StdRng, n: usize) -> (LoroDoc, Vec) { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + let mut versions = vec![Frontiers::default()]; + for _ in 0..n { + let doc = if rng.gen_bool(0.5) { &a } else { &b }; + for _ in 0..rng.gen_range(1..4) { + random_edit(doc, rng).unwrap(); + } + doc.commit(); + if rng.gen_bool(0.3) { + a.import(&b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + } + versions.push(a.oplog_frontiers()); + } + a.import(&b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + versions.push(a.oplog_frontiers()); + (a, versions) +} + +fn seeds() -> std::ops::Range { + let n = std::env::var("LORO_APPLY_DIFF_ATOMICITY_SEEDS") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(120); + 0..n +} + +/// The doc's value after each of peer 3's ops. (The ops' storage may be split +/// differently: a rolled back batch keeps its values in the arena, so the next +/// op's values are not adjacent to the previous op's and don't merge with it.) +fn history_of_peer_3(doc: &LoroDoc) -> Vec { + let end = doc.oplog_vv().get(&3).copied().unwrap_or(0); + (0..end) + .map(|counter| { + let at = doc.fork_at(&loro::ID::new(3, counter).into()).unwrap(); + value_without_empty_roots(&at) + }) + .collect() +} + +/// A failed `apply_diff` is indistinguishable from not calling it: a twin that +/// makes the same edits without the call ends up with the same state and the +/// same history, op ids included, although the edits after the call reuse the +/// ids of the rolled back batch. +#[test] +fn stale_diffs_are_applied_whole_or_not_at_all() { + let mut failures = 0; + for seed in seeds() { + let mut rng = StdRng::seed_from_u64(seed); + let (source, versions) = random_history(&mut rng, 12); + let pick = |rng: &mut StdRng| versions[rng.gen_range(0..versions.len())].clone(); + let (va, vb, vc) = (pick(&mut rng), pick(&mut rng), pick(&mut rng)); + let diff = source.diff(&va, &vb).unwrap(); + let target = source.fork_at(&vc).unwrap(); + target.set_peer_id(3).unwrap(); + let twin = source.fork_at(&vc).unwrap(); + twin.set_peer_id(3).unwrap(); + // Uncommitted edits before the call must survive it. + let mut edits = StdRng::seed_from_u64(seed ^ 0xed17); + random_edit(&target, &mut edits.clone()).unwrap(); + random_edit(&twin, &mut edits).unwrap(); + + match target.apply_diff(diff) { + Ok(()) => { + target.commit(); + for _ in 0..3 { + random_edit(&target, &mut rng).unwrap(); + } + target.commit(); + assert_consistent_with_replay(&target); + } + Err(_) => { + failures += 1; + target.commit(); + twin.commit(); + assert_eq!(observe(&target), observe(&twin), "seed {seed}"); + for doc in [&target, &twin] { + let mut rng = edits.clone(); + for _ in 0..3 { + random_edit(doc, &mut rng).unwrap(); + } + // Creates many containers and tree nodes (or fails the same way). + let full = source.diff(&Frontiers::default(), &vb).unwrap(); + let _ = doc.apply_diff(full); + doc.commit(); + } + assert_eq!(observe(&target), observe(&twin), "seed {seed}"); + assert_eq!( + history_of_peer_3(&target), + history_of_peer_3(&twin), + "seed {seed}" + ); + assert_consistent_with_replay(&target); + } + } + } + // The random history must actually exercise the failure path. + assert!( + failures > seeds().end / 10, + "only {failures} failing batches" + ); +} + +/// The tree as a set of nodes with metadata: `revert_to` recreates deleted +/// nodes under new ids, and it does not always restore sibling order +/// (a separate issue). +fn without_tree_ids(mut value: Value) -> Value { + fn strip(v: &mut Value) { + match v { + Value::Object(o) => { + o.remove("id"); + o.remove("parent"); + o.remove("fractional_index"); + o.remove("index"); + o.values_mut().for_each(strip); + } + Value::Array(a) => { + a.iter_mut().for_each(strip); + a.sort_by_key(|v| v.to_string()); + } + _ => {} + } + } + if let Some(tree) = value.get_mut("tree") { + strip(tree); + } + value +} + +#[test] +fn revert_to_is_whole_or_not_at_all() { + for seed in seeds() { + let mut rng = StdRng::seed_from_u64(seed ^ 0x5eed); + let (doc, versions) = random_history(&mut rng, 10); + let target = versions[rng.gen_range(0..versions.len())].clone(); + let expected = without_tree_ids(value_without_empty_roots(&doc.fork_at(&target).unwrap())); + let before = observe(&doc); + match doc.revert_to(&target) { + Ok(()) => { + doc.commit(); + assert_eq!( + without_tree_ids(value_without_empty_roots(&doc)), + expected, + "seed {seed}" + ); + } + Err(_) => { + doc.commit(); + assert_eq!(observe(&doc), before, "seed {seed}"); + } + } + assert_consistent_with_replay(&doc); + } +} + +#[test] +fn failed_batch_is_retried_successfully() { + // The typical recovery: fix the batch and apply it again. + let doc = LoroDoc::new(); + let list = doc.get_list("list"); + list.push(1).unwrap(); + doc.commit(); + let mut batch = DiffBatch::default(); + batch + .push( + root("map", ContainerType::Map), + map_diff( + "child", + ValueOrContainer::Value(LoroValue::Container(ContainerID::new_normal( + loro::ID::new(99, 0), + ContainerType::Text, + ))), + ), + ) + .unwrap(); + batch + .push( + ContainerID::new_normal(loro::ID::new(99, 0), ContainerType::Text), + text_insert("child text"), + ) + .unwrap(); + let mut failing = batch.clone(); + failing + .push( + root("list", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 3 }]), + ) + .unwrap(); + assert!(doc.apply_diff(failing).is_err()); + doc.apply_diff(batch).unwrap(); + doc.commit(); + assert_eq!( + doc.get_deep_value().to_json_value(), + serde_json::json!({"list": [1], "map": {"child": "child text"}}) + ); + assert_consistent_with_replay(&doc); +} + +/// Edits another thread makes while a batch runs can land in the batch's +/// transaction, or commit it. Such a batch is not rolled back, so that those +/// edits are kept. +#[test] +fn edits_of_another_thread_during_a_failing_apply_diff_are_kept() { + let doc = LoroDoc::new(); + doc.get_list("list").push(1).unwrap(); + doc.commit(); + let n = 2000; + std::thread::scope(|s| { + s.spawn(|| { + for i in 0..n { + doc.get_list("b").push(i as i64).unwrap(); + if i % 7 == 0 { + doc.commit(); + } + } + doc.commit(); + }); + s.spawn(|| { + for _ in 0..n { + assert!(doc.apply_diff(batch_failing_at_the_end()).is_err()); + } + }); + }); + doc.commit(); + assert_eq!(doc.get_list("b").len(), n); + assert_eq!(doc.get_list("list").len(), 1); + assert_consistent_with_replay(&doc); +} + +/// Edits to containers that existed before the rolled back batch (movable-list +/// elements, tree nodes, style anchors and counters keyed by the op ids the +/// next edits reuse) give the same doc as on a twin that never made the call. +#[test] +fn edits_to_existing_containers_after_a_rollback_match_a_twin() { + let base = LoroDoc::new(); + base.set_peer_id(1).unwrap(); + let ml = base.get_movable_list("ml"); + ml.push("a").unwrap(); + ml.push("b").unwrap(); + let node = base.get_tree("tree").create(TreeParentId::Root).unwrap(); + base.get_tree("tree") + .get_meta(node) + .unwrap() + .insert("v", 1) + .unwrap(); + base.get_text("text").insert(0, "hello").unwrap(); + base.get_counter("c").increment(0.1).unwrap(); + base.commit(); + let snapshot = base.export(ExportMode::Snapshot).unwrap(); + + let source = base.fork(); + source.set_peer_id(2).unwrap(); + let ml = source.get_movable_list("ml"); + ml.insert(1, "x").unwrap(); + ml.mov(0, 2).unwrap(); + ml.set(0, "y").unwrap(); + let tree = source.get_tree("tree"); + let child = tree.create(node).unwrap(); + tree.get_meta(child).unwrap().insert("w", 2).unwrap(); + tree.mov(child, TreeParentId::Root).unwrap(); + let text = source.get_text("text"); + text.mark(0..3, "bold", true).unwrap(); + text.insert(1, "XY").unwrap(); + source.get_counter("c").increment(0.2).unwrap(); + source.commit(); + let diff = source + .diff(&base.oplog_frontiers(), &source.oplog_frontiers()) + .unwrap(); + + let open = |peer| { + let doc = LoroDoc::new(); + doc.import(&snapshot).unwrap(); + doc.set_peer_id(peer).unwrap(); + doc + }; + let target = open(3); + let twin = open(3); + + let mut failing = diff.clone(); + failing + .push( + root("zzz", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 1 }]), + ) + .unwrap(); + assert!(target.apply_diff(failing).is_err()); + target.commit(); + assert_eq!(observe(&target), observe(&twin)); + assert_eq!(target.get_counter("c").get_value(), 0.1); + + for doc in [&target, &twin] { + let ml = doc.get_movable_list("ml"); + ml.insert(0, "p").unwrap(); + ml.set(1, "q").unwrap(); + ml.mov(0, 2).unwrap(); + let tree = doc.get_tree("tree"); + let n = tree.create(node).unwrap(); + tree.get_meta(n).unwrap().insert("z", 3).unwrap(); + let text = doc.get_text("text"); + text.mark(1..4, "bold", true).unwrap(); + text.insert(2, "Z").unwrap(); + doc.get_counter("c").increment(0.2).unwrap(); + doc.commit(); + doc.apply_diff(diff.clone()).unwrap(); + doc.commit(); + } + assert_eq!(observe(&target), observe(&twin)); + assert_eq!( + target.get_text("text").get_richtext_value(), + twin.get_text("text").get_richtext_value() + ); + assert_eq!(history_of_peer_3(&target), history_of_peer_3(&twin)); + assert_consistent_with_replay(&target); +} + +/// Pending edits stay in the transaction, uncommitted, even when the batch's +/// first ops were merged into theirs (the same text insert run, the same +/// counter): the commit afterwards reports exactly the pending edits. +#[test] +fn pending_edits_merged_with_a_failed_batch_keep_their_ops_and_events() { + fn events_of(doc: &LoroDoc) -> Arc>> { + let events = Arc::new(std::sync::Mutex::new(Vec::new())); + let events_in_cb = events.clone(); + doc.subscribe_root(Arc::new(move |e| { + let diffs: Vec = e + .events + .iter() + .map(|d| format!("{:?} {:?}", d.target, d.diff)) + .collect(); + events_in_cb.lock().unwrap().push(format!("{:?}", diffs)); + })) + .detach(); + events + } + let open = || { + let doc = LoroDoc::new(); + doc.set_peer_id(3).unwrap(); + doc.get_list("list").push(1).unwrap(); + doc.get_text("text").insert(0, "xy").unwrap(); + doc.commit(); + doc + }; + let pending = |doc: &LoroDoc| { + doc.get_text("text").insert(2, "abc").unwrap(); + doc.get_counter("c").increment(1.0).unwrap(); + doc.get_list("list").push(2).unwrap(); + }; + let target = open(); + let twin = open(); + let target_events = events_of(&target); + let twin_events = events_of(&twin); + pending(&target); + pending(&twin); + let changes = target.len_changes(); + + let mut batch = DiffBatch::default(); + batch + .push( + root("text", ContainerType::Text), + Diff::Text(vec![ + TextDelta::Retain { + retain: 5, + attributes: None, + }, + TextDelta::Insert { + insert: "partial".into(), + attributes: None, + }, + ]), + ) + .unwrap(); + batch + .push(root("c", ContainerType::Counter), Diff::Counter(2.0)) + .unwrap(); + batch + .push( + root("list", ContainerType::List), + Diff::List(vec![ + ListDiffItem::Retain { retain: 2 }, + ListDiffItem::Insert { + insert: vec![ValueOrContainer::Value(3.into())], + is_move: false, + }, + ]), + ) + .unwrap(); + batch + .push( + root("zzz", ContainerType::List), + Diff::List(vec![ListDiffItem::Delete { delete: 1 }]), + ) + .unwrap(); + assert!(target.apply_diff(batch).is_err()); + // Still pending: nothing was committed or reported. + assert_eq!(target.len_changes(), changes); + assert!(target_events.lock().unwrap().is_empty()); + assert_eq!( + target.get_deep_value().to_json_value(), + twin.get_deep_value().to_json_value() + ); + + target.commit(); + twin.commit(); + assert_eq!(observe(&target), observe(&twin)); + assert_eq!(*target_events.lock().unwrap(), *twin_events.lock().unwrap()); + assert_eq!(target_events.lock().unwrap().len(), 1); + assert_eq!(history_of_peer_3(&target), history_of_peer_3(&twin)); + assert_consistent_with_replay(&target); +} From 0862c8dac89270973eb62a1b9120e018e357458d Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 30 Sep 2026 14:15:10 +0800 Subject: [PATCH 10/10] fix(loro-crdt): keep the known keys of a map event with an unknown child (#1151) A map event that set a key to a container of an unknown type (created by a newer loro-crdt) failed to convert as a whole, so edits to known keys in the same event never reached subscribers. Leave out only the unknown child's entry. List events holding an unknown child are still left out whole, and delivering the unknown child itself needs a public representation (an API decision); see context/unknown-containers.md. Co-Authored-By: Claude Opus 5.5 --- .changeset/unknown-child-map-event.md | 5 ++ context/unknown-containers.md | 15 ++++- crates/loro-wasm/src/convert.rs | 5 ++ .../tests/unknown_container_events.test.ts | 66 +++++++++++++++++++ 4 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 .changeset/unknown-child-map-event.md create mode 100644 crates/loro-wasm/tests/unknown_container_events.test.ts diff --git a/.changeset/unknown-child-map-event.md b/.changeset/unknown-child-map-event.md new file mode 100644 index 000000000..27ab7132a --- /dev/null +++ b/.changeset/unknown-child-map-event.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +A map event that sets a key to a container of a type unknown to this version (created by a newer `loro-crdt`) is now delivered with its other keys; only the unknown child's entry is left out. Previously the whole map event was dropped, so edits to known keys in the same event never reached subscribers. diff --git a/context/unknown-containers.md b/context/unknown-containers.md index 60d954685..59e06932f 100644 --- a/context/unknown-containers.md +++ b/context/unknown-containers.md @@ -118,9 +118,18 @@ the rejected steps. ## Events and `diff()` (loro-crdt) `diff_event_to_js_value` (`crates/loro-wasm/src/lib.rs`) leaves out the event of -an unknown container, and of a container whose diff holds an unknown child that -can't be turned into a JS value (logged with `console.error`, #1151). The other -events of the batch are still delivered. +an unknown container. A map event that sets a key to an unknown child leaves out +only that entry (`map_delta_to_js` in `convert.rs`) and keeps the other keys; in +JSON form (`for_json`) the child is written as its container id like any other. +A list or movable-list event that inserts an unknown child is still left out +whole (logged with `console.error`), because dropping one item would shift the +indices of the rest. The other events of the batch are always delivered +(#1142, #1151). + +There is no JS value for an unknown container: handlers throw, and `toJSON` shows +it as `null`. Delivering the unknown child itself (the left-out map entry or list +item) needs a public representation, e.g. `null` as in `toJSON`, its +`๐Ÿฆœ:cid:...` string, or a wrapper class. That is an API decision left open. `LoroDoc.diff()` leaves out the entries of unknown containers. Applying one is a no-op in this version, also in a full-state batch (the final clear of an diff --git a/crates/loro-wasm/src/convert.rs b/crates/loro-wasm/src/convert.rs index 5b7e341b7..a7fde7a63 100644 --- a/crates/loro-wasm/src/convert.rs +++ b/crates/loro-wasm/src/convert.rs @@ -434,6 +434,11 @@ fn map_delta_to_js(value: &ResolvedMapDelta, for_json: bool) -> JsResult convert(v)?, + // A container of a type unknown to this version (created by a newer + // loro-crdt) has no JS handler. Leave its entry out and keep the other + // keys of the event, which used to be dropped with it (#1151). In JSON + // form it is written as its container id, like any other child. + ValueOrHandler::Handler(Handler::Unknown(_)) if !for_json => continue, ValueOrHandler::Handler(h) => handler_to_js_value(h, for_json)?, } } else { diff --git a/crates/loro-wasm/tests/unknown_container_events.test.ts b/crates/loro-wasm/tests/unknown_container_events.test.ts new file mode 100644 index 000000000..c9ef4f15b --- /dev/null +++ b/crates/loro-wasm/tests/unknown_container_events.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from "vitest"; +import { LoroCounter, LoroDoc, LoroEventBatch, LoroMap } from "../bundler/index"; + +// Events that touch a container of a type unknown to this version (created by +// a newer loro-crdt). #1142 stopped one such event from dropping the whole +// batch. A map event that sets a key to an unknown container used to be left +// out as a whole, losing the known keys it also updated. See loro-dev/loro#1151. + +/** Rewrites every Counter in `src`'s JSON updates into an unknown type. */ +function asNewerVersion(src: LoroDoc) { + return JSON.parse( + JSON.stringify(src.exportJsonUpdates()) + .replace(/(cid:\d+@\d+):Counter/g, "$1:Unknown(9)") + .split('"type":"counter"') + .join('"type":"unknown"'), + ); +} + +describe("events next to unknown containers", () => { + it("delivers the known keys of a map event that also sets an unknown child", async () => { + const src = new LoroDoc(); + src.setPeerId(1); + const m = src.getMap("m"); + m.set("title", "x"); + m.setContainer("u", new LoroCounter()).increment(1); + m.set("n", 2); + src.commit(); + + const doc = new LoroDoc(); + const batches: LoroEventBatch[] = []; + doc.subscribe((e) => batches.push(e)); + const containerEvents: LoroEventBatch[] = []; + doc.getMap("m").subscribe((e) => containerEvents.push(e)); + doc.importJsonUpdates(asNewerVersion(src)); + await Promise.resolve(); + + for (const batch of [batches, containerEvents]) { + expect(batch.length).toBe(1); + const mapEvent = batch[0].events.find((e) => e.target === "cid:root-m:Map"); + expect(mapEvent?.diff).toStrictEqual({ + type: "map", + // The unknown child `u` has no JS value, so its entry is left out. + updated: { title: "x", n: 2 }, + }); + } + expect(doc.toJSON()).toStrictEqual({ m: { title: "x", u: null, n: 2 } }); + }); + + it("a nested map event with an unknown child keeps its known keys", async () => { + const src = new LoroDoc(); + src.setPeerId(1); + const inner = src.getMap("m").setContainer("inner", new LoroMap()); + inner.set("k", 1); + inner.setContainer("u", new LoroCounter()).increment(1); + src.commit(); + + const doc = new LoroDoc(); + const batches: LoroEventBatch[] = []; + doc.subscribe((e) => batches.push(e)); + doc.importJsonUpdates(asNewerVersion(src)); + await Promise.resolve(); + + const innerEvent = batches[0].events.find((e) => e.path.length === 2); + expect(innerEvent?.diff).toStrictEqual({ type: "map", updated: { k: 1 } }); + }); +});