From 9f93b02e9b3bb37e1c67be5df1030e4b8f2abd14 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:12:24 +0000 Subject: [PATCH] Await the start_url navigation and surface its failures Page.navigate only resolves once the navigation commits, so the 3s budget on /configure's start_url step expired against origins that were slow to answer. The handler then logged a warning, returned 200, and handed the session over still rendering the pre-navigation page while the navigation committed seconds later. Chrome also reports an unreachable URL by returning an errorText from Page.navigate and committing an error page. DispatchStartURL discarded the result, so that read as a successful navigation. Raise the navigate budget to 15s, which covers a slow origin's DNS, connect and first byte; commit semantics mean it does not have to cover subresource loading. Return Chrome's errorText from DispatchStartURL so callers can tell a committed navigation from a committed error page, and log both failure modes with the elapsed time. A navigation that still fails keeps returning 200 rather than discarding an otherwise healthy browser over a start_url the caller may not control. --- server/cmd/api/api/chromium_configure.go | 31 ++++++++--- server/cmd/wrapper/snapshot_start_page.go | 2 +- server/lib/cdpclient/cdpclient.go | 46 ++++++++++------ server/lib/cdpclient/cdpclient_test.go | 65 ++++++++++++++++++++++- server/openapi.yaml | 11 ++-- 5 files changed, 128 insertions(+), 27 deletions(-) diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index 7d995314..ff7f6cb4 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -24,7 +24,13 @@ import ( const userDataProfileDir = "/home/kernel/user-data" const maxStartURLLen = 2048 -const startURLDispatchTimeout = 3 * time.Second + +// startURLNavigateTimeout bounds the start_url navigation. Page.navigate only +// resolves once the navigation commits, so this budget has to cover the origin's +// DNS, connect and first byte; the previous 3s budget expired against slow +// origins and handed the session over still showing the pre-navigation page. It +// does not have to cover subresource loading, which continues after commit. +const startURLNavigateTimeout = 15 * time.Second type chromiumConfigureState struct { displayJSON *string @@ -181,8 +187,19 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu } if spec.needsNav { - if err := chromiumDoNavigate(ctx, s, spec); err != nil { - logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err) + // A failed navigation leaves the session on whatever the browser was + // showing, so log it and keep going rather than discarding a browser over + // a start_url the caller may not control. + navStart := time.Now() + navErrorText, navErr := chromiumDoNavigate(ctx, s, spec) + if navErr == nil && navErrorText != "" { + navErr = errors.New(navErrorText) + } + if navErr != nil { + logger.FromContext(ctx).Warn("start_url dispatch failed", + "error", navErr, + "elapsed", time.Since(navStart).String(), + "timeout", startURLNavigateTimeout.String()) } } @@ -234,12 +251,14 @@ func normalizeStartURL(rawURL string) string { return rawURL } -func chromiumDoNavigate(ctx context.Context, s *ApiService, spec startURLParsed) error { +// chromiumDoNavigate navigates to spec.url and returns Chrome's navigation +// errorText, which is non-empty when an error page committed instead. +func chromiumDoNavigate(ctx context.Context, s *ApiService, spec startURLParsed) (string, error) { upstream := s.upstreamMgr.Current() if upstream == "" { - return fmt.Errorf("devtools upstream not available") + return "", fmt.Errorf("devtools upstream not available") } - navCtx, cancel := context.WithTimeout(ctx, startURLDispatchTimeout) + navCtx, cancel := context.WithTimeout(ctx, startURLNavigateTimeout) defer cancel() return cdpclient.DispatchStartURL(navCtx, upstream, spec.url) } diff --git a/server/cmd/wrapper/snapshot_start_page.go b/server/cmd/wrapper/snapshot_start_page.go index b1c25afe..7e1401e2 100644 --- a/server/cmd/wrapper/snapshot_start_page.go +++ b/server/cmd/wrapper/snapshot_start_page.go @@ -62,7 +62,7 @@ func prepareSnapshotStartPage(ctx context.Context, internalPort string) (retErr logf("WARNING: snapshot start page unavailable, using about:blank: %v", navErr) blankCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() - if err := cdpclient.DispatchStartURL(blankCtx, devtoolsURL, "about:blank"); err != nil { + if _, err := cdpclient.DispatchStartURL(blankCtx, devtoolsURL, "about:blank"); err != nil { return fmt.Errorf("reset snapshot start page: %w", err) } return nil diff --git a/server/lib/cdpclient/cdpclient.go b/server/lib/cdpclient/cdpclient.go index a59073c5..1cf89cbf 100644 --- a/server/lib/cdpclient/cdpclient.go +++ b/server/lib/cdpclient/cdpclient.go @@ -220,19 +220,26 @@ func (c *Client) CountPageTargets(ctx context.Context) (int, error) { return n, nil } -// DispatchStartURL closes extra page targets and dispatches a navigation on the -// first page target. It does not wait for lifecycle events; Chrome owns the -// eventual navigation result. -func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { +// DispatchStartURL closes extra page targets and navigates the first page +// target. Page.navigate resolves once the navigation commits, so this returns +// after the new document has replaced the old one but before subresources +// finish loading. +// +// The first return value is Chrome's navigation errorText, non-empty when the +// navigation committed an error page instead of the requested URL (an +// unreachable host, for example). Callers that need the requested page to have +// actually loaded must check it; a nil error alone only means the command was +// accepted. +func DispatchStartURL(ctx context.Context, devtoolsURL, url string) (string, error) { c, err := Dial(ctx, devtoolsURL) if err != nil { - return fmt.Errorf("dial devtools: %w", err) + return "", fmt.Errorf("dial devtools: %w", err) } defer c.Close() targetsResult, err := c.send(ctx, "Target.getTargets", nil, "") if err != nil { - return fmt.Errorf("Target.getTargets: %w", err) + return "", fmt.Errorf("Target.getTargets: %w", err) } var targets struct { @@ -242,7 +249,7 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { } `json:"targetInfos"` } if err := json.Unmarshal(targetsResult, &targets); err != nil { - return fmt.Errorf("unmarshal targets: %w", err) + return "", fmt.Errorf("unmarshal targets: %w", err) } var pageTargetID string @@ -263,13 +270,13 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { "url": "about:blank", }, "") if err != nil { - return fmt.Errorf("Target.createTarget: %w", err) + return "", fmt.Errorf("Target.createTarget: %w", err) } var created struct { TargetID string `json:"targetId"` } if err := json.Unmarshal(createResult, &created); err != nil { - return fmt.Errorf("unmarshal create target: %w", err) + return "", fmt.Errorf("unmarshal create target: %w", err) } pageTargetID = created.TargetID } @@ -279,14 +286,14 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { "flatten": true, }, "") if err != nil { - return fmt.Errorf("Target.attachToTarget: %w", err) + return "", fmt.Errorf("Target.attachToTarget: %w", err) } var attach struct { SessionID string `json:"sessionId"` } if err := json.Unmarshal(attachResult, &attach); err != nil { - return fmt.Errorf("unmarshal attach: %w", err) + return "", fmt.Errorf("unmarshal attach: %w", err) } defer func() { detachCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -296,16 +303,25 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { }, "") }() - if _, err := c.send(ctx, "Page.navigate", map[string]any{"url": url}, attach.SessionID); err != nil { - return fmt.Errorf("Page.navigate: %w", err) + navResult, err := c.send(ctx, "Page.navigate", map[string]any{"url": url}, attach.SessionID) + if err != nil { + return "", fmt.Errorf("Page.navigate: %w", err) } - return nil + var nav struct { + ErrorText string `json:"errorText"` + } + if err := json.Unmarshal(navResult, &nav); err != nil { + return "", fmt.Errorf("unmarshal navigate result: %w", err) + } + return nav.ErrorText, nil } // DispatchStartURLAndWait navigates through navigationURL and waits for // destination to load without resolving to Chrome's network error page. func DispatchStartURLAndWait(ctx context.Context, devtoolsURL, navigationURL, destination string) error { - if err := DispatchStartURL(ctx, devtoolsURL, navigationURL); err != nil { + // The polling loop below re-navigates through chrome-error:// pages, so a + // reported navigation error is not terminal here. + if _, err := DispatchStartURL(ctx, devtoolsURL, navigationURL); err != nil { return err } diff --git a/server/lib/cdpclient/cdpclient_test.go b/server/lib/cdpclient/cdpclient_test.go index 8154b361..54cb1d69 100644 --- a/server/lib/cdpclient/cdpclient_test.go +++ b/server/lib/cdpclient/cdpclient_test.go @@ -34,6 +34,8 @@ type fakeCDP struct { navigateCalled bool navigateCalls int navigateURL string + navigateErrorText string + navigateBlock time.Duration pageStates []string pageStateIndex int } @@ -115,7 +117,20 @@ func (f *fakeCDP) handler(w http.ResponseWriter, r *http.Request) { var params map[string]any _ = json.Unmarshal(req.Params, ¶ms) f.navigateURL, _ = params["url"].(string) - result = map[string]any{"frameId": "frame-1"} + // Page.navigate resolves on commit, so a slow origin holds the + // response open rather than replying and navigating later. + if f.navigateBlock > 0 { + select { + case <-time.After(f.navigateBlock): + case <-ctx.Done(): + return + } + } + nav := map[string]any{"frameId": "frame-1"} + if f.navigateErrorText != "" { + nav["errorText"] = f.navigateErrorText + } + result = nav case "Runtime.evaluate": state := `{"url":"about:blank","readyState":"loading"}` if len(f.pageStates) > 0 { @@ -235,6 +250,54 @@ func TestSetDeviceMetricsOverride(t *testing.T) { }) } +func TestDispatchStartURL(t *testing.T) { + t.Run("reports no error text on a committed navigation", func(t *testing.T) { + f := &fakeCDP{pageTargetID: "target-123", sessionID: "session-abc"} + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + errorText, err := DispatchStartURL(ctx, url, "https://example.com/") + require.NoError(t, err) + assert.Empty(t, errorText) + assert.Equal(t, "https://example.com/", f.navigateURL) + }) + + // Chrome answers Page.navigate with an errorText and commits an error page + // when the requested URL cannot load, which must not read as success. + t.Run("surfaces the navigation error text", func(t *testing.T) { + f := &fakeCDP{ + pageTargetID: "target-123", + sessionID: "session-abc", + navigateErrorText: "net::ERR_CONNECTION_REFUSED", + } + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + errorText, err := DispatchStartURL(ctx, url, "https://unreachable.example/") + require.NoError(t, err) + assert.Equal(t, "net::ERR_CONNECTION_REFUSED", errorText) + }) + + // When the origin is slower to commit than the caller's budget, the caller + // has to see the failure rather than assume the page was replaced. + t.Run("errors when the navigation does not commit within the budget", func(t *testing.T) { + f := &fakeCDP{ + pageTargetID: "target-123", + sessionID: "session-abc", + navigateBlock: 2 * time.Second, + } + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + _, err := DispatchStartURL(ctx, url, "https://slow.example/") + require.Error(t, err) + assert.Contains(t, err.Error(), "Page.navigate") + }) +} + func TestDispatchStartURLAndWait(t *testing.T) { t.Run("waits for loaded destination", func(t *testing.T) { f := &fakeCDP{ diff --git a/server/openapi.yaml b/server/openapi.yaml index 20dfdef9..450e1050 100644 --- a/server/openapi.yaml +++ b/server/openapi.yaml @@ -1253,9 +1253,12 @@ paths: description: | Optional multipart parts apply configuration while Chromium stays stopped once (policy, flags, extensions, profile archive, optional display sizing), then Chromium is started exactly - once and DevTools readiness is awaited. Optional `start_url` dispatches a best-effort - navigation after readiness without waiting for page load. Bare hosts are normalized to - `https://`. Omit any part you do not need. At least one actionable part must be present. + once and DevTools readiness is awaited. Optional `start_url` is navigated after readiness and + awaited until the navigation commits, so the requested document has replaced whatever the + browser was showing; subresources may still be loading. A navigation that fails or exceeds + the internal budget is logged and still returns 200, leaving the browser on its + pre-navigation page. Bare hosts are normalized to `https://`. Omit any part you do not need. + At least one actionable part must be present. Required configuration steps run in this order: policies, extensions, display, flags, then profile archive. Chromium is started only once at the end. The endpoint is not transactional: if a later required step fails, earlier successful side effects may remain @@ -1315,7 +1318,7 @@ paths: required: [zip_file, name] responses: "200": - description: Configuration applied; optional navigate completed successfully. + description: Configuration applied; optional navigate committed, failed, or timed out. content: application/json: schema: