Skip to content

fix(loro.js): treat containers under deleted ancestors as deleted - #1147

Merged
zxch3n merged 6 commits into
mainfrom
fix/loro-js-deleted-ancestors
Sep 29, 2026
Merged

zxch3n merged 6 commits into
mainfrom
fix/loro-js-deleted-ancestors

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Summary

loro.js decided whether a container was deleted from its own parent binding only, and a tree node from its own deleted flag, so anything below a deleted container or tree node counted as alive. loro-crdt follows the whole chain. Differences on main, checked against a WASM build of main (c0ff1255):

After Call main loro-crdt / this PR
root.delete("m") where m holds a text text.isDeleted() false true
same doc.getPathToContainer(text.id) ["root","m","t"] undefined
tree.delete(parent) tree.getNodes() includes the children alive nodes only
same tree.isNodeDeleted(child), child.isDeleted(), grandchild.data.isDeleted() false true
any tree.nodes() alive nodes every node, including deleted ("Get all tree nodes of the forest, including deleted nodes")

Rationale

Document._isContainerUnreachable walks the parent chain, and for tree metadata the tree ancestors (LoroTree._isNodeHidden, bounded by the node count so a malformed cycle cannot loop). It backs isDeleted(), getPathToContainer(), and getCursorPos() only. getNodes() now lists alive nodes breadth-first from the roots, then (with withDeleted) the deleted subtrees, as Rust's get_nodes_under does.

Changed after #1131 merged: _isContainerDeleted stays main's one-level check. It is used by getContainerById and by #1131's #isReachable, which already walks ancestors; walking them again per level made deep trees quadratic (tree-diff-order.test.ts). The earlier guard that rejected local edits to deleted containers is dropped: it threw on a redo that main (and #1131's diff-attached-children.test.ts) accepts.

Known limitations (unchanged from main)

  • loro-crdt rejects local edits to a deleted container (Transaction::apply_local_op); loro.js still records them.
  • For a directly deleted node, Rust's parent() returns its internal deleted-root node and index() returns undefined; loro.js returns undefined and -1.

Validation

  • New loro-js/tests/deleted-containers.test.ts (map descendants, list children, tree subtrees including revival); all three fail on main.
  • pnpm -C loro-js check with LORO_REQUIRE_WASM_REFERENCE=1 and a Rust reference built from main: 29 files, 482 passed, 1 skipped.
  • The local edit, import, and diff paths are main's code again; bench:complexity for the unchanged getNodes/nodes change (7 alternating rounds vs main, loadavg 10–14): 284 items, none both >1.5× and >0.3 ms slower.

🤖 Generated with Claude Code

isDeleted() only looked at a container's own parent binding, and a tree
node only at its own deleted flag. A child of a removed map value, or a
node under a deleted tree node, therefore counted as alive: it kept a
path, getNodes() listed it, and local edits to it recorded ops on
invisible state. Rust's DocState::is_deleted follows the whole chain and
rejects such edits with ContainerDeleted.

Walk the parent chain (and tree ancestors) for isDeleted,
getPathToContainer, and a new guard in #appendAndApply that throws
Rust's message. getContainerById keeps its one-level check, since Rust
still resolves those IDs. nodes() now returns every node and getNodes()
lists alive nodes breadth-first, as Rust does.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

WASM Size Report

  • Original size: 3277.59 KB
  • Gzipped size: 1089.33 KB
  • Brotli size: 762.83 KB

zxch3n commented Sep 29, 2026

Copy link
Copy Markdown
Member Author

Merged the latest main (c0ff1255) with no conflicts. pnpm -C loro-js check with LORO_REQUIRE_WASM_REFERENCE=1 passes (26 files, 417 tests), including #1136's Rust differential suite, so the new "container is deleted" guard does not trip on its undo/revertTo paths.

Still needed on main: edits to a container under a deleted ancestor are accepted (Rust rejects them), isDeleted()/getPathToContainer() ignore deleted ancestors, and getNodes() lists nodes under a deleted node. All liveness probes now match loro-crdt.

Known limitation (unchanged, not a data issue): for a directly deleted node, Rust's parent() returns its internal deleted-root node and index() returns undefined; loro.js returns undefined and -1. #1131's #isTreeRecordHidden and this PR's LoroTree._isNodeHidden compute the same thing and can be merged in a follow-up once both land.

zxch3n and others added 2 commits September 29, 2026 19:18
…path

Keep main's one-level _isContainerDeleted for getContainerById and the
diff/event reachability walk, and use the ancestor walk only in
isDeleted(), getPathToContainer() and getCursorPos(). Drop the guard on
local edits to deleted containers: it threw on a redo that main allows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

zxch3n commented Sep 29, 2026

Copy link
Copy Markdown
Member Author

Merged main (with #1131) and reworked in 03dc8dc: the ancestor walk now backs only isDeleted(), getPathToContainer() and getCursorPos(); _isContainerDeleted is main's again, so #1131's reachability walk stays linear in depth. The local-edit guard is dropped (it broke a redo that main allows) and listed under known limitations. pnpm -C loro-js check with the Rust reference: 482 passed, 1 skipped.

zxch3n and others added 2 commits September 29, 2026 21:26
getNodes() spread each child list, and with withDeleted the deleted
subtrees, into one push() call, which throws a RangeError past V8's
argument limit (about 125k on Node's main thread).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

zxch3n commented Sep 29, 2026

Copy link
Copy Markdown
Member Author

Merged main and fixed the blocking issue in c783b88: getNodes() and _subtreeRecords append records in a loop instead of push(...list), so a node with more children than the argument limit (or that many deleted nodes with withDeleted) no longer throws a RangeError. The limit is about 125k on Node's main thread but 400k–500k in vitest's worker threads, so reproducing it in CI would need about 500k nodes; the new test instead checks the widest push() during getNodes() at 2,000 children (2,000 before the fix, 1 after). A one-off 600k-node run passes locally with the fix. pnpm -C loro-js check with the Rust reference: 32 files, 497 passed, 2 skipped.

@zxch3n
zxch3n merged commit 4b04934 into main Sep 29, 2026
2 checks passed
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