fix(testrun): report a sidecar that dies at startup as the exit it was

internal/android/android.go

@@ -221,11 +221,7 @@ func navOverlayCommand(ctx context.Context, adb, serial, overlay string) *exec.C
 // EnvWithAndroidPlatformTools returns env with the directory containing adb
 // prepended to PATH, so child processes (the sidecar) can invoke adb even
 // when the user hasn't set up their shell PATH.
-func EnvWithAndroidPlatformTools(env []string) []string {
-	adb, err := AdbBinary()
-	if err != nil {
-		return env
-	}
+func EnvWithAndroidPlatformTools(env []string, adb string) []string {
 	adbDir := filepath.Dir(adb)
 	result := make([]string, 0, len(env))
 	found := false

internal/testrun/driver.go

@@ -148,10 +148,14 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver
 	if options.Device != "" {
 		sidecarArgs = append(sidecarArgs, "--serial", options.Device)
 	}
+	adbPath, err := android.AdbBinary()
+	if err != nil {
+		return nil, nil, preflightFailure("android", err)
+	}
 	sidecarCommand := exec.CommandContext(ctx, "java", sidecarArgs...)
 	sidecarCommand.Stdout = stdout
 	sidecarCommand.Stderr = stdout
-	sidecarCommand.Env = android.EnvWithAndroidPlatformTools(os.Environ())
+	sidecarCommand.Env = android.EnvWithAndroidPlatformTools(os.Environ(), adbPath)
 	// SIGTERM lets the sidecar's shutdown hook stop the iOS XCTest runner.
 	// SIGKILL skips the hook and orphans an xcodebuild session that later
 	// restarts its runner and hijacks the simulator mid-run.
@@ -162,11 +166,19 @@ 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)
 	}
-	fmt.Fprintf(stdout, "sidecar pid=%d listening on 127.0.0.1:%d\n", sidecarCommand.Process.Pid, sidecarPort)
+	// 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)
+	}()
+	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)
 
-	driverClient, err := driverSidecar.Dial(fmt.Sprintf("127.0.0.1:%d", sidecarPort))
+	driverClient, err := driverSidecar.Dial(address)
 	if err != nil {
-		stopSidecar(sidecarCommand)
+		stopSidecar(sidecarCommand, sidecarExited)
 		return nil, nil, fmt.Errorf("dial sidecar: %w", err)
 	}
 	driverClient.SetPlatform(options.Platform)
@@ -175,22 +187,70 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver
 	// (absorbing the XCUITest startup race) runs inside IosDriverBackend.init
 	// in the sidecar - no additional sleep needed here.
 	healthCtx, healthCancel := context.WithTimeout(ctx, sidecarStartupTimeout)
-	if err := driverClient.WaitForHealth(healthCtx, 250e6); err != nil {
-		healthCancel()
-		stopSidecar(sidecarCommand)
-		_ = driverClient.Close()
-		return nil, nil, fmt.Errorf("sidecar health check: %w", err)
-	}
+	healthErr := awaitSidecar(healthCtx, address, sidecarStartupTimeout, func(pollCtx context.Context) error {
+		return driverClient.WaitForHealth(pollCtx, 250e6)
+	}, sidecarExited)
 	healthCancel()
+	if healthErr != nil {
+		stopSidecar(sidecarCommand, sidecarExited)
+		_ = driverClient.Close()
+		return nil, nil, healthErr
+	}
 	fmt.Fprintln(stdout, "sidecar is healthy")
 
 	cleanup := func() {
 		_ = driverClient.Close()
-		stopSidecar(sidecarCommand)
+		stopSidecar(sidecarCommand, sidecarExited)
 	}
 	return driverClient, cleanup, nil
 }
 
+// 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
+// failure knows why the sidecar was unhappy, so both name what to look at
+// instead of picking a cause.
+func awaitSidecar(
+	ctx context.Context,
+	address string,
+	timeout time.Duration,
+	health func(context.Context) error,
+	exited <-chan error,
+) error {
+	healthy := make(chan error, 1)
+	go func() { healthy <- health(ctx) }()
+	select {
+	case exitErr := <-exited:
+		return sidecarExitedError(address, exitErr)
+	case err := <-healthy:
+		if err == nil {
+			return nil
+		}
+		select {
+		case exitErr := <-exited:
+			return sidecarExitedError(address, exitErr)
+		default:
+			return fmt.Errorf(
+				"sidecar did not answer a health check on %s within %s and is still running\n%s",
+				address, timeout, sidecarWhatToCheck,
+			)
+		}
+	}
+}
+
+const sidecarWhatToCheck = "check the sidecar output above, then `sanderling doctor --platform=android` (java 17+, adb, Android SDK)"
+
+func sidecarExitedError(address string, exitErr error) error {
+	status := "exit status 0"
+	if exitErr != nil {
+		status = exitErr.Error()
+	}
+	return fmt.Errorf(
+		"sidecar exited before it answered a health check on %s: %s\n%s",
+		address, status, sidecarWhatToCheck,
+	)
+}
+
 // sidecarShutdownGrace bounds how long the sidecar gets to run its shutdown
 // hook (terminate the app, stop the XCTest runner) before being killed.
 const sidecarShutdownGrace = 15 * time.Second
@@ -198,25 +258,20 @@ const sidecarShutdownGrace = 15 * time.Second
 // stopSidecar terminates the sidecar gracefully so its shutdown hook can stop
 // the device-side runner processes, escalating to SIGKILL when it does not
 // exit within the grace window.
-func stopSidecar(sidecarCommand *exec.Cmd) {
+func stopSidecar(sidecarCommand *exec.Cmd, exited <-chan error) {
 	if sidecarCommand.Process == nil {
 		return
 	}
 	if err := sidecarCommand.Process.Signal(syscall.SIGTERM); err != nil {
 		_ = sidecarCommand.Process.Kill()
-		_ = sidecarCommand.Wait()
+		<-exited
 		return
 	}
-	done := make(chan struct{})
-	go func() {
-		_ = sidecarCommand.Wait()
-		close(done)
-	}()
 	select {
-	case <-done:
+	case <-exited:
 	case <-time.After(sidecarShutdownGrace):
 		_ = sidecarCommand.Process.Kill()
-		<-done
+		<-exited
 	}
 }
 

internal/testrun/driver_test.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"errors"
 	"io"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/priyanshujain/sanderling/internal/driver"
 	"github.com/priyanshujain/sanderling/internal/driver/ioscompanion"
@@ -65,6 +67,76 @@ func TestBuildDriverSurfacesDeviceConstructionError(t *testing.T) {
 	}
 }
 
+// A sidecar that dies during startup leaves the health poll with nothing to
+// talk to, and reporting that as a deadline sends the reader after a gRPC
+// timeout instead of the exit that already happened.
+func TestAwaitSidecarReportsTheExitItSaw(t *testing.T) {
+	exited := make(chan error, 1)
+	exited <- errors.New("exit status 1")
+	close(exited)
+
+	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 {
+		t.Fatal("expected an error when the sidecar exits before it is healthy")
+	}
+	for _, want := range []string{
+		"sidecar exited before it answered a health check on 127.0.0.1:54321: exit status 1",
+		"check the sidecar output above",
+		"sanderling doctor --platform=android",
+	} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q missing %q", err, want)
+		}
+	}
+}
+
+func TestAwaitSidecarTimeoutSaysOnlyWhatItObserved(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+	defer cancel()
+
+	err := awaitSidecar(
+		ctx,
+		"127.0.0.1:54321",
+		50*time.Millisecond,
+		func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() },
+		make(chan error, 1),
+	)
+	if err == nil {
+		t.Fatal("expected an error when the sidecar never answers")
+	}
+	for _, want := range []string{
+		"sidecar did not answer a health check on 127.0.0.1:54321 within 50ms and is still running",
+		"check the sidecar output above",
+		"sanderling doctor --platform=android",
+	} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q missing %q", err, want)
+		}
+	}
+	if strings.Contains(err.Error(), "context deadline exceeded") {
+		t.Errorf("error %q must not hand the reader a bare gRPC deadline", err)
+	}
+}
+
+func TestAwaitSidecarHealthyReturnsNil(t *testing.T) {
+	err := awaitSidecar(
+		context.Background(),
+		"127.0.0.1:54321",
+		30*time.Second,
+		func(context.Context) error { return nil },
+		make(chan error, 1),
+	)
+	if err != nil {
+		t.Fatalf("expected a healthy sidecar to pass, got %v", err)
+	}
+}
+
 // 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) {