Skip to content

fix: drop cached container deletions that a move can undo - #1153

Merged
zxch3n merged 3 commits into
mainfrom
fix/dead-container-cache-revival
Sep 28, 2026
Merged

zxch3n merged 3 commits into
mainfrom
fix/dead-container-cache-revival

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

After isDeleted() was called on the metadata of a tree node under a deleted ancestor, moving the subtree back left that metadata reported as deleted: isNodeDeleted returned false, data.isDeleted() returned true, and edits threw ContainerDeleted until the document was reloaded. Found by the loro.js differential fuzz (#1141, "Rust-side findings"; related to the revival semantics in #1119).

The same happened for an imported move that revives the subtree, a concurrent move with a greater lamport than the delete, a movable-list child revived by importing a concurrent move, and (review finding, fixed in 77a75f3) a container queried at an older version or while detached and brought in by a forward checkout (checkout(old) → isDeleted() → checkoutToLatest(), or detach → import → query → attach).

Root cause

DocState::is_deleted caches deleted containers (dead_containers_cache.rs), and release builds return a cached true without walking. DocState::apply_diff cleared the cache only for Checkout; for forward transitions it called clear_alive(), which retained deleted entries (and since the cache only ever stored true, did nothing). apply_local_op did the same. The assumption was that deleted containers stay deleted going forward, which is false for:

  • tree metadata: moving a node, or one of its ancestors, out of a deleted subtree revives it (local mov, imported move, concurrent move);
  • movable-list children: a delete concurrent with a move of the same element keeps the element when the move has the greater lamport;
  • containers without a parent in the arena yet (a later op can attach them);
  • containers created after the state's version while the state is behind the oplog: they are cut at their map or list parent in that state, and the next forward checkout brings them in. Forward checkouts (attach, checkout_to_latest, a checkout to a newer version) have an Import/Linear/ImportGreaterUpdates direction mode, not Checkout.

Debug builds recompute the answer, and their cross-check only compared the last container on the walked chain, so the stale entry went unnoticed in tests.

Fix

Deletions are cached in two sets, by the kind of parent that no longer holds the chain:

  • final (Map or List parent): a map key only moves to a newer value and list deletes are final, so these stay cached until a checkout.
  • revivable (Tree or MovableList parent): dropped before every local tree Move and on every forward diff batch that touches a tree or movable list. Dropping is O(1) when nothing was queried.

Any checkout clears both: the direction mode is Checkout or the diff comes from a checkout (EventTriggerKind::Checkout, which covers forward checkouts and import_batch's reattach). The only checkout triggered as Import is a snapshot import into an empty document, whose state is new. Children behind a mergeable edge are still never cached, and neither are containers with no parent in the arena yet (a later op can attach them; they were cached before). clear_alive is gone: the cache only ever stored deletions, so it removed nothing. Debug builds now assert that no cached entry anywhere on the walked chain contradicts the result; before, they compared only the chain's last container, which is why debug tests never saw the stale entry. The assertion runs while the caller holds the state lock, so when it fires the process aborts (unwinding locks the poisoned state again), like any other internal assertion under that lock; it is an internal-invariant check (fail fast), and with 77a75f3 the reviewer's harness no longer reaches it.

Revival routes checked:

Route Same container ID revived? Result
local mov out of a deleted subtree yes fixed (revivable set dropped)
imported move / concurrent move with greater lamport yes fixed (revivable set dropped)
imported movable-list move beating a delete yes fixed (revivable set dropped)
checkout backwards yes both sets cleared (unchanged)
forward checkout (checkoutToLatest, attach, newer version) after a query at an older version or while detached yes both sets cleared (added in 77a75f3; before, a map/list-cut answer survived)
undo / redo / revertTo / applyDiff no: Handler::apply_diff creates a new tree node for a deleted target and remaps its metadata; map/list children get new IDs. Their tree moves still go through apply_local_op tests pin the old metadata staying deleted and the new one editable
mergeable reactivation yes never cached (unchanged)

Rationale: context/dead-container-cache.md, linked from the root and state/ AGENTS.md.

Tests

  • crates/loro/tests/dead_container_cache.rs: local move (also with the edit in the same transaction), imported move, concurrent move, undo, revert_to, checkout, movable-list import, and map/list deletions staying cached. On main in release the local, imported, concurrent, and movable-list tests fail (debug recomputes, so run it with --release too).
  • Unit tests in dead_containers_cache.rs (local_tree_move_drops_revivable_deletions, movable_list_import_drops_revivable_deletions, map_and_list_child_deletions_are_cached) check the cache entries directly, so a missing invalidation fails in debug builds too.
  • crates/loro/tests/dead_container_cache.rs also covers the forward checkouts: checkout(v1) → query → checkout_to_latest / attach / checkout(v2) for a map child and a text in tree metadata created after v1, and detach → import → query → attach. Without the EventTriggerKind::Checkout clear they fail in release (stale true) and hit the stale-entry assertion in debug.
  • Unit test forward_checkout_drops_deletions_cached_behind_the_oplog checks the cache entry is gone after checkout_to_latest.
  • crates/loro-wasm/tests/dead_container_cache.test.ts: local move, imported move, movable list (all three fail on a WASM build of main, expected true to be false), plus the two forward-checkout cases.

Validation

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

  • cargo test -p loro -p loro-internal: pass; cargo test --release -p loro --test dead_container_cache: 11/11; unit tests in dead_containers_cache.rs: 7/7.
  • pnpm check (clippy -Dwarnings): pass.
  • cargo test -p fuzz (whole crate, exercising the stricter debug assertion): pass.
  • WASM, on a release build of main + fix: take the tree checkout window from the two versions #1152 + this PR (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 dead-container harness (after every step, isDeleted() for every container ever seen vs an independent reachability walk; queries while detached included), main + both PRs, one seed per process: release 148/150 and debug 59/60 pass; the aborts (seeds 5 and 116) are the pre-existing "Parent is not registered" panic in snapshot-loaded docs, identical on main (filed as is_deleted() aborts with "Parent is not registered" on deleted-node metadata in a snapshot-loaded doc #1158). No mismatch and no stale-entry assertion. Before 77a75f3 all 150 failed on the forward-checkout finding.
  • 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 / #1152 / this PR run alternately in 5 rounds, thread CPU time (1-min loadavg at run start 7.4–11.5). ms: median of the per-round medians, min of mins in parentheses.

scenario main this PR
isDeleted on 10k alive tree metas (depth ≤ 21) ×10 14.3 (13.3) 15.2 (13.1)
isDeleted on 10k metas under a deleted node ×10 0.632 (0.602) 0.621 (0.605)
isDeleted on 10k deleted map children ×10 0.628 (0.613) 0.628 (0.619)
reviewer: 10k deleted-meta queries ×20, no moves 1.26 (1.22) 1.29 (1.22)
reviewer: (1 local tree move + 10k deleted-meta queries) ×20 1.26 (1.22) 24.3 (23.5)
10k local edits on alive tree metas 4.14 (3.04) 3.61 (3.26)
100k-node tree: 20k local moves 31.6 (22.3) 30.1 (25.0)
100k-node tree: import 20k moves 96.2 (82.7) 89.1 (85.4)

The one slower row is expected: a local tree move drops the revivable set, so the next query on each deleted meta walks its chain again, which costs what a query on an alive meta always costs (about 0.12 µs here). main is fast there only because it returns the stale cached answer. Queries without an interleaved move stay cached.

Merge order and conflicts

Independent of #1152 (the tree checkout window fix for the other two findings); 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. #1125's note that a movable-list move of a deleted element is applied rather than rejected is the same behavior that makes movable-list deletions revivable here.

🤖 Generated with Claude Code

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

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

WASM Size Report

  • Original size: 3233.47 KB
  • Gzipped size: 1073.52 KB
  • Brotli size: 751.88 KB

zxch3n and others added 2 commits September 28, 2026 18:25
…che-revival

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`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) <noreply@anthropic.com>

zxch3n commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

Review follow-up:

  • Blocking (forward checkouts kept final_deletions): fixed in 77a75f3 with your suggested condition: DocState::apply_diff clears both sets when diff_mode == DiffMode::Checkout || diff.by == EventTriggerKind::Checkout. That covers attach, checkout_to_latest, a checkout to a newer version, and import_batch's reattach; the only checkout triggered as Import is a snapshot import into an empty document.
  • Regression tests: forward_checkout_revives_containers_created_after_the_old_version (map child and a text in tree metadata, via checkout_to_latest / attach / checkout(v2)) and attach_after_detached_import_revives_containers_queried_while_detached in dead_container_cache.rs: both fail without the fix (stale true in release, the stale-entry assertion in debug). Unit test forward_checkout_drops_deletions_cached_behind_the_oplog checks the cache entry. Two WASM tests for the same flows.
  • The PR description's route table and context/dead-container-cache.md are corrected: final deletions are final only while the state is at the oplog's latest version.
  • Debug assertion under the state lock: I kept it as a fail-fast internal-invariant check. When it fires the process aborts because unwinding locks the poisoned state again, the same as any other internal assertion under that lock (e.g. the tree_state.rs:1075 panic in fix: take the tree checkout window from the two versions #1152's repro). Your harness no longer reaches it: with queries while detached, one seed per process, release 148/150 and debug 59/60 pass; the aborts (seeds 5, 116) are the pre-existing arena.rs:462 panic, filed as is_deleted() aborts with "Parent is not registered" on deleted-node metadata in a snapshot-loaded doc #1158.
  • Non-blocking perf (1 move + 10k deleted-meta queries ×20): 24.3 ms vs main 1.26 ms, i.e. the uncached cost of the query (same as for alive metas); documented in the description. Queries without an interleaved move: 1.29 vs 1.26 ms.
  • Merged main (1eb14b3). Re-validated: cargo test -p loro -p loro-internal, pnpm check, cargo test -p fuzz, WASM package tests (vitest 401, tsc, deno, bun), cargo-fuzz tree / diff_calc, no crashes. CI green.

🤖 Generated with Claude Code

@zxch3n
zxch3n merged commit 229c8eb 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>
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