roachtest: minor task manager refactoring

Implement a few refactoring changes based on recent feedback.
1. Rename the `ExpectedCancel` event field to `TriggeredByTest` as it makes more
sense in the context of the flag that gets set on the event. A comment has been
added to explain the reasoning behind this flag.
2. Update the commentary on task termination to indicate that it does not wait
indefinitely for tasks to stop.
3. Update the test framework panic handler logging to be more inline with how
panics are logged in a test's main goroutine.

Epic: None
Release note: None

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go

@@ -187,7 +187,7 @@ func (tr *testRunner) run() (retErr error) {
 			if event.Err == nil {
 				tr.logger.Printf("background step finished: %s", event.Name)
 				continue
-			} else if event.ExpectedCancel {
+			} else if event.TriggeredByTest {
 				tr.logger.Printf("background step canceled by test: %s", event.Name)
 				continue
 			}

pkg/cmd/roachtest/roachtestutil/task/manager.go

@@ -28,9 +28,9 @@ type (
 
 	// Event represents the result of a task execution.
 	Event struct {
-		Name           string
-		Err            error
-		ExpectedCancel bool
+		Name            string
+		Err             error
+		TriggeredByTest bool
 	}
 
 	manager struct {
@@ -59,7 +59,7 @@ func NewManager(ctx context.Context, l *logger.Logger) Manager {
 func (m *manager) defaultOptions() []Option {
 	// The default panic handler simply returns the panic as an error.
 	defaultPanicHandlerFn := func(_ context.Context, name string, l *logger.Logger, r interface{}) error {
-		return r.(error)
+		return fmt.Errorf("panic: %v", r)
 	}
 	// The default error handler simply returns the error as is.
 	defaultErrorHandlerFn := func(_ context.Context, name string, l *logger.Logger, err error) error {
@@ -98,17 +98,26 @@ func (m *manager) GoWithCancel(fn Func, opts ...Option) context.CancelFunc {
 		}
 		err = internalFunc(l)
 		event := Event{
-			Name:           opt.Name,
-			Err:            err,
-			ExpectedCancel: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(),
+			Name: opt.Name,
+			Err:  err,
+			// TriggeredByTest is set to true if the task was canceled intentionally,
+			// by the test, and we encounter an error. The assumption is that we
+			// expect the error to have been caused by the cancelation, hence the
+			// error above was not caused by a failure. This ensures we don't register
+			// a test failure if the task was meant to be canceled. It's possible that
+			// `expectedContextCancellation` could be set before the context is
+			// canceled, thus we also ensure that the context is canceled.
+			TriggeredByTest: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(),
 		}
-		select {
-		case m.events <- event:
-			// exit goroutine
-		case <-m.ctx.Done():
-			// Parent context already finished, exit goroutine.
+
+		// Do not send the event if the parent context is canceled. The test is
+		// already aware of the cancelation and sending an event would be redundant.
+		// For instance, a call to test.Fatal would already have captured the error
+		// and canceled the context.
+		if IsContextCanceled(m.ctx) {
 			return nil
 		}
+		m.events <- event
 		return err
 	})
 
@@ -129,7 +138,9 @@ func (m *manager) Go(fn Func, opts ...Option) {
 }
 
 // Terminate will call the stop functions for every task started during the
-// test. Returns when all task functions have returned.
+// test. Returns when all task functions have returned, or after a 5-minute
+// timeout, whichever comes first. If the timeout is reached, the function logs
+// a warning message and returns.
 func (m *manager) Terminate(l *logger.Logger) {
 	func() {
 		m.mu.Lock()

pkg/cmd/roachtest/test_impl.go

@@ -12,7 +12,6 @@ import (
 	"math/rand"
 	"os"
 	"regexp"
-	"runtime/debug"
 	"strings"
 	"sync"
 	"time"
@@ -654,8 +653,7 @@ func (t *testImpl) IsBuildVersion(minVersion string) bool {
 }
 
 func panicHandler(_ context.Context, name string, l *logger.Logger, r interface{}) error {
-	l.Printf("panic stack trace in task %s:\n%s", name, string(debug.Stack()))
-	return fmt.Errorf("panic (stack trace above): %v", r)
+	return fmt.Errorf("test task %s panicked: %v", name, r)
 }
 
 // GoWithCancel runs the given function in a goroutine and returns a

pkg/cmd/roachtest/test_runner.go

@@ -1299,7 +1299,7 @@ func (r *testRunner) runTest(
 				if event.Err == nil {
 					t.L().Printf("task finished: %s", event.Name)
 					continue
-				} else if event.ExpectedCancel {
+				} else if event.TriggeredByTest {
 					t.L().Printf("task canceled by test: %s", event.Name)
 					continue
 				}