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

.changeset/movable-list-invalid-move-set.md

@@ -0,0 +1,17 @@
+---
+"loro-crdt": patch
+---
+
+Importing a malformed movable-list `move` or `set` op no longer aborts the process. These ops used to panic inside the document locks, poisoning them, and the process then aborted during unwind. Now:
+
+- A `move`/`set` whose element id is not an element of the same list in the op's causal history, or a `move` whose index is past the end of the list, makes `import`, `importJsonUpdates` and `importBatch` return an error. Nothing from that import is applied and the document stays usable. This also holds when the op depends on another change in the same import, which used to skip validation.
+- Insert, delete and move positions of 1,073,741,822 (about 2^30) or more in a List, MovableList or Text op are rejected when the op is decoded, instead of panicking during import. This is a hard limit on sequence length: it also applies when a document reads its own stored history or a snapshot. No peer could import such a document before either.
+- A `move`/`set` that targets an element deleted earlier in its own history is applied the same way a concurrent move/set of a deleted element is: the move brings the element back, and the set is not visible. Every import path, and a replay of the full history, gives the same result.
+
+Also fixed:
+
+- `getChangeAtLamport` returned `undefined` or the wrong change for changes that were loaded from a snapshot and not yet read. The change store misread the lamport range of stored blocks, a leftover from the block format change in 1.0. On a snapshot-loaded peer with many blocks, 42 of 669 sampled lamports returned `undefined` and 625 returned the wrong change; all 669 are now correct.
+- A rolled-back import could leave a cached change block behind that made the next import of the same changes panic with "counter should be continuous".
+- Imports of List and Tree ops no longer copy the peer's last change block on every import to keep a rollback copy. Many small List or Tree imports are about 30% faster.
+
+Performance: movable-list imports now check every `move`/`set` against the document's history. In release benchmarks that makes them about 2–8% slower for many small imports and 0–4% slower for a 120k-op import where about 70% of the ops are moves and sets.

AGENTS.md

@@ -24,6 +24,9 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec.
   [context/mergeable-containers.md](context/mergeable-containers.md).
 - `import_batch` force-detach, batch-wide rollback scope, and the never-exit-detached
   invariant: [context/import-batch-atomicity.md](context/import-batch-atomicity.md).
+- Movable-list `Move`/`Set` import validation, and why a move of a deleted element
+  is applied rather than rejected:
+  [context/movable-list-op-validation.md](context/movable-list-op-validation.md).
 - Shallow-snapshot dead-style redaction (null-only, both-expand exclusion,
   root-whitelisted latest state):
   [context/shallow-snapshot-style-redaction.md](context/shallow-snapshot-style-redaction.md).

context/import-batch-atomicity.md

@@ -32,7 +32,9 @@ is still at its pre-batch version and the `OpLog` is the only thing that moved.
 the `OpLog` therefore makes the two agree again, which is what lets the doc stay
 attached. `import_batch` opens `OpLog::begin_import_rollback` before the loop and
 `BatchImportGuard::finish` either commits it or rolls the whole batch back and returns
-the state-apply error.
+the state-apply error. Before that checkout, `finish` also validates the movable-list element references
+of every blob in the batch ([movable-list-op-validation.md](movable-list-op-validation.md)).
+A failure there rolls the batch back the same way.
 
 Consequences to keep in mind:
 

context/movable-list-op-validation.md

@@ -0,0 +1,169 @@
+# Movable List `Move`/`Set` Validation on Import
+
+Verified against code 2026-09-28 (merged with main `6e294c87`).
+
+Imported movable-list `Move { from, to, elem_id }` and `Set { elem_id, value }` ops
+come from other peers, so they are external input. Several shapes of them used
+to panic while the doc held its locks (`pos.unwrap()`/`value_id.unwrap()` in
+`MovableListState::apply_diff_and_convert`, `idlp_to_id(..).unwrap()` in
+`MovableListHistoryCache::last_pos`, `last_value(..).unwrap()` in
+`MovableListDiffCalculator::calculate_diff`, `convert_index(..).unwrap()` for
+an overrunning list delta, and the diff tracker's B-tree for huge positions).
+The panic poisoned a `LoroMutex`, and the process aborted when a destructor hit
+the poisoned lock during unwind.
+
+Tests:
+- `crates/loro/tests/movable_list_invalid_ops.rs`.
+- The binary and `import_batch` cases in `crates/loro-internal/src/tests/import_atomicity.rs`.
+- The `ChangeStore` rollback and KV lookup tests in `src/oplog/change_store.rs`.
+- `crates/loro-wasm/tests/movable_list_invalid_ops.test.ts`.
+
+## Rejected with `Err`: ops with no meaning
+
+1. **Positions no real sequence can reach.** `InnerListOp::check_positions`
+   (`src/container/list/list_op.rs`) runs when binary (`outdated_encode_reordered::decode_op`)
+   and JSON (`json_schema::decode_op`) ops are decoded.
+   - It rejects any position of a List/MovableList/Text op at or past
+     `UNKNOWN_SPAN_LEN - 1` (`src/container/richtext/tracker.rs`). That is the length
+     of the tracker's placeholder span for unreplayed history.
+   - Positions past it panicked inside the tracker before `validate_diff` could run.
+   - This check needs no history, so it covers every import path, including
+     detached imports.
+   - **It is a hard document limit, not only an import check.** `decode_op` is also
+     how a doc parses its own stored change blocks and snapshot blocks
+     (`block_encode.rs`), so a sequence position ≥ 1,073,741,822 is rejected there
+     too. That is deliberate:
+     - Snapshot and update blocks are external input as well.
+     - A sequence that long (≈1.07e9 items, ≥1 GB of text) cannot be diffed or
+       imported by any peer anyway: the tracker panics before this check existed.
+     - Skipping the check for "own" blocks would need a trust flag threaded through
+       block decoding.
+2. **Unknown or out-of-history element.** `OpLog::validate_movable_list_elem_refs_in_import_scope`
+   (`src/oplog.rs`) checks each `Move`/`Set`. `elem_id` must be an `Insert` op in the
+   same container that lies in the op's causal history (see the causal check below).
+   It returns `LoroError::DecodeError`.
+   - The references are recorded by `OpLog::insert_new_change` into the open
+     `ImportRollback`, so they cover:
+     - directly imported changes,
+     - pending changes the import unlocks,
+     - every blob of an `import_batch`.
+   - The validator reads the recorded list, so it never re-reads the imported range
+     from the change store.
+   - `OpLog::resolve_movable_list_elem` finds the element with one
+     `ChangeStore::get_change_by_lamport_lte` lookup. That lookup scans both parsed
+     and KV-only blocks, so a miss means the lamport is not in the stored history.
+   - What an element resolves to (insert op, container) does not depend on the op, so
+     each pass caches it per `elem_id`. Moves/sets keep hitting the same elements.
+   - The causal check reads the cached start version of the op's DAG node
+     (`AppDag::ensure_vv_for`); earlier ops of the same node are by the same peer.
+     `AppDag::get_vv` would clone and insert into a version vector per op.
+   - Only on a shallow doc does a miss fall back to the shallow-root state
+     (`ContainerHistoryCache::shallow_root_has_movable_list_elem`): an op after the
+     root can only see pre-root elements that are still alive at the root.
+   - Call sites:
+     - the attached branch of `import_changes_and_apply_delta_to_state_if_needed`
+       (when `rollback_enabled`);
+     - its detached branch (it opens its own scope when the preflight asks for one
+       and no batch scope is open);
+     - `BatchImportGuard::finish`, which validates the whole batch once before the
+       closing checkout and rolls the whole batch back on failure
+       ([import-batch-atomicity.md](import-batch-atomicity.md)).
+3. **Out-of-bounds `from`/`to` that survive the import.** `MovableListState::validate_diff`
+   bounds-checks the list delta in op-index space (dead list items count), the same
+   way `ListState::validate_diff` does.
+   - This check needs state, so a detached import cannot run it (see the gaps below).
+   - It checks the composed delta, so an out-of-bounds move that a later op in the
+     same import cancels (e.g. `move to: 9` followed by `delete pos: 9`) is accepted.
+     Every path gives the same result for it.
+
+### Why every such import gets a rollback scope
+
+`ImportChangesPreflight` (`OpLog::preflight_import_changes`) and
+`PendingChanges::has_state_apply_rollback_ops` set `needs_state_apply_rollback`
+for List, MovableList and Tree ops.
+- The preflight inspects the ops of **every** new change, including ones whose
+  deps are not in the DAG yet. Those deps may be earlier changes of the same import,
+  which then unlock them during the import.
+- It used to skip such changes before looking at their ops. That let
+  `[C1: map-only change, C2: forged op depending on C1]` skip both the rollback
+  scope and the validation. On that path a cross-container move was accepted
+  silently and put one element in two lists.
+
+## Applied with CRDT semantics: move/set of a deleted element
+
+A `Move`/`Set` that is causally after the delete of its element cannot come from
+the public API. It is still **accepted**, with the same meaning as a *concurrent*
+move/set of a deleted element. The move creates a new position, so the element
+comes back with its last value. The set changes a value nobody can see. Why:
+
+- It gives a forger nothing new. The forger can declare deps from before the
+  delete and get the same effect from a legitimate concurrent move.
+- Rejecting it consistently is not affordable. Checkout-mode replay (`checkout`,
+  `import_batch`'s closing reattach, concurrent imports, detached → `attach`) would
+  have to know whether the element is alive at the op's version. That needs an
+  index from list items to the deletes that cover them. If only the linear path
+  rejected it, `import` and `import_batch` of the same bytes would disagree.
+
+The one path that could not apply it is the forward fast path
+(`DiffMode::Linear`/`ImportGreaterUpdates`). There the movable-list diff only
+carries the fields the op touched, and `apply_diff_and_convert` fills in the rest
+from the element in `DocState`. Detection and fallback:
+- `MovableListState::references_absent_elem` / `DocState::needs_checkout_diff`
+  detect a delta that moves/sets an element missing from the state.
+- `recalc_in_checkout_mode_if_needed` (`src/loro.rs`) recomputes that import's
+  diff with `DiffCalculator::new(true)` (Persist, so always Checkout mode). This
+  matches a full replay.
+- Honest imports never trigger it.
+- `validate_diff` still returns `Err` for such a delta as a backstop.
+
+## Change-store pitfalls this work exposed
+
+- **`decode_block_range` read a version varint that blocks do not have.**
+  `encode_block` writes a postcard `EncodedBlock` with no version prefix, so every
+  field was read one position off.
+  - History: `3d2d9d9c` (2024-09, "refactor: optimize block encoder") removed the
+    `version` field from the block, its encoder and the full decoder, but not from
+    `decode_block_range`.
+  - Every `loro-crdt@1.0.0*` release contains that commit, so no 1.x build ever wrote
+    version-prefixed blocks and the fix cannot make stored data unreadable.
+
+  Consequences:
+  - Blocks that do not start at counter 0 were skipped.
+  - Block `0@P` got its lamport length as its lamport start.
+  - Lamport lookups on KV-only blocks were wrong:
+    `get_change_with_lamport_lte` / JS `getChangeAtLamport` after a snapshot load
+    (this was also broken on main), and the element validator above.
+  - Existing tests missed it because they parsed every block first.
+- **Rollback must not leave an old block in front of the next insert.**
+  `insert_change_inner` merges a change into the cached block right before it.
+  - Failure sequence: an import creates a peer's newest block without loading the
+    older ones; a read during the scope (such as the validator's lamport lookup)
+    caches an older KV block; rollback removes the newest block. The next insert of
+    the same change then panicked with "counter should be continuous".
+  - `ChangeStore::rollback_import` therefore evicts the flushed blocks of every peer
+    the rollback touched. They reload from KV on demand.
+- **Cheap rollback records.** `ChangeStoreRollback` keeps a `BlockShape` for each
+  unflushed pre-scope block an import appended to: change count, the last change's
+  op count and its last op. Rollback truncates back to it.
+  - Imports only append (push changes, or push ops that may merge into the last op),
+    so the shape is enough.
+  - Flushed blocks need no record: their KV copy is the pre-scope version.
+  - The earlier record was an `Arc` of the whole block, which made the next append
+    copy the block's changes on every import under a scope. That made small
+    movable-list imports about 40% slower, and List/Tree imports on main already
+    paid it (they are about 45% faster now).
+
+## Known gaps (not fixed here)
+
+- A `Move` whose `from` does not point at its element is accepted. The tracker
+  removes whatever list item sits at `from`, so another element can disappear.
+  The result is the same on every path, but it does not match any honest op.
+- An explicitly detached doc that imports an op which only state validation
+  rejects (an out-of-bounds list insert, or a movable-list move out of bounds)
+  panics on `attach()`/`checkout_to_latest`. Those return `()` and `expect` the
+  checkout. This affects every container type and predates this change.
+- The oplog inside a `FastSnapshot` is not validated op by op, because that would
+  decode every block. A forged snapshot can still reach the diff calculator's
+  unwraps on a later checkout.
+- A forged change parked as pending fails the import that later unlocks it, so
+  that import is rolled back, the same as for list bounds errors.

crates/loro-internal/AGENTS.md

@@ -44,6 +44,9 @@ over graceful degradation.
   mergeable container regression tests.
 - `src/tests/import_atomicity.rs`: import rollback and malformed-input
   regressions.
+- Movable-list `Move`/`Set` element validation on import, plus change-store
+  rollback records and KV block-range decoding:
+  [../../context/movable-list-op-validation.md](../../context/movable-list-op-validation.md).
 - `import_batch` force-detach, its batch-wide rollback scope, and the
   never-exit-detached invariant:
   [../../context/import-batch-atomicity.md](../../context/import-batch-atomicity.md).

crates/loro-internal/src/container/list/list_op.rs

@@ -2,12 +2,13 @@ use std::ops::Range;
 
 use append_only_bytes::BytesSlice;
 use enum_as_inner::EnumAsInner;
-use loro_common::{ContainerType, HasId, HasIdSpan, IdLp, LoroValue, ID};
+use loro_common::{ContainerType, HasId, HasIdSpan, IdLp, LoroError, LoroResult, LoroValue, ID};
 use rle::{HasLength, Mergable, Sliceable};
 use serde::{Deserialize, Serialize};
 
 use crate::{
     container::richtext::richtext_state::unicode_to_utf8_index,
+    container::richtext::tracker::UNKNOWN_SPAN_LEN,
     container::richtext::TextStyleInfoFlag,
     op::{ListSlice, SliceRange},
     InternalString,
@@ -88,6 +89,37 @@ impl ListOp<'_> {
 }
 
 impl InnerListOp {
+    /// Reject positions that no real sequence can reach.
+    ///
+    /// Diff calculation replays ops on a tracker whose unreplayed history is one
+    /// placeholder span of [`UNKNOWN_SPAN_LEN`] items, and positions past it panic
+    /// inside the tracker. Positions are op-index based (deleted items included), so
+    /// a valid document stays many orders of magnitude below this. Smaller
+    /// out-of-bounds positions are rejected later by state validation.
+    pub(crate) fn check_positions(&self) -> LoroResult<()> {
+        const MAX_POS: i64 = UNKNOWN_SPAN_LEN as i64 - 1;
+        let in_range = |pos: i64| (0..=MAX_POS).contains(&pos);
+        let ok = match self {
+            InnerListOp::Insert { pos, .. } => in_range(*pos as i64),
+            InnerListOp::InsertText { pos, .. } => in_range(*pos as i64),
+            InnerListOp::Delete(span) => {
+                in_range(span.start() as i64) && in_range(span.last() as i64)
+            }
+            InnerListOp::Move { from, to, .. } => in_range(*from as i64) && in_range(*to as i64),
+            InnerListOp::StyleStart { start, end, .. } => {
+                in_range(*start as i64) && in_range(*end as i64)
+            }
+            InnerListOp::Set { .. } | InnerListOp::StyleEnd => true,
+        };
+        if ok {
+            Ok(())
+        } else {
+            Err(LoroError::DecodeError(
+                format!("Sequence op position is out of range: {self:?}").into_boxed_str(),
+            ))
+        }
+    }
+
     pub fn new_del(id: ID, pos: usize, len: isize) -> Self {
         assert!(len != 0);
         Self::Delete(DeleteSpanWithId {

crates/loro-internal/src/container/richtext.rs

@@ -17,7 +17,7 @@ pub(crate) mod str_slice;
 mod style_range_map;
 #[cfg(all(test, miri))]
 mod tinyvec;
-mod tracker;
+pub(crate) mod tracker;
 
 use crate::{change::Lamport, delta::StyleMeta, utils::string_slice::StringSlice, InternalString};
 use fugue_span::*;

crates/loro-internal/src/container/richtext/tracker.rs

@@ -19,6 +19,12 @@ use super::{
 
 mod crdt_rope;
 mod id_to_cursor;
+
+/// Length of the placeholder span that stands for history the tracker has not
+/// replayed (`Tracker::new_with_unknown`). Op positions must stay below it; see
+/// `InnerListOp::check_positions`.
+pub(crate) const UNKNOWN_SPAN_LEN: u32 = u32::MAX / 4;
+
 pub(crate) use crdt_rope::CrdtRopeDelta;
 
 #[derive(Debug)]
@@ -46,7 +52,7 @@ impl Tracker {
         };
 
         let result = this.rope.tree.push(FugueSpan {
-            content: RichtextChunk::new_unknown(u32::MAX / 4),
+            content: RichtextChunk::new_unknown(UNKNOWN_SPAN_LEN),
             id: IdFull::new(UNKNOWN_PEER_ID, 0, 0),
             real_id: None,
             status: Status::default(),
@@ -56,7 +62,7 @@ impl Tracker {
         });
         this.id_to_cursor.insert_without_split(
             ID::new(UNKNOWN_PEER_ID, 0),
-            id_to_cursor::Cursor::new_insert(result.leaf, u32::MAX as usize / 4),
+            id_to_cursor::Cursor::new_insert(result.leaf, UNKNOWN_SPAN_LEN as usize),
         );
         this
     }

crates/loro-internal/src/encoding/json_schema.rs

@@ -1049,6 +1049,9 @@ fn decode_op(op: json::JsonOp, arena: &SharedArena, peers: &Option<Vec<PeerID>>)
             }
         } // Note: The Future Type need try to parse Op from the unknown content
     };
+    if let InnerContent::List(list_op) = &content {
+        list_op.check_positions()?;
+    }
     Ok(Op {
         counter,
         container: idx,

crates/loro-internal/src/encoding/outdated_encode_reordered.rs

@@ -472,6 +472,10 @@ pub(crate) fn decode_op(
         }),
     };
 
+    if let crate::op::InnerContent::List(list_op) = &content {
+        list_op.check_positions()?;
+    }
+
     Ok(content)
 }
 

crates/loro-internal/src/history_cache.rs

@@ -352,6 +352,35 @@ impl ContainerHistoryCache {
         }
     }
 
+    /// Whether the movable list `idx` holds `elem_id` in the shallow root state.
+    ///
+    /// Elements created before the shallow root cannot be resolved from the trimmed
+    /// history; the checkout index only knows the ones still alive at the root
+    /// (`MovableListHistoryCache::record_shallow_root_state`).
+    pub(crate) fn shallow_root_has_movable_list_elem(
+        &self,
+        idx: ContainerIdx,
+        elem_id: IdLp,
+    ) -> bool {
+        let Some(state) = self.shallow_root_state.as_ref() else {
+            return false;
+        };
+        let mut store = state.store.lock();
+        let Some(c) = store.get_mut(idx) else {
+            return false;
+        };
+        let ctx = ContainerCreationContext {
+            configure: &Default::default(),
+            peer: 0,
+        };
+        match c.get_state_mut(idx, ctx) {
+            crate::state::State::MovableListState(l) => {
+                l.elements().contains_key(&elem_id.compact())
+            }
+            _ => false,
+        }
+    }
+
     pub(crate) fn find_text_chunks_in(
         &self,
         idx: ContainerIdx,