Repository navigation
Stop creating empty transaction logs named after unmapped node ids - #2787
Merged
Merged
Conversation
…ode id
RocksTransactionLogStore.getRange({ log: <number> }) fell through to
rootStore.useLog(number) when the node id had no log, and useLog is
get-or-create: every audit lookup by a relayed or removed origin's id
created a permanently empty log named after the id, which the next
load then mapped to a freshly minted node id. A numeric log is now a
pure lookup into the id→log map and an unmapped id yields an empty
range; string log names keep get-or-create.
Closes #2778
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lt1QAheetzHhsS8TFW9cGj
…mes a log Refs #2778 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lt1QAheetzHhsS8TFW9cGj
…its enforcement Refs #2778 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lt1QAheetzHhsS8TFW9cGj
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates RocksTransactionLogStore.getRange to ensure that a numeric transaction-log selector (representing a node ID) is treated strictly as a lookup and never as a log name. This prevents the unintended creation of a new transaction log via useLog when an unmapped node ID is queried, returning an empty range instead. The changes are documented in DESIGN.md and verified with a new unit test in auditLog.test.js. There are no review comments to address.
kriszyp
marked this pull request as ready for review
September 25, 2026 13:02
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi and
heskew
September 25, 2026 13:02
Contributor
|
Reviewed; no blockers found. |
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
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.
Reading the audit log by a node id that has no transaction log of its own (a relayed origin, or one whose log was removed) no longer creates an empty RocksDB transaction log named after that id.
RocksTransactionLogStore.getRangenow treats a numericlogas a lookup into the node-id→log map and returns an empty range when there is no log; a string log name keeps get-or-create.Closes #2778
Before: every
auditStore.get(key, tableId, id, nodeId)for such an id fell through torootStore.useLog(nodeId), which is get-or-create in rocksdb-js, so a permanently empty log"53"appeared on disk. On the next startloadLogs()treated"53"as a node name and minted a new node id for it. The lookup itself missed either way, because the new log was empty.For the human reviewer
getRange(replication and subscriptions). Not superseded by in-flight work; Report a transaction log's corrupt-frame stop to getRange callers through an onCorruptFrame hook editsgetRangefor a different reason and may conflict textually, not semantically.put()already routes those writes to the via-node orlocallog). Adding a log line later is cheap."53"etc., and the node ids minted for them, stay. Alternative: prune empty numeric-named logs at load. Not done because that cannot tell a junk log from a node whose configured name is all digits, and deleting stores at startup races writers. After every node runs this fix, the number of junk logs stops growing; during a rolling deploy, older nodes still create them. Removing them is an operator action or a separate issue.put()wrote to the via-node orlocallog for an origin with no log of its own. This PR does not change that: those lookups already missed (they opened an empty numeric log). Fixing it needsviaNodeIdat the 12get/getSynccall sites or an all-logs scan per miss, so it belongs in its own issue.Planning review:
Framing-Verdict: chosen-approach-sound(Codex,--mode plan). Alternatives rejected there: change rocksdb-jsuseLog(the numeric id is the wrong key whether or notuseLogcreates), resolve the id to whereput()wrote (see 4), clean up junk logs at load (see 3).Changes
resources/RocksTransactionLogStore.ts— ingetRange, a numericlogresolves through the existinglogById(nodeLogs[id] ?? loadLogs()[id], the same lookupput()uses). A stringlogkeepslogByName→loadLogs()→useLog(name). Only a resolved log reachestrackCorruptFrames; a numeric miss gets an empty source iterator in its own branch instead (trackCorruptFrameswould dereference the missing log), andgetRangestill returns the usualTransactionLogIterablewithcorruptFrameStop,failedLogsandexactStartFailuresattached.log: undefinedstill reads all logs.resources/DESIGN.md,DESIGN.md— a short note (and its index line) stating that a numeric selector never names a log, pinned by the new test.Verification
unitTests/resources/auditLog.test.js"a numeric log id with no log misses without creating one", RocksDB only; skipped on LMDB, which has no per-node logs): a local write is found by node id 0; a peer write through a source-apply transaction is found by its mapped node id and bynodeId: undefined(all logs); an unmapped id returnsundefinedfromauditStore.get, andgetRangefor it is empty with no failures and does not addString(id)torootStore.listLogs(); a string log name still creates its log.07b2c9015the test fails withreading by node id 1001 must not create a transaction log named after it(log"1001"created); with the fix it passes. rocksdb-js 2.10.0 probe:db.useLog(53)makeslistLogs()return['53', 'local'].npm run test:unit:resources— 3030 passing, 48 pending, 0 failing.npm run test:unit:main— 1843 passing, 8 failing, none in transaction-log code. 7 were timeouts inunitTests/components/{packageComponent,prepareApplicationSerialization,RuntimeModuleTracker}.test.jsunder a load average of ~18 from other worktrees' suites; those three files pass 22/22 when run alone. The 8th,gitCredentials.test.js"grants the credential environment to the spawn that clones", fails locally because the dispatch worker's own shell setsGIT_CONFIG_GLOBAL/GIT_EDITOR/GIT_SSH_COMMAND, independent of this diff.npm run test:integration:all— 2150 pass, 0 fail, 18 skipped, 6 cancelled. All 6 cancelled areOllamaBackend against a real Ollama instance, which needs Ollama models this machine does not have.npx oxlint --deny-warningsandprettier --checkon the changed files,npm run check:design-docs— clean.Unit Test (Node.js v26)run failed twice. One failure wascaching.test.js:365"Cache invalidation triggers updates", which asserts after fixed 10 ms / 20 ms sleeps around a 5 ms TTL and never reaches a numeric log lookup. The other was theUnit tests: lmdbstep's 4-minute timeout, whichmainalso hit in 3 of its last 6 completed unit-test runs. The caching test passed 5/5 locally on Node 26.2 and on this PR's Node 22 and 24 legs. The rerun of the Node 26 job passed, and all 43 checks are green.auditStore.geton a real RocksDB-backed table).Signed: Claude Opus 5.5
🤖 Generated with Claude Code
https://claude.ai/code/session_01Lt1QAheetzHhsS8TFW9cGj
Complexity: medium
Dispatch: task
chat-issue-harper-2778-kriszyp· queued by unknown · ran by claude/opus/xhigh · worker kzyp-xps-1Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=3; full=1 @ f450474
Human-Review-Need: 3 (decisions: numeric-miss-is-empty-not-fallback, no-cleanup-of-stray-numeric-logs) @ f450474