From f7a9a02a752944f40b412c8f47cac7f58e2a34ba Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 09:54:36 +0800 Subject: [PATCH 1/3] fix: take the tree checkout window from the two versions The tree diff calculator retreated and replayed ops from the change-start lamport of `find_replay_base`. In Checkout mode that base can be a meet that is not a critical version, and an op concurrent with it can have a lower lamport, so the op was skipped: a checkout to a concurrent version kept a node it does not have, and with a subscriber a node's child was created without the node (panic at tree_state.rs:1075). Both cache transitions now start at the smallest lamport that differs between the two versions. Rationale in context/tree-checkout-window.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/tree-checkout-window.md | 5 + AGENTS.md | 2 + context/tree-checkout-window.md | 68 +++ crates/fuzz/tests/checkout_path.rs | 242 ++++++++++ crates/loro-internal/AGENTS.md | 13 +- .../docs/critical-version-spec.md | 32 +- crates/loro-internal/src/dag.rs | 1 + crates/loro-internal/src/diff_calc/tree.rs | 434 ++++++++---------- crates/loro-internal/src/history_cache.rs | 8 - crates/loro-internal/src/oplog.rs | 4 - .../tests/tree_checkout_path.test.ts | 99 ++++ crates/loro/tests/issue.rs | 13 +- crates/loro/tests/tree_checkout_path.rs | 126 +++++ 13 files changed, 775 insertions(+), 272 deletions(-) create mode 100644 .changeset/tree-checkout-window.md create mode 100644 context/tree-checkout-window.md create mode 100644 crates/fuzz/tests/checkout_path.rs create mode 100644 crates/loro-wasm/tests/tree_checkout_path.test.ts create mode 100644 crates/loro/tests/tree_checkout_path.rs diff --git a/.changeset/tree-checkout-window.md b/.changeset/tree-checkout-window.md new file mode 100644 index 000000000..bbf0a4b9c --- /dev/null +++ b/.changeset/tree-checkout-window.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix tree checkouts that depended on the path taken. Checking out from a version to a concurrent one could keep a tree node the target version does not have, or, with a subscriber, panic with `get_index_by_tree_id(..).unwrap()` in `tree_state.rs` when the diff created a node's child without the node. The tree diff now retreats and replays from the smallest lamport that differs between the two versions instead of from a replay base, which could be a non-critical meet that skipped earlier concurrent ops. Tree checkouts also no longer search the DAG for a replay base. diff --git a/AGENTS.md b/AGENTS.md index d1c2488f8..4051322d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,6 +29,8 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. [context/shallow-snapshot-style-redaction.md](context/shallow-snapshot-style-redaction.md). - Rich-text insert positions next to style anchors, the insert cursor cache, 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). - User-facing Loro usage, sync, editor integration, and performance guidance: [skills/loro/SKILL.md](skills/loro/SKILL.md). - Pure TypeScript runtime indexes, complexity contracts, benchmarks, and remaining gaps: diff --git a/context/tree-checkout-window.md b/context/tree-checkout-window.md new file mode 100644 index 000000000..5a5afee70 --- /dev/null +++ b/context/tree-checkout-window.md @@ -0,0 +1,68 @@ +# Tree Checkout Window + +Verified against code 2026-09-28. + +`TreeDiffCalculator` (`crates/loro-internal/src/diff_calc/tree.rs`) computes +tree diffs for checkouts and for imports in `DiffMode::Import`. It keeps one +`TreeCacheForDiff` per tree in the history cache (`TreeOpGroup` in +`history_cache.rs`): the tree's move ops of `current_vv`, grouped by target. + +## Invariant + +Each cached op carries `effected`: whether the cycle check let it apply +(`TreeCacheForDiff::apply`). That flag depends on every op ordered before it +by (lamport, peer), so the cache must always hold ops applied in that order. A +node's parent is its last effected op (`get_parent_with_id`). + +## Transitions + +`checkout` (moves the cache to `from` without diffs) and `checkout_diff` +(records `TreeDeltaItem`s from `from` to `to`) use the same window: + +1. `min_lamport_of_version_diff(a, b)`: the smallest lamport among ops that are + in exactly one of the two versions. Lamports grow along a peer's ops, so it + reads only the first op of each per-peer span of the version-vector + difference, O(peers). +2. Retreat every cached op at or above it, newest first. `checkout_diff` emits + the inverse of each effected op. +3. Apply every op of the target version at or above it, oldest first. + +Ops below the window are in both versions and stay as they are. Both passes +walk `TreeOpGroup::ops` (ordered by `IdLp`), which also holds the seeded +shallow-root entries, so they never scan nodes that did not change. + +## Why not the replay base + +Until 2026-09-28 the window started at the change-start lamport of +`find_replay_base(from, to)`. In `Checkout` mode that base is the meet even +when the meet is not a critical version (`MeetAsBase::Valid`), and an op +concurrent with the meet can have a lower lamport than the meet. Example +(`crates/loro/tests/tree_checkout_path.rs`): `0@3` creates a node at lamport 1 +and is concurrent with `2@2` (lamport 2); checking out from `[3@2]` to +`[1@1]` used the meet `[2@2]`, the window started at 2, and the creation was +never retreated. In the other direction the forward pass skipped a node's +creation but applied its child's, and `TreeState::apply_diff_and_convert` +panicked on the child's `Create` (with a subscriber; without one the state was +wrong). `docs/critical-version-spec.md` Q7 had argued the window was safe. + +The cache's own `checkout` had the same problem with a window taken from the +target frontiers. + +## Shallow documents + +The cache of a shallow document starts at the shallow root, seeded with the +root's nodes (`record_shallow_root_state`). `TreeCacheForDiff::init_current_vv` +sets `current_vv` to the root version before the first transition, so the +window never reaches trimmed ops. The seeding itself only covers the seeded op +IDs, which is why `current_vv` has to be replaced. + +## Tests + +- `crates/loro/tests/tree_checkout_path.rs`: the two repros. +- `crates/fuzz/tests/checkout_path.rs`: random concurrent edits on all + container kinds, every ordered pair of versions (including heads no peer + had), compared with a document that imported only that version's changes, + with and without a subscriber (events are mirrored). Wider sweeps: + `CHECKOUT_PATH_SEEDS=0..300 cargo test --release -p fuzz --test checkout_path`. +- `crates/loro/tests/issue.rs`: `checkout_across_non_critical_meet_stays_canonical` + and `checkout_with_low_lamport_concurrent_branch_stays_canonical`. diff --git a/crates/fuzz/tests/checkout_path.rs b/crates/fuzz/tests/checkout_path.rs new file mode 100644 index 000000000..bdec8de2b --- /dev/null +++ b/crates/fuzz/tests/checkout_path.rs @@ -0,0 +1,242 @@ +//! Checkouts must not depend on the path taken. +//! +//! Several peers make concurrent edits (tree create/move/delete, moves into +//! and out of deleted subtrees, metadata edits, plus map/list/movable +//! list/text/counter edits) and sync at random. Then one document walks +//! through every ordered pair `(a, b)` of the collected versions: +//! `checkout(a)`, then `checkout(b)`. After each checkout its value must equal +//! a fresh document that imported only the changes of that version, and the +//! value rebuilt from its events must equal its state. The versions include +//! heads that no peer ever had, so the transitions cross concurrent branches. +//! +//! The seeds are fixed; `CHECKOUT_PATH_SEEDS=start..end` runs others. The +//! `loro.js` differential fuzz found the tree cases (loro-dev/loro#1141): the +//! other fuzzers only check out versions from one peer's own history, which +//! are ordered by inclusion. See `context/tree-checkout-window.md`. + +use fuzz::actor::{assert_value_eq, Actor}; +use loro::{ + ExportMode, Frontiers, LoroDoc, LoroMap, LoroText, LoroTree, LoroValue, TreeID, TreeParentId, + ID, +}; +use rand::{rngs::StdRng, seq::SliceRandom, Rng, SeedableRng}; + +const PEERS: u64 = 3; +const ACTIONS: usize = 40; +const MAX_VERSIONS: usize = 24; + +fn random_parent(tree: &LoroTree, rng: &mut StdRng) -> TreeParentId { + let nodes = tree.nodes(); + if nodes.is_empty() || rng.gen_bool(0.3) { + TreeParentId::Root + } else { + TreeParentId::Node(*nodes.choose(rng).unwrap()) + } +} + +fn random_node(tree: &LoroTree, rng: &mut StdRng) -> Option { + tree.nodes().choose(rng).copied() +} + +fn random_edit(doc: &LoroDoc, rng: &mut StdRng) { + let tree = doc.get_tree("tree"); + let value = rng.gen_range(0..100); + // Errors are expected: moving a node below itself, editing a deleted + // node's metadata, out-of-range indexes after a sync, and so on. + let _ = match rng.gen_range(0..16) { + 0..=2 => { + let parent = random_parent(&tree, rng); + tree.create(parent).map(|_| ()) + } + 3..=5 => match random_node(&tree, rng) { + // Includes moves into deleted subtrees and moves that revive + // nodes under deleted ancestors. + Some(target) => { + let parent = random_parent(&tree, rng); + tree.mov(target, parent) + } + None => Ok(()), + }, + 6 => match random_node(&tree, rng) { + Some(target) => tree.delete(target), + None => Ok(()), + }, + 7..=8 => match random_node(&tree, rng) { + Some(target) => tree + .get_meta(target) + .and_then(|meta| meta.insert(&format!("k{}", value % 3), value)), + None => Ok(()), + }, + 9 => match random_node(&tree, rng) { + Some(target) => tree.get_meta(target).and_then(|meta| { + meta.insert_container("text", LoroText::new())? + .insert(0, "t") + }), + None => Ok(()), + }, + 10 => { + let map = doc.get_map("map"); + if rng.gen_bool(0.3) { + map.delete(&format!("k{}", value % 3)) + } else if rng.gen_bool(0.3) { + map.insert_container(&format!("k{}", value % 3), LoroMap::new()) + .and_then(|m| m.insert("v", value)) + } else { + map.insert(&format!("k{}", value % 3), value) + } + } + 11 => { + let list = doc.get_list("list"); + if !list.is_empty() && rng.gen_bool(0.4) { + list.delete(rng.gen_range(0..list.len()), 1) + } else { + list.insert(rng.gen_range(0..=list.len()), value) + } + } + 12 => { + let list = doc.get_movable_list("movable"); + let len = list.len(); + match rng.gen_range(0..4) { + 0 if len > 1 => list.mov(rng.gen_range(0..len), rng.gen_range(0..len)), + 1 if len > 0 => list.delete(rng.gen_range(0..len), 1), + 2 if len > 0 => list.set(rng.gen_range(0..len), value), + 3 => list + .insert_container(rng.gen_range(0..=len), LoroMap::new()) + .and_then(|m| m.insert("v", value)), + _ => list.insert(rng.gen_range(0..=len), value), + } + } + 13 => { + let text = doc.get_text("text"); + let len = text.len_unicode(); + if len > 0 && rng.gen_bool(0.3) { + let pos = rng.gen_range(0..len); + text.delete(pos, 1) + } else { + text.insert(rng.gen_range(0..=len), "ab") + } + } + 14 => doc.get_counter("counter").increment(value as f64), + _ => { + // A burst of local edits gives this peer higher lamports than the + // others, so concurrent branches end up with very different + // lamport ranges. + (0..3).try_for_each(|_| { + doc.get_map("burst").insert("k", value)?; + doc.commit(); + Ok(()) + }) + } + }; + doc.commit(); +} + +fn random_version(doc: &LoroDoc, rng: &mut StdRng) -> Frontiers { + let vv = doc.oplog_vv(); + let mut ids = Vec::new(); + for _ in 0..rng.gen_range(1..=2) { + let peers: Vec<_> = vv.iter().filter(|(_, end)| **end > 0).collect(); + let Some((peer, end)) = peers.choose(rng) else { + break; + }; + ids.push(ID::new(**peer, rng.gen_range(0..**end))); + } + let vv = doc.frontiers_to_vv(&Frontiers::from(ids)).unwrap(); + doc.vv_to_frontiers(&vv) +} + +fn value_at(all: &LoroDoc, version: &Frontiers) -> LoroValue { + let vv = all.frontiers_to_vv(version).unwrap(); + let doc = LoroDoc::new(); + doc.set_hide_empty_root_containers(true); + doc.import(&all.export(ExportMode::updates_till(&vv)).unwrap()) + .unwrap(); + doc.get_deep_value() +} + +fn check_checkout(doc: &LoroDoc, version: &Frontiers, expected: &LoroValue, context: &str) { + doc.checkout(version).unwrap(); + assert_value_eq( + expected, + &doc.get_deep_value(), + Some(&mut || format!("{context}, checkout to {version:?}")), + ); +} + +fn run(seed: u64) { + let mut rng = StdRng::seed_from_u64(seed); + let peers: Vec = (1..=PEERS) + .map(|peer| { + let doc = LoroDoc::new(); + doc.set_peer_id(peer).unwrap(); + doc + }) + .collect(); + for _ in 0..ACTIONS { + let i = rng.gen_range(0..peers.len()); + if rng.gen_bool(0.25) { + let j = rng.gen_range(0..peers.len()); + if i != j { + let update = peers[j] + .export(ExportMode::updates(&peers[i].oplog_vv())) + .unwrap(); + peers[i].import(&update).unwrap(); + } + } else { + random_edit(&peers[i], &mut rng); + } + } + + let all = LoroDoc::new(); + for peer in peers.iter() { + all.import(&peer.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + } + let mut versions = vec![Frontiers::default(), all.oplog_frontiers()]; + while versions.len() < MAX_VERSIONS { + let version = random_version(&all, &mut rng); + if !versions.contains(&version) { + versions.push(version); + } + } + let expected: Vec = versions.iter().map(|v| value_at(&all, v)).collect(); + + // `Actor` mirrors the document from its events. + let with_events = Actor::new(100); + let plain = LoroDoc::new(); + let updates = all.export(ExportMode::all_updates()).unwrap(); + for doc in [with_events.loro.as_ref(), &plain] { + doc.set_hide_empty_root_containers(true); + doc.import(&updates).unwrap(); + } + with_events.check_tracker(); + for (a, b) in (0..versions.len()).flat_map(|a| (0..versions.len()).map(move |b| (a, b))) { + for (i, context) in [(a, "from"), (b, "to")] { + let context = format!( + "seed {seed}, {context} of {:?} -> {:?}", + versions[a], versions[b] + ); + check_checkout(&with_events.loro, &versions[i], &expected[i], &context); + with_events.check_tracker(); + check_checkout(&plain, &versions[i], &expected[i], &context); + } + } +} + +#[test] +fn checkout_between_any_two_versions_is_path_independent() { + let seeds: Vec = match std::env::var("CHECKOUT_PATH_SEEDS") { + Ok(range) => { + let (start, end) = range + .split_once("..") + .expect("CHECKOUT_PATH_SEEDS=start..end"); + (start.parse().unwrap()..end.parse().unwrap()).collect() + } + Err(_) => (0..24).collect(), + }; + let failed: Vec = seeds + .into_iter() + .filter(|&seed| std::panic::catch_unwind(|| run(seed)).is_err()) + .collect(); + assert!(failed.is_empty(), "failed seeds: {failed:?}"); +} diff --git a/crates/loro-internal/AGENTS.md b/crates/loro-internal/AGENTS.md index 48a49132a..6498d1324 100644 --- a/crates/loro-internal/AGENTS.md +++ b/crates/loro-internal/AGENTS.md @@ -29,7 +29,9 @@ over graceful degradation. `src/handler/movable_list_apply_delta.rs` applies movable-list diffs for `apply_diff` and undo: [../../context/movable-list-apply-diff.md](../../context/movable-list-apply-diff.md). -- `src/diff_calc/`: diff calculation when moving between versions. +- `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). - `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). @@ -65,16 +67,17 @@ coverage under `crates/fuzz` and ask before running long fuzz targets. - Replay-base selection uses Eg-walker terminology (arXiv:2409.14252 §3.5): a version V is **critical** when every event outside `Events(V)` happened after all of `Events(V)` — no concurrency crosses the cut. Non-`Checkout` - diff modes and the tree calculator's lamport windows are only sound when - the base satisfies this; `dag.rs`/`oplog.rs` enforce it via the + diff modes are only sound when the base satisfies this (the tree + calculator's `Checkout` path takes its window from the two versions + instead); `dag.rs`/`oplog.rs` enforce it via the `ImportGreaterUpdates` entry check and, on conservative retreat, the multi-head fixpoint `OpLog::latest_critical_version_below_meet`, with the `latest_single_head_critical_version` descent as fallback. Do not use "LCA" or "common ancestor" in new code or docs: the meet of two versions is generally NOT a safe replay base, and a caller that needs a critical version must ask for one (loro-dev/loro#1095). Read - `docs/critical-version-spec.md` before touching `find_replay_base`, diff - modes, or `diff_calc/tree.rs` windows. + `docs/critical-version-spec.md` before touching `find_replay_base` or diff + modes. `OpLog::iter_from_replay_base_causally` may bypass the DAG when the concurrency is register-only (`docs/diff_calc.md`, "Register-only concurrency"); keep that check container-granular and history-based. diff --git a/crates/loro-internal/docs/critical-version-spec.md b/crates/loro-internal/docs/critical-version-spec.md index 65d559307..d4014bf95 100644 --- a/crates/loro-internal/docs/critical-version-spec.md +++ b/crates/loro-internal/docs/critical-version-spec.md @@ -459,7 +459,7 @@ ancestry(B) 中的事件并发。Eg-walker 的理论最优是"发生在双方之 text/list 依赖下游守卫兜底——本类不受 2026-09 变更影响:L13 只在 第 2 类(保守回退)触发,本类的 meet 基准永远不会被认证为 critical,守卫(base == from ∧ ¬source_not_in_op_context)仍是 - 唯一保障。Tree 的 Checkout 路径见 Q7(已核查安全)。 + 唯一保障。Tree 的 Checkout 路径不再依赖基准,见 Q7。 因此严格的安全陈述是**双层契约**:「(ans, mode) + 下游守卫」合起来 保证收敛;单看走查,无条件承诺的只有 S1/S2/S3/T。用论文语言可以把 @@ -632,20 +632,22 @@ criss-cross ladder 测试钉"tips 去重后复杂度线性"(性能声明,不 Tree 不成立,故不再把守卫当作 IGU 正确性的依据:入场检查(L12)使 S3/S4 直接成立。守卫仍是 text/list 在 Checkout 模式下的兜底,其公理 化留给 Q7。 -- **Q7**(已核查,2026-08-01:安全)Checkout 模式下 meet 非 critical 时 - Tree 的 checkout_diff 是否一致?机制审计发现 Tree 的 Checkout 路径是 - **相对**计算(retreat/forward 都按 meet 前沿的 change 起点 lamport 开窗, - 窗外 op 静默跳过)——窗口隐含 critical 假设。但可证明修复后恒安全: - 区域内经会合进入公共历史的 op 必 ≥ 某 meet 头(在窗口内);与 meet 头 - 并发的 op 必产生 uncovered 死路 → L11 扫描 → critical 基准 → 窗口从 - 基准起点覆盖全区域。实证:非 critical meet 菱形(tree/movable list/ - text)与低 lamport 并发分支两组 checkout 探针全部 canonical,已钉为 - 回归测试 `checkout_across_non_critical_meet_stays_canonical` 与 - `checkout_with_low_lamport_concurrent_branch_stays_canonical` - (后者显式保护"扫描 ↔ Tree 窗口"的耦合:削弱扫描会静默破坏它)。 - 2026-09 注:oplog 的重放基准可能比 Tree 自算的窗口基准**更晚** - (L13 多头 ≥ L11 单头),重放区域只会更小、仍被窗口覆盖;反向 - (把多头基准喂给 Tree 窗口)需要重做本证明,故未做。 +- **Q7**(2026-08-01 判为安全;2026-09-28 推翻并改掉)Checkout 模式下 + meet 非 critical 时 Tree 的 checkout_diff 是否一致?原机制按 meet 前沿 + 的 change 起点 lamport 开窗,窗外 op 静默跳过。原论证称"与 meet 头并发 + 的 op 必产生 uncovered 死路 → L11 扫描 → critical 基准",但这不成立: + 并发 op 的依赖可以先走进 meet 之下的公共历史而会合,不留死路。反例 + (`crates/loro/tests/tree_checkout_path.rs`):`0@3`(lamport 1,建树 + 节点,依赖 `0@1`)与 meet 头 `2@2`(lamport 2)并发;从 `[3@2]` 切到 + `[1@1]` 时 meet = `[2@2]`、无死路,窗口从 2 起,`0@3` 永不被撤回;反方向 + 则前进时漏掉父节点的创建而应用子节点的创建,带订阅者时在 + `TreeState::apply_diff_and_convert` panic。修法:Tree 不再用重放基准, + 而取 from/to 两个版本对称差中最小的 lamport 作窗口(每个 peer 的差段 + 只看首个 op,O(peers)),撤回窗口内全部缓存 op,再按 (lamport, peer) + 顺序应用 to 的窗口内 op;窗口下的 op 两侧相同。此后 Tree 的 Checkout + 路径与基准选择无关,本问题关闭。详见 + `context/tree-checkout-window.md`;随机回归 + `crates/fuzz/tests/checkout_path.rs` 覆盖全部版本对。 - **Q10**(新,2026-09)L13 的搜索只在 ≤ meet 的范围内进行,长期离线 分叉后的每次导入仍付出 O(分歧量)(分岔点确为最晚 critical version,无状态的基准选择无法更好)。是否值得为这一形状缓存 diff --git a/crates/loro-internal/src/dag.rs b/crates/loro-internal/src/dag.rs index 3dc98f50b..2405991a3 100644 --- a/crates/loro-internal/src/dag.rs +++ b/crates/loro-internal/src/dag.rs @@ -83,6 +83,7 @@ pub(crate) trait DagUtils: Dag { /// that need every other op to be before or after the result, such as the /// shallow snapshot root, must use /// [`Self::latest_single_head_critical_version`] (loro-dev/loro#1095). + #[allow(unused)] fn find_replay_base(&self, a_id: &Frontiers, b_id: &Frontiers) -> (Frontiers, DiffMode); /// Like [`Self::find_replay_base`], but leaves the conservative /// fallback to the caller: on [`MeetAsBase::NeedsCriticalRetreat`] the diff --git a/crates/loro-internal/src/diff_calc/tree.rs b/crates/loro-internal/src/diff_calc/tree.rs index b89fbf93d..08a5a3955 100644 --- a/crates/loro-internal/src/diff_calc/tree.rs +++ b/crates/loro-internal/src/diff_calc/tree.rs @@ -1,13 +1,11 @@ use std::{collections::BTreeSet, sync::Arc}; use fractional_index::FractionalIndex; -use itertools::Itertools; use loro_common::{ContainerID, IdFull, IdLp, Lamport, PeerID, TreeID, ID}; use rustc_hash::FxHashMap; use crate::{ container::{idx::ContainerIdx, tree::tree_op::TreeOp}, - dag::DagUtils, delta::{TreeDelta, TreeDeltaItem, TreeInternalDiff}, event::InternalDiff, state::TreeParentId, @@ -160,69 +158,44 @@ impl TreeDiffCalculator { self.checkout_diff(info, oplog) } + /// Moves the diff cache to `to` without recording diffs. fn checkout(&mut self, to: &VersionVector, to_frontiers: &Frontiers, oplog: &OpLog) { oplog.with_history_cache(|h| { let mark = h.ensure_importing_caches_exist(); let tree_ops = h.get_tree(&self.container, mark).unwrap(); let mut tree_cache = tree_ops.tree().lock(); + tree_cache.init_current_vv(oplog); let s = format!("checkout current {:?} to {:?}", &tree_cache.current_vv, &to); let s = tracing::span!(tracing::Level::INFO, "checkout", s = s); let _e = s.enter(); if to == &tree_cache.current_vv { return; } - let min_lamport = self.get_min_lamport_by_frontiers(to_frontiers, oplog); - // retreat - let mut retreat_ops = vec![]; - for (_target, ops) in tree_cache.tree.iter() { - for op in ops.iter().rev() { - if op.id.lamport < min_lamport { - break; - } - if !to.includes_id(op.id.id()) { - retreat_ops.push(op.clone()); - } - } - } + let Some(min_lamport) = min_lamport_of_version_diff(&tree_cache.current_vv, to, oplog) + else { + tree_cache.current_vv = to.clone(); + return; + }; - for op in retreat_ops { - tree_cache.retreat_op(&op); + let ops = tree_ops.ops(); + for (idlp, op) in ops.range(IdLp::new(0, min_lamport)..) { + tree_cache.take(IdFull::new(idlp.peer, op.counter, idlp.lamport), &op.value); } - // forward and apply - let max_lamport = self.get_max_lamport_by_frontiers(to_frontiers, oplog); - let mut forward_ops = vec![]; - let group = h - .get_importing_cache(&self.container, mark) - .unwrap() - .as_tree() - .unwrap(); - for (idlp, op) in group.ops().range( - IdLp { - lamport: 0, - peer: 0, - }..=IdLp { - lamport: max_lamport, - peer: PeerID::MAX, - }, - ) { - if !tree_cache - .current_vv - .includes_id(ID::new(idlp.peer, op.counter)) - && to.includes_id(ID::new(idlp.peer, op.counter)) - { - forward_ops.push((IdFull::new(idlp.peer, op.counter, idlp.lamport), op)); + let max_lamport = self + .get_max_lamport_by_frontiers(to_frontiers, oplog) + .max(min_lamport); + for (idlp, op) in + ops.range(IdLp::new(0, min_lamport)..=IdLp::new(PeerID::MAX, max_lamport)) + { + if to.includes_id(ID::new(idlp.peer, op.counter)) { + tree_cache.apply(MoveLamportAndID { + id: IdFull::new(idlp.peer, op.counter, idlp.lamport), + op: op.value.clone(), + effected: false, + }); } } - - for (id, op) in forward_ops { - let op = MoveLamportAndID { - id, - op: op.value.clone(), - effected: false, - }; - tree_cache.apply(op); - } tree_cache.current_vv = to.clone(); }); } @@ -232,190 +205,125 @@ impl TreeDiffCalculator { let mark = h.ensure_importing_caches_exist(); let tree_ops = h.get_tree(&self.container, mark).unwrap(); let mut tree_cache = tree_ops.tree().lock(); + debug_assert_eq!(&tree_cache.current_vv, info.from_vv); let mut parent_to_children_cache = TreeParentToChildrenCache::init_from_tree_cache(&tree_cache); let s = tracing::span!(tracing::Level::INFO, "checkout_diff"); let _e = s.enter(); - let to_frontiers = info.to_frontiers; - let from_frontiers = info.from_frontiers; - let (replay_base, _mode) = oplog.dag.find_replay_base(from_frontiers, to_frontiers); - let base_vv = oplog.dag.frontiers_to_vv(&replay_base).unwrap(); - let base_frontiers = replay_base; - let to_max_lamport = self.get_max_lamport_by_frontiers(to_frontiers, oplog); - // CORRECTNESS: the retreat/forward passes below only look at ops - // with lamport >= `base_min_lamport` (the minimum change-start - // lamport of the base frontiers). Ops outside the window are - // silently skipped, which is only sound if no op of the diff - // region sits below the window. That holds because of how - // `find_replay_base` picks the base: - // - a region op that reaches the base through causal edges is a - // descendant of some base head, so its lamport is strictly - // greater than that head's change-start lamport, which is >= the - // window minimum; - // - a region op CONCURRENT with a base head forces the walk to - // detect an uncovered branch and retreat the base to a critical - // version (latest_single_head_critical_version), below which no - // concurrency crosses — the window then starts at that base. - // See docs/critical-version-spec.md (Q7) and the regression tests - // `checkout_across_non_critical_meet_stays_canonical` and - // `checkout_with_low_lamport_concurrent_branch_stays_canonical` - // in crates/loro/tests/issue.rs. Weakening either the critical - // version fallback or the ImportGreaterUpdates entry check in - // dag.rs breaks this invariant. - // - // The oplog replay base that feeds the calculators may be *later* - // than this window's base: `OpLog::latest_critical_version_below_meet` - // can find a multi-head critical version above the single-head one - // recomputed here. That only shrinks the replayed region relative - // to the window, which stays sound; feeding this window the - // multi-head base instead would require redoing the Q7 proof. - // (The oplog's register-only - // bypass never reaches this path: it only replays from `from` - // when no tree has ops on both sides of the concurrency, and - // such trees take the ImportGreaterUpdates branch instead.) - let base_min_lamport = self.get_min_lamport_by_frontiers(&base_frontiers, oplog); - - // retreat for diff - let mut diffs = vec![]; + // CORRECTNESS: `effected` (the cycle check) of each cached op + // depends on every op ordered before it, so the cache must always + // hold ops applied in (lamport, peer) order. Ops below + // `min_lamport` are the same in `from` and `to`; everything at or + // above it is retreated from `from` and replayed for `to` in that + // order. The window is taken from the versions themselves, not + // from a replay base: in `Checkout` mode `find_replay_base` may + // return a meet that is not a critical version, and ops concurrent + // with that meet can have lower lamports than it. See + // `context/tree-checkout-window.md`. + let Some(min_lamport) = min_lamport_of_version_diff(info.from_vv, info.to_vv, oplog) + else { + tree_cache.current_vv = info.to_vv.clone(); + return TreeDelta::default(); + }; + // `max` keeps the forward range well-formed when `to` only drops ops. + let to_max_lamport = self + .get_max_lamport_by_frontiers(info.to_frontiers, oplog) + .max(min_lamport); + let ops = tree_ops.ops(); - if !(tree_cache.current_vv == base_vv && &base_vv == info.from_vv) { - let mut retreat_ops = vec![]; - for (_target, ops) in tree_cache.tree.iter() { - for op in ops.iter().rev() { - if op.id.lamport < base_min_lamport { - break; - } - if !base_vv.includes_id(op.id.id()) { - retreat_ops.push(op.clone()); - } - } + let mut diffs = vec![]; + // retreat, newest first + for (idlp, op) in ops.range(IdLp::new(0, min_lamport)..).rev() { + let Some(op) = + tree_cache.take(IdFull::new(idlp.peer, op.counter, idlp.lamport), &op.value) + else { + continue; + }; + if !op.effected { + continue; } - - for op in retreat_ops.into_iter().sorted().rev() { - tree_cache.retreat_op(&op); - let (old_parent, position, last_effective_move_op_id) = - tree_cache.get_parent_with_id(op.op.target()); - if op.effected { - // we need to know whether old_parent is deleted - let is_parent_deleted = tree_cache.is_parent_deleted(op.op.parent_id()); - let is_old_parent_deleted = tree_cache.is_parent_deleted(old_parent); - if op.op.target().id() == op.id.id() { - assert_eq!( - old_parent, - TreeParentId::Unexist, - "old_parent = {:?} instead", - &old_parent - ); - } - parent_to_children_cache.record_change( - op.op.target(), - op.op.parent_id(), - old_parent, - ); - let this_diff = TreeDeltaItem::new( - op.op.target(), - old_parent, - op.op.parent_id(), - last_effective_move_op_id, - is_old_parent_deleted, - is_parent_deleted, - position, - ); - let is_create = matches!(this_diff.action, TreeInternalDiff::Create { .. }); - diffs.push(this_diff); - if is_create { - let mut s = vec![op.op.target()]; - while let Some(t) = s.pop() { - let children = tree_cache.get_children_with_id( - TreeParentId::Node(t), - &parent_to_children_cache, - ); - children.iter().for_each(|c| { - diffs.push(TreeDeltaItem { - target: c.0, - action: TreeInternalDiff::Create { - parent: TreeParentId::Node(t), - position: c.1.clone().unwrap(), - }, - last_effective_move_op_id: c.2, - }) - }); - s.extend(children.iter().map(|c| c.0)); - } - } - } + let (old_parent, position, last_effective_move_op_id) = + tree_cache.get_parent_with_id(op.op.target()); + // we need to know whether old_parent is deleted + let is_parent_deleted = tree_cache.is_parent_deleted(op.op.parent_id()); + let is_old_parent_deleted = tree_cache.is_parent_deleted(old_parent); + if op.op.target().id() == op.id.id() { + assert_eq!( + old_parent, + TreeParentId::Unexist, + "old_parent = {:?} instead", + &old_parent + ); + } + parent_to_children_cache.record_change( + op.op.target(), + op.op.parent_id(), + old_parent, + ); + let this_diff = TreeDeltaItem::new( + op.op.target(), + old_parent, + op.op.parent_id(), + last_effective_move_op_id, + is_old_parent_deleted, + is_parent_deleted, + position, + ); + let is_create = matches!(this_diff.action, TreeInternalDiff::Create { .. }); + diffs.push(this_diff); + if is_create { + tree_cache.push_children_creation( + op.op.target(), + &parent_to_children_cache, + &mut diffs, + ); } } - tree_cache.current_vv = base_vv; - // forward - let group = h - .get_importing_cache(&self.container, mark) - .unwrap() - .as_tree() - .unwrap(); - for (idlp, op) in group.ops().range( - IdLp { - lamport: base_min_lamport, - peer: 0, - }..=IdLp { - lamport: to_max_lamport, - peer: PeerID::MAX, - }, - ) { + + // forward, oldest first + for (idlp, op) in + ops.range(IdLp::new(0, min_lamport)..=IdLp::new(PeerID::MAX, to_max_lamport)) + { let id = ID::new(idlp.peer, op.counter); - if !tree_cache.current_vv.includes_id(id) && info.to_vv.includes_id(id) { - let op = MoveLamportAndID { - id: IdFull { - peer: id.peer, - lamport: idlp.lamport, - counter: id.counter, - }, - op: op.value.clone(), - effected: false, - }; - let (old_parent, _position, _id) = - tree_cache.get_parent_with_id(op.op.target()); - let is_parent_deleted = tree_cache.is_parent_deleted(op.op.parent_id()); - let is_old_parent_deleted = tree_cache.is_parent_deleted(old_parent); - let effected = tree_cache.apply(op.clone()); - if effected { - let this_diff = TreeDeltaItem::new( - op.op.target(), - op.op.parent_id(), - old_parent, - op.id_full(), - is_parent_deleted, - is_old_parent_deleted, - op.op.fractional_index(), - ); - parent_to_children_cache.record_change( + if !info.to_vv.includes_id(id) { + continue; + } + let op = MoveLamportAndID { + id: IdFull { + peer: id.peer, + lamport: idlp.lamport, + counter: id.counter, + }, + op: op.value.clone(), + effected: false, + }; + let (old_parent, _position, _id) = tree_cache.get_parent_with_id(op.op.target()); + let is_parent_deleted = tree_cache.is_parent_deleted(op.op.parent_id()); + let is_old_parent_deleted = tree_cache.is_parent_deleted(old_parent); + let effected = tree_cache.apply(op.clone()); + if effected { + let this_diff = TreeDeltaItem::new( + op.op.target(), + op.op.parent_id(), + old_parent, + op.id_full(), + is_parent_deleted, + is_old_parent_deleted, + op.op.fractional_index(), + ); + parent_to_children_cache.record_change( + op.op.target(), + old_parent, + op.op.parent_id(), + ); + let is_create = matches!(this_diff.action, TreeInternalDiff::Create { .. }); + diffs.push(this_diff); + if is_create { + tree_cache.push_children_creation( op.op.target(), - old_parent, - op.op.parent_id(), + &parent_to_children_cache, + &mut diffs, ); - let is_create = matches!(this_diff.action, TreeInternalDiff::Create { .. }); - diffs.push(this_diff); - if is_create { - // TODO: per - let mut s = vec![op.op.target()]; - while let Some(t) = s.pop() { - let children = tree_cache.get_children_with_id( - TreeParentId::Node(t), - &parent_to_children_cache, - ); - children.iter().for_each(|c| { - diffs.push(TreeDeltaItem { - target: c.0, - action: TreeInternalDiff::Create { - parent: TreeParentId::Node(t), - position: c.1.clone().unwrap(), - }, - last_effective_move_op_id: c.2, - }) - }); - s.extend(children.iter().map(|x| x.0)); - } - } } } } @@ -425,14 +333,6 @@ impl TreeDiffCalculator { }) } - fn get_min_lamport_by_frontiers(&self, frontiers: &Frontiers, oplog: &OpLog) -> Lamport { - frontiers - .iter() - .map(|id| oplog.get_min_lamport_at(id)) - .min() - .unwrap_or(0) - } - fn get_max_lamport_by_frontiers(&self, frontiers: &Frontiers, oplog: &OpLog) -> Lamport { frontiers .iter() @@ -442,6 +342,28 @@ impl TreeDiffCalculator { } } +/// The smallest lamport among the ops that are in exactly one of `a` and `b`, +/// or `None` when both contain the same ops. +/// +/// Every tree op below it is in both versions, so moving the diff cache +/// between them only has to retreat and replay the ops at or above it. +fn min_lamport_of_version_diff( + a: &VersionVector, + b: &VersionVector, + oplog: &OpLog, +) -> Option { + a.sub_iter(b) + .chain(b.sub_iter(a)) + .map(|span| { + let id = ID::new(span.peer, span.counter.start); + oplog + .dag + .get_lamport(&id) + .unwrap_or_else(|| panic!("op {id} of a tree diff version is not in the DAG")) + }) + .min() +} + /// All information of an operation for diff calculating of movable tree. #[derive(Debug, Clone)] pub struct MoveLamportAndID { @@ -485,6 +407,10 @@ impl Ord for MoveLamportAndID { pub(crate) struct TreeCacheForDiff { tree: FxHashMap>, current_vv: VersionVector, + /// `false` until the first transition. In a shallow doc the cache starts + /// at the shallow root (seeded with its state, or empty when the tree had + /// no nodes there), which `current_vv` does not express yet. + current_vv_initialized: bool, } impl std::fmt::Debug for TreeCacheForDiff { @@ -499,9 +425,51 @@ impl std::fmt::Debug for TreeCacheForDiff { } impl TreeCacheForDiff { - fn retreat_op(&mut self, op: &MoveLamportAndID) { - self.tree.get_mut(&op.op.target()).unwrap().remove(op); - self.current_vv.set_end(op.id.id()); + /// Removes the op from the cache if it is there, returning it with its + /// `effected` flag. + fn take(&mut self, id: IdFull, op: &Arc) -> Option { + self.tree.get_mut(&op.target())?.take(&MoveLamportAndID { + id, + op: op.clone(), + effected: false, + }) + } + + fn init_current_vv(&mut self, oplog: &OpLog) { + if self.current_vv_initialized { + return; + } + + self.current_vv_initialized = true; + if oplog.is_shallow() { + self.current_vv = oplog + .dag + .frontiers_to_vv(oplog.dag.shallow_since_frontiers()) + .expect("the shallow root version must be in the DAG"); + } + } + + /// After `target` is (re)created, pushes a `Create` for each node of its + /// current subtree. + fn push_children_creation( + &self, + target: TreeID, + cache: &TreeParentToChildrenCache, + diffs: &mut Vec, + ) { + let mut s = vec![target]; + while let Some(t) = s.pop() { + let children = self.get_children_with_id(TreeParentId::Node(t), cache); + diffs.extend(children.iter().map(|c| TreeDeltaItem { + target: c.0, + action: TreeInternalDiff::Create { + parent: TreeParentId::Node(t), + position: c.1.clone().unwrap(), + }, + last_effective_move_op_id: c.2, + })); + s.extend(children.iter().map(|c| c.0)); + } } fn is_ancestor_of(&self, maybe_ancestor: &TreeID, node_id: &TreeParentId) -> bool { diff --git a/crates/loro-internal/src/history_cache.rs b/crates/loro-internal/src/history_cache.rs index 6769e7d5f..c0cad3ed6 100644 --- a/crates/loro-internal/src/history_cache.rs +++ b/crates/loro-internal/src/history_cache.rs @@ -289,14 +289,6 @@ impl ContainerHistoryCache { } } - pub(crate) fn get_importing_cache( - &self, - container_idx: &ContainerIdx, - _: HasImportingCacheMark, - ) -> Option<&HistoryCacheForImporting> { - self.for_importing.as_ref().unwrap().get(container_idx) - } - pub(crate) fn get_tree( &self, container_idx: &ContainerIdx, diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index cf3de3afd..68e888c0d 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -538,10 +538,6 @@ impl OpLog { self.dag.cmp_frontiers(a, b) } - pub(crate) fn get_min_lamport_at(&self, id: ID) -> Lamport { - self.get_change_at(id).map(|c| c.lamport).unwrap_or(0) - } - pub(crate) fn get_lamport_at(&self, id: ID) -> Option { self.get_change_at(id) .map(|c| c.lamport + (id.counter - c.id.counter) as Lamport) diff --git a/crates/loro-wasm/tests/tree_checkout_path.test.ts b/crates/loro-wasm/tests/tree_checkout_path.test.ts new file mode 100644 index 000000000..3c0e6d84c --- /dev/null +++ b/crates/loro-wasm/tests/tree_checkout_path.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// Repros from the loro.js differential fuzz (loro-dev/loro#1141). See +// context/tree-checkout-window.md. + +function docWithPeer(peer: bigint): LoroDoc { + const doc = new LoroDoc(); + doc.setPeerId(peer); + return doc; +} + +function merged(docs: LoroDoc[]): LoroDoc { + const doc = new LoroDoc(); + for (const d of docs) doc.import(d.export({ mode: "update" })); + return doc; +} + +describe("tree checkout path", () => { + it("does not panic on a checkout sequence with a subscriber", () => { + const p1 = docWithPeer(1n); + p1.getList("l").insert(0, "x"); + const a = p1.getTree("tr").createNode(); + const b = p1.getTree("tr").createNode(a.id); + p1.commit(); + + const p3 = docWithPeer(3n); + p3.import( + p1.export({ + mode: "updates-in-range", + spans: [{ id: { peer: "1", counter: 0 }, len: 1 }], + }), + ); + p3.getList("l").insert(0, "y"); + p3.commit(); + + const p2 = docWithPeer(2n); + p2.import(p3.export({ mode: "update" })); + p2.getCounter("c").increment(1); + p2.commit(); + + p3.import(p2.export({ mode: "update" })); + p3.getMap("m").set("k", 1); + p3.commit(); + + p2.import(p1.export({ mode: "update" })); + p2.getTree("tr").getNodeByID(b.id)!.data.set("k", true); + p2.commit(); + + const doc = merged([p1, p2, p3]); + let events = 0; + doc.subscribe(() => { + events += 1; + }); + for (const version of [ + [{ peer: "3" as const, counter: 1 }], + [{ peer: "2" as const, counter: 1 }], + ]) { + doc.checkout(version); + const fresh = merged([p1, p2, p3]); + fresh.checkout(version); + expect(doc.toJSON()).toStrictEqual(fresh.toJSON()); + } + expect(events).toBeGreaterThan(0); + }); + + it("removes a node when checking out a concurrent version without its creation", () => { + const [p1, p2, p3] = [1n, 2n, 3n].map(docWithPeer); + p1.getCounter("c").increment(1); + p1.commit(); // 0@1 + p3.import(p1.export({ mode: "update" })); + p3.getTree("tr").createNode(); + p3.commit(); // 0@3, deps 0@1 + p2.getList("l").insert(0, 1); + p2.getList("l").delete(0, 1); + p2.commit(); // 0@2..1@2 + p2.import(p1.export({ mode: "update" })); + p2.getCounter("c").increment(1); + p2.commit(); // 2@2, deps [0@1, 1@2] + p1.import(p2.export({ mode: "update" })); + p1.getMap("m").set("k", 1); + p1.commit(); // 1@1, deps 2@2 + p2.import(p3.export({ mode: "update" })); + p2.getText("t").insert(0, "x"); + p2.commit(); // 3@2, deps [2@2, 0@3] + + const target = [{ peer: "1" as const, counter: 1 }]; + const direct = merged([p1, p2, p3]); + direct.checkout(target); + expect(direct.getTree("tr").toJSON()).toStrictEqual([]); + + const doc = merged([p1, p2, p3]); + doc.checkout([{ peer: "2", counter: 3 }]); + expect(doc.getTree("tr").toJSON().length).toBe(1); + doc.checkout(target); + expect(doc.getTree("tr").toJSON()).toStrictEqual([]); + expect(doc.toJSON()).toStrictEqual(direct.toJSON()); + }); +}); diff --git a/crates/loro/tests/issue.rs b/crates/loro/tests/issue.rs index 3cfb9a152..4068f1dbb 100644 --- a/crates/loro/tests/issue.rs +++ b/crates/loro/tests/issue.rs @@ -586,8 +586,8 @@ fn tree_concurrent_moves_incremental_state_matches_replay() { /// Checkout between divergent versions whose meet is NOT a critical version /// (diamond: the region op `1@1` is concurrent with the meet head `0@2`). -/// The tree/movable-list/text calculators replay relatively around the base; -/// this pins that such checkouts stay canonical. +/// The movable-list/text calculators replay relatively around the base; this +/// pins that such checkouts stay canonical. #[test] fn checkout_across_non_critical_meet_stays_canonical() { use loro::{Frontiers, TreeID, ID}; @@ -645,11 +645,10 @@ fn checkout_across_non_critical_meet_stays_canonical() { } /// Checkout where the diff region contains a LOW-lamport concurrent branch -/// (below the meet frontier's lamport window). The tree calculator's -/// retreat/forward windows skip ops below `lca_min_lamport`, so this is only -/// safe because the walk retreats the base to a critical version (the -/// latest-singleton-cut sweep) whenever such a branch exists. Pins that -/// interplay: weakening the sweep would silently break this. +/// (below the lamport of the meet frontier). The tree calculator takes its +/// retreat/forward window from the two versions, not from the replay base +/// (`context/tree-checkout-window.md`); this pins that the branch is still +/// forwarded and retreated. #[test] fn checkout_with_low_lamport_concurrent_branch_stays_canonical() { use loro::{Frontiers, TreeID, ID}; diff --git a/crates/loro/tests/tree_checkout_path.rs b/crates/loro/tests/tree_checkout_path.rs new file mode 100644 index 000000000..17edeb0fe --- /dev/null +++ b/crates/loro/tests/tree_checkout_path.rs @@ -0,0 +1,126 @@ +//! Tree checkouts must not depend on the path taken. Minimal repros found by +//! the `loro.js` differential fuzz (loro-dev/loro#1141). See +//! `context/tree-checkout-window.md`. + +use loro::{ExportMode, Frontiers, IdSpan, LoroDoc, TreeParentId, ID}; + +fn doc_with_peer(peer: u64) -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(peer).unwrap(); + doc +} + +fn merged(docs: &[&LoroDoc]) -> LoroDoc { + let doc = LoroDoc::new(); + for d in docs { + doc.import(&d.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + } + doc +} + +fn tree_node_ids(doc: &LoroDoc, name: &str) -> Vec { + let tree = doc.get_tree(name); + let mut ids: Vec = tree + .nodes() + .into_iter() + .filter(|id| !tree.is_node_deleted(id).unwrap()) + .map(|id| id.to_string()) + .collect(); + ids.sort(); + ids +} + +/// With a subscriber, `checkout([3@1])` then `checkout([2@1])` panicked in +/// `TreeState::apply_diff_and_convert` (`get_index_by_tree_id(..).unwrap()` on +/// a `Create`). +#[test] +fn checkout_sequence_with_subscriber_does_not_panic() { + let p1 = doc_with_peer(1); + p1.get_list("l").insert(0, "x").unwrap(); + let tree = p1.get_tree("tr"); + let a = tree.create(TreeParentId::Root).unwrap(); + let b = tree.create(a).unwrap(); + p1.commit(); + + let p3 = doc_with_peer(3); + p3.import( + &p1.export(ExportMode::updates_in_range(vec![IdSpan::new(1, 0, 1)])) + .unwrap(), + ) + .unwrap(); + p3.get_list("l").insert(0, "y").unwrap(); + p3.commit(); + + let p2 = doc_with_peer(2); + p2.import(&p3.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p2.get_counter("c").increment(1.0).unwrap(); + p2.commit(); + + p3.import(&p2.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p3.get_map("m").insert("k", 1).unwrap(); + p3.commit(); + + p2.import(&p1.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p2.get_tree("tr") + .get_meta(b) + .unwrap() + .insert("k", true) + .unwrap(); + p2.commit(); + + let doc = merged(&[&p1, &p2, &p3]); + let _sub = doc.subscribe_root(std::sync::Arc::new(|_| {})); + for f in [ID::new(3, 1), ID::new(2, 1)] { + let f = Frontiers::from_id(f); + doc.checkout(&f).unwrap(); + let fresh = merged(&[&p1, &p2, &p3]); + fresh.checkout(&f).unwrap(); + assert_eq!(doc.get_deep_value(), fresh.get_deep_value(), "at {f:?}"); + } +} + +/// Checking out `[3@2]` (which contains the creation of a tree node) and then +/// the concurrent `[1@1]` (which does not) kept the node. +#[test] +fn checkout_to_concurrent_version_without_node_creation_removes_node() { + let p1 = doc_with_peer(1); + let p2 = doc_with_peer(2); + let p3 = doc_with_peer(3); + p1.get_counter("c").increment(1.0).unwrap(); + p1.commit(); // 0@1 + p3.import(&p1.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p3.get_tree("tr").create(TreeParentId::Root).unwrap(); + p3.commit(); // 0@3, deps 0@1 + p2.get_list("l").insert(0, 1).unwrap(); + p2.get_list("l").delete(0, 1).unwrap(); + p2.commit(); // 0@2..1@2 + p2.import(&p1.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p2.get_counter("c").increment(1.0).unwrap(); + p2.commit(); // 2@2, deps [0@1, 1@2] + p1.import(&p2.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p1.get_map("m").insert("k", 1).unwrap(); + p1.commit(); // 1@1, deps 2@2 + p2.import(&p3.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + p2.get_text("t").insert(0, "x").unwrap(); + p2.commit(); // 3@2, deps [2@2, 0@3] + + let target = Frontiers::from_id(ID::new(1, 1)); + let direct = merged(&[&p1, &p2, &p3]); + direct.checkout(&target).unwrap(); + assert!(tree_node_ids(&direct, "tr").is_empty()); + + let doc = merged(&[&p1, &p2, &p3]); + doc.checkout(&Frontiers::from_id(ID::new(2, 3))).unwrap(); + assert_eq!(tree_node_ids(&doc, "tr").len(), 1); + doc.checkout(&target).unwrap(); + assert_eq!(tree_node_ids(&doc, "tr"), tree_node_ids(&direct, "tr")); + assert_eq!(doc.get_deep_value(), direct.get_deep_value()); +} From 4d1c6d43fb60245ec0ba3507701785a8d8de5f56 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 18:25:34 +0800 Subject: [PATCH 2/3] fix: bound the tree retreat by the cached version's max lamport The retreat walked every tree op in the history at or above the window and tried to take each from the cache, so a checkout between two old versions scaled with all later history (about 3.8 ms per checkout at 300k later moves, 0.1 ms on main). `TreeCacheForDiff::max_lamport` now bounds it: no cached op has a greater lamport. `find_replay_base` is only compiled for the DAG tests now. Co-Authored-By: Claude Opus 5.5 (1M context) --- context/tree-checkout-window.md | 11 +++ crates/loro-internal/src/dag.rs | 8 +- crates/loro-internal/src/diff_calc/tree.rs | 79 ++++++++++++++++++- .../tests/tree_checkout_path.test.ts | 41 ++++++++++ .../tests/perf_tree_checkout_old_versions.rs | 74 +++++++++++++++++ 5 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 crates/loro/tests/perf_tree_checkout_old_versions.rs diff --git a/context/tree-checkout-window.md b/context/tree-checkout-window.md index 5a5afee70..b117d54f1 100644 --- a/context/tree-checkout-window.md +++ b/context/tree-checkout-window.md @@ -31,6 +31,14 @@ Ops below the window are in both versions and stay as they are. Both passes walk `TreeOpGroup::ops` (ordered by `IdLp`), which also holds the seeded shallow-root entries, so they never scan nodes that did not change. +The retreat pass ends at `TreeCacheForDiff::max_lamport`: no cached op has a +greater lamport (`apply` and the shallow-root seeding raise it, +`retreated_from` lowers it to just below the window). The group holds the +whole history, so without that bound a checkout between two old versions +walked every later tree op and grew linearly with the history (about 3.8 ms +per checkout at 300k later moves). The forward pass ends at the target's max +lamport. + ## Why not the replay base Until 2026-09-28 the window started at the change-start lamport of @@ -59,6 +67,9 @@ IDs, which is why `current_vv` has to be replaced. ## Tests - `crates/loro/tests/tree_checkout_path.rs`: the two repros. +- `retreat_bound_follows_the_cached_version` (unit test in `tree.rs`) and the + ignored `crates/loro/tests/perf_tree_checkout_old_versions.rs` (run with + `--release -- --ignored`): the retreat bound. - `crates/fuzz/tests/checkout_path.rs`: random concurrent edits on all container kinds, every ordered pair of versions (including heads no peer had), compared with a document that imported only that version's changes, diff --git a/crates/loro-internal/src/dag.rs b/crates/loro-internal/src/dag.rs index 2405991a3..cf4538560 100644 --- a/crates/loro-internal/src/dag.rs +++ b/crates/loro-internal/src/dag.rs @@ -83,9 +83,12 @@ pub(crate) trait DagUtils: Dag { /// that need every other op to be before or after the result, such as the /// shallow snapshot root, must use /// [`Self::latest_single_head_critical_version`] (loro-dev/loro#1095). - #[allow(unused)] + /// + /// Only the DAG tests call it: the tree calculator used to, and now takes its + /// window from the two versions (`context/tree-checkout-window.md`). + #[cfg(test)] fn find_replay_base(&self, a_id: &Frontiers, b_id: &Frontiers) -> (Frontiers, DiffMode); - /// Like [`Self::find_replay_base`], but leaves the conservative + /// Like `find_replay_base`, but leaves the conservative /// fallback to the caller: on [`MeetAsBase::NeedsCriticalRetreat`] the /// caller must retreat to a critical version (via /// [`Self::latest_single_head_critical_version`], or a cheaper multi-head @@ -117,6 +120,7 @@ pub(crate) trait DagUtils: Dag { } impl DagUtils for T { + #[cfg(test)] #[inline] fn find_replay_base(&self, a_id: &Frontiers, b_id: &Frontiers) -> (Frontiers, DiffMode) { // TODO: perf: make it also return the spans to reach the replay base diff --git a/crates/loro-internal/src/diff_calc/tree.rs b/crates/loro-internal/src/diff_calc/tree.rs index 08a5a3955..7543e5b4e 100644 --- a/crates/loro-internal/src/diff_calc/tree.rs +++ b/crates/loro-internal/src/diff_calc/tree.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeSet, sync::Arc}; +use std::{collections::BTreeSet, ops::RangeInclusive, sync::Arc}; use fractional_index::FractionalIndex; use loro_common::{ContainerID, IdFull, IdLp, Lamport, PeerID, TreeID, ID}; @@ -178,9 +178,11 @@ impl TreeDiffCalculator { }; let ops = tree_ops.ops(); - for (idlp, op) in ops.range(IdLp::new(0, min_lamport)..) { + let retreat = tree_cache.retreat_range(min_lamport); + for (idlp, op) in retreat.map(|r| ops.range(r)).into_iter().flatten() { tree_cache.take(IdFull::new(idlp.peer, op.counter, idlp.lamport), &op.value); } + tree_cache.retreated_from(min_lamport); let max_lamport = self .get_max_lamport_by_frontiers(to_frontiers, oplog) @@ -233,7 +235,8 @@ impl TreeDiffCalculator { let mut diffs = vec![]; // retreat, newest first - for (idlp, op) in ops.range(IdLp::new(0, min_lamport)..).rev() { + let retreat = tree_cache.retreat_range(min_lamport); + for (idlp, op) in retreat.map(|r| ops.range(r)).into_iter().flatten().rev() { let Some(op) = tree_cache.take(IdFull::new(idlp.peer, op.counter, idlp.lamport), &op.value) else { @@ -280,6 +283,8 @@ impl TreeDiffCalculator { } } + tree_cache.retreated_from(min_lamport); + // forward, oldest first for (idlp, op) in ops.range(IdLp::new(0, min_lamport)..=IdLp::new(PeerID::MAX, to_max_lamport)) @@ -411,6 +416,10 @@ pub(crate) struct TreeCacheForDiff { /// at the shallow root (seeded with its state, or empty when the tree had /// no nodes there), which `current_vv` does not express yet. current_vv_initialized: bool, + /// No cached op has a greater lamport. Bounds the retreat so it does not + /// walk the ops above the cached version, which can be most of the + /// history when checking out between two old versions. + max_lamport: Lamport, } impl std::fmt::Debug for TreeCacheForDiff { @@ -435,6 +444,19 @@ impl TreeCacheForDiff { }) } + /// The `IdLp` range holding every cached op whose lamport is at least + /// `min_lamport`, or `None` when there is none. + fn retreat_range(&self, min_lamport: Lamport) -> Option> { + (self.max_lamport >= min_lamport) + .then(|| IdLp::new(0, min_lamport)..=IdLp::new(PeerID::MAX, self.max_lamport)) + } + + /// Records that every cached op whose lamport is at least `min_lamport` + /// has been retreated. + fn retreated_from(&mut self, min_lamport: Lamport) { + self.max_lamport = self.max_lamport.min(min_lamport.saturating_sub(1)); + } + fn init_current_vv(&mut self, oplog: &OpLog) { if self.current_vv_initialized { return; @@ -500,6 +522,7 @@ impl TreeCacheForDiff { effected = false; } node.effected = effected; + self.max_lamport = self.max_lamport.max(node.id.lamport); self.current_vv.set_last(node.id.id()); self.tree.entry(node.op.target()).or_default().insert(node); effected @@ -512,6 +535,7 @@ impl TreeCacheForDiff { debug_assert!(self.tree.is_empty()); for node in nodes.into_iter() { + self.max_lamport = self.max_lamport.max(node.id.lamport); self.current_vv.extend_to_include_last_id(node.id.id()); self.current_vv .extend_to_include_last_id(node.op.target().id()); @@ -650,3 +674,52 @@ impl TreeParentToChildrenCache { self.cache.entry(new_parent).or_default().insert(target); } } + +#[cfg(test)] +mod test { + use loro_common::{TreeID, ID}; + + use crate::{state::TreeParentId, version::Frontiers, HandlerTrait, LoroDoc}; + + /// Checking out between two old versions must retreat only what the cache + /// holds: `max_lamport` bounds every cached op, and stays at the cached + /// version instead of the end of the history. + #[test] + fn retreat_bound_follows_the_cached_version() { + let doc = LoroDoc::new_auto_commit(); + doc.set_peer_id(1).unwrap(); + let tree = doc.get_tree("tree"); + let nodes: Vec = (0..20) + .map(|_| tree.create(TreeParentId::Root).unwrap()) + .collect(); + doc.commit_then_renew(); + let mut versions = vec![]; + for i in 0..400 { + let _ = tree.mov(nodes[i % 20], TreeParentId::Node(nodes[(i * 7 + 3) % 20])); + doc.commit_then_renew(); + versions.push(doc.oplog_frontiers()); + } + + let (v1, v2) = (&versions[10], &versions[20]); + let lamport_of = |f: &Frontiers| { + let id: ID = f.as_single().unwrap(); + doc.oplog().lock().dag.get_lamport(&id).unwrap() + }; + for v in [v1, v2, v1, v2] { + doc.checkout(v).unwrap(); + let (cached_max, bound) = doc.oplog().lock().with_history_cache(|h| { + let mark = h.ensure_importing_caches_exist(); + let cache = h.get_tree(&tree.idx(), mark).unwrap().tree().lock(); + let cached_max = cache + .tree + .values() + .flat_map(|ops| ops.iter().map(|op| op.id.lamport)) + .max() + .unwrap(); + (cached_max, cache.max_lamport) + }); + assert!(cached_max <= bound); + assert!(bound <= lamport_of(v)); + } + } +} diff --git a/crates/loro-wasm/tests/tree_checkout_path.test.ts b/crates/loro-wasm/tests/tree_checkout_path.test.ts index 3c0e6d84c..9c81fb082 100644 --- a/crates/loro-wasm/tests/tree_checkout_path.test.ts +++ b/crates/loro-wasm/tests/tree_checkout_path.test.ts @@ -97,3 +97,44 @@ describe("tree checkout path", () => { expect(doc.toJSON()).toStrictEqual(direct.toJSON()); }); }); + +describe("tree checkout between old versions", () => { + it("matches a direct checkout after scrubbing back and forth", () => { + const doc = docWithPeer(1n); + const tree = doc.getTree("tree"); + const nodes = Array.from({ length: 30 }, () => tree.createNode().id); + doc.commit(); + let seed = 7; + const next = (n: number) => { + seed = (seed * 1103515245 + 12345) % 2147483648; + return seed % n; + }; + for (let i = 0; i < 1200; i++) { + try { + tree.move(nodes[next(nodes.length)], nodes[next(nodes.length)]); + } catch { + // moving a node under its own descendant is rejected + } + if (i % 20 === 19) doc.commit(); + } + doc.commit(); + + const versions = [ + [{ peer: "1" as const, counter: 200 }], + [{ peer: "1" as const, counter: 260 }], + [{ peer: "1" as const, counter: 900 }], + ]; + const expected = versions.map((v) => { + const fresh = new LoroDoc(); + fresh.import(doc.export({ mode: "update" })); + fresh.checkout(v); + return fresh.getTree("tree").toJSON(); + }); + for (let round = 0; round < 3; round++) { + for (const [i, v] of versions.entries()) { + doc.checkout(v); + expect(doc.getTree("tree").toJSON()).toStrictEqual(expected[i]); + } + } + }); +}); diff --git a/crates/loro/tests/perf_tree_checkout_old_versions.rs b/crates/loro/tests/perf_tree_checkout_old_versions.rs new file mode 100644 index 000000000..ae48452c8 --- /dev/null +++ b/crates/loro/tests/perf_tree_checkout_old_versions.rs @@ -0,0 +1,74 @@ +use loro::{Frontiers, LoroDoc, TreeID, TreeParentId, ID}; +use std::time::{Duration, Instant}; + +/// A tree with 2k nodes and `moves` random moves by one peer, committed every +/// 500 moves. +fn tree_history(moves: usize) -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let tree = doc.get_tree("tree"); + let mut nodes: Vec = Vec::new(); + let mut seed = 1u64; + let mut next = move |n: usize| { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + (seed >> 33) as usize % n + }; + for i in 0..2_000 { + let parent = if i < 20 { + TreeParentId::Root + } else { + TreeParentId::Node(nodes[next(nodes.len())]) + }; + nodes.push(tree.create(parent).unwrap()); + } + doc.commit(); + for i in 0..moves { + let target = nodes[next(nodes.len())]; + let parent = nodes[next(nodes.len())]; + let _ = tree.mov(target, parent); + if i % 500 == 499 { + doc.commit(); + } + } + doc.commit(); + doc +} + +/// Checking out back and forth between two old versions, 40 checkouts. +fn scrub_old_versions(doc: &LoroDoc) -> Duration { + let a = Frontiers::from_id(ID::new(1, 2_500)); + let b = Frontiers::from_id(ID::new(1, 2_510)); + doc.checkout(&a).unwrap(); + doc.checkout(&b).unwrap(); + let start = Instant::now(); + for _ in 0..20 { + doc.checkout(&a).unwrap(); + doc.checkout(&b).unwrap(); + } + start.elapsed() +} + +/// The tree diff retreats only the ops the cached version holds, so checking +/// out between two old versions must not get slower as later history grows +/// (it scanned every later tree op before `TreeCacheForDiff::max_lamport`). +/// See `context/tree-checkout-window.md`. +#[test] +#[ignore] +fn perf_tree_checkout_between_old_versions_does_not_scale_with_later_history() { + // Run with: + // cargo test --release -p loro --test perf_tree_checkout_old_versions -- --ignored --nocapture + let short = tree_history(10_000); + let long = tree_history(200_000); + let (mut t_short, mut t_long) = (Duration::MAX, Duration::MAX); + for _ in 0..3 { + t_short = t_short.min(scrub_old_versions(&short)); + t_long = t_long.min(scrub_old_versions(&long)); + } + println!("10k later moves: {t_short:?}, 200k later moves: {t_long:?}"); + assert!( + t_long < t_short * 4 + Duration::from_millis(5), + "checkout between old versions scales with later history: {t_short:?} -> {t_long:?}" + ); +} From 2645b68f7e0214c0404b1791671ea239752476aa Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 18:38:11 +0800 Subject: [PATCH 3/3] test: pick the old versions from the history's last counter Rejected moves record no op, so fixed counters can be missing. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/loro-wasm/tests/tree_checkout_path.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/loro-wasm/tests/tree_checkout_path.test.ts b/crates/loro-wasm/tests/tree_checkout_path.test.ts index 9c81fb082..e78c235c6 100644 --- a/crates/loro-wasm/tests/tree_checkout_path.test.ts +++ b/crates/loro-wasm/tests/tree_checkout_path.test.ts @@ -119,11 +119,11 @@ describe("tree checkout between old versions", () => { } doc.commit(); - const versions = [ - [{ peer: "1" as const, counter: 200 }], - [{ peer: "1" as const, counter: 260 }], - [{ peer: "1" as const, counter: 900 }], - ]; + // Rejected moves record no op, so pick versions from the last counter. + const last = doc.frontiers()[0].counter; + const versions = [0.2, 0.25, 0.7].map((f) => [ + { peer: "1" as const, counter: Math.floor(last * f) }, + ]); const expected = versions.map((v) => { const fresh = new LoroDoc(); fresh.import(doc.export({ mode: "update" }));