chore(v3): fix state management race condition (#3203)

* chore(v3): fix state management

* f

* f

* f

* f

* reporting

* f

* f

* f

* f

* f

* cursor feedback

* cursor feedback

Author: emosbaugh

api/controllers/app/apppreflight.go

@@ -23,6 +23,8 @@ type RunAppPreflightOptions struct {
 }
 
 func (c *AppController) RunAppPreflights(ctx context.Context, opts RunAppPreflightOptions) (finalErr error) {
+	logger := c.logger.WithField("operation", "run-app-preflights")
+
 	lock, err := c.stateMachine.AcquireLock()
 	if err != nil {
 		return types.NewConflictError(err)
@@ -41,6 +43,11 @@ func (c *AppController) RunAppPreflights(ctx context.Context, opts RunAppPreflig
 		return types.NewConflictError(err)
 	}
 
+	// Clear any previous preflight results immediately to prevent serving stale data
+	if err := c.appPreflightManager.ClearAppPreflightResults(ctx); err != nil {
+		return fmt.Errorf("clear previous preflight results: %w", err)
+	}
+
 	// Get the app config values
 	configValues, err := c.GetAppConfigValues(ctx)
 	if err != nil {
@@ -57,7 +64,7 @@ func (c *AppController) RunAppPreflights(ctx context.Context, opts RunAppPreflig
 		return fmt.Errorf("no app preflight spec found")
 	}
 
-	err = c.stateMachine.Transition(lock, states.StateAppPreflightsRunning)
+	err = c.stateMachine.Transition(lock, states.StateAppPreflightsRunning, nil)
 	if err != nil {
 		return fmt.Errorf("transition states: %w", err)
 	}
@@ -78,14 +85,14 @@ func (c *AppController) RunAppPreflights(ctx context.Context, opts RunAppPreflig
 				finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
 			}
 			if finalErr != nil {
-				c.logger.Error(finalErr)
+				logger.Error(finalErr)
 
-				if err := c.stateMachine.Transition(lock, states.StateAppPreflightsExecutionFailed); err != nil {
-					c.logger.WithError(err).Error("failed to transition states")
+				if err := c.stateMachine.Transition(lock, states.StateAppPreflightsExecutionFailed, finalErr); err != nil {
+					logger.WithError(err).Error("failed to transition states")
 				}
 
 				if err := c.setAppPreflightStatus(types.StateFailed, finalErr.Error()); err != nil {
-					c.logger.WithError(err).Error("failed to set status to failed")
+					logger.WithError(err).Error("failed to set status to failed")
 				}
 			}
 		}()
@@ -110,15 +117,15 @@ func (c *AppController) RunAppPreflights(ctx context.Context, opts RunAppPreflig
 		}
 
 		if output.HasFail() {
-			if err := c.stateMachine.Transition(lock, states.StateAppPreflightsFailed); err != nil {
+			if err := c.stateMachine.Transition(lock, states.StateAppPreflightsFailed, output); err != nil {
 				return fmt.Errorf("transition states: %w", err)
 			}
 
 			if err := c.setAppPreflightStatus(types.StateFailed, "App preflights failed"); err != nil {
 				return fmt.Errorf("set status to failed: %w", err)
 			}
 		} else {
-			if err := c.stateMachine.Transition(lock, states.StateAppPreflightsSucceeded); err != nil {
+			if err := c.stateMachine.Transition(lock, states.StateAppPreflightsSucceeded, output); err != nil {
 				return fmt.Errorf("transition states: %w", err)
 			}
 

api/controllers/app/config.go

@@ -21,7 +21,7 @@ func (c *AppController) PatchAppConfigValues(ctx context.Context, values types.A
 		return types.NewConflictError(err)
 	}
 
-	err = c.stateMachine.Transition(lock, states.StateApplicationConfiguring)
+	err = c.stateMachine.Transition(lock, states.StateApplicationConfiguring, nil)
 	if err != nil {
 		return fmt.Errorf("failed to transition states: %w", err)
 	}
@@ -32,8 +32,8 @@ func (c *AppController) PatchAppConfigValues(ctx context.Context, values types.A
 		}
 
 		if finalErr != nil {
-			if err := c.stateMachine.Transition(lock, states.StateApplicationConfigurationFailed); err != nil {
-				c.logger.Errorf("failed to transition states: %w", err)
+			if err := c.stateMachine.Transition(lock, states.StateApplicationConfigurationFailed, finalErr); err != nil {
+				c.logger.WithError(err).Error("failed to transition states")
 			}
 		}
 	}()
@@ -48,7 +48,7 @@ func (c *AppController) PatchAppConfigValues(ctx context.Context, values types.A
 		return fmt.Errorf("patch app config values: %w", err)
 	}
 
-	err = c.stateMachine.Transition(lock, states.StateApplicationConfigured)
+	err = c.stateMachine.Transition(lock, states.StateApplicationConfigured, nil)
 	if err != nil {
 		return fmt.Errorf("failed to transition states: %w", err)
 	}

api/controllers/app/install.go

@@ -17,6 +17,8 @@ var (
 
 // InstallApp triggers app installation with proper state transitions and panic handling
 func (c *AppController) InstallApp(ctx context.Context, ignoreAppPreflights bool) (finalErr error) {
+	logger := c.logger.WithField("operation", "install-app")
+
 	lock, err := c.stateMachine.AcquireLock()
 	if err != nil {
 		return types.NewConflictError(err)
@@ -47,7 +49,7 @@ func (c *AppController) InstallApp(ctx context.Context, ignoreAppPreflights bool
 			return types.NewBadRequestError(ErrAppPreflightChecksFailed)
 		}
 
-		err = c.stateMachine.Transition(lock, states.StateAppPreflightsFailedBypassed)
+		err = c.stateMachine.Transition(lock, states.StateAppPreflightsFailedBypassed, preflightOutput)
 		if err != nil {
 			return fmt.Errorf("failed to transition states: %w", err)
 		}
@@ -63,7 +65,7 @@ func (c *AppController) InstallApp(ctx context.Context, ignoreAppPreflights bool
 		return fmt.Errorf("get kotsadm config values for app install: %w", err)
 	}
 
-	err = c.stateMachine.Transition(lock, states.StateAppInstalling)
+	err = c.stateMachine.Transition(lock, states.StateAppInstalling, nil)
 	if err != nil {
 		return fmt.Errorf("transition states: %w", err)
 	}
@@ -79,14 +81,14 @@ func (c *AppController) InstallApp(ctx context.Context, ignoreAppPreflights bool
 				finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
 			}
 			if finalErr != nil {
-				c.logger.Error(finalErr)
+				logger.Error(finalErr)
 
-				if err := c.stateMachine.Transition(lock, states.StateAppInstallFailed); err != nil {
-					c.logger.WithError(err).Error("failed to transition states")
+				if err := c.stateMachine.Transition(lock, states.StateAppInstallFailed, finalErr); err != nil {
+					logger.WithError(err).Error("failed to transition states")
 				}
 
 				if err := c.setAppInstallStatus(types.StateFailed, finalErr.Error()); err != nil {
-					c.logger.WithError(err).Error("failed to set status to failed")
+					logger.WithError(err).Error("failed to set status to failed")
 				}
 			}
 		}()
@@ -101,7 +103,7 @@ func (c *AppController) InstallApp(ctx context.Context, ignoreAppPreflights bool
 			return fmt.Errorf("install app: %w", err)
 		}
 
-		if err := c.stateMachine.Transition(lock, states.StateSucceeded); err != nil {
+		if err := c.stateMachine.Transition(lock, states.StateSucceeded, nil); err != nil {
 			return fmt.Errorf("transition states: %w", err)
 		}
 

api/controllers/app/tests/test_suite.go

@@ -221,6 +221,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateAppPreflightsSucceeded,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(expectedAPF, nil),
@@ -256,6 +257,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateAppPreflightsSucceeded,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(expectedAPF, nil),
@@ -291,6 +293,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateAppPreflightsSucceeded,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(expectedAPF, nil),
@@ -326,6 +329,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateAppPreflightsFailed,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(expectedAPF, nil),
@@ -361,6 +365,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateInfrastructureInstalled,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(nil, errors.New("extraction error")),
@@ -377,6 +382,7 @@ func (s *AppControllerTestSuite) TestRunAppPreflights() {
 			expectedState: states.StateAppPreflightsExecutionFailed,
 			setupMocks: func(apm *apppreflightmanager.MockAppPreflightManager, arm *appreleasemanager.MockAppReleaseManager, acm *appconfig.MockAppConfigManager, store *store.MockStore) {
 				mock.InOrder(
+					apm.On("ClearAppPreflightResults", mock.Anything).Return(nil),
 					acm.On("GetConfigValues").Return(types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, nil),
 
 					arm.On("ExtractAppPreflightSpec", mock.Anything, types.AppConfigValues{"test-item": types.AppConfigValue{Value: "test-value"}}, mock.Anything, mock.Anything).Return(expectedAPF, nil),
@@ -721,14 +727,14 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 		ignoreAppPreflights bool
 		currentState        statemachine.State
 		expectedState       statemachine.State
-		setupMocks          func(*appconfig.MockAppConfigManager, *appupgrademanager.MockAppUpgradeManager)
+		setupMocks          func(*appconfig.MockAppConfigManager, *appupgrademanager.MockAppUpgradeManager, *store.MockStore)
 		expectedErr         bool
 	}{
 		{
 			name:          "invalid state transition from succeeded state",
 			currentState:  states.StateSucceeded,
 			expectedState: states.StateSucceeded,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				// No mocks needed for invalid state transition
 			},
 			expectedErr: true,
@@ -737,7 +743,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "invalid state transition from app upgrading state",
 			currentState:  states.StateAppUpgrading,
 			expectedState: states.StateAppUpgrading,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				// No mocks needed for invalid state transition
 			},
 			expectedErr: true,
@@ -746,7 +752,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "invalid state transition from app upgrade failed state",
 			currentState:  states.StateAppUpgradeFailed,
 			expectedState: states.StateAppUpgradeFailed,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				// No mocks needed for invalid state transition
 			},
 			expectedErr: true,
@@ -755,7 +761,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "invalid state transition from app configured state",
 			currentState:  states.StateApplicationConfigured,
 			expectedState: states.StateApplicationConfigured,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				// No mocks needed for invalid state transition
 			},
 			expectedErr: true,
@@ -764,7 +770,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "successful app upgrade from app preflights succeeded state",
 			currentState:  states.StateAppPreflightsSucceeded,
 			expectedState: states.StateSucceeded,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				mock.InOrder(
 					acm.On("GetKotsadmConfigValues").Return(kotsv1beta1.ConfigValues{
 						Spec: kotsv1beta1.ConfigValuesSpec{
@@ -773,9 +779,15 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 							},
 						},
 					}, nil),
+					st.AppUpgradeMockStore.On("SetStatus", mock.MatchedBy(func(status types.Status) bool {
+						return status.State == types.StateRunning
+					})).Return(nil),
 					aum.On("Upgrade", mock.Anything, mock.MatchedBy(func(cv kotsv1beta1.ConfigValues) bool {
 						return cv.Spec.Values["test-key"].Value == "test-value"
 					})).Return(nil),
+					st.AppUpgradeMockStore.On("SetStatus", mock.MatchedBy(func(status types.Status) bool {
+						return status.State == types.StateSucceeded
+					})).Return(nil),
 				)
 			},
 			expectedErr: false,
@@ -784,7 +796,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "get config values error",
 			currentState:  states.StateAppPreflightsSucceeded,
 			expectedState: states.StateAppPreflightsSucceeded,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				acm.On("GetKotsadmConfigValues").Return(kotsv1beta1.ConfigValues{}, errors.New("config values error"))
 			},
 			expectedErr: true,
@@ -793,7 +805,7 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			name:          "app upgrade manager error",
 			currentState:  states.StateAppPreflightsSucceeded,
 			expectedState: states.StateAppUpgradeFailed,
-			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager) {
+			setupMocks: func(acm *appconfig.MockAppConfigManager, aum *appupgrademanager.MockAppUpgradeManager, st *store.MockStore) {
 				mock.InOrder(
 					acm.On("GetKotsadmConfigValues").Return(kotsv1beta1.ConfigValues{
 						Spec: kotsv1beta1.ConfigValuesSpec{
@@ -802,9 +814,15 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 							},
 						},
 					}, nil),
+					st.AppUpgradeMockStore.On("SetStatus", mock.MatchedBy(func(status types.Status) bool {
+						return status.State == types.StateRunning
+					})).Return(nil),
 					aum.On("Upgrade", mock.Anything, mock.MatchedBy(func(cv kotsv1beta1.ConfigValues) bool {
 						return cv.Spec.Values["test-key"].Value == "test-value"
 					})).Return(errors.New("upgrade error")),
+					st.AppUpgradeMockStore.On("SetStatus", mock.MatchedBy(func(status types.Status) bool {
+						return status.State == types.StateFailed && status.Description == "upgrade app: upgrade error"
+					})).Return(nil),
 				)
 			},
 			expectedErr: false,
@@ -822,19 +840,21 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 			appConfigManager := &appconfig.MockAppConfigManager{}
 			appConfigManager.On("TemplateConfig", types.AppConfigValues{}, false, false).Return(types.AppConfig{}, nil)
 
+			mockStore := &store.MockStore{}
+
 			controller, err := appcontroller.NewAppController(
 				appcontroller.WithStateMachine(sm),
 				appcontroller.WithAppConfigManager(appConfigManager),
 				appcontroller.WithAppPreflightManager(appPreflightManager),
 				appcontroller.WithAppReleaseManager(appReleaseManager),
 				appcontroller.WithAppInstallManager(appInstallManager),
 				appcontroller.WithAppUpgradeManager(appUpgradeManager),
-				appcontroller.WithStore(&store.MockStore{}),
+				appcontroller.WithStore(mockStore),
 				appcontroller.WithReleaseData(&release.ReleaseData{}),
 			)
 			require.NoError(t, err, "failed to create app controller")
 
-			tt.setupMocks(appConfigManager, appUpgradeManager)
+			tt.setupMocks(appConfigManager, appUpgradeManager, mockStore)
 			err = controller.UpgradeApp(t.Context(), tt.ignoreAppPreflights)
 
 			if tt.expectedErr {
@@ -857,73 +877,3 @@ func (s *AppControllerTestSuite) TestUpgradeApp() {
 		})
 	}
 }
-
-func (s *AppControllerTestSuite) TestGetAppUpgradeStatus() {
-	expectedAppUpgrade := types.AppUpgrade{
-		Status: types.Status{
-			State:       types.StateRunning,
-			Description: "Upgrading application",
-			LastUpdated: time.Now(),
-		},
-		Logs: "Upgrade logs\n",
-	}
-
-	tests := []struct {
-		name        string
-		setupMocks  func(*appupgrademanager.MockAppUpgradeManager)
-		expectedErr bool
-	}{
-		{
-			name: "successful status retrieval",
-			setupMocks: func(aum *appupgrademanager.MockAppUpgradeManager) {
-				aum.On("GetStatus").Return(expectedAppUpgrade, nil)
-			},
-			expectedErr: false,
-		},
-		{
-			name: "manager returns error",
-			setupMocks: func(aum *appupgrademanager.MockAppUpgradeManager) {
-				aum.On("GetStatus").Return(types.AppUpgrade{}, errors.New("status error"))
-			},
-			expectedErr: true,
-		},
-	}
-
-	for _, tt := range tests {
-		s.T().Run(tt.name, func(t *testing.T) {
-			appPreflightManager := &apppreflightmanager.MockAppPreflightManager{}
-			appReleaseManager := &appreleasemanager.MockAppReleaseManager{}
-			appInstallManager := &appinstallmanager.MockAppInstallManager{}
-			appUpgradeManager := &appupgrademanager.MockAppUpgradeManager{}
-			sm := s.CreateUpgradeStateMachine(states.StateNew)
-
-			appConfigManager := &appconfig.MockAppConfigManager{}
-			appConfigManager.On("TemplateConfig", types.AppConfigValues{}, false, false).Return(types.AppConfig{}, nil)
-
-			controller, err := appcontroller.NewAppController(
-				appcontroller.WithStateMachine(sm),
-				appcontroller.WithAppConfigManager(appConfigManager),
-				appcontroller.WithAppPreflightManager(appPreflightManager),
-				appcontroller.WithAppReleaseManager(appReleaseManager),
-				appcontroller.WithAppInstallManager(appInstallManager),
-				appcontroller.WithAppUpgradeManager(appUpgradeManager),
-				appcontroller.WithStore(&store.MockStore{}),
-				appcontroller.WithReleaseData(&release.ReleaseData{}),
-			)
-			require.NoError(t, err, "failed to create app controller")
-
-			tt.setupMocks(appUpgradeManager)
-			result, err := controller.GetAppUpgradeStatus(t.Context())
-
-			if tt.expectedErr {
-				assert.Error(t, err)
-				assert.Equal(t, types.AppUpgrade{}, result)
-			} else {
-				assert.NoError(t, err)
-				assert.Equal(t, expectedAppUpgrade, result)
-			}
-
-			appUpgradeManager.AssertExpectations(t)
-		})
-	}
-}