Conversation
🦋 Changeset detectedLatest commit: 2eb5d4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ben-reitz
added this pull request to stack #2373
September 25, 2026 08:21
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
ben-reitz
marked this pull request as ready for review
September 25, 2026 08:49
ben-reitz
force-pushed
the
fix/browser-connect-expiry-race
branch
from
September 25, 2026 09:13
5fefaea to
acda0cb
Compare
ben-reitz
force-pushed
the
fix/browser-connect-expiry-race
branch
2 times, most recently
from
September 25, 2026 11:27
acda0cb to
5fefaea
Compare
Contributor
|
✅ agents import sizes: no significant changes ( |
ben-reitz
force-pushed
the
fix/browser-connect-expiry-race
branch
8 times, most recently
from
September 28, 2026 12:29
213b48c to
e339ad8
Compare
ben-reitz
removed this pull request from stack #2373
September 29, 2026 13:34
ben-reitz
force-pushed
the
fix/browser-connect-expiry-race
branch
from
September 29, 2026 13:35
e339ad8 to
2eb5d4a
Compare
ben-reitz
added this pull request to stack #2412
September 29, 2026 13:36
Comment on lines
257
to
+269
| async connect(): Promise<BrowserConnection> { | ||
| const resolved = await this.resolve(); | ||
| try { | ||
| return await this.#attach(resolved); | ||
| } catch (error) { | ||
| if (!isMissingBrowserSession(error)) throw error; | ||
| await this.#retireIfCurrent(resolved.sessionId); | ||
| } | ||
| // The browser this caller resolved is gone, so report a restart even | ||
| // when a concurrent resolver already replaced it and this resolve | ||
| // reattaches to that replacement. | ||
| const replaced = await this.resolve(); | ||
| return this.#attach({ ...replaced, restarted: true }); |
Contributor
There was a problem hiding this comment.
🔍 Connector remains outside the retry
The retry applies to Browser.connect(), but BrowserConnector calls connectBrowserSession() directly. Its probe-to-upgrade race remains, despite the PR description naming the connector as a beneficiary.
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: fixes a race in connecting to the browser.
Browser.connect()gives the host a working browser. It does two steps:resolve()checks whether the browser is still alive, and creates a replacement if it isn't.connect()then opens a CDP WebSocket to the resolved browser.The persistent browser tool later in this stack uses it.
connect()could throw if a browser expired afterresolve()'s liveness probe but before the WebSocket upgrade. It now replaces the browser, the same wayresolve()does. This fixes the Devin thread on #2302.restarted: true.connectBrowserSessionnow throwsBrowserRenderingError, with the HTTP status in the message, instead of a plainError. It still extendsError, so existingcatchblocks keep working.Tests in
browser.test.ts: a 410 on upgrade returns a replacement; a 502 is not retried.Architecture diff
Each outlined box is a component this PR changes, with one green box per change. Grey boxes are unchanged.
flowchart TB classDef ctx fill:none,stroke:#8c959f,color:#8c959f classDef changes fill:#dafbe1,stroke:#1a7f37,color:#1f2328 Agent["Agent (host Durable Object)"]:::ctx subgraph B["Browser · browser.ts"] direction TB N1["+ connect() retries once on a 404/410 upgrade"]:::changes N2["+ retires the dead record only if it's still current"]:::changes N1 ~~~ N2 end subgraph Run["connectBrowserSession · browser-run.ts"] R1["+ throws BrowserRenderingError with the upgrade status"]:::changes end Store["StoredBrowserSession · session-store.ts"]:::ctx BR["Browser Run"]:::ctx Agent -->|lifecycle.use| B B -->|"one record"| Store B -->|"open CDP socket"| Run Run -->|"WebSocket upgrade"| BR style B stroke:#1a7f37,stroke-width:2px style Run stroke:#1a7f37,stroke-width:2px