Repository navigation
fix: reject creating unknown-type containers instead of panicking - #1142
Conversation
`apply_diff` (and `revert_to`/undo, which share `_apply_diff`) hit `unreachable!()` in `Handler::new_unattached` when a diff inserted a container whose type is unknown to this version, e.g. from JS `applyDiff` with "🦜:cid:0@1:Unknown(9)". Local ops have no rollback, so `_apply_diff` now checks the whole batch before applying anything and returns `ArgErr`; MovableList moves of existing unknown elements and mergeable map children still work. Copying a container that is or holds an unknown container via `insert_container`/`set_container` is rejected the same way. Also fixes other panics reachable with unknown containers: import or checkout of a change that creates and edits an unknown container, cursor queries on non-sequence containers, `parent()` of a child of an unknown container, and undo cursors on missing containers. Rationale: crates/loro-internal/src/state/AGENTS.md (unknown_state entry). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
WASM Size Report
|
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review of #1142 found the pre-check could let a batch through that still failed half way: the apply loop recreates container values under new ids, revives mergeable children and recreates deleted tree nodes, so diffs it skipped as unreachable were applied after all. The pre-check now predicts those changes from the whole batch (fresh / revived / removed), reuses the MovableList move planner, skips diffs targeting unknown containers, and only reads the list when a move needs it. An undo/redo step that would recreate an unknown container now fails as a whole with ArgErr and is dropped, instead of being applied partially. loro-crdt event batches keep the events they can convert. Adds a get_parent regression test and a no-op Diff::compose_ref arm for unknown diffs, and unifies the error message. Rationale: context/unknown-containers.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the adversarial review and merged main (#1138 and #1135; the Blocking 1, partial application: the pre-check now predicts what the apply loop does to recreated (fresh), revived (mergeable marker) and removed (deleted child) containers. Every prediction errs towards checking a diff. With this change:
Non-blocking items:
JS events: opened #1151 and fixed the simple part here. A batch touching an unknown container now keeps its other events. Representing an unknown child value inside a parent's event is left to #1151. Rust: 1209 passed, 0 failed; the combination with #1125 and #1134 passes 1251. |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tainers With #1134, a full-state batch (`LoroDoc::diff`) aligns a re-activated mergeable child with the state this doc kept for it. The pre-check looked at the raw full states, so re-activating a child that still holds an unknown container was rejected as creating it. Plan the alignment without side effects (`align_full_state` dry run) and check the edits it produces; the targets it aligns are never skipped by the apply loop, so they are always checked. Rationale: context/unknown-containers.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Adds the reviewer's Map/List/MovableList/tree meta cases for full-state batches, extends the random atomicity check to both full-state and incremental application, and ships a Rust-generated fixture for the WASM tests (JSON updates can't carry the binary mergeable markers). JS `doc.diff()` leaves out the empty entries of unknown containers instead of throwing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Merged main with #1134 (1eb14b3) and fixed the combination blocker from #1134 review round 4 (B1). Head: b550599. B1, full-state revival falsely rejected: re-activating a mergeable Map, List, MovableList or tree meta that still holds an unknown container was rejected as creating it.
Tests:
JS Issues opened: #1154 ( Results:
|
…ontainer Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ep right Review round 2 of #1142 found three ways the unknown container check and the apply loop disagreed: - B1: the full-state plan resolved ids with the initial remap only, so the mergeable child of a container recreated by the batch was aligned against its old hidden state and accepted, while the loop aligned it against a new, empty child and failed half way. The plan now treats containers the loop may recreate (and their mergeable children) as new. - B3: the skip prediction built the ancestor chain from the visible path, which is empty below a hidden mergeable container. It now follows arena parents. - B2: a rejected undo step already rebased the other steps over itself. The rebase now waits until the step is accepted; a rejected step's changes are composed into the remote diff of its row instead. The loop now applies the edits planned for full-state entries instead of aligning them again, and asserts in debug builds that it never applies an entry the check predicted to be skipped or checked differently. `align_full_state` has no side effects any more. Rationale: context/unknown-containers.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… oracle Random edits on a doc holding unknown containers, compared with a twin holding Counters; atomicity alone couldn't see an Ok with a wrong state. Seeds 0-2999 pass (60 run by default). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tainer docs Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ainers Batches without unknown values are aligned by the loop as before, so the plan adds nothing to them. Adds the review's benchmarks with many small full-state targets. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the second adversarial review (of b550599) and merged main (5c7552d: #1125, #1152, #1153). Head: df94c67.
NB1 ( |
…are planning Third review of #1142: - R3-1/R3-2: the check keyed a mergeable child by the value's id, while the loop writes the marker of `mergeable(parent, key)` and remaps the value to it. With a parent the undo manager had already remapped (or a value under another key) the check looked into the old hidden child and accepted a move of U that the loop turned into a creation: undo returned Ok(true) half applied. Such a child is now checked as new and empty. - R3-3: the full-state plan counted marker writes of an entry the loop skips (its target unreachable before the batch, in a reordered batch). - `_apply_diff` asserts in debug builds that an accepted batch never fails on creating an unknown container. Prototype by the reviewer. Rationale: context/unknown-containers.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Adds the R3-1 (undo into a host the undo manager recreated: list element, tree meta, nested), R3-2 (mergeable value under another key) and R3-3 (reordered full-state batch) reproductions, a test pinning that undo after a rejected move behaves like after a remote move, and the reviewer's oracle with a remote peer, interleaved undo/redo, reordered batches and 3-level mergeable chains (60 seeds by default; 0-2999 pass with debug assertions). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…limits of undo A rejected undo step's edits are rebased over like a remote peer's, with the same limits (a move of an element an earlier step inserted). Documented in the undo docs, the changeset and context/unknown-containers.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the third adversarial review (of df94c67). Head: fc16330; CI passes. Main hasn't moved (5c7552d), so no merge was needed.
|
Summary
This version can import, export, check out, move and delete a container whose type it doesn't know (
ContainerType::Unknown, written by a newer Loro). It must never create one, because the content is opaque to it. On main, several paths could be driven into creating one and panicked. The first report was JSapplyDiffwith"🦜:cid:0@1:Unknown(9)"hittingunreachable!()inHandler::new_unattached(found by the author of #1138).apply_diff / revert_to / undo
LoroDoc::check_apply_diff_creates_no_unknown_containerruns before_apply_difftouches the doc. It returnsLoroError::ArgErrwhen the batch would create an unknown container.Handler::new_unattachedreturns the same error as a backstop.mergeable(parent, key)under the parent as it resolves it. When that is not the value's own id, the child is checked as new and empty. This covers a parent the undo manager already remapped, or a value under another key.unknown_container_created_by_delta, which reuses fix: apply multi-child MovableList diffs as moves without losing elements #1138'splan_moves.ArgErr.UndoManager::undo/redoin Rust and JS.Other paths
insert_container/set_container/push_containerreject an unknown container, or a detached or attached container holding one, before any op is written. Before this, RustLoroUnknownsilently became an empty copy.UnknownState::apply_diff. That is the forward-compatibility path itself.get_cursor_poson Map, Tree, Counter or unknown containers now returns not found (undefinedin JS).parent(): a child whose parent is an unknown container, as stored in a newer snapshot, now returnsHandler::Unknown. JS throws the readable "unknown container" error.onPushcursor on a missing container no longer panics.Diff::compose_refcomposes twoDiff::Unknownas a no-op.doc.diff(): since fix: restore deleted mergeable children once on revert/apply_diff/undo #1134 the diff can hold an entry for an unknown container, which made the conversion throw "Unknown diff". Those entries have no content and applying them is a no-op, so they are left out.Cannot create a container of type Unknown(9): the type is unknown to this version of Loro.Rationale and test-forging recipes:
context/unknown-containers.md.Full-state batches (#1134)
A batch marked
full_state(fromLoroDoc::diff, or JSapplyDiff(diff, { fullState: true })) aligns a re-activated mergeable child with the state this doc kept for it. Keeping an unknown container that is already there creates nothing, so the check must see the aligned edits, not the raw full states.plan_full_state_batchaligns the whole batch once, before anything is applied. The check runs on the planned edits, and the loop applies those same edits instead of aligning again.align_full_statenever mutates now; a counter gets an increment diff.applyDiffaccepts any order, so the entry that revives it may come later.full_state_targets, because only it followscontainer_remapas containers are recreated. It uses a planned edit only when it aligns the same container the plan aligned. The one expected difference is a mergeable child of a container the batch recreates. The plan marks it fresh and checks it as new and empty; the loop aligns it against the new child.Review follow-ups
Round 1 (review of 9bd96f3):
get_parenttest;compose_refarm; one error message; performance.Round 2 (#1134 review round 4):
doc.diff(): no longer throws on unknown entries.Round 3 (review of b550599):
is_movecase).Ok(true)with U silently lost. Fixed: ancestors come from arena parents.Round 4 (review of df94c67):
Ok(true)half applied in release; a debug build panicked and the JS dev build trapped. The check looked into the old hidden child while the loop writes tomergeable(new host, key). Fixed as above._apply_diffassertion: it now also asserts, in debug builds, that an accepted batch never fails on creating an unknown container.Validation
Rust.
cargo test -p loro -p loro-internal: 1300 passed, 0 failed, 23 ignored. That is on main 5c7552d, which includes #1125, #1152 and #1153.pnpm checkis clean.cargo fmtwas applied to the touched files only.Tests:
crates/loro/tests/unknown_container_atomicity.rs:is_movevariant);crates/loro/tests/unknown_container_twin_oracle.rs:apply_diff, full-state and incremental, everyrevert_toand every undo/redo must be rejected without changes, or give the same state as the twin;LORO_UNKNOWN_TWIN_SEEDSruns more.crates/loro/tests/unknown_container_support/.WASM.
pnpm -C crates/loro-wasm build-devplus package tests: vitest 434 passed / 33 files, plus tsc, the CommonJS smoke test, deno and bun. The JS reproductions of B1, B2, B3 and R3-1 use Rust-generated fixtures. R3-1 used to trap the dev build.Performance (measured at df94c67; round 4 only adds checks to batches that hold unknown containers, and the reviewer measured the same numbers). Release build, median of 7 runs per test, main 5c7552d vs this PR (df94c67).
apply_difflist 100k ints → fresh docapply_diffmap 100k keys → fresh docapply_diff10k child containers → fresh docapply_diff100k plain values into a MovableListrevert_toempty from 100k listapply_diff/revert_to/ undoThe review measured 1.7–1.8× on batches holding unknown values; that came from aligning twice and is gone. The remaining +13% on 2000 small targets that each hold an unknown container is the plan's own bookkeeping and the check. It only runs for batches that hold unknown values.
Known leftovers (not changed here)
apply_diffstill applies part of a batch when it fails for other reasons, such asOutOfBound.diff()omits unknown entries: lossless in this version. A newer version that knows the type reads a missing full-state entry as "empty", so applying such adiff()there withfullStateclears the hidden state of a kept container of that type (review NB1; documented in the context doc).Binarybecomes a list of numbers)._apply_difffails for another reason is still only logged, as on main.🤖 Generated with Claude Code