Repository navigation
fix(replication): use getSync for seq/copyCursor resume-cursor reads (RocksDB MaybePromise) - #484
Merged
Merged
Conversation
…(RocksDB MaybePromise) Completes the RocksDB MaybePromise getSync audit (#476/#474/#470, which swept only hdb_nodes) for the replication subscription handshake's resume-cursor reads. __dbis__ is RocksDB, whose get() returns a Promise on a block-cache miss and the value synchronously on a hit. The handshake read the per-source seq/copyCursor with an un-awaited get(); right after a restart/upgrade the cursor key is cold, so get() returns a Promise. For a seq row Promise?.seqId is undefined, startTime collapses to 1, and the "no resume cursor for this source" branch forces an unnecessary FULL COPY of a node that holds a valid persisted cursor. The larger the system DB, the more block-cache pressure and the likelier the eviction — so large customer databases full-copy on a routine upgrade while small ones resume incrementally (the field signature). - replicationConnection.ts: route the seq (direct + proxied) and copyCursor point reads through a new readDbisCursorSync helper (getSync), mirroring the writer at core Table.ts updateRecordedSequenceId / nodeIdMapping.ts. - replicator.ts: await the residency_by_id get() in the async load() path (a miss there made residency a Promise that the following for...of would throw on). - unit test covering the cold-cache (get()->Promise) regression for seq + copyCursor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 25, 2026 02:37
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
kriszyp
added a commit
that referenced
this pull request
Jun 25, 2026
…edCopyCursor (#485) Tightens discardMalformedCopyCursor's first param from `any` to `DbisCursor | undefined` (and `dbisDB` to `DbisStore | undefined`), so the #485 compile-time invariant reaches the copyCursor path too: passing a raw dbisDB.get() result (a MaybePromise on a RocksDB block-cache miss) is now a tsc error at the call site instead of a Promise that reads as malformed and gets remove()d from disk — a spurious full-recopy on reconnect (#321). Type-only: the production read already uses getSync via readDbisCursorSync (#484), so compiled JS is byte-identical; this closes the type gap David flagged so the whole __dbis__ read path is compile-safe, not just `seq`. tsc error multiset unchanged from baseline; enforcement verified (a raw get() at the call site now yields TS2345 MaybePromise-not-assignable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jun 25, 2026
…se is a compile error (#485) Follow-up to #484 (the seq/copyCursor getSync fix): makes the whole MaybePromise misuse class a compile-time error rather than a latent full-copy-on-upgrade bug, mirroring what #477 did for the hdb_nodes store. On RocksDB `__dbis__.get()` is a MaybePromise — the value synchronously on a block-cache hit, a Promise on a miss. A new `DbisStore` type declares `get(): MaybePromise<DbisCursor | undefined>` and `getSync(): DbisCursor | undefined`, with an index signature so put/remove/getRange/etc. fall through (no broad retype / blast radius). `tableSubscriptionToReplicator.dbisDB` and the `readDbisCursorSync` helper are typed against it, so any synchronous consumption of `get()` — `store.get(k)?.seqId`, truthiness, spread — is now a `tsc` error: use `getSync()` or `await`. - This makes the #484 fix an invariant: reverting `readDbisCursorSync`'s `getSync` back to `get()` no longer satisfies the `DbisCursor | undefined` return type (TS2322), and a direct `tableSubscriptionToReplicator.dbisDB.get(k)?.seqId` read errors (seqId does not exist on MaybePromise). Both verified locally. - Removed a now-redundant `?? 1` on the seq read (every consumer is already `sequenceEntry?.seqId ?? 1/0`); behavior-identical. Pure types + one redundancy removal; no runtime behavior change. tsc adds zero errors vs baseline; unit suite green (194). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jun 25, 2026
…edCopyCursor (#485) Tightens discardMalformedCopyCursor's first param from `any` to `DbisCursor | undefined` (and `dbisDB` to `DbisStore | undefined`), so the #485 compile-time invariant reaches the copyCursor path too: passing a raw dbisDB.get() result (a MaybePromise on a RocksDB block-cache miss) is now a tsc error at the call site instead of a Promise that reads as malformed and gets remove()d from disk — a spurious full-recopy on reconnect (#321). Type-only: the production read already uses getSync via readDbisCursorSync (#484), so compiled JS is byte-identical; this closes the type gap David flagged so the whole __dbis__ read path is compile-safe, not just `seq`. tsc error multiset unchanged from baseline; enforcement verified (a raw get() at the call site now yields TS2345 MaybePromise-not-assignable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the RocksDB
MaybePromisegetSyncaudit (#476/#474/#470, which swept only thehdb_nodespoint reads) for the replication subscription handshake's per-source resume-cursor reads.__dbis__is a RocksDB store, soget()is aMaybePromise— it returns the value synchronously on a block-cache hit, but a Promise on a cache miss. The handshake (replicateOverWS) read the per-sourceseq/copyCursorcursors with a non-awaitedget(). Right after a restart/upgrade — exactly when the handshake runs — the cursor key is cold, soget()returns a Promise:seq:Promise?.seqIdisundefined→startTimecollapses to1→ the "no resume cursor for this source" branch forces an unnecessary full copy of a node that holds a valid persisted cursor.copyCursor: the Promise is treated as a malformed/absent cursor → an interrupted-copy resume point is dropped.seq:Promise?.nodesisundefined→ the proxied leading-duplicate fast-skip (Replication: transitive/proxied re-delivery floods peers with already-applied out-of-order writes (reduce volume; complements harper#1310) #399) is silently disarmed.This scales with system-DB size: the larger the system DB, the more block-cache pressure, the likelier the
seqkey is evicted — so large customer databases full-copy on a routine upgrade while small ones resume incrementally (the observed field signature on the 5.1.12 wave).Changes
replication/replicationConnection.ts: newreadDbisCursorSync(dbisDB, kind, id)helper (getSync); the seq (direct + proxied) and copyCursor handshake reads route through it. Mirrors the writer atcore/resources/Table.tsupdateRecordedSequenceIdandnodeIdMapping.ts, which already usegetSyncwith the same rationale.replication/replicator.ts:awaittheresidency_by_idget()in the asyncload()path. A Promise there would makeresidencytruthy and the followingfor...ofthrow;load()is already async, soawaitis the right fix rather than blocking withgetSync.unitTests/replication/readDbisCursorSync.test.mjsreproducing the cold-cache (get()→ Promise) regression for bothseqandcopyCursor, mirroringreadNodeRowSync.test.mjs.Where to look
get()andgetSync()return the value synchronously); the only change is that a cold miss now blocks for the real value instead of yielding a Promise that silently reads as "no cursor."replicator.tsload()usesawait(yields) rather thangetSync(blocks), since it's a hotter retrieval path and already async — worth a sanity check that the short-circuit (entry.residency || (await …)) preserves the prior semantics.Related
hdb_nodes-only sweep fix(replication): use getSync for hdb_nodes point reads (RocksDB MaybePromise) #476/fix(replication): synchronous getSync for hdb_nodes point lookups (#470) #474/replication: hdb_nodes point lookups assume sync get() but system is RocksDB (MaybePromise) — self-gate/auth read a Promise, disabling recovery #470 (+ typed-store follow-up feat(types): make sync get() misuse on hdb_nodes a compile error (#470 follow-up) #477).Follow-up (not in this PR)
Same durable-prevention treatment as #477: a typed
Store<V>for the__dbis__/seq store whose sync.get()is the typed surface, soMaybePromisemisuse on these reads becomes a compile error rather than a silent full-copy trigger.🤖 Generated by Claude (Opus 4.8). Reviewed by Codex (clean — no correctness issues) and the GitHub
claudereview bot (no blockers). The Gemini leg was skipped — Gemini's daily quota is exhausted (the gemini-code-assist bot posted a quota warning); proceeding on the Codex review since the change mirrors the already-reviewed #476/#477 pattern.