feat(agents): add the named browser session core - #2302
Conversation
|
🟡 agents import sizesMeasured 344 runtime imports as minified bundles. The primary size is gzip; raw minified size is included for diagnosis. An existing import growing by more than 10% is marked red. This report is informational.
Compared Changed imports (10)
All 344 current runtime imports
Reported by agent-think[bot]. |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
5b4dad0 to
438e007
Compare
| keepAliveMs?: number; | ||
| includeTargets?: boolean; | ||
| recording?: boolean; | ||
| guardrails?: BrowserSessionGuardrails; |
There was a problem hiding this comment.
If nothing is passed, is everything allowed or nothing?
There was a problem hiding this comment.
Follows the default Browser Run behaviour of allowing all if nothing is passed.
aron-cf
left a comment
There was a problem hiding this comment.
I think a usage example would help in the PR description. My understanding of this is it means that we can recover an existing browser session across executions?
|
|
||
| /** The store key for a named browser session. */ | ||
| export function namedBrowserSessionKey(name: string): string { | ||
| return `${NAMED_SESSION_KEY_PREFIX}${name}`; |
There was a problem hiding this comment.
Does name need to be validated somewhere?
There was a problem hiding this comment.
It's set in the host code, and only used in the session store so never reaches anything dangerous. I think the only limit it could break is the DO storage key max size which is massive and would break loudly.
we could iterate on this later if/when we start letting models or users influence names - that's when we might want to validate a more sensible length cap or check chars are printable etc.
| * (and before it prunes a tombstone). Defaults to | ||
| * {@link DEFAULT_SWEEP_IDLE_MS} (10 minutes). | ||
| */ | ||
| sweepIdleMs?: number; |
There was a problem hiding this comment.
This is weird that this exists here because the session doesn't actually handle the timeout, it's just here as metadata for the sweep method. I think it'd be cleaner to pass it into sweep(since: number) in the lifecycle capability.
There was a problem hiding this comment.
I actually moved both sweepIdleMs and touchIntervalMs to be a property of the session itself to fix a bug that occurred when the touchIntervalMs < sweepIdleMs.
This diagram hopefully explains it better than i can in words, but in essence, we have to keep track of when we last used the CDP connection between the agent and the browser, but we also need to throttle how often we write that message to the store (using touchIntervalMs). That throttle means if the sweep happens more often than the touchIntervalMs, the CDP connection could be in active use, but the browser will still get swept up if touchIntervalMs < sweepIdleMs
flowchart LR
subgraph W["WRITER: every open connection (connect())"]
A[CDP message<br/>sent or received] --> B{last touch<br/>older than<br/>touchIntervalMs?}
B -- no --> X[skip]
B -- yes --> C[touch:<br/>updatedAt = now]
end
C --> S[("store<br/>browser:session:checkout<br/>updatedAt")]
subgraph R["READER: sweep() on the Lifecycle alarm"]
D{now − updatedAt<br/>≥ sweepIdleMs?} -- yes --> E[close browser<br/>+ tombstone]
D -- no --> F[leave it]
end
S --> D
| /** | ||
| * Minimum interval between activity-driven `updatedAt` refreshes on | ||
| * connected sockets. Defaults to {@link SESSION_TOUCH_INTERVAL_MS} and is | ||
| * always capped at half of `sweepIdleMs`, so continuous CDP traffic | ||
| * refreshes the idle clock before a sweep deadline can pass. Overridable | ||
| * primarily for tests. | ||
| */ | ||
| touchIntervalMs?: number; |
There was a problem hiding this comment.
I'm not sure I understand this one either, it looks like it exists to throttle the touch calls in the onActivity handler, but I don't follow how it connects to the sleep.
Yep that's right. I've updated the PR description to try and make it clearer. |
6c81bde to
3186292
Compare
Review question on #2302: omitting guardrails entirely follows the documented Browser Run default — all hostnames are allowed. Say so on the type instead of making every reader ask.
…ssion Review question on #2302 asked whether the conditional body existed because the acquire endpoint rejects an empty body. Verified against the live endpoint: acquire returns 200 for no body, an empty {} JSON body, and a guardrails body alike. With the concern disproven, drop the conditional and send one uniform request shape.
Five positional parameters with two trailing optional callbacks made construction sites unreadable (review feedback on #2302). The socket stays positional; timeoutMs, onClose, sessionId, and onActivity move into a named CdpSessionOptions object.
Review feedback on #2302: Chromium-only options paired with browser: "kitesurf" were only caught at runtime. ConnectBrowserOptions and OneShotBrowserSessionOptions are now engine-discriminated unions — selecting Kitesurf removes keepAliveMs/includeTargets/recording (and guardrails for one-shots) at the type level, with browser: "chromium" accepted explicitly on the default arm. Runtime guards remain for plain-JS callers, consolidated into one check with one message per entry point. New tests-d file pins the compile-time rejections; runtime smuggle tests use casts to keep exercising the guards.
| const cdp = await connectBrowserSession(this.#browser, resolved.sessionId, { | ||
| timeoutMs: this.#timeoutMs, | ||
| onActivity: () => { |
There was a problem hiding this comment.
🟡 Expiry race aborts named connection
When the browser expires after resolve, connect attaches to its stale ID and rejects instead of replacing it. Separate liveness and upgrade requests leave this expiration window.
Learn more
resolve() proves liveness with a /json/list request, then connect() performs a separate WebSocket upgrade. Browser Run can expire or reclaim the session between those requests. connectBrowserSession makes one upgrade attempt and turns a response without a WebSocket into an error, so this path never reaches the dead-session replacement logic.
Example: The liveness request for session-7 succeeds at the end of its keep-alive window. Browser Run expires it before the upgrade arrives. connect("checkout") rejects instead of returning a new session with restarted: true.
Recommended fix: Make attachment failure identify 404/410 responses, then atomically tombstone the matching stored session and retry resolution and attachment. Bound retries and avoid replacing an entry that another caller already swapped.
Was this helpful? React with 👍 or 👎 to provide feedback.
b5b8657 to
fd42849
Compare
Provider-independent named sessions over the existing store contract: - NamedBrowserSessions.resolve() is reattach-or-create keyed by browser:session:<name> — hosts wire names, models never see session identity. restarted: true signals loudly whenever a prior browser was lost (died upstream, closed, or swept); first-ever use is not a restart. - Durable creation options (keepAliveMs, recording, guardrails.allowedDomains/allowedDomainSets) are reapplied on every create so they survive the restart path. keep_alive pins to the 600-second platform max by default. - createBrowserSession() gains guardrails support — they ride the acquire POST body per the Browser Run REST contract; Kitesurf rejects them. - close() tombstones and deletes; sweep() closes idle sessions, leaves tombstones, and prunes aged ones (creator-overridable 10-minute TTL, alarm scheduling lands with the Lifecycle capability slice). - openOneShotBrowserSession() is the trivial create-and-close branch: no store, platform session deleted when the CDP socket closes. connectBrowserSession() gains an optional onClose to support it. Lock discipline mirrors BrowserConnector: locks wrap storage only, probes and Browser Run calls run outside, commits re-check for concurrent swaps (covered by tests, including a lock-across-network violation detector). Internal module — no public export changes until the starter-pack slice. Existing browser_execute behavior untouched.
Review question on #2302: omitting guardrails entirely follows the documented Browser Run default — all hostnames are allowed. Say so on the type instead of making every reader ask.
…ssion Review question on #2302 asked whether the conditional body existed because the acquire endpoint rejects an empty body. Verified against the live endpoint: acquire returns 200 for no body, an empty {} JSON body, and a guardrails body alike. With the concern disproven, drop the conditional and send one uniform request shape.
Five positional parameters with two trailing optional callbacks made construction sites unreadable (review feedback on #2302). The socket stays positional; timeoutMs, onClose, sessionId, and onActivity move into a named CdpSessionOptions object.
Review feedback on #2302: Chromium-only options paired with browser: "kitesurf" were only caught at runtime. ConnectBrowserOptions and OneShotBrowserSessionOptions are now engine-discriminated unions — selecting Kitesurf removes keepAliveMs/includeTargets/recording (and guardrails for one-shots) at the type level, with browser: "chromium" accepted explicitly on the default arm. Runtime guards remain for plain-JS callers, consolidated into one check with one message per entry point. New tests-d file pins the compile-time rejections; runtime smuggle tests use casts to keep exercising the guards.
CDP activity refreshed the idle clock only through a throttled, asynchronous store touch. A sweep that won the key lock first still saw the stale updatedAt and deleted the browser under a command that had already been sent. Record each send in memory synchronously and have the sweep's locked recheck consult it, so any command sent before that recheck keeps its browser.
…stone Pruning an aged-out tombstone removed the only record that a name had ever owned a browser, so the next resolve reported restarted: false and the caller was never told its state was gone — the common return-after-a-pause case. Move pruned tombstones to a browser:retired:<name> marker outside the swept keyspace (so the Lifecycle sweep job still retires) and treat it as restart evidence on the create path.
A superseded socket that kept sending after its session was closed, swept, or replaced re-recorded activity for a dead session id with no cleanup path. Each sweep now drops activity for ids the store no longer holds live. Also document that activity tracking is per instance: use one NamedBrowserSessions per store.
…ed overload Plain-JavaScript and prebuilt callers of new CdpSession(ws, timeoutMs, dispose, sessionId) would otherwise silently fall back to the 10s default and drop the close callback and session id. Normalize either form at runtime; the options object stays the preferred API.
fd42849 to
ff32623
Compare
What this is
A host-side session engine. You name a browser and get a live Browser Run session for it.
connect(name)=resolve+ open the CDP socket.Why
Today the model manages browser lifetime inside
browser_execute(cdp.startSession(),sessionInfo(), carrying raw ids), and evals showed that's where it fails most. This moves lifetime host-side. Nothing model-facing changes here:browser_executeis untouched, and wiring it to named sessions comes later in the stack.Behavior
sessionId; models never will.restarted: trueinstead of silently appearing blank.keepAliveMs(default: 600s platform max),recording, andguardrailsare stored and reapplied on every create.close()tombstones durably, then deletes best-effort.sweep()closes browsers idle longer thansweepIdleMs(10 min) and prunes old tombstones. Alarm scheduling is in feat(agents): add the browser-sessions Lifecycle capability #2293.restarted: true.openOneShotBrowserSession()is create-use-delete with no store, and it's the only path for Kitesurf right now.Storage uses
browser:session:<name>, separate from the connector's existingcdp:exec:/cdp:reuse:keys.Public API (
agents/browser)createBrowserSession(browser, { keepAliveMs, includeTargets, recording, + guardrails }) // acquire now always sends JSON ({} if none) -connectBrowserSession(browser, id, timeoutMs) // still works, @deprecated +connectBrowserSession(browser, id, { timeoutMs, onClose, onActivity }) -new CdpSession(ws, timeoutMs, dispose, sessionId) +new CdpSession(ws, { timeoutMs, onClose, sessionId, onActivity }) // breaking for direct construction connectBrowser(browser, options) // options: engine-discriminated union; // kitesurf rejects Chromium-only options at compile timeType exports and the changeset are in #2294.
Reviewing
session-core.tsreads top to bottom: resolve → connect → close → sweep → one-shot. 23 tests inbrowser-session-core.test.ts; compile-time option checks intests-d/browser-engine-options.test-d.ts.Extra: where this is heading
Where this is heading
Before
This is today, with
dynamicmode enabled. The model owns both the Browser Run platform session AND the CDP session:After
Target state: both Browser Run and CDP lifecycle management gone from the model's code: