Skip to content

add trySetBusy #7815

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
AdoAdoAdo merged 5 commits into from
Apr 2, 2026
Merged

add trySetBusy #7815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
93df6ac
add trySetBusy
AdoAdoAdo Mar 31, 2026
5aa64ab
Merge branch 'feat/supernova-async-exec' into status-handler-try-set-…
AdoAdoAdo Apr 2, 2026
6bfb296
Merge branch 'feat/supernova-async-exec' into status-handler-try-set-…
sstanculeanu Apr 2, 2026
b656f3b
add unit tests
AdoAdoAdo Apr 2, 2026
bcca331
Merge branch 'status-handler-try-set-busy' of github.com:multiversx/m…
AdoAdoAdo Apr 2, 2026
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
3 changes: 3 additions & 0 deletions common/disabled/processStatusHandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ func NewProcessStatusHandler() *processStatusHandler {
// SetBusy does nothing
func (psh *processStatusHandler) SetBusy(_ string) {}

// TrySetBusy returns true
func (psh *processStatusHandler) TrySetBusy(_ string) bool { return true }

// SetIdle does nothing
func (psh *processStatusHandler) SetIdle() {}

Expand Down
1 change: 1 addition & 0 deletions common/disabled/processStatusHandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func TestProcessStatusHandler_MethodsShouldNotPanic(t *testing.T) {
psh := NewProcessStatusHandler()
assert.False(t, check.IfNil(psh))
psh.SetBusy("")
assert.True(t, psh.TrySetBusy(""))
psh.SetIdle()
assert.True(t, psh.IsIdle())
}
1 change: 1 addition & 0 deletions common/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ type StateStatisticsHandler interface {
// able to tell if the node is idle or processing/committing a block
type ProcessStatusHandler interface {
SetBusy(reason string)
Copy link
Copy Markdown
Contributor

@raduchis raduchis Apr 2, 2026

Choose a reason for hiding this comment

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

Is the SetBusy still needed? Could it be deleted?

Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

yes can be removed, only used in some tests now. Currently left it in.

TrySetBusy(reason string) bool
SetIdle()
IsIdle() bool
IsInterfaceNil() bool
Comment on lines 255 to 261
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Adding TrySetBusy to the exported ProcessStatusHandler interface is a breaking change for any downstream code that implements this interface (including satellite projects). If backward compatibility is required, consider introducing a new, narrower interface (e.g., ProcessStatusTryBusyHandler) and using interface assertions/optional support at call sites, or embedding the old interface into a new one while updating constructors accordingly.

Copilot uses AI. Check for mistakes.
Expand Down
8 changes: 6 additions & 2 deletions process/block/baseProcess.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2637,7 +2637,9 @@ func (bp *baseProcessor) Close() error {
// ProcessScheduledBlock processes a scheduled block
func (bp *baseProcessor) ProcessScheduledBlock(headerHandler data.HeaderHandler, bodyHandler data.BodyHandler, haveTime func() time.Duration) error {
var err error
bp.processStatusHandler.SetBusy("baseProcessor.ProcessScheduledBlock")
if !bp.processStatusHandler.TrySetBusy("baseProcessor.ProcessScheduledBlock") {
return process.ErrBlockProcessorBusy
}
defer func() {
Comment on lines 2638 to 2643
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

ProcessScheduledBlock can now return ErrBlockProcessorBusy when TrySetBusy fails, but there is no test covering this new branch. Please add a unit test (similar to existing busy/idle sequence tests) asserting ErrBlockProcessorBusy is returned when TrySetBusyCalled returns false.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

added tests

if err != nil {
bp.RevertCurrentBlock()
Expand Down Expand Up @@ -3562,14 +3564,16 @@ func (bp *baseProcessor) setCurrentBlockInfo(
func (bp *baseProcessor) getLastExecutedRootHash(
header data.HeaderHandler,
) []byte {
rootHash := bp.getRootHash()
var rootHash []byte
if !header.IsHeaderV3() {
rootHash = bp.getRootHash()
return rootHash
}

lastExecutionResult, err := common.GetLastBaseExecutionResultHandler(header)
if err != nil {
log.Warn("failed to get last execution result for header", "err", err)
_, _, rootHash = bp.blockChain.GetLastExecutedBlockInfo()
return rootHash
}

Expand Down
32 changes: 29 additions & 3 deletions process/block/baseProcess_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2651,6 +2651,29 @@ func TestBaseProcessor_updateState(t *testing.T) {
assert.Equal(t, []byte(strconv.Itoa(len(headers)-2)), cancelPruneRootHash)
}

func TestBaseProcessor_ProcessScheduledBlockShouldErrWhenProcessorBusy(t *testing.T) {
t.Parallel()

arguments := CreateMockArguments(createComponentHolderMocks())
processHandler := arguments.CoreComponents.ProcessStatusHandler()
mockProcessHandler := processHandler.(*testscommon.ProcessStatusHandlerStub)
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
return false
}
setIdleCalled := false
mockProcessHandler.SetIdleCalled = func() {
setIdleCalled = true
}

bp, _ := blproc.NewShardProcessor(arguments)

err := bp.ProcessScheduledBlock(
&block.MetaBlock{}, &block.Body{}, haveTime,
)
require.Equal(t, process.ErrBlockProcessorBusy, err)
require.False(t, setIdleCalled, "SetIdle should not be called when TrySetBusy fails")
}

func TestBaseProcessor_ProcessScheduledBlockShouldFail(t *testing.T) {
t.Parallel()

Expand All @@ -2664,8 +2687,9 @@ func TestBaseProcessor_ProcessScheduledBlockShouldFail(t *testing.T) {
mockProcessHandler.SetIdleCalled = func() {
busyIdleCalled = append(busyIdleCalled, idleIdentifier)
}
mockProcessHandler.SetBusyCalled = func(reason string) {
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
busyIdleCalled = append(busyIdleCalled, busyIdentifier)
return true
}

scheduledTxsExec := &testscommon.ScheduledTxsExecutionStub{
Expand Down Expand Up @@ -2694,8 +2718,9 @@ func TestBaseProcessor_ProcessScheduledBlockShouldFail(t *testing.T) {
mockProcessHandler.SetIdleCalled = func() {
busyIdleCalled = append(busyIdleCalled, idleIdentifier)
}
mockProcessHandler.SetBusyCalled = func(reason string) {
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
busyIdleCalled = append(busyIdleCalled, busyIdentifier)
return true
}

accounts := &stateMock.AccountsStub{
Expand Down Expand Up @@ -2775,8 +2800,9 @@ func TestBaseProcessor_ProcessScheduledBlockShouldWork(t *testing.T) {
mockProcessHandler.SetIdleCalled = func() {
busyIdleCalled = append(busyIdleCalled, idleIdentifier)
}
mockProcessHandler.SetBusyCalled = func(reason string) {
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
busyIdleCalled = append(busyIdleCalled, busyIdentifier)
return true
}

arguments.AccountsDB[state.UserAccountsState] = accounts
Expand Down
12 changes: 9 additions & 3 deletions process/block/metablock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ func (mp *metaProcessor) ProcessBlock(
return process.ErrNilHaveTimeHandler
}

mp.processStatusHandler.SetBusy("metaProcessor.ProcessBlock")
if !mp.processStatusHandler.TrySetBusy("metaProcessor.ProcessBlock") {
return process.ErrBlockProcessorBusy
}
defer mp.processStatusHandler.SetIdle()
Comment on lines +162 to 165
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This new early-return branch (ErrBlockProcessorBusy) is not covered by tests. Please add a unit test that makes TrySetBusy return false and asserts ProcessBlock returns ErrBlockProcessorBusy.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

added tests


err := mp.checkBlockValidity(headerHandler, bodyHandler)
Expand Down Expand Up @@ -724,7 +726,9 @@ func (mp *metaProcessor) CreateBlock(
return nil, nil, process.ErrWrongTypeAssertion
}

mp.processStatusHandler.SetBusy("metaProcessor.CreateBlock")
if !mp.processStatusHandler.TrySetBusy("metaProcessor.CreateBlock") {
return nil, nil, process.ErrBlockProcessorBusy
}
defer mp.processStatusHandler.SetIdle()

metaHdr.SoftwareVersion = []byte(mp.headerIntegrityVerifier.GetVersion(metaHdr.Epoch, metaHdr.Round))
Expand Down Expand Up @@ -1227,7 +1231,9 @@ func (mp *metaProcessor) CommitBlock(
prevBlockHeaderHash := mp.blockChain.GetCurrentBlockHeaderHash()

if !headerHandler.IsHeaderV3() {
mp.processStatusHandler.SetBusy("metaProcessor.CommitBlock")
if !mp.processStatusHandler.TrySetBusy("metaProcessor.CommitBlock") {
return process.ErrBlockProcessorBusy
}
defer func() {
if err != nil {
mp.RevertCurrentBlock()
Expand Down
4 changes: 3 additions & 1 deletion process/block/metablockProposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,9 @@ func (mp *metaProcessor) ProcessBlockProposal(
return nil, process.ErrInvalidHeader
}

mp.processStatusHandler.SetBusy("metaProcessor.ProcessBlockProposal")
if !mp.processStatusHandler.TrySetBusy("metaProcessor.ProcessBlockProposal") {
return nil, process.ErrBlockProcessorBusy
}
defer mp.processStatusHandler.SetIdle()

Comment on lines +311 to 315
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

Same issue as in shardProcessor: ProcessBlockProposal releases busy before CommitBlockProposalState runs (and CommitBlockProposalState is not guarded), so another goroutine can start processing in between and corrupt shared processor/account state. Consider guarding commit/revert proposal state as well or otherwise holding the busy lock across the full proposal+commit sequence.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

not possible on Supernova, single worker for execution, and the mentioned unprotected methods are only called on Supernova.

mp.roundNotifier.CheckRound(headerHandler)
Expand Down
49 changes: 47 additions & 2 deletions process/block/metablock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,49 @@ func TestMetaProcessor_ProcessBlockWithNilHaveTimeFuncShouldErr(t *testing.T) {
assert.Equal(t, process.ErrNilHaveTimeHandler, err)
}

func TestMetaProcessor_ProcessBlockShouldErrWhenProcessorBusy(t *testing.T) {
t.Parallel()

arguments := createMockMetaArguments(createMockComponentHolders())

processHandler := arguments.CoreComponents.ProcessStatusHandler()
mockProcessHandler := processHandler.(*testscommon.ProcessStatusHandlerStub)
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
return false
}

mp, _ := processBlock.NewMetaProcessor(arguments)
blk := &block.Body{}

err := mp.ProcessBlock(&block.MetaBlock{
Nonce: 1,
PubKeysBitmap: []byte("0100101"),
PrevHash: []byte(""),
Signature: []byte("signature"),
RootHash: []byte("roothash"),
}, blk, haveTime)
assert.Equal(t, process.ErrBlockProcessorBusy, err)
}

func TestMetaProcessor_CreateBlockShouldErrWhenProcessorBusy(t *testing.T) {
t.Parallel()

arguments := createMockMetaArguments(createMockComponentHolders())

processHandler := arguments.CoreComponents.ProcessStatusHandler()
mockProcessHandler := processHandler.(*testscommon.ProcessStatusHandlerStub)
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
return false
}

mp, _ := processBlock.NewMetaProcessor(arguments)

hdr, body, err := mp.CreateBlock(&block.MetaBlock{Round: 1}, func() bool { return true })
assert.Equal(t, process.ErrBlockProcessorBusy, err)
assert.Nil(t, hdr)
assert.Nil(t, body)
}

func TestMetaProcessor_ProcessWithDirtyAccountShouldErr(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1053,8 +1096,9 @@ func TestMetaProcessor_CommitBlockStorageFailsForHeaderShouldNotReturnError(t *t
mockProcessHandler.SetIdleCalled = func() {
busyIdleCalled = append(busyIdleCalled, idleIdentifier)
}
mockProcessHandler.SetBusyCalled = func(reason string) {
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
busyIdleCalled = append(busyIdleCalled, busyIdentifier)
return true
}

arguments.HeadersForBlock.AddHeaderUsedInBlock("hdr_hash1", &block.Header{})
Expand Down Expand Up @@ -3275,8 +3319,9 @@ func TestMetaProcessor_CreateAndProcessBlockCallsProcessAfterFirstEpoch(t *testi
mockProcessHandler.SetIdleCalled = func() {
busyIdleCalled = append(busyIdleCalled, idleIdentifier)
}
mockProcessHandler.SetBusyCalled = func(reason string) {
mockProcessHandler.TrySetBusyCalled = func(reason string) bool {
busyIdleCalled = append(busyIdleCalled, busyIdentifier)
return true
}

mp, _ := processBlock.NewMetaProcessor(arguments)
Expand Down
12 changes: 9 additions & 3 deletions process/block/shardblock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ func (sp *shardProcessor) ProcessBlock(
return process.ErrNilHaveTimeHandler
}

sp.processStatusHandler.SetBusy("shardProcessor.ProcessBlock")
if !sp.processStatusHandler.TrySetBusy("shardProcessor.ProcessBlock") {
return process.ErrBlockProcessorBusy
}
defer sp.processStatusHandler.SetIdle()
Comment on lines +101 to 104
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This new early-return branch (ErrBlockProcessorBusy) is not covered by tests in this package. Please add a unit test that sets TrySetBusy to return false and asserts ProcessBlock returns ErrBlockProcessorBusy (and does not call SetIdle).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

added unit tests


err := sp.checkBlockValidity(headerHandler, bodyHandler)
Expand Down Expand Up @@ -836,7 +838,9 @@ func (sp *shardProcessor) CreateBlock(
return nil, nil, process.ErrWrongTypeAssertion
}

sp.processStatusHandler.SetBusy("shardProcessor.CreateBlock")
if !sp.processStatusHandler.TrySetBusy("shardProcessor.CreateBlock") {
return nil, nil, process.ErrBlockProcessorBusy
}
defer sp.processStatusHandler.SetIdle()

err := sp.createBlockStarted()
Expand Down Expand Up @@ -942,7 +946,9 @@ func (sp *shardProcessor) CommitBlock(
prevBlockHeaderHash := sp.blockChain.GetCurrentBlockHeaderHash()

if !headerHandler.IsHeaderV3() {
sp.processStatusHandler.SetBusy("shardProcessor.CommitBlock")
if !sp.processStatusHandler.TrySetBusy("shardProcessor.CommitBlock") {
return process.ErrBlockProcessorBusy
}
defer func() {
if err != nil {
sp.RevertCurrentBlock()
Expand Down
4 changes: 3 additions & 1 deletion process/block/shardblockProposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,9 @@ func (sp *shardProcessor) ProcessBlockProposal(
return nil, process.ErrInvalidHeader
}

sp.processStatusHandler.SetBusy("shardProcessor.ProcessBlockProposal")
if !sp.processStatusHandler.TrySetBusy("shardProcessor.ProcessBlockProposal") {
return nil, process.ErrBlockProcessorBusy
}
defer sp.processStatusHandler.SetIdle()

Comment on lines +289 to 293
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

ProcessBlockProposal sets the processor back to idle at the end of this method, but the async execution flow calls CommitBlockProposalState afterwards and that method is not guarded by TrySetBusy/SetIdle. This allows another goroutine to acquire busy and start processing between proposal execution and state commit, which can break the intended mutual exclusion. Consider extending the busy window across CommitBlockProposalState/RevertBlockProposalState (or adding a similar TrySetBusy guard there).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdoAdoAdo AdoAdoAdo Apr 2, 2026

Choose a reason for hiding this comment

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

this is only possible on pre-Supernova (execution triggered from Consensus or Sync flow), on Supernova (where CommitBlockProposalState/RevertBlockProposalState) are called, headersExecutor is the only caller, and calls sequentially sequentially.

sp.roundNotifier.CheckRound(headerHandler)
Expand Down
Loading
Loading