diff --git a/changes/20250718123400.bugfix b/changes/20250718123400.bugfix new file mode 100644 index 0000000000..870ede1ce0 --- /dev/null +++ b/changes/20250718123400.bugfix @@ -0,0 +1 @@ +:bug: `[subprocess]` Improve the error reporting when a process is cancelled diff --git a/utils/platform/deletion.go b/utils/platform/deletion.go index 0125c7e511..ce6fa7e879 100644 --- a/utils/platform/deletion.go +++ b/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 } } diff --git a/utils/proc/interrupt.go b/utils/proc/interrupt.go index b9dbe1645c..de7cbd73d3 100644 --- a/utils/proc/interrupt.go +++ b/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 +} diff --git a/utils/subprocess/command_wrapper.go b/utils/subprocess/command_wrapper.go index 0c4db74e36..c935bdb139 100644 --- a/utils/subprocess/command_wrapper.go +++ b/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 } diff --git a/utils/subprocess/supervisor/supervisor.go b/utils/subprocess/supervisor/supervisor.go index da67a7c1a4..41ebb3ceef 100644 --- a/utils/subprocess/supervisor/supervisor.go +++ b/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") } }