Handle shutdown goroutine panic recovery differently

ee/errgroup/errgroup.go

@@ -119,29 +119,48 @@ func (l *LoggedErrgroup) StartRepeatedGoroutine(ctx context.Context, goroutineNa
 // AddShutdownGoroutine adds the given goroutine to the errgroup, ensuring that we log its start and exit.
 // The goroutine will not execute until the errgroup has received a signal to exit.
 func (l *LoggedErrgroup) AddShutdownGoroutine(ctx context.Context, goroutineName string, goroutine func() error) {
-	l.StartGoroutine(ctx, goroutineName, func() error {
+	l.errgroup.Go(func() error {
 		slogger := l.slogger.With("goroutine_name", goroutineName)
 
+		// Catch any panicking goroutines and log them. We do not want to return
+		// the error from this routine, as we do for StartGoroutine and StartRepeatedGoroutine --
+		// shutdown goroutines should not return an error besides the errgroup's initial error.
+		defer func() {
+			if r := recover(); r != nil {
+				slogger.Log(ctx, slog.LevelError,
+					"panic occurred in shutdown goroutine",
+					"err", r,
+				)
+				if err, ok := r.(error); ok {
+					slogger.Log(ctx, slog.LevelError,
+						"panic stack trace",
+						"stack_trace", fmt.Sprintf("%+v", errors.WithStack(err)),
+					)
+				}
+			}
+		}()
+
 		// Wait for errgroup to exit
 		<-l.doneCtx.Done()
 
+		slogger.Log(ctx, slog.LevelInfo,
+			"starting shutdown goroutine in errgroup",
+		)
+
 		goroutineStart := time.Now()
 		err := goroutine()
 		elapsedTime := time.Since(goroutineStart)
 
-		// Log anything amiss about the shutdown goroutine -- did it return an error? Did it take too long?
-		if err != nil {
-			slogger.Log(ctx, slog.LevelWarn,
-				"shutdown routine returned err",
-				"goroutine_run_time", elapsedTime.String(),
-				"goroutine_err", err,
-			)
-		} else if elapsedTime > maxShutdownGoroutineDuration {
-			slogger.Log(ctx, slog.LevelWarn,
-				"noticed slow shutdown routine",
-				"goroutine_run_time", elapsedTime.String(),
-			)
+		logLevel := slog.LevelInfo
+		if elapsedTime > maxShutdownGoroutineDuration || err != nil {
+			logLevel = slog.LevelWarn
 		}
+		slogger.Log(ctx, logLevel,
+			"exiting shutdown goroutine in errgroup",
+			"goroutine_name", goroutineName,
+			"goroutine_run_time", elapsedTime.String(),
+			"goroutine_err", err,
+		)
 
 		// We don't want to actually return the error here, to avoid causing an otherwise successful call
 		// to `Shutdown` => `Wait` to return an error. Shutdown routine errors don't matter for the success

ee/errgroup/errgroup_test.go

@@ -102,7 +102,7 @@ func TestShutdown(t *testing.T) {
 	require.True(t, canceled, "errgroup did not exit")
 }
 
-func Test_HandlesPanic(t *testing.T) {
+func TestStartGoroutine_HandlesPanic(t *testing.T) {
 	t.Parallel()
 
 	ctx, cancel := context.WithCancel(context.TODO())
@@ -116,8 +116,36 @@ func Test_HandlesPanic(t *testing.T) {
 		return nil
 	})
 
-	// We expect that the errgroup shuts down -- the test should not panic
-	eg.Shutdown()
+	// We expect that the errgroup shuts itself down -- the test should not panic
+	require.Error(t, eg.Wait(), "should have returned error from panicking goroutine")
+	canceled := false
+	select {
+	case <-eg.Exited():
+		canceled = true
+	default:
+	}
+
+	require.True(t, canceled, "errgroup did not exit")
+}
+
+func TestStartRepeatedGoroutine_HandlesPanic(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	eg := NewLoggedErrgroup(ctx, multislogger.NewNopLogger())
+
+	eg.StartRepeatedGoroutine(ctx, "test_goroutine", 100*time.Millisecond, 50*time.Millisecond, func() error {
+		testArr := make([]int, 0)
+		fmt.Println(testArr[2]) // cause out-of-bounds panic
+		return nil
+	})
+
+	// Wait for long enough that the repeated goroutine executes at least once
+	time.Sleep(500 * time.Millisecond)
+
+	// We expect that the errgroup shuts itself down -- the test should not panic
 	require.Error(t, eg.Wait(), "should have returned error from panicking goroutine")
 	canceled := false
 	select {
@@ -128,3 +156,33 @@ func Test_HandlesPanic(t *testing.T) {
 
 	require.True(t, canceled, "errgroup did not exit")
 }
+
+func TestAddShutdownGoroutine_HandlesPanic(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	eg := NewLoggedErrgroup(ctx, multislogger.NewNopLogger())
+
+	eg.AddShutdownGoroutine(ctx, "test_goroutine", func() error {
+		testArr := make([]int, 0)
+		fmt.Println(testArr[2]) // cause out-of-bounds panic
+		return nil
+	})
+
+	// Call shutdown so the shutdown goroutine runs and the errgroup returns.
+	eg.Shutdown()
+
+	// We expect that the errgroup shuts itself down -- the test should not panic.
+	// Since we called `Shutdown`, `Wait` should not return an error.
+	require.Nil(t, eg.Wait(), "should not returned error after call to Shutdown")
+	canceled := false
+	select {
+	case <-eg.Exited():
+		canceled = true
+	default:
+	}
+
+	require.True(t, canceled, "errgroup did not exit")
+}