oficina / docs Public

Task 2 fix round 1: correct three untrue comments from review

- core/src/lib.rs: the ABI-version doc invented a general "an added
  export can share a version" rule that contradicts the paragraph
  right below it and internal/core/core.go. Replaced with the actual
  reason sharing v4 is safe here: both the snapshot pair and
  verify_snapshot landed before v4 ever shipped, so no host in the
  field saw a 4 without this export. An export added after a version
  ships still forces its own bump.

- core/src/snapshot.rs: verify_snapshot's doc oversold the state
  vector/delete-set comparison. ReadTxn::snapshot() reads only
  Store::blocks; pending/pending_ds are separate fields it and
  fragment_json both never see. A snapshot_bytes built from
  encode_diff_v1 (which drops the pending queue) would verify as a
  perfect match despite losing data. Documented the blind spot and
  named what actually guards it: the pending-queue regression test.

- core/src/snapshot.rs: the deadlock-avoidance comment blamed "two
  overlapping reads," which yrs permits fine. The real mechanism is
  fragment_json's root() calling get_or_insert_xml_fragment, which
  takes a WRITE lock internally before fragment_json's own read
  transaction opens — colliding with a read guard still held from
  verify_snapshot's earlier doc.transact(). Rewrote the comment to
  name that path and state the actual rule.

No logic changes; wasm artifact is unaffected (comments only).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago 310169298f3288cc32315f533a51b2fcfa3a257b parent 1c44b30

2 files changed, +35 −13

core/src/lib.rs +7 −4
core/src/snapshot.rs +28 −9

diff --git a/core/src/lib.rs b/core/src/lib.rs

index 1db2f1c..e5fb37e 100644

--- a/core/src/lib.rs

+++ b/core/src/lib.rs

@@ -78,10 +78,13 @@ extern "Rust" fn __getrandom_v03_custom(

/// read the document's structure through, and to 4 for the snapshot

/// pair (`docs_core_snapshot_len` / `docs_core_snapshot_read`) that

/// exports a document's whole state as one update. `docs_core_verify_snapshot`,

-/// which checks a candidate snapshot against a live document, joins

-/// that same version 4 rather than forcing a 5 — it is one more export

-/// added to the same already-bumped generation, not an incompatible

-/// change to an existing one.

+/// which checks a candidate snapshot against a live document, lands

+/// within that same version 4 rather than needing one of its own —

+/// both pairs were added before version 4 ever shipped, so no host in

+/// the field has seen a 4 that lacks this export. That is the only

+/// reason a shared number is safe here: an export added after a

+/// version has already shipped forces its own bump regardless, for the

+/// reason the next paragraph gives.

///

/// Adding an export is, strictly, backwards compatible — a host that

/// never calls the new function cannot tell 3 from 2 — so the bump is

diff --git a/core/src/snapshot.rs b/core/src/snapshot.rs

index 02c4923..42ce9b9 100644

--- a/core/src/snapshot.rs

+++ b/core/src/snapshot.rs

@@ -55,6 +55,20 @@ pub fn snapshot_bytes(doc: &Doc) -> Vec<u8> {

/// failure is closed by taking the snapshot from the replayed state

/// (the spec's D3), not here.

///

+/// Nor can it catch `snapshot_bytes` being built from the wrong call.

+/// `ReadTxn::snapshot()` reads only `Store::blocks` — `pending` and

+/// `pending_ds` are separate `Store` fields it never touches — and

+/// `fragment_json` cannot render pending content either, so a candidate

+/// built by `encode_diff_v1` (which drops the pending queue; see this

+/// module's header) verifies as a perfect match against a document that

+/// is still holding data the candidate lost. This verifier is exactly

+/// as blind to that substitution as a render comparison would be. What

+/// actually guards it is `core/tests/snapshot.rs`'s

+/// `a_snapshot_with_a_parked_update_heals_once_its_dependency_arrives`,

+/// which parks an update, snapshots around it, and asserts the replay

+/// heals once the dependency arrives — a property only the correct call

+/// has.

+///

/// guid/client_id are the caller's, never generated here — the clippy

/// disallowed-methods rule against drawing entropy from the CRDT library

/// applies to this throwaway document exactly as it does to a real one.

@@ -80,15 +94,20 @@ pub fn verify_snapshot(doc: &Doc, candidate: &[u8], guid: &str, client_id: u64)

}

// The state-vector/delete-set comparison is scoped to its own block

// so both read-only transactions (`a`, `b`) are dropped before

- // `fragment_json` runs below. `fragment_json` opens its own

- // transaction on the same `Doc`/`Store` internally

- // (`doc.transact()`) — holding `a` alive across that call would be

- // a second read acquisition on a store the first is still reading,

- // and `yrs`'s underlying `async_lock::RwLock` blocks that

- // read-while-reading-blocking case rather than permitting it

- // reentrantly, deadlocking the thread. Dropping `a`/`b` first

- // avoids relying on reentrant-read behaviour this lock doesn't

- // provide.

+ // `fragment_json` runs below. The hazard is not two overlapping

+ // reads — yrs's own docs say concurrent readers are fine, and

+ // that's not what this is anyway. `fragment_json` calls `root(doc)`

+ // (`get_or_insert_xml_fragment`) BEFORE it opens its own read

+ // transaction, and that helper goes through `doc.transact_mut()` —

+ // a WRITE acquisition — internally. Calling `fragment_json(doc)`

+ // while `a` (a read guard from `doc.transact()`) is still alive

+ // means that write acquisition blocks on the outstanding reader on

+ // this same thread, and since `a` is never going to be dropped

+ // while the very call it's blocking on hasn't returned, the thread

+ // deadlocks itself. Dropping `a`/`b` first, before calling

+ // `fragment_json` on either document, is what avoids it — the rule

+ // is "never hold a transaction guard across a call that opens one

+ // of its own," not "never overlap two reads."

let same_state = {

let (a, b) = (doc.transact(), rebuilt.transact());

let (a_snapshot, b_snapshot) = (a.snapshot(), b.snapshot());