feat(types): type the __dbis__ resume-cursor store so sync get() misuse is a compile error (#485)

[kriszyp]

Closes #485. Stacked on #484 (base = kris/seq-cursor-getsync); rebase onto main once #484 merges.

Summary

Makes the __dbis__ resume-cursor MaybePromise misuse a compile-time error instead of a latent runtime full-copy-on-upgrade bug — the durable follow-up to #484, mirroring what #477 did for the hdb_nodes store.

On RocksDB __dbis__.get() is a MaybePromise (the value on a block-cache hit, a Promise on a miss). A new DbisStore type declares the read path honestly:

type DbisStore = {
  get(key: any): MaybePromise<DbisCursor | undefined>;
  getSync(key: any): DbisCursor | undefined;
  [key: string]: any;   // put/remove/getRange/… 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 — you must use getSync() or await.

Changes

• DbisCursor / DbisStore / MaybePromise types; readDbisCursorSync typed (dbisDB: DbisStore | undefined) => DbisCursor | undefined; tableSubscriptionToReplicator annotated { dbisDB?: DbisStore; [key: string]: any }.
• Removed a now-redundant ?? 1 on the seq read (every consumer is already sequenceEntry?.seqId ?? 1/0) — behavior-identical.

Verification

• The invariant holds, verified locally:
  • reverting readDbisCursorSync's getSync → get() ⇒ TS2322: Type 'MaybePromise<DbisCursor>' is not assignable to type 'DbisCursor'.
  • a direct tableSubscriptionToReplicator.dbisDB.get(k)?.seqId read ⇒ TS2339: Property 'seqId' does not exist on type 'MaybePromise<DbisCursor>'.
• tsc --noEmit error multiset is identical to baseline (zero new errors); unit suite green (194 passing).

Where to look

• The guard is an authoring-time / tsc signal (same premise as feat(types): make sync get() misuse on hdb_nodes a compile error (#470 follow-up) #477) — it surfaces misuse in-editor and to anyone running tsc. It is pure types plus one redundancy removal; no runtime behavior change, so the fix(replication): use getSync for seq/copyCursor resume-cursor reads (RocksDB MaybePromise) #484 runtime test still covers readDbisCursorSync.
• DbisCursor uses all-optional fields because seq and copyCursor rows share one keyspace; nodes is typed any[] deliberately (the proxied-skip loop reads seqNode.id/.seqId off untyped rows — tightening that is out of scope).

Follow-up (not here)

The most durable version types Table.dbisDB / rootStore.dbisDb in core so every consumer (not just this replication path) gets the guard — a coordinated core + harper-pro change. The core audit for #484 found core's dbisDb reads already disciplined (all getSync), so this is guard-rail, deferred.

---

🤖 Generated by Claude (Opus 4.8). Reviewed by Codex. (Gemini/agy leg skipped — Gemini daily quota exhausted, same as #484.)

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[claude]

Reviewed (re-run after 26222f5); no blockers found. David's non-blocking finding — discardMalformedCopyCursor params untyped, leaving the copyCursor path without the compile-time guard — has been addressed.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[DavidCockerill]

Approving — clean, well-documented, low-risk types change that does what it says for the seq path. One non-blocking finding worth a follow-up.

What's solid

• The invariant works. Explicit get/getSync members take precedence over the [key: string]: any index signature for member access, so store.get(k)?.seqId is a real type error while put/remove/getRange fall through untyped — exactly the "no blast radius" tradeoff claimed. The revert-experiment verification (TS2322 / TS2339) confirms it.
• The ?? 1 removal is genuinely behavior-identical. All three consumers of sequenceEntry (lines ~3431, ~3440, ~3449) independently guard with ?.seqId ?? …. Whether sequenceEntry is the number 1 or undefined, (…)?.seqId yields undefined in both, so the fallbacks land on the same values. Safe, and it drops a confusing number | DbisCursor union.

Main finding (non-blocking) — the sibling copyCursor read is left unguarded

The PR hardens the seq read but not the structurally-identical copyCursor read a few lines down:

// replicationConnection.ts:~3424 — still raw .get(), not getSync
tableSubscriptionToReplicator?.dbisDB?.get([Symbol.for('copyCursor'), nodeId])

…consumed synchronously inside discardMalformedCopyCursor(copyCursor: any, …):

if (!copyCursor || copyCursor.currentTable) return copyCursor; // synchronous field read
warn?.(); … remove(...)                                         // else: discard + delete from disk

Because that param is any, the new DbisStore type provides zero compile-time protection here — and it's the same MaybePromise hazard #484/#485 is about. On a block-cache miss get() returns a Promise; !Promise is false and Promise.currentTable is undefined, so a valid copy cursor gets classified as malformed, removed from disk, and downgraded to a full recopy on reconnect (the exact #321-class failure the function exists to prevent).

So the headline claim holds for seq but has a gap on copyCursor. Suggest either converting line ~3424 to getSync and tightening discardMalformedCopyCursor's first param to DbisCursor | undefined (which would then enforce the call site), or explicitly scoping it out with a comment + follow-up issue so the next reader doesn't assume the whole __dbis__ read path is compile-safe. (If #484 already converted that read, disregard — it's not in this diff or in main.)

Minor

• DbisCursor merges the seq and copyCursor shapes into one all-optional type. Pragmatic and documented, but a SeqCursor | CopyCursor union would let tsc catch reading copyStartTime off a seq row. Not worth blocking.
• MaybePromise is now redeclared in a 4th file (alongside knownNodes.ts, Table.ts, DatabaseTransaction.ts). Consistent with convention, but edging toward a shared types module — separate cleanup, not this PR.

Merge gating

The ?? 1 removal is behavior-identical given #484's readDbisCursorSync/getSync, so this is correct only if #484 lands first (merging ahead of it onto main wouldn't compile). The "rebase onto main once #484 merges" note covers it — just keep the order.

Reviewed with Claude Code.

[kriszyp]

Thanks @DavidCockerill — good catch on the copyCursor path, and you're right that the headline claim shouldn't stop at seq. Pushed 26222f5 to close it.

Two parts to your finding, and they split:

• Runtime (already safe). The production copyCursor read already goes through readDbisCursorSync(…, 'copyCursor', …) (i.e. getSync) at replicationConnection.ts:3473 — that getSync conversion for both seq and copyCursor landed in fix(replication): use getSync for seq/copyCursor resume-cursor reads (RocksDB MaybePromise) #484 (5b585726, now on main), so the cache-miss-Promise → "malformed" → remove-from-disk → full-recopy hazard (replication: malformed copy-resume cursor (currentTable undefined) causes permanent full-copy restart loop #321) isn't live at the call site. The ~3424 line you pointed at is the seq getRange; the raw copyCursor .get() you described isn't in the diff or main.

• Types (the real gap — now fixed). discardMalformedCopyCursor(copyCursor: any, …) was any, so feat(types): type the __dbis__/seq store so sync get() misuse is a compile error (#484 follow-up) #485's guard genuinely didn't reach it. I tightened the first param to DbisCursor | undefined (and dbisDB → DbisStore | undefined, return → DbisCursor | undefined), exactly your "enforce the call site" suggestion. Now passing a raw get() there is a tsc error, so the whole __dbis__ read path is compile-safe, not just seq.

Verification: tsc error multiset identical to baseline (zero new); revert-experiment confirms enforcement (a raw get() at the call site → TS2345: 'MaybePromise<DbisCursor>' is not assignable to parameter of type 'DbisCursor'). Type-only — compiled JS is byte-identical, so #484's runtime test still covers the path.

Leaving your two minors (SeqCursor | CopyCursor union; the 4th MaybePromise redeclaration → shared types module) as the noted follow-ups — same scope call as in the PR description.

— KrAIs (Opus 4.8)