diff --git a/agent/reaper/reaper.go b/agent/reaper/reaper.go index 58e2196dc3..5c27b3d13a 100644 --- a/agent/reaper/reaper.go +++ b/agent/reaper/reaper.go @@ -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 } diff --git a/agent/reaper/reaper_test.go b/agent/reaper/reaper_test.go index f8d4b32a9c..2b5a44a21c 100644 --- a/agent/reaper/reaper_test.go +++ b/agent/reaper/reaper_test.go @@ -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) } diff --git a/agent/reaper/reaper_unix.go b/agent/reaper/reaper_unix.go index dd73fc5537..bd2a8c807d 100644 --- a/agent/reaper/reaper_unix.go +++ b/agent/reaper/reaper_unix.go @@ -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)