feat(node): per-node life-cycle management #1143
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughContext propagation and cancellation were added widely: Start/Run methods now accept contexts and components store/observe them to cancel loops. Node replaced its done channel with ctx/cancel and threads the context into ledger, connection manager, peer governor, utxorpc, and protocol handlers. Ledger processing and chain iteration gained context-aware reads and cancellation; ChainIterator supports Cancel(). Ouroboros protocol handlers (blockfetch, chainsync, txsubmission) were reworked to validate connections, handle connection errors, and use select/goroutine patterns for cancellation. Tests and callers were updated to pass contexts. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="node.go">
<violation number="1" location="node.go:158">
P1: Double shutdown: `ledgerState.Close()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 3. The component will be closed twice during shutdown, which could cause errors or panics. Either remove this registration or remove the explicit call in `shutdown()`.</violation>
<violation number="2" location="node.go:202">
P1: Double shutdown: `connManager.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 2. The component will be stopped twice during shutdown, which could cause errors or panics.</violation>
<violation number="3" location="node.go:227">
P1: Double shutdown: `peerGov.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 1. The component will be stopped twice during shutdown.</violation>
<violation number="4" location="node.go:244">
P1: Double shutdown: `utxorpc.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 1. The component will be stopped twice during shutdown.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
node.go
Outdated
| return err | ||
| } | ||
| n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error { | ||
| return n.connManager.Stop(ctx) |
There was a problem hiding this comment.
P1: Double shutdown: connManager.Stop() is registered in shutdownFuncs here, but it's also explicitly called in shutdown() Phase 2. The component will be stopped twice during shutdown, which could cause errors or panics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node.go, line 202:
<comment>Double shutdown: `connManager.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 2. The component will be stopped twice during shutdown, which could cause errors or panics.</comment>
<file context>
@@ -192,6 +198,9 @@ func (n *Node) Run() error {
return err
}
+ n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error {
+ return n.connManager.Stop(ctx)
+ })
// Configure peer governor
</file context>
node.go
Outdated
| if err := n.ledgerState.Start(n.ctx); err != nil { | ||
| return fmt.Errorf("failed to start ledger: %w", err) | ||
| } | ||
| n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error { |
There was a problem hiding this comment.
P1: Double shutdown: ledgerState.Close() is registered in shutdownFuncs here, but it's also explicitly called in shutdown() Phase 3. The component will be closed twice during shutdown, which could cause errors or panics. Either remove this registration or remove the explicit call in shutdown().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node.go, line 158:
<comment>Double shutdown: `ledgerState.Close()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 3. The component will be closed twice during shutdown, which could cause errors or panics. Either remove this registration or remove the explicit call in `shutdown()`.</comment>
<file context>
@@ -149,9 +152,12 @@ func (n *Node) Run() error {
+ if err := n.ledgerState.Start(n.ctx); err != nil {
return fmt.Errorf("failed to start ledger: %w", err)
}
+ n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error {
+ return n.ledgerState.Close()
+ })
</file context>
node.go
Outdated
| return err | ||
| } | ||
| n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error { | ||
| return n.utxorpc.Stop(ctx) |
There was a problem hiding this comment.
P1: Double shutdown: utxorpc.Stop() is registered in shutdownFuncs here, but it's also explicitly called in shutdown() Phase 1. The component will be stopped twice during shutdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node.go, line 244:
<comment>Double shutdown: `utxorpc.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 1. The component will be stopped twice during shutdown.</comment>
<file context>
@@ -227,9 +240,16 @@ func (n *Node) Run() error {
return err
}
+ n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error {
+ return n.utxorpc.Stop(ctx)
+ })
</file context>
node.go
Outdated
| return err | ||
| } | ||
| n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error { | ||
| n.peerGov.Stop() |
There was a problem hiding this comment.
P1: Double shutdown: peerGov.Stop() is registered in shutdownFuncs here, but it's also explicitly called in shutdown() Phase 1. The component will be stopped twice during shutdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node.go, line 227:
<comment>Double shutdown: `peerGov.Stop()` is registered in `shutdownFuncs` here, but it's also explicitly called in `shutdown()` Phase 1. The component will be stopped twice during shutdown.</comment>
<file context>
@@ -214,6 +223,10 @@ func (n *Node) Run() error {
return err
}
+ n.shutdownFuncs = append(n.shutdownFuncs, func(ctx context.Context) error {
+ n.peerGov.Stop()
+ return nil
+ })
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
node.go (2)
155-245: Critical: Duplicate shutdown calls for components.The code registers shutdown hooks for
ledgerState,connManager,peerGov, andutxorpc(lines 158-160, 201-203, 226-229, 243-245), but theshutdown()method also stops these same components manually in phases 1-3 (lines 283-320) before executing the registered shutdown functions in phase 4 (lines 327-331). This causes each component to be stopped twice, which may lead to errors, panics, or resource leaks depending on how each component's Stop/Close method handles repeated calls.🔎 Recommended solution
Choose one approach:
Option 1 (recommended): Use shutdown hooks exclusively
Remove manual shutdown calls and rely solely on the registered hooks, ensuring they're called in the correct order:
func (n *Node) shutdown() error { // Create shutdown context with timeout (default 30s if not configured) shutdownTimeout := 30 * time.Second if n.config.shutdownTimeout > 0 { shutdownTimeout = n.config.shutdownTimeout } ctx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) defer cancel() if n.cancel != nil { n.cancel() } var err error n.config.logger.Debug("starting graceful shutdown") - - // Phase 1: Stop accepting new work - n.config.logger.Debug("shutdown phase 1: stopping new work") - - if n.peerGov != nil { - n.peerGov.Stop() - } - - if n.utxorpc != nil { - if stopErr := n.utxorpc.Stop(ctx); stopErr != nil { - err = errors.Join(err, fmt.Errorf("utxorpc shutdown: %w", stopErr)) - } - } - - // Phase 2: Drain and close connections - n.config.logger.Debug("shutdown phase 2: draining connections") - - if n.mempool != nil { - if stopErr := n.mempool.Stop(ctx); stopErr != nil { - err = errors.Join(err, fmt.Errorf("mempool shutdown: %w", stopErr)) - } - } - - if n.connManager != nil { - if stopErr := n.connManager.Stop(ctx); stopErr != nil { - err = errors.Join( - err, - fmt.Errorf("connection manager shutdown: %w", stopErr), - ) - } - } - - // Phase 3: Flush state and close database - n.config.logger.Debug("shutdown phase 3: flushing state") - - if n.ledgerState != nil { - if closeErr := n.ledgerState.Close(); closeErr != nil { - err = errors.Join( - err, - fmt.Errorf("ledger state close: %w", closeErr), - ) - } - } - - // Phase 4: Cleanup resources - n.config.logger.Debug("shutdown phase 4: cleanup resources") // Call registered shutdown functions + n.config.logger.Debug("executing shutdown functions") for _, fn := range n.shutdownFuncs { if fnErr := fn(ctx); fnErr != nil { err = errors.Join(err, fmt.Errorf("shutdown function: %w", fnErr)) } } n.shutdownFuncs = nil if n.eventBus != nil { n.eventBus.Stop() } n.config.logger.Debug("graceful shutdown complete") close(n.done) return err }Note: You'll also need to register a shutdown hook for mempool if using this approach.
Option 2: Remove shutdown hooks and keep manual shutdown
Remove the shutdown hook registrations (lines 158-160, 201-203, 226-229, 243-245) and keep only the manual shutdown logic.
296-300: Note: Mempool lacks a shutdown hook.Unlike other components (
utxorpc,connManager,peerGov,ledgerState), the mempool doesn't have a registered shutdown hook, yet it's stopped manually here. This inconsistency should be addressed when resolving the duplicate shutdown issue flagged in the earlier comment.
🧹 Nitpick comments (1)
node.go (1)
248-252: Optional: Simplify nil check.Since
n.ctxis always initialized on line 78 before reaching this point, the nil check is unnecessary. However, this defensive check is acceptable if you want to maintain robustness against future refactoring.🔎 Simplified version
// Wait for shutdown signal - if n.ctx != nil { - <-n.ctx.Done() - } else { - <-n.done - } + <-n.ctx.Done() return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/node/load.go(2 hunks)ledger/state.go(3 hunks)node.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
node.go (3)
49-50: LGTM: Context lifecycle fields added.The context and cancel function fields enable proper lifecycle management for the node. These are appropriately private to the package.
78-78: LGTM: Context initialization.The cancellable context is properly initialized early in the Run method, making it available for component startup.
272-274: LGTM: Context cancellation in shutdown.The context is properly cancelled at the start of the shutdown sequence, which will signal any components listening on
ctx.Done().internal/node/load.go (1)
99-99: LGTM: Ledger startup with context.Using
context.Background()is appropriate for this load operation. If future requirements need cancellation support during loading, consider accepting a context parameter in theLoadfunction signature.
wolf31o2
force-pushed
the
feat/node-context
branch
from
7767cf7 to
4ce442e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ledger/state.go (1)
339-340: Context stored but not observed in long-running goroutines.The context is stored in
ls.ctxbut theledgerProcessBlocks()goroutine (started at line 399) does not checkls.ctx.Done()for cancellation signals. This means the goroutine will continue running even after the context is cancelled, preventing graceful shutdown.This issue was previously flagged in past reviews. The goroutine needs to periodically select on
ls.ctx.Done()within its main loop to respect cancellation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/node/load.go(2 hunks)ledger/state.go(3 hunks)node.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: nilaway
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: lint
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
node.go (4)
78-78: Context initialization looks good.The node context and cancel function are properly initialized early in
Run(), ensuring downstream components can receive a valid context.
155-155: Correct context propagation to ledger.The ledger is now started with the node's context (
n.ctx), enabling cancellation to propagate through the ledger lifecycle.
235-235: Wait logic successfully migrated to context-based approach.The main wait loop now correctly uses
<-n.ctx.Done()instead of a plain channel, enabling coordinated shutdown via context cancellation.
255-257: Context cancellation properly triggered during shutdown.The
cancel()function is called with appropriate nil-checking, propagating the shutdown signal to all context-aware components.internal/node/load.go (1)
99-99: Load is a standalone CLI subcommand, not part of main application startup.The
Loadfunction (invoked viadingo load db-path) is a separate operational utility command, not an initialization step of the main application. The main application (dingo serve) properly implements signal-based graceful shutdown withsignal.NotifyContext()at lines 184-189 ofinternal/node/node.go. The Load command runs independently without active shutdown handlers, makingcontext.Background()intentional and appropriate for this use case—similar to a database migration utility that should complete its task without interruption.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
feat/node-context
branch
2 times, most recently
from
5008902 to
04613ae
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="ouroboros/chainsync.go">
<violation number="1" location="ouroboros/chainsync.go:182">
P1: Missing nil check for `conn` before using `conn.ErrorChan()`. If `GetConnectionById` returns nil, this will cause a nil pointer panic inside the goroutine. Add a nil check and return early if the connection is not found.</violation>
</file>
<file name="ouroboros/blockfetch.go">
<violation number="1" location="ouroboros/blockfetch.go:69">
P0: `break` inside the `default` case only exits the `select` statement, not the `for` loop. This will cause an infinite loop. Use a labeled break (e.g., `break Loop` with `Loop:` label on the for) or restructure the logic.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| return err | ||
| } | ||
| // Wait for next block and send | ||
| conn := o.ConnManager.GetConnectionById(ctx.ConnectionId) |
There was a problem hiding this comment.
P1: Missing nil check for conn before using conn.ErrorChan(). If GetConnectionById returns nil, this will cause a nil pointer panic inside the goroutine. Add a nil check and return early if the connection is not found.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ouroboros/chainsync.go, line 182:
<comment>Missing nil check for `conn` before using `conn.ErrorChan()`. If `GetConnectionById` returns nil, this will cause a nil pointer panic inside the goroutine. Add a nil check and return early if the connection is not found.</comment>
<file context>
@@ -176,24 +179,34 @@ func (o *Ouroboros) chainsyncServerRequestNext(
return err
}
// Wait for next block and send
+ conn := o.ConnManager.GetConnectionById(ctx.ConnectionId)
go func() {
- next, _ := clientState.ChainIter.Next(true)
</file context>
✅ Addressed in 19f6aec
ouroboros/blockfetch.go
Outdated
| default: | ||
| next, _ := chainIter.Next(false) | ||
| if next == nil { | ||
| break |
There was a problem hiding this comment.
P0: break inside the default case only exits the select statement, not the for loop. This will cause an infinite loop. Use a labeled break (e.g., break Loop with Loop: label on the for) or restructure the logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ouroboros/blockfetch.go, line 69:
<comment>`break` inside the `default` case only exits the `select` statement, not the `for` loop. This will cause an infinite loop. Use a labeled break (e.g., `break Loop` with `Loop:` label on the for) or restructure the logic.</comment>
<file context>
@@ -55,36 +55,39 @@ func (o *Ouroboros) blockfetchServerRequestRange(
+ default:
+ next, _ := chainIter.Next(false)
+ if next == nil {
+ break
+ }
+ if next.Block.Slot > end.Slot {
</file context>
✅ Addressed in 19f6aec
wolf31o2
force-pushed
the
feat/node-context
branch
from
04613ae to
19f6aec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
node.go (1)
306-313: Verify thatshutdownFuncsno longer causes double shutdown.The past review flagged that components like
ledgerState,connManager,peerGov, andutxorpcwere being registered inshutdownFuncswhile also being explicitly stopped in the shutdown phases above. The current code shows these registrations have been removed fromRun(), which resolves the double-shutdown issue.The
shutdownFuncsslice now appears to be reserved for external/plugin shutdown handlers that may be registered via other mechanisms.
🧹 Nitpick comments (2)
ouroboros/chainsync.go (1)
186-212: Errors fromRollBackwardandRollForwardare silently discarded.The inner goroutine ignores errors from
ctx.Server.RollBackward(line 196) andctx.Server.RollForward(line 201) using blank identifiers. While these operations occur asynchronously afterAwaitReply, silently discarding errors could mask connection or protocol issues. Consider logging errors at debug level for observability.🔎 Suggested improvement
if next.Rollback { - _ = ctx.Server.RollBackward( + if err := ctx.Server.RollBackward( next.Point, tip, - ) + ); err != nil { + o.config.Logger.Debug( + "failed to send RollBackward", + "error", err, + "connection_id", ctx.ConnectionId.String(), + ) + } } else { - _ = ctx.Server.RollForward( + if err := ctx.Server.RollForward( next.Block.Type, next.Block.Cbor, tip, - ) + ); err != nil { + o.config.Logger.Debug( + "failed to send RollForward", + "error", err, + "connection_id", ctx.ConnectionId.String(), + ) + } }ledger/state.go (1)
760-844: Consider propagating context toledgerReadChainfor complete cancellation support.The
ledgerReadChaingoroutine runs an infinite loop reading from the chain iterator but does not observels.ctx.Done(). While the main processing loop will exit on context cancellation,ledgerReadChainwill continue running until it blocks onresultCh <- result(which will never be received). This could delay shutdown or leave the goroutine hanging.🔎 Suggested improvement
Pass the context to
ledgerReadChainand check for cancellation:-func (ls *LedgerState) ledgerReadChain(resultCh chan readChainResult) { +func (ls *LedgerState) ledgerReadChain(ctx context.Context, resultCh chan readChainResult) { // ...existing code... for { + select { + case <-ctx.Done(): + return + default: + } // ...rest of loop... - resultCh <- result + select { + case resultCh <- result: + case <-ctx.Done(): + return + } } }And update the call site:
- go ls.ledgerReadChain(readChainResultCh) + go ls.ledgerReadChain(ls.ctx, readChainResultCh)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/node/load.goledger/state.gonode.goouroboros/blockfetch.goouroboros/chainsync.goouroboros/txsubmission.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/node/load.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ouroboros/chainsync.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ouroboros/chainsync.goouroboros/blockfetch.go
🧬 Code graph analysis (2)
ledger/state.go (1)
database/plugin/log.go (1)
Logger(18-27)
ouroboros/chainsync.go (1)
ledger/state.go (1)
LedgerState(284-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: go-test (1.25.x, ubuntu-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
🔇 Additional comments (11)
ouroboros/chainsync.go (2)
57-59: Good defensive check for ChainSync availability.This guard ensures the ChainSync protocol is available before proceeding, preventing potential nil pointer issues downstream.
182-185: Nil check for connection is now properly implemented.This addresses the previously flagged issue where
conncould be nil before accessingconn.ErrorChan().ouroboros/blockfetch.go (2)
58-61: Good nil check for connection lookup.The early return when
connis nil prevents potential nil pointer panics when accessingconn.ErrorChan().
65-95: Loop label correctly fixes the break-from-select issue.The
Loop:label withbreak Loopstatements now correctly exits theforloop rather than just theselectstatement. This addresses the previously flagged infinite loop issue.However,
BatchDone()is no longer called after serving the block range. Looking atblockfetchClientBatchDone(lines 218-236), the protocol expects aBatchDonesignal to mark the end of a batch. Without callingctx.Server.BatchDone(), the client may hang waiting for the batch completion signal.Please verify that removing the
BatchDone()call is intentional. If the protocol requires signaling batch completion:} + if err := ctx.Server.BatchDone(); err != nil { + // Log or handle error + return + } }() return nilledger/state.go (2)
339-340: Context is now properly stored and used.The
Startmethod correctly accepts a context and stores it inls.ctx, enabling context-aware lifecycle management.
893-914: Context cancellation is now correctly observed in the block processing loop.The
ledgerProcessBlocksgoroutine now listens forls.ctx.Done()alongside the chain result channel, enabling graceful termination when the context is cancelled. This addresses the previous review comment about the stored context never being used.node.go (5)
49-50: Context-based lifecycle fields are properly declared.The addition of
ctxandcancelfields enables proper context propagation and cancellation throughout the node's lifecycle.
76-76: Context initialization at the start of Run() is appropriate.Creating the context at the beginning of
Run()ensures all subsequent component initializations can receive the same lifecycle context.
153-155: Context is correctly propagated to ledger state.Passing
n.ctxtoledgerState.Start()enables the ledger to observe shutdown signals and terminate gracefully.
233-234: Clean wait-for-shutdown implementation.The simple
<-n.ctx.Done()pattern is cleaner than the previous channel-based approach and correctly blocks until shutdown is triggered.
253-255: Context cancellation in shutdown is correctly guarded.The nil check before calling
n.cancel()handles the edge case where shutdown could be called beforeRun()initializes the context.
wolf31o2
force-pushed
the
feat/node-context
branch
from
19f6aec to
44f976d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
utxorpc/utxorpc.go (1)
75-178: Context parameter accepted but unused.The
Startmethod now accepts acontext.Contextparameter but doesn't use it. While not critical, consider either:
- Using the context to control the lifecycle of the server goroutine (e.g., monitoring
ctx.Done()to initiate shutdown)- Documenting why the context is accepted but not used (e.g., for API consistency with other components)
This maintains consistency with the context-aware lifecycle pattern introduced across the codebase.
peergov/peergov.go (1)
231-263: Context parameter accepted but unused.The
Startmethod now accepts acontext.Contextparameter but doesn't use it. The reconcile goroutine at line 250 uses the internalstopChfor cancellation. Consider either:
- Using the context to control the reconcile goroutine lifecycle (observe
ctx.Done()alongside or instead ofstopCh)- Documenting why the context is accepted but not used (e.g., for API consistency)
This maintains consistency with the context-aware lifecycle pattern introduced across the codebase.
ouroboros/chainsync.go (1)
193-196: Consider logging discarded error from iterator.The error from
ChainIter.Next(true)is discarded. While the nil check onnexthandles some failure cases, actual errors are silently lost. For observability, consider logging the error similarly to how errors are logged on lines 203-207 and 215.🔎 Proposed fix
go func() { defer close(done) - next, _ := clientState.ChainIter.Next(true) + next, err := clientState.ChainIter.Next(true) + if err != nil { + o.config.Logger.Debug("chain iterator error", "error", err) + } if next == nil { return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
connmanager/connection_manager.goconnmanager/connection_manager_test.goconnmanager/listener.godatabase/plugin/metadata/sqlite/transaction.gointernal/node/load.gointernal/node/node.goledger/state.gonode.goouroboros/blockfetch.goouroboros/chainsync.goouroboros/txsubmission.gopeergov/peergov.goutxorpc/utxorpc.goutxorpc/utxorpc_test.go
✅ Files skipped from review due to trivial changes (1)
- database/plugin/metadata/sqlite/transaction.go
🚧 Files skipped from review as they are similar to previous changes (3)
- ouroboros/txsubmission.go
- internal/node/load.go
- ouroboros/blockfetch.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ouroboros/chainsync.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ouroboros/chainsync.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: In utxorpc code paths, always operate on github.com/blinklabs-io/gouroboros/ledger/common.Block. Use database/models.Block only as the storage type and call models.Block.Decode() to obtain a ledger/common.Block before further processing.
Applied to files:
utxorpc/utxorpc_test.go
🧬 Code graph analysis (3)
connmanager/listener.go (2)
connmanager/connection_manager.go (1)
ConnectionManager(51-59)config.go (1)
ListenerConfig(33-33)
node.go (1)
internal/node/node.go (1)
Run(37-267)
ouroboros/chainsync.go (2)
ledger/state.go (1)
LedgerState(284-311)database/plugin/log.go (1)
Logger(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: nilaway
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, ubuntu-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
connmanager/connection_manager_test.go (1)
230-232: LGTM!Test correctly updated to use the context-aware
StartAPI withcontext.Background().internal/node/node.go (1)
195-195: LGTM!Correctly passes the signal-aware context to
d.Run(), enabling graceful shutdown when OS signals (SIGINT/SIGTERM) are received.utxorpc/utxorpc_test.go (1)
36-38: LGTM!Test correctly updated to use the context-aware
StartAPI withcontext.Background().connmanager/connection_manager.go (1)
213-218: LGTM!
Startmethod correctly accepts and propagates the context tostartListeners, enabling context-aware listener initialization.node.go (5)
49-50: LGTM!Added
ctxandcancelfields to support node-scoped context lifecycle management, replacing the previous channel-based approach.
69-76: LGTM!
Runnow accepts a parent context and creates a cancellable child context for the node's lifecycle. This enables proper cancellation propagation throughout the node and its components.
153-228: LGTM!Node context is correctly propagated to all component
Start()methods (ledgerState, connManager, peerGov, utxorpc), enabling coordinated lifecycle management and graceful shutdown.
233-234: LGTM!Correctly blocks on the node context's
Done()channel, allowing the node to run until shutdown is signaled via context cancellation.
253-255: LGTM!Properly cancels the node context during shutdown, signaling all components observing
n.ctxto terminate gracefully.connmanager/listener.go (2)
37-44: LGTM!
startListenerscorrectly accepts and propagates the context to eachstartListenercall, enabling context-aware listener initialization.
46-77: LGTM!
startListenercorrectly accepts the context parameter and uses it in thelistenConfig.Listen()call (line 69), allowing the listener creation to respect cancellation and timeouts.ledger/state.go (2)
339-340: LGTM! Context properly accepted and stored.The
Startsignature now accepts a context, and it's correctly stored inls.ctxfor propagation to the processing goroutines. This enables clean shutdown signaling throughout the ledger lifecycle.
760-763: LGTM! Context cancellation properly monitored.The context is correctly propagated to
ledgerReadChain(line 861) and monitored at strategic points:
- Line 779-783: Non-blocking check before reading batches allows early exit
- Line 850-854: Blocking check when sending results prevents goroutine leaks if the receiver has stopped
- Line 905-926: Select monitors both the result channel and context cancellation, enabling clean shutdown
The implementation allows in-flight database transactions to complete before checking cancellation, which provides graceful shutdown without corrupting state. This directly addresses the previous review concern about goroutines not observing context cancellation.
Also applies to: 779-783, 850-854, 861-861, 905-926
ouroboros/chainsync.go (1)
57-62: LGTM!Good defensive check to validate ChainSync protocol availability before attempting to use it. This prevents potential nil pointer dereferences on subsequent calls to
conn.ChainSync().Client.
wolf31o2
force-pushed
the
feat/node-context
branch
from
44f976d to
5c1e009
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utxorpc/utxorpc.go (1)
219-256: TLS detection is duplicated betweenStart()andstartServer().TLS configuration is checked at lines 136 and again at lines 223/249. Consider passing a boolean or the server type to
startServer()to avoid re-checking the config fields.🔎 Proposed simplification
-func (u *Utxorpc) startServer(server *http.Server) error { +func (u *Utxorpc) startServer(server *http.Server, isTLS bool) error { startErr := make(chan error, 1) go func() { var err error - if u.config.TlsCertFilePath != "" && u.config.TlsKeyFilePath != "" { + if isTLS { err = server.ListenAndServeTLS( u.config.TlsCertFilePath, u.config.TlsKeyFilePath, ) } else { err = server.ListenAndServe() } // ... }() select { case err := <-startErr: - serverType := "non-TLS" - if u.config.TlsCertFilePath != "" && u.config.TlsKeyFilePath != "" { - serverType = "TLS" - } + serverType := "non-TLS" + if isTLS { + serverType = "TLS" + } return fmt.Errorf("failed to start gRPC %s server: %w", serverType, err) // ... } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
connmanager/connection_manager.goconnmanager/connection_manager_test.goconnmanager/listener.godatabase/plugin/metadata/sqlite/transaction.gointernal/node/load.gointernal/node/node.goledger/state.gonode.goouroboros/blockfetch.goouroboros/chainsync.goouroboros/txsubmission.gopeergov/peergov.goutxorpc/utxorpc.goutxorpc/utxorpc_test.go
✅ Files skipped from review due to trivial changes (1)
- database/plugin/metadata/sqlite/transaction.go
🚧 Files skipped from review as they are similar to previous changes (3)
- peergov/peergov.go
- internal/node/load.go
- internal/node/node.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ouroboros/chainsync.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: In utxorpc code paths, always operate on github.com/blinklabs-io/gouroboros/ledger/common.Block. Use database/models.Block only as the storage type and call models.Block.Decode() to obtain a ledger/common.Block before further processing.
Applied to files:
utxorpc/utxorpc_test.goouroboros/blockfetch.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ouroboros/blockfetch.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.go
🧬 Code graph analysis (4)
connmanager/listener.go (2)
connmanager/connection_manager.go (1)
ConnectionManager(51-59)config.go (1)
ListenerConfig(33-33)
ouroboros/chainsync.go (2)
chain/iter.go (1)
ChainIteratorResult(31-35)ledger/state.go (1)
LedgerState(284-311)
ledger/state.go (1)
database/plugin/log.go (1)
Logger(18-27)
utxorpc/utxorpc.go (1)
database/plugin/log.go (1)
Logger(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (19)
connmanager/connection_manager.go (1)
213-218: LGTM!The context-aware Start signature aligns well with the PR's lifecycle management goals. Context is properly threaded to startListeners for coordinated cancellation during startup.
ouroboros/txsubmission.go (2)
60-63: LGTM!Multi-line formatting improves readability without changing behavior.
74-93: LGTM!The nil check for the connection (lines 75-77) and the non-blocking RequestTxIds pattern with cancellation properly address the previous review concerns. The select statement ensures the goroutine terminates when the connection errors out.
connmanager/listener.go (1)
37-43: LGTM!Context propagation through the listener startup path is correct. The provided context is properly threaded from
startListeners→startListener→listenConfig.Listen, enabling cancellation/timeout semantics during listener creation.Also applies to: 46-49, 69-69
connmanager/connection_manager_test.go (1)
230-230: LGTM!Test correctly updated to pass context to the new Start signature.
ouroboros/chainsync.go (1)
58-63: LGTM!Defensive check ensures ChainSync protocol is available before attempting to use it, preventing nil pointer panics.
ledger/state.go (2)
339-340: LGTM!Start method now properly accepts and stores the context for lifecycle management.
760-763: LGTM!Context cancellation is properly integrated throughout the ledger state lifecycle:
ledgerReadChainaccepts and monitors the context (lines 779-783)- Result delivery respects cancellation (lines 850-854)
- Block processing loop checks for cancellation (lines 905-926)
This addresses the previous review concern about the stored context not being used. The implementation now enables clean shutdown when the node context is cancelled.
Also applies to: 779-783, 850-854, 861-861, 905-926
utxorpc/utxorpc_test.go (1)
36-36: LGTM!Test correctly updated to pass context to the new Start signature.
ouroboros/blockfetch.go (1)
58-95: Verify if BatchDone() call is required after StartBatch() in server goroutine.The server handler (line 46-98) calls
ctx.Server.StartBatch()but has no correspondingBatchDone()call before returning, unlike the client side (line 218-236) which explicitly registersWithBatchDoneFuncand handles batch completion. The server goroutine returns on error (lines 60, 63, 69, 85) and after serving blocks (lines 92, 73) without batch cleanup. Confirm whether the gouroboros blockfetch protocol requires explicitctx.Server.BatchDone()invocation or if the library auto-completes batches when the goroutine exits. Consider adding defer-based cleanup or explicit batch completion before all return paths if required by the protocol.utxorpc/utxorpc.go (3)
74-173: LGTM - Context-aware Start with proper server initialization.The context parameter addition enables clean lifecycle management. Server configuration is consistent between TLS and non-TLS paths with appropriate timeouts.
183-199: Context-aware shutdown is idempotent and safe.The goroutine correctly uses a fresh
context.Background()for shutdown since the parent context is already cancelled. Note thatStop()may be called twice (once from this goroutine and once fromNode.shutdown()), but this is safe becauseStop()is idempotent via the mutex and nil-check onu.server.
204-216: LGTM - Idempotent Stop implementation.The mutex protection and nil-check ensure safe handling of concurrent or repeated calls.
node.go (6)
49-51: LGTM - Context fields enable proper lifecycle management.The addition of
ctxandcancelfields, along with the removal of the unuseddonechannel, aligns with the per-node lifecycle management objective.
69-76: Correct context propagation pattern.Creating a child context with
context.WithCancel(ctx)allows the node to be cancelled either by the parent context or by callingStop(), which is the expected behavior for component lifecycle management.
152-230: Component lifecycle correctly tied to node context.All components now receive
n.ctxfor coordinated shutdown. The past review comments about double shutdown (fromshutdownFuncsregistrations) have been addressed by removing those registrations while keeping the explicit calls inshutdown().
232-234: Clean context-based wait replaces channel mechanism.Waiting on
n.ctx.Done()is idiomatic and integrates well with the context-based lifecycle.
253-255: Good defensive nil check on cancel function.This prevents a panic if
Stop()is called beforeRun()has initialized the context.
304-313:shutdownFuncsmechanism is actively used.The
setupTracing()method, called duringRun(), registers a shutdown function vian.shutdownFuncs = append(n.shutdownFuncs, tracerProvider.Shutdown)when tracing is enabled. The loop at lines 308-312 correctly executes these registered cleanup functions, and the nil assignment at line 313 performs standard cleanup. No changes needed.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
feat/node-context
branch
from
5c1e009 to
9aef0f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ouroboros/chainsync.go (1)
186-263: LGTM! Comprehensive cancellation handling with nested goroutines.The implementation correctly handles connection-driven cancellation:
- Connection is validated before use
- Cancellable context is created and monitored for connection errors
- The nested goroutine structure allows Next(true) to be interrupted via context cancellation
- Error logging appropriately filters out expected context.Canceled errors
- The outer select ensures cleanup on both completion and connection failure
This addresses the goroutine leak concerns from previous reviews.
Optional: Consider simplifying the nested goroutine structure
The current three-layer goroutine structure (outer, monitor, and worker) is correct but complex. If this becomes hard to maintain, consider extracting the cancellation logic into a helper function or using a more linear control flow with select statements. However, the current implementation works correctly, so this is just a maintainability suggestion for future refactoring.
node.go (1)
72-72: Verify whether nolint:contextcheck annotations can be removed.Multiple
nolint:contextcheckannotations are present. While they may be necessary if the linter doesn't recognize the context flow pattern (creating a child context and using it), consider whether the code structure could be adjusted to avoid these suppressions.If the linter warnings are legitimate, the suppressions are fine. If not, consider restructuring to satisfy the linter without suppressions.
Also applies to: 153-153, 193-193, 215-215, 228-228
utxorpc/utxorpc.go (1)
135-173: Race window exists between server assignment and startup, but with benign consequences.The server is assigned to
u.server(line 172) and the mutex is unlocked (line 173) beforestartServer()completes (line 176). This creates a race window where another goroutine could callStop()on a server that hasn't started listening yet.However, calling Shutdown() before ListenAndServe() returns nil, so this is handled gracefully. Still, the typical pattern would be to either keep the lock until startup is confirmed or assign
u.serveronly after successful startup.Additionally, the TLS detection logic (
u.config.TlsCertFilePath != "" && u.config.TlsKeyFilePath != "") is duplicated three times: at lines 136, 223, and 249. Consider extracting this into a helper method to reduce duplication and improve maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
chain/chain.gochain/iter.gochainsync/chainsync.goconnmanager/connection_manager.goconnmanager/connection_manager_test.goconnmanager/listener.godatabase/plugin/metadata/sqlite/transaction.gointernal/node/load.gointernal/node/node.goledger/state.gonode.goouroboros/blockfetch.goouroboros/chainsync.goouroboros/txsubmission.gopeergov/peergov.goutxorpc/utxorpc.goutxorpc/utxorpc_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/node/load.go
- peergov/peergov.go
- connmanager/listener.go
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ouroboros/blockfetch.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
chain/iter.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: chain.Chain.AddBlock expects a github.com/blinklabs-io/gouroboros/ledger.Block. In tests, construct a database/models.Block only as a container and call its Decode() to obtain the gouroboros ledger.Block (which also satisfies github.com/blinklabs-io/gouroboros/ledger/common.Block) before inserting into the chain.
Applied to files:
chain/iter.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
chain/iter.goouroboros/chainsync.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: In utxorpc code paths, always operate on github.com/blinklabs-io/gouroboros/ledger/common.Block. Use database/models.Block only as the storage type and call models.Block.Decode() to obtain a ledger/common.Block before further processing.
Applied to files:
utxorpc/utxorpc_test.go
🧬 Code graph analysis (3)
node.go (1)
internal/node/node.go (1)
Run(37-267)
ledger/state.go (1)
database/plugin/log.go (1)
Logger(18-27)
ouroboros/chainsync.go (1)
chain/iter.go (1)
ChainIteratorResult(35-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
database/plugin/metadata/sqlite/transaction.go (1)
687-698: LGTM - Logging style alignment.The multi-line formatting improves readability and aligns with broader logging conventions in the repository. No functional changes.
ouroboros/txsubmission.go (2)
59-64: LGTM - Improved error handling.The nil check on
txprevents a potential panic ifTxSubmission()returns nil, and the error message is clear.
73-94: LGTM - Connection-aware cancellation pattern.The implementation correctly:
- Guards against nil connection before accessing
ErrorChan()- Uses a done channel with proper
defer close(done)to signal completion- Enables early termination when the connection errors out
This addresses the previously flagged nil pointer concern.
ouroboros/blockfetch.go (1)
58-61: LGTM - Nil guard added.The nil check on line 59 prevents a potential nil pointer dereference when accessing
conn.ErrorChan().utxorpc/utxorpc_test.go (1)
36-45: LGTM - Context-aware test lifecycle.The test correctly:
- Passes
context.Background()toStart()matching the new signature- Uses a timeout context for
Stop()to prevent indefinite hangs- Properly defers
cancel()for cleanupchain/chain.go (1)
480-491: LGTM - Context-aware blocking iteration.The implementation correctly:
- Captures
waitChanbefore releasing the mutex to avoid races- Uses
selectto allow cancellation viaiter.ctx.Done()- Returns the context error on cancellation, enabling clean iterator shutdown
- Recursively retries when new data arrives via
waitChanThis integrates well with the
ChainIterator.Cancel()method added inchain/iter.go.chainsync/chainsync.go (1)
79-82: LGTM! Clean cancellation on client removal.The implementation properly cancels any pending iterator operations before removing the client state, preventing resource leaks.
connmanager/connection_manager.go (1)
213-218: LGTM! Context propagation is correct.The Start method now accepts a context and properly propagates it through the listener initialization flow, enabling context-aware lifecycle management.
internal/node/node.go (1)
193-200: LGTM! Proper context propagation to node lifecycle.The signal context is correctly passed to the node's Run method, enabling graceful shutdown on signals.
connmanager/connection_manager_test.go (1)
230-232: LGTM! Test properly updated for new API.The test now correctly passes a context to the Start method.
chain/iter.go (1)
17-77: LGTM! Well-structured cancellation support for iterators.The implementation properly adds context-based cancellation to ChainIterator:
- Context and cancel function are initialized and stored
- Cancel() method allows external callers to terminate iterations
- This addresses the goroutine leak concerns from previous reviews by enabling cleanup when clients are removed
ouroboros/chainsync.go (1)
58-63: LGTM! Good defensive check.Validating ChainSync protocol availability before attempting to start the client prevents runtime errors and provides clear error messages.
node.go (3)
49-50: LGTM! Context-based lifecycle properly integrated.The Node now uses context for lifecycle management:
- Stores its own cancellable context derived from the provided context
- Properly initializes ctx and cancel in Run
- This enables clean propagation of cancellation signals to all components
Also applies to: 69-76
233-234: LGTM! Clean context-based wait.Waiting on
n.ctx.Done()properly integrates with the context-based lifecycle, replacing the previous channel-based mechanism.
253-255: LGTM! Safe context cancellation in shutdown.The nil check before calling cancel ensures safety even if shutdown is called before Run completes initialization.
ledger/state.go (3)
304-304: LGTM! Context properly stored and will be used.The Start method now accepts and stores the context, enabling cancellation support throughout the ledger state lifecycle. This addresses the previous review concern about unused context.
Also applies to: 339-340
760-856: LGTM! Context-aware chain reading with proper cancellation.The ledgerReadChain method now respects context cancellation:
- Checks for cancellation in the main loop (lines 779-783)
- Checks for cancellation when sending results (lines 850-854)
This ensures the chain reader goroutine can be cleanly terminated during shutdown.
858-926: LGTM! Block processing respects context cancellation.The ledgerProcessBlocks goroutine now properly handles context cancellation:
- Passes context to ledgerReadChain (line 861)
- Checks for cancellation when receiving results (lines 905-926)
This completes the cancellation path and addresses the goroutine leak concern from previous reviews.
utxorpc/utxorpc.go (3)
74-134: LGTM! Context-aware signature and handler setup.The updated signature accepting
context.Contextaligns with the PR's node-scoped lifecycle goals. The server existence check and handler registration logic are correct.
184-199: Well-implemented context cancellation handling.The goroutine correctly monitors
ctx.Done()and initiates graceful shutdown with an appropriate 30-second timeout. Thenolint:contextcheckcomment is well-justified—usingcontext.Background()for the shutdown context is correct since the parent context is already cancelled.The goroutine lifetime assumes the provided context will eventually be cancelled. Based on the PR description indicating node-scoped lifecycle management, this assumption is valid.
218-257: Simplified signature; timeout limitation acknowledged.The removal of the
serverTypeparameter simplifies the API. The 100ms startup timeout (line 253) is a reasonable tradeoff for detecting immediate binding errors, and the comment at lines 244-245 appropriately documents that delayed failures (e.g., certificate loading) may not be detected.The TLS detection logic at lines 223 and 249 duplicates the logic from
Start(line 136), which was noted in an earlier comment.
Signed-off-by: Chris Gianelloni <[email protected]>
wolf31o2
force-pushed
the
feat/node-context
branch
from
9aef0f2 to
a2932a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
chain/chain_test.go (1)
150-152: LGTM! Defensive nil checks prevent panics under cancellation.The nil checks are well-placed after error handling and provide clear failure messages. They align with the PR's context-aware cancellation features and prevent potential panics when dereferencing
next.Consider adding a dedicated test case that exercises context cancellation to validate these nil checks are triggered in cancellation scenarios:
func TestChainIteratorCancellation(t *testing.T) { cm, err := chain.NewManager(nil, nil) if err != nil { t.Fatalf("unexpected error creating chain manager: %s", err) } c := cm.PrimaryChain() for _, testBlock := range testBlocks { if err := c.AddBlock(testBlock, nil); err != nil { t.Fatalf("unexpected error adding block to chain: %s", err) } } iter, err := c.FromPoint(ocommon.NewPointOrigin(), false) if err != nil { t.Fatalf("unexpected error creating chain iterator: %s", err) } // Cancel the iterator iter.Cancel() // Verify Next() handles cancellation appropriately next, err := iter.Next(false) if err == nil && next == nil { // Expected: nil without error after cancellation return } if err != nil { // Also acceptable: explicit cancellation error return } t.Fatal("expected nil or error after iterator cancellation") }Also applies to: 250-252, 298-300, 589-591
internal/node/load.go (1)
99-99: Consider accepting a context parameter for cancellation support.The
Loadfunction performs potentially long-running operations including block copying loops and a sleep-based wait loop (lines 179-185). Usingcontext.Background()satisfies the new API requirement but prevents callers from cancelling or timing out the load operation.Consider updating the function signature to accept a context parameter:
func Load(ctx context.Context, cfg *config.Config, logger *slog.Logger, immutableDir string) errorThen pass it through:
if err := ls.Start(ctx); err != nil {This would enable graceful cancellation of long-running load operations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
chain/chain.gochain/chain_test.gochain/iter.gochainsync/chainsync.goconnmanager/connection_manager.goconnmanager/connection_manager_test.goconnmanager/listener.godatabase/plugin/metadata/sqlite/transaction.gointernal/node/load.gointernal/node/node.goledger/state.gonode.goouroboros/blockfetch.goouroboros/chainsync.goouroboros/txsubmission.gopeergov/peergov.goutxorpc/utxorpc.goutxorpc/utxorpc_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- chain/chain.go
- chainsync/chainsync.go
- chain/iter.go
- peergov/peergov.go
- ouroboros/txsubmission.go
- utxorpc/utxorpc_test.go
- internal/node/node.go
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
chain/chain_test.goouroboros/chainsync.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: chain.Chain.AddBlock expects a github.com/blinklabs-io/gouroboros/ledger.Block. In tests, construct a database/models.Block only as a container and call its Decode() to obtain the gouroboros ledger.Block (which also satisfies github.com/blinklabs-io/gouroboros/ledger/common.Block) before inserting into the chain.
Applied to files:
chain/chain_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
chain/chain_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
chain/chain_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
chain/chain_test.goledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
chain/chain_test.goledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
chain/chain_test.goledger/state.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ouroboros/blockfetch.go
🧬 Code graph analysis (5)
ouroboros/blockfetch.go (1)
database/plugin/log.go (1)
Logger(18-27)
ouroboros/chainsync.go (2)
database/plugin/log.go (1)
Logger(18-27)ledger/state.go (1)
LedgerState(284-311)
utxorpc/utxorpc.go (1)
database/plugin/log.go (1)
Logger(18-27)
node.go (1)
internal/node/node.go (1)
Run(37-267)
connmanager/listener.go (2)
connmanager/connection_manager.go (1)
ConnectionManager(51-59)config.go (1)
ListenerConfig(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (go)
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: docker (ubuntu-latest, amd64)
🔇 Additional comments (21)
database/plugin/metadata/sqlite/transaction.go (1)
687-698: LGTM: Formatting improvement for structured logging.The multi-line structured log call improves readability. The error message and control flow remain functionally unchanged.
connmanager/connection_manager.go (1)
213-218: LGTM: Context propagation integrated correctly.The Start method now accepts a context and properly propagates it to startListeners, enabling cancellation support throughout the connection manager startup sequence. This aligns with the broader PR pattern of context-aware lifecycle management.
ouroboros/blockfetch.go (1)
58-103: LGTM: Connection-aware blockfetch with proper cancellation.The refactored implementation addresses previous review concerns:
- Connection validation prevents nil pointer dereference (line 58-61)
- Select-driven loop monitors connection errors and enables graceful exit (line 67-70)
- Labeled break statements correctly exit the for loop (lines 73, 76, 92)
- Batch completion is properly signaled with error logging (lines 97-103)
The asynchronous pattern with connection error monitoring aligns well with the PR's context-aware lifecycle management.
ouroboros/chainsync.go (2)
58-63: LGTM: Runtime validation prevents nil pointer dereference.The ChainSync protocol availability check adds defensive validation before attempting to use the protocol, providing a clear error message if the protocol is unavailable on the connection.
186-234: LGTM: Connection-aware chainsync with iterator cancellation.The refactored asynchronous pattern addresses previous review concerns:
- Connection validation prevents nil pointer dereference (lines 186-189)
- Inner goroutine monitors connection errors and cancels the iterator (lines 192-195), preventing the goroutine leak identified in past reviews
- Context.Canceled errors are filtered from logs as they're expected during clean shutdown (lines 201-207)
- Error logging provides visibility into roll forward/backward failures (lines 219-232)
The two-goroutine pattern (outer for processing, inner for monitoring) enables proper cancellation when connections fail.
connmanager/listener.go (1)
37-49: LGTM: Context propagation through listener startup.The context parameter is properly threaded through startListeners and startListener, and used in the listenConfig.Listen call (line 69). This enables cancellation and timeout support during listener creation, aligning with the PR's context-aware startup pattern.
Also applies to: 69-69
connmanager/connection_manager_test.go (1)
230-230: LGTM: Test updated for new Start signature.The test correctly calls Start with context.Background(), consistent with the new context-aware API.
ledger/state.go (3)
304-304: LGTM: Context field added and properly initialized.The Start method now accepts and stores the context in ls.ctx (lines 339-340), addressing the previous review concern about unused context. The stored context is subsequently used for cancellation throughout the ledger processing lifecycle.
Also applies to: 339-340
760-856: LGTM: Context-aware chain reading with cancellation support.The ledgerReadChain method properly implements cancellation:
- Accepts context parameter and checks ctx.Done() in the outer loop (lines 779-783)
- Checks ctx.Done() before sending results (lines 850-854)
- Returns immediately on context cancellation, enabling clean shutdown
This addresses the previous review concern about missing context cancellation checks in the ledgerReadChain goroutine.
861-861: LGTM: Context propagation in block processing.The ledgerProcessBlocks method correctly:
- Passes ls.ctx to ledgerReadChain (line 861)
- Monitors ctx.Done() alongside readChainResultCh in the select statement (lines 905-926)
- Returns immediately on context cancellation (line 924-925)
This completes the context-aware cancellation support throughout the ledger processing pipeline.
Also applies to: 905-926
utxorpc/utxorpc.go (2)
74-210: LGTM: Context-aware startup with graceful shutdown.The refactored Start method properly integrates context-aware lifecycle management:
- Server construction is unified and assigned to u.server after configuration (lines 135-173)
- startServer is called with proper error handling (lines 176-181)
- A goroutine monitors ctx.Done() and performs graceful shutdown with a 30s timeout (lines 184-207)
- Server state is properly cleared on startup failure (lines 177-180)
The shutdown pattern using a separate context (line 192-194) is correct, ensuring graceful shutdown can complete even if the parent context is cancelled.
227-264: LGTM: Simplified startServer signature.The startServer method has been simplified by removing the serverType parameter and determining TLS vs non-TLS internally based on configuration (lines 231, 256-259). This reduces coupling and makes the error messages more accurate.
internal/node/load.go (1)
18-18: LGTM - necessary import for context-aware Start call.node.go (8)
49-50: LGTM - clean context-based lifecycle management.The addition of
ctxandcancelfields enables proper context propagation and cancellation throughout the node's lifecycle, replacing the previous channel-based approach.
69-69: LGTM - enables caller-controlled cancellation.The updated signature follows Go conventions for long-running operations. This is a breaking API change but aligns with the PR's objective of per-node lifecycle management.
76-76: LGTM - correct context derivation pattern.Creating a derived context with
WithCancelallows the node to respond to both parent context cancellation and its own shutdown signal. This is the idiomatic approach for lifecycle management.
153-153: Context propagation is correct despite linter warnings.The
//nolint:contextcheckdirectives suppress warnings about passingn.ctxinstead of the receivedctxparameter. This is actually the correct pattern:
n.ctxis derived fromctxviacontext.WithCancel(ctx)on line 76- Child contexts inherit cancellation from parents, so when
ctxis cancelled,n.ctxis also cancelled- The node can additionally cancel
n.ctxindependently during shutdownThe linter cannot detect this derivation relationship, hence the necessary suppressions. The approach enables proper multi-level lifecycle management.
Also applies to: 193-193, 215-215, 228-228
233-233: LGTM - clean context-based shutdown wait.Blocking on
n.ctx.Done()provides a clean wait mechanism that responds to both parent context cancellation and the node's own shutdown signal vian.cancel().
253-255: LGTM - proper cancellation signal during shutdown.Calling
n.cancel()early in the shutdown sequence signals all context-aware operations to stop gracefully. The nil check provides defensive safety, thoughcancelshould always be initialized by the timeshutdown()is called.
245-321: Shutdown sequence looks correct - previous double-shutdown issue resolved.The shutdown implementation follows a logical phased approach:
- Cancel context signal (line 253-255)
- Stop accepting new work (peerGov, utxorpc)
- Drain connections (mempool, connManager)
- Flush state (ledgerState)
- Cleanup resources (shutdownFuncs, eventBus)
The components are each stopped exactly once through explicit calls. The previous concern about double shutdown (components being registered in
shutdownFuncsand called explicitly) appears to have been addressed, as no such registrations are visible in the current code.
72-72: The//nolint:contextchecksuppression is justified.
setupTracing()is an initialization function that intentionally usescontext.TODO()to set up the OpenTelemetry tracer provider. It performs synchronous setup and doesn't need the caller's cancellation context—it's properly suppressed.
Summary by cubic
Introduce node-scoped context and shutdown hooks to manage service lifecycles. Core services now start with the node context and stop cleanly on cancel.
New Features
Bug Fixes
Written for commit a2932a2. Summary will update automatically on new commits.
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.