Skip to content

fix: reject shallow imports that depend on trimmed history instead of aborting - #1083

Open
rexikan wants to merge 2 commits into
loro-dev:mainfrom
rexikan:fix/shallow-import-aborts
Open

fix: reject shallow imports that depend on trimmed history instead of aborting#1083
rexikan wants to merge 2 commits into
loro-dev:mainfrom
rexikan:fix/shallow-import-aborts

Conversation

@rexikan

@rexikan rexikan commented Sep 3, 2026

Copy link
Copy Markdown

Summary

Two import routes into a shallow doc ended in a process abort rather than an
Err: apply_change_from_remote hit calc_unknown_lamport_change(..).unwrap()
on Err(()), the panic poisoned the doc mutex, and the process died with
SIGABRT in a destructor. Reachable through import, import_json_updates, and
import_batch.

  1. An update whose deps are exactly the shallow root's own deps (the trimmed
    cut). A fork one op lower already failed gracefully with
    ImportUpdatesThatDependsOnOutdatedVersion.
  2. A change parked as pending before the doc became shallow (its deps not yet
    imported), followed by a shallow-snapshot import whose cut trims those deps,
    followed by the update that unlocks the parked change.

Cause

Both routes let a change with a trimmed dep reach the lamport calculation,
which has no dag node for a trimmed id.

  1. The preflight AppDag::import_deps_before_shallow_root resolved the deps
    with frontiers_to_vv, which special-cases exactly the root's own deps to
    shallow_since_vv (needed so a shallow doc can re-export at its own cut), so
    the vv inclusion check passed.
  2. The snapshot path only requires the dag and the arena to be empty, so a
    parked map op survives it. The pending replay
    (remote_change_apply_state) then checked the parked change's deps against
    the oplog vv, which covers trimmed history, and applied it.

Such a change is concurrent with the shallow root: the doc's earliest state is
the root's, the version below it is not representable, and the dag has no
trimmed nodes. It can never be merged, any more than an update that depends
further below the cut.

Fix

  • A shared predicate AppDag::deps_reach_trimmed_history (any dep inside
    shallow_since_vv).
  • The preflight runs it before frontiers_to_vv and rejects with
    ImportUpdatesThatDependsOnOutdatedVersion. The old "touches the boundary"
    arm from fix: harden diff calc for shallow text histories #974 is gone: it was a second route to the same abort for JSON deps
    that mix the root with a trimmed id of another peer. The case fix: harden diff calc for shallow text histories #974 added it
    for, a cross-peer update whose deps include the root plus an id that arrives
    later in the same batch, still parks and imports: no dep is trimmed and
    frontiers_to_vv has nothing to resolve. Its test
    (shallow_doc_accepts_cross_peer_op_whose_deps_include_boundary) is
    untouched.
  • The pending replay runs it after the applied check (the root's own change
    has trimmed deps too and may be parked when the snapshot brings it in) and
    drops the change; the unlocking import returns the same error, the outcome
    the change would have had in the other import order. The valid part of the
    import is applied and kept, as for an import carrying a rejected change of
    its own.
  • The now-reachable !applies_to_dag error arm in
    import_changes_and_apply_delta_to_state_if_needed no longer rolls the
    arena back: the import's own parked changes reference what they allocated,
    and with the rollback kept a parked text op read freed arena bytes once
    unlocked. The detached and applying paths never rolled back on this error.

Validation

crates/loro/tests/integration_test/shallow_snapshot_test.rs: six tests that
SIGABRT on main (root-deps, root plus cross-peer trimmed JSON dep, parked
change unlocked by import, by JSON, by import_batch, by an import that only
parks), plus pins for the neighbours (deps one below the root still rejected,
deps on the root still accepted, a parked root change the snapshot then
applied is not rejected). Two dag unit tests. Rationale in
context/internal-encoding.md.

cargo test -p loro-internal, -p loro, -p fuzz green; no new clippy or
rustfmt findings on touched lines.

Noted, not fixed (pre-existing)

  • A snapshot import never replays parked changes; a later import that only
    parks can put one into the oplog without a state diff, leaving
    state_frontiers != oplog_frontiers.
  • A parked text change makes the doc non-empty, so a shallow snapshot then
    imports as updates whose root change parks forever.
  • import_batch reports Err after applying the valid sibling blob
    (documented at loro.rs); this fix makes that reachable where it used to
    abort.

Disclosure

This fix was produced with substantial AI assistance (Claude): the
reproduction, root-cause analysis, patch, tests, and five independent agent
reviews (root cause, adversarial probing, maintainer view, test quality,
regression) were AI-driven, directed and reviewed by me.

Importing into a shallow doc an update whose deps equal the shallow
root's deps aborted the process. `import_deps_before_shallow_root`
resolved the deps through `frontiers_to_vv`, which special-cases exactly
that set to `shallow_since_vv` so a shallow doc can re-export at its own
cut; the vv inclusion check then passed. With no lamport computable the
change was parked as pending, and the pending replay
(`remote_change_apply_state`) checks deps against the oplog vv, which
covers trimmed history, so it applied the change and hit
`calc_unknown_lamport_change(..).unwrap()`. The panic poisoned the doc
mutex and the process aborted in a destructor.

Such an update is concurrent with the shallow root: the doc's earliest
state is the root's and the dag has no trimmed nodes, so it can never be
merged, any more than an update that depends further below the cut. The
preflight now checks for trimmed deps before resolving the frontier and
rejects with `ImportUpdatesThatDependsOnOutdatedVersion`. The old
boundary arm, which let any frontier touching the root through, was a
second route to the same abort for hand-written JSON deps that mix the
root with a trimmed id of another peer; it is gone.

Rationale in context/internal-encoding.md.
A change parked as pending (its deps not imported yet) stays parked
when the doc then imports a shallow snapshot: the snapshot path only
requires the dag and the arena to be empty, and a parked change with
no arena allocations (a map op, say) passes that. If the shallow cut
trims the parked change's deps, the change is concurrent with the
shallow root and can never be merged. The pending replay did not see
that: `remote_change_apply_state` ignored its `shallow_vv` parameter
and checked deps against the oplog vv, which covers trimmed history,
so the next import that touched the parked slot applied the change and
hit `calc_unknown_lamport_change(..).unwrap()` (no dag node, no
lamport). The panic poisoned the doc mutex and the process aborted in
a destructor. Reachable through `import`, `import_json_updates` and
`import_batch`.

The replay now runs the same trimmed-dep test as the import preflight
(`import_deps_before_shallow_root`), shared as
`AppDag::deps_reach_trimmed_history`, before looking at the oplog vv.
Such a change is dropped and the unlocking
import returns `ImportUpdatesThatDependsOnOutdatedVersion`, the outcome
it would have had in the other import order; the valid part of the
import is applied and kept, as for an import that carries a rejected
change of its own. The check runs after the applied check: the shallow
root's own change has trimmed deps too, and a doc can hold it parked
when the snapshot brings it in.

That makes the error arm of the `!applies_to_dag` branch in
`import_changes_and_apply_delta_to_state_if_needed` reachable for the
first time: the preflight rejects every import the apply could flag on
its own, so there the flag can only come from a dropped parked change.
Its arena rollback is gone, since the import's own changes are parked
and reference what they allocated in the arena; with the rollback kept,
a parked text op read freed arena bytes once unlocked. The detached and
applying paths never rolled back on this error either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant