From 47540241707e9957fcae64a99455f4886be689f4 Mon Sep 17 00:00:00 2001
From: Samantha <samantha.loop@samsara.com>
Date: Tue, 1 Apr 2025 05:01:59 +0000
Subject: [PATCH] runner: improve error logging

This clarifies some error messages, and provides the stack trace when taskrunner fails on execution.
---
 executor.go         |  8 ++++--
 executor_test.go    | 61 +++++++++++++++++++++++++++++++++++++++++++++
 runner.go           |  7 ++++--
 shell/shell.go      |  3 ++-
 shell/shell_test.go | 17 +++++++++++++
 5 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/executor.go b/executor.go
index 9c6772c..2d71802 100644
--- a/executor.go
+++ b/executor.go
@@ -314,7 +314,11 @@ func (e *Executor) ShellRun(ctx context.Context, command string, opts ...shell.R
 	}
 	options = append(options, e.shellRunOptions...)
 	options = append(options, opts...)
-	return shell.Run(ctx, command, options...)
+	err := shell.Run(ctx, command, options...)
+	if err != nil {
+		return oops.Wrapf(err, "Executor failed to run shell command")
+	}
+	return nil
 }
 
 func (e *Executor) taskExecution(t *Task) *taskExecution { return e.tasks[t] }
@@ -685,7 +689,7 @@ func (e *Executor) runPass() {
 
 					e.runPass()
 					if err != nil && ctx.Err() != context.Canceled {
-						return err
+						return oops.Wrapf(err, "Executor failed to run task")
 					}
 
 					return nil
diff --git a/executor_test.go b/executor_test.go
index 1c4f79d..9b39e7a 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -2,6 +2,7 @@ package taskrunner_test
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -194,3 +195,63 @@ func TestExecutorInvalidations(t *testing.T) {
 		mockB.Reset()
 	}
 }
+
+func TestExecutorErrorHandling(t *testing.T) {
+	config := &config.Config{}
+
+	for _, testcase := range []struct {
+		Name string
+		Test func(t *testing.T)
+	}{
+		{
+			"shell command error",
+			func(t *testing.T) {
+				executor := taskrunner.NewExecutor(config, []*taskrunner.Task{
+					{
+						Name: "shell-error",
+						Run: func(ctx context.Context, shellRun shell.ShellRun) error {
+							return shellRun(ctx, "invalid_command_that_will_fail")
+						},
+					},
+				})
+
+				events := executor.Subscribe()
+				go func() {
+					event := consumeUntil(t, events, taskrunner.ExecutorEventKind_TaskFailed)
+					assert.Contains(t, event.(*taskrunner.TaskFailedEvent).Error.Error(),
+						"Executor failed to run shell command")
+				}()
+
+				err := executor.Run(context.Background(), []string{"shell-error"}, &taskrunner.Runtime{})
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), "Executor failed to run task")
+			},
+		},
+		{
+			"task execution error",
+			func(t *testing.T) {
+				executor := taskrunner.NewExecutor(config, []*taskrunner.Task{
+					{
+						Name: "failing-task",
+						Run: func(ctx context.Context, shellRun shell.ShellRun) error {
+							return errors.New("task failed")
+						},
+					},
+				})
+
+				events := executor.Subscribe()
+				go func() {
+					event := consumeUntil(t, events, taskrunner.ExecutorEventKind_TaskFailed)
+					assert.Contains(t, event.(*taskrunner.TaskFailedEvent).Error.Error(),
+						"task failed")
+				}()
+
+				err := executor.Run(context.Background(), []string{"failing-task"}, &taskrunner.Runtime{})
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), "Executor failed to run task")
+			},
+		},
+	} {
+		t.Run(testcase.Name, testcase.Test)
+	}
+}
diff --git a/runner.go b/runner.go
index 338cdcd..dfa95ac 100644
--- a/runner.go
+++ b/runner.go
@@ -253,7 +253,10 @@ func Run(options ...RunOption) {
 		sub := runtime.subscriptions[i]
 		ch := executor.Subscribe()
 		g.Go(func() error {
-			return sub(ch)
+			if err := sub(ch); err != nil {
+				return oops.Wrapf(err, "subscription error")
+			}
+			return nil
 		})
 	}
 
@@ -264,7 +267,7 @@ func Run(options ...RunOption) {
 		// if it's a well-known executor error, or the underlying task failed AND
 		// we're not in watch mode.
 		if oops.Cause(err) == errUndefinedTaskName || !watchMode {
-			return err
+			return oops.Wrapf(err, "running executor")
 		}
 
 		return nil
diff --git a/shell/shell.go b/shell/shell.go
index 8835eb3..cbf6b2e 100644
--- a/shell/shell.go
+++ b/shell/shell.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"strings"
 
+	"github.com/samsarahq/go/oops"
 	"mvdan.cc/sh/interp"
 	"mvdan.cc/sh/syntax"
 )
@@ -48,7 +49,7 @@ func Env(vars map[string]string) RunOption {
 func Run(ctx context.Context, command string, opts ...RunOption) error {
 	p, err := syntax.NewParser().Parse(strings.NewReader(command), "")
 	if err != nil {
-		return err
+		return oops.Wrapf(err, "failed to parse shell command")
 	}
 
 	r, err := interp.New()
diff --git a/shell/shell_test.go b/shell/shell_test.go
index b993f20..b09e1c7 100644
--- a/shell/shell_test.go
+++ b/shell/shell_test.go
@@ -13,3 +13,20 @@ func TestRunShell(t *testing.T) {
 	assert.NoError(t, Run(context.Background(), "echo 1", Stdout(&buffer)))
 	assert.Equal(t, "1\n", buffer.String())
 }
+
+func TestRunShellInterpreterError(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			err, ok := r.(error)
+			assert.True(t, ok)
+			assert.Contains(t, err.Error(), "failed to set up interpreter")
+		}
+	}()
+	Run(context.Background(), "")
+}
+
+func TestRunShellParseError(t *testing.T) {
+	err := Run(context.Background(), "`invalid command")
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to parse shell command")
+}