fix: buffer pending ops on mergeable children instead of panicking#1017
Merged
Conversation
Importing an incremental update whose op targets a mergeable child whose creation (and/or its parent map) has not arrived yet panicked with `unreachable`/`Option::unwrap()` in `ContainerWrapper::new` at `arena.get_depth(idx).unwrap()`. Root cause: `pending_root_containers_to_materialize` collects every op target with `is_root()` and eagerly builds its state. Mergeable containers reuse the `ContainerID::Root` namespace but are logical *children* whose parent can be any (possibly not-yet-imported) map, so their depth chain is unresolvable while their deps are still pending, tripping the unwrap. Skip mergeable cids in that path, mirroring the `!is_mergeable()` guard already present in `ensure_root_container` (added in #1010 but missed here). Pending changes on mergeable children are now buffered and materialized correctly through the normal diff path once the creating change applies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WASM Size Report
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback: - Capture the creation-only payload before the edit exists, so the second import delivers strictly the missing dependency. Previously it was exported from `Default::default()` after the edit, re-including the edit, so the convergence assert could pass even if the pending edit was dropped. - Assert `import(edit_only).pending.is_some()` to confirm the edit is actually retained as pending, not silently discarded. - Drop the unused `HandlerTrait` import (clippy -Dwarnings). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Importing an incremental update whose op targets a mergeable child container (
ensureMergeable*) panics withunreachable/Option::unwrap()on aNonevalue when the importing doc hasn't yet received that container's creation (out-of-order / partial delivery):For regular (non-mergeable) containers loro correctly buffers such ops as pending; for mergeable containers it traps. Reported against
loro-crdt1.13.2.Root cause
When a change can't yet apply to the DAG (missing deps → buffered as pending),
import_changes_and_apply_delta_to_state_if_neededtakes the!preflight.applies_to_dagbranch and callspending_root_containers_to_materialize, which collects every op target withis_root()and eagerly builds its state viaensure_container.Mergeable containers reuse the
ContainerID::Rootnamespace but are logical children: their cid encodes(parent, key, kind)and their parent can be any map — including a Normal (non-root) map created by an op that also hasn't arrived yet. Eagerly materializing one while its deps are pending leaves its depth chain unresolvable (arena.get_depthreturnsNone, and theparent_resolveris only installed during snapshot/KV decode), soContainerWrapper::new'sget_depth(idx).unwrap()panics.This is a missed spot: #1010 added the
!id.is_mergeable()guard to the siblingensure_root_containerpath but not topending_root_containers_to_materialize.This also explains why the naive standalone repro didn't trigger it: the mergeable parent must itself be a missing Normal map (
root → Normal map → mergeable child); if the mergeable child sits directly under a root map, its depth (=1) resolves fine.Fix
Skip mergeable cids in
pending_root_containers_to_materialize, mirroring the existingensure_root_containerguard. Pending changes on mergeable children are now buffered and materialized correctly through the normal diff path once the creating change applies.Tests
Added
mergeable_container::pendingregression test:root → Normal map → mergeable text, deliver only the edit commit — it must buffer as pending (no panic) and converge once the creation arrives. Verified it reproduces the exact panic before the fix and passes after. Fullcargo test -p loro-internal --features counteris green.🤖 Generated with Claude Code