Skip to content

fix: issue sweep (#1118, #1162, #1155, #1165, #1156, #1164, #1157, #1154, #1151; #1149 documented) - #1171

Merged
zxch3n merged 10 commits into
mainfrom
fix/misc-issue-sweep
Sep 30, 2026
Merged

zxch3n merged 10 commits into
mainfrom
fix/misc-issue-sweep

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

One PR for the remaining issue backlog, one commit per issue (each commit message carries its issue number and points at the context article with the rationale). It replaces #1170, which GitHub closed when its branch was renamed.

Issue Result Commit
#1118 import with a reused peer id panics / poisons the doc fixed b2c6a4d9
#1162 dropping a Subscription deadlocks fixed ae1a9bdf
#1155 updates_in_range panics with several spans of one peer fixed 11e8518a
#1165 snapshot with an unclosed style mark panics on read fixed e148f0ec
#1149 legacy text-delete start_ids from loro-crdt ≤ 1.16.3 not fixable in code; documented, advice pinned by tests 207c81f0
#1156 empty root visible live but not after replay fixed (history part); handle-acquisition part is an API decision 2ced4e6c
#1164 failed import leaves DocState at freed arena indices fixed 4de63a99
#1157 tree import misses Create for children of a revived node fixed 8c20916b
#1154 apply_diff applies part of a batch before erroring fixed ccdcee35
#1151 no events when a batch touches an unknown container fixed (the rest is an API decision) 0862c8da

Every fix was first reproduced by a test that fails on main. The details are in each section below.


#1118: imports that reuse local op ids

Repro. Two docs share peer 7 and write different ops at the same ids. Importing one into the other trimmed the "known" part by version vector and applied the tail on top of the wrong history. That panicked in RichtextState::apply_diff under the doc locks (then Drop aborted on the poisoned mutex), or returned Ok with scrambled text.

Fix. OpLog::check_and_trim_known_part_of_changes (oplog/known_history.rs) runs right after decode, before anything is mutated. When the import brings anything new, it compares the overlap of each imported change with local history:

  • deps per counter;
  • ops atom by atom, arena-independent: a one-atom delete ignores its sign, and JSON imports skip value payloads, which JSON text can't round-trip.

A mismatch returns the existing LoroError::UsedOpID { id }. A change whose deps are present but which skips counters of its own peer returns DecodeError; it used to hit a DAG assertion under the lock. See context/import-peer-id-reuse.md.

Validation. crates/loro/tests/import_reused_peer_id.rs (9 tests: updates, snapshot, import_batch, JSON, drop inside catch_unwind, legit overlapping re-imports) and crates/loro-wasm/tests/import_reused_peer_id.test.ts. On main these abort, trap (unreachable) or scramble.

#1162: dropping a Subscription deadlocks

Repro. The issue's two shapes: a callback owning another Subscription, or owning an UndoManager on the same doc. Dropping the outer subscription dropped the callback while the subscriber set's mutex was held, and the inner drop took the same mutex again.

Fix. Three sites now drop removed subscribers only after releasing the lock: the unsubscribe closure, retain's sweep of subscribers dropped mid-emit, and remove().

Validation. crates/loro/tests/subscription_drop_reentrant.rs (4 tests, each with a 5 s deadlock timeout); all 4 time out on main. pnpm test-loom passes. No WASM test: a JS callback's captures are GC-managed, so it cannot own a Rust Subscription.

#1155: updates_in_range with several spans of one peer

Repro. Spans 1@0..2 and 1@5..7 panicked with "counter should be continuous" under the op log lock.

Fix. The ranges of each peer are merged, and the k-th range of every peer goes into the k-th temporary change store. Their blocks are written one after another, which the update decoder already reads. With one range per peer (the common case) there is still a single store.

Validation. crates/loro/tests/export_updates_in_range.rs covers non-contiguous, overlapping and unordered spans across several peers, and many one-op spans. crates/loro-wasm/tests/export_updates_in_range.test.ts.

#1165: snapshot of a version with a mark's start but not its end

Repro. forkAt or a snapshot at an op-level version that holds StyleStart but not StyleEnd panicked on the first read ("unclosed style mark").

Fix. checkout already keeps such a lone start anchor, which styles nothing until its end arrives.

  • The fast state decoder now accepts it.
  • Dead-style redaction leaves it alone.
  • The debug-only anchor-pair checks allow it.
  • A shallow snapshot only advances a root on a StyleStart to c + 1 when that op exists. It used to fail with FrontiersNotFound for a doc whose history ends at the start.

See context/shallow-snapshot-style-redaction.md.

Validation. crates/loro/tests/snapshot_unclosed_style_mark.rs (fork, snapshot, re-export, shallow snapshot, then closing the mark by importing the rest) and crates/loro-wasm/tests/unclosed_style_mark.test.ts.

#1149: legacy text-delete start_ids (not fixed in code)

The bad ops are already in users' histories and are immutable. The readers that go wrong have nothing but start_id to work with:

  • placeholders of trimmed history in a shallow import;
  • a cursor on a character that no delete names.

Deriving the IDs from positions there would mean seeding the shallow tracker from the root state's per-character IDs, a large diff-calculation change for a narrow legacy case. Instead, context/richtext-insert-positions.md now documents what stays correct and how to avoid the wrong paths:

  • full imports, full snapshots and checkouts are right;
  • take shallow snapshots at the latest frontiers, or after the last affected delete;
  • re-create cursors on text such a delete removed.

crates/loro/tests/legacy_text_delete_start_id.rs forges the legacy op through JSON and pins both sides:

  • the recommended paths give the right text;
  • a shallow root before the delete still diverges ("x!"), which is the open part.

No Fixes: the issue should stay open, or be closed as documented, at the maintainers' call.

#1156: empty root containers and replay

Repro. Text inserted and then deleted showed {"t": ""} on the writer, but {} in a fresh replay of the same history.

Fix. A root is visible when the state store has it. An import only created the root's state when its net diff was non-empty. Now, after an attached import, and after a checkout to the latest version (attach, the end of import_batch), every non-mergeable root the diff calculator touched gets its state (materialize_touched_roots). It reuses the calculator's container set; there is no per-op scan.

Remaining part (API decision, not changed). The issue's random workload also found mismatches where the history never touches the root. Bisecting showed they come from reads: doc.get_movable_list("l").len() alone makes "l": [] visible, by design (get_* creates the root's state, snapshots keep it, and hide_empty_root_containers is the opt-out). A value that depends on history only would need a change to that public behavior.

Checking out an older version still keeps roots first touched later. See context/root-container-visibility.md.

Validation. crates/loro/tests/root_visibility_replay.rs:

  • targeted tests (single import, snapshot, batch, JSON, detached + attach, separate imports, undo, map/list/movable list); these fail on main;
  • a port of the issue's random workload (124 seeds, including the reported ones) with every root written once, as a guard.

Plus crates/loro-wasm/tests/root_visibility_replay.test.ts.

#1164: failed import and freed arena indices

Repro. The issue's deterministic test: after a failed import, a new root map inherited "t": "z" from state held at an index the rollback had freed. Retrying the same bad update panicked with an InnerStore type assertion.

Fix. A failed import's arena rollback no longer frees indices. DocState entries, the dead-container cache and handlers on other threads are all keyed by them. What the rollback drops instead:

  • the parent links of normal containers registered during the import, and links to them;
  • cached depths;
  • values and text.

Every rollback path also clears the dead-container cache. A failed import changes no container state (the whole batch is validated first), so the kept entries are safe. Roots it registered without state stay hidden. I checked that a container created only by the failed import reports has_container == false.

Known gaps (pre-existing, documented in context/failed-import-arena-indices.md):

  • a link set by the failed import on an older, parentless container is kept; undoing it needs a set_parent journal;
  • different failing updates grow the arena by the ids they name.

Validation. crates/loro/tests/failed_import_state_rollback.rs: the issue repro, containers created after a failure, retry rejected again, and a Barrier-based threaded stress test (failed 10/10 on main). Plus crates/loro-wasm/tests/failed_import_state_rollback.test.ts, which revives 40 nodes because hash order differs on wasm32.

#1157: Create events for children of a revived tree node

Repro. The issue's two blobs: 3@1008 ended up alive under revived 0@1008 with no Create. A smaller shape: a linear delete of p, then a move of its child out.

Root cause. In Linear / ImportGreaterUpdates mode, TreeState::apply_diff_and_convert turned a move out of a deleted subtree into a single Create, and nothing below the node got one. The same code emitted nothing for a move into a deleted subtree, and skipped the cycle check for raw Create.

Fix. A new apply_raw_move_and_convert classifies each raw op by whether the node is alive before and after it:

  • alive → alive: Move, or nothing if parent, index and position are unchanged;
  • alive → dead: Delete;
  • dead → alive: Create, then a Create for every node of its subtree, parents first;
  • dead → dead: nothing.

Checkout mode is unchanged. See context/tree-events.md.

Validation. crates/loro/tests/tree_revival_events.rs replays events into a mirror and compares it with the state. It covers:

  • the blobs, imported incrementally and fresh, plus a checkout to every version;
  • the linear in/out cases;
  • concurrent delete and move;
  • an undone delete;
  • a 100-seed random sync (3,000 seeds pass in release).

On main the random test failed 15 of 100 seeds. Plus crates/loro-wasm/tests/tree_revival_events.test.ts.

#1154: apply_diff / revert_to are all-or-nothing

Repro. The issue's batch (text insert plus out-of-bounds list delete) returned OutOfBound with the text still inserted. The same applies to a diff type mismatch, a missing container, and stale doc.diff(a, b) applied off a.

Fix. Validating the whole batch up front would mean predicting every handler (tree and movable-list semantics, container recreation, id remapping), so the batch is rolled back instead:

  1. The batch's ops are cut out of the transaction. Earlier uncommitted ops stay, together with their commit options and event hints.
  2. The state is checked out from the batch's last op back to the op before it, using a fresh calculator with events off.
  3. The op log is restored, and leftovers keyed by the reused op ids are cleared: container state created by the batch, exact counter values, parent links, and the dead/alive caches.

A failed call emits nothing, so subscribers and UndoManager see nothing. If another thread edits the same transaction mid-batch, the old partial behavior is kept (and logged) so its edits aren't lost. Undo/redo keeps its existing best-effort behavior. Separately, a stale diff naming a missing tree parent now returns an error instead of hitting an unwrap. See context/apply-diff-atomicity.md.

Validation. crates/loro/tests/apply_diff_atomicity.rs (16 tests). It includes a twin test: random stale diffs on two-peer histories, where the doc must match a twin that never made the call, through later edits that reuse the ids (10,000 seeds pass in release). I re-ran the suite against the pre-fix sources on this branch: 12 of 16 fail, including the issue repro. Plus crates/loro-wasm/tests/apply_diff_atomicity.test.ts.

#1151: events next to unknown containers

#1142 already fixed the title problem: a batch touching an unknown container no longer drops the events of known containers. The existing test is "still emits the events of known containers next to unknown ones".

What was still lost: a map event that sets a key to an unknown child failed to convert as a whole, so known keys updated in the same event (e.g. m.title next to m.u) never reached subscribers. map_delta_to_js now leaves out only the unknown entry. JSON-form output already writes the child as its container id.

Left open (API decision). The unknown child itself (the omitted map entry, or a list insert holding one, which is still dropped whole because leaving out one item would shift the indices of the rest) needs a public JS representation: null as in toJSON, the 🦜:cid:... string, or a wrapper class. The issue itself defers that to a follow-up; it's written up in context/unknown-containers.md. Fixes is used because the reported problem is solved.

Validation. crates/loro-wasm/tests/unknown_container_events.test.ts covers a root map and a nested map, through doc.subscribe and a container subscription. Both tests failed before this commit; the existing unknown-container tests still pass.


Validation (head 0862c8da)

  • cargo test --no-fail-fast -p loro -p loro-internal: 1381 passed, 0 failed (68 binaries).
  • pnpm check: clean.
  • pnpm test-loom: 9/9 pass.
  • WASM: pnpm -C crates/loro-wasm build-dev (vitest, tsc, deno, bun): vitest 43 files / 459 passed (2 skipped, 2 todo); the CommonJS smoke, tsc, deno and bun tests all pass.
  • loro.js is not touched by this PR. An earlier local run of the loro.js tests had one load-sensitive ratio flake that passes alone (movable-list "stay linear": ratio 24.46 against a limit of 24 at loadavg ≈ 24). The CI loro-js job passes.
  • CI (build, loro-js) is green on 0862c8da.

Perf (release, best of 7, three alternating main/PR rounds, loadavg 17–20 on a shared machine)

Hot path main 892ed30b this PR
import: 2k small blobs one by one 13.9–14.5 ms 13.9–14.3 ms
import: only the new change into a 10k-change doc 49.1–49.5 49.2–50.0
import: re-sent tail + new change 49.1–49.7 49.2–49.8
import: all updates, all known (no-op) 6.4–6.6 6.7–6.8
import: snapshot, all known (no-op) 6.6–6.9 6.8–7.1
import: all updates + 1 new change 55.9–56.5 63.8–64.7
import: snapshot + 1 new change 56.0–56.7 65.1–65.8
emit: 5k commits, 2 subscribers 6.6–7.0 6.7–7.7
subscribe + drop ×20k 2.72–2.95 2.99–3.14
apply_diff: 5k-commit history into a fresh doc 3.76–3.87 3.75–3.79
apply_diff: 2k small diffs one by one 3.37–3.56 3.47–3.62

Everything is within noise except two costs:

The subagent probes for #1164 (success path unchanged; crates/examples/examples/failed_import_rollback_perf.rs) and #1157 (40k tree ops, within noise) showed no regression. crates/examples/examples/import_known_history_perf.rs is the import probe.

Follow-ups (not in this PR)

Fixes #1118
Fixes #1162
Fixes #1155
Fixes #1165
Fixes #1156
Fixes #1164
Fixes #1157
Fixes #1154
Fixes #1151

🤖 Generated with Claude Code

zxch3n and others added 2 commits September 30, 2026 12:56
…1118)

Importing history from a client that shared a peer id trimmed the "known"
part by version vector and applied the tail on top of different local
history: it panicked in state apply (poisoning the doc, aborting on drop)
or silently scrambled text. Compare the overlap with local history before
anything is mutated and return UsedOpID; also reject changes that skip
counters of their peer instead of tripping a DAG assertion.

Rationale: context/import-peer-id-reuse.md

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dropping a subscriber's callback runs arbitrary code; when it owns another
Subscription or an UndoManager on the same doc, their drop re-entered the
set's mutex on the same thread and deadlocked. The unsubscribe closure,
retain's dropped-subscriber sweep, and remove() now move the removed
subscribers out and drop them after releasing the lock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zxch3n and others added 4 commits September 30, 2026 13:11
…nge (#1155)

export_blocks_in_range inserted every selected change into one temporary
change store, which requires each peer's counters to be contiguous, so two
non-contiguous spans of one peer panicked under the op log lock. Merge the
ranges per peer and put the k-th range of every peer into the k-th store;
the blocks of all stores are written one after another.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A version addressed with op-level frontiers can hold a mark's StyleStart but
not its StyleEnd. checkout keeps the lone start anchor, but the fast state
decoder rejected it, and the first read of a fork or snapshot panicked under
the doc locks. Decode it the way checkout keeps it (no style range until the
end arrives), relax the debug-only pair checks accordingly, leave it alone
in dead-style redaction, and only advance a shallow root past a StyleStart
when the StyleEnd exists in the op log.

Rationale: context/shallow-snapshot-style-redaction.md

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Deletes written by WASM loro-crdt <= 1.16.3 can name the wrong characters in
start_id. The ops are immutable and the affected readers (shallow-snapshot
placeholders, cursors on deleted text) have nothing but start_id, so the
history is not repaired in code. Document which paths stay correct and how
to avoid the wrong ones, and pin that advice with a test on a forged legacy
history (plus the still-divergent shallow root before the delete).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A root is visible when the state store has it. The writing peer creates it
with its first local op, but an import only did so when the root's net diff
was non-empty, so text inserted then deleted showed {"t": ""} live and {}
in a replay. After an attached import, and after a checkout to the latest
version, create the state of every non-mergeable root the diff calculator
touched.

Getting a root handle also makes it visible, by design; that half of the
issue's random workload is an API decision and is documented, not changed.

Rationale: context/root-container-visibility.md

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zxch3n
zxch3n marked this pull request as ready for review September 30, 2026 05:46
zxch3n and others added 2 commits September 30, 2026 13:47
A failed import's arena rollback freed the indices of containers registered
during the import, while DocState store entries, the dead-container cache and
handlers on other threads still referred to them. The next container
registered at a freed index inherited the old state. The rollback now keeps
every index and drops only what the failed import may have told the arena
(parent links of new normal containers, cached depths), plus values and text;
every rollback path also clears the dead-container cache.

Rationale: context/failed-import-arena-indices.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In the Linear and ImportGreaterUpdates diff modes the tree diff is the raw
ops, and TreeState::apply_diff_and_convert labeled a move that brought a
node out of a deleted subtree as a single Create. The nodes below it got
no Create, so a tree rebuilt from events lost them. In Linear mode a move
of an alive node under a deleted one also emitted no Delete, and in
ImportGreaterUpdates mode Create ops skipped the cycle check that
apply_diff applies.

Raw Create/Move ops are now classified by whether the target is alive
before and after the op, and a revived node is followed by a Create for
each node of its subtree. Checkout-mode diffs are unchanged. Rationale and
the local-event gap: context/tree-events.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

WASM Size Report

  • Original size: 3315.36 KB
  • Gzipped size: 1103.42 KB
  • Brotli size: 772.87 KB

zxch3n and others added 2 commits September 30, 2026 14:06
A batch that fails is rolled back: its ops are taken out of the
transaction (uncommitted edits before it stay), undone from the state by
a checkout through a temporary change under an import rollback scope,
and removed from the DAG. No event is emitted. A stale tree diff naming
a missing parent now returns an error instead of panicking.

Rationale, reused op ids and the multi-thread fallback:
context/apply-diff-atomicity.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ild (#1151)

A map event that set a key to a container of an unknown type (created by a
newer loro-crdt) failed to convert as a whole, so edits to known keys in the
same event never reached subscribers. Leave out only the unknown child's
entry. List events holding an unknown child are still left out whole, and
delivering the unknown child itself needs a public representation (an API
decision); see context/unknown-containers.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zxch3n zxch3n changed the title fix: issue sweep (#1118, #1162, and more) fix: issue sweep (#1118, #1162, #1155, #1165, #1156, #1164, #1157, #1154, #1151; #1149 documented) Sep 30, 2026
@zxch3n
zxch3n merged commit 1b38000 into main Sep 30, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment