amadan

oficina / docs Public

Plan 4/Task 2: verify a candidate snapshot against its source document

Adds verify_snapshot (core/src/snapshot.rs), its ABI export
docs_core_verify_snapshot (core/src/abi.rs, joining ABI version 4), and
regression tests in core/tests/snapshot.rs — including a behavioural
test that only the correct encode_state_as_update_v1 call (not its
encode_diff_v1 neighbour) can pass: a document holding an update parked
in yrs's pending queue only heals after snapshot-and-replay if the
snapshot carried that queue along.

Fixes a self-deadlock the brief's own listing would have introduced:
holding a doc.transact() guard alive across a call to fragment_json,
which opens its own transact() on the same store internally, blocks
forever rather than reentering. Also corrects the brief's use of
Transaction::delete_set() (only defined on TransactionMut in
yrs 0.23.5) in favour of ReadTxn::snapshot(), and makes ERR_DECODE
actually reachable at the ABI boundary rather than collapsed into a
plain mismatch. Restores the three inline comments (stale-handle,
in_bounds-safety parenthetical, clamp-reason) Task 1's review found
missing from the snapshot read pair, and extends them to the new
export.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 1c44b302621252f1047a21295e3dc27be39db3c0 parent 442fb91
5 files changed, +286 −8
  • core/src/abi.rs +55 −3
  • core/src/lib.rs +6 −2
  • core/src/snapshot.rs +68 −1
  • core/tests/snapshot.rs +157 −2
  • internal/core/testdata/docs_core.wasm binary
diff --git a/core/src/abi.rs b/core/src/abi.rs
index 277b4fc..b86b56e 100644
--- a/core/src/abi.rs
+++ b/core/src/abi.rs
@@ -46,9 +46,13 @@
//! held a stale handle is exactly the kind of silent corruption §5.2
//! exists to avoid.
-use crate::{apply_update, fragment_html, fragment_json, fragment_text, snapshot_bytes, CoreError};
+use crate::{
+ apply_update, fragment_html, fragment_json, fragment_text, snapshot_bytes, verify_snapshot,
+ CoreError,
+};
use std::cell::RefCell;
use std::collections::HashMap;
+use yrs::updates::decoder::Decode;
use yrs::{Doc, Options};
thread_local! {
@@ -431,12 +435,14 @@ pub extern "C" fn docs_core_snapshot_len(handle: u32) -> i32 {
///
/// `out_ptr` and `out_len` must describe a byte range that lies entirely
/// within this module's linear memory and is valid for writes for the
-/// duration of this call.
+/// duration of this call. (Checked by `in_bounds` before any write — this
+/// safety requirement covers what that check cannot see, such as
+/// concurrent mutation.)
#[no_mangle]
pub unsafe extern "C" fn docs_core_snapshot_read(handle: u32, out_ptr: u32, out_len: u32) -> i32 {
let v = match snapshot_vec(handle) {
Some(v) => v,
- None => return ERR_NO_DOC,
+ None => return ERR_NO_DOC, // stale/unknown handle, not an empty read
};
if v.len() > out_len as usize {
return ERR_BUFFER_TOO_SMALL;
@@ -445,5 +451,51 @@ pub unsafe extern "C" fn docs_core_snapshot_read(handle: u32, out_ptr: u32, out_
return ERR_INVALID_LENGTH;
}
std::ptr::copy_nonoverlapping(v.as_ptr(), out_ptr as *mut u8, v.len());
+ // Same clamp, same reason as the two read functions above.
v.len().try_into().unwrap_or(i32::MAX)
}
+
+/// 0 = the candidate is this document, 1 = it is not, `ERR_DECODE` = the
+/// bytes are not an update at all, `ERR_NO_DOC` = no such handle.
+///
+/// Deviation from the brief's Step 5 listing: that code called
+/// `verify_snapshot` directly and let its `bool` collapse "undecodable"
+/// into the same `1` as a genuine mismatch, which contradicts this
+/// export's own documented contract (`ERR_DECODE` as a distinct
+/// outcome). `verify_snapshot`'s signature — `-> bool` — is the
+/// documented Rust-level interface and stays that way; the decode is
+/// checked here, at the ABI boundary, so the two failure modes reach
+/// the host as different codes without changing that signature.
+///
+/// # Safety
+///
+/// `ptr` and `len` must describe a byte range that lies entirely within
+/// this module's linear memory and is not mutated for the duration of
+/// this call. (Checked by `in_bounds` before any slice is constructed —
+/// this safety requirement covers what that check cannot see, such as
+/// concurrent mutation.)
+#[no_mangle]
+pub unsafe extern "C" fn docs_core_verify_snapshot(handle: u32, ptr: u32, len: u32) -> i32 {
+ if !in_bounds(ptr, len) {
+ return ERR_INVALID_LENGTH;
+ }
+ let candidate = std::slice::from_raw_parts(ptr as *const u8, len as usize);
+ match get_doc(handle) {
+ None => ERR_NO_DOC, // stale/unknown handle, not a mismatch verdict
+ Some(doc) => {
+ if yrs::Update::decode_v1(candidate).is_err() {
+ return ERR_DECODE;
+ }
+ // The verifier's throwaway document takes a fixed guid and
+ // client id rather than generating any: entropy inside the
+ // core is banned (clippy disallowed-methods), and this
+ // document is discarded before the call returns, so a fixed
+ // pair is not merely acceptable but correct.
+ if verify_snapshot(&doc, candidate, "docs-core-verify", 1) {
+ 0
+ } else {
+ 1
+ }
+ }
+ }
+}
diff --git a/core/src/lib.rs b/core/src/lib.rs
index c425d91..1db2f1c 100644
--- a/core/src/lib.rs
+++ b/core/src/lib.rs
@@ -57,7 +57,7 @@ pub mod abi;
mod fragment;
mod snapshot;
pub use fragment::{fragment_html, fragment_json, fragment_text, ROOT as FRAGMENT_ROOT};
-pub use snapshot::snapshot_bytes;
+pub use snapshot::{snapshot_bytes, verify_snapshot};
#[cfg(target_arch = "wasm32")]
#[no_mangle]
@@ -77,7 +77,11 @@ extern "Rust" fn __getrandom_v03_custom(
/// pair (`docs_core_json_len` / `docs_core_json_read`) the export paths
/// 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.
+/// 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.
///
/// 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 5694341..02c4923 100644
--- a/core/src/snapshot.rs
+++ b/core/src/snapshot.rs
@@ -23,10 +23,77 @@
//! - yrs's own `Snapshot` type and `encode_state_from_snapshot` are a
//! different feature (historical views, requiring `skip_gc`) that
//! happens to share this module's name.
-use yrs::{Doc, ReadTxn, StateVector, Transact};
+use yrs::updates::decoder::Decode;
+use yrs::{Doc, Options, ReadTxn, StateVector, Transact};
/// The whole document, as one update.
pub fn snapshot_bytes(doc: &Doc) -> Vec<u8> {
doc.transact()
.encode_state_as_update_v1(&StateVector::default())
}
+
+/// True when `candidate`, replayed into an empty document, is the same
+/// document as `doc`.
+///
+/// Three comparisons, none of them on encoded bytes: update encoding is
+/// not canonical (merge boundaries vary), so byte equality would report
+/// false mismatches and no cheap byte-level proof exists. State vector
+/// equality says both documents saw the same clock ranges from the same
+/// clients; delete set equality says they agree on every deletion;
+/// `fragment_json` equality says they render to the same structure,
+/// which catches formatting-only divergence that a text comparison
+/// misses at identical cost.
+///
+/// This is deliberately not a proof. What survives all three is "yrs
+/// corrupted content while preserving every clock range, every deletion
+/// and the full structural render", and that is a narrow enough residual
+/// to accept.
+///
+/// What it CANNOT catch, stated because the temptation is to believe it
+/// can: a snapshot taken from a stale state verifies perfectly, because
+/// it is compared against the very document that produced it. That
+/// failure is closed by taking the snapshot from the replayed state
+/// (the spec's D3), not here.
+///
+/// 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.
+///
+/// Note on the state-vector/delete-set comparison: `doc.transact()`
+/// returns the read-only `Transaction`, which — unlike `TransactionMut`
+/// — has no `delete_set()` of its own (that method tracks only the
+/// current transaction's deletions, not the document's whole history).
+/// `ReadTxn::snapshot()` is what exposes the document-wide state vector
+/// and delete set together (as `Snapshot { state_map, delete_set }`) on
+/// either transaction kind, so both comparisons go through it.
+pub fn verify_snapshot(doc: &Doc, candidate: &[u8], guid: &str, client_id: u64) -> bool {
+ let update = match yrs::Update::decode_v1(candidate) {
+ Ok(u) => u,
+ Err(_) => return false,
+ };
+ let rebuilt = Doc::with_options(Options::with_guid_and_client_id(guid.into(), client_id));
+ {
+ let mut txn = rebuilt.transact_mut();
+ if txn.apply_update(update).is_err() {
+ return false;
+ }
+ }
+ // 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.
+ let same_state = {
+ let (a, b) = (doc.transact(), rebuilt.transact());
+ let (a_snapshot, b_snapshot) = (a.snapshot(), b.snapshot());
+ a_snapshot.state_map == b_snapshot.state_map
+ && a_snapshot.delete_set == b_snapshot.delete_set
+ };
+ same_state && crate::fragment_json(doc) == crate::fragment_json(&rebuilt)
+}
diff --git a/core/tests/snapshot.rs b/core/tests/snapshot.rs
index 96a0300..b3b585d 100644
--- a/core/tests/snapshot.rs
+++ b/core/tests/snapshot.rs
@@ -1,6 +1,6 @@
-use docs_core::{fragment_text, snapshot_bytes};
+use docs_core::{fragment_text, snapshot_bytes, text_content};
use yrs::updates::decoder::Decode;
-use yrs::{Doc, Options, Transact};
+use yrs::{Doc, Options, ReadTxn, StateVector, Text, Transact};
fn doc_with(client_id: u64) -> Doc {
Doc::with_options(Options::with_guid_and_client_id(
@@ -32,3 +32,158 @@ fn a_snapshot_replayed_into_an_empty_doc_reproduces_the_document() {
"fixture produced no text"
);
}
+
+#[test]
+fn a_snapshot_of_this_document_verifies() {
+ let source = doc_with(1);
+ let update = std::fs::read("../fixtures/vectors/fragment/simple.bin").unwrap();
+ source
+ .transact_mut()
+ .apply_update(yrs::Update::decode_v1(&update).unwrap())
+ .unwrap();
+ let snap = snapshot_bytes(&source);
+ assert!(docs_core::verify_snapshot(
+ &source,
+ &snap,
+ "verify-guid",
+ 99
+ ));
+}
+
+#[test]
+fn a_snapshot_of_a_different_document_does_not_verify() {
+ let source = doc_with(1);
+ source
+ .transact_mut()
+ .apply_update(
+ yrs::Update::decode_v1(
+ &std::fs::read("../fixtures/vectors/fragment/simple.bin").unwrap(),
+ )
+ .unwrap(),
+ )
+ .unwrap();
+
+ let other = doc_with(1);
+ other
+ .transact_mut()
+ .apply_update(
+ yrs::Update::decode_v1(
+ &std::fs::read("../fixtures/vectors/fragment/marks.bin").unwrap(),
+ )
+ .unwrap(),
+ )
+ .unwrap();
+ let wrong = snapshot_bytes(&other);
+
+ assert!(!docs_core::verify_snapshot(
+ &source,
+ &wrong,
+ "verify-guid",
+ 99
+ ));
+}
+
+#[test]
+fn garbage_does_not_verify() {
+ let source = doc_with(1);
+ assert!(!docs_core::verify_snapshot(
+ &source,
+ &[0xff, 0x00, 0x13, 0x37],
+ "verify-guid",
+ 99
+ ));
+}
+
+/// RULING (Task 2 brief, "beyond the brief" item 1): a test that tells
+/// `snapshot_bytes`'s correct call, `encode_state_as_update_v1`, apart
+/// from its dangerous neighbour `encode_diff_v1`.
+///
+/// Both calls render identically whenever nothing is parked, so no test
+/// that only checks rendered content after a clean replay can catch a
+/// regression to the wrong call. The two calls diverge only when the
+/// source document is holding an update it received out of order — one
+/// whose dependency (an earlier clock range from the same client) has
+/// not arrived yet, so `apply_update` accepted it but yrs parked it in
+/// the pending queue instead of integrating it. `encode_state_as_update_v1`
+/// carries that pending queue along (via yrs's own `merge_pending_v1`);
+/// `encode_diff_v1` drops it (its own doc comment says so).
+///
+/// To construct that scenario without drawing entropy from the CRDT
+/// library (every `Doc` below still takes an explicit guid/client id):
+/// one `origin` document produces two updates in sequence — `dep`
+/// (its first edit) and `dependent` (its second edit, whose blocks'
+/// clocks for `origin`'s client id start exactly where `dep`'s left
+/// off). Handing `dependent` to a fresh document without `dep` first is
+/// what makes yrs park it — the document has never seen client 1 at
+/// all, so `dependent`'s blocks fail their dependency check and go into
+/// the pending queue rather than being rejected outright.
+#[test]
+fn a_snapshot_with_a_parked_update_heals_once_its_dependency_arrives() {
+ // Build `dep` and `dependent` as two genuinely sequential updates
+ // from one document, so `dependent` really does depend on `dep`.
+ let origin = doc_with(1);
+ let origin_text = origin.get_or_insert_text("content");
+ {
+ let mut txn = origin.transact_mut();
+ origin_text.insert(&mut txn, 0, "hello ");
+ }
+ let dep = origin
+ .transact()
+ .encode_state_as_update_v1(&StateVector::default());
+ let state_after_dep = origin.transact().state_vector();
+ {
+ let mut txn = origin.transact_mut();
+ origin_text.insert(&mut txn, 6, "world");
+ }
+ let dependent = origin.transact().encode_diff_v1(&state_after_dep);
+
+ // A document that only ever sees `dependent`: yrs cannot integrate
+ // it (client 1's clock range 0..6 is missing) so it is parked, not
+ // rejected — `apply_update` still returns `Ok`.
+ let parked = doc_with(2);
+ parked
+ .transact_mut()
+ .apply_update(yrs::Update::decode_v1(&dependent).unwrap())
+ .unwrap();
+ assert_eq!(
+ text_content(&parked),
+ "",
+ "a parked update must not be visible yet"
+ );
+
+ // Snapshot the document holding the pending update and replay that
+ // snapshot into a completely fresh document. Under the correct
+ // `encode_state_as_update_v1` call the pending queue rides along;
+ // under `encode_diff_v1` it would be silently dropped, and
+ // `rebuilt` would never heal below no matter what arrives.
+ let snap = snapshot_bytes(&parked);
+ let rebuilt = doc_with(3);
+ rebuilt
+ .transact_mut()
+ .apply_update(yrs::Update::decode_v1(&snap).unwrap())
+ .unwrap();
+ assert_eq!(
+ text_content(&rebuilt),
+ "",
+ "the dependency still hasn't arrived, so this must still be pending, not lost"
+ );
+
+ // Deliver the missing dependency to both. If the pending queue
+ // survived the snapshot/replay, both heal to the same content.
+ parked
+ .transact_mut()
+ .apply_update(yrs::Update::decode_v1(&dep).unwrap())
+ .unwrap();
+ rebuilt
+ .transact_mut()
+ .apply_update(yrs::Update::decode_v1(&dep).unwrap())
+ .unwrap();
+
+ assert_eq!(text_content(&parked), "hello world");
+ assert_eq!(
+ text_content(&rebuilt),
+ text_content(&parked),
+ "the snapshot-and-replay document must converge with the original \
+ once the missing dependency arrives"
+ );
+}
diff --git a/internal/core/testdata/docs_core.wasm b/internal/core/testdata/docs_core.wasm
index 419de9d..61a292a 100755
Binary files a/internal/core/testdata/docs_core.wasm and b/internal/core/testdata/docs_core.wasm differ