NethermindEth / NethermindEth/juno

node: service closing without error shouldn't shut down the whole node

Open
#3,685 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

no-stale
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.