fix: resolve ensured-but-empty mergeable containers by id#1010
Merged
Conversation
After ensure_mergeable_*, the child is attached, map.get(key) returns a handle, and toJSON() shows it, but has_container / getContainerById returned false/undefined until the first op was written into the child. Any "hold a cid, look the container back up" pattern silently failed on this state, locally and on remote peers after sync. does_container_exist now falls back to the logical alive-walk (get_reachable) for mergeable cids after a store miss: parse the cid, register the arena edge, and verify the parent chain's child refs are alive. The cost lands only on the path that previously returned false. Semantics now align with set_container: - ensured (ref alive) -> id resolves, even before any op or commit, and on remote peers after sync - never-ensured mergeable cid -> still does not resolve (no materialization out of thin air) - deleted: a child with its own state stays resolvable like ordinary deleted containers; an ensured-but-empty child stops resolving once its ref is removed ensure_root_container now skips mergeable roots so id lookup does not eagerly materialize container state; they stay lazy until first op or export. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
WASM Size Report
|
Co-Authored-By: Claude Fable 5 <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
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
After
ensureMergeableMap(and the otherensure_mergeable_*APIs), the child container is attached,map.get(key)returns a handle, anddoc.toJSON()shows it — butdoc.getContainerById(child.id)/has_containerreturnedundefined/falseuntil the first op was written into the child (commit or not). The document claimed the container existed while refusing to resolve its own id.This silently breaks any "hold a cid → look the container back up" pattern, and the inconsistency propagates across the network: peer A ensures an empty mergeable child and syncs; peer B sees the empty map in
toJSON()but cannot resolve the same cid either. loro-mirror was the first downstream victim (all three criticals in loro-mirror PR #85 trace back to this single behavior).Fix
Mergeable cids are self-describing, and the parent map already stores a binary child ref that
map.get(key)resolves into a handle.does_container_existnow reuses the existing logical alive-walk (get_reachable) for mergeable cids after a store miss: parse the cid, register the arena edge (no state creation), and verify the child refs along the parent chain are alive. The cost lands only on the path that previously returnedfalse; hot paths are untouched.ensure_root_containernow skips mergeable roots so that id lookup does not eagerly materialize container state — they stay lazy until their first op (or export, via the alive-container walk), as before.Semantics (aligned with
setContainer)undefined; ids are not materialized out of thin air.Tests
loro_has_container_for_mergeable_cid_through_public_api, which had encoded the old behavior as a contract.mergeable.test.tscoveringgetContainerById/hasContainerfor the original repro shape ({"records":{"note":{}}}), including the never-ensured-cid and remote-peer cases.LoroDoc::has_containerand the wasmgetContainerById.Verified:
cargo test -p loro,cargo test -p loro-internal --features=test_utils,jsonpath,counter, and the wasm vitest suite all pass (the only failures are the pre-existingbase64.test.tsartifact-loading failures in dev builds, unrelated to this change).🤖 Generated with Claude Code