fix: keep exact common ancestor when checking out to the past - #1071
fix: keep exact common ancestor when checking out to the past#1071lodystage[bot] wants to merge 3 commits into
Conversation
`_find_common_ancestor` discarded the ancestor it had just computed and returned empty frontiers whenever a branch of its DAG walk ended without meeting the other side. Checking out to a version already in the past -- what `fork_at(frontiers)` does -- always ends that way, so the diff calculator replayed from the start of history, waking a calculator for every container in the document. That is O(containers x ops) and hangs large documents in the browser. Skip the conservative fallback when the computed ancestor is exactly the target: every id of the target is then reachable from the source, so it is provably the maximal common ancestor. It is also the safe direction for the trackers, since the retreat is never empty there. Rationale and the measured cost model live in context/diff-calc-replay-base.md. Closes #1056 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four ways of relaxing the richtext/list `should_rebuild` were measured and reverted while following up on #1056. Each looks obviously correct on paper, so record the evidence that rules them out - including that an oracle which only checks a single checkout's output wrongly concludes the rebuild is unnecessary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto main brought in #1058, which reworks replay-base selection around critical versions: `all_tips_covered_by_ancestors` tells a genuine concurrent branch apart from a redundant path into a known ancestor, and `latest_single_head_critical_version` retreats to the latest safe cut instead of to empty frontiers. That subsumes the fix this branch carried, so the conflicting hunk in `dag.rs` was resolved in favour of main. The reasoning this branch used was also wrong, not merely redundant: it treated the meet of the two versions as a safe base whenever the target was already in the source's past. For `current = [10@1, 5@2]` with peer 2 branching at `3@1`, the meet `10@1` is not a critical version, because `0@2..5@2` is concurrent with `4@1..10@1`. The unit test now asserts the branch point `3@1`, and separately that the base is not empty, so it still guards the regression it was written for. Drop the changeset: no user-visible behaviour is left in this branch. Rewrite the falsified section of the replay-base article to point at `docs/critical-version-spec.md`, and note the bad argument so it is not retried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cf583b5 to
a4b6d20
Compare
|
Superseded by #1058, which reworked replay-base selection around critical versions — The argument here was also unsound, not just redundant: it treated the meet of the two versions as a safe replay base whenever the target was already in the source's past. For Closing rather than reworking. Nothing here is needed on top of #1058. |
WASM Size Report
|
Summary
Fixes #1056 —
forkAt(frontiers)hanging the browser with no error on large documents._find_common_ancestordiscarded the ancestor it had just computed and returned emptyfrontiers whenever a branch of its DAG walk reached a root without meeting the other
side. Checking out to a version already in the past — exactly what
forkAtdoes —always ends that way:
The conservative fallback is now skipped when the computed ancestor is exactly the
target version.
Rationale
An empty replay base is not a constant-factor cost, it is document-sized and
multiplicative: the causal iterator walks the entire oplog, so every container in
the document gets a diff calculator, and each of those independently re-walks the whole
oplog to rebuild its tracker —
O(containers x ops).When
ans == right, every id ofrightis reachable fromleft, sorightis themaximal common ancestor and there is nothing to be conservative about. It is also the
safe direction for the trackers:
from_vvthen strictly containsto_vv, so theretreat is never empty and the richtext/list/movable-list trackers are still rebuilt
from CRDT ids rather than advanced incrementally.
The symmetric relaxation (
ans == left, fast-forwarding past a concurrent branch) isnot safe — it has no retreat to force a rebuild, and
undo_treeincrates/fuzzfails under it. The code comment names that test so the next person does not retry it.
The regression came from
b81abfce(#974) in 1.13.7, which is why 1.13.6 is unaffected.Validation
Full gate
cargo test --features=test_utils,jsonpath --no-fail-fast: 73 groups,1417 tests, 0 failures
crates/fuzz128 tests including theundo_treecanaryNew
common_ancestor_of_subset_target_is_the_target_itself(dag unit test) andtest_fork_at_with_concurrent_frontiers(fork / checkout / attach / re-import)Perf harness
crates/examples/examples/fork_at_many_containers.rs, 1600 containers /194k ops:
checkoutforkAtKnown limitation
forkAtstays above 1.13.6 because it internally checks back to the original version,which is the
ans == leftdirection that cannot be relaxed safely. Separately, a singlelarge text container is still ~2x slower than 1.12 due to the unconditional tracker
rebuild
b81abfceadded. Both are recorded incontext/CONTEXT-GAPS.md, along withfour relaxations of that rebuild that were measured and ruled out — including the trap
that an oracle checking only a single checkout wrongly reports the rebuild as redundant.
Background and the cost model live in the new
context/diff-calc-replay-base.md.🤖 Generated with Claude Code