Skip to content

fix: take the tree checkout window from the two versions - #1152

Merged
zxch3n merged 4 commits into
mainfrom
fix/tree-checkout-window
Sep 28, 2026
Merged

zxch3n merged 4 commits into
mainfrom
fix/tree-checkout-window

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes two LoroTree checkout bugs found by the loro.js differential fuzz (#1141, "Rust-side findings"), both reproducible with loro-crdt alone. They have one root cause:

  1. Panic on a checkout sequence with a subscriber. p1 inserts a list element and creates node A with child B in one commit; p3 imports only the list op and inserts; p2 increments a counter on top of p3; p3 sets a map key; p2 imports p1 and edits B's metadata. With a subscriber, checkout([1@3]) then checkout([1@2]) panicked at tree_state.rs:1075 (get_index_by_tree_id(..).unwrap() on a Create).
  2. Checkout depended on the path taken. Checking out [3@2] (which contains a node's creation) and then the concurrent [1@1] (which does not) kept the node. Checking out [1@1] directly was correct.

Root cause

TreeDiffCalculator::checkout_diff retreated and replayed only ops at or above 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, and an op concurrent with the meet can have a lower lamport:

  • Bug 2: 0@3 creates the node at lamport 1 and is concurrent with the meet [2@2] (lamport 2). The window started at 2, so the creation was never retreated.
  • Bug 1: going [1@3] → [1@2], the meet is [0@2] (window starts at 2). The forward pass skipped create A (lamport 1) but applied create B under A (lamport 2). Converting that to an event panicked; without a subscriber the state was wrong instead.

docs/critical-version-spec.md Q7 had argued the window was safe because an op concurrent with a meet head always leaves an uncovered branch that forces a critical base. It doesn't: the op's dependencies can reach shared history below the meet, so the walk sees no dead end. The cache's own checkout (moving it to from) had the same problem.

Fix

Both transitions of TreeCacheForDiff now start at min_lamport_of_version_diff(from, to): the smallest lamport among ops that are in exactly one of the two versions. Lamports grow along a peer's ops, so this reads only the first op of each per-peer span of the version-vector difference (O(peers)). Everything cached at or above it is retreated newest first, and the target's ops at or above it are replayed oldest first, which keeps each op's effected (cycle check) flag computed in (lamport, peer) order. Ops below it are in both versions.

  • Both passes walk TreeOpGroup::ops (ordered by IdLp) instead of scanning every node's op set, and the tree calculator no longer calls find_replay_base, which could walk the whole DAG.
  • The retreat pass ends at TreeCacheForDiff::max_lamport, an upper bound on every cached op's 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 the bound a checkout between two old versions walked every later tree op (review finding; fixed in 4d1c6d4).
  • A shallow doc's cache starts at the shallow root. It is seeded with the root's nodes, but current_vv only covered the seeded op IDs, so init_current_vv sets it to the root version before the first transition.
  • Removed helpers that became unused (get_importing_cache, get_min_lamport_at); find_replay_base is only compiled for the DAG tests (#[cfg(test)]).

Rationale: context/tree-checkout-window.md. Q7 in the spec and the loro-internal AGENTS.md rule about tree windows are updated.

Same class of bug in List / MovableList / Map?

No. The new random test (below) covers map, list, movable list, text, and counter edits on the same concurrent histories and every version pair, with and without a subscriber. None of those diverged on main or here: text/list trackers retreat and advance individual ops, and map/movable-list checkouts read the history cache per version.

Tests

  • crates/loro/tests/tree_checkout_path.rs: the two repros. On main the first aborts with the tree_state.rs:1075 panic and the second keeps node 0@3.
  • crates/loro-wasm/tests/tree_checkout_path.test.ts: the same through loro-crdt. Both fail on a WASM build of main (RuntimeError: unreachable, and the extra node).
  • crates/fuzz/tests/checkout_path.rs: 3 peers make random concurrent edits (tree create/move/delete, moves into and out of deleted subtrees, metadata edits, plus map/list/movable-list/text/counter) and sync at random. For every ordered pair (a, b) of 24 versions, including heads no peer had, a document checks out a, then b. After each checkout its value must equal a document that imported only that version's changes, and the event mirror (fuzz::actor::Actor's tracker) must equal the state; a second document does the same without a subscriber. 24 fixed seeds run by default (13 s debug). On main seeds 19 and 22 fail; a 300-seed release sweep fails 9 seeds on main and none here (CHECKOUT_PATH_SEEDS=0..300). The existing fuzzers never hit this because they only check out versions from one peer's own history, which are ordered by inclusion.
  • retreat_bound_follows_the_cached_version (unit test in tree.rs): after scrubbing between two old versions, every cached op is at or below max_lamport, and max_lamport is at or below the cached version's lamport (it fails without retreated_from).
  • crates/loro/tests/perf_tree_checkout_old_versions.rs (#[ignore], like the other perf_* tests; cargo test --release -p loro --test perf_tree_checkout_old_versions -- --ignored): 40 checkouts between two old versions with 10k vs 200k later moves must not grow more than 4×. Unbounded retreat: 7.0 ms → 82.5 ms (fails); bounded: 3.7 ms → 4.4 ms.
  • tree_checkout_path.test.ts also scrubs back and forth between three old versions and compares each with a direct checkout (no timing assertions in the WASM suite).
  • Comments on checkout_across_non_critical_meet_stays_canonical / checkout_with_low_lamport_concurrent_branch_stays_canonical (issue.rs) updated; both still pass.

Validation

Branch merged with main 1eb14b3 (includes #1134), clean.

  • cargo test -p loro -p loro-internal: pass.
  • pnpm check (clippy -Dwarnings): pass.
  • cargo test -p fuzz (whole crate): pass; checkout_path's 24 default seeds pass.
  • WASM, on a release build of main + this PR + fix: drop cached container deletions that a move can undo #1153 (pnpm -C crates/loro-wasm build-release, then the package test chain): vitest 31 files / 401 tests, tsc --noEmit, deno 4/4, bun 4/4: pass.
  • The reviewer's independent checkout-path harness (200 seeds × 5 modes, main + both PRs, release): the only failure is seed 138, a pre-existing missing tree Create event on import, identical on main (filed as Tree import can miss the Create event for a child of a node revived from a deleted subtree #1157).
  • Short cargo-fuzz on main + both PRs at the current heads: tree 215,855 runs and diff_calc 244,433 runs, 181 s each (LORO_FUZZ_SANITIZER=none), no crashes, artifact dirs empty.

Performance

Release, isolated target dirs, main / this PR / #1153 run alternately in 5 rounds, thread CPU time (the machine is shared; 1-min loadavg at run start 6.9–11.9). ms: median of the per-round medians, min of mins in parentheses. The first rows are the reviewer's harness (single peer, 2k nodes then N random moves).

scenario main this PR
checkout between two old versions [2500]↔[2510] ×20, N = 300k 3.45 (3.32) 3.61 (3.20)
same with a subscriber 3.41 (3.34) 3.35 (3.24)
same, N = 100k 3.51 (3.30) 3.58 (3.20)
[end-20]↔[end-10] ×20, N = 100k 4.63 (4.30) 5.64 (4.06)
[2500]↔latest ×3, N = 100k 399 (348) 359 (334)
one low-lamport op concurrent with 100k ops, toggled ×3 787 (691) 700 (670)
one op on top of 100k ops, toggled ×20 4.81 (4.18) 4.21 (3.94)
5k-node tree, 3 peers, 3k concurrent moves: checkout through 30 versions 63.2 (51.6) 56.6 (50.0)
same with a subscriber 81.1 (61.8) 71.4 (61.4)
latest ↔ one version back ×20 273 (223) 306 (222)
100k-node tree: 20k local moves 31.6 (22.3) 28.8 (24.9)
100k-node tree: import 20k moves 96.2 (82.7) 92.2 (84.4)
100k-node tree: checkout before/after the 20k moves 316 (209) 306 (242)

Before the retreat bound (f7a9a02), the first row was about 151 ms at N = 300k and 48 ms at N = 100k (review measurement); it is now at main's level and does not grow with N. The other rows are within the noise of this machine (medians of identical binaries move by about ±10–15% between rounds).

Merge order and conflicts

Independent of #1153 (the dead-container cache fix, the third finding); either can merge first, and they merge cleanly together. #1134 is merged into this branch through main; trial merges with the current heads of #1125 and #1142 are clean. After this lands, the corresponding "Rust-side findings" entries in context/loro-js-differential-fuzz.md (#1141) can be dropped.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

WASM Size Report

  • Original size: 3230.83 KB
  • Gzipped size: 1073.05 KB
  • Brotli size: 751.31 KB

zxch3n and others added 2 commits September 28, 2026 18:25
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Rejected moves record no op, so fixed counters can be missing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

zxch3n commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

Review follow-up (thanks for the adversarial pass):

🤖 Generated with Claude Code

@zxch3n
zxch3n merged commit 6e294c8 into main Sep 28, 2026
1 check passed
zxch3n added a commit that referenced this pull request Sep 28, 2026
…ontainer

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AVGVSTVS96 added a commit to AVGVSTVS96/seqno that referenced this pull request Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant