fix(audit-sweep): batch A — trivial bug-hunt fixes (process.exit, error leaks, missing try/catch)#882
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 48 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR systematically improves error resilience across the entire network and peer management layers by replacing uncaught ChangesComprehensive Error Handling & Resilience Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR is a targeted audit sweep fixing trivial-but-real bugs across 20 files: removing
Confidence Score: 5/5Safe to merge — all changes are failure-path only; success paths are untouched and the fixes are mechanical in nature. Every changed code path is a failure handler, a missing guard, or a duplicate import removal. No success-path logic is altered. The two observations noted are observability gaps (no logging on dropped broadcast rejections) and a contract change in Peer.fetch() that affects unknown callers, both of which are low-risk in context. broadcastManager.ts and Peer.ts warrant a second look — the former silently drops rejected peer promises with no logging, the latter changes fetch() from throw-on-error to return-error-object. Important Files Changed
Reviews (2): Last reviewed commit: "fix(audit-sweep): address Greptile revie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two findings from Greptile (4/5 confidence): P1 — src/libs/network/manageP2P.ts:74-80 (getMessagesForPartecipant) Adjacent methods in this class were guarded against unset Maps in the original batch A, but this method was missed. `this.messages.get( publicKey)` returns undefined when no relayMessage has ever been called for that key, and the subsequent `for-of` then throws `TypeError: undefined is not iterable`. Same crash category the rest of the PR was targeting. Fix: `?? []` fallback on the Map.get result so callers always receive an iterable Message[]. P2 — src/libs/peer/routines/peerBootstrap.ts:218-222 (genesis I/O) The try/catch around `JSON.parse(fs.readFileSync(...))` wrapped both ENOENT (missing file) and SyntaxError (corrupt JSON) under the same "Corrupt genesis file" label. An operator with a missing genesis file would chase a non-existent corruption issue. Fix: differentiate on `(error as NodeJS.ErrnoException).code === "ENOENT"` and emit "Missing genesis file" vs "Corrupt genesis file" accordingly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
269-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch non-
EADDRINUSEprobe errors ingetNextAvailablePortto avoid aborting port scanning
isPortAvailable()nowreject(err)for anynet.Serverlisten error code other thanEADDRINUSE, butgetNextAvailablePort()awaits it in its scan loop with notry/catch. That means errors likeEACCES(permission denied) /EADDRNOTAVAIL(address not available) will escape the helper instead of being treated as “port unavailable”.In
src/index.ts,getNextAvailablePort(...)is only invoked for signaling, omni, metrics, and mcp probes (no other callers found). Ensure every call site either handles those rejections consistently or (preferably) handle them insidegetNextAvailablePort(log with context and continue scanning, or rethrow with context in a way that matches the intended startup semantics).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 269 - 287, isPortAvailable currently rejects for non-EADDRINUSE listen errors and getNextAvailablePort awaits it without protection, causing scans to abort on EACCES/EADDRNOTAVAIL; update getNextAvailablePort to catch errors from isPortAvailable (or call isPortAvailable inside a try/catch) and treat non-EADDRINUSE errors as "port unavailable" (resolve/continue scanning) while logging the error with context (include probe name/port and error.code) so startup continues for signaling/omni/metrics/mcp probes; alternatively, rethrow only if the error is unrecoverable per your startup semantics, but do not let typical probe listen errors escape the scan loop.
🧹 Nitpick comments (2)
src/libs/peer/routines/peerBootstrap.ts (1)
217-223: ⚡ Quick winConsider typing
genesisDatawith the expected structure.The variable
genesisDatais declared asunknown(line 217) and then passed directly tohashGenesisData()(line 223) without validation. IfhashGenesisDatadoesn't perform structural validation, this could lead to runtime errors when the genesis file contains unexpected data.Consider either:
- Typing
genesisDatawith the expected genesis structure type- Adding validation after parsing to ensure the structure matches expectations
🔧 Suggested improvement
- let genesisData: unknown + let genesisData: GenesisData // or appropriate type try { genesisData = JSON.parse(fs.readFileSync(genesisFile, "utf8")) } catch (error) { throw new Error(`Corrupt genesis file at data/genesis.json: ${error instanceof Error ? error.message : String(error)}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/peer/routines/peerBootstrap.ts` around lines 217 - 223, genesisData is currently typed as unknown and passed straight to hashGenesisData, risking runtime errors; change this by defining an expected Genesis type (e.g., interface Genesis { ... }) and replace genesisData: unknown with genesisData: Genesis, or after JSON.parse run a runtime validation (schema check or small validator function) that verifies required fields/types before calling hashGenesisData(genesisData); ensure the validator throws a clear error mentioning genesisFile on failure so downstream code only receives a correctly shaped Genesis object.src/libs/peer/Peer.ts (1)
403-408: 💤 Low valueConsider typing the fetch return value.
The
fetchmethod now returns either the successful response data or a sentinel object{status: 0, error: string}on failure. The return type is currentlyPromise<any>, which doesn't capture this union behavior.Consider defining a union type or at least documenting the sentinel return value so callers know to check for
status: 0.💡 Optional type improvement
// Add to Peer class or types file type FetchResult<T = any> = T | { status: 0; error: string } // Update method signature async fetch(endpoint: string): Promise<FetchResult> { // ... implementation }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/peer/Peer.ts` around lines 403 - 408, The fetch method in class Peer currently returns Promise<any> but actually yields either response data or a sentinel {status: 0, error: string}; define a proper union return type (e.g., add a type alias FetchResult<T = any> = T | { status: 0; error: string } in the Peer class or a shared types file) and update the signature of Peer.fetch (and any callers if necessary) to async fetch<T = any>(endpoint: string): Promise<FetchResult<T>> so callers can statically check for status === 0 and access the typed success payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/index.ts`:
- Around line 269-287: isPortAvailable currently rejects for non-EADDRINUSE
listen errors and getNextAvailablePort awaits it without protection, causing
scans to abort on EACCES/EADDRNOTAVAIL; update getNextAvailablePort to catch
errors from isPortAvailable (or call isPortAvailable inside a try/catch) and
treat non-EADDRINUSE errors as "port unavailable" (resolve/continue scanning)
while logging the error with context (include probe name/port and error.code) so
startup continues for signaling/omni/metrics/mcp probes; alternatively, rethrow
only if the error is unrecoverable per your startup semantics, but do not let
typical probe listen errors escape the scan loop.
---
Nitpick comments:
In `@src/libs/peer/Peer.ts`:
- Around line 403-408: The fetch method in class Peer currently returns
Promise<any> but actually yields either response data or a sentinel {status: 0,
error: string}; define a proper union return type (e.g., add a type alias
FetchResult<T = any> = T | { status: 0; error: string } in the Peer class or a
shared types file) and update the signature of Peer.fetch (and any callers if
necessary) to async fetch<T = any>(endpoint: string): Promise<FetchResult<T>> so
callers can statically check for status === 0 and access the typed success
payload.
In `@src/libs/peer/routines/peerBootstrap.ts`:
- Around line 217-223: genesisData is currently typed as unknown and passed
straight to hashGenesisData, risking runtime errors; change this by defining an
expected Genesis type (e.g., interface Genesis { ... }) and replace genesisData:
unknown with genesisData: Genesis, or after JSON.parse run a runtime validation
(schema check or small validator function) that verifies required fields/types
before calling hashGenesisData(genesisData); ensure the validator throws a clear
error mentioning genesisFile on failure so downstream code only receives a
correctly shaped Genesis object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6970e91e-3e0d-4db5-a291-c6a929883886
📒 Files selected for processing (20)
src/index.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/mempool.tssrc/libs/communications/broadcastManager.tssrc/libs/consensus/v2/PoRBFT.tssrc/libs/network/bunServer.tssrc/libs/network/handlers/blockHandlers.tssrc/libs/network/handlers/identityHandlers.tssrc/libs/network/manageP2P.tssrc/libs/network/middleware/rateLimiter.tssrc/libs/network/routines/normalizeWebBuffers.tssrc/libs/network/rpcDispatch.tssrc/libs/network/server_rpc.tssrc/libs/omniprotocol/transport/ConnectionPool.tssrc/libs/omniprotocol/transport/PeerConnection.tssrc/libs/peer/Peer.tssrc/libs/peer/PeerManager.tssrc/libs/peer/routines/peerBootstrap.tssrc/libs/peer/routines/peerGossip.tssrc/utilities/mainLoop.ts
💤 Files with no reviewable changes (2)
- src/libs/peer/routines/peerGossip.ts
- src/libs/blockchain/chainBlocks.ts
1 participant
Summary
Batch A of the 2026-05-28 read-only audit sweep. Picks up the trivial, high-confidence findings — fixes that don't need design discussion or feature ownership. All CRITICAL/HIGH items needing real architectural work (nonce stub, vote race, GCREdit stubs, fee-sum assertion,
Sync.ts/secretaryManager.tsexits) are out of scope and will land in a follow-up PR.20 files, +125 / -72.
Fixes by theme
process.exit()outside graceful shutdownEpic-14 fixed this pattern at
index.ts:827. Audit found 9 more in-scope sites. All converted to throw or log-and-continue somain().catch → gracefulShutdownhandles uniformly:peerGossip.ts:29— unconditionalprocess.exit(0)removed (time bomb; one uncomment of the mainLoop callsite would have killed the node silently)PeerManager.ts:62—loadPeerList()filesystem error: log + treat as empty peer list, letpeerBootstraprepopulatepeerBootstrap.ts(7 sites: 81, 86, 98, 113, 123, 180, 207) — bootstrap failures now throw upward instead of exiting with partial state on diskConnectionPool.ts:762—getInstance()init-order error throws instead of permanent killRaw
Errorobjects leaked over RPC wireTypeORM errors include SQL fragments + parameter values; axios errors include internal URLs, auth headers, request bodies. Fixed at all 5 sites with consistent
e instanceof Error ? e.message : String(e)pattern:Peer.ts:385blockHandlers.ts:118rpcDispatch.ts:134identityHandlers.ts:24PeerConnection.ts:890Missing try/catch on async RPC/handler chains
Server-side exceptions used to surface to SDK clients as
400 "Invalid request format"or silent 500s:bunServer.ts:107-130— outer handler catch +Bun.serveerror callbackserver_rpc.ts:239-247—/genesishandler wrappedblockHandlers.ts:75-90—getLastBlock,getLastBlockHash,getLastBlockNumber,getBlockTransactionsall wrapped (same pattern as thegetBlockByHashHIGH fix)Peer.ts:403—Peer.fetch()axios errors now caught with structured failure returnUninitialised Maps in
manageP2P.tsLines 24, 44, 47, 68, 73 — added init guards. Was a remote crash trigger for any RPC route hitting
DemosP2Pbefore init completed.Other targeted fixes
broadcastManager.ts:50,142-153— null-guard onSecretaryManager.getInstance(block.number)+log.warnon silent-return block-loss path;Promise.allfailure mode for first-bad-peer-kills-broadcast addressedrateLimiter.ts:378-401—loadIPssyncfs.readFileSync→fs.promises.readFile(was blocking event loop on large blocked-IP files)normalizeWebBuffers.ts:18,21—e["message"]onunknowncatch →e instanceof Error ? e.message : String(e)(non-Error throws were returning[null, undefined], callers may have read as success)PoRBFT.ts:197— N serial per-tx mempool deletes → single batchremoveTransactionsByHashes(failedTxs)call (method already accepted arrays)chainBlocks.ts:5-6,15-16,30-34— removed duplicateTransactionimport + duplicate fork-migration function imports (merge-conflict residue)mempool.ts:152— removed staleREVIEW: CRITICAL FIXcomment describing already-implemented behaviourindex.ts:269-288—isPortAvailabledifferentiatesEADDRINUSE(return false) from other socket errors likeEACCES/EMFILE(reject); resource exhaustion no longer masquerades as "port busy"mainLoop.ts— minor cleanup tied topeerGossipremovalExplicitly out of scope (deferred to batch B)
These need design decisions, feature ownership, or sit outside the original audit scope. Filed as the next PR:
validateTransaction.ts:358—assignNonce()hardcodedtrue(replay attack vector — needs per-account nonce design)validateTransaction.ts:304— PROD flag bypassing gas balance check (needs PROD policy call)broadcastBlockHash.ts:40-155— pro/con vote race at block-accept timehandleGCR.ts:920,926— GCREditassign/smartContract/escrow/subnetsTxstubs returning{ success: true, message: "Not implemented" }applyGasFeeSeparation.ts:107-118— fee breakdown component sum not asserted against totalSync.ts:282,305,867+secretaryManager.ts:394,410,682,887— 7 moreprocess.exitcalls flagged out-of-scope by reviewer; particularly dangerous inside active consensus roundscreateBlock.ts:72-77—hashNativeTables()proxy unfinishedsubOperations.ts:297-305—addAsset/removeAssetsilent stubsVerification
.ccb/bug-hunt-2026-05-28/FINAL_REPORT.md(not committed — local CCB workspace).testing/devnet/scripts/test-transfer-e2e.sh, fixture compose, funded-genesis JSON) prepared locally but not run in CI — the CCB session was killed before execution. Recommend running it manually against this branch before merge.Test plan
bun installand verify compilation succeedsbun test)testing/devnet, confirm node-1 reaches block productionloadPeerList()permission error in a sandbox and confirm node does not exit (should log and continue)/blocks/lastBlockHashwith a forced DB errorSummary by CodeRabbit
Bug Fixes
Refactor