Merge cockroachdb#156951

156951: roachtest: task logger creation error handling r=DarrylWong,srosenberg a=herkolategan

Previously, if a task failed to create a logger internally during the call to create a goroutine, the error would only be returned if the task (or task group) was waited on with `WaitE`. This could lead to a race condition in tests, since it's expected that the function passed to the `task.Go` call is executed. And if something within the test is waiting on results from that goroutine, it could end up waiting indefinitely.

This change ensures the function passed to a `task.Go` call is always executed. The only way this could not happen currently was if an error occurred while trying to create a logger for the task. This has been updated to rather fall back to the root logger; and print an error to the root logger.

Informs: cockroachdb#156635
Epic: None

Co-authored-by: Herko Lategan <[email protected]>

Author: craig[bot]

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

@@ -205,7 +205,8 @@ func (t *group) GoWithCancel(fn Func, opts ...Option) context.CancelFunc {
 	t.ctxGroup.Go(func() error {
 		l, err := opt.L(opt.Name)
 		if err != nil {
-			return err
+			t.manager.logger.Errorf("WARN: defaulting to root logger after failing to create logger for task %q: %v", opt.Name, err)
+			l = t.manager.logger
 		}
 		err = internalFunc(l)
 		event := Event{

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

@@ -6,6 +6,7 @@
 package task
 
 import (
+	"bytes"
 	"context"
 	"io"
 	"sync"
@@ -38,6 +39,39 @@ func TestPanicHandler(t *testing.T) {
 	m.Terminate(nilLogger())
 }
 
+func TestLoggerFallback(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	stdoutBuf := &bytes.Buffer{}
+	stderrBuf := &bytes.Buffer{}
+	loggerConf := logger.Config{
+		Stdout: stdoutBuf,
+		Stderr: stderrBuf,
+	}
+	rootLogger, err := loggerConf.NewLogger("" /* path */)
+	if err != nil {
+		panic(err)
+	}
+
+	ctx := context.Background()
+	m := NewManager(ctx, rootLogger)
+
+	brokenLoggerSupplierFunc := func(name string) (*logger.Logger, error) {
+		return nil, errors.New("logger error")
+	}
+
+	m.Go(func(ctx context.Context, l *logger.Logger) error {
+		l.Printf("this should be logged to the root logger")
+		return nil
+	}, LoggerFunc(brokenLoggerSupplierFunc), Name("def"))
+
+	e := <-m.CompletedEvents()
+	require.Equal(t, "def", e.Name)
+	require.Contains(t, stderrBuf.String(), "logger error")
+	require.Contains(t, stdoutBuf.String(), "this should be logged to the root logger")
+	m.Terminate(nilLogger())
+}
+
 func TestErrorHandler(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()