diff --git a/internal/testrun/driver.go b/internal/testrun/driver.go index 109fd53..e11db44 100644 --- a/internal/testrun/driver.go +++ b/internal/testrun/driver.go @@ -166,13 +166,7 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver if err := sidecarCommand.Start(); err != nil { return nil, nil, fmt.Errorf("spawn sidecar: %w", err) } - // The close after the send lets the cleanup path receive again once the - // startup path has already taken the exit status. - sidecarExited := make(chan error, 1) - go func() { - sidecarExited <- sidecarCommand.Wait() - close(sidecarExited) - }() + sidecarExited := watchSidecar(sidecarCommand) address := fmt.Sprintf("127.0.0.1:%d", sidecarPort) fmt.Fprintf(stdout, "sidecar pid=%d listening on %s (adb: %s)\n", sidecarCommand.Process.Pid, address, adbPath) @@ -205,6 +199,18 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver return driverClient, cleanup, nil } +// watchSidecar reaps the sidecar and publishes its exit status. The channel is +// closed after the send so the shutdown path can still receive once the startup +// path has taken the status. +func watchSidecar(sidecarCommand *exec.Cmd) <-chan error { + exited := make(chan error, 1) + go func() { + exited <- sidecarCommand.Wait() + close(exited) + }() + return exited +} + // awaitSidecar waits for the sidecar to answer a health check, racing that // against the process exiting so a sidecar that dies during startup is reported // as the exit it was rather than as a deadline half a minute later. Neither diff --git a/internal/testrun/driver_test.go b/internal/testrun/driver_test.go index 35b2a04..44f63c2 100644 --- a/internal/testrun/driver_test.go +++ b/internal/testrun/driver_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "os/exec" "strings" "testing" "time" @@ -137,6 +138,60 @@ func TestAwaitSidecarHealthyReturnsNil(t *testing.T) { } } +func TestStopSidecarTerminatesARunningSidecar(t *testing.T) { + command := exec.Command("sleep", "60") + if err := command.Start(); err != nil { + t.Fatalf("start: %v", err) + } + exited := watchSidecar(command) + + stopped := make(chan struct{}) + go func() { + stopSidecar(command, exited) + close(stopped) + }() + select { + case <-stopped: + case <-time.After(10 * time.Second): + t.Fatal("stopSidecar never returned for a running sidecar") + } + if got := command.ProcessState.String(); got != "signal: terminated" { + t.Errorf("sidecar ended as %q, want the SIGTERM its shutdown hook needs", got) + } +} + +// The startup path takes the exit status to report it, so the shutdown path +// that follows must not sit waiting for a status nobody will send again. +func TestStopSidecarAfterTheStartupPathTookTheExitStatus(t *testing.T) { + command := exec.Command("sh", "-c", "exit 3") + if err := command.Start(); err != nil { + t.Fatalf("start: %v", err) + } + exited := watchSidecar(command) + + err := awaitSidecar( + context.Background(), + "127.0.0.1:54321", + 30*time.Second, + func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() }, + exited, + ) + if err == nil || !strings.Contains(err.Error(), "exit status 3") { + t.Fatalf("expected the sidecar's real exit status, got %v", err) + } + + stopped := make(chan struct{}) + go func() { + stopSidecar(command, exited) + close(stopped) + }() + select { + case <-stopped: + case <-time.After(10 * time.Second): + t.Fatal("stopSidecar blocked on an exit status the startup path had already taken") + } +} + // stubPreflight bypasses the host-readiness checks so routing tests exercise // driver construction on a Linux CI runner that lacks xcrun/java. func stubPreflight(t *testing.T) {