Greptile Summary

This PR improves observability when Browserbase returns HTTP 429 (max concurrent sessions exceeded) by surfacing actionable, no-retry error messages instead of the previous generic "Browser control failed" text. The change touches three layers of the browser stack — the HTTP/dispatcher fetch client, the CDP helper used for WebSocket-endpoint discovery, and the Playwright session retry loop — and adds four test cases covering both fetch paths.

Key changes:

• client-fetch.ts: Adds BROWSER_RATE_LIMIT_MESSAGE constant + isRateLimitStatus helper; both fetchHttpJson and the dispatcher branch now throw BrowserServiceError with the rate-limit message on 429.
• cdp.helpers.ts: fetchCdpChecked detects 429 before the generic HTTP ${status} throw, but the message is hand-rolled rather than shared — it differs in structure and no-retry wording from client-fetch.ts.
• pw-session.ts: Breaks out of the connectBrowser retry loop on rate-limit errors; the errMsg.includes("429") substring check is overly broad and could suppress retries for unrelated errors containing that token.
• client-fetch.loopback-auth.test.ts: Four new tests cover HTTP-path 429 (with/without body), dispatcher-path 429, and a regression guard for non-429 errors.

Confidence Score: 4/5

• This PR is safe to merge; the logic is correct and the two flagged issues are style/maintenance concerns with low runtime impact.
• The core 429 detection and error-propagation logic is sound across all three layers, and the new tests validate the primary paths. Score is 4 rather than 5 because of two minor issues: the errMsg.includes("429") substring check in pw-session.ts could theoretically suppress retries on unrelated errors, and the rate-limit message in cdp.helpers.ts is a duplicate with slightly different wording/structure that will drift over time.
• Minor attention on src/browser/pw-session.ts (fragile "429" substring match) and src/browser/cdp.helpers.ts (message duplication/inconsistency).

_{Last reviewed commit: 91a553f}

Thanks for picking this up! The implementation looks solid.

A few observations from my testing that led to filing the original issue:

1. The error message is exactly what I needed — "Do NOT retry" is critical because agents will otherwise loop indefinitely burning tokens.
2. Question: Does this also catch the case where Browserbase returns 429 during session creation (via Stagehand init())? That's where I hit it most often with cron jobs competing for the single free-tier slot.
3. Minor: Would it be worth including the current session limit in the error message if Browserbase returns it? e.g., "max concurrent sessions: 1" — helps users know if upgrading would even help.

Happy to test this on my setup (Docker/GCP, cron jobs using Stagehand + Browserbase) once it's ready for review.

Thanks @wirelessjoe! Glad the error messages match what you needed. Your original issue report was really clear - made it easy to reproduce and fix.

Addressed bot feedback in 3702810:

P2 - "429" substring false positives: Removed errMsg.includes("429") from the retry-skip check in pw-session.ts. The errMsg.includes("rate limit") check alone is sufficient since every 429 thrown by fetchCdpChecked and fetchBrowserJson includes "rate limit" in the message. This avoids false positives from errors containing "429" in non-rate-limit contexts (e.g. "timed out after 4290ms").

P2 - Inconsistent rate-limit messages: Exported BROWSER_RATE_LIMIT_MESSAGE from client-fetch.ts and imported it in cdp.helpers.ts to replace the hand-rolled message, ensuring consistent wording across all 429 error paths.

This new 429 path reflects upstream response text directly into the thrown error in src/browser/cdp.helpers.ts:176-180. That text is provider-controlled for remote CDP/browser endpoints, so it should not be embedded verbatim in the error surface.

That message is later logged and returned to the tool caller unchanged in src/agents/pi-tool-definition-adapter.ts:179-188, which creates an avoidable log / agent-output injection surface. It also reads the full body before slicing, so I think this should use a stable message instead, or at most a bounded and sanitized snippet / request-id style metadata.

Good catch, @altaywtf - you're right that reflecting upstream response text verbatim into the error surface is an unnecessary injection risk.

Fixed in 490b675: removed the ${detail} interpolation from all three 429 error paths (cdp.helpers.ts and both client-fetch.ts callers). The stable BROWSER_RATE_LIMIT_MESSAGE already tells the user/agent exactly what happened - the provider-controlled body text added no actionable information.

@mvanhorn sir... your claude is not running format before pushing commits 😅

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Undrained fetch Response body on HTTP 429 in fetchCdpChecked can exhaust connections (DoS)

Description

fetchCdpChecked throws on HTTP 429 without consuming or cancelling the response body.

In Node.js (v22+), fetch is backed by Undici. Undici will generally not reuse/release the underlying connection back to the pool until the response body is fully consumed or the body stream is cancelled. If many 429s occur (e.g., upstream rate limiting), this can lead to:

• connection pool exhaustion / socket leak (connections stuck in-use)
• increased memory usage from uncollected streams
• eventual denial of service (subsequent fetches hang/fail)

This risk is amplified because fetchCdpChecked is used by CDP polling paths (e.g., reachability checks) and may be called repeatedly in loops; repeated 429s can accumulate leaked/undrained bodies.

Vulnerable code:

if (!res.ok) {
  if (res.status === 429) {
    ​// Do not reflect upstream response text into the error surface (log/agent injection risk)
    throw new Error(`${resolveBrowserRateLimitMessage(url)} Do NOT retry the browser tool.`);
  }
  throw new Error(`HTTP ${res.status}`);
}

Note: this repo already introduced a best-effort body cancellation helper (discardResponseBody) for the same 429 scenario in src/browser/client-fetch.ts, suggesting this is an expected mitigation but was missed here.

Recommendation

Before throwing, cancel (or drain) the response body so the underlying socket can be reused.

Minimal safe fix (best-effort cancel) for the new 429 branch:

if (!res.ok) {
  if (res.status === 429) {
    try {
      await res.body?.cancel();
    } catch {
      ​// best-effort
    }
    throw new Error(`${resolveBrowserRateLimitMessage(url)} Do NOT retry the browser tool.`);
  }
  throw new Error(`HTTP ${res.status}`);
}

Better: apply the same pattern for all non-OK responses (either await res.text().catch(() => "") without reflecting it, or await res.body?.cancel()), to prevent similar leaks on other error statuses.

Analyzed PR: #40491 at commit 13839c2

_{Last updated on: 2026-03-10T22:03:48Z}

Merged via squash.

• Prepared head SHA: 13839c2dbd0396d8a582aa2c5c206d2efaff1b07
• Merge commit: 5ed96da9906a50551bfcb0827b3c5edcad69fa81

Thanks @mvanhorn!