diff --git a/internal/android/android.go b/internal/android/android.go index 17470ac..5983056 100644 --- a/internal/android/android.go +++ b/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 diff --git a/internal/testrun/driver.go b/internal/testrun/driver.go index a46a857..109fd53 100644 --- a/internal/testrun/driver.go +++ b/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 } } diff --git a/internal/testrun/driver_test.go b/internal/testrun/driver_test.go index 5e461b3..35b2a04 100644 --- a/internal/testrun/driver_test.go +++ b/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) {