Skip to content

closing order change supernova #7776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sstanculeanu merged 1 commit into from
Mar 2, 2026
Merged

closing order change supernova #7776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion node/nodeRunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,8 @@ func (nr *nodeRunner) executeOneComponentCreationCycle(
goRoutinesNumberStart,
managedCoreComponents.ClosingNodeStarted(),
closeComponentsDelay,
managedConsensusComponents,
managedProcessComponents.ExecutionManager(),
)

return nextOperation == nextOperationShouldStop, nil
Expand Down Expand Up @@ -998,6 +1000,8 @@ func waitForSignal(
goRoutinesNumberStart int,
closingNodeStarted *atomic.Bool,
closeComponentsDelay time.Duration,
consensusComponentsCloser io.Closer,
executionManagerCloser io.Closer,
) nextOperationForNode {
var sig endProcess.ArgEndProcess
reshuffled := false
Expand All @@ -1020,7 +1024,7 @@ func waitForSignal(

chanCloseComponents := make(chan struct{})
go func() {
closeAllComponents(healthService, facade, httpServer, currentNode, chanCloseComponents, closingNodeStarted, closeComponentsDelay)
closeAllComponents(healthService, facade, httpServer, currentNode, chanCloseComponents, closingNodeStarted, closeComponentsDelay, consensusComponentsCloser, executionManagerCloser)
}()

select {
Expand Down Expand Up @@ -1595,11 +1599,19 @@ func closeAllComponents(
chanCloseComponents chan struct{},
closingNodeStarted *atomic.Bool,
closeComponentsDelay time.Duration,
consensusComponentsCloser io.Closer,
executionManagerCloser io.Closer,
) {
closingNodeStarted.Store(true)
// stop pruning, but wait a bit before closing the components to let the node finish processing the current block
time.Sleep(closeComponentsDelay)

log.Debug("stopping consensus...")
log.LogIfError(consensusComponentsCloser.Close())

log.Debug("stopping async execution...")
log.LogIfError(executionManagerCloser.Close())
Comment on lines +1609 to +1613

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consensusComponentsCloser.Close() / executionManagerCloser.Close() are invoked without any nil/IsInterfaceNil guard. Since these are interface-typed parameters, a nil value (or a typed-nil) would cause a panic during shutdown. Please add a defensive nil check (consistent with the check.IfNil(...) pattern used elsewhere for closables) or ensure the callers always pass non-nil closers.

Suggested change
log.Debug("stopping consensus...")
log.LogIfError(consensusComponentsCloser.Close())
log.Debug("stopping async execution...")
log.LogIfError(executionManagerCloser.Close())
if !check.IfNil(consensusComponentsCloser) {
log.Debug("stopping consensus...")
log.LogIfError(consensusComponentsCloser.Close())
}
if !check.IfNil(executionManagerCloser) {
log.Debug("stopping async execution...")
log.LogIfError(executionManagerCloser.Close())
}

Copilot uses AI. Check for mistakes.

Comment on lines +1609 to +1614

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closeAllComponents now closes consensus components and the execution manager explicitly, but both are already closed later via node.Close() (consensus handler is in n.closableComponents) and via processComponents.Close() (which closes executionManager). This duplicates shutdown responsibilities and makes the shutdown order harder to reason about if any of these Close() methods become non-idempotent. Consider consolidating ownership by closing them only through node.Close() (e.g., by adjusting n.closableComponents ordering) or documenting/guaranteeing idempotency if explicit early closes are required.

Suggested change
log.Debug("stopping consensus...")
log.LogIfError(consensusComponentsCloser.Close())
log.Debug("stopping async execution...")
log.LogIfError(executionManagerCloser.Close())
// consensus and async execution are closed by their respective owners during node shutdown

Copilot uses AI. Check for mistakes.
log.Debug("closing health service...")
err := healthService.Close()
log.LogIfError(err)
Expand Down
31 changes: 27 additions & 4 deletions node/nodeRunner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
"time"

"github.com/multiversx/mx-chain-core-go/data/endProcess"
logger "github.com/multiversx/mx-chain-logger-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/multiversx/mx-chain-go/common"
"github.com/multiversx/mx-chain-go/node/mock"
"github.com/multiversx/mx-chain-go/testscommon"
"github.com/multiversx/mx-chain-go/testscommon/api"
logger "github.com/multiversx/mx-chain-logger-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const originalConfigsPath = "../cmd/node/config"
Expand Down Expand Up @@ -174,6 +175,18 @@ func TestWaitForSignal(t *testing.T) {
return nil
},
}
consensusClosableComponent := &mock.CloserStub{
CloseCalled: func() error {
closedCalled["consensus"] = struct{}{}
return nil
},
}
executionManagerClosableComponent := &mock.CloserStub{
CloseCalled: func() error {
closedCalled["executionManager"] = struct{}{}
return nil
},
}
n, _ := NewNode()
n.closableComponents = append(n.closableComponents, internalNodeClosableComponent1)
n.closableComponents = append(n.closableComponents, internalNodeClosableComponent2)
Expand All @@ -199,6 +212,8 @@ func TestWaitForSignal(t *testing.T) {
1,
&atomic.Bool{},
time.Millisecond,
consensusClosableComponent,
executionManagerClosableComponent,
)

assert.Equal(t, nextOperationShouldStop, nextOperation)
Expand Down Expand Up @@ -227,6 +242,8 @@ func TestWaitForSignal(t *testing.T) {
1,
&atomic.Bool{},
time.Millisecond,
consensusClosableComponent,
executionManagerClosableComponent,
)

assert.Equal(t, nextOperationShouldRestart, nextOperation)
Expand Down Expand Up @@ -257,6 +274,8 @@ func TestWaitForSignal(t *testing.T) {
1,
&atomic.Bool{},
time.Millisecond,
consensusClosableComponent,
executionManagerClosableComponent,
)
close(functionFinished)
}()
Expand Down Expand Up @@ -299,6 +318,8 @@ func TestWaitForSignal(t *testing.T) {
1,
&atomic.Bool{},
time.Millisecond,
consensusClosableComponent,
executionManagerClosableComponent,
)

// these exceptions appear because the delayedComponent prevented the call of the first 2 components
Expand Down Expand Up @@ -330,6 +351,8 @@ func TestWaitForSignal(t *testing.T) {
1,
&atomic.Bool{},
time.Millisecond,
consensusClosableComponent,
executionManagerClosableComponent,
)

// these exceptions appear because the delayedComponent prevented the call of the first 2 components
Expand All @@ -342,7 +365,7 @@ func TestWaitForSignal(t *testing.T) {
}

func checkCloseCalledMap(tb testing.TB, closedCalled map[string]struct{}, exceptions ...string) {
allKeys := []string{"healthService", "facade", "http", "node closable component 1", "node closable component 2"}
allKeys := []string{"consensus", "executionManager", "healthService", "facade", "http", "node closable component 1", "node closable component 2"}
numKeys := 0
for _, key := range allKeys {
if contains(key, exceptions) {
Expand Down
Loading