Skip to content

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

Description

@rodrodros

Follow-up from #3601 (comment).

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions