fix: reject shallow imports that depend on trimmed history instead of aborting - #1083
Open
rexikan wants to merge 2 commits into
Open
fix: reject shallow imports that depend on trimmed history instead of aborting#1083rexikan wants to merge 2 commits into
rexikan wants to merge 2 commits into
Conversation
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.
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.
Summary
Two import routes into a shallow doc ended in a process abort rather than an
Err:apply_change_from_remotehitcalc_unknown_lamport_change(..).unwrap()on
Err(()), the panic poisoned the doc mutex, and the process died withSIGABRT in a destructor. Reachable through
import,import_json_updates, andimport_batch.cut). A fork one op lower already failed gracefully with
ImportUpdatesThatDependsOnOutdatedVersion.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.
AppDag::import_deps_before_shallow_rootresolved the depswith
frontiers_to_vv, which special-cases exactly the root's own deps toshallow_since_vv(needed so a shallow doc can re-export at its own cut), sothe vv inclusion check passed.
parked map op survives it. The pending replay
(
remote_change_apply_state) then checked the parked change's deps againstthe 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
AppDag::deps_reach_trimmed_history(any dep insideshallow_since_vv).frontiers_to_vvand rejects withImportUpdatesThatDependsOnOutdatedVersion. 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_vvhas nothing to resolve. Its test(
shallow_doc_accepts_cross_peer_op_whose_deps_include_boundary) isuntouched.
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.
!applies_to_dagerror arm inimport_changes_and_apply_delta_to_state_if_neededno longer rolls thearena 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 thatSIGABRT on
main(root-deps, root plus cross-peer trimmed JSON dep, parkedchange unlocked by
import, by JSON, byimport_batch, by an import that onlyparks), 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 fuzzgreen; no new clippy orrustfmt findings on touched lines.
Noted, not fixed (pre-existing)
parks can put one into the oplog without a state diff, leaving
state_frontiers != oplog_frontiers.imports as updates whose root change parks forever.
import_batchreportsErrafter applying the valid sibling blob(documented at
loro.rs); this fix makes that reachable where it used toabort.
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.