diff --git a/pkg/tools/browser.go b/pkg/tools/browser.go index 4a092320..a773ea87 100644 --- a/pkg/tools/browser.go +++ b/pkg/tools/browser.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/mafredri/cdp" "github.com/mafredri/cdp/devtool" "github.com/mafredri/cdp/protocol/dom" "github.com/mafredri/cdp/protocol/emulation" @@ -405,21 +406,46 @@ func (b *BrowserTool) navigate(ctx context.Context, params map[string]interface{ return "", fmt.Errorf("failed to navigate: %w", err) } - // Wait for page load - domLoaded, err := client.Page.DOMContentEventFired(ctx) - if err != nil { - b.log.Warn("Failed to wait for DOMContentLoaded", + // Wait for page load using a bounded readyState poll rather than a CDP event + // stream. This avoids CI flakes where the event listener can hang indefinitely. + if err := waitForPageReadyState(ctx, client, b.timeout); err != nil { + b.log.Warn("Failed to wait for readyState after navigation", zap.Error(err)) - } else { - defer func() { - _ = domLoaded.Close() - }() - _, _ = domLoaded.Recv() } return fmt.Sprintf("Navigated to: %s\nFrame ID: %s", urlStr, nav.FrameID), nil } +func waitForPageReadyState(parent context.Context, client *cdp.Client, timeout time.Duration) error { + if timeout <= 0 { + timeout = 30 * time.Second + } + waitCtx, cancel := context.WithTimeout(parent, timeout) + defer cancel() + + for { + evalArgs := runtime.NewEvaluateArgs("document.readyState").SetReturnByValue(true) + result, err := client.Runtime.Evaluate(waitCtx, evalArgs) + if err == nil && result.ExceptionDetails == nil { + var state string + if unmarshalErr := json.Unmarshal(result.Result.Value, &state); unmarshalErr == nil { + if state == "interactive" || state == "complete" { + return nil + } + } + } + + select { + case <-waitCtx.Done(): + if err != nil { + return err + } + return waitCtx.Err() + case <-time.After(100 * time.Millisecond): + } + } +} + func (b *BrowserTool) startMode(params map[string]interface{}) (BrowserConnectionMode, error) { opts, err := b.startOptions(params) if err != nil { diff --git a/pkg/tools/browser_session.go b/pkg/tools/browser_session.go index 424e5f4a..99b5bc6a 100644 --- a/pkg/tools/browser_session.go +++ b/pkg/tools/browser_session.go @@ -46,6 +46,7 @@ type BrowserSession struct { connectFn func(port int, timeout time.Duration) error connectEndpointFn func(endpoint string, timeout time.Duration) error launchFn func(timeout time.Duration) error + findChromeFn func() string devtoolsFactory func(endpoint string) browserDevTools } @@ -95,6 +96,7 @@ func GetBrowserSession(log *logger.Logger) *BrowserSession { browserSession.connectFn = browserSession.connect browserSession.connectEndpointFn = browserSession.connectEndpoint browserSession.launchFn = browserSession.launch + browserSession.findChromeFn = findChrome browserSession.devtoolsFactory = func(endpoint string) browserDevTools { return devtool.New(endpoint) } @@ -136,6 +138,9 @@ func (s *BrowserSession) StartWithOptions(timeout time.Duration, opts BrowserSta if s.launchFn == nil { s.launchFn = s.launch } + if s.findChromeFn == nil { + s.findChromeFn = findChrome + } if s.devtoolsFactory == nil { s.devtoolsFactory = func(endpoint string) browserDevTools { return devtool.New(endpoint) @@ -190,8 +195,10 @@ func (s *BrowserSession) launch(timeout time.Duration) error { s.cancel = cancel // Try to find Chrome executable - chromePath := findChrome() + chromePath := s.findChromeFn() if chromePath == "" { + cancel() + s.cancel = nil return fmt.Errorf("chrome not found in PATH") } @@ -217,14 +224,12 @@ func (s *BrowserSession) launch(timeout time.Duration) error { if err := s.cmd.Start(); err != nil { cancel() + s.cancel = nil return fmt.Errorf("failed to start chrome: %w", err) } - // Wait for Chrome to be ready - time.Sleep(2 * time.Second) - - if err := s.connect(9222, timeout); err != nil { - if stopErr := s.Stop(); stopErr != nil { + if err := s.connectWithRetry(9222, timeout); err != nil { + if stopErr := s.stopLocked(); stopErr != nil { s.log.Warn("Failed to stop browser session after launch failure", zap.Error(stopErr)) } return err @@ -237,6 +242,28 @@ func (s *BrowserSession) launch(timeout time.Duration) error { return nil } +func (s *BrowserSession) connectWithRetry(port int, timeout time.Duration) error { + if timeout <= 0 { + timeout = s.timeout + } + connectFn := s.connectFn + if connectFn == nil { + connectFn = s.connect + } + deadline := time.Now().Add(timeout) + var lastErr error + for { + lastErr = connectFn(port, timeout) + if lastErr == nil { + return nil + } + if time.Now().After(deadline) { + return lastErr + } + time.Sleep(250 * time.Millisecond) + } +} + func (s *BrowserSession) connect(port int, timeout time.Duration) error { return s.connectEndpoint(fmt.Sprintf("http://127.0.0.1:%d", port), timeout) } @@ -264,12 +291,8 @@ func (s *BrowserSession) connectEndpoint(endpoint string, timeout time.Duration) return nil } -// Stop stops the browser session. -func (s *BrowserSession) Stop() error { - s.mu.Lock() - defer s.mu.Unlock() - - if !s.ready { +func (s *BrowserSession) stopLocked() error { + if s.conn == nil && s.cancel == nil && s.cmd == nil && !s.ready { return nil } @@ -292,6 +315,7 @@ func (s *BrowserSession) Stop() error { s.client = nil s.conn = nil s.cmd = nil + s.cancel = nil s.debugURL = "" s.mode = BrowserModeAuto s.endpoint = "" @@ -299,6 +323,13 @@ func (s *BrowserSession) Stop() error { return nil } +// Stop stops the browser session. +func (s *BrowserSession) Stop() error { + s.mu.Lock() + defer s.mu.Unlock() + return s.stopLocked() +} + // IsReady returns whether the session is ready. func (s *BrowserSession) IsReady() bool { s.mu.RLock() diff --git a/pkg/tools/browser_session_test.go b/pkg/tools/browser_session_test.go index f72e1502..31dcb1b6 100644 --- a/pkg/tools/browser_session_test.go +++ b/pkg/tools/browser_session_test.go @@ -2,6 +2,8 @@ package tools import ( "context" + "strings" + "os/exec" "errors" "testing" "time" @@ -308,3 +310,69 @@ func TestBrowserSessionGetDevToolsReturnsFactoryForEndpoint(t *testing.T) { t.Fatalf("expected devtools factory to be called once, got %d", called) } } + +func TestBrowserSessionStartWithModeAutoCleansUpAfterLaunchFailure(t *testing.T) { + session := &BrowserSession{timeout: 5 * time.Second, log: newToolsTestLogger(t)} + session.connectFn = func(port int, timeout time.Duration) error { + return errors.New("not running") + } + cmd := exec.Command("bash", "-lc", "sleep 30") + session.launchFn = func(timeout time.Duration) error { + _, cancel := context.WithTimeout(context.Background(), timeout) + session.cancel = cancel + session.cmd = cmd + if err := session.cmd.Start(); err != nil { + return err + } + if err := session.stopLocked(); err != nil { + return err + } + return errors.New("failed to connect after launch") + } + + err := session.StartWithMode(2*time.Second, BrowserModeAuto) + if err == nil { + t.Fatal("expected launch failure") + } + if session.cmd != nil || session.cancel != nil || session.conn != nil || session.client != nil || session.ready { + t.Fatalf("expected failed launch cleanup, got %+v", session.Status()) + } +} + +func TestBrowserSessionConnectWithRetryEventuallySucceeds(t *testing.T) { + session := &BrowserSession{timeout: 2 * time.Second, log: newToolsTestLogger(t)} + attempts := 0 + session.connectFn = func(port int, timeout time.Duration) error { + attempts++ + if attempts < 3 { + return errors.New("not ready") + } + session.ready = true + return nil + } + + err := session.connectWithRetry(9222, 500*time.Millisecond) + if err != nil { + t.Fatalf("connectWithRetry returned error: %v", err) + } + if attempts != 3 { + t.Fatalf("expected 3 attempts before success, got %d", attempts) + } +} + +func TestBrowserSessionLaunchWithoutChromeCleansUpCancel(t *testing.T) { + session := &BrowserSession{timeout: 5 * time.Second, log: newToolsTestLogger(t)} + session.findChromeFn = func() string { return "" } + + err := session.launch(2 * time.Second) + if err == nil { + t.Fatal("expected chrome-not-found failure") + } + if !strings.Contains(err.Error(), "chrome not found in PATH") { + t.Fatalf("unexpected error: %v", err) + } + if session.cancel != nil || session.cmd != nil || session.conn != nil || session.client != nil || session.ready { + t.Fatalf("expected launch failure without chrome to leave no session state, got ready=%v cmd=%v cancel=%v conn=%v client=%v", + session.ready, session.cmd, session.cancel, session.conn, session.client) + } +} diff --git a/pkg/webui/server_goalruns_test.go b/pkg/webui/server_goalruns_test.go index 73e8d2bf..e73109f2 100644 --- a/pkg/webui/server_goalruns_test.go +++ b/pkg/webui/server_goalruns_test.go @@ -647,17 +647,15 @@ func TestGoalRunBrowserToolManualFlowE2E(t *testing.T) { steps := []map[string]interface{}{ {"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('New goal run'))?.click(); true;`}, - {"action": "type", "selector": "#goal-run-name", "text": "Browser tool e2e"}, - {"action": "type", "selector": "#goal-run-goal", "text": "Verify browser-tool Goal Runs flow"}, - {"action": "type", "selector": "#goal-run-criteria", "text": "confirm manually"}, + {"action": "execute_script", "script": `(async function waitForDialog(deadline){ while (Date.now() < deadline) { if (document.querySelector('[role=\"dialog\"]')) { return true; } await new Promise(resolve => setTimeout(resolve, 50)); } throw new Error('goal run dialog did not render in time'); })(Date.now() + 4000);`}, + {"action": "execute_script", "script": `(async function(){ const waitFor = async (selector) => { const deadline = Date.now() + 4000; while (Date.now() < deadline) { const el = document.querySelector(selector); if (el) return el; await new Promise(resolve => setTimeout(resolve, 50)); } throw new Error('missing ' + selector); }; const set = async (selector, value) => { const el = await waitFor(selector); el.value = value; el.dispatchEvent(new Event('input', { bubbles: true })); }; await set('#goal-run-name', 'Browser tool e2e'); await set('#goal-run-goal', 'Verify browser-tool Goal Runs flow'); await set('#goal-run-criteria', 'confirm manually'); return true; })();`}, {"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Create run'))?.click(); true;`}, {"action": "wait", "duration": float64(1200)}, {"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Confirm criteria'))?.click(); true;`}, {"action": "wait", "duration": float64(600)}, {"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Start run'))?.click(); true;`}, {"action": "wait", "duration": float64(1200)}, - {"action": "execute_script", "script": `const areas = Array.from(document.querySelectorAll('textarea')); if (areas.length > 0) { areas[areas.length - 1].focus(); } true;`}, - {"action": "type", "selector": "textarea", "text": "browser tool e2e confirmed"}, + {"action": "execute_script", "script": `(function(){ const areas = Array.from(document.querySelectorAll('textarea')); if (areas.length === 0) throw new Error('missing textarea'); const el = areas[areas.length - 1]; el.value = 'browser tool e2e confirmed'; el.dispatchEvent(new Event('input', { bubbles: true })); return true; })();`}, {"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Approve criterion'))?.click(); true;`}, {"action": "wait", "duration": float64(1200)}, }