🐛 subprocess Make process supervisor more robust by handling errors and undefined commands (#563)

<!--
Copyright (C) 2020-2022 Arm Limited or its affiliates and Contributors.
All rights reserved.
SPDX-License-Identifier: Apache-2.0
-->
### Description

<!--
Please add any detail or context that would be useful to a reviewer.
-->

Make process supervisor more robust by handling errors and undefined
commands

### Test Coverage

<!--
Please put an `x` in the correct box e.g. `[x]` to indicate the testing
coverage of this change.
-->

- [x]  This change is covered by existing or additional automated tests.
- [ ] Manual testing has been performed (and evidence provided) as
automated testing was not feasible.
- [ ] Additional tests are not required for this change (e.g.
documentation update).

Author: joshjennings98

changes/20250212110908.bugfix

@@ -0,0 +1 @@
+:bug: `subprocess` Make process supervisor more robust by handling errors and undefined commands

utils/subprocess/supervisor/supervisor.go

@@ -15,7 +15,7 @@ import (
 // A Supervisor will run a command and automatically restart it if it exits. Hooks can be used to execute code at
 // different points in the execution lifecyle. Restarts can be delayed
 type Supervisor struct {
-	newCommand    func(ctx context.Context) *subprocess.Subprocess
+	newCommand    func(ctx context.Context) (*subprocess.Subprocess, error)
 	preStart      func(context.Context) error
 	postStart     func(context.Context) error
 	postStop      func(context.Context, error) error
@@ -27,7 +27,7 @@ type SupervisorOption func(*Supervisor)
 
 // NewSupervisor will take a function 'newCommand' that creates a command and run it every time the command exits.
 // Options can be supplied by the 'opts' variadic argument that control the lifecyle of the supervisor
-func NewSupervisor(newCommand func(ctx context.Context) *subprocess.Subprocess, opts ...SupervisorOption) *Supervisor {
+func NewSupervisor(newCommand func(ctx context.Context) (*subprocess.Subprocess, error), opts ...SupervisorOption) *Supervisor {
 	supervisor := &Supervisor{
 		newCommand:   newCommand,
 		restartDelay: 0,
@@ -86,20 +86,33 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 		if s.preStart != nil {
 			err = s.preStart(ctx)
 			if err != nil {
-				err = fmt.Errorf("%w: error running pre-start hook: %v", commonerrors.ErrUnexpected, err.Error())
-				return
+				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
+					return err
+				}
+				return fmt.Errorf("%w: error running pre-start hook: %v", commonerrors.ErrUnexpected, err.Error())
 			}
 		}
 
 		g, _ := errgroup.WithContext(ctx)
-		cmd := s.newCommand(ctx)
+		cmd, err := s.newCommand(ctx)
+		if err != nil {
+			if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
+				return err
+			}
+			return fmt.Errorf("%w: error occurred when creating new command: %v", commonerrors.ErrUnexpected, err.Error())
+		}
+		if cmd == nil {
+			return fmt.Errorf("%w: command was undefined", commonerrors.ErrUndefined)
+		}
 		g.Go(cmd.Execute)
 
 		if s.postStart != nil {
 			err = s.postStart(ctx)
 			if err != nil {
-				err = fmt.Errorf("%w: error running post-start hook: %v", commonerrors.ErrUnexpected, err.Error())
-				return err
+				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
+					return err
+				}
+				return fmt.Errorf("%w: error running post-start hook: %v", commonerrors.ErrUnexpected, err.Error())
 			}
 		}
 
@@ -108,8 +121,10 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 		if s.postStop != nil {
 			err = s.postStop(context.WithoutCancel(ctx), processErr)
 			if err != nil {
-				err = fmt.Errorf("%w: error running post-stop hook: %v", commonerrors.ErrUnexpected, err.Error())
-				return err
+				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
+					return err
+				}
+				return fmt.Errorf("%w: error running post-stop hook: %v", commonerrors.ErrUnexpected, err.Error())
 			}
 		}
 

utils/subprocess/supervisor/supervisor_test.go

@@ -2,6 +2,7 @@ package supervisor
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"testing"
@@ -38,8 +39,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "sed", "-i", `$a test123`, testFile)
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		})
 
 		require.False(t, filesystem.Exists(testFile))
@@ -55,6 +56,30 @@ func TestSupervisor(t *testing.T) {
 		assert.Contains(t, string(written), "test\ntest123\ntest123")
 	})
 
+	t.Run("with command error", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+		defer cancel()
+
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return nil, errors.New("something happened")
+		})
+
+		err := runner.Run(ctx)
+		errortest.AssertError(t, err, commonerrors.ErrUnexpected)
+	})
+
+	t.Run("with nil command", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+		defer cancel()
+
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return nil, nil
+		})
+
+		err := runner.Run(ctx)
+		errortest.AssertError(t, err, commonerrors.ErrUndefined)
+	})
+
 	t.Run("with pre run", func(t *testing.T) {
 		if platform.IsWindows() {
 			t.SkipNow()
@@ -72,8 +97,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithPreStart(func(_ context.Context) error {
 			_ = counter.Inc()
 			return nil
@@ -102,8 +127,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithPostStart(func(_ context.Context) error {
 			_ = counter.Inc()
 			return nil
@@ -132,8 +157,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithPostStop(func(_ context.Context, _ error) error {
 			_ = counter.Inc()
 			return nil
@@ -164,8 +189,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithPreStart(func(_ context.Context) error {
 			_ = counter1.Inc()
 			return nil
@@ -182,6 +207,81 @@ func TestSupervisor(t *testing.T) {
 		assert.Equal(t, counter1.Load(), counter2.Load())
 	})
 
+	t.Run("with pre run (timeout)", func(t *testing.T) {
+		if platform.IsWindows() {
+			t.SkipNow()
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+		defer cancel()
+
+		logger, err := logs.NewLogrLogger(logstest.NewTestLogger(t), "Test")
+		require.NoError(t, err)
+
+		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
+		require.NoError(t, err)
+
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
+		}, WithPreStart(func(_ context.Context) error {
+			return commonerrors.ErrTimeout
+		}))
+
+		err = runner.Run(ctx)
+		errortest.AssertError(t, err, commonerrors.ErrTimeout)
+		assert.NotContains(t, err.Error(), "error running pre-start hook")
+	})
+
+	t.Run("with post run (timeout)", func(t *testing.T) {
+		if platform.IsWindows() {
+			t.SkipNow()
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+		defer cancel()
+
+		logger, err := logs.NewLogrLogger(logstest.NewTestLogger(t), "Test")
+		require.NoError(t, err)
+
+		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
+		require.NoError(t, err)
+
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
+		}, WithPostStart(func(_ context.Context) error {
+			return commonerrors.ErrTimeout
+		}))
+
+		err = runner.Run(ctx)
+		errortest.AssertError(t, err, commonerrors.ErrTimeout)
+		assert.NotContains(t, err.Error(), "error running post-start hook")
+	})
+
+	t.Run("with post stop (timeout)", func(t *testing.T) {
+		if platform.IsWindows() {
+			t.SkipNow()
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+		defer cancel()
+
+		logger, err := logs.NewLogrLogger(logstest.NewTestLogger(t), "Test")
+		require.NoError(t, err)
+
+		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "echo", "123")
+		require.NoError(t, err)
+
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
+		}, WithPostStop(func(_ context.Context, _ error) error {
+			return commonerrors.ErrTimeout
+		}))
+
+		err = runner.Run(ctx)
+		errortest.AssertError(t, err, commonerrors.ErrTimeout)
+		assert.NotContains(t, err.Error(), "error running post-stop hook")
+	})
+
 	t.Run("with cancel", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
@@ -195,8 +295,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "sed", "-i", `$a test123`, testFile)
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		})
 
 		cancel()
@@ -223,8 +323,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", failMessage, "sed", "-i", `$a test123`, testFile)
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithHaltingErrors(fmt.Errorf("%v %v", failMessage, commonerrors.ErrCancelled)))
 
 		require.False(t, filesystem.Exists(testFile))
@@ -262,8 +362,8 @@ func TestSupervisor(t *testing.T) {
 		cmd, err := subprocess.New(ctx, logger, "starting", "success", "failed", "sed", "-i", `$a test123`, testFile)
 		require.NoError(t, err)
 
-		runner := NewSupervisor(func(ctx context.Context) *subprocess.Subprocess {
-			return cmd
+		runner := NewSupervisor(func(ctx context.Context) (*subprocess.Subprocess, error) {
+			return cmd, nil
 		}, WithRestartDelay(time.Hour)) // won't have time to restart
 
 		require.False(t, filesystem.Exists(testFile))