Fix potential deadlocks in the statemachine (#2346)

sgalsaleh · web-flow · commit 8fa788bbb64d · 2025-06-23T05:39:00.000-07:00
diff --git a/api/README.md b/api/README.md
@@ -16,7 +16,7 @@ Contains the business logic for different API endpoints. Each controller package
 Contains shared utilities and helper packages that provide common functionality used across different parts of the API. This includes both general-purpose utilities and domain-specific helpers.
 
 #### `/internal/managers`
-Each manager is responsible for a specific subdomain of functionality and provides a clean, thread-safe interface for controllers to interact with. For example, the Preflight Manager manages system requirement checks and validation.
+Each manager is responsible for a specific subdomain of functionality and provides a clean interface for controllers to interact with. For example, the Preflight Manager manages system requirement checks and validation.
 
 #### `/internal/statemachine`
 The statemachine is used by controllers to capture workflow state and enforce valid transitions.
diff --git a/api/controllers/install/hostpreflight.go b/api/controllers/install/hostpreflight.go
@@ -11,12 +11,21 @@ import (
 	"github.com/replicatedhq/embedded-cluster/pkg/netutils"
 )
 
-func (c *InstallController) RunHostPreflights(ctx context.Context, opts RunHostPreflightsOptions) error {
+func (c *InstallController) RunHostPreflights(ctx context.Context, opts RunHostPreflightsOptions) (finalErr error) {
 	lock, err := c.stateMachine.AcquireLock()
 	if err != nil {
 		return types.NewConflictError(err)
 	}
 
+	defer func() {
+		if r := recover(); r != nil {
+			finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
+		}
+		if finalErr != nil {
+			lock.Release()
+		}
+	}()
+
 	if err := c.stateMachine.ValidateTransition(lock, StatePreflightsRunning); err != nil {
 		return types.NewConflictError(err)
 	}
@@ -34,7 +43,6 @@ func (c *InstallController) RunHostPreflights(ctx context.Context, opts RunHostP
 		IsUI:                  opts.IsUI,
 	})
 	if err != nil {
-		lock.Release()
 		return fmt.Errorf("failed to prepare host preflights: %w", err)
 	}
 
@@ -43,18 +51,24 @@ func (c *InstallController) RunHostPreflights(ctx context.Context, opts RunHostP
 		return fmt.Errorf("failed to transition states: %w", err)
 	}
 
-	go func() {
+	go func() (finalErr error) {
 		// Background context is used to avoid canceling the operation if the context is canceled
 		ctx := context.Background()
 
 		defer lock.Release()
 
 		defer func() {
 			if r := recover(); r != nil {
-				c.logger.Errorf("panic running host preflights: %v: %s", r, string(debug.Stack()))
+				finalErr = fmt.Errorf("panic running host preflights: %v: %s", r, string(debug.Stack()))
+			}
+			if finalErr != nil {
+				c.logger.Error(finalErr)
 
-				err := c.stateMachine.Transition(lock, StatePreflightsFailed)
-				if err != nil {
+				if err := c.stateMachine.Transition(lock, StatePreflightsFailed); err != nil {
+					c.logger.Errorf("failed to transition states: %w", err)
+				}
+			} else {
+				if err := c.stateMachine.Transition(lock, StatePreflightsSucceeded); err != nil {
 					c.logger.Errorf("failed to transition states: %w", err)
 				}
 			}
@@ -63,20 +77,11 @@ func (c *InstallController) RunHostPreflights(ctx context.Context, opts RunHostP
 		err := c.hostPreflightManager.RunHostPreflights(ctx, c.rc, preflight.RunHostPreflightOptions{
 			HostPreflightSpec: hpf,
 		})
-
 		if err != nil {
-			c.logger.Errorf("failed to run host preflights: %w", err)
-
-			err = c.stateMachine.Transition(lock, StatePreflightsFailed)
-			if err != nil {
-				c.logger.Errorf("failed to transition states: %w", err)
-			}
-		} else {
-			err = c.stateMachine.Transition(lock, StatePreflightsSucceeded)
-			if err != nil {
-				c.logger.Errorf("failed to transition states: %w", err)
-			}
+			return fmt.Errorf("failed to run host preflights: %w", err)
 		}
+
+		return nil
 	}()
 
 	return nil
diff --git a/api/controllers/install/infra.go b/api/controllers/install/infra.go
@@ -8,7 +8,7 @@ import (
 	"github.com/replicatedhq/embedded-cluster/api/types"
 )
 
-func (c *InstallController) SetupInfra(ctx context.Context) error {
+func (c *InstallController) SetupInfra(ctx context.Context) (finalErr error) {
 	if c.stateMachine.CurrentState() == StatePreflightsFailed {
 		err := c.bypassPreflights(ctx)
 		if err != nil {
@@ -21,44 +21,48 @@ func (c *InstallController) SetupInfra(ctx context.Context) error {
 		return types.NewConflictError(err)
 	}
 
+	defer func() {
+		if r := recover(); r != nil {
+			finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
+		}
+		if finalErr != nil {
+			lock.Release()
+		}
+	}()
+
 	err = c.stateMachine.Transition(lock, StateInfrastructureInstalling)
 	if err != nil {
-		lock.Release()
 		return types.NewConflictError(err)
 	}
 
-	go func() {
+	go func() (finalErr error) {
 		// Background context is used to avoid canceling the operation if the context is canceled
 		ctx := context.Background()
 
 		defer lock.Release()
 
 		defer func() {
 			if r := recover(); r != nil {
-				c.logger.Errorf("panic installing infrastructure: %v: %s", r, string(debug.Stack()))
+				finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
+			}
+			if finalErr != nil {
+				c.logger.Error(finalErr)
 
-				err := c.stateMachine.Transition(lock, StateFailed)
-				if err != nil {
+				if err := c.stateMachine.Transition(lock, StateFailed); err != nil {
+					c.logger.Errorf("failed to transition states: %w", err)
+				}
+			} else {
+				if err := c.stateMachine.Transition(lock, StateSucceeded); err != nil {
 					c.logger.Errorf("failed to transition states: %w", err)
 				}
 			}
 		}()
 
-		err := c.infraManager.Install(ctx, c.rc)
-
-		if err != nil {
-			c.logger.Errorf("failed to install infrastructure: %w", err)
-
-			err := c.stateMachine.Transition(lock, StateFailed)
-			if err != nil {
-				c.logger.Errorf("failed to transition states: %w", err)
-			}
-		} else {
-			err = c.stateMachine.Transition(lock, StateSucceeded)
-			if err != nil {
-				c.logger.Errorf("failed to transition states: %w", err)
-			}
+		if err := c.infraManager.Install(ctx, c.rc); err != nil {
+			return fmt.Errorf("failed to install infrastructure: %w", err)
 		}
+
+		return nil
 	}()
 
 	return nil
diff --git a/api/internal/managers/infra/install.go b/api/internal/managers/infra/install.go
@@ -52,23 +52,20 @@ func (m *infraManager) Install(ctx context.Context, rc runtimeconfig.RuntimeConf
 	defer func() {
 		if r := recover(); r != nil {
 			finalErr = fmt.Errorf("panic: %v: %s", r, string(debug.Stack()))
-
-			if err := m.setStatus(types.StateFailed, "Installation failed to run: panic"); err != nil {
+		}
+		if finalErr != nil {
+			if err := m.setStatus(types.StateFailed, finalErr.Error()); err != nil {
 				m.logger.WithField("error", err).Error("set failed status")
 			}
+		} else {
+			if err := m.setStatus(types.StateSucceeded, "Installation complete"); err != nil {
+				m.logger.WithField("error", err).Error("set succeeded status")
+			}
 		}
 	}()
 
-	err = m.install(ctx, rc)
-
-	if err != nil {
-		if err := m.setStatus(types.StateFailed, err.Error()); err != nil {
-			m.logger.WithField("error", err).Error("set failed status")
-		}
-	} else {
-		if err := m.setStatus(types.StateSucceeded, "Installation complete"); err != nil {
-			m.logger.WithField("error", err).Error("set succeeded status")
-		}
+	if err := m.install(ctx, rc); err != nil {
+		return err
 	}
 
 	return nil
