Skip to content

fix(kb): character-level CRDT updates for node title/body/tags - #625

Merged
cuttlefisch merged 6 commits into
mainfrom
fix/kb-node-crdt-wholesale-replace
Aug 5, 2026
Merged

fix(kb): character-level CRDT updates for node title/body/tags#625
cuttlefisch merged 6 commits into
mainfrom
fix/kb-node-crdt-wholesale-replace

Conversation

@cuttlefisch

Copy link
Copy Markdown
Owner

ADR-092 D2 (design record in #624). Two coupled bugs — the second was hidden by the first.

1. Wholesale replace corrupted concurrent edits

KbNodeDoc::set_body/set_title did remove_range(0, len) + insert(0, new) on a YText. Two peers editing the same field from a shared base converge and lose neither edit — but each tombstones the base once and re-inserts its own full copy at origin 0, so the text neither peer touched survives once per peer:

Line one.        <- shared base
Line two.
From A.
Line one.        <- shared base, DUPLICATED
Line two.
From B.

A 500-line node edited concurrently became 1000 lines of doubled text. Titles concatenated outright: "Design Notes (draft)Design Notes v2". set_tags had the same bug in YArray form: ["rust","kb","from-a","rust","kb","from-b"].

Why no test caught it. Both existing "convergence" tests only ever have peers edit different fields — three_client_concurrent_edits_converge says so in its own comment. The one same-field test asserted a.body() == b.body(), and that oracle is worthless here: a CRDT gives convergence for free, so it passes on garbage. The oracle that matters is that the untouched base appears exactly once.

The fix extracts the diff core from TextSync::reconcile_to into reconcile_text_ref, so the buffer path and the KB-node path share one implementation (principle #8) rather than one correct and one not. reconcile_to delegates with no behavior change — its existing tests, including utf16_reconcile_with_emoji, pass unmodified as the regression guard. Also corrects its doc comment, which claimed OffsetKind::Bytes; new_doc configures OffsetKind::Utf16.

2. op_set::materialize depended on op order — and the order was wrong

Fixing #1 broke three daemon tests. Root cause was not the new diff:

materialize seeded from opened.ops[0], but ops[0] is not the structure op. open_new_ops ranks by the update's state-vector total, and Update::state_vector() is non-empty only for a self-contained full-state op — every incremental delta reports an empty state vector, hence rank 0. Measured: root ranks 6, both child deltas rank 0. Ascending sort put the root last, so materialize always seeded from a delta, which carries no type for the other ops to attach to. Confirmed identical on main.

It worked there only by accident: the old bulk insert was one item yrs could integrate whenever the root arrived. Character-level updates emit many items and don't reconstruct that way.

Worse, op order is not causal and can't be made so — with every delta at rank 0 the sort falls through to op_id, a ciphertext hash, effectively random per author. Two deltas that do depend on each other can arrive reversed, leaving the later pending forever and silently yielding stale content (the v1 title where the author wrote v2).

Fixes:

  • open_new_ops sorts self-contained ops before deltas, then rank, then op_id — still a total order, so peers replay identically.
  • materialize combines all ops into one update via Update::merge_updates and applies it once, making the result a function of the op set, not delivery order. Re-applying in a loop is not a substitute — it panics inside yrs (block_store.rs:318).
  • No unwrap on this path: an op-set is attacker-supplied ciphertext, so a forged or truncated op must degrade to an empty node, not panic the client.

Tests

All of the following failed before their respective fix:

  • concurrent_same_field_edits_do_not_duplicate_the_untouched_base — selective oracle: base appears exactly once
  • three_peers_editing_one_body_converge_identically_under_every_apply_order — ≥3 peers, all six orders, order-independence and base-once
  • concurrent_title_edits_..., concurrent_tag_edits_do_not_duplicate_shared_tags
  • reconciled_body_handles_utf16_surrogate_and_cjk_boundaries — emoji/CJK across a surrogate pair
  • setting_a_field_to_its_current_value_produces_no_ops — an unchanged save grew the doc 111→125 bytes, churning tombstones into a replicated, compacted document
  • open_new_ops_returns_a_self_contained_op_before_any_delta — pins the ordering invariant over 50 fresh keys (the tiebreak is a ciphertext hash; one iteration can pass by luck). Verified to fail against the old sort. Asserts its precondition so it isn't a tautology.
  • an_op_set_missing_its_root_materializes_empty_without_panicking — a relay withholding the structure op yields empty, never partial content
  • a_forged_op_that_decrypts_to_garbage_is_skipped_not_fatal

A storage-order permutation test was written and discarded as vacuousmerge re-encodes the op-set so all 24 permutations re-sort identically, and it passed against the broken code. It was testing merge, not ordering. Replaced with the direct ordering assertion above.

Also drops the // set_body replaces, so last-write-wins comment, which described the defect as the design.

Verification

  • mae-sync 309 passed (×5 runs — these tests are hash-order-variable by construction)
  • mae-core 3099 passed
  • daemon workspace clean ×3 runs (separate lock, ADR-014)
  • make pre-commit green

🤖 Generated with Claude Code

cuttlefisch and others added 2 commits August 5, 2026 12:43
ADR-092 D2. `KbNodeDoc::set_body` and `set_title` performed
`remove_range(0, len)` + `insert(0, new)` on a `YText` — a wholesale
replace. Two peers editing the same field from a shared base converge and
lose neither edit, but each tombstones the shared base once and re-inserts
its own full copy at origin 0, so the text NEITHER peer touched survives
once per peer:

    Line one.        <- shared base
    Line two.
    From A.
    Line one.        <- shared base, duplicated
    Line two.
    From B.

Two people editing a 500-line node concurrently produced a 1000-line node
with everything doubled. Titles fared worse, concatenating outright:
"Design Notes (draft)Design Notes v2".

`set_tags` had the identical hazard in `YArray` form — both peers wiped
the array and re-appended their own full list, so shared tags returned
once per peer: ["rust","kb","from-a","rust","kb","from-b"].

Why no test caught it: both existing "convergence" tests only ever have
peers edit DIFFERENT fields (three_client_concurrent_edits_converge says
so in its own comment), and the one same-field test asserts
`a.body() == b.body()`. That oracle is worthless here — a CRDT gives
convergence for free, so the assertion passes on garbage. The oracle that
matters is that the untouched base appears exactly once.

The diff core is extracted from `TextSync::reconcile_to` into
`reconcile_text_ref`, so the buffer path and the KB-node path now share one
implementation (principle #8) instead of one correct and one not.
`reconcile_to` delegates to it with no behavior change — its existing
tests, including utf16_reconcile_with_emoji, pass unmodified as the
regression guard. Both carry an @ai-caution against reintroducing the
replace. Also corrects reconcile_to's doc comment, which claimed yrs uses
`OffsetKind::Bytes`; `new_doc` configures `OffsetKind::Utf16`.

Tests added, all failing before this change:
- concurrent_same_field_edits_do_not_duplicate_the_untouched_base
- three_peers_editing_one_body_converge_identically_under_every_apply_order
  (>=3 peers, all six apply orders, order-independence + base-once)
- concurrent_title_edits_do_not_duplicate_the_untouched_base
- concurrent_tag_edits_do_not_duplicate_shared_tags
- reconciled_body_handles_utf16_surrogate_and_cjk_boundaries
- setting_a_field_to_its_current_value_produces_no_ops (an unchanged save
  grew the document 111 -> 125 bytes, churning tombstones into a
  replicated, compacted doc)

Also drops the misleading "set_body replaces, so last-write-wins" comment
on two_clients_merge_body, which described the defect as the design.

mae-sync: 306 passed, 0 failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WIP-safe commit: gate runs next.

`op_set::materialize` seeded its document from `opened.ops[0]` and applied
the rest in sequence. Two things were wrong with that.

First, `ops[0]` is not the structure op. `open_new_ops` ranks each op by
its update's state-vector total, and `Update::state_vector()` is NON-EMPTY
only for a self-contained full-state op — every incremental delta reports
an EMPTY state vector, hence rank 0. Measured, not assumed: the root ranks
6 while both of its child deltas rank 0. Ascending sort therefore put the
root LAST, so materialize was always seeding from a delta, which carries
no type for the other ops to attach to.

Second, op order is not causal and cannot be made so. With every delta at
rank 0 the sort falls through to `op_id` — a ciphertext hash, effectively
random and different for every author. Two deltas that genuinely depend on
each other (successive edits to one field) can arrive reversed, leaving
the later one pending forever and silently yielding stale content: the v1
title where the author wrote v2.

This only ever appeared to work because set_body/set_title emitted a
single bulk insert per edit — one item yrs could integrate whenever the
root turned up. Character-level updates (ADR-092 D2) emit many items and
do not reconstruct from a delta-seeded document, which is how the
accident surfaced.

Fixes:
- open_new_ops sorts self-contained ops before deltas, then rank, then
  op_id — still a total order, so every peer replays identically.
- materialize combines every op into ONE update via Update::merge_updates
  and applies it once, so the result is a function of the op SET rather
  than of delivery order. Re-applying ops in a loop to force convergence
  is NOT a substitute: it panics inside yrs's block store
  (yrs-0.27.0/src/block_store.rs:318).
- No `unwrap` on this path. An op-set is attacker-supplied ciphertext, so
  a forged or truncated op that nonetheless decrypts must degrade to an
  empty node, not panic the client.

Adversarial tests (ADR-037: the relay is key-blind, not honest):
- open_new_ops_returns_a_self_contained_op_before_any_delta — pins the
  ordering invariant directly, over 50 fresh keys because the tiebreak is
  a ciphertext hash and one iteration can pass by luck. Verified to FAIL
  against the old sort. Asserts the precondition (exactly one root among
  four ops) so "root first" is a claim, not a tautology.
- an_op_set_missing_its_root_materializes_empty_without_panicking — a
  relay that withholds the structure op gets empty content, never partial
  content a caller might treat as authoritative. Includes the
  complete-op-set precondition so it cannot pass vacuously.
- a_forged_op_that_decrypts_to_garbage_is_skipped_not_fatal — honest
  content survives an op sealed under the real key whose plaintext is not
  a valid update.

A storage-order permutation test was written first and DISCARDED as
vacuous: `merge` re-encodes the op-set, so all 24 permutations re-sort
identically and it passed against the broken code. It was testing `merge`,
not ordering.

mae-sync 309 passed; mae-core 3099 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cuttlefisch cuttlefisch added the release:none Skip version bump on merge label Aug 5, 2026
cuttlefisch and others added 3 commits August 5, 2026 14:08
…tests

Two CI fixes for the ADR-092 D2 change.

1. kb_sync_n_peer_e2e's concurrent-edit tests asserted that each peer's
whole `from-{name}` fragment survives the merge as a contiguous substring.
That property is only achievable with the wholesale
`remove_range(0,len)` + `insert(0,new)` this PR removes, where every peer
re-inserted its full string at origin 0 — which is precisely what
duplicated the characters nobody had edited. The test was pinning the
defect as the contract; the two cannot both hold.

Under character-level CRDT text, concurrent edits to the SAME region
interleave. What CRDT does guarantee — and what the test's own comment
says it is checking, "no silent last-writer-wins drop" — is that no
INSERTED character is lost. The oracle is now each peer's unique index
digit, which appears in nothing but that peer's own edit, asserted on
both title and body. Convergence across peers is unchanged and still
checked by assert_all_agree.

2. node_tests.rs hit the 500-line structural ceiling (538). Split the
ADR-092 convergence cases into node_convergence_tests.rs, following the
existing one-file-per-theme convention in kb/tests/. No test content
changed in the move. Blessing the ceiling was the alternative and would
have been the wrong call for a file that grew because tests were added.

mae-sync 309 passed; kb_sync_n_peer_e2e 13 passed; ratchet clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Takes dependabot's rust-dependencies group bump directly rather than
landing it as a separate PR that would immediately conflict with this
branch's lockfiles. clap 4.6.4 -> 4.6.5 and clap_builder 4.6.2 -> 4.6.5,
applied to BOTH workspaces (the daemon keeps its own Cargo.lock per
ADR-014, and dependabot only ever touches the root one).

Also carries the 0.14.89 -> 0.14.92 workspace-internal version sync that
appears in every one of these PRs — issue #61: the version-bump workflow
does not update Cargo.lock, so the locks drift behind the released
version and every branch rediscovers it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes the P2P mesh e2e regression on this branch: 'FAIL(mesh): canary
absent from the owner daemon's store'. 8/8 green on main, so this was
mine.

Cause: reconcile_text_ref used TextDiff::from_chars with
iter_all_changes(), which yields one change PER CHARACTER. Inserting into
an empty text still produced one run, so node CREATION looked fine — but
REPLACING an existing body made the character-level LCS match scattered
shared letters, fragmenting the new text into many small inserts
separated by spurious Equal runs. Verified directly: contiguous=true over
an empty body, contiguous=false over an existing one.

That breaks more than the failing assertion.
scripts/collab-p2p-mesh-e2e.sh greps the daemon store for canary
PLAINTEXT twice: once to prove content reached the owner (non-vacuity),
and once to prove an E2E KB's plaintext is ABSENT (key-blindness).
Fragmented plaintext is unfindable by grep, so the first check fails —
and the second would start passing whether or not anything was actually
sealed. A security assertion quietly becoming vacuous is the worse half
of this bug.

Two changes:

- Coalesce consecutive same-tag changes into a single op, so a run is one
  insert rather than one per character. This alone is a real reduction in
  item count and encoded size.

- Diff by WORD rather than character. Coalescing is not sufficient on its
  own, because a char-level LCS interleaves Equal runs through unrelated
  text no matter how the ops are batched. Word granularity still leaves
  untouched text alone — so concurrent edits still never duplicate the
  base, which is the ADR-092 D2 property this branch exists to fix — while
  emitting contiguous runs. The trade-off, documented at the function: two
  peers editing the same WORD concurrently replace it instead of merging
  within it.

Adds an_edited_body_is_stored_as_a_contiguous_run, covering both the
empty and replacing cases. Only the replacing case regressed, which is
why a creation-only test would have missed it.

mae-sync 310 passed; daemon workspace clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cuttlefisch
cuttlefisch merged commit 397d502 into main Aug 5, 2026
22 checks passed
@cuttlefisch
cuttlefisch deleted the fix/kb-node-crdt-wholesale-replace branch August 5, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:none Skip version bump on merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant