fix: reject malformed movable list move/set ops on import instead of aborting

[zxch3n]

Summary

Importing a malformed movable-list Move/Set op (found by Codex while reviewing #1119, also on main) panicked while the doc held its locks, poisoned them, and aborted the process. This PR makes those imports return Err with nothing applied, or, for moves/sets of deleted elements, applies them with the same result on every import path.

The adversarial review of the first version (NOT READY) found four blocking problems. All four are fixed here, and two of them were also bugs on main:

• B1: honest imports were rejected on docs loaded from a snapshot. Root cause on main: decode_block_range read a version varint that the block encoding does not have, so every field was read one position off. As a result, get_change_with_lamport_lte / JS getChangeAtLamport returned None or the wrong change after a snapshot load, also on main (on a snapshot-loaded multi-block peer, 42 of 669 sampled lamports returned None and 625 the wrong change; 0 after the fix). The PR's element lookup used the same path.

  • Fixed the decoder.
  • A lookup miss now means "not in the stored history"; only on a shallow doc does it fall back to the shallow-root state.

• B2: abort after a rolled-back import. Rollback could leave an older cached KV block right before the next insert ("counter should be continuous"). ChangeStore::rollback_import now evicts the flushed blocks of the peers it touched.

• B3: validation could be bypassed. The import preflight skipped the ops of changes whose deps arrive in the same import. That affected [map-only change, forged op depending on it]:

  • no rollback scope and no validation;
  • the original panics were reachable again;
  • a cross-container move was accepted silently, putting one element in two lists.

  The preflight now inspects every new change's ops.

• B4: positions ≥ 2^30 still panicked inside the diff tracker. InnerListOp::check_positions now rejects them when binary and JSON ops are decoded, for List, MovableList and Text ops (all were affected on main). Block decoding goes through the same decode_op, so this is a hard sequence-length limit (1,073,741,822 items) that also applies when a doc reads its own stored blocks or a snapshot. I kept it there on purpose: snapshot blocks are external input too, a sequence that long could not be imported by any peer before this PR either (the tracker panics), and an import-only check would need a trust flag threaded through block decoding. Documented in the context file and the changeset.

What the import does now

Rejected with Err, doc unchanged:

• a Move/Set whose elem_id is not an Insert of the same list in the op's causal history (validated for every Move/Set inserted inside the import's rollback scope: direct changes, unlocked pending changes, whole import_batch);
• out-of-bounds moves that survive the import (MovableListState::validate_diff);
• positions past the tracker's placeholder span.

Applied with CRDT semantics: move/set of a deleted element. It is equivalent to a legitimate concurrent move/set (a forger can declare deps from before the delete), and Checkout-mode paths already applied it. The linear fast path now falls back to a Checkout-mode diff when a delta targets an element missing from the state. import, import_batch, snapshot + import, detached + attach and full replay agree (tested). Honest imports never take the fallback.

Performance

Round 1 measured +30–35% for many small movable-list imports and +10–18% for large ones. Round 2 still found large imports at +6–11%. Profiling showed three causes:

1. The rollback record held an Arc of the peer's last change block, so every append copied that block's changes (copy-on-write). main already pays this for List/Tree imports.
2. The validator re-read the imported range from the change store.
3. For each Move/Set, the validator did a lamport lookup, a binary search through the change's ops, and a version-vector clone-and-insert (AppDag::get_vv). It was about 6.3% of a 120k-op import.

Fixes:

• The rollback records a block's shape and truncates back to it.
• insert_new_change records the Move/Set refs.
• Each element id is resolved once per validation pass, since the result doesn't depend on the op.
• The causal check reads the cached start version of the op's DAG node.

After these, the validator is about 2% of a large import.

Release builds, main 6e294c87 vs this PR (ca3cfeb4, same code as the head), alternating order each round, 7 rounds, import time only.

The reviewer's probe (r1125_perf, load 6.2–6.8):

workload	main med ms	PR med ms	Δ med	Δ min
small-ml (4000 small movable-list updates)	38.3	39.2	+2.4%	+2.9%
small-ml-from-snapshot	41.4	43.0	+4.1%	+4.4%
small-list	29.0	19.9	−31.5%	−31.2%
small-tree	20.2	14.4	−28.6%	−29.6%
big-ml-empty (120k ops, ~70% move/set)	180.3	187.1	+3.8%	+3.5%
big-ml-linear	329.5	329.6	+0.0%	+0.5%
big-ml-snapshot-nonempty	340.6	333.4	−2.1%	+0.8%
big-ml-batch-snapshot-plus-update	223.4	228.8	+2.4%	+2.9%
big-text-empty	138.4	136.3	−1.5%	−0.6%

My probe (ml_import_perf, load 4.5–6.8):

workload	main med ms	PR med ms	Δ med	Δ min
small-ml	22.7	23.9	+5.2%	+7.9%
small-ml-from-snapshot	24.6	26.0	+5.4%	+6.0%
small-list	25.6	14.0	−45.5%	−45.4%
small-text	9.7	9.7	+0.0%	+0.8%
big-ml-empty	315.6	320.3	+1.5%	+4.3%
big-ml-linear	351.3	359.6	+2.4%	+1.1%
big-ml-snapshot-nonempty	366.7	373.9	+2.0%	+1.6%
big-ml-batch-snapshot-plus-update	380.4	393.6	+3.5%	+1.6%

Summary: movable-list imports are about 2–8% slower for many small imports and 0–4% slower for large ones. List/Tree imports are 30–45% faster. The earlier description's "+0.5%" for big-ml-empty came from one noisy series, and the reviewer could not reproduce it (they measured +11% at the time); these tables replace it.

Validation

• (Numbers below are from round 1 unless noted; after merging main 6e294c87 and the validator change: cargo test -p loro -p loro-internal 52 binaries, 1,267 passed, 0 failed; pnpm check clean.)
• Rust regressions:
  • crates/loro/tests/movable_list_invalid_ops.rs (16 tests: B1 snapshot-loaded move/set, B3 same-import dependency for 1 and 2 peers, attached/concurrent/detached, cross-container, B4 huge positions incl. List/Text, reversed delete at 0 still accepted, shallow-root cases);
  • change_store_test.rs (lamport lookup after snapshot load);
  • ChangeStore unit tests: KV-only lamport lookup, rollback evicting an older cached block (B2), rollback restoring an appended-to unflushed block byte-for-byte;
  • import_atomicity.rs: binary 2-peer B3 via import, single-blob import_batch, detached.
• Each new test was checked to fail without its fix.
• WASM: crates/loro-wasm/tests/movable_list_invalid_ops.test.ts (6 tests); the full package test script passes: 30 vitest files / 399 tests, tsc, Deno and Bun.
• cargo test -p loro -p loro-internal: 49 test binaries, 0 failed. pnpm check: clean.
• Fuzz: movable_list 10 min (632,713 runs), no crash; pnpm run-fuzz-corpus: exit 0.
• Differential against main (random 3–5 peer histories with movable-list/nested/text/map edits, undo/redo, and syncs by:
  • plain import
  • snapshot import
  • import_batch (with and without a snapshot)
  • detached import + attach
  • JSON
  • checkout + import
  • restarts from full and shallow snapshots)
  • Result: 3 × 1500 seeds give identical per-step results, final values and events on both builds, with 0 panics.
• Combined with fix: reject creating unknown-type containers instead of panicking #1142 head b550599d: merges cleanly; cargo test -p loro -p loro-internal 53 binaries, 0 failed.

Non-blocking review items

• N1: an out-of-bounds move that a later op in the same import cancels is accepted, with the same result on every path. Documented.
• N3 (event granularity) still reproduces. The reviewer's round-2 seed 1178 (1/1600) does detach → import 5 chunks → attach: main emits one Text Insert "abab" and the PR emits Insert "ab", Insert "ab". The content and resulting values are identical. My targeted shapes and my own event-hashing differential did not hit it, which is why the earlier text said it did not reproduce; that was wrong. It is a cosmetic difference in how the attach delta is split, and consumers that apply the deltas get the same text.
• N5: the unreachable validation in the legacy import path was removed.

Known gaps (documented in context/movable-list-op-validation.md)

• A Move whose from does not point at its element is accepted (the result is consistent across paths).
• A detached import of an op that only state validation rejects still panics on attach() (all container types, predates this PR).
• Ops inside a FastSnapshot are not validated one by one.

Filed separately (pre-existing on main, not fixed here):

• export(updates_in_range) panics with several non-contiguous spans of one peer #1155: updates_in_range with several spans of one peer panics.
• Empty root container shows up in a doc's value but not after replaying its own history #1156: an empty root container is missing after replaying a doc's own history.
• Importing a Text insert past the end of the text panics (richtext_state.rs:1653) #1160: a Text insert past the end of the text panics (richtext_state.rs:1653).
• Honest import panics after an import rejected by state validation was rolled back (arena values) #1161: an honest import panics after an import rejected by state validation was rolled back (arena values; seed repro, root cause unconfirmed).

🤖 Generated with Claude Code

[github-actions]

WASM Size Report

• Original size: 3244.12 KB
• Gzipped size: 1077.22 KB
• Brotli size: 754.35 KB

[zxch3n]

Addressed the adversarial review (NOT READY) in a45d957d..bc9dc955, merged with main 1eb14b3b:

• B1: fixed the root cause. decode_block_range read a nonexistent version varint, which also broke getChangeAtLamport after snapshot loads on main. A lookup miss now falls back to the shallow-root state only on shallow docs.
• B2: ChangeStore::rollback_import evicts flushed blocks of the peers it touched, so a block cached during the scope can no longer precede the next insert.
• B3: the preflight inspects the ops of every new change, including ones whose deps arrive in the same import. Covered for attached, concurrent, detached and single-blob import_batch, with 1 and 2 peers, plus cross-container.
• B4: positions ≥ u32::MAX/4 - 1 are rejected when decoding binary and JSON ops, for List, MovableList and Text.
• Perf: the rollback record no longer copies the peer's last change block on every import. Small movable-list imports went from +30–50% to about +5–9%, large ones are within noise, and small List imports are about 45% faster than main.
• N5: the unreachable legacy-path validation was removed. N3 doesn't reproduce on the current head.

Tables, the differential runs (3×1500 seeds identical to main, including events) and the #1142 combination are in the updated description. The pre-existing main bugs from the report are #1155 and #1156.

🤖 Generated with Claude Code

[zxch3n]

Round-2 follow-ups, pushed through d6888948 (merged main 6e294c87):

1. Perf: profiled the 120k-op import. The validator was about 6.3% of it: a lamport lookup plus an op search per Move/Set, and a version-vector clone-and-insert (AppDag::get_vv) per op. Each element id is now resolved once per validation pass, and the causal check reads the cached start version of the op's DAG node, so the validator is about 2% of the import. With your r1125_perf probe (alternating, 7 rounds, load 6.2–6.8), comparing median / min:

   • big-ml-empty: +3.8% / +3.5% (was +11%)
   • big-ml-linear: +0.0% / +0.5%
   • snapshot-nonempty: −2.1% / +0.8%
   • batch: +2.4% / +2.9%
   • small-ml: +2.4% / +2.9%
   • small-list and small-tree: about −30%

   My own probe gives 0–4% for large and 5–8% for small movable-list imports. The description and changeset now state these numbers; the old "+0.5%" claim is gone.

2. Changeset states the main bug: 42 undefined + 625 wrong of 669 getChangeAtLamport samples after a snapshot load, now 0.

3. N3 is recorded as reproducible (seed 1178, cosmetic delta split), replacing "does not reproduce".

4. 2^30 bound: kept at decode and documented as a hard sequence-length limit that also covers stored blocks and snapshots (reasons in the description and the context file).

5. Issues: Importing a Text insert past the end of the text panics (richtext_state.rs:1653) #1160 (Text insert past the end panics) and Honest import panics after an import rejected by state validation was rolled back (arena values) #1161 (honest import panics after a state-validation rollback; reproduces with the harness at seed 120 on main in a debug build; I couldn't reduce it to a standalone test, and the root-cause hypothesis is in the issue).

cargo test -p loro -p loro-internal: 52 binaries, 1,267 passed, 0 failed. pnpm check: clean.

🤖 Generated with Claude Code