NethermindEth / NethermindEth/juno
node: service closing without error shouldn't shut down the whole node
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 444
- Forks
- 244
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 78
Description
Follow-up from https://github.com/NethermindEth/juno/pull/3601#discussion_r3304634462.
Problem
node.Node.StartService unconditionally defer cancel()s the node-level context as soon as a service's Run returns:
https://github.com/NethermindEth/juno/blob/main/node/node.go#L729-L744
func (n *Node) StartService(
wg *conc.WaitGroup, ctx context.Context, cancel context.CancelFunc, s service.Service,
) {
wg.Go(func() {
// Immediately acknowledge panicing services by shutting down the node
// Without the deffered cancel(), we would have to wait for user to hit Ctrl+C
defer cancel()
if err := s.Run(ctx); err != nil {
n.logger.Error(
"Service error",
zap.String("name", reflect.TypeOf(s).String()),
zap.Error(err),
)
}
})
}
The stated intent (per the inline comment) is to react to a panicking/failed service by tearing the whole node down. But the current implementation fires cancel() on any return, including a clean nil. That forces every service.Service implementation to keep Run blocking until ctx.Done() — even when it has nothing left to do — purely to avoid bringing the node down.
MigrationFeeder.Run (added in #3601) is an example of a service that has to artificially block on <-ctx.Done() for this reason:
https://github.com/NethermindEth/juno/blob/main/starknetdata/feeder/migration_feeder.go
Proposal
Only trigger the node-wide cancel when Run returns an error (or panics). A service that completes its work and returns nil should be allowed to exit without taking the rest of the node with it.
Roughly:
defer func() {
if r := recover(); r != nil {
// log + cancel
cancel()
panic(r) // or handle
}
}()
if err := s.Run(ctx); err != nil {
n.logger.Error("Service error", ...)
cancel()
}
Care needs to be taken around services that are expected to run for the node's lifetime — for those, an early clean return is itself surprising and may still warrant logging, but not necessarily a full shutdown.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in node/node.go at Node.StartService, then read starknetdata/feeder/migration_feeder.go to understand the clean-return case. Trace how service errors and panics are handled, and verify that an error or panic cancels the node while a service returning nil does not; check the affected node and service behavior with the relevant tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 56/100