|
| 1 | +# Movable List `Move`/`Set` Validation on Import |
| 2 | + |
| 3 | +Verified against code 2026-09-28 (merged with main `6e294c87`). |
| 4 | + |
| 5 | +Imported movable-list `Move { from, to, elem_id }` and `Set { elem_id, value }` ops |
| 6 | +come from other peers, so they are external input. Several shapes of them used |
| 7 | +to panic while the doc held its locks (`pos.unwrap()`/`value_id.unwrap()` in |
| 8 | +`MovableListState::apply_diff_and_convert`, `idlp_to_id(..).unwrap()` in |
| 9 | +`MovableListHistoryCache::last_pos`, `last_value(..).unwrap()` in |
| 10 | +`MovableListDiffCalculator::calculate_diff`, `convert_index(..).unwrap()` for |
| 11 | +an overrunning list delta, and the diff tracker's B-tree for huge positions). |
| 12 | +The panic poisoned a `LoroMutex`, and the process aborted when a destructor hit |
| 13 | +the poisoned lock during unwind. |
| 14 | + |
| 15 | +Tests: |
| 16 | +- `crates/loro/tests/movable_list_invalid_ops.rs`. |
| 17 | +- The binary and `import_batch` cases in `crates/loro-internal/src/tests/import_atomicity.rs`. |
| 18 | +- The `ChangeStore` rollback and KV lookup tests in `src/oplog/change_store.rs`. |
| 19 | +- `crates/loro-wasm/tests/movable_list_invalid_ops.test.ts`. |
| 20 | + |
| 21 | +## Rejected with `Err`: ops with no meaning |
| 22 | + |
| 23 | +1. **Positions no real sequence can reach.** `InnerListOp::check_positions` |
| 24 | + (`src/container/list/list_op.rs`) runs when binary (`outdated_encode_reordered::decode_op`) |
| 25 | + and JSON (`json_schema::decode_op`) ops are decoded. |
| 26 | + - It rejects any position of a List/MovableList/Text op at or past |
| 27 | + `UNKNOWN_SPAN_LEN - 1` (`src/container/richtext/tracker.rs`). That is the length |
| 28 | + of the tracker's placeholder span for unreplayed history. |
| 29 | + - Positions past it panicked inside the tracker before `validate_diff` could run. |
| 30 | + - This check needs no history, so it covers every import path, including |
| 31 | + detached imports. |
| 32 | + - **It is a hard document limit, not only an import check.** `decode_op` is also |
| 33 | + how a doc parses its own stored change blocks and snapshot blocks |
| 34 | + (`block_encode.rs`), so a sequence position ≥ 1,073,741,822 is rejected there |
| 35 | + too. That is deliberate: |
| 36 | + - Snapshot and update blocks are external input as well. |
| 37 | + - A sequence that long (≈1.07e9 items, ≥1 GB of text) cannot be diffed or |
| 38 | + imported by any peer anyway: the tracker panics before this check existed. |
| 39 | + - Skipping the check for "own" blocks would need a trust flag threaded through |
| 40 | + block decoding. |
| 41 | +2. **Unknown or out-of-history element.** `OpLog::validate_movable_list_elem_refs_in_import_scope` |
| 42 | + (`src/oplog.rs`) checks each `Move`/`Set`. `elem_id` must be an `Insert` op in the |
| 43 | + same container that lies in the op's causal history (see the causal check below). |
| 44 | + It returns `LoroError::DecodeError`. |
| 45 | + - The references are recorded by `OpLog::insert_new_change` into the open |
| 46 | + `ImportRollback`, so they cover: |
| 47 | + - directly imported changes, |
| 48 | + - pending changes the import unlocks, |
| 49 | + - every blob of an `import_batch`. |
| 50 | + - The validator reads the recorded list, so it never re-reads the imported range |
| 51 | + from the change store. |
| 52 | + - `OpLog::resolve_movable_list_elem` finds the element with one |
| 53 | + `ChangeStore::get_change_by_lamport_lte` lookup. That lookup scans both parsed |
| 54 | + and KV-only blocks, so a miss means the lamport is not in the stored history. |
| 55 | + - What an element resolves to (insert op, container) does not depend on the op, so |
| 56 | + each pass caches it per `elem_id`. Moves/sets keep hitting the same elements. |
| 57 | + - The causal check reads the cached start version of the op's DAG node |
| 58 | + (`AppDag::ensure_vv_for`); earlier ops of the same node are by the same peer. |
| 59 | + `AppDag::get_vv` would clone and insert into a version vector per op. |
| 60 | + - Only on a shallow doc does a miss fall back to the shallow-root state |
| 61 | + (`ContainerHistoryCache::shallow_root_has_movable_list_elem`): an op after the |
| 62 | + root can only see pre-root elements that are still alive at the root. |
| 63 | + - Call sites: |
| 64 | + - the attached branch of `import_changes_and_apply_delta_to_state_if_needed` |
| 65 | + (when `rollback_enabled`); |
| 66 | + - its detached branch (it opens its own scope when the preflight asks for one |
| 67 | + and no batch scope is open); |
| 68 | + - `BatchImportGuard::finish`, which validates the whole batch once before the |
| 69 | + closing checkout and rolls the whole batch back on failure |
| 70 | + ([import-batch-atomicity.md](import-batch-atomicity.md)). |
| 71 | +3. **Out-of-bounds `from`/`to` that survive the import.** `MovableListState::validate_diff` |
| 72 | + bounds-checks the list delta in op-index space (dead list items count), the same |
| 73 | + way `ListState::validate_diff` does. |
| 74 | + - This check needs state, so a detached import cannot run it (see the gaps below). |
| 75 | + - It checks the composed delta, so an out-of-bounds move that a later op in the |
| 76 | + same import cancels (e.g. `move to: 9` followed by `delete pos: 9`) is accepted. |
| 77 | + Every path gives the same result for it. |
| 78 | + |
| 79 | +### Why every such import gets a rollback scope |
| 80 | + |
| 81 | +`ImportChangesPreflight` (`OpLog::preflight_import_changes`) and |
| 82 | +`PendingChanges::has_state_apply_rollback_ops` set `needs_state_apply_rollback` |
| 83 | +for List, MovableList and Tree ops. |
| 84 | +- The preflight inspects the ops of **every** new change, including ones whose |
| 85 | + deps are not in the DAG yet. Those deps may be earlier changes of the same import, |
| 86 | + which then unlock them during the import. |
| 87 | +- It used to skip such changes before looking at their ops. That let |
| 88 | + `[C1: map-only change, C2: forged op depending on C1]` skip both the rollback |
| 89 | + scope and the validation. On that path a cross-container move was accepted |
| 90 | + silently and put one element in two lists. |
| 91 | + |
| 92 | +## Applied with CRDT semantics: move/set of a deleted element |
| 93 | + |
| 94 | +A `Move`/`Set` that is causally after the delete of its element cannot come from |
| 95 | +the public API. It is still **accepted**, with the same meaning as a *concurrent* |
| 96 | +move/set of a deleted element. The move creates a new position, so the element |
| 97 | +comes back with its last value. The set changes a value nobody can see. Why: |
| 98 | + |
| 99 | +- It gives a forger nothing new. The forger can declare deps from before the |
| 100 | + delete and get the same effect from a legitimate concurrent move. |
| 101 | +- Rejecting it consistently is not affordable. Checkout-mode replay (`checkout`, |
| 102 | + `import_batch`'s closing reattach, concurrent imports, detached → `attach`) would |
| 103 | + have to know whether the element is alive at the op's version. That needs an |
| 104 | + index from list items to the deletes that cover them. If only the linear path |
| 105 | + rejected it, `import` and `import_batch` of the same bytes would disagree. |
| 106 | + |
| 107 | +The one path that could not apply it is the forward fast path |
| 108 | +(`DiffMode::Linear`/`ImportGreaterUpdates`). There the movable-list diff only |
| 109 | +carries the fields the op touched, and `apply_diff_and_convert` fills in the rest |
| 110 | +from the element in `DocState`. Detection and fallback: |
| 111 | +- `MovableListState::references_absent_elem` / `DocState::needs_checkout_diff` |
| 112 | + detect a delta that moves/sets an element missing from the state. |
| 113 | +- `recalc_in_checkout_mode_if_needed` (`src/loro.rs`) recomputes that import's |
| 114 | + diff with `DiffCalculator::new(true)` (Persist, so always Checkout mode). This |
| 115 | + matches a full replay. |
| 116 | +- Honest imports never trigger it. |
| 117 | +- `validate_diff` still returns `Err` for such a delta as a backstop. |
| 118 | + |
| 119 | +## Change-store pitfalls this work exposed |
| 120 | + |
| 121 | +- **`decode_block_range` read a version varint that blocks do not have.** |
| 122 | + `encode_block` writes a postcard `EncodedBlock` with no version prefix, so every |
| 123 | + field was read one position off. |
| 124 | + - History: `3d2d9d9c` (2024-09, "refactor: optimize block encoder") removed the |
| 125 | + `version` field from the block, its encoder and the full decoder, but not from |
| 126 | + `decode_block_range`. |
| 127 | + - Every `loro-crdt@1.0.0*` release contains that commit, so no 1.x build ever wrote |
| 128 | + version-prefixed blocks and the fix cannot make stored data unreadable. |
| 129 | + |
| 130 | + Consequences: |
| 131 | + - Blocks that do not start at counter 0 were skipped. |
| 132 | + - Block `0@P` got its lamport length as its lamport start. |
| 133 | + - Lamport lookups on KV-only blocks were wrong: |
| 134 | + `get_change_with_lamport_lte` / JS `getChangeAtLamport` after a snapshot load |
| 135 | + (this was also broken on main), and the element validator above. |
| 136 | + - Existing tests missed it because they parsed every block first. |
| 137 | +- **Rollback must not leave an old block in front of the next insert.** |
| 138 | + `insert_change_inner` merges a change into the cached block right before it. |
| 139 | + - Failure sequence: an import creates a peer's newest block without loading the |
| 140 | + older ones; a read during the scope (such as the validator's lamport lookup) |
| 141 | + caches an older KV block; rollback removes the newest block. The next insert of |
| 142 | + the same change then panicked with "counter should be continuous". |
| 143 | + - `ChangeStore::rollback_import` therefore evicts the flushed blocks of every peer |
| 144 | + the rollback touched. They reload from KV on demand. |
| 145 | +- **Cheap rollback records.** `ChangeStoreRollback` keeps a `BlockShape` for each |
| 146 | + unflushed pre-scope block an import appended to: change count, the last change's |
| 147 | + op count and its last op. Rollback truncates back to it. |
| 148 | + - Imports only append (push changes, or push ops that may merge into the last op), |
| 149 | + so the shape is enough. |
| 150 | + - Flushed blocks need no record: their KV copy is the pre-scope version. |
| 151 | + - The earlier record was an `Arc` of the whole block, which made the next append |
| 152 | + copy the block's changes on every import under a scope. That made small |
| 153 | + movable-list imports about 40% slower, and List/Tree imports on main already |
| 154 | + paid it (they are about 45% faster now). |
| 155 | + |
| 156 | +## Known gaps (not fixed here) |
| 157 | + |
| 158 | +- A `Move` whose `from` does not point at its element is accepted. The tracker |
| 159 | + removes whatever list item sits at `from`, so another element can disappear. |
| 160 | + The result is the same on every path, but it does not match any honest op. |
| 161 | +- An explicitly detached doc that imports an op which only state validation |
| 162 | + rejects (an out-of-bounds list insert, or a movable-list move out of bounds) |
| 163 | + panics on `attach()`/`checkout_to_latest`. Those return `()` and `expect` the |
| 164 | + checkout. This affects every container type and predates this change. |
| 165 | +- The oplog inside a `FastSnapshot` is not validated op by op, because that would |
| 166 | + decode every block. A forged snapshot can still reach the diff calculator's |
| 167 | + unwraps on a later checkout. |
| 168 | +- A forged change parked as pending fails the import that later unlocks it, so |
| 169 | + that import is rolled back, the same as for list bounds errors. |
0 commit comments