From f96139f0df00ee54f8714a94a26717076afe81cf Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 09:56:34 +0800 Subject: [PATCH 1/2] fix: drop cached container deletions that a move can undo `DocState::is_deleted` cached every deletion and kept the entries across commits and imports. A tree node under a deleted ancestor is revived by moving it (or the ancestor) out, and a movable-list element deleted by one peer is revived by a concurrent move with a greater lamport, so after an `is_deleted()` query the revived metadata or child stayed "deleted" in release builds and rejected edits until the document was reloaded. Deletions cut at a Tree or MovableList edge now go into a separate set that is dropped before every local tree move and on every forward diff batch touching a tree or movable list. Map/list deletions stay cached until a checkout. Debug builds assert that no cached entry on the walked chain contradicts the result. Rationale in context/dead-container-cache.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/dead-container-cache.md | 5 + AGENTS.md | 2 + context/dead-container-cache.md | 64 +++++ crates/loro-internal/src/state.rs | 39 +-- crates/loro-internal/src/state/AGENTS.md | 6 +- .../src/state/dead_containers_cache.rs | 174 ++++++++--- .../tests/dead_container_cache.test.ts | 82 ++++++ crates/loro/tests/dead_container_cache.rs | 271 ++++++++++++++++++ 8 files changed, 591 insertions(+), 52 deletions(-) create mode 100644 .changeset/dead-container-cache.md create mode 100644 context/dead-container-cache.md create mode 100644 crates/loro-wasm/tests/dead_container_cache.test.ts create mode 100644 crates/loro/tests/dead_container_cache.rs diff --git a/.changeset/dead-container-cache.md b/.changeset/dead-container-cache.md new file mode 100644 index 000000000..766957625 --- /dev/null +++ b/.changeset/dead-container-cache.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix containers that stayed deleted after being revived. After `isDeleted()` was called on the metadata of a tree node under a deleted ancestor, moving the subtree back (locally or through an import) left that metadata reported as deleted, and edits to it threw until the document was reloaded. The same happened to a movable-list child revived by importing a concurrent move. Cached deletions of tree metadata and movable-list children are now dropped whenever a tree move or a movable-list change is applied. diff --git a/AGENTS.md b/AGENTS.md index d1c2488f8..df857e4d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -41,6 +41,8 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec. - Bounded decoded-value cache in `InnerStore` (second-chance FIFO, eviction safety contract, loro-dev/loro#1092): [context/container-value-cache.md](context/container-value-cache.md). +- Which deletions `DocState::is_deleted` caches, and how tree/movable-list + children are revived: [context/dead-container-cache.md](context/dead-container-cache.md). - Movable-list `apply_diff`/undo: delete + re-insert of a child becomes a move, cursor invariant, pitfalls: [context/movable-list-apply-diff.md](context/movable-list-apply-diff.md). - Context backlog: [context/CONTEXT-GAPS.md](context/CONTEXT-GAPS.md). diff --git a/context/dead-container-cache.md b/context/dead-container-cache.md new file mode 100644 index 000000000..a452294f8 --- /dev/null +++ b/context/dead-container-cache.md @@ -0,0 +1,64 @@ +# Dead Container Cache + +Verified against code 2026-09-28. + +`DocState::is_deleted` (`crates/loro-internal/src/state/dead_containers_cache.rs`) +answers whether a container is unreachable from a root: it walks the arena +parent chain and asks each parent whether it still holds the child +(`DocState::contains_logical_child`). It runs for every local op +(`Transaction::apply_local_op` rejects ops on deleted containers) and for each +handler's `is_deleted()`. + +## What is cached + +A deleted answer is cached for the container whose edge is broken and every +container below it on the walked chain, in one of two sets: + +- `final_deletions`: the parent that lost the chain is a Map (a key only + changes to a newer value, so a replaced or deleted child never becomes the + value again) or a List (deletes are final). No later version gives these + back. +- `revivable`: the parent is a Tree or a MovableList. Moving a node, or one of + its ancestors, out of a deleted subtree revives the node's metadata and + everything below it (a local `mov`, an imported move, or a concurrent move + whose lamport is greater than the delete's). A movable-list delete + concurrent with a move of the same element keeps the element when the move + has the greater lamport, so importing the move revives it. + +Not cached: children behind a mergeable edge (the parent map's marker can come +back) and containers without a parent in the arena (a later op can still +attach them). + +Release builds return a cached `true` without walking; debug builds always +walk and assert that no cached entry on the chain contradicts the result. + +## Invalidation + +- `DocState::apply_diff` with direction mode `Checkout` clears both sets: + moving backwards can revive anything. +- `DocState::apply_diff` in any other mode clears `revivable` when the batch + touches a Tree or MovableList container. +- `DocState::apply_local_op` clears `revivable` before a tree `Move`. A local + op cannot address a deleted movable-list element. +- Resets replace the cache. + +`clear_revivable` is O(1) when the set is empty, so documents that never query +deleted containers pay nothing. + +Undo, redo, `revert_to`, and `apply_diff` do not revive container IDs by +themselves: `Handler::apply_diff` creates a new tree node for a deleted target +and remaps its metadata, and map/list children are recreated with new IDs. +Their tree moves still go through `apply_local_op`. Only mergeable children +keep their IDs, and those are not cached. + +Before 2026-09-28 every deletion went into one set that forward transitions +never cleared (`clear_alive` removed nothing, since only deletions were +stored), so after `is_deleted()` a revived container still counted as deleted +in release builds and rejected edits until the document was reloaded. + +## Tests + +- `crates/loro/tests/dead_container_cache.rs`: every revival route. Run it with + `--release` as well; debug builds recompute the answer. +- Unit tests in `dead_containers_cache.rs` check the cache entries directly, + so they also fail in debug builds when an invalidation is missing. diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index 3c65bbba5..316934515 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -18,7 +18,7 @@ use tracing::{info_span, instrument, warn}; use crate::{ configure::{Configure, DefaultRandom, SecureRandomGenerator}, - container::{idx::ContainerIdx, richtext::config::StyleConfigMap}, + container::{idx::ContainerIdx, richtext::config::StyleConfigMap, tree::tree_op::TreeOp}, cursor::{Cursor, PosType}, delta::TreeExternalDiff, diff_calc::{DiffCalculator, DiffMode}, @@ -27,7 +27,7 @@ use crate::{ handler::ValueOrHandler, id::PeerID, lock::{LoroLockGroup, LoroMutex}, - op::{Op, RawOp}, + op::{Op, RawOp, RawOpContent}, version::{Frontiers, VersionVector}, ContainerDiff, ContainerType, DocDiff, InternalString, LoroDocInner, LoroValue, OpLog, }; @@ -675,17 +675,19 @@ impl DocState { } // `diff_mode` here is the DIRECTION mode (`origin_diff_mode` from // `calc_diff_internal`), not the mode the calculators computed with: - // Checkout means the transition may go backwards, so any cached - // dead/alive knowledge can be invalidated; every other mode implies a - // forward transition, where alive-markers may change but dead - // containers stay dead unless a diff revives them. - match diff_mode { - DiffMode::Checkout => { - self.dead_containers_cache.clear(); - } - _ => { - self.dead_containers_cache.clear_alive(); - } + // Checkout means the transition may go backwards, which can revive any + // container. Every other mode implies a forward transition, where only + // a tree or movable-list move can revive one (see + // `dead_containers_cache.rs`). + if diff_mode == DiffMode::Checkout { + self.dead_containers_cache.clear(); + } else if diffs.iter().any(|d| { + matches!( + d.idx.get_type(), + ContainerType::Tree | ContainerType::MovableList + ) + }) { + self.dead_containers_cache.clear_revivable(); } self.pre_txn(diff.origin.clone(), diff.by); @@ -991,11 +993,14 @@ impl DocState { if self.in_txn { self.changed_idx_in_txn.insert(op.container); } - let ret = state.apply_local_op(raw_op, op)?; - if !ret.deleted_containers.is_empty() { - self.dead_containers_cache.clear_alive(); + // A local move can take a node out of a deleted subtree. Movable-list + // elements cannot be revived locally: a deleted one has no index. + if let RawOpContent::Tree(tree_op) = &raw_op.content { + if matches!(**tree_op, TreeOp::Move { .. }) { + self.dead_containers_cache.clear_revivable(); + } } - + state.apply_local_op(raw_op, op)?; Ok(()) } diff --git a/crates/loro-internal/src/state/AGENTS.md b/crates/loro-internal/src/state/AGENTS.md index 60e6c9a28..d35af4227 100644 --- a/crates/loro-internal/src/state/AGENTS.md +++ b/crates/loro-internal/src/state/AGENTS.md @@ -22,8 +22,10 @@ before changing mergeable child behavior. [../../../../context/shallow-snapshot-style-redaction.md](../../../../context/shallow-snapshot-style-redaction.md) before changing the richtext snapshot codec or that pass. - `mergeable.rs`: logical child edge resolution for mergeable containers. -- `dead_containers_cache.rs`: dead/alive tracking and marker-driven mergeable - reactivation. +- `dead_containers_cache.rs`: dead/alive tracking. Map/list deletions are + final; tree and movable-list ones are dropped whenever a move could revive + them; mergeable ones are never cached: + [../../../../context/dead-container-cache.md](../../../../context/dead-container-cache.md). - `unknown_state.rs` and `../diff_calc/unknown.rs`: forward compatibility for unknown container types. diff --git a/crates/loro-internal/src/state/dead_containers_cache.rs b/crates/loro-internal/src/state/dead_containers_cache.rs index 3a15e3fa2..f90158d31 100644 --- a/crates/loro-internal/src/state/dead_containers_cache.rs +++ b/crates/loro-internal/src/state/dead_containers_cache.rs @@ -1,20 +1,37 @@ use super::DocState; use crate::container::idx::ContainerIdx; -use rustc_hash::FxHashMap; +use loro_common::ContainerType; +use rustc_hash::FxHashSet; use smallvec::SmallVec; +/// Containers known to be deleted. See `context/dead-container-cache.md`. #[derive(Default, Debug, Clone)] pub(super) struct DeadContainersCache { - cache: FxHashMap, + /// Removed from a Map or List: no later version gives them back. + final_deletions: FxHashSet, + /// Cut off at a Tree or MovableList edge: a later move can revive them. + revivable: FxHashSet, } impl DeadContainersCache { + /// Must be called whenever the state may move to a version that is not a + /// descendant of the current one (checkout, reset), because that can + /// revive any container. pub fn clear(&mut self) { - self.cache.clear(); + self.final_deletions.clear(); + self.revivable.clear(); } - pub(crate) fn clear_alive(&mut self) { - self.cache.retain(|_, is_deleted| *is_deleted); + /// Must be called whenever a tree or movable-list change is applied going + /// forward, because it may move a deleted node or element back. + pub fn clear_revivable(&mut self) { + if !self.revivable.is_empty() { + self.revivable.clear(); + } + } + + fn contains(&self, idx: &ContainerIdx) -> bool { + self.final_deletions.contains(idx) || self.revivable.contains(idx) } } @@ -22,8 +39,7 @@ impl DocState { pub(crate) fn is_deleted(&mut self, idx: ContainerIdx) -> bool { #[cfg(not(debug_assertions))] { - // Cache stores only deleted containers. - if self.dead_containers_cache.cache.contains_key(&idx) { + if self.dead_containers_cache.contains(&idx) { return true; } } @@ -34,14 +50,15 @@ impl DocState { visited.push(idx); let mut idx = idx; let mut depends_on_mergeable_edge = false; - let is_deleted = loop { + // The parent that no longer holds the chain, if any. + let (is_deleted, cut_at) = loop { let id = self.arena.idx_to_id(idx).unwrap(); if id.is_mergeable() { depends_on_mergeable_edge = true; } if let Some(parent_idx) = self.arena.get_parent(idx) { if !self.contains_logical_child(parent_idx, &id) { - break true; + break (true, Some(parent_idx)); } idx = parent_idx; @@ -49,39 +66,34 @@ impl DocState { } else { // No parent in the arena: top-level Roots are always alive; anything else // (including a mergeable Root whose parent edge was never wired) is treated - // as deleted. - break !id.is_root() || id.is_mergeable(); + // as deleted. A later op can still attach it, so this is never cached. + break (!id.is_root() || id.is_mergeable(), None); } }; + // Every container on the walked chain shares the answer: below the + // missing edge all of them are deleted, otherwise all are alive. #[cfg(debug_assertions)] - { - if !depends_on_mergeable_edge { - if let Some(cached_is_deleted) = self.dead_containers_cache.cache.get(&idx) { - assert_eq!(is_deleted, *cached_is_deleted); - } - } + for idx in visited.iter() { + assert!( + is_deleted || !self.dead_containers_cache.contains(idx), + "stale dead-container cache entry for {:?}", + self.arena.idx_to_id(*idx) + ); } // A mergeable ancestor can be deleted and later reactivated by changing the parent map's // marker. Do not cache deletion for any descendant whose liveness depends on that // logical edge, including ordinary children nested inside a mergeable map. - if depends_on_mergeable_edge { - if !is_deleted { - for idx in visited { - self.dead_containers_cache.cache.remove(&idx); + if is_deleted && !depends_on_mergeable_edge { + match cut_at.map(|parent| parent.get_type()) { + Some(ContainerType::Map | ContainerType::List) => { + self.dead_containers_cache.final_deletions.extend(visited) } - } - return is_deleted; - } - - if is_deleted { - for idx in visited { - self.dead_containers_cache.cache.insert(idx, true); - } - } else { - for idx in visited { - self.dead_containers_cache.cache.remove(&idx); + Some(ContainerType::Tree | ContainerType::MovableList) => { + self.dead_containers_cache.revivable.extend(visited) + } + _ => {} } } @@ -90,7 +102,7 @@ impl DocState { #[cfg(test)] pub(crate) fn dead_cache_entry(&self, idx: ContainerIdx) -> Option { - self.dead_containers_cache.cache.get(&idx).copied() + self.dead_containers_cache.contains(&idx).then_some(true) } } @@ -243,4 +255,100 @@ mod tests { "imported reactivation must drop the stale deleted-cache entry" ); } + + /// A tree node under a deleted ancestor is revived by moving it out, so a + /// local move must drop the cached deletion of its metadata. + #[test] + fn local_tree_move_drops_revivable_deletions() { + use crate::state::TreeParentId; + + let doc = LoroDoc::new_auto_commit(); + doc.set_peer_id(1).unwrap(); + let tree = doc.get_tree("tree"); + let parent = tree.create(TreeParentId::Root).unwrap(); + let child = tree.create(TreeParentId::Node(parent)).unwrap(); + let meta = tree.get_meta(child).unwrap(); + doc.commit_then_renew(); + tree.delete(parent).unwrap(); + doc.commit_then_renew(); + + let idx = doc.state.lock().arena.id_to_idx(&meta.id()).unwrap(); + assert!(meta.is_deleted()); + assert_eq!(doc.state.lock().dead_cache_entry(idx), Some(true)); + + tree.mov(child, TreeParentId::Root).unwrap(); + assert_eq!(doc.state.lock().dead_cache_entry(idx), None); + doc.commit_then_renew(); + assert!(!meta.is_deleted()); + } + + /// A movable-list element deleted locally is revived by a concurrent move + /// with a greater lamport, so importing movable-list changes must drop the + /// cached deletion. + #[test] + fn movable_list_import_drops_revivable_deletions() { + use crate::{loro::ExportMode, MapHandler}; + + let doc = LoroDoc::new_auto_commit(); + doc.set_peer_id(1).unwrap(); + let list = doc.get_movable_list("list"); + list.insert(0, 0).unwrap(); + let child = list + .insert_container(1, MapHandler::new_detached()) + .unwrap(); + doc.commit_then_renew(); + let other = LoroDoc::new_auto_commit(); + other.set_peer_id(2).unwrap(); + other + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + other.get_map("pad").insert("k", 1).unwrap(); + other.commit_then_renew(); + other.get_movable_list("list").mov(1, 0).unwrap(); + other.commit_then_renew(); + + list.delete(1, 1).unwrap(); + doc.commit_then_renew(); + let idx = doc.state.lock().arena.id_to_idx(&child.id()).unwrap(); + assert!(child.is_deleted()); + assert_eq!(doc.state.lock().dead_cache_entry(idx), Some(true)); + + doc.import(&other.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(doc.state.lock().dead_cache_entry(idx), None); + assert!(!child.is_deleted()); + } + + /// Map and list removals are final for every later version, so they are + /// cached for the child and everything below it. + #[test] + fn map_and_list_child_deletions_are_cached() { + use crate::{ListHandler, MapHandler}; + + let doc = LoroDoc::new_auto_commit(); + doc.set_peer_id(1).unwrap(); + let map = doc.get_map("map"); + let in_map = map + .insert_container("k", MapHandler::new_detached()) + .unwrap(); + let nested = in_map + .insert_container("t", TextHandler::new_detached()) + .unwrap(); + let list = doc.get_list("list"); + let in_list = list + .insert_container(0, ListHandler::new_detached()) + .unwrap(); + doc.commit_then_renew(); + map.delete("k").unwrap(); + list.delete(0, 1).unwrap(); + doc.commit_then_renew(); + + assert!(nested.is_deleted()); + assert!(in_list.is_deleted()); + let state = doc.state.lock(); + for id in [in_map.id(), nested.id(), in_list.id()] { + let idx = state.arena.id_to_idx(&id).unwrap(); + assert_eq!(state.dead_cache_entry(idx), Some(true), "{id:?}"); + } + } } diff --git a/crates/loro-wasm/tests/dead_container_cache.test.ts b/crates/loro-wasm/tests/dead_container_cache.test.ts new file mode 100644 index 000000000..407245580 --- /dev/null +++ b/crates/loro-wasm/tests/dead_container_cache.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc, LoroMap } from "../bundler/index"; + +// A container that was reported deleted must count as alive again once it is +// revived. See context/dead-container-cache.md. + +function deletedSubtree() { + const doc = new LoroDoc(); + doc.setPeerId(1n); + const tree = doc.getTree("tree"); + const parent = tree.createNode(); + const child = tree.createNode(parent.id); + const grandchild = tree.createNode(child.id); + doc.commit(); + tree.delete(parent.id); + doc.commit(); + + const meta = tree.getNodeByID(grandchild.id)!.data; + expect(meta.isDeleted()).toBe(true); + expect(() => { + meta.set("k", 1); + doc.commit(); + }).toThrow(); + return { doc, tree, child, grandchild, meta }; +} + +describe("dead container cache", () => { + it("revives tree metadata after a local move", () => { + const { doc, tree, child, grandchild, meta } = deletedSubtree(); + tree.move(child.id, undefined, 0); + doc.commit(); + + expect(tree.isNodeDeleted(grandchild.id)).toBe(false); + expect(meta.isDeleted()).toBe(false); + meta.set("key", 1); + doc.commit(); + expect(tree.getNodeByID(grandchild.id)!.data.get("key")).toBe(1); + }); + + it("revives tree metadata after an imported move", () => { + const { doc, child, grandchild, meta } = deletedSubtree(); + const other = new LoroDoc(); + other.setPeerId(2n); + other.import(doc.export({ mode: "update" })); + other.getTree("tree").move(child.id, undefined, 0); + other.commit(); + + doc.import(other.export({ mode: "update", from: doc.oplogVersion() })); + expect(doc.getTree("tree").isNodeDeleted(grandchild.id)).toBe(false); + expect(meta.isDeleted()).toBe(false); + meta.set("key", 1); + doc.commit(); + }); + + it("revives a movable list child after importing a concurrent move", () => { + const a = new LoroDoc(); + a.setPeerId(1n); + const list = a.getMovableList("list"); + list.insert(0, 0); + const child = list.insertContainer(1, new LoroMap()); + child.set("x", 1); + a.commit(); + + const b = new LoroDoc(); + b.setPeerId(2n); + b.import(a.export({ mode: "update" })); + b.getMap("pad").set("k", 1); + b.commit(); + b.getMovableList("list").move(1, 0); + b.commit(); + + list.delete(1, 1); + a.commit(); + expect(child.isDeleted()).toBe(true); + + a.import(b.export({ mode: "update" })); + expect(list.length).toBe(2); + expect(child.isDeleted()).toBe(false); + child.set("y", 2); + a.commit(); + }); +}); diff --git a/crates/loro/tests/dead_container_cache.rs b/crates/loro/tests/dead_container_cache.rs new file mode 100644 index 000000000..d83b00833 --- /dev/null +++ b/crates/loro/tests/dead_container_cache.rs @@ -0,0 +1,271 @@ +//! A container's "deleted" answer must follow every way the container can be +//! revived. `DocState` caches deletions (`dead_containers_cache.rs`); these +//! tests query `is_deleted()` first so that an entry exists, then revive the +//! container and check both the answer and that edits are accepted. +//! +//! On `main` the cache kept the stale answer in release builds (debug builds +//! recompute and only cross-check one entry), so run these with `--release` +//! as well. + +use loro::{ + ContainerTrait, ExportMode, Frontiers, LoroDoc, LoroMap, LoroMovableList, LoroTree, TreeID, + TreeParentId, UndoManager, +}; + +struct DeletedSubtree { + doc: LoroDoc, + tree: LoroTree, + child: TreeID, + grandchild: TreeID, + /// The metadata of `grandchild`, queried while its grandparent is deleted. + meta: LoroMap, + alive_version: Frontiers, +} + +/// `parent -> child -> grandchild`, with `parent` deleted and the deletion of +/// `grandchild`'s metadata already observed. +fn deleted_subtree(peer: u64) -> DeletedSubtree { + let doc = LoroDoc::new(); + doc.set_peer_id(peer).unwrap(); + let tree = doc.get_tree("tree"); + let parent = tree.create(TreeParentId::Root).unwrap(); + let child = tree.create(parent).unwrap(); + let grandchild = tree.create(child).unwrap(); + tree.get_meta(grandchild).unwrap().insert("v", 0).unwrap(); + doc.commit(); + let alive_version = doc.state_frontiers(); + tree.delete(parent).unwrap(); + doc.commit(); + + let meta = tree.get_meta(grandchild).unwrap(); + assert!(meta.is_deleted()); + assert!(meta.insert("k", 1).is_err()); + DeletedSubtree { + doc, + tree, + child, + grandchild, + meta, + alive_version, + } +} + +fn assert_meta_alive_and_editable(s: &DeletedSubtree) { + assert!(!s.tree.is_node_deleted(&s.grandchild).unwrap()); + assert!(!s.meta.is_deleted()); + s.meta.insert("after", 1).unwrap(); + s.doc.commit(); + assert_eq!( + s.tree + .get_meta(s.grandchild) + .unwrap() + .get("after") + .unwrap() + .into_value() + .unwrap(), + 1.into() + ); +} + +#[test] +fn local_move_revives_metadata_after_is_deleted_query() { + let s = deleted_subtree(1); + s.tree.mov(s.child, TreeParentId::Root).unwrap(); + s.doc.commit(); + assert_meta_alive_and_editable(&s); +} + +#[test] +fn move_and_edit_in_one_transaction_after_is_deleted_query() { + let s = deleted_subtree(1); + s.tree.mov(s.child, TreeParentId::Root).unwrap(); + s.meta.insert("k", 1).unwrap(); + s.doc.commit(); + assert_meta_alive_and_editable(&s); +} + +#[test] +fn imported_move_revives_metadata_after_is_deleted_query() { + let s = deleted_subtree(1); + let other = LoroDoc::new(); + other.set_peer_id(2).unwrap(); + other + .import(&s.doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + other + .get_tree("tree") + .mov(s.child, TreeParentId::Root) + .unwrap(); + other.commit(); + + s.doc + .import( + &other + .export(ExportMode::updates(&s.doc.oplog_vv())) + .unwrap(), + ) + .unwrap(); + assert_meta_alive_and_editable(&s); +} + +#[test] +fn concurrent_move_revives_metadata_after_is_deleted_query() { + // The delete and the move are concurrent; the move has the greater lamport, + // so importing it revives the subtree. + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let tree = doc.get_tree("tree"); + let parent = tree.create(TreeParentId::Root).unwrap(); + let child = tree.create(parent).unwrap(); + doc.commit(); + let other = LoroDoc::new(); + other.set_peer_id(2).unwrap(); + other + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + tree.delete(parent).unwrap(); + doc.commit(); + let meta = tree.get_meta(child).unwrap(); + assert!(meta.is_deleted()); + + let other_tree = other.get_tree("tree"); + other_tree.create(TreeParentId::Root).unwrap(); + other.commit(); + other_tree.mov(parent, TreeParentId::Root).unwrap(); + other.commit(); + doc.import(&other.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + assert!(!tree.is_node_deleted(&child).unwrap()); + assert!(!meta.is_deleted()); + meta.insert("k", 1).unwrap(); + doc.commit(); +} + +/// `node` moved under a deleted node, with the deletion of its metadata +/// already observed. +fn node_moved_into_deleted_subtree(doc: &LoroDoc) -> (LoroTree, TreeID, LoroMap, Frontiers) { + let tree = doc.get_tree("tree"); + let deleted = tree.create(TreeParentId::Root).unwrap(); + let node = tree.create(TreeParentId::Root).unwrap(); + tree.get_meta(node).unwrap().insert("v", 1).unwrap(); + doc.commit(); + tree.delete(deleted).unwrap(); + doc.commit(); + let alive_version = doc.state_frontiers(); + tree.mov(node, deleted).unwrap(); + doc.commit(); + + let meta = tree.get_meta(node).unwrap(); + assert!(meta.is_deleted()); + assert!(meta.insert("k", 1).is_err()); + (tree, node, meta, alive_version) +} + +/// Undo and `revert_to` go through `Handler::apply_diff`, which never moves a +/// deleted node back: it creates a new node and copies the metadata. The old +/// metadata stays deleted, and the new one is alive. +fn assert_recreated_as_new_node(tree: &LoroTree, node: TreeID, meta: &LoroMap) { + assert!(tree.is_node_deleted(&node).unwrap()); + assert!(meta.is_deleted()); + assert!(meta.insert("k", 1).is_err()); + + let roots = tree.roots(); + assert_eq!(roots.len(), 1); + assert_ne!(roots[0], node); + let new_meta = tree.get_meta(roots[0]).unwrap(); + assert!(!new_meta.is_deleted()); + assert_eq!(new_meta.get("v").unwrap().into_value().unwrap(), 1.into()); + new_meta.insert("k", 1).unwrap(); +} + +#[test] +fn undo_after_is_deleted_query_recreates_node() { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let mut undo = UndoManager::new(&doc); + let (tree, node, meta, _) = node_moved_into_deleted_subtree(&doc); + + assert!(undo.undo().unwrap()); + assert_recreated_as_new_node(&tree, node, &meta); + doc.commit(); +} + +#[test] +fn revert_to_after_is_deleted_query_recreates_node() { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let (tree, node, meta, alive_version) = node_moved_into_deleted_subtree(&doc); + + doc.revert_to(&alive_version).unwrap(); + doc.commit(); + assert_recreated_as_new_node(&tree, node, &meta); + doc.commit(); +} + +#[test] +fn checkout_revives_metadata_after_is_deleted_query() { + let s = deleted_subtree(1); + s.doc.checkout(&s.alive_version).unwrap(); + assert!(!s.tree.is_node_deleted(&s.grandchild).unwrap()); + assert!(!s.meta.is_deleted()); + + s.doc.checkout_to_latest(); + assert!(s.meta.is_deleted()); +} + +#[test] +fn imported_movable_list_move_revives_child_after_is_deleted_query() { + // Loro keeps a movable-list element deleted by one peer if another peer + // concurrently moves it with a greater lamport. + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + let list = a.get_movable_list("list"); + list.insert(0, 0).unwrap(); + let child = list.insert_container(1, LoroMap::new()).unwrap(); + child.insert("x", 1).unwrap(); + a.commit(); + + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + b.get_map("pad").insert("k", 1).unwrap(); + b.commit(); + b.get_movable_list("list").mov(1, 0).unwrap(); + b.commit(); + + list.delete(1, 1).unwrap(); + a.commit(); + assert!(child.is_deleted()); + assert!(child.insert("y", 1).is_err()); + + a.import(&b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(list.len(), 2); + assert!(!child.is_deleted()); + child.insert("y", 2).unwrap(); + a.commit(); +} + +#[test] +fn map_and_list_deletions_stay_deleted() { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + let map = doc.get_map("map"); + let in_map = map.insert_container("k", LoroMap::new()).unwrap(); + let list = doc.get_list("list"); + let in_list = list.insert_container(0, LoroMovableList::new()).unwrap(); + doc.commit(); + map.delete("k").unwrap(); + list.delete(0, 1).unwrap(); + doc.commit(); + + for _ in 0..2 { + assert!(in_map.is_deleted()); + assert!(in_list.is_deleted()); + assert!(in_map.insert("x", 1).is_err()); + assert!(in_list.insert(0, 1).is_err()); + } +} From 77a75f3a471c5330ef653877ce8868ee90061b4c Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 18:30:25 +0800 Subject: [PATCH 2/2] fix: clear the dead-container cache on every checkout `DocState::apply_diff` cleared the cache only when the direction mode was `Checkout`, but forward checkouts (`attach`, `checkout_to_latest`, a checkout to a newer version) use `Import`/`Linear` modes. A container queried while the state was behind the oplog, created after the state's version, is cut at its map or list parent and was cached as a final deletion; it stayed "deleted" after going forward, and debug builds hit the stale-entry assertion. Clear on any checkout too (`EventTriggerKind::Checkout`). Rationale in context/dead-container-cache.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- context/dead-container-cache.md | 24 ++++-- crates/loro-internal/src/state.rs | 11 ++- .../src/state/dead_containers_cache.rs | 29 +++++++ .../tests/dead_container_cache.test.ts | 44 ++++++++++- crates/loro/tests/dead_container_cache.rs | 77 ++++++++++++++++++- 5 files changed, 173 insertions(+), 12 deletions(-) diff --git a/context/dead-container-cache.md b/context/dead-container-cache.md index a452294f8..5e42fd44a 100644 --- a/context/dead-container-cache.md +++ b/context/dead-container-cache.md @@ -16,8 +16,11 @@ container below it on the walked chain, in one of two sets: - `final_deletions`: the parent that lost the chain is a Map (a key only changes to a newer value, so a replaced or deleted child never becomes the - value again) or a List (deletes are final). No later version gives these - back. + value again) or a List (deletes are final). While the state is at the + oplog's latest version, no later version gives these back. While it is + behind (after `checkout`, or detached while imports arrive), a container + created after the state's version is also cut at its map or list parent, + and the next checkout forward brings it in. - `revivable`: the parent is a Tree or a MovableList. Moving a node, or one of its ancestors, out of a deleted subtree revives the node's metadata and everything below it (a local `mov`, an imported move, or a concurrent move @@ -34,8 +37,13 @@ walk and assert that no cached entry on the chain contradicts the result. ## Invalidation -- `DocState::apply_diff` with direction mode `Checkout` clears both sets: - moving backwards can revive anything. +- `DocState::apply_diff` clears both sets when the direction mode is + `Checkout` (moving backwards can revive anything) or the diff comes from a + checkout (`EventTriggerKind::Checkout`). The latter covers forward + checkouts (`attach`, `checkout_to_latest`, `checkout` to a newer version, + and `import_batch`'s reattach), whose direction mode is `Import`, `Linear`, + or `ImportGreaterUpdates`. The only checkout triggered as `Import` is the + snapshot import into an empty document, whose state is new. - `DocState::apply_diff` in any other mode clears `revivable` when the batch touches a Tree or MovableList container. - `DocState::apply_local_op` clears `revivable` before a tree `Move`. A local @@ -54,7 +62,13 @@ keep their IDs, and those are not cached. Before 2026-09-28 every deletion went into one set that forward transitions never cleared (`clear_alive` removed nothing, since only deletions were stored), so after `is_deleted()` a revived container still counted as deleted -in release builds and rejected edits until the document was reloaded. +in release builds and rejected edits until the document was reloaded. The +same held for a container queried at an older version and brought in by a +forward checkout. + +The debug assertion runs while the caller holds the state lock, so a stale +entry aborts the process (the unwinding code locks the poisoned state again), +like any other internal assertion under that lock. ## Tests diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index cf3355b9e..7bb0db897 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -682,10 +682,13 @@ impl DocState { // `diff_mode` here is the DIRECTION mode (`origin_diff_mode` from // `calc_diff_internal`), not the mode the calculators computed with: // Checkout means the transition may go backwards, which can revive any - // container. Every other mode implies a forward transition, where only - // a tree or movable-list move can revive one (see - // `dead_containers_cache.rs`). - if diff_mode == DiffMode::Checkout { + // container. Any checkout also clears it, even a forward one + // (`attach`, `checkout_to_latest`): while the state was behind the + // oplog, a container created later looked cut at its map or list + // parent. Every other forward transition keeps the state at the + // oplog's latest version, where only a tree or movable-list move can + // revive a container (see `dead_containers_cache.rs`). + if diff_mode == DiffMode::Checkout || diff.by == EventTriggerKind::Checkout { self.dead_containers_cache.clear(); } else if diffs.iter().any(|d| { matches!( diff --git a/crates/loro-internal/src/state/dead_containers_cache.rs b/crates/loro-internal/src/state/dead_containers_cache.rs index f90158d31..d3382fa7a 100644 --- a/crates/loro-internal/src/state/dead_containers_cache.rs +++ b/crates/loro-internal/src/state/dead_containers_cache.rs @@ -351,4 +351,33 @@ mod tests { assert_eq!(state.dead_cache_entry(idx), Some(true), "{id:?}"); } } + + /// While checked out to an older version, a map child created later is + /// cut at its map parent and cached as a final deletion. A forward + /// checkout is not a `DiffMode::Checkout` transition, but it must still + /// drop that entry. + #[test] + fn forward_checkout_drops_deletions_cached_behind_the_oplog() { + use crate::MapHandler; + + let doc = LoroDoc::new_auto_commit(); + doc.set_peer_id(1).unwrap(); + let map = doc.get_map("m"); + map.insert("a", 1).unwrap(); + doc.commit_then_renew(); + let v1 = doc.oplog_frontiers(); + let child = map + .insert_container("c", MapHandler::new_detached()) + .unwrap(); + doc.commit_then_renew(); + + doc.checkout(&v1).unwrap(); + let idx = doc.state.lock().arena.id_to_idx(&child.id()).unwrap(); + assert!(child.is_deleted()); + assert_eq!(doc.state.lock().dead_cache_entry(idx), Some(true)); + + doc.checkout_to_latest(); + assert_eq!(doc.state.lock().dead_cache_entry(idx), None); + assert!(!child.is_deleted()); + } } diff --git a/crates/loro-wasm/tests/dead_container_cache.test.ts b/crates/loro-wasm/tests/dead_container_cache.test.ts index 407245580..f06bc890b 100644 --- a/crates/loro-wasm/tests/dead_container_cache.test.ts +++ b/crates/loro-wasm/tests/dead_container_cache.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { LoroDoc, LoroMap } from "../bundler/index"; +import { LoroDoc, LoroMap, LoroText } from "../bundler/index"; // A container that was reported deleted must count as alive again once it is // revived. See context/dead-container-cache.md. @@ -80,3 +80,45 @@ describe("dead container cache", () => { a.commit(); }); }); + +describe("dead container cache after a forward checkout", () => { + it("drops the answer given at an older version", () => { + const doc = new LoroDoc(); + doc.setPeerId(1n); + doc.getMap("m").set("a", 1); + doc.commit(); + const v1 = doc.frontiers(); + const child = doc.getMap("m").setContainer("c", new LoroMap()); + doc.commit(); + + doc.checkout(v1); + expect(child.isDeleted()).toBe(true); + doc.checkoutToLatest(); + expect(child.isDeleted()).toBe(false); + child.set("x", 1); + doc.commit(); + expect(child.get("x")).toBe(1); + }); + + it("drops the answer given while detached after attach", () => { + const a = new LoroDoc(); + a.setPeerId(1n); + a.getMap("m").set("a", 1); + a.commit(); + const b = new LoroDoc(); + b.setPeerId(2n); + b.import(a.export({ mode: "update" })); + const text = a.getMap("m").setContainer("c", new LoroText()); + a.commit(); + + b.detach(); + b.import(a.export({ mode: "update" })); + const inB = b.getContainerById(text.id) as LoroText; + expect(inB.isDeleted()).toBe(true); + b.attach(); + expect(inB.isDeleted()).toBe(false); + inB.insert(0, "x"); + b.commit(); + expect(inB.toString()).toBe("x"); + }); +}); diff --git a/crates/loro/tests/dead_container_cache.rs b/crates/loro/tests/dead_container_cache.rs index d83b00833..69682a57b 100644 --- a/crates/loro/tests/dead_container_cache.rs +++ b/crates/loro/tests/dead_container_cache.rs @@ -8,8 +8,8 @@ //! as well. use loro::{ - ContainerTrait, ExportMode, Frontiers, LoroDoc, LoroMap, LoroMovableList, LoroTree, TreeID, - TreeParentId, UndoManager, + ContainerTrait, ExportMode, Frontiers, LoroDoc, LoroMap, LoroMovableList, LoroText, LoroTree, + TreeID, TreeParentId, UndoManager, }; struct DeletedSubtree { @@ -269,3 +269,76 @@ fn map_and_list_deletions_stay_deleted() { assert!(in_list.insert(0, 1).is_err()); } } + +/// While checked out to an older version, a container created later is +/// reported deleted (it is cut at its map parent in that state). Going +/// forward again, by `checkout_to_latest`, `attach`, or a checkout to a newer +/// version, must drop that answer. +#[test] +fn forward_checkout_revives_containers_created_after_the_old_version() { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + doc.get_map("m").insert("a", 1).unwrap(); + let tree = doc.get_tree("tree"); + let node = tree.create(TreeParentId::Root).unwrap(); + doc.commit(); + let v1 = doc.oplog_frontiers(); + let child = doc + .get_map("m") + .insert_container("c", LoroMap::new()) + .unwrap(); + let title = tree + .get_meta(node) + .unwrap() + .insert_container("title", LoroText::new()) + .unwrap(); + doc.commit(); + let v2 = doc.oplog_frontiers(); + doc.get_map("m").insert("b", 2).unwrap(); + doc.commit(); + + for forward in [ + |doc: &LoroDoc, _: &Frontiers| doc.checkout_to_latest(), + |doc: &LoroDoc, _: &Frontiers| doc.attach(), + |doc: &LoroDoc, v2: &Frontiers| doc.checkout(v2).unwrap(), + ] { + doc.checkout(&v1).unwrap(); + assert!(child.is_deleted()); + assert!(title.is_deleted()); + forward(&doc, &v2); + assert!(!child.is_deleted()); + assert!(!title.is_deleted()); + } + + doc.checkout_to_latest(); + child.insert("x", 1).unwrap(); + title.insert(0, "x").unwrap(); + doc.commit(); +} + +#[test] +fn attach_after_detached_import_revives_containers_queried_while_detached() { + let a = LoroDoc::new(); + a.set_peer_id(1).unwrap(); + a.get_map("m").insert("a", 1).unwrap(); + a.commit(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + let text = a + .get_map("m") + .insert_container("c", LoroText::new()) + .unwrap(); + a.commit(); + + b.detach(); + b.import(&a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + let in_b = b.get_text(text.id()); + assert!(in_b.is_deleted(), "not in b's detached state yet"); + b.attach(); + assert!(!in_b.is_deleted()); + in_b.insert(0, "x").unwrap(); + b.commit(); +}