From 557bd9d8152d40d5c04679c54bbcca4d12f02f3b Mon Sep 17 00:00:00 2001 From: PJ Date: Wed, 12 Aug 2026 21:38:09 +0530 Subject: [PATCH] fix(chrome): honor the caller context in Launch Launch and clearState ran against d.tabCtx, so a target that accepts the connection and never answers wedged the process past its own --duration and through SIGTERM, needing SIGKILL. Unattended that is a campaign worker lost for the rest of the sweep with no diagnostic. The browser is still allocated against d.tabCtx first, because chromedp starts Chrome under whichever context calls Run first and allocating under a caller deadline would kill the browser when Launch returns. Everything after allocation goes through runCtx. Claude-Session: https://claude.ai/code/session_01A5KmftdEJ49A9z5mF5ESrX --- internal/driver/chrome/driver.go | 28 +++++++--- internal/driver/chrome/driver_test.go | 76 +++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 8 deletions(-) diff --git a/internal/driver/chrome/driver.go b/internal/driver/chrome/driver.go index 4914672..3137afd 100644 --- a/internal/driver/chrome/driver.go +++ b/internal/driver/chrome/driver.go @@ -97,19 +97,31 @@ func New() *Driver { } func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ map[string]string) error { + // Allocate the browser against the driver's own context before anything + // caller-bound runs. chromedp starts Chrome under whichever context first + // calls Run, so allocating under a caller deadline would tie the browser + // process to this one call and kill it the moment Launch returns. + if err := chromedp.Run(d.tabCtx); err != nil { + return err + } + // Everything after allocation goes through runCtx, so a caller deadline or + // a SIGTERM aborts a launch that would otherwise wait forever on a target + // that accepts the connection and never answers. + runCtx, cancel := d.runCtx(ctx) + defer cancel() if clearState { - if err := d.clearState(bundleID); err != nil { + if err := d.clearState(runCtx, bundleID); err != nil { return err } } - if err := chromedp.Run(d.tabCtx, chromedp.Navigate(bundleID)); err != nil { + if err := chromedp.Run(runCtx, chromedp.Navigate(bundleID)); err != nil { return err } // After navigation, read CSS custom properties --frame-w / --frame-h (common // mobile-frame convention) so screenshots fit the app without grey borders. // Falls back to the body scroll dimensions if the properties are absent. var dims [2]int64 - if err := chromedp.Run(d.tabCtx, chromedp.Evaluate(` + if err := chromedp.Run(runCtx, chromedp.Evaluate(` (function() { const s = getComputedStyle(document.documentElement); const pw = parseInt(s.getPropertyValue('--frame-w'), 10); @@ -118,7 +130,7 @@ func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ const h = isNaN(ph) ? document.body.scrollHeight : ph; return [w, h]; })()`, &dims)); err == nil && dims[0] > 0 && dims[1] > 0 { - _ = chromedp.Run(d.tabCtx, chromedp.EmulateViewport(dims[0], dims[1])) + _ = chromedp.Run(runCtx, chromedp.EmulateViewport(dims[0], dims[1])) } return nil } @@ -130,8 +142,8 @@ func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ // which needs no navigation. sessionStorage is per-tab and outside that // domain's reach; it only survives when a relaunch reuses a tab already on // the target origin, which is the one case where script can reach it. -func (d *Driver) clearState(bundleID string) error { - if err := chromedp.Run(d.tabCtx, network.ClearBrowserCookies()); err != nil { +func (d *Driver) clearState(runCtx context.Context, bundleID string) error { + if err := chromedp.Run(runCtx, network.ClearBrowserCookies()); err != nil { return fmt.Errorf("clear cookies: %w", err) } origin := securityOrigin(bundleID) @@ -139,12 +151,12 @@ func (d *Driver) clearState(bundleID string) error { return nil } clearForOrigin := storage.ClearDataForOrigin(origin, string(storage.TypeAll)) - if err := chromedp.Run(d.tabCtx, clearForOrigin); err != nil { + if err := chromedp.Run(runCtx, clearForOrigin); err != nil { return fmt.Errorf("clear storage for %s: %w", origin, err) } script := fmt.Sprintf( `location.origin === %q && (sessionStorage.clear(), true)`, origin) - return chromedp.Run(d.tabCtx, chromedp.ActionFunc(func(ctx context.Context) error { + return chromedp.Run(runCtx, chromedp.ActionFunc(func(ctx context.Context) error { _, exception, err := runtime.Evaluate(script).Do(ctx) if err != nil { return fmt.Errorf("clear session storage: %w", err) diff --git a/internal/driver/chrome/driver_test.go b/internal/driver/chrome/driver_test.go index 7f21087..d561f6f 100644 --- a/internal/driver/chrome/driver_test.go +++ b/internal/driver/chrome/driver_test.go @@ -6,6 +6,7 @@ import ( "context" "encoding/json" "errors" + "net" "net/http" "net/http/httptest" "testing" @@ -398,3 +399,78 @@ func TestHierarchy_RootsAtDocumentElementWithoutHead(t *testing.T) { } } } + +// TestLaunch_HonorsCallerDeadline points the driver at a listener that accepts +// the connection and never answers. Chrome has no page-load deadline of its +// own, so a Launch that ignored its caller context waited forever: an +// unattended campaign worker aimed at an unreachable target wedged with no +// diagnostic and did not even answer SIGTERM. +func TestLaunch_HonorsCallerDeadline(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer listener.Close() + held := make(chan net.Conn, 8) + go func() { + for { + conn, err := listener.Accept() + if err != nil { + return + } + held <- conn + } + }() + defer func() { + close(held) + for conn := range held { + conn.Close() + } + }() + + d := New() + defer d.Terminate(context.Background()) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + done := make(chan error, 1) + go func() { done <- d.Launch(ctx, "http://"+listener.Addr().String(), false, nil) }() + select { + case err := <-done: + if err == nil { + t.Fatal("Launch returned nil against a target that never answers") + } + if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, context.Canceled) { + t.Fatalf("Launch error = %v, want a context error", err) + } + case <-time.After(45 * time.Second): + t.Fatal("Launch ignored the caller deadline and blocked") + } +} + +// TestLaunch_KeepsBrowserAliveAfterCallerContextEnds guards the trap that made +// Launch use the driver context in the first place: chromedp starts Chrome +// under whichever context runs first, so allocating under the caller's +// short-lived context would kill the browser as soon as Launch returned. +func TestLaunch_KeepsBrowserAliveAfterCallerContextEnds(t *testing.T) { + d := New() + defer d.Terminate(context.Background()) + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + if err := d.Launch(ctx, "data:text/html,alive", false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + cancel() + + var text string + if err := chromedp.Run(d.tabCtx, + chromedp.Evaluate(`document.body.textContent`, &text)); err != nil { + t.Fatalf("browser died with the caller context: %v", err) + } + if text != "alive" { + t.Errorf("body text = %q, want \"alive\"", text) + } + if err := d.Launch(context.Background(), "data:text/html,again", false, nil); err != nil { + t.Fatalf("second Launch after the first caller context ended: %v", err) + } +}