fix: avoid false concurrent import replay - #1058
Merged
Merged
Conversation
zxch3n
marked this pull request as ready for review
July 31, 2026 12:05
Contributor
WASM Size Report
|
The branch-tip vectors introduced by the unmatched-branch fix were concatenated without dedup on every heap merge and cloned per dep push, making the LCA walk O(2^rounds) on criss-cross sync histories (a 1 KB import at 20 rounds took 86 s vs 168 us on the previous behaviour). Tips are now unioned with dedup, the trimmed-history arm drops its dead recording, the coverage check runs as a single multi-source walk with a shared visited set, and changed_containers_between scans borrowed changes instead of materializing a RichOp per op. Adds regression tests: a depth-26 criss-cross ladder, trimmed-history and multi-head-left-frontier LCA cases, a retreat-direction filter case, and content assertions for the diff_calc fixtures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the LCA walk detects a genuinely concurrent branch, fall back to the latest single-head critical version (Eg-walker sec. 3.5/3.6) of the union graph instead of the empty version: a one-colour descent from both frontiers reusing the coalesce/align machinery returns the span end at the first moment the pending heap narrows to a single span; it bails out to the empty version when a chain dies at a root or trimmed history while other chains remain. Replay after a concurrent import now skips the fully-synced common prefix. Rationale, definitions, and the correctness argument (lemma L11) live in docs/lca_spec_draft.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A multi-head frontier that is version-included in the merged version could be misclassified as ImportGreaterUpdates even when the imported operations were concurrent with one of its heads (version inclusion is weaker than the mode's no-concurrency contract). The tree diff calculator's fast path trusts that contract and applied such moves without adjudicating them against the existing concurrent branch, so the incrementally maintained DocState silently diverged from a full replay of the same oplog. The classifier now runs an entry check when the left frontier has multiple heads: every entry change of the newly imported region (causal parents all in old history) must causally cover the whole left frontier, with a set-equality fast path for the common deps == frontier shape. On failure the mode demotes to the conservative path and the replay base retreats to the latest critical version via latest_singleton_common_cut, matching the pre-regression replay boundary. Adds a DAG-level regression pair (misclassified shape and healthy multi-head merge) and an end-to-end movable-tree test asserting the incremental state matches replay in both import orders. Spec and lemma L12 in docs/lca_spec_draft.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two regression tests for subtle invariants found during the diff-mode audit: checkout between divergent versions whose meet is not a critical version stays canonical, and a checkout whose diff region contains a low-lamport concurrent branch (below the tree calculator's lamport window) stays canonical. The latter guards the coupling between the critical-version fallback in dag.rs and the tree calculator's bounded retreat/forward windows: weakening the fallback would silently break it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The helper tmp dirs were keyed by (pid, nanos) alone; parallel tests in one process can land in the same nanosecond bucket, share a dir, and overwrite each other's in.blob — surfacing as another test's peers in the decoded output. Binary-layout changes made the collision deterministic on this branch. Add a process-wide counter to the dir names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rename the misleading LCA-based names to Eg-walker-aligned terms (arXiv:2409.14252): iter_from_lca_causally -> iter_from_replay_base_causally, lca_vv -> replay_base_vv, latest_singleton_common_cut -> latest_single_head_critical_version, and docs/lca_spec_draft.md -> docs/critical-version-spec.md. The meet of two versions is generally not a safe replay base, so the old naming invited exactly the class of bug fixed by the ImportGreaterUpdates entry check. Document the correctness arguments at the error-prone sites: _find_common_ancestor_new now states its S1-S3 contract; the tree calculator's lamport-window comment explains why bounded retreat/forward is sound and which invariants it depends on; calc_diff_internal distinguishes the direction mode from the computation mode (calc_mode) and DocState::apply_diff documents the direction-keyed dead-containers policy. AGENTS.md gains a critical-version working rule pointing at the spec. No behavior change. 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
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.
Summary
Root cause
The DAG walk expands both explicit dependencies and the implicit previous counter of the same peer. When an existing peer continues after a relay peer becomes the frontier, the relay can already contain that implicit predecessor. The shared relay was recorded as the candidate common ancestor and was not expanded, leaving the redundant implicit path in the queue. A boolean
has_unmatched_branchthen classified that exhausted path as concurrent and discarded the candidate replay base.That could replay the complete history and instantiate calculators for thousands of unchanged list-like containers.
Fix
The LCA queue now carries the dependency tip where a path first splits. When such a path remains unmatched, the algorithm performs a targeted, Lamport-pruned ancestor lookup:
This avoids the proposed per-peer full VersionVector containment check.
The diff calculator also filters conservative common-history replay to containers that actually differ between the two versions.
Validation
cargo check -p loro-internalcargo test -p loro-internal: 468 passed, 5 ignoredcargo test -p fuzz --test test: 127 passed, 1 ignoredcargo test -p loro: 503 passed, 3 ignoredThe private production snapshot is not included in this PR.