test(testrun): cover the sidecar shutdown path after an early exit

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

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) {