Skip to content

perf(sync): batch CRDT snapshot pushes so seeding stops paying per-note round trips - #1858

Merged
h4yfans merged 2 commits into
mainfrom
sync/crdt-snapshot-batch-push
Aug 26, 2026
Merged

perf(sync): batch CRDT snapshot pushes so seeding stops paying per-note round trips#1858
h4yfans merged 2 commits into
mainfrom
sync/crdt-snapshot-batch-push

Conversation

@h4yfans

@h4yfans h4yfans commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #1857.

Why

Seeding a vault pushed note bodies one at a time — one POST /sync/crdt/snapshot per note, six serial D1/R2 ops on the server for each. Measured on a 1000-note vault against staging:

iteration POST /sync/push (100 records) snapshot phase (100 bodies)
before the #1842 deploy 52.7 s 37.7 s 15.0 s
after 21.0 s 6.0 s 15.0 s

The record push improved 6.2x from #1833's batch rewrite. The snapshot phase did not move — that rewrite never touched this endpoint.

Of the ~750 ms per snapshot request: ~600 ms server, ~150 ms round trip, and at most 149 ms client CPU — that last figure is arithmetic, not an estimate. The push sustains 6.7 notes/s on one JS thread, so per-note CPU cannot exceed 1000/6.7 ms without saturating it. Bandwidth is not a factor either: ~11 MB total against a measured 830 KB/s upload to the same edge, so line rate for the whole run would be ~14 s against the observed 207 s.

Server

POST /sync/crdt/snapshot/batch takes up to 50 snapshots and amortises what the single-note path pays per note: one db.batch for the metadata reads (chunked at the bind-param ceiling), one reserveStorage for the batch's summed growth, R2 puts at concurrency 8, one db.batch for the upserts, one for the prune.

storeSnapshot keeps every invariant its comments document — a fresh revision on every write, the watermark rule, putBlob ahead of the D1 upsert, refund on failure. The upsert SQL is now a shared constant so the two writers cannot drift on the DO UPDATE SET list. The single-note endpoint is untouched.

Per-note outcomes are reported separately ({ noteId, accepted, sequenceNum? , reason? }, request order), so one bad note does not fail the batch. A quota the batch as a whole cannot satisfy still rejects the whole request, as the single-note path does — nothing partial lands.

Client

The provider splits into prepareSnapshotForNote / settle, so the per-note bookkeeping has exactly one implementation shared by the batched and single paths: binary and localOnly skips, the empty-doc skip, zeroing the pending-snapshot debt before the send and restoring it on failure, closing a doc that was not already open. settle re-reads the doc entry rather than closing over it, because a batch holds notes across a round trip long enough for the 32-slot LRU to evict one.

Encryption moved to the batch function, which reads credentials once per batch instead of three async fetches (including a keychain read) per note.

Two things that were not in the issue's plan

A note with unmerged remote CRDT state never enters a batch. The batch endpoint prunes updates below the new watermark exactly as the single-note one does, so a device that merged around server state it could not verify must not assert "I contain everything up to here". Those notes are split out first — before the capability check, before credentials are read — and routed to the non-pruning POST /sync/crdt/updates path, reusing the existing hasUnmergedRemoteCrdtState branch in runtime.ts so there is one source of truth. Missing this would have been a silent data-loss regression (#1503/#1489).

CRDT_SNAPSHOT_CONCURRENCY drops 5 → 4 rather than rising. The unit is now requests, not notes. An iteration dequeues at most pushBatchSize (100) creates and the wire cap is 50, so it yields at most 2 chunks, and iterations are serial. The old 5 was already over budget at one round trip per note — roughly 400 req/min against the server's 300/min crdt_push bucket.

Backward compatibility

Old servers answer 404 for the batch route and 200 for the single-note one. The first 404 latches a per-session flag and every later chunk falls back to one request per note, so a vault pays one wasted request rather than one per batch — the same mechanism as the existing snapshotMetaUnsupported latch. A 413 falls back per note without latching: the aggregate body being too large says nothing about which note is oversized, and only the per-note path names it to the user.

crdtPushRateLimit now asks bootstrapRateLimitElevation for a multiplier. None is registered for crdt_push and this deliberately changes nothing at runtime — BOOTSTRAP_ELEVATION_MULTIPLIERS is documented as pull-only. With 50 notes per request a 1000-note vault needs ~20 requests, so the elevation the issue asked for is no longer necessary.

Verification

pnpm typecheck            19/19 successful
pnpm lint                 0 errors (no warnings in changed files)
pnpm test                 11/11 tasks — desktop 1405 files, sync-server 80 files
pnpm ipc:check            pass
pnpm check:architecture   pass
pnpm check:contracts      pass
pnpm docs:impact --strict pass
pnpm docs:build           pass
git diff --check          clean

New tests: 11 service-level cases against the real SQLite/migration harness, 9 route cases, 10 batch-function cases, plus provider/http-client coverage. engine-crdt.test.ts came off the tsconfig.test.node.json exclude backlog — the list shrank by one, nothing was added.

Not measured yet: the end-to-end effect on a real seeding run. That is the next step against staging, using the numbers above as the baseline.

…te round trips

Seeding a vault pushed note bodies one at a time: one POST /sync/crdt/snapshot
per note, and six serial D1/R2 ops on the server for each one. Measured on a
1000-note vault against staging, 100 bodies took 15.0s — unchanged by the #1833
record-push batch rewrite, which never touched this endpoint. Of the ~750ms per
request, ~600ms was server time, ~150ms round trip, and at most 149ms client CPU
(the push sustains 6.7 notes/s on one thread, so per-note CPU cannot exceed that
without saturating it).

Server: POST /sync/crdt/snapshot/batch takes up to 50 snapshots and amortises
the work — one db.batch for metadata reads chunked at the bind-param ceiling,
one reserveStorage for the summed growth, R2 puts at concurrency 8, one
db.batch for the upserts, one for the prune. storeSnapshot keeps every
invariant it documented; the upsert SQL is now a shared constant so the two
writers cannot drift on the DO UPDATE SET list. The single-note endpoint is
untouched.

Client: the provider splits into prepareSnapshotForNote / settle so the
per-note bookkeeping (binary and localOnly skips, the empty-doc skip, zeroing
the pending-snapshot debt before the send and restoring it on failure, closing
a doc that was not already open) has exactly one implementation, shared by the
batched and single paths. settle re-reads the doc entry rather than closing
over it, because a batch holds notes across a round trip long enough for the
LRU to evict one.

A note with unmerged remote CRDT state never enters a batch. The batch endpoint
prunes updates below the snapshot watermark exactly as the single-note one
does, so those notes are split out first and routed to the non-pruning
/sync/crdt/updates path, preserving #1503/#1489.

Old servers answer 404 for the batch route and 200 for the single-note one; the
first 404 latches a per-session flag and every later chunk falls back, so a
vault pays one wasted request, not one per batch. A 413 falls back per note
without latching, because the aggregate body being too large says nothing about
which note is oversized and only the per-note path names it to the user.

CRDT_SNAPSHOT_CONCURRENCY drops 5 -> 4: the unit is now requests, not notes. An
iteration dequeues at most pushBatchSize (100) creates and the wire cap is 50,
so it yields at most 2 chunks, and iterations are serial. The old 5 was already
over budget at one round trip per note — roughly 400 req/min against the
server's 300/min crdt_push bucket.

crdtPushRateLimit now asks bootstrapRateLimitElevation for a multiplier. None is
registered for crdt_push, so this changes nothing at runtime and deliberately
does not: BOOTSTRAP_ELEVATION_MULTIPLIERS is documented as pull-only. With 50
notes per request a 1000-note vault needs 20 requests, so the elevation the
issue asked for is no longer necessary.

Closes #1857
@github-actions github-actions Bot added documentation Improvements or additions to documentation test labels Aug 26, 2026
@github-actions

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 248b427.

@h4yfans
h4yfans merged commit f514aed into main Aug 26, 2026
16 checks passed
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.46512% with 25 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
apps/desktop/src/main/sync/runtime.ts 14.28% 12 Missing ⚠️
apps/desktop/src/main/sync/crdt-provider.ts 89.18% 8 Missing ⚠️
apps/desktop/src/main/sync/crdt-snapshot-batch.ts 94.28% 4 Missing ⚠️
...s/desktop/src/main/sync/engine/push-coordinator.ts 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bootstrap-sync] P0.8: CRDT snapshot push batch rewrite — seeding bodies still pay 6 serial D1/R2 ops per note

1 participant