🐛 [subprocess] Improve the error reporting when a process is cancelled (#655)

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

 Improve the error reported when a process is cancelled


### 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: acabarbaye

changes/20250718123400.bugfix

@@ -0,0 +1 @@
+:bug: `[subprocess]` Improve the error reporting when a process is cancelled

utils/platform/deletion.go

@@ -2,7 +2,7 @@ package platform
 
 import (
 	"context"
-	"fmt"
+	"errors"
 	"os"
 	"os/exec"
 	"time"
@@ -16,7 +16,7 @@ import (
 func RemoveWithPrivileges(ctx context.Context, path string) (err error) {
 	fi, err := os.Stat(path)
 	if err != nil {
-		err = fmt.Errorf("%w: could not find path [%v]: %v", commonerrors.ErrNotFound, path, err.Error())
+		err = commonerrors.WrapErrorf(commonerrors.ErrNotFound, err, "could not find path [%v]", path)
 		return
 	}
 	if fi.IsDir() {
@@ -25,33 +25,25 @@ func RemoveWithPrivileges(ctx context.Context, path string) (err error) {
 		err = removeFileAs(ctx, WithPrivileges(command.Me()), path)
 	}
 	if err != nil {
-		err = fmt.Errorf("%w: could not remove the path [%v]: %v", commonerrors.ErrUnexpected, path, err.Error())
+		err = commonerrors.WrapErrorf(commonerrors.ErrUnexpected, err, "could not remove the path [%v]", path)
 	}
 	return
 }
 
 func executeCommandAs(ctx context.Context, as *command.CommandAsDifferentUser, args ...string) error {
 	if as == nil {
-		return fmt.Errorf("%w: missing command wrapper", commonerrors.ErrUndefined)
+		return commonerrors.UndefinedVariable("command wrapper")
 	}
 	if len(args) == 0 {
-		return fmt.Errorf("%w: missing command to execute", commonerrors.ErrUndefined)
+		return commonerrors.UndefinedVariable("command to execute")
 	}
 	cmdName, cmdArgs := as.RedefineCommand(args...)
 	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 	// setting the following to avoid having hanging subprocesses as described in https://github.com/golang/go/issues/24050
 	cmd.WaitDelay = 5 * time.Second
-	cmd.Cancel = func() error {
-		if cmd.Process == nil {
-			return nil
-		}
-		p, err := proc.FindProcess(context.Background(), cmd.Process.Pid)
-		if err == nil {
-			return p.KillWithChildren(context.Background())
-		} else {
-			// Default behaviour
-			return cmd.Process.Kill()
-		}
+	cmd, err := proc.DefineCmdCancel(cmd)
+	if err != nil {
+		return commonerrors.WrapError(commonerrors.ErrUnexpected, err, "could not set the command cancel function")
 	}
 	return runCommand(args[0], cmd)
 }
@@ -71,10 +63,11 @@ func runCommand(cmdDescription string, cmd *exec.Cmd) error {
 		return newErr
 	default:
 		details := "no further details"
-		if exitError, ok := err.(*exec.ExitError); ok {
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
 			details = string(exitError.Stderr)
 		}
-		newErr = fmt.Errorf("%w: the command `%v` failed: %v (%v)", commonerrors.ErrUnknown, cmdDescription, err.Error(), details)
+		newErr = commonerrors.WrapErrorf(commonerrors.ErrUnknown, err, "the command `%v` failed (%v)", cmdDescription, details)
 		return newErr
 	}
 }

utils/proc/interrupt.go

@@ -2,6 +2,8 @@ package proc
 
 import (
 	"context"
+	"os"
+	"os/exec"
 	"time"
 
 	"github.com/ARM-software/golang-utils/utils/commonerrors"
@@ -12,9 +14,10 @@ import (
 type InterruptType int
 
 const (
-	SigInt  InterruptType = 2
-	SigKill InterruptType = 9
-	SigTerm InterruptType = 15
+	SigInt                           InterruptType = 2
+	SigKill                          InterruptType = 9
+	SigTerm                          InterruptType = 15
+	SubprocessTerminationGracePeriod               = 10 * time.Millisecond
 )
 
 func InterruptProcess(ctx context.Context, pid int, signal InterruptType) (err error) {
@@ -62,3 +65,32 @@ func TerminateGracefully(ctx context.Context, pid int, gracePeriod time.Duration
 	err = InterruptProcess(ctx, pid, SigKill)
 	return
 }
+
+// CancelExecCommand defines a more robust way to cancel subprocesses than what is done per default by [CommandContext](https://pkg.go.dev/os/exec#CommandContext)
+func CancelExecCommand(cmd *exec.Cmd) (err error) {
+	if cmd == nil {
+		err = commonerrors.UndefinedVariable("command")
+		return
+	}
+	if cmd.Process == nil {
+		return
+	}
+	err = TerminateGracefully(context.Background(), cmd.Process.Pid, SubprocessTerminationGracePeriod)
+	err = commonerrors.Ignore(err, os.ErrProcessDone)
+	if err != nil {
+		// Default behaviour
+		err = cmd.Process.Kill()
+	}
+	return
+}
+
+// DefineCmdCancel sets and overwrites the cmd.Cancel function with CancelExecCommand so that it is more robust and thorough.
+func DefineCmdCancel(cmd *exec.Cmd) (*exec.Cmd, error) {
+	if cmd == nil {
+		return nil, commonerrors.UndefinedVariable("command")
+	}
+	cmd.Cancel = func() error {
+		return CancelExecCommand(cmd)
+	}
+	return cmd, nil
+}

utils/subprocess/command_wrapper.go

@@ -20,8 +20,6 @@ import (
 	commandUtils "github.com/ARM-software/golang-utils/utils/subprocess/command"
 )
 
-const subprocessTerminationGracePeriod = 10 * time.Millisecond
-
 // INTERNAL
 // wrapper over an exec cmd.
 type cmdWrapper struct {
@@ -73,7 +71,7 @@ func (c *cmdWrapper) interruptWithContext(ctx context.Context, interrupt proc.In
 	stopErr := atomic.NewError(nil)
 	if subprocess != nil {
 		pid := subprocess.Pid
-		parallelisation.ScheduleAfter(ctx, subprocessTerminationGracePeriod, func(time.Time) {
+		parallelisation.ScheduleAfter(ctx, proc.SubprocessTerminationGracePeriod, func(time.Time) {
 			process, sErr := proc.FindProcess(ctx, pid)
 			if process == nil || sErr != nil {
 				return
@@ -134,12 +132,9 @@ type command struct {
 func (c *command) createCommand(cmdCtx context.Context) *exec.Cmd {
 	newCmd, newArgs := c.as.Redefine(c.cmd, c.args...)
 	cmd := exec.CommandContext(cmdCtx, newCmd, newArgs...) //nolint:gosec
-	cmd.Cancel = func() error {
-		p := cmd.Process
-		if p == nil {
-			return nil
-		}
-		return proc.TerminateGracefully(context.Background(), p.Pid, subprocessTerminationGracePeriod)
+	cancellableCmd, err := proc.DefineCmdCancel(cmd)
+	if err == nil {
+		cmd = cancellableCmd
 	}
 	cmd.Stdout = newOutStreamer(cmdCtx, c.loggers)
 	cmd.Stderr = newErrLogStreamer(cmdCtx, c.loggers)
@@ -209,6 +204,6 @@ func CleanKillOfCommand(ctx context.Context, cmd *exec.Cmd) (err error) {
 	if thisP == nil {
 		return
 	}
-	err = proc.TerminateGracefully(ctx, thisP.Pid, subprocessTerminationGracePeriod)
+	err = proc.TerminateGracefully(ctx, thisP.Pid, proc.SubprocessTerminationGracePeriod)
 	return
 }

utils/subprocess/supervisor/supervisor.go

@@ -2,7 +2,6 @@ package supervisor
 
 import (
 	"context"
-	"fmt"
 	"time"
 
 	"golang.org/x/sync/errgroup"
@@ -113,7 +112,7 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
 					return err
 				}
-				return fmt.Errorf("%w: error running pre-start hook: %v", commonerrors.ErrUnexpected, err.Error())
+				return commonerrors.WrapError(commonerrors.ErrUnexpected, err, "error running pre-start hook")
 			}
 		}
 
@@ -123,10 +122,10 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 			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())
+			return commonerrors.WrapError(commonerrors.ErrUnexpected, err, "error occurred when creating new command")
 		}
 		if s.cmd == nil {
-			return fmt.Errorf("%w: command was undefined", commonerrors.ErrUndefined)
+			return commonerrors.UndefinedVariable("command to be supervised")
 		}
 
 		g.Go(s.cmd.Execute)
@@ -137,7 +136,7 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
 					return err
 				}
-				return fmt.Errorf("%w: error running post-start hook: %v", commonerrors.ErrUnexpected, err.Error())
+				return commonerrors.WrapError(commonerrors.ErrUnexpected, err, "error running post-start hook")
 			}
 		}
 
@@ -149,7 +148,7 @@ func (s *Supervisor) Run(ctx context.Context) (err error) {
 				if commonerrors.Any(err, commonerrors.ErrCancelled, commonerrors.ErrTimeout) {
 					return err
 				}
-				return fmt.Errorf("%w: error running post-stop hook: %v", commonerrors.ErrUnexpected, err.Error())
+				return commonerrors.WrapError(commonerrors.ErrUnexpected, err, "error running post-stop hook")
 			}
 		}