fix(core): clean up failed initialization#2396
Open
GautamSharma99 wants to merge 1 commit into
Open
Conversation
🦋 Changeset detectedLatest commit: f599b34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
Contributor
|
This PR is from an external contributor and must be approved by a stagehand team member with write access before CI can run. |
github-actions
Bot
added
external-contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 10 files
Confidence score: 2/5
- In
packages/core/lib/v3/v3.ts, session-ownership checks are inconsistent across close and crash-shutdown paths, so caller-owned Browserbase sessions can be released unexpectedly; this can break upstream session lifecycle assumptions and cause hard-to-debug outages — gate all release/supervisor paths onownsSessionconsistently. - In
packages/core/lib/v3/v3.ts, failed Stagehand API end responses are treated as successful cleanup, which skips the Browserbase release fallback on HTTP errors and can leave sessions orphaned — only setendedViaApiafter a confirmed success response. - In
packages/core/lib/v3/v3.ts, an emptybrowserbaseSessionIDis treated as caller-owned even though launch treats it as missing and creates a new session, so that new session can leak on failure/close — align ownership detection with launcher semantics for empty IDs. - In
packages/core/lib/v3/launch/browserbase.ts, malformed-session cleanup has no timeout, so a stalled release call can hangcreateBrowserbaseSession()and mask the intended invalid-response error — apply the same session timeout to the cleanup release request.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/launch/browserbase.ts">
<violation number="1" location="packages/core/lib/v3/launch/browserbase.ts:76">
P2: Malformed-session cleanup is unbounded, so a stalled release request can hang `createBrowserbaseSession()` and prevent the promised invalid-response error from surfacing. Applying the same session timeout to the release request would keep cleanup best-effort and bounded.</violation>
</file>
<file name="packages/core/lib/v3/v3.ts">
<violation number="1" location="packages/core/lib/v3/v3.ts:1073">
P2: An empty `browserbaseSessionID` is classified as caller-owned even though the launcher treats it as absent and creates a new session, leaking that newly created session on failure or close. The ownership check should use the same truthiness rule as the resume path, or the option should be rejected before launch.</violation>
<violation number="2" location="packages/core/lib/v3/v3.ts:1161">
P1: A process crash can still release caller-owned Browserbase sessions: the shutdown supervisor is configured without consulting `ownsSession`. The supervisor should only be started for a session created by this initialization, or its configuration should carry ownership and skip release for caller-supplied sessions.</violation>
<violation number="3" location="packages/core/lib/v3/v3.ts:1652">
P1: Closing a V3 instance with a caller-supplied Browserbase session still ends that session whenever API mode is enabled. The ownership guard only applies to failed-initialization cleanup; using `this.ownsBrowserbaseSession` as the normal `apiClient.end()` guard would preserve caller-owned sessions.</violation>
<violation number="4" location="packages/core/lib/v3/v3.ts:1656">
P1: An unsuccessful Stagehand API end response is treated as successful cleanup, so the Browserbase release fallback never runs on HTTP failures. Checking the returned response and only setting `endedViaApi` for a successful status would preserve the fallback path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant C as Client
participant Init as V3.init()
participant Launch as launchLocalChrome
participant Cleanup as cleanupLocalBrowser
participant State as V3.state
participant Ctx as V3Context.create
participant CDP as CdpConnection
participant BB as BrowserbaseSession
participant API as StagehandAPI
participant Close as V3.close
C->>Init: init()
alt env === "LOCAL"
Init->>Launch: launchLocalChrome(opts)
alt Launch succeeds
Launch-->>Init: { ws, chrome }
Init->>State: assign LOCAL state (chrome, ws, profile, createdTempProfile)
Note over Init,State: Ownership transferred to V3.state<br/>BEFORE context creation
Init->>Ctx: V3Context.create(ws)
alt Context creation fails
Ctx-->>Init: throw
Note over Init: Init catch block
Init->>Close: close(force, cleanupOwnedResources)
Close->>Close: Kill Chrome, remove temp profile<br/>Unbind logger, destroy store,<br/>remove registry entry
Close-->>Init: done
Init-->>C: rethrow original error
else Context succeeds
Ctx-->>Init: V3Context instance
Init-->>C: success
end
else Launch fails (debugger discovery)
Launch-->>Init: throw
Init->>Cleanup: cleanupLocalBrowser (remove temp profile)
Cleanup-->>Init: done
Init-->>C: rethrow
end
else env === "BROWSERBASE"
Init->>BB: createBrowserbaseSession(...)
alt Response malformed (sessionId but no connectUrl)
BB-->>Init: { id: "sess1" }
Init->>BB: sessions.update(id, REQUEST_RELEASE)
BB-->>Init: ok
Init-->>C: throw StagehandInitError
else Valid response
BB-->>Init: { id, connectUrl, bb }
Init->>State: assign BROWSERBASE state (sessionId, ws, bb, ownsSession)
Note over Init,State: Ownership (ownsSession) set BEFORE context
Init->>Ctx: V3Context.create(ws)
alt Context creation fails
Ctx-->>Init: throw
Init->>Close: close(force, cleanupOwnedResources)
alt ownsSession === true
Close->>API: end() (preferred)
alt API end fails
API-->>Close: error
Close->>BB: sessions.update(sessionId, REQUEST_RELEASE)
end
else ownsSession === false (caller-supplied)
Note over Close: Do not release caller-owned session
end
Close->>Close: unbind logger, destroy store,<br/>remove registry entry
Close-->>Init: done
Init-->>C: rethrow
else Context succeeds
Ctx-->>Init: V3Context instance
Init-->>C: success
end
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ) { | ||
| try { | ||
| await this.apiClient.end(); | ||
| endedViaApi = true; |
Contributor
There was a problem hiding this comment.
P1: An unsuccessful Stagehand API end response is treated as successful cleanup, so the Browserbase release fallback never runs on HTTP failures. Checking the returned response and only setting endedViaApi for a successful status would preserve the fallback path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 1656:
<comment>An unsuccessful Stagehand API end response is treated as successful cleanup, so the Browserbase release fallback never runs on HTTP failures. Checking the returned response and only setting `endedViaApi` for a successful status would preserve the fallback path.</comment>
<file context>
@@ -1615,10 +1644,31 @@ export class V3 {
+ ) {
try {
await this.apiClient.end();
+ endedViaApi = true;
+ } catch {
+ // best-effort cleanup
</file context>
| sessionId, | ||
| ws, | ||
| bb, | ||
| ownsSession: this.ownsBrowserbaseSession, |
Contributor
There was a problem hiding this comment.
P1: A process crash can still release caller-owned Browserbase sessions: the shutdown supervisor is configured without consulting ownsSession. The supervisor should only be started for a session created by this initialization, or its configuration should carry ownership and skip release for caller-supplied sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 1161:
<comment>A process crash can still release caller-owned Browserbase sessions: the shutdown supervisor is configured without consulting `ownsSession`. The supervisor should only be started for a session created by this initialization, or its configuration should carry ownership and skip release for caller-supplied sessions.</comment>
<file context>
@@ -1136,13 +1151,15 @@ export class V3 {
+ sessionId,
+ ws,
+ bb,
+ ownsSession: this.ownsBrowserbaseSession,
+ };
this.browserbaseSessionId = sessionId;
</file context>
| if ( | ||
| !preserveOwnedResources && | ||
| this.apiClient && | ||
| (!opts?.cleanupOwnedResources || this.ownsBrowserbaseSession) |
Contributor
There was a problem hiding this comment.
P1: Closing a V3 instance with a caller-supplied Browserbase session still ends that session whenever API mode is enabled. The ownership guard only applies to failed-initialization cleanup; using this.ownsBrowserbaseSession as the normal apiClient.end() guard would preserve caller-owned sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 1652:
<comment>Closing a V3 instance with a caller-supplied Browserbase session still ends that session whenever API mode is enabled. The ownership guard only applies to failed-initialization cleanup; using `this.ownsBrowserbaseSession` as the normal `apiClient.end()` guard would preserve caller-owned sessions.</comment>
<file context>
@@ -1615,10 +1644,31 @@ export class V3 {
+ if (
+ !preserveOwnedResources &&
+ this.apiClient &&
+ (!opts?.cleanupOwnedResources || this.ownsBrowserbaseSession)
+ ) {
try {
</file context>
Suggested change
| (!opts?.cleanupOwnedResources || this.ownsBrowserbaseSession) | |
| + this.ownsBrowserbaseSession |
| if (!created?.connectUrl || !created?.id) { | ||
| if (created?.id) { | ||
| try { | ||
| await bb.sessions.update(created.id, { |
Contributor
There was a problem hiding this comment.
P2: Malformed-session cleanup is unbounded, so a stalled release request can hang createBrowserbaseSession() and prevent the promised invalid-response error from surfacing. Applying the same session timeout to the release request would keep cleanup best-effort and bounded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/launch/browserbase.ts, line 76:
<comment>Malformed-session cleanup is unbounded, so a stalled release request can hang `createBrowserbaseSession()` and prevent the promised invalid-response error from surfacing. Applying the same session timeout to the release request would keep cleanup best-effort and bounded.</comment>
<file context>
@@ -68,9 +68,19 @@ export async function createBrowserbaseSession(
if (!created?.connectUrl || !created?.id) {
+ if (created?.id) {
+ try {
+ await bb.sessions.update(created.id, {
+ status: "REQUEST_RELEASE",
+ ...(resolvedProjectId ? { projectId: resolvedProjectId } : {}),
</file context>
| if (this.opts.env === "BROWSERBASE") { | ||
| const { apiKey, projectId } = this.requireBrowserbaseCreds(); | ||
| const browserbaseSessionWasSupplied = | ||
| this.opts.browserbaseSessionID !== undefined; |
Contributor
There was a problem hiding this comment.
P2: An empty browserbaseSessionID is classified as caller-owned even though the launcher treats it as absent and creates a new session, leaking that newly created session on failure or close. The ownership check should use the same truthiness rule as the resume path, or the option should be rejected before launch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 1073:
<comment>An empty `browserbaseSessionID` is classified as caller-owned even though the launcher treats it as absent and creates a new session, leaking that newly created session on failure or close. The ownership check should use the same truthiness rule as the resume path, or the option should be rejected before launch.</comment>
<file context>
@@ -1057,6 +1069,9 @@ export class V3 {
if (this.opts.env === "BROWSERBASE") {
const { apiKey, projectId } = this.requireBrowserbaseCreds();
+ const browserbaseSessionWasSupplied =
+ this.opts.browserbaseSessionID !== undefined;
+ this.ownsBrowserbaseSession = !browserbaseSessionWasSupplied;
this.logger({
</file context>
Suggested change
| this.opts.browserbaseSessionID !== undefined; | |
| + Boolean(this.opts.browserbaseSessionID); |
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
1 participant
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.
Summary
Initialization previously had several resource-ownership gaps where an error could occur after acquiring a resource but before publishing it
to
V3.state.In those cases, the outer initialization handler could not find or clean up the resource. Depending on where initialization failed, this could
leave behind:
This PR gives each acquisition stage structured cleanup and transfers ownership to
V3before subsequent asynchronous work can fail.What changed
Kill Chrome when debugger discovery fails
launchLocalChrome()now wraps debugger URL discovery and WebSocket readiness checks in a cleanup boundary.If either step fails after
chrome-launcherhas returned:chrome.kill()is called;Previously, failures while polling
/json/versionor probing the CDP WebSocket left the launched Chrome process running.Remove generated profiles when launch fails
V3.init()creates temporary browser profiles before callinglaunchLocalChrome().The launch call is now wrapped so that when it rejects:
preserveUserDataDirremains respected;This complements process cleanup inside
launchLocalChrome(), which does not know whether a profile was generated by Stagehand or supplied bythe caller.
Publish local browser ownership immediately
After
launchLocalChrome()succeeds, the local Chrome handle, WebSocket URL, profile path, and profile ownership metadata are assigned toV3.statebefore creating the CDP context.The shutdown supervisor is also started immediately after ownership transfer when applicable.
If
V3Context.create()or another later initialization step fails, the outer cleanup can now see and clean:Previously, state assignment happened after context creation, leaving
close()with anUNINITIALIZEDstate during that failure window.Close CDP connections after bootstrap failure
V3Context.create()now owns the connection as soon asCdpConnection.connect()succeeds.Context construction and bootstrap run inside a
try/catch. If auto-attach, target discovery, initial-page setup, or another bootstrap stagefails, the context or raw connection is closed before the original error is rethrown.
This prevents a failed context bootstrap from leaving an open WebSocket and its event listeners behind.
Publish Browserbase ownership before CDP setup
Browserbase session state is now assigned before
V3Context.create().The state records whether the session was:
When later initialization fails:
REQUEST_RELEASE;Malformed Browserbase creation responses that contain a session ID but no connect URL are also released before reporting the invalid response.
Fully clean failed V3 instances
The outer
init()catch now invokes complete V3 cleanup instead of only conditionally unbinding an external logger.Failure cleanup now covers:
The original initialization exception remains the error returned to the caller, even if best-effort cleanup encounters another failure.
Correct logger registration lifetime
Instance logger registration has been moved to the end of the constructor, after constructor work that may throw.
Both logger variants are now handled consistently:
StagehandLoggerfallback.This avoids registering a logger for an object whose constructor never completed and ensures failed initialization always unbinds whichever
logger was installed.
Preserve successful keep-alive behavior
Initialization failure cleanup overrides
keepAlivefor resources Stagehand acquired during that failed attempt. A failed initializationshould not leave an unusable process or session alive.
Normal
close()behavior still respectskeepAlive, and explicitly supplied Browserbase sessions remain caller-owned during failure cleanup.Tests added
Added focused fault-injection coverage for the affected acquisition windows.
Local launch cleanup
Verifies that:
keepAlive: true.CDP context cleanup
Verifies that:
V3 lifecycle cleanup
Verifies that failed initialization:
Browserbase cleanup
Verifies that:
keepAlive: trueduring failed initialization;Validation
Passed:
The full core unit run passes 887/891 tests. The remaining four failures are the existing
flowlogger-eventstorestderr-capture failures,which reproduce independently and are unrelated to this change.
The test runner also prints the existing Node 24
glob/minimatchCTRF-conversion error after test execution; it does not affect the passingtest results.
Release impact
Patch release.
No successful initialization or public response behavior changes. The primary behavioral change is that resources acquired during an
initialization attempt are now released if that attempt fails.
Summary by cubic
Ensures all resources acquired during initialization are cleaned up on failure, preventing stray Chrome processes, temp profiles, CDP sockets, Browserbase sessions, loggers, and registry entries. Successful runs are unchanged; failures now perform best-effort cleanup and return the original error.
V3.statebefore CDP; start shutdown supervisor earlier.V3Context.create()bootstrap fails.REQUEST_RELEASEif API end fails.close({ force: true, cleanupOwnedResources: true }); ignores secondary cleanup errors;keepAliveis ignored for resources acquired during a failed init.Written for commit f599b34. Summary will update on new commits.