From e7fe47d63fbc02ad122d474a035f53ae0b46b6e5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 18 Jul 2026 10:58:22 +0100 Subject: [PATCH] fix(browser): allow one retry after transient failures (#110607) * fix(browser): soften transient retry guidance * style(browser): format retry hint test * chore: keep release note in PR body --- .../client-fetch.attach-only.e2e.test.ts | 2 + .../client-fetch.loopback-auth.test.ts | 253 +++++++++++++++++- .../browser/src/browser/client-fetch.ts | 113 ++++++-- 3 files changed, 334 insertions(+), 34 deletions(-) diff --git a/extensions/browser/src/browser/client-fetch.attach-only.e2e.test.ts b/extensions/browser/src/browser/client-fetch.attach-only.e2e.test.ts index 37360e0e3bf0..e0aa3f91837b 100644 --- a/extensions/browser/src/browser/client-fetch.attach-only.e2e.test.ts +++ b/extensions/browser/src/browser/client-fetch.attach-only.e2e.test.ts @@ -75,6 +75,8 @@ describe("browser client fetch attachOnly diagnostics", () => { const message = thrown instanceof Error ? thrown.message : String(thrown); expect(message).toContain("browser profile is external to OpenClaw"); expect(message).toContain("Restarting the OpenClaw gateway will not launch it"); + expect(message).toContain("Retry the browser tool once"); + expect(message).toContain("If the same error persists"); expect(message).not.toContain("Restart the OpenClaw gateway"); expect(message).not.toContain("Do NOT retry the browser tool"); } finally { diff --git a/extensions/browser/src/browser/client-fetch.loopback-auth.test.ts b/extensions/browser/src/browser/client-fetch.loopback-auth.test.ts index c08b0fb66186..ba61fe748774 100644 --- a/extensions/browser/src/browser/client-fetch.loopback-auth.test.ts +++ b/extensions/browser/src/browser/client-fetch.loopback-auth.test.ts @@ -226,11 +226,16 @@ describe("fetchBrowserJson loopback auth", () => { expect(headers.get("authorization")).toBeNull(); }); - it("preserves dispatcher timeout context without no-retry hint", async () => { + it("preserves dispatcher timeout context with retry-once hint", async () => { mocks.dispatch.mockRejectedValueOnce(new Error("Chrome CDP handshake timeout")); await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { - contains: ["Chrome CDP handshake timeout", "Restart the OpenClaw gateway"], + contains: [ + "Chrome CDP handshake timeout", + "Restart the OpenClaw gateway", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["Can't reach the OpenClaw browser control service", "Do NOT retry the browser tool"], }); }); @@ -267,6 +272,8 @@ describe("fetchBrowserJson loopback auth", () => { "Chrome CDP handshake timeout", "browser profile is external to OpenClaw", "Restarting the OpenClaw gateway will not launch it", + "Retry the browser tool once", + "If the same error persists", ], omits: ["Restart the OpenClaw gateway", "Do NOT retry the browser tool"], }, @@ -319,6 +326,8 @@ describe("fetchBrowserJson loopback auth", () => { "timed out", "browser profile is external to OpenClaw", "Restarting the OpenClaw gateway will not launch it", + "Retry the browser tool once", + "If the same error persists", ], omits: ["Restart the OpenClaw gateway", "Do NOT retry the browser tool"], }, @@ -342,7 +351,12 @@ describe("fetchBrowserJson loopback auth", () => { await expectThrownBrowserFetchError( () => fetchBrowserJson<{ ok: boolean }>("/tabs?profile=openclaw"), { - contains: ["Chrome CDP handshake timeout", "Restart the OpenClaw gateway"], + contains: [ + "Chrome CDP handshake timeout", + "Restart the OpenClaw gateway", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["browser profile is external to OpenClaw", "Do NOT retry the browser tool"], }, ); @@ -357,7 +371,12 @@ describe("fetchBrowserJson loopback auth", () => { await expectThrownBrowserFetchError( () => fetchBrowserJson<{ ok: boolean }>("/tabs?profile=manual"), { - contains: ["Chrome CDP handshake timeout", "Restart the OpenClaw gateway"], + contains: [ + "Chrome CDP handshake timeout", + "Restart the OpenClaw gateway", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["browser profile is external to OpenClaw", "Do NOT retry the browser tool"], }, ); @@ -380,7 +399,12 @@ describe("fetchBrowserJson loopback auth", () => { await expectThrownBrowserFetchError( () => fetchBrowserJson<{ ok: boolean }>("/tabs?profile=missing"), { - contains: ["Chrome CDP handshake timeout", "Restart the OpenClaw gateway"], + contains: [ + "Chrome CDP handshake timeout", + "Restart the OpenClaw gateway", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["browser profile is external to OpenClaw", "Do NOT retry the browser tool"], }, ); @@ -406,6 +430,8 @@ describe("fetchBrowserJson loopback auth", () => { "Chrome CDP handshake timeout", "browser profile is external to OpenClaw", "Restarting the OpenClaw gateway will not launch it", + "Retry the browser tool once", + "If the same error persists", ], omits: ["Restart the OpenClaw gateway", "Do NOT retry the browser tool"], }); @@ -449,6 +475,51 @@ describe("fetchBrowserJson loopback auth", () => { }); }); + it("keeps transient dispatcher connection resets retryable once", async () => { + mocks.dispatch.mockRejectedValueOnce(new Error("Chrome CDP connection reset")); + + await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { + contains: [ + "Chrome CDP connection reset", + "Retry the browser tool once", + "If the same error persists", + ], + omits: ["Do NOT retry the browser tool"], + }); + }); + + it("uses top-level reset codes to classify dispatcher failures as transient", async () => { + mocks.dispatch.mockRejectedValueOnce( + Object.assign(new Error("socket closed"), { code: "ECONNRESET" }), + ); + + await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { + contains: ["socket closed", "Retry the browser tool once", "If the same error persists"], + omits: ["Do NOT retry the browser tool"], + }); + }); + + it("keeps refusal causes non-retryable when the outer error mentions a timeout", async () => { + const refused = Object.assign(new Error("connect refused"), { code: "ECONNREFUSED" }); + mocks.dispatch.mockRejectedValueOnce( + new Error("browser request timed out", { cause: refused }), + ); + + await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { + contains: ["browser request timed out", "Do NOT retry the browser tool"], + omits: ["Retry the browser tool once"], + }); + }); + + it("keeps disabled browser control failures non-retryable", async () => { + mocks.dispatch.mockRejectedValueOnce(new Error("browser control disabled")); + + await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { + contains: ["browser control disabled", "Do NOT retry the browser tool"], + omits: ["Retry the browser tool once"], + }); + }); + it("preserves validated structured errors from dispatcher routes", async () => { mocks.dispatch.mockResolvedValueOnce({ status: 409, @@ -540,7 +611,115 @@ describe("fetchBrowserJson loopback auth", () => { () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), { contains: ["internal error"], - omits: ["rate limit"], + omits: ["rate limit", "Retry the browser tool once", "Do NOT retry the browser tool"], + }, + ); + }); + + it("keeps transient HTTP error payloads retryable once", async () => { + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response(JSON.stringify({ error: "Chrome CDP handshake timeout" }), { + status: 504, + }), + ), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), + { + contains: [ + "Chrome CDP handshake timeout", + "Retry the browser tool once", + "If the same error persists", + ], + omits: ["Do NOT retry the browser tool"], + }, + ); + }); + + it.each([408, 504])("uses HTTP %i to classify generic payloads as transient", async (status) => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("request failed", { status })), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), + { + contains: ["request failed", "Retry the browser tool once", "If the same error persists"], + omits: ["Do NOT retry the browser tool"], + }, + ); + }); + + it("does not mark client validation errors transient from timeout wording alone", async () => { + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response(JSON.stringify({ error: "invalid timeout value" }), { + status: 400, + }), + ), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), + { + contains: ["invalid timeout value"], + omits: ["Retry the browser tool once", "Do NOT retry the browser tool"], + }, + ); + }); + + it("keeps pre-annotated persistent payload hints mutually exclusive", async () => { + const persistentHint = + "Do NOT retry the browser tool — it will keep failing. Use an alternative approach or inform the user that the browser is currently unavailable."; + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response(JSON.stringify({ error: `browser request timed out. ${persistentHint}` }), { + status: 504, + }), + ), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), + { + contains: ["browser request timed out", persistentHint], + omits: ["Retry the browser tool once"], + }, + ); + }); + + it("keeps transient dispatcher error payloads retryable once", async () => { + mocks.dispatch.mockResolvedValueOnce({ + status: 500, + body: { error: "read ECONNRESET" }, + }); + + await expectThrownBrowserFetchError(() => fetchBrowserJson<{ ok: boolean }>("/tabs"), { + contains: ["read ECONNRESET", "Retry the browser tool once", "If the same error persists"], + omits: ["Do NOT retry the browser tool"], + }); + }); + + it("keeps authentication failures non-retryable", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("Unauthorized", { status: 401 })), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:18888/"), + { + contains: ["Unauthorized", "Do NOT retry the browser tool"], + omits: ["Retry the browser tool once"], }, ); }); @@ -557,7 +736,7 @@ describe("fetchBrowserJson loopback auth", () => { }); }); - it("keeps absolute URL failures wrapped as reachability errors", async () => { + it("keeps transient absolute URL failures retryable once", async () => { vi.stubGlobal( "fetch", vi.fn(async () => { @@ -570,13 +749,51 @@ describe("fetchBrowserJson loopback auth", () => { { contains: [ "Can't reach the OpenClaw browser control service", - "Do NOT retry the browser tool", + "Retry the browser tool once", + "If the same error persists", ], + omits: ["Do NOT retry the browser tool"], }, ); }); - it("omits no-retry hint for absolute HTTP timeout failures", async () => { + it("uses nested reset causes to classify generic fetch failures as transient", async () => { + const reset = Object.assign(new Error("socket closed"), { code: "ECONNRESET" }); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new TypeError("fetch failed", { cause: reset }); + }), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://example.com/"), + { + contains: ["fetch failed", "Retry the browser tool once", "If the same error persists"], + omits: ["Do NOT retry the browser tool"], + }, + ); + }); + + it("uses nested refusal causes to keep unavailable services non-retryable", async () => { + const refused = Object.assign(new Error("connect refused"), { code: "ECONNREFUSED" }); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new TypeError("fetch failed", { cause: refused }); + }), + ); + + await expectThrownBrowserFetchError( + () => fetchBrowserJson<{ ok: boolean }>("http://example.com/"), + { + contains: ["fetch failed", "Do NOT retry the browser tool"], + omits: ["Retry the browser tool once"], + }, + ); + }); + + it("uses retry-once hint for absolute HTTP timeout failures", async () => { vi.stubGlobal( "fetch", vi.fn(async () => { @@ -587,7 +804,11 @@ describe("fetchBrowserJson loopback auth", () => { await expectThrownBrowserFetchError( () => fetchBrowserJson<{ ok: boolean }>("http://example.com/", { timeoutMs: 1234 }), { - contains: ["timed out after 1234ms"], + contains: [ + "timed out after 1234ms", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["Do NOT retry the browser tool"], }, ); @@ -604,7 +825,11 @@ describe("fetchBrowserJson loopback auth", () => { await expectThrownBrowserFetchError( () => fetchBrowserJson<{ ok: boolean }>("http://example.com/", { timeoutMs: Number.NaN }), { - contains: ["timed out after 5000ms"], + contains: [ + "timed out after 5000ms", + "Retry the browser tool once", + "If the same error persists", + ], omits: ["NaNms", "Do NOT retry the browser tool"], }, ); @@ -628,7 +853,11 @@ describe("fetchBrowserJson loopback auth", () => { timeoutMs: Number.MAX_SAFE_INTEGER, }), { - contains: [`timed out after ${MAX_TIMER_TIMEOUT_MS}ms`], + contains: [ + `timed out after ${MAX_TIMER_TIMEOUT_MS}ms`, + "Retry the browser tool once", + "If the same error persists", + ], omits: ["Do NOT retry the browser tool"], }, ); diff --git a/extensions/browser/src/browser/client-fetch.ts b/extensions/browser/src/browser/client-fetch.ts index 9281df4ca9a3..7101f0c951d1 100644 --- a/extensions/browser/src/browser/client-fetch.ts +++ b/extensions/browser/src/browser/client-fetch.ts @@ -5,6 +5,7 @@ * in-process dispatcher, adding loopback auth and operator-facing diagnostics. */ import { parseBrowserHttpUrl } from "openclaw/plugin-sdk/browser-config"; +import { extractErrorCode, formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime"; import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime"; import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime"; @@ -45,8 +46,10 @@ function browserServiceErrorFromPayload( status?: number, ): BrowserServiceError { const parsed = parseBrowserErrorPayload(value); + const message = parsed?.error ?? fallback; + const modelHint = resolveBrowserServiceModelHint(message, status); return new BrowserServiceError( - parsed?.error ?? fallback, + modelHint ? appendBrowserToolModelHint(message, modelHint) : message, parsed && "reason" in parsed ? parsed : undefined, status, ); @@ -126,9 +129,19 @@ function withLoopbackBrowserAuth( }); } -const BROWSER_TOOL_MODEL_HINT = +const BROWSER_TOOL_PERSISTENT_MODEL_HINT = "Do NOT retry the browser tool — it will keep failing. " + "Use an alternative approach or inform the user that the browser is currently unavailable."; +const BROWSER_TOOL_TRANSIENT_MODEL_HINT = + "This may be a transient browser error. Retry the browser tool once. " + + "If the same error persists, use an alternative approach or inform the user that the browser is currently unavailable."; + +// Retry history already lives in the model transcript. Keep this classifier stateless so one +// session's transient failure cannot suppress browser retries in another session. +const BROWSER_TRANSIENT_NETWORK_ERROR_RE = + /\b(?:ECONNRESET|ECONNABORTED|ENETRESET|ETIMEDOUT|EPIPE|EHOSTUNREACH|ENETUNREACH|EAI_AGAIN|UND_ERR_(?:CONNECT_TIMEOUT|HEADERS_TIMEOUT|BODY_TIMEOUT|SOCKET))\b|fetch failed|network error|other side closed|socket (?:hang up|terminated)|connection (?:reset|aborted|timed out)/i; +const BROWSER_PERSISTENT_FAILURE_RE = + /\bECONNREFUSED\b|connection refused|browser control (?:is )?(?:disabled|not enabled)|invalid (?:auth|authentication|credentials|password|token)|authentication (?:failed|required)|unauthorized/i; const BROWSER_ERROR_BODY_LIMIT_BYTES = 16 * 1024; // `response/body` supports 5M characters; 32 MiB covers worst-case JSON escaping while staying bounded. @@ -185,38 +198,87 @@ function normalizeErrorMessage(err: unknown): string { return String(err); } -function appendBrowserToolModelHint(message: string): string { - if (message.includes(BROWSER_TOOL_MODEL_HINT)) { - return message; - } - return `${message} ${BROWSER_TOOL_MODEL_HINT}`; +function appendBrowserToolModelHint(message: string, hint: string): string { + const messageWithoutHints = message + .replaceAll(BROWSER_TOOL_PERSISTENT_MODEL_HINT, "") + .replaceAll(BROWSER_TOOL_TRANSIENT_MODEL_HINT, "") + .trim(); + return `${messageWithoutHints} ${hint}`; } -type BrowserFetchFailureKind = "timeout" | "aborted" | "persistent"; +type BrowserFetchFailureKind = "timeout" | "aborted" | "transient-network" | "persistent"; function resolveBrowserFetchTimeoutMs(timeoutMs: number | undefined): number { return resolveTimerTimeoutMs(timeoutMs, 5000); } function classifyBrowserFetchFailure(err: unknown): BrowserFetchFailureKind { - const msg = normalizeErrorMessage(err); - const msgLower = normalizeLowercaseStringOrEmpty(msg); + const directCode = extractErrorCode(err); + const formatted = formatErrorMessage(err); + const detail = directCode ? `${formatted} | ${directCode}` : formatted; + const detailLower = normalizeLowercaseStringOrEmpty(detail); const nameLower = err instanceof Error ? normalizeLowercaseStringOrEmpty(err.name) : ""; + if (nameLower === "aborterror") { + return "aborted"; + } + if (BROWSER_PERSISTENT_FAILURE_RE.test(detail)) { + return "persistent"; + } const looksLikeTimeout = - nameLower.includes("timeout") || msgLower.includes("timed out") || msgLower.includes("timeout"); + nameLower.includes("timeout") || + detailLower.includes("timed out") || + detailLower.includes("timeout"); if (looksLikeTimeout) { return "timeout"; } + if (BROWSER_TRANSIENT_NETWORK_ERROR_RE.test(detail)) { + return "transient-network"; + } const looksLikeAbort = - nameLower === "aborterror" || - msgLower.includes("aborterror") || - msgLower.includes("aborted") || - msgLower.includes("abort") || - msgLower.includes("cancelled") || - msgLower.includes("canceled"); + detailLower.includes("aborterror") || + detailLower.includes("aborted") || + detailLower.includes("abort") || + detailLower.includes("cancelled") || + detailLower.includes("canceled"); return looksLikeAbort ? "aborted" : "persistent"; } +function isPersistentBrowserServiceFailure(message: string, status: number | undefined): boolean { + return status === 401 || BROWSER_PERSISTENT_FAILURE_RE.test(message); +} + +function resolveBrowserServiceModelHint( + message: string, + status: number | undefined, +): string | undefined { + if (message.includes(BROWSER_TOOL_PERSISTENT_MODEL_HINT)) { + return BROWSER_TOOL_PERSISTENT_MODEL_HINT; + } + if (message.includes(BROWSER_TOOL_TRANSIENT_MODEL_HINT)) { + return BROWSER_TOOL_TRANSIENT_MODEL_HINT; + } + if (isPersistentBrowserServiceFailure(message, status)) { + return BROWSER_TOOL_PERSISTENT_MODEL_HINT; + } + if (status === 408 || status === 504) { + return BROWSER_TOOL_TRANSIENT_MODEL_HINT; + } + if (status === undefined || status < 500 || status > 599) { + return undefined; + } + const kind = classifyBrowserFetchFailure(new Error(message)); + return kind === "timeout" || kind === "transient-network" + ? BROWSER_TOOL_TRANSIENT_MODEL_HINT + : undefined; +} + +function resolveBrowserToolModelHint(kind: BrowserFetchFailureKind): string | undefined { + if (kind === "timeout" || kind === "transient-network") { + return BROWSER_TOOL_TRANSIENT_MODEL_HINT; + } + return kind === "persistent" ? BROWSER_TOOL_PERSISTENT_MODEL_HINT : undefined; +} + async function discardResponseBody(res: Response): Promise { try { await res.body?.cancel(); @@ -230,8 +292,8 @@ function enhanceDispatcherPathError(url: string, err: unknown): Error { const kind = classifyBrowserFetchFailure(err); const ownership = resolveDispatcherBrowserControlOwnership(url); const operatorHint = resolveBrowserFetchOperatorHint(url, { ownership }); - const suffix = - kind === "persistent" ? `${operatorHint} ${BROWSER_TOOL_MODEL_HINT}` : operatorHint; + const modelHint = resolveBrowserToolModelHint(kind); + const suffix = modelHint ? `${operatorHint} ${modelHint}` : operatorHint; const normalized = msg.endsWith(".") ? msg : `${msg}.`; return new Error(`${normalized} ${suffix}`, err instanceof Error ? { cause: err } : undefined); } @@ -242,7 +304,7 @@ function enhanceBrowserFetchError(url: string, err: unknown, timeoutMs: number): const kind = classifyBrowserFetchFailure(err); if (kind === "timeout") { return new Error( - `Can't reach the OpenClaw browser control service (timed out after ${timeoutMs}ms). ${operatorHint}`, + `Can't reach the OpenClaw browser control service (timed out after ${timeoutMs}ms). ${operatorHint} ${BROWSER_TOOL_TRANSIENT_MODEL_HINT}`, err instanceof Error ? { cause: err } : undefined, ); } @@ -252,9 +314,16 @@ function enhanceBrowserFetchError(url: string, err: unknown, timeoutMs: number): err instanceof Error ? { cause: err } : undefined, ); } + if (kind === "transient-network") { + return new Error( + `Can't reach the OpenClaw browser control service. ${operatorHint} (${msg}) ${BROWSER_TOOL_TRANSIENT_MODEL_HINT}`, + err instanceof Error ? { cause: err } : undefined, + ); + } return new Error( appendBrowserToolModelHint( `Can't reach the OpenClaw browser control service. ${operatorHint} (${msg})`, + BROWSER_TOOL_PERSISTENT_MODEL_HINT, ), err instanceof Error ? { cause: err } : undefined, ); @@ -298,7 +367,7 @@ async function fetchHttpJson( // Do not reflect upstream response text into the error surface (log/agent injection risk) await discardResponseBody(res); throw new BrowserServiceError( - `${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_MODEL_HINT}`, + `${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_PERSISTENT_MODEL_HINT}`, ); } // Overflow cancels the stream and releases its reader lock before the guarded fetch below. @@ -420,7 +489,7 @@ export async function fetchBrowserJson( if (isRateLimitStatus(result.status)) { // Do not reflect upstream response text into the error surface (log/agent injection risk) throw new BrowserServiceError( - `${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_MODEL_HINT}`, + `${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_PERSISTENT_MODEL_HINT}`, ); } throw browserServiceErrorFromPayload(result.body, `HTTP ${result.status}`, result.status);