Skip to content

fix: use getSync for system-table point reads (RocksDB MaybePromise) - #1473

Merged
kriszyp merged 1 commit into
mainfrom
kris/get-maybepromise-core
Jun 24, 2026
Merged

kriszyp merged 1 commit into
mainfrom
kris/get-maybepromise-core

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 24, 2026

Copy link
Copy Markdown
Member

Fixes a class of RocksDB store.get() misuse. On RocksDB store.get() returns a MaybePromise — the record synchronously when it's in the block cache/memtable, but a Promise on a cache miss. So point reads whose result is consumed synchronously work while the data is cached, then break once the table grows past the block cache (or right after a cold-cache restart, when the cache is empty). LMDB is always synchronous, so this is RocksDB-only.

Summary

Switch four system-table point reads from get() to the synchronous getSync():

  • Table.ts — the __dbis__ per-peer seq-tracking read in the replication apply path (existingSeq?.nodes), and the residency_by_id / residency_by_set lookups (getResidencyRecord + getResidencyId). A Promise here silently reset sequence tracking or returned a Promise as a residency id.
  • nodeIdMapping.ts — the seq-init existence check on a node-id reassignment (a truthy Promise skipped the init).
  • security/keys.ts — the hdb_certificate staleness comparison in the synchronous loadAndWatch callback (a Promise bypassed the timestamp guard, allowing a stale on-disk cert to overwrite a newer replicated record).

Where to look

  • The raw dbisDb reads use (dbisDb as any).getSync(...) to match the existing precedent in this file (Table.ts:1064). The lmdb type doesn't declare getSync, but it exists at runtime (lmdb/read.js aliases it to the sync get). A properly typed Store<V> surface is a planned follow-up that would let the compiler flag this whole class.
  • getResidencyRecord is changed once and covers all four call sites (a sync .includes(...) in a commit callback plus three context.previousResidency consumers).

Paired with the harper-pro PR, which bumps the core submodule to this commit and carries the replication-side fixes plus unit/integration tests.

Generated by Claude Code (Claude Opus 4.8).

store.get() on RocksDB returns a Promise on a block-cache miss, so point reads
consumed synchronously break once a table grows past the block cache or right
after a cold-cache restart. Switch to getSync at the __dbis__ seq-tracking read
in the replication apply path and the residency lookups (Table.ts), the seq-init
read in nodeIdMapping, and the hdb_certificate staleness check (keys.ts).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces asynchronous .get() calls with synchronous .getSync() calls on RocksDB-backed stores (dbisDb and certificateTable.primaryStore) across resources/Table.ts, resources/nodeIdMapping.ts, and security/keys.ts. This prevents issues where a block-cache miss returns a Promise instead of the actual value, which would otherwise break synchronous logic or cause incorrect truthy evaluations. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp
kriszyp marked this pull request as ready for review June 24, 2026 13:10
@kriszyp
kriszyp merged commit ef78614 into main Jun 24, 2026
81 of 83 checks passed
@kriszyp
kriszyp deleted the kris/get-maybepromise-core branch June 24, 2026 13:51
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.

1 participant