Repository navigation
fix: use getSync for system-table point reads (RocksDB MaybePromise) - #1473
Conversation
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>
There was a problem hiding this comment.
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.
|
Reviewed; no blockers found. |
Fixes a class of RocksDB
store.get()misuse. On RocksDBstore.get()returns aMaybePromise— 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 synchronousgetSync():Table.ts— the__dbis__per-peer seq-tracking read in the replication apply path (existingSeq?.nodes), and theresidency_by_id/residency_by_setlookups (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— thehdb_certificatestaleness comparison in the synchronousloadAndWatchcallback (a Promise bypassed the timestamp guard, allowing a stale on-disk cert to overwrite a newer replicated record).Where to look
dbisDbreads use(dbisDb as any).getSync(...)to match the existing precedent in this file (Table.ts:1064). The lmdb type doesn't declaregetSync, but it exists at runtime (lmdb/read.jsaliases it to the syncget). A properly typedStore<V>surface is a planned follow-up that would let the compiler flag this whole class.getResidencyRecordis changed once and covers all four call sites (a sync.includes(...)in a commit callback plus threecontext.previousResidencyconsumers).Paired with the harper-pro PR, which bumps the
coresubmodule to this commit and carries the replication-side fixes plus unit/integration tests.Generated by Claude Code (Claude Opus 4.8).