Skip to content

feat(crdt): separate local reload from peer admission - #231

Open
mkhairi wants to merge 3 commits into
NodeDB-Lab:mainfrom
mkhairi:fix/crdt-local-import-admission
Open

feat(crdt): separate local reload from peer admission#231
mkhairi wants to merge 3 commits into
NodeDB-Lab:mainfrom
mkhairi:fix/crdt-local-import-admission

Conversation

@mkhairi

@mkhairi mkhairi commented Aug 1, 2026

Copy link
Copy Markdown
Member

Summary

A document this library writes can grow past what it will read back.

export_snapshot has no size bound. Import has three, and until now every import went through them — including the ones where the bytes came from this process rather than from a peer: restoring a document from durable storage, and folding a shallow snapshot back in during compaction. Once a document's snapshot exceeds DEFAULT_MAX_IMPORT_BYTES or its history exceeds DEFAULT_MAX_IMPORT_OPS, that document can no longer be opened. It also can no longer be compacted, because compact_history and compact_at_version re-admit their own output through the same ceiling — so the one operation that would bring it back under the limit is gated by the limit. There is no recovery path from inside the library; raising max_bytes only moves the wall to max_encoded_operations.

The limits themselves are correct for what they were written for: bounding how much work an untrusted peer can make a document do. They just answer a different question when applied to bytes we produced ourselves. This splits the two so each keeps its own answer — the peer path stays capped, and the local path keeps every structural check while dropping the size ceilings.

The same PR carries two accessors that make the same assumption in a smaller way: collection_names and estimated_memory_bytes both walk or serialise the whole document to produce something much cheaper, and both sit on paths that are called repeatedly.

Context in #230. The embedded-side half of that investigation — flush rewriting a full snapshot per tick — is a nodedb-lite change and is open as NodeDB-Lab/nodedb-lite#10; the two PRs share no files.

Changes

feat(crdt): admit locally-generated imports without the peer ceilings

admit_import splits into admit_structure — the authenticated metadata decode and the rejection of regressing per-peer ranges — and the three ceiling checks layered on top of it. import_local runs the structure half and keeps the pending-dependency check; restore, compact_history and compact_at_version use it. The peer path is unchanged, and import_with_limits remains the knob for tuning it.

It is a separate entry point rather than an unbounded CrdtImportLimits constructor, so bytes off the wire cannot reach the exempt path by passing a value.

perf(crdt): stop materialising the document to answer cheap questions

collection_names called get_deep_value, materialising every row and field of every collection to read the top-level keys — O(document) for a list whose length is the number of collections, called per query during name resolution. The shallow value carries the same keys.

estimated_memory_bytes exports a full snapshot as its size proxy. That is the honest proxy, but callers poll it to decide when to compact and an unwritten document returns the same number every time, so it is now computed once per frontier.

Tests

  • local_import_admits_what_the_peer_ceilings_would_reject — the peer path still rejects, the local path admits the same bytes
  • local_import_still_rejects_malformed_metadata — dropping the ceilings does not drop the structural checks
  • collection_names_lists_top_level_keys_only — pins what the shallow read must return
  • estimated_memory_grows_with_data (existing) — covers cache invalidation; it fails if the estimate is memoised without a frontier check

The behaviour past the default ceilings is not covered. Reaching them needs a document over a gigabyte or over a million operations, and a fixture that size does not belong in the suite. A round-trip test against explicit tight limits was written and dropped: it passed with import_local wired to the capped path, so it asserted nothing about the change.

Validation

  • cargo nextest run -p nodedb-crdt — 157/157
  • cargo test -p nodedb-crdt --doc — clean
  • cargo clippy -p nodedb-crdt --all-targets --all-features -- -D warnings — clean
  • cargo fmt — clean
  • cargo check --workspace --all-targets — clean, no downstream caller affected by the new field or the accessor changes

Follow-up

The embedded side does not benefit until its restore path calls import_local — it currently goes through CrdtState::import and stays capped. That is a one-line change in nodedb-lite, blocked on a release carrying import_local. The store preserved in #230 is the acceptance case for both and will be exercised once that lands; it is kept until then.

mkhairi added 2 commits August 1, 2026 23:41
`CrdtImportLimits` bounds how much work an untrusted peer can make a
document do. Restore and compaction went through the same capped path,
where the bound means something else entirely: how large a document this
library may reload after writing it.

`export_snapshot` has no bound at all, so the two are not symmetric — a
document a healthy process wrote can exceed what the same binary will
re-import. Past that point the document neither opens nor compacts, and
compaction is the operation that would bring it back under the limit:
`compact_history` and `compact_at_version` re-admit their own shallow
snapshot, so the recovery path is gated by the ceiling it exists to
escape. Raising `max_bytes` alone only moves the wall to
`max_encoded_operations`, which counts over the whole snapshot's range.

`import_local` keeps every structural check — Loro's authenticated
metadata decode, rejection of regressing per-peer ranges, the pending
dependency check — and drops the size ceilings. Restore, `compact_history`
and `compact_at_version` use it. The peer path keeps its caps, and
`import_with_limits` remains the knob for tuning them.

It is a separate entry point rather than an unbounded `CrdtImportLimits`
constructor so bytes off the wire cannot reach the exempt path by passing
a value.

The behaviour past the default ceilings — a gigabyte, or a million
operations — is not covered by a unit test: a fixture that large does not
belong in the suite. What is covered is that the peer path stays capped
and that dropping the ceilings does not drop the structural checks.
Two accessors on the hot path treated a full document walk as free.

`collection_names` called `get_deep_value`, which materialises every row
and every field of every collection, to read the top-level keys — O(document)
for a list whose length is the number of collections, and name resolution
calls it per query. The shallow value has the same keys.

`estimated_memory_bytes` exports a full snapshot as its size proxy. That
is the honest proxy, but callers poll it to decide when to compact, and a
document nobody is writing returns the same number every time. It is now
computed once per frontier.
@mkhairi
mkhairi force-pushed the fix/crdt-local-import-admission branch from 42a5cf5 to 184114a Compare August 1, 2026 15:42
Rebuilding a CrdtState from a snapshot this process exported itself
went through the plain constructor plus a raw import, which enforced
the peer size ceilings meant for untrusted replicated deltas. Once a
collection outgrew them, replicated-apply seeding and transaction
rollback restore would both fail on data the process wrote itself:
apply would misreport every subsequent delta as malformed, and
rollback would leave the transaction driver with a failure it can't
act on.

Add CrdtState::from_local_snapshot, which imports through the same
admit_local_import path already used by compaction, and route
apply_validated's seeding and snapshot_restore's rollback through it.
Share the compaction swap itself via a new compact_to_frontiers
helper, and move the document plus its derived memory-estimate cache
into a DocumentCell so replacing the document (compaction) can no
longer leave a stale cache describing bytes that no longer exist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants