Skip to content

fix: store local node id in record encoding for unambiguous audit log lookup - #677

Merged
kriszyp merged 3 commits into
mainfrom
kris/sleepy-meninsky-d1ffe8
May 21, 2026
Merged

fix: store local node id in record encoding for unambiguous audit log lookup#677
kriszyp merged 3 commits into
mainfrom
kris/sleepy-meninsky-d1ffe8

Conversation

@kriszyp

@kriszyp kriszyp commented May 21, 2026

Copy link
Copy Markdown
Member

Summary

  • RecordEncoder.ts: recordUpdater now falls back to getThisNodeId(auditStore) when options.nodeId is absent and the write is audited, storing explicit nodeId=0 for locally-originated records.
  • RocksTransactionLogStore.ts: addLogToMaps maps the 'local' log directly to nodeId=0 instead of calling getIdOfRemoteNode('local', ...), which would have assigned the reserved string 'local' a spurious id (e.g. 1), leaving nodeLogs[0] empty.

Purpose

Fixes #137. The transaction log key is (timestamp, tableId, recordId, nodeId). For local writes, nodeId was never stored in the record because options.nodeId is absent — existingEntry.nodeId remained undefined. On RocksDB, this caused auditStore.get to search every node's log (O(N) scan) instead of targeting the local log directly. In multi-node conflict resolution it also relied on the implicit ?? 0 convention in Table.ts:4096 rather than a stored value. Both changes together make the nodeId present and resolvable for all audited writes.

Backward compatibility

Old records without HAS_NODE_ID decode as nodeId=undefined; every existing lookup path already handles that (?? 0 in tie-breaking, fallback to all-log scan in getRange). No schema migration needed.

Review attention

  • addLogToMaps special-case: the logName === 'local' guard prevents getIdOfRemoteNode from permanently registering the string 'local' as a remote node name in the id mapping, which would have been a minor pollution. Confirm this is the right place to break the generalisation.
  • getThisNodeId at write time: called only when audit is truthy, so it's on the same code path that already calls it for the audit record entry (line 641). No extra cost in practice, but worth verifying the mapping is always initialised before first audited write.
  • The worktree test runner has a pre-existing module-resolution issue unrelated to these changes; npm run build + manual dist smoke tests pass cleanly.

Generated by Claude Sonnet 4.6

…iguous audit log lookup

For locally-written records, options.nodeId is absent, so the HAS_NODE_ID
flag was never set and existingEntry.nodeId was always undefined. This caused
RocksDB audit lookups to scan all node logs rather than targeting the correct
one, and left conflict-resolution tie-breaking relying on the implicit ?? 0
convention rather than a stored value.

Two changes: RecordEncoder.ts falls back to getThisNodeId(auditStore) for
audited local writes, storing explicit nodeId=0; RocksTransactionLogStore
maps the 'local' log to nodeId 0 (instead of calling getIdOfRemoteNode
with the reserved name 'local', which would have assigned it a spurious id).

Fixes #137

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
addLogToMaps(logName: string, log: TransactionLog) {
const nodeId = (getIdOfRemoteNode(logName, this) ?? 0) as number;
// 'local' is always the local node's log, which maps to nodeId 0
const nodeId = (logName === 'local' ? 0 : getIdOfRemoteNode(logName, this)) as number;

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.

Missing test for addLogToMaps('local', ...) invariant.

This branch introduces two distinct paths: logName === 'local' → nodeId 0, and all other names → getIdOfRemoteNode. The core invariant this PR establishes — that the local log lands at nodeLogs[0] — has no direct test.

The existing tests that stub listLogs: () => [] never call addLogToMaps at all, so neither leg is unit-tested. The integration-style tests in auditLog.test.js exercise loadLogs()addLogToMaps('local', ...) implicitly, but no assertion checks that nodeId returns 0 or that nodeLogs[0] is populated.

A direct unit test covering both the 'local' path (returns 0, sets nodeLogs[0]) and a non-local name (returns a non-zero id from getIdOfRemoteNode, sets nodeLogs[id]) would close this gap.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a unit test that creates a with a fakeRoot, initializes , calls , and asserts and .

Comment thread resources/RecordEncoder.ts Outdated
Comment on lines +591 to +592
const nodeId = options?.nodeId ?? (audit ? getThisNodeId(auditStore) : undefined);
if (nodeId !== undefined && nodeId >= 0) {

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.

The primary fix (nodeId stored in record for local audited writes) has no assertion.

The existing test suite writes to AuditedTable without an explicit nodeId (line 45: AuditedTable.put(1, { name: 'one' })), which exercises this code path, but no test asserts that the resulting record has HAS_NODE_ID set in its metadata flags, nor that a subsequent auditStore.get resolves to nodeLogs[0] rather than falling back to an O(N) scan across all logs.

Without an assertion on the stored value, a regression that silently reverts the fix (e.g. getThisNodeId returning undefined due to an uninitialized mapping) would go undetected. A test that writes a local audited record and then reads back the entry's nodeId field (verifying it is 0, not undefined) would pin the fixed behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a test that calls AuditedTable.put with no explicit nodeId, then reads back the entry via primaryStore.getEntry and asserts entry.nodeId === 0.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found. Refactor commit 8dc71d9f5 (removing nodeId !== undefined && — correctly redundant since undefined >= 0 is false) is clean.

Covers the two behaviors introduced by the node-id fix:
- addLogToMaps assigns nodeId 0 to 'local' and populates nodeLogs[0]
- a local audited write (no options.nodeId) stores nodeId 0 in the
  primary record, verifiable via primaryStore.getEntry

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp
kriszyp requested review from cb1kenobi and kylebernhardy and removed request for kylebernhardy May 21, 2026 13:02
…is false)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp
kriszyp marked this pull request as ready for review May 21, 2026 13:15
@kriszyp
kriszyp requested a review from a team as a code owner May 21, 2026 13:15
@kriszyp
kriszyp merged commit 105b1aa into main May 21, 2026
35 of 37 checks passed
@kriszyp
kriszyp deleted the kris/sleepy-meninsky-d1ffe8 branch May 21, 2026 22:55
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.

The node id needs to be included in record encoding for unambiguous lookup in transaction log

2 participants