fix(agent/reaper): run reaper tests in isolated subprocesses (#22894)

Tests that call ForkReap or send signals to their own process now re-exec as isolated subprocesses. This prevents ForkReap's syscall.ForkExec and process-directed signals from interfering with the parent test binary or other tests running in parallel. Also: - Wait for the reaper goroutine to fully exit between subtests to prevent overlapping reapers from competing on Wait4(-1). - Register signal handlers synchronously before spawning the forwarding goroutine so no signal is lost between ForkExec and the handler being ready.
2026-09-22 05:05:20 +08:00 · 2026-03-13 19:33:02 +02:00
parent c3b6284955
commit efe114119f
3 changed files with 150 additions and 61 deletions
@@ -2,6 +2,7 @@ package reaper

 import (
 	"os"
+	"sync"

 	"github.com/hashicorp/go-reap"

@@ -42,20 +43,42 @@ func WithLogger(logger slog.Logger) Option {
 	}
 }

-// WithDone sets a channel that, when closed, stops the reaper
+// WithReaperStop sets a channel that, when closed, stops the reaper
 // goroutine. Callers that invoke ForkReap more than once in the
 // same process (e.g. tests) should use this to prevent goroutine
 // accumulation.
-func WithDone(ch chan struct{}) Option {
+func WithReaperStop(ch chan struct{}) Option {
 	return func(o *options) {
-		o.Done = ch
+		o.ReaperStop = ch
+	}
+}
+
+// WithReaperStopped sets a channel that is closed after the
+// reaper goroutine has fully exited.
+func WithReaperStopped(ch chan struct{}) Option {
+	return func(o *options) {
+		o.ReaperStopped = ch
+	}
+}
+
+// WithReapLock sets a mutex shared between the reaper and Wait4.
+// The reaper holds the write lock while reaping, and ForkReap
+// holds the read lock during Wait4, preventing the reaper from
+// stealing the child's exit status. This is only needed for
+// tests with instant-exit children where the race window is
+// large.
+func WithReapLock(mu *sync.RWMutex) Option {
+	return func(o *options) {
+		o.ReapLock = mu
 	}
 }

 type options struct {
-	ExecArgs     []string
-	PIDs         reap.PidCh
-	CatchSignals []os.Signal
-	Logger       slog.Logger
-	Done         chan struct{}
+	ExecArgs      []string
+	PIDs          reap.PidCh
+	CatchSignals  []os.Signal
+	Logger        slog.Logger
+	ReaperStop    chan struct{}
+	ReaperStopped chan struct{}
+	ReapLock      *sync.RWMutex
 }
@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/exec"
 	"os/signal"
+	"sync"
 	"syscall"
 	"testing"
 	"time"
@@ -18,35 +19,79 @@ import (
 	"github.com/coder/coder/v2/testutil"
 )

-// withDone returns an option that stops the reaper goroutine when t
-// completes, preventing goroutine accumulation across subtests.
-func withDone(t *testing.T) reaper.Option {
+// subprocessEnvKey is set when a test re-execs itself as an
+// isolated subprocess. Tests that call ForkReap or send signals
+// to their own process check this to decide whether to run real
+// test logic or launch the subprocess and wait for it.
+const subprocessEnvKey = "CODER_REAPER_TEST_SUBPROCESS"
+
+// runSubprocess re-execs the current test binary in a new process
+// running only the named test. This isolates ForkReap's
+// syscall.ForkExec and any process-directed signals (e.g. SIGINT)
+// from the parent test binary, making these tests safe to run in
+// CI and alongside other tests.
+//
+// Returns true inside the subprocess (caller should proceed with
+// the real test logic). Returns false in the parent after the
+// subprocess exits successfully (caller should return).
+func runSubprocess(t *testing.T) bool {
 	t.Helper()
-	done := make(chan struct{})
-	t.Cleanup(func() { close(done) })
-	return reaper.WithDone(done)
+
+	if os.Getenv(subprocessEnvKey) == "1" {
+		return true
+	}
+
+	ctx := testutil.Context(t, testutil.WaitMedium)
+
+	//nolint:gosec // Test-controlled arguments.
+	cmd := exec.CommandContext(ctx, os.Args[0],
+		"-test.run=^"+t.Name()+"$",
+		"-test.v",
+	)
+	cmd.Env = append(os.Environ(), subprocessEnvKey+"=1")
+
+	out, err := cmd.CombinedOutput()
+	t.Logf("Subprocess output:\n%s", out)
+	require.NoError(t, err, "subprocess failed")
+
+	return false
 }

-// TestReap checks that's the reaper is successfully reaping
-// exited processes and passing the PIDs through the shared
-// channel.
-//
-//nolint:paralleltest
+// withDone returns options that stop the reaper goroutine when t
+// completes and wait for it to fully exit, preventing
+// overlapping reapers across sequential subtests.
+func withDone(t *testing.T) []reaper.Option {
+	t.Helper()
+	stop := make(chan struct{})
+	stopped := make(chan struct{})
+	t.Cleanup(func() {
+		close(stop)
+		<-stopped
+	})
+	return []reaper.Option{
+		reaper.WithReaperStop(stop),
+		reaper.WithReaperStopped(stopped),
+	}
+}
+
+// TestReap checks that the reaper successfully reaps exited
+// processes and passes their PIDs through the shared channel.
 func TestReap(t *testing.T) {
-	// Don't run the reaper test in CI. It does weird
-	// things like forkexecing which may have unintended
-	// consequences in CI.
-	if testutil.InCI() {
-		t.Skip("Detected CI, skipping reaper tests")
+	t.Parallel()
+	if !runSubprocess(t) {
+		return
 	}

 	pids := make(reap.PidCh, 1)
-	exitCode, err := reaper.ForkReap(
+	var reapLock sync.RWMutex
+	opts := append([]reaper.Option{
 		reaper.WithPIDCallback(pids),
-		// Provide some argument that immediately exits.
 		reaper.WithExecArgs("/bin/sh", "-c", "exit 0"),
-		withDone(t),
-	)
+		reaper.WithReapLock(&reapLock),
+	}, withDone(t)...)
+	reapLock.RLock()
+	exitCode, err := reaper.ForkReap(opts...)
+	reapLock.RUnlock()
 	require.NoError(t, err)
 	require.Equal(t, 0, exitCode)

@@ -66,7 +111,7 @@ func TestReap(t *testing.T) {

 	expectedPIDs := []int{cmd.Process.Pid, cmd2.Process.Pid}

-	for i := 0; i < len(expectedPIDs); i++ {
+	for range len(expectedPIDs) {
 		select {
 		case <-time.After(testutil.WaitShort):
 			t.Fatalf("Timed out waiting for process")
@@ -76,10 +121,11 @@ func TestReap(t *testing.T) {
 	}
 }

-//nolint:paralleltest
+//nolint:tparallel // Subtests must be sequential, each starts its own reaper.
 func TestForkReapExitCodes(t *testing.T) {
-	if testutil.InCI() {
-		t.Skip("Detected CI, skipping reaper tests")
+	t.Parallel()
+	if !runSubprocess(t) {
+		return
 	}

 	tests := []struct {
@@ -95,25 +141,31 @@ func TestForkReapExitCodes(t *testing.T) {
 		{"SIGTERM", "kill -15 $$", 128 + 15},
 	}

+	//nolint:paralleltest // Subtests must be sequential, each starts its own reaper.
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			exitCode, err := reaper.ForkReap(
+			var reapLock sync.RWMutex
+			opts := append([]reaper.Option{
 				reaper.WithExecArgs("/bin/sh", "-c", tt.command),
-				withDone(t),
-			)
+				reaper.WithReapLock(&reapLock),
+			}, withDone(t)...)
+			reapLock.RLock()
+			exitCode, err := reaper.ForkReap(opts...)
+			reapLock.RUnlock()
 			require.NoError(t, err)
 			require.Equal(t, tt.expectedCode, exitCode, "exit code mismatch for %q", tt.command)
 		})
 	}
 }

-//nolint:paralleltest // Signal handling.
+// TestReapInterrupt verifies that ForkReap forwards caught signals
+// to the child process. The test sends SIGINT to its own process
+// and checks that the child receives it. Running in a subprocess
+// ensures SIGINT cannot kill the parent test binary.
 func TestReapInterrupt(t *testing.T) {
-	// Don't run the reaper test in CI. It does weird
-	// things like forkexecing which may have unintended
-	// consequences in CI.
-	if testutil.InCI() {
-		t.Skip("Detected CI, skipping reaper tests")
+	t.Parallel()
+	if !runSubprocess(t) {
+		return
 	}

 	errC := make(chan error, 1)
@@ -126,24 +178,28 @@ func TestReapInterrupt(t *testing.T) {
 	defer signal.Stop(usrSig)

 	go func() {
-		exitCode, err := reaper.ForkReap(
+		opts := append([]reaper.Option{
 			reaper.WithPIDCallback(pids),
 			reaper.WithCatchSignals(os.Interrupt),
-			withDone(t),
 			// Signal propagation does not extend to children of children, so
 			// we create a little bash script to ensure sleep is interrupted.
-			reaper.WithExecArgs("/bin/sh", "-c", fmt.Sprintf("pid=0; trap 'kill -USR2 %d; kill -TERM $pid' INT; sleep 10 &\npid=$!; kill -USR1 %d; wait", os.Getpid(), os.Getpid())),
-		)
+			reaper.WithExecArgs("/bin/sh", "-c", fmt.Sprintf(
+				"pid=0; trap 'kill -USR2 %d; kill -TERM $pid' INT; sleep 10 &\npid=$!; kill -USR1 %d; wait",
+				os.Getpid(), os.Getpid(),
+			)),
+		}, withDone(t)...)
+		exitCode, err := reaper.ForkReap(opts...)
 		// The child exits with 128 + SIGTERM (15) = 143, but the trap catches
 		// SIGINT and sends SIGTERM to the sleep process, so exit code varies.
 		_ = exitCode
 		errC <- err
 	}()

-	require.Equal(t, <-usrSig, syscall.SIGUSR1)
+	require.Equal(t, syscall.SIGUSR1, <-usrSig)
+
 	err := syscall.Kill(os.Getpid(), syscall.SIGINT)
 	require.NoError(t, err)
-	require.Equal(t, <-usrSig, syscall.SIGUSR2)

+	require.Equal(t, syscall.SIGUSR2, <-usrSig)
 	require.NoError(t, <-errC)
 }
@@ -19,31 +19,36 @@ func IsInitProcess() bool {
 	return os.Getpid() == 1
 }

-func catchSignals(logger slog.Logger, pid int, sigs []os.Signal) {
+// startSignalForwarding registers signal handlers synchronously
+// then forwards caught signals to the child in a background
+// goroutine. Registering before the goroutine starts ensures no
+// signal is lost between ForkExec and the handler being ready.
+func startSignalForwarding(logger slog.Logger, pid int, sigs []os.Signal) {
 	if len(sigs) == 0 {
 		return
 	}

 	sc := make(chan os.Signal, 1)
 	signal.Notify(sc, sigs...)
-	defer signal.Stop(sc)

 	logger.Info(context.Background(), "reaper catching signals",
 		slog.F("signals", sigs),
 		slog.F("child_pid", pid),
 	)

-	for {
-		s := <-sc
-		sig, ok := s.(syscall.Signal)
-		if ok {
-			logger.Info(context.Background(), "reaper caught signal, killing child process",
-				slog.F("signal", sig.String()),
-				slog.F("child_pid", pid),
-			)
-			_ = syscall.Kill(pid, sig)
+	go func() {
+		defer signal.Stop(sc)
+		for s := range sc {
+			sig, ok := s.(syscall.Signal)
+			if ok {
+				logger.Info(context.Background(), "reaper caught signal, killing child process",
+					slog.F("signal", sig.String()),
+					slog.F("child_pid", pid),
+				)
+				_ = syscall.Kill(pid, sig)
+			}
 		}
-	}
+	}()
 }

 // ForkReap spawns a goroutine that reaps children. In order to avoid
@@ -64,7 +69,12 @@ func ForkReap(opt ...Option) (int, error) {
 		o(opts)
 	}

-	go reap.ReapChildren(opts.PIDs, nil, opts.Done, nil)
+	go func() {
+		reap.ReapChildren(opts.PIDs, nil, opts.ReaperStop, opts.ReapLock)
+		if opts.ReaperStopped != nil {
+			close(opts.ReaperStopped)
+		}
+	}()

 	pwd, err := os.Getwd()
 	if err != nil {
@@ -90,7 +100,7 @@ func ForkReap(opt ...Option) (int, error) {
 		return 1, xerrors.Errorf("fork exec: %w", err)
 	}

-	go catchSignals(opts.Logger, pid, opts.CatchSignals)
+	startSignalForwarding(opts.Logger, pid, opts.CatchSignals)

 	var wstatus syscall.WaitStatus
 	_, err = syscall.Wait4(pid, &wstatus, 0, nil)