chore: validate state machine lock in tests (#2362)

Author: JGAntunes

api/controllers/install/controller_test.go

@@ -246,6 +246,7 @@ func TestConfigureInstallation(t *testing.T) {
 			assert.Eventually(t, func() bool {
 				return sm.CurrentState() == tt.expectedState
 			}, time.Second, 100*time.Millisecond, "state should be %s but is %s", tt.expectedState, sm.CurrentState())
+			assert.False(t, sm.IsLockAcquired(), "state machine should not be locked after configuration")
 
 			mockManager.AssertExpectations(t)
 		})
@@ -413,6 +414,7 @@ func TestRunHostPreflights(t *testing.T) {
 			assert.Eventually(t, func() bool {
 				return sm.CurrentState() == tt.expectedState
 			}, time.Second, 100*time.Millisecond, "state should be %s but is %s", tt.expectedState, sm.CurrentState())
+			assert.False(t, sm.IsLockAcquired(), "state machine should not be locked after running preflights")
 
 			mockPreflightManager.AssertExpectations(t)
 		})
@@ -746,6 +748,7 @@ func TestSetupInfra(t *testing.T) {
 			assert.Eventually(t, func() bool {
 				return sm.CurrentState() == tt.expectedState
 			}, time.Second, 100*time.Millisecond, "state should be %s but is %s", tt.expectedState, sm.CurrentState())
+			assert.False(t, sm.IsLockAcquired(), "state machine should not be locked after running infra setup")
 
 			mockPreflightManager.AssertExpectations(t)
 			mockInstallationManager.AssertExpectations(t)

api/internal/statemachine/statemachine.go

@@ -26,6 +26,8 @@ type Interface interface {
 	Transition(lock Lock, nextState State) error
 	// AcquireLock acquires a lock on the state machine.
 	AcquireLock() (Lock, error)
+	// IsLockAcquired checks if a lock already exists on the state machine.
+	IsLockAcquired() bool
 }
 
 type Lock interface {
@@ -83,6 +85,13 @@ func (sm *stateMachine) AcquireLock() (Lock, error) {
 	return sm.lock, nil
 }
 
+func (sm *stateMachine) IsLockAcquired() bool {
+	sm.mu.RLock()
+	defer sm.mu.RUnlock()
+
+	return sm.lock != nil
+}
+
 func (sm *stateMachine) ValidateTransition(lock Lock, nextState State) error {
 	sm.mu.RLock()
 	defer sm.mu.RUnlock()

api/internal/statemachine/statemachine_test.go

@@ -43,11 +43,13 @@ var validStateTransitions = map[State][]State{
 
 func TestLockAcquisitionAndRelease(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	// Test valid lock acquisition
 	lock, err := sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Test transition with lock
 	err = sm.Transition(lock, StateInstallationConfigured)
@@ -56,120 +58,118 @@ func TestLockAcquisitionAndRelease(t *testing.T) {
 
 	// Release lock
 	lock.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// Test double lock acquisition
 	lock, err = sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock)
+	assert.True(t, sm.IsLockAcquired())
 
 	err = sm.Transition(lock, StatePreflightsRunning)
 	assert.NoError(t, err)
 
 	// Release lock
 	lock.Release()
 	assert.Equal(t, StatePreflightsRunning, sm.CurrentState())
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestDoubleLockAcquisition(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	lock1, err := sm.AcquireLock()
 	assert.NoError(t, err)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Try to acquire second lock while first is held
 	lock2, err := sm.AcquireLock()
 	assert.Error(t, err, "second lock acquisition should fail while first is held")
 	assert.Nil(t, lock2)
 	assert.Contains(t, err.Error(), "lock already acquired")
+	assert.True(t, sm.IsLockAcquired())
 
 	// Release first lock
 	lock1.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// Now second lock should work
 	lock2, err = sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock2)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Release second lock
 	lock2.Release()
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestLockReleaseAfterTransition(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	lock, err := sm.AcquireLock()
 	assert.NoError(t, err)
+	assert.True(t, sm.IsLockAcquired())
 
 	err = sm.Transition(lock, StateInstallationConfigured)
 	assert.NoError(t, err)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Release lock after transition
 	lock.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// State should remain changed
 	assert.Equal(t, StateInstallationConfigured, sm.CurrentState())
 }
 
 func TestDoubleLockRelease(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	lock, err := sm.AcquireLock()
 	assert.NoError(t, err)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Release lock
 	lock.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// Acquire another lock
 	lock2, err := sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock2)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Second release should not actually do anything
 	lock.Release()
+	assert.True(t, sm.IsLockAcquired())
 
 	// Should not be able to acquire lock after as the other lock is still held
 	nilLock, err := sm.AcquireLock()
 	assert.Error(t, err, "should not be able to acquire lock after as the other lock is still held")
 	assert.Nil(t, nilLock)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Release the second lock
 	lock2.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// Should be able to acquire lock after the other lock is released
 	lock3, err := sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock3)
+	assert.True(t, sm.IsLockAcquired())
 
 	lock3.Release()
-}
-
-func TestConcurrentLockBlocking(t *testing.T) {
-	sm := New(StateNew, validStateTransitions)
-
-	// Start first lock acquisition
-	lock1, err := sm.AcquireLock()
-	assert.NoError(t, err)
-	assert.NotNil(t, lock1)
-
-	// Try to acquire second lock while first is held
-	lock2, err := sm.AcquireLock()
-	assert.Error(t, err, "second lock should fail while first is held")
-	assert.Nil(t, lock2)
-	assert.Contains(t, err.Error(), "lock already acquired")
-
-	// Release first lock
-	lock1.Release()
-
-	// Now second lock should work
-	lock2, err = sm.AcquireLock()
-	assert.NoError(t, err)
-	assert.NotNil(t, lock2)
-
-	lock2.Release()
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestRaceConditionMultipleGoroutines(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	var wg sync.WaitGroup
 	successCount := 0
@@ -203,10 +203,13 @@ func TestRaceConditionMultipleGoroutines(t *testing.T) {
 	// Only one transition should succeed
 	assert.Equal(t, 1, successCount, "only one transition should succeed")
 	assert.Equal(t, StateInstallationConfigured, sm.CurrentState())
+	// There should be no lock acquired at the end
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestRaceConditionReadWrite(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	var wg sync.WaitGroup
 
@@ -257,6 +260,8 @@ func TestRaceConditionReadWrite(t *testing.T) {
 	finalState := sm.CurrentState()
 	assert.True(t, finalState == StateInstallationConfigured || finalState == StatePreflightsRunning,
 		"final state should be one of the expected states")
+	// There should be no lock acquired at the end
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestIsFinalState(t *testing.T) {
@@ -309,11 +314,13 @@ func TestFinalStateTransitionBlocking(t *testing.T) {
 
 func TestMultiStateTransitionWithLock(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	// Acquire lock and transition through multiple states
 	lock, err := sm.AcquireLock()
 	assert.NoError(t, err)
 	assert.NotNil(t, lock)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Transition 1: New -> StateInstallationConfigured
 	err = sm.Transition(lock, StateInstallationConfigured)
@@ -335,18 +342,22 @@ func TestMultiStateTransitionWithLock(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, StateInfrastructureInstalling, sm.CurrentState())
 
+	assert.True(t, sm.IsLockAcquired())
 	// Release the lock
 	lock.Release()
+	assert.False(t, sm.IsLockAcquired())
 
 	// State should be the final state in the transition chain
 	assert.Equal(t, StateInfrastructureInstalling, sm.CurrentState(), "state should be the final transitioned state after lock release")
 }
 
 func TestInvalidTransition(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
+	assert.False(t, sm.IsLockAcquired())
 
 	lock, err := sm.AcquireLock()
 	assert.NoError(t, err)
+	assert.True(t, sm.IsLockAcquired())
 
 	// Try invalid transition
 	err = sm.Transition(lock, StateSucceeded)
@@ -356,12 +367,15 @@ func TestInvalidTransition(t *testing.T) {
 	// State should remain unchanged
 	assert.Equal(t, StateNew, sm.CurrentState())
 
+	assert.True(t, sm.IsLockAcquired())
 	lock.Release()
+	assert.False(t, sm.IsLockAcquired())
 }
 
 func TestTransitionWithoutLock(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
 
+	assert.False(t, sm.IsLockAcquired())
 	err := sm.Transition(nil, StateInstallationConfigured)
 	assert.Error(t, err, "transition should be invalid")
 	assert.Contains(t, err.Error(), "lock not acquired")
@@ -370,6 +384,7 @@ func TestTransitionWithoutLock(t *testing.T) {
 func TestValidateTransitionWithoutLock(t *testing.T) {
 	sm := New(StateNew, validStateTransitions)
 
+	assert.False(t, sm.IsLockAcquired())
 	err := sm.ValidateTransition(nil, StateInstallationConfigured)
 	assert.Error(t, err, "transition should be invalid")
 	assert.Contains(t, err.Error(), "lock not acquired")