From 629f8310647c523c9c5b1b9c98f31aa457f2751e Mon Sep 17 00:00:00 2001
From: Herko Lategan <herko@cockroachlabs.com>
Date: Wed, 5 Nov 2025 17:27:53 +0000
Subject: [PATCH] roachtest: task logger creation error handling

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: #156635
Epic: None
---
 .../roachtest/roachtestutil/task/manager.go   |  3 +-
 .../roachtestutil/task/manager_test.go        | 34 +++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/pkg/cmd/roachtest/roachtestutil/task/manager.go b/pkg/cmd/roachtest/roachtestutil/task/manager.go
index bf96325c5c2d..73c148ec00dc 100644
--- a/pkg/cmd/roachtest/roachtestutil/task/manager.go
+++ b/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{
diff --git a/pkg/cmd/roachtest/roachtestutil/task/manager_test.go b/pkg/cmd/roachtest/roachtestutil/task/manager_test.go
index 4d6a739df496..3f40c3e52bb5 100644
--- a/pkg/cmd/roachtest/roachtestutil/task/manager_test.go
+++ b/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()