Merged
Conversation
🦋 Changeset detectedLatest commit: 3887d8f 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 |
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 7 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant User as User Script
participant V3 as Stagehand V3 Instance
participant Guard as Process Guards (SIGINT/Error)
participant Local as Local Chrome Process
participant BBApi as Browserbase API / Proxy
Note over User,V3: Initialization with keepAlive: true
User->>V3: NEW: init({ keepAlive: true })
alt Launch: LOCAL
V3->>Local: Spawn Chrome
V3->>Local: NEW: unref() (Allows Node to exit independently)
Note right of Local: NEW: handleSIGINT: false (Chrome ignores Ctrl+C)
else Launch: BROWSERBASE
V3->>BBApi: NEW: Create session (keepAlive: true)
end
V3-->>User: Context Ready
Note over User,Guard: Runtime / Shutdown Scenarios
alt Scenario A: Graceful Close
User->>V3: close()
else Scenario B: Process Signal/Error
Guard->>V3: _immediateShutdown(reason)
end
V3->>V3: Check keepAlive status
alt keepAlive is TRUE
Note over V3,Local: Local: Skip chrome.kill() and profile deletion
Note over V3,BBApi: Remote: Skip apiClient.end()
V3-->>User: Resolve (Browser stays open)
else keepAlive is FALSE (Default)
V3->>Local: CHANGED: Kill Chrome & delete temp profile
V3->>BBApi: CHANGED: apiClient.end() / terminate session
V3-->>User: Resolve (Browser closed)
end
opt Scenario B Error Path
Guard->>User: Re-throw original Exception/Rejection
end
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add optional boolean keepAlive parameter in stagehand constructor params (default false) | Added V3Options keepAlive and this.keepAlive resolution |
| ✅ | Constructor keepAlive overrides browserbaseSessionCreateParams.keepAlive when both provided | Constructor resolves opts.keepAlive ?? sessionCreateParams.keepAlive |
| ✅ | Update local launch to pass chrome launcher handleSIGINT based on keepAlive | Added handleSIGINT param and pass !keepAlive from V3 |
| ✅ | Unref chrome child process after launch so Node can exit while chrome stays open when keepAlive true | Calls chrome.process?.unref() when keepAlive is true |
| ✅ | stagehand.close() skips apiClient.end() when keepAlive true | shutdownBrowserSession avoids endApiClient when keepAlive |
| ✅ | keepAlive:true keeps browser alive on uncaught error, SIGTERM/SIGINT, and explicit stagehand.close() | Shutdown guards and shutdown helpers honor keepAlive |
| ✅ | Propagate resolved keepAlive to Browserbase session creation (constructor override respected) | Builds effectiveSessionParams including resolved keepAlive |
| ✅ | Refactor shutdown into focused modules that honor keepAlive and use a 4s hard timeout | Added shutdown modules and 4000ms hard timeout in guards |
| ✅ | Add tests validating behavior across scenarios (unhandledRejection/uncaughtException, SIGTERM/SIGINT, stagehand.close()) and envs (local, bb direct WS, bb via API) | Playwright tests and helper/child scripts added for scenarios/envs |
| ✅ | Document keepAlive in public types with note it overrides session param | Types file includes doc comment noting override behavior |
Member
Author
Contributor
Greptile OverviewGreptile SummaryThis PR adds a new Key Changes:
Architecture: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant V3
participant Chrome/Browserbase
participant Supervisor
User->>V3: new V3({keepAlive: false})
User->>V3: init()
V3->>Chrome/Browserbase: launch/connect
Chrome/Browserbase-->>V3: ws URL
V3->>Supervisor: spawn detached process
V3->>Supervisor: send config via IPC
Supervisor->>Supervisor: start PID polling
Supervisor-->>V3: ready acknowledgement
V3-->>User: init complete
Note over User,Supervisor: Normal operation
alt keepAlive: false (parent crashes)
User->>V3: crash/signal/error
Note over V3: parent dies
Supervisor->>Supervisor: stdin/IPC closes
Supervisor->>Chrome/Browserbase: kill Chrome / REQUEST_RELEASE
Supervisor->>Supervisor: cleanup temp profile
else keepAlive: true (parent crashes)
User->>V3: crash/signal/error
Note over V3: parent dies
Note over Chrome/Browserbase: remains running
Note over Supervisor: not spawned
else keepAlive: false (normal close)
User->>V3: close()
V3->>Supervisor: send exit message
V3->>Chrome/Browserbase: kill Chrome / end session
V3->>V3: cleanup resources
else keepAlive: true (normal close)
User->>V3: close()
Note over V3: skip browser shutdown
V3->>V3: cleanup resources only
Note over Chrome/Browserbase: remains running
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Additional Comments (1)
|
Member
Author
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
3 issues found across 10 files
Confidence score: 2/5
- Signal handling in
packages/core/lib/v3/shutdown/shutdownGuards.tscan leave the process hanging indefinitely after SIGINT/SIGTERM because default exit is overridden without a finalprocess.exit(), which is a concrete user-facing shutdown failure. - Repeated SIGINT/SIGTERM are swallowed in
packages/core/lib/v3/shutdown/shutdownGuards.ts, preventing users from force-exiting during a stuck shutdown, compounding the risk. - There’s a race in
packages/core/lib/v3/v3.tswhereshutdownBrowserSessionruns before the_isClosingguard, so concurrentclose()calls can conflict during signal-triggered shutdown. - Pay close attention to
packages/core/lib/v3/shutdown/shutdownGuards.tsandpackages/core/lib/v3/v3.ts- shutdown/close sequencing can hang or race.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/shutdown/shutdownGuards.ts">
<violation number="1" location="packages/core/lib/v3/shutdown/shutdownGuards.ts:33">
P1: After SIGINT/SIGTERM handling, the process will hang indefinitely. `process.on('SIGINT')` overrides Node's default exit behavior, but after shutdown completes (or the 4s hard timeout fires), neither `process.exit()` nor signal re-emission occurs. In a browser automation context there will almost certainly be active handles (WebSocket connections, etc.) keeping the event loop alive.
The `uncaughtException`/`unhandledRejection` handlers correctly re-throw via `setImmediate` to crash the process, but the signal handlers lack an equivalent exit mechanism.</violation>
<violation number="2" location="packages/core/lib/v3/shutdown/shutdownGuards.ts:44">
P2: Repeated SIGINT/SIGTERM signals are silently swallowed, preventing force-exit. Since `process.on` (not `process.once`) is used and the `shuttingDown` guard short-circuits on subsequent signals, the user cannot force-kill the process with a second Ctrl+C if shutdown stalls. The common convention is to force-exit on the second signal.</violation>
</file>
<file name="packages/core/lib/v3/v3.ts">
<violation number="1" location="packages/core/lib/v3/v3.ts:1348">
P2: Race condition: `shutdownBrowserSession` is called before the re-entry guard (`_isClosing` check). If `close()` is called concurrently (e.g., user calls `close()` while signal handler triggers `_immediateShutdown`), both invocations can reach `shutdownBrowserSession` before either sets `_isClosing = true`, potentially calling `apiClient.end()` twice on an already-ended client.
Move `shutdownBrowserSession` call after the re-entry guard or after setting `_isClosing = true` to prevent duplicate API client shutdown attempts.</violation>
</file>
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add optional boolean keepAlive parameter in stagehand constructor params (default false) | Added V3Options keepAlive and this.keepAlive resolution |
| ✅ | Constructor keepAlive overrides browserbaseSessionCreateParams.keepAlive when both provided | Constructor resolves opts.keepAlive ?? sessionCreateParams.keepAlive |
| ✅ | Update local launch to pass chrome launcher handleSIGINT based on keepAlive | Added handleSIGINT param and pass !keepAlive from V3 |
| ✅ | Unref chrome child process after launch so Node can exit while chrome stays open when keepAlive true | Calls chrome.process?.unref() when keepAlive is true |
| ✅ | stagehand.close() skips apiClient.end() when keepAlive true | shutdownBrowserSession avoids endApiClient when keepAlive |
| ✅ | keepAlive:true keeps browser alive on uncaught error, SIGTERM/SIGINT, and explicit stagehand.close() | Shutdown guards and shutdown helpers honor keepAlive |
| ✅ | Propagate resolved keepAlive to Browserbase session creation (constructor override respected) | Builds effectiveSessionParams including resolved keepAlive |
| ✅ | Refactor shutdown into focused modules that honor keepAlive and use a 4s hard timeout | Added shutdown modules and 4000ms hard timeout in guards |
| ✅ | Add tests validating behavior across scenarios (unhandledRejection/uncaughtException, SIGTERM/SIGINT, stagehand.close()) and envs (local, bb direct WS, bb via API) | Playwright tests and helper/child scripts added for scenarios/envs |
| ✅ | Document keepAlive in public types with note it overrides session param | Types file includes doc comment noting override behavior |
Architecture diagram
sequenceDiagram
participant User
participant V3 as Stagehand V3
participant SG as Shutdown Guards
participant LC as Local Chrome
participant BB as Browserbase API
Note over User,BB: Initialization Phase
User->>V3: new V3({ keepAlive: true/false })
V3->>SG: NEW: installShutdownGuards()
alt Local Browser
V3->>LC: CHANGED: launchLocalChrome(handleSIGINT: !keepAlive)
opt NEW: keepAlive is true
V3->>LC: NEW: process.unref() (Allow Node to exit)
end
else Browserbase
V3->>BB: CHANGED: createSession(keepAlive)
end
Note over User,BB: Termination Flow (Signal, Error, or close())
alt Signal (SIGINT/SIGTERM)
SG->>V3: NEW: shutdownAll(reason)
else Explicit Close
User->>V3: close()
end
V3->>V3: NEW: shutdownStagehandResources() (CDP/Logs)
alt NEW: keepAlive is FALSE (Default)
alt Local
V3->>LC: CHANGED: Kill process & cleanup UserDataDir
else Browserbase
V3->>BB: CHANGED: apiClient.end()
end
else NEW: keepAlive is TRUE
Note over V3,LC: Skip process kill / profile deletion
Note over V3,BB: Skip apiClient.end()
end
V3->>V3: NEW: finalizeStagehandShutdown() (Reset internal state)
alt SIGINT/SIGTERM with keepAlive
SG->>SG: NEW: Hold event loop (4s max) for clean state reset
else Uncaught Exception
SG->>User: Re-throw error after best-effort resource cleanup
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
Member
Author
Contributor
There was a problem hiding this comment.
cubic analysis
2 issues found across 10 files
Confidence score: 3/5
- Potential for concurrent
shutdownAllcalls becauseshuttingDownonly guards SIGINT/SIGTERM paths inpackages/core/lib/v3/shutdown/shutdownGuards.ts, which could lead to racey or duplicated shutdown behavior. - Exception/rejection handlers in
packages/core/lib/v3/shutdown/shutdownGuards.tslack the hard timeout used by signal handlers, so a hungshutdownAllmay cause the process to hang indefinitely. - These are medium-severity runtime risks in shutdown paths, so some caution is warranted before merging.
- Pay close attention to
packages/core/lib/v3/shutdown/shutdownGuards.ts- shared shutdown guard and timeout coverage.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/shutdown/shutdownGuards.ts">
<violation number="1" location="packages/core/lib/v3/shutdown/shutdownGuards.ts:51">
P1: Exception/rejection handlers lack the hard timeout that signal handlers have. If `shutdownAll` hangs (e.g., broken CDP connection), the process will hang indefinitely because the re-throw in `.finally()` only fires after the promise settles. Consider wrapping these calls in `runShutdownWithKeepAlive` (without signal re-emission) or applying a `Promise.race` with a timeout, similar to the signal path.</violation>
<violation number="2" location="packages/core/lib/v3/shutdown/shutdownGuards.ts:86">
P1: Race condition: `shuttingDown` guard is not shared between signal and exception/rejection handlers, allowing concurrent `shutdownAll` calls.
The `shuttingDown` flag only guards SIGINT/SIGTERM paths (via `startShutdown`), but `onUncaughtException` and `onUnhandledRejection` bypass it entirely, calling `opts.shutdownAll()` directly. If a signal and an exception fire in quick succession, `shutdownAll` can run concurrently — potentially double-closing browser connections or API clients.
Consider checking (and setting) `shuttingDown` at the top of all four handlers, or extracting a single entry-point that all handlers route through.</violation>
</file>
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add optional boolean keepAlive parameter in the Stagehand V3 constructor params (defaults to false). | Added keepAlive option and constructor resolves it |
| ✅ | Constructor keepAlive overrides browserbaseSessionCreateParams.keepAlive when both are provided. | Constructor uses opts.keepAlive ?? browserbaseSessionCreateParams.keepAlive |
| ✅ | Update local launch to pass Chrome launcher's handleSIGINT based on keepAlive (v3.ts and local.ts) to prevent Chrome being killed on Ctrl+C when keepAlive:true. | Added handleSIGINT param and v3 passes !keepAlive |
| ✅ | Unref() the Chrome child process after launch so Node can exit while Chrome stays open when keepAlive:true. | Calls chrome.process?.unref() when keepAlive true |
| ✅ | stagehand.close() skips apiClient.end() when keepAlive:true. | close() delegates to shutdownBrowserSession which returns early if keepAlive |
| ✅ | Setting keepAlive:true keeps browser alive for uncaught errors (unhandledRejection/uncaughtException). | Installed guards for unhandled errors and shutdown skips browser teardown |
| ✅ | Setting keepAlive:true keeps browser alive for SIGTERM/SIGINT. | SIGINT/SIGTERM guards + local handleSIGINT logic and shutdown skip |
| ✅ | Setting keepAlive:true keeps browser alive when user explicitly calls stagehand.close(). | close() uses shutdownBrowserSession which returns early on keepAlive |
| ✅ | Propagate keepAlive into browserbase session creation so browserbase sessions honor keepAlive. | Effective session params include resolved keepAlive when set |
| ✅ | Split teardown into shutdownBrowser and shutdownStagehand modules, install process guards with a 4s hard timeout and keep-alive interval. | Added shutdown modules and guards with 4000ms hard timeout |
| ✅ | Add tests validating keepAlive behavior across scenarios: unhandledRejection/uncaughtException, SIGTERM/SIGINT, stagehand.close(), and across browser configs (local, browserbase WS, browserbase via API). | Comprehensive test files covering scenarios and envs were added |
Architecture diagram
sequenceDiagram
participant User as User / Node Process
participant Guards as ShutdownGuards
participant V3 as Stagehand V3
participant S_Browser as shutdownBrowser.ts
participant S_Internal as shutdownStagehand.ts
participant LocalBrowser as Local Chrome
participant BB_API as Browserbase API
Note over User,BB_API: Initialization Phase
User->>V3: new V3({ keepAlive: true })
V3->>V3: NEW: Resolve keepAlive (Constructor > Params)
alt Local Browser
V3->>LocalBrowser: NEW: launchLocalChrome(handleSIGINT: !keepAlive)
opt NEW: keepAlive is true
V3->>LocalBrowser: chrome.process.unref()
end
else Browserbase
V3->>BB_API: NEW: Create session with keepAlive param
end
Note over User,BB_API: Shutdown Phase (SIGINT, Error, or .close())
User->>Guards: SIGINT / Uncaught Exception
Guards->>Guards: NEW: Start 4s hard timeout & Keep-alive interval
Guards->>V3: _immediateShutdown(reason)
V3->>S_Browser: CHANGED: shutdownBrowserSession / shutdownLocalBrowser
alt NEW: keepAlive is true
S_Browser-->>V3: No-op (Browser stays running)
else keepAlive is false (Default)
S_Browser->>BB_API: CHANGED: apiClient.end()
S_Browser->>LocalBrowser: CHANGED: chrome.kill() & cleanupUserDataDir()
end
V3->>S_Internal: NEW: shutdownStagehandResources()
S_Internal-->>V3: Close loggers, unhook CDP, close context
V3->>V3: NEW: finalizeStagehandShutdown() (Reset internal state)
alt NEW: Process Signal Logic
Guards->>User: Re-emit signal (SIGINT/SIGTERM)
else NEW: Error Logic
Guards->>User: Rethrow Uncaught Exception
end
Note over Guards: If 4s passed: Force exit without full cleanup
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 14 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add optional boolean keepAlive parameter in the V3 constructor params (default false) | Added keepAlive? in V3Options and constructor assignment |
| ✅ | Constructor keepAlive overrides browserbaseSessionCreateParams.keepAlive when both provided | EffectiveSessionParams uses resolvedKeepAlive to override |
| ✅ | Pass chrome launcher's handleSIGINT based on keepAlive (v3.ts + launch/local.ts) | Added handleSIGINT option and pass !keepAlive into launchLocalChrome |
| ✅ | Unref the chrome child process after launch so Node can exit while chrome stays open (when keepAlive true) | Calls chrome.process?.unref() when keepAlive is true |
| ✅ | stagehand.close() skips apiClient.end() when keepAlive is true | shutdownBrowserSession bypasses endApiClient when keepAlive true |
| ✅ | Setting keepAlive:true keeps browser alive on uncaught errors/unhandledRejection (instance death) | Supervisor armed only when keepAlive=false; keepAlive true avoids cleanup |
| ✅ | Setting keepAlive:true keeps browser alive on SIGTERM/SIGINT (process signals) | Supervisor watches lifeline and runs cleanup only when !keepAlive |
| ✅ | Setting keepAlive:true keeps browser alive on explicit stagehand.close() | close() skips ending API and avoids killing local Chrome when keepAlive true |
| ✅ | Supervisor process requests REQUEST_RELEASE for Browserbase sessions on crash when keepAlive is false | cleanupBrowserbase updates session status to REQUEST_RELEASE when !keepAlive |
| ✅ | Replace global process guards with per-instance crash-only shutdown supervisor using ready handshake and PID polling | Removed global guards; added supervisor with pid polling and ready message |
| ✅ | Start shutdown supervisor for LOCAL launches (with pid/userDataDir) when keepAlive is false and await ready | Starts LOCAL supervisor with pid/userDataDir when !keepAlive and awaits ready |
| ✅ | Start shutdown supervisor for STAGEHAND_API (Browserbase) when keepAlive is false and await ready | Starts STAGEHAND_API supervisor for browserbase sessions when !keepAlive |
| ✅ | Add shutdown helper modules for browser/session and stagehand resource teardown | Added shutdownBrowser.ts and shutdownStagehand.ts helper modules |
| ✅ | Tests added validating keepAlive behavior for unhandled, SIGTERM/SIGINT, and stagehand.close across local, browserbase direct WS, and browserbase+API | Added keep-alive.spec.ts, helpers and child script to exercise scenarios |
| ✅ | Build changes include bundling supervisor entry so supervisor script is available in dist | package.json build-js adds entry.supervisor and files list includes dist/supervisor.* |
Architecture diagram
sequenceDiagram
participant User
participant App as Stagehand (Main Process)
participant Super as Supervisor Process
participant Local as Local Chrome
participant BAPI as Browserbase API
Note over App: NEW: keepAlive param (default false)<br/>Overrides Browserbase config
User->>>App: init(keepAlive)
alt Environment: LOCAL
App->>Local: CHANGED: launchLocalChrome(handleSIGINT: !keepAlive)
opt NEW: keepAlive is true
App->>App: unref() Chrome child process
end
else Environment: BROWSERBASE
App->>BAPI: Create Session (propagate keepAlive)
end
opt NEW: If keepAlive is false
App->>Super: Spawn supervisor.js
App->>Super: NEW: send config (PID or SessionID)
Super->>Super: Start PID/lifeline polling
Super-->>App: NEW: "ready" handshake
end
Note over App,Super: Runtime Execution
alt Scenario: Graceful close()
User->>App: stagehand.close()
App->>Super: NEW: Stop supervisor (no cleanup)
alt keepAlive is false
App->>App: CHANGED: Perform resource cleanup
App->>Local: Kill process / delete profile
App->>BAPI: End session (REQUEST_RELEASE)
else keepAlive is true
Note over App,Local: NEW: Skip resource cleanup
end
App-->>User: Done
else Scenario: Process Crash / SIGINT / SIGTERM
Note over App: App process terminates
App-xSuper: NEW: Stdin lifeline closed
alt NEW: keepAlive is false
Super->>Super: Detect parent exit
alt kind: LOCAL
Super->>Local: safeKill(pid) + rmSync(userDataDir)
else kind: STAGEHAND_API
Super->>BAPI: update(sessionId, status: REQUEST_RELEASE)
end
Super->>Super: exit(0)
else keepAlive is true
Note over Local,BAPI: Browser / Session remains alive
end
end
Member
Author
Contributor
There was a problem hiding this comment.
cubic analysis
1 issue found across 14 files
Confidence score: 3/5
- Missing
'error'handler onprocess.stdininpackages/core/lib/v3/shutdown/supervisor.tscould cause an uncaught exception (e.g., EPIPE) if the parent dies abruptly, terminating the supervisor unexpectedly. - Given the medium-high severity and clear runtime impact, this carries some regression risk despite being localized.
- Pay close attention to
packages/core/lib/v3/shutdown/supervisor.ts- add stdin error handling to avoid uncaught exceptions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/shutdown/supervisor.ts">
<violation number="1" location="packages/core/lib/v3/shutdown/supervisor.ts:157">
P1: Missing `'error'` handler on `process.stdin`. If the parent dies abruptly, stdin may emit an `'error'` event (e.g., `EPIPE`) before `'end'`/`'close'`. Without a handler, this throws an uncaught exception that kills the supervisor before it can run cleanup — defeating its purpose. Add an error handler that triggers the same lifeline-closed path.</violation>
</file>
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add keepAlive option in V3 constructor params; optional boolean default false | Added keepAlive option and constructor handling |
| ✅ | Constructor keepAlive overrides browserbaseSessionCreateParams.keepAlive when both provided | Constructor uses opts.keepAlive ?? browserbase param |
| ✅ | Replace in-process global signal handlers with a detached supervisor process using a lifeline (stdin + IPC) | Removed global guards and added supervisor process approach |
| ✅ | When keepAlive=false, supervisor is spawned during init() and performs cleanup if parent dies unexpectedly | Supervisor started on init when keepAlive is false |
| ✅ | Supervisor kills Chrome PID with SIGTERM → SIGKILL escalation for LOCAL when keepAlive=false | safeKill implements SIGTERM then SIGKILL escalation |
| ✅ | Supervisor removes temp user-data directory for LOCAL when created and keepAlive=false | cleanupLocal removes userDataDir when createdTempProfile false/false |
| ✅ | When env=BROWSERBASE & using stagehand API, supervisor sends REQUEST_RELEASE to Browserbase API when keepAlive=false | cleanupBrowserbase issues REQUEST_RELEASE via SDK |
| ✅ | Supervisor includes PID polling to guard against process reuse | startPidPolling periodically checks pid and sets pidGone |
| ✅ | When keepAlive=true, no supervisor is spawned and the browser is left running | Supervisor only started when !keepAlive; shutdown helpers skip cleanup if keepAlive |
| ✅ | When disableAPI=true (direct WS), no supervisor spawned (browser will auto release when WS dropped) | Supervisor for Browserbase only started when !disableAPI |
| ✅ | Refactor close() browser teardown into shutdown/shutdownBrowser.ts helpers (shutdownBrowserSession, shutdownLocalBrowser) | Added shutdownBrowser.ts and used in close() path |
| ✅ | Pull Stagehand internal state cleanup into shutdown/shutdownStagehand.ts (shutdownStagehandResources, finalizeStagehandShutdown) | Added shutdownStagehand helpers and used in close() finalize |
| ✅ | .close() now skips apiClient.end() when keepAlive: true | apiClient.end passed into shutdownBrowserSession only when keepAlive false |
| ✅ | Update local launch to pass chrome launcher's handleSIGINT based on keepAlive (v3.ts & local.ts) to prevent chrome kill on Ctrl+C when keepAlive=true | handleSIGINT option added and passed as !keepAlive |
| ✅ | Unref the chrome child process after launch so Node can exit while chrome stays open when keepAlive=true | chrome.process?.unref() invoked when keepAlive true |
| ✅ | Supervisor not spawned when keepAlive=true (explicit behavior) | Supervisor armed only when config.keepAlive===false |
| ✅ | Supervisor not spawned when disableAPI=true (direct WS) for Browserbase sessions | v3 only starts STAGEHAND_API supervisor when !disableAPI |
| ✅ | Supervisor uses stdin lifeline and IPC handshake with ready acknowledgement | supervisor listens on stdin/disconnect and sends 'ready' on IPC |
| ✅ | Supervisor is spawned detached with stdio pipe + ipc and unrefs child and stdin | spawn uses detached:true stdio pipe+ipc and child.unref()/stdin.unref() used |
| ✅ | Include supervisor script in build outputs/package.json changes | build-js entry and files include supervisor artifacts |
| ✅ | Add internal types and errors for shutdown supervisor messages/configs | Added types and error classes for supervisor messages and errors |
| ✅ | Tests added for keep-alive behavior covering scenarios (unhandledRejection/unhandledException, SIGTERM/SIGINT, stagehand.close()) | Test child handles scenarios; spec invokes them |
| ✅ | Tests validate each scenario across configurations: local, browserbase direct WS, browserbase via stagehand API | buildKeepAliveCases covers local and Browserbase (direct/API) permutations |
Architecture diagram
sequenceDiagram
participant User
participant App as Stagehand (Main Process)
participant Super as Supervisor Process
participant Local as Local Chrome
participant BAPI as Browserbase API
Note over App: NEW: keepAlive param (default false)<br/>Overrides Browserbase config
User->>>App: init(keepAlive)
alt Environment: LOCAL
App->>Local: CHANGED: launchLocalChrome(handleSIGINT: !keepAlive)
opt NEW: keepAlive is true
App->>App: unref() Chrome child process
end
else Environment: BROWSERBASE
App->>BAPI: Create Session (propagate keepAlive)
end
opt NEW: If keepAlive is false
App->>Super: Spawn supervisor.js
App->>Super: NEW: send config (PID or SessionID)
Super->>Super: Start PID/lifeline polling
Super-->>App: NEW: "ready" handshake
end
Note over App,Super: Runtime Execution
alt Scenario: Graceful close()
User->>App: stagehand.close()
App->>Super: NEW: Stop supervisor (no cleanup)
alt keepAlive is false
App->>App: CHANGED: Perform resource cleanup
App->>Local: Kill process / delete profile
App->>BAPI: End session (REQUEST_RELEASE)
else keepAlive is true
Note over App,Local: NEW: Skip resource cleanup
end
App-->>User: Done
else Scenario: Process Crash / SIGINT / SIGTERM
Note over App: App process terminates
App-xSuper: NEW: Stdin lifeline closed
alt NEW: keepAlive is false
Super->>Super: Detect parent exit
alt kind: LOCAL
Super->>Local: safeKill(pid) + rmSync(userDataDir)
else kind: STAGEHAND_API
Super->>BAPI: update(sessionId, status: REQUEST_RELEASE)
end
Super->>Super: exit(0)
else keepAlive is true
Note over Local,BAPI: Browser / Session remains alive
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 14 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Linked issue analysis
Linked issue: STG-1338: add keepAlive param
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add optional boolean keepAlive parameter in the stagehand constructor params; keepAlive defaults to false. | Added keepAlive?: in V3Options and set in V3 constructor |
| ✅ | keepAlive on the constructor overrides browserbaseSessionCreateParams.keepAlive when both are provided. | Constructor value used to build effectiveSessionParams |
| ✅ | Replace old in-process global signal handlers with a detached supervisor process that watches a lifeline (stdin pipe + IPC channel) to the parent. | Removed process guards and added supervisor process files |
| ✅ | When keepAlive is false, the supervisor is spawned during init() and performs best-effort cleanup if the parent dies unexpectedly. | Supervisor started during init when keepAlive=false |
| ✅ | If env is "LOCAL" the supervisor kills the Chrome PID (with SIGTERM → SIGKILL escalation) and removes any temp user-data directory. | safeKill escalation and rmSync userDataDir implemented |
| ✅ | If env is "BROWSERBASE" & using the stagehand API, the supervisor sends REQUEST_RELEASE to the browserbase API. | Supervisor updates session status to REQUEST_RELEASE |
| ✅ | The supervisor includes PID polling to guard against process reuse (killing a recycled PID). | startPidPolling implemented to detect pid gone |
| ✅ | When keepAlive is set to true, no supervisor is spawned, & the browser is left running. | Checks prevent starting supervisor when keepAlive=true |
| ✅ | When disableAPI is set to true (direct WS to Browserbase), no supervisor is spawned since the browser will auto release when the WS is dropped. | Supervisor start guarded by !this.disableAPI for Browserbase |
| ✅ | Refactor close() into helpers: shutdown/shutdownBrowser.ts (shutdownBrowserSession, shutdownLocalBrowser) and shutdown/shutdownStagehand.ts (shutdownStagehandResources, finalizeStagehandShutdown). | New shutdown helper modules added and used in close() |
| ✅ | .close() now skips apiClient.end() when keepAlive: true. | shutdownBrowserSession returns early when keepAlive=true |
| ✅ | Local browser launch changes: pass chrome launcher’s handleSIGINT based on keepAlive. | launchLocalChrome now accepts handleSIGINT and v3 passes it |
| ✅ | Local launch unref() the chrome child process after launch so Node can exit while chrome stays open (when keepAlive true). | chrome.process?.unref() called when keepAlive |
| ✅ | Added tests in keep-alive.spec.ts validating behavior for unhandledRejection/unhandledException, SIGTERM/SIGINT, and stagehand.close(). | Spec plus helpers and child test script added |
| ✅ | Each scenario is validated across: local browser launched by stagehand, browserbase direct WS, browserbase via stagehand API. | Helpers construct cases for local, bb direct WS, and bb via API |
| ✅ | Build packaging updated to include the supervisor script so it can be spawned in production builds. | package.json build-js updated and dist supervisor files added |
Architecture diagram
sequenceDiagram
participant User as User Script
participant SH as Stagehand (V3 Instance)
participant SV as NEW: Shutdown Supervisor (Detached)
participant Browser as Browser (Local/Remote)
participant API as Browserbase API
Note over SH,SV: Initialization (init)
User->>>SH: init()
SH->>>Browser: Launch or Connect
alt NEW: keepAlive is FALSE (Default)
SH->>>SV: spawn() detached process
SH->>>SV: Send config (PID, SessionID, API Key)
SV->>SV: Start PID polling & watch stdin lifeline
SV-->>SH: "ready" handshake
else NEW: keepAlive is TRUE
Note over SH,Browser: Skip supervisor spawning
opt LOCAL Environment
SH->>>SH: NEW: unref() Chrome process
end
end
SH-->>User: initialized
Note over SH,API: Runtime & Termination
alt Scenario: Parent Process Crash / SIGINT / Uncaught Error
Note over SV,Browser: Supervisor detects stdin/IPC closure
alt kind == LOCAL
SV->>>Browser: NEW: SIGTERM -> SIGKILL PID
SV->>SV: NEW: Clean up temp userDataDir
else kind == STAGEHAND_API
SV->>>API: NEW: update(sessionId, { status: 'REQUEST_RELEASE' })
end
else Scenario: Explicit close()
User->>>SH: close()
SH->>>SV: NEW: stop() (no cleanup)
alt CHANGED: keepAlive is TRUE
Note over SH,Browser: Skip API end() / Skip process kill
SH->>>SH: Finalize internal state only
else keepAlive is FALSE
SH->>>Browser: Kill process / end session
SH->>>SH: Finalize internal state
end
SH-->>User: closed
end
Note over User,Browser: NEW: If keepAlive=true, Browser remains running after SH exit
pirate
reviewed
Feb 13, 2026
pirate
reviewed
Feb 13, 2026
pirate
reviewed
Feb 13, 2026
pirate
reviewed
Feb 13, 2026
pirate
reviewed
Feb 13, 2026
pirate
approved these changes
Feb 13, 2026
pirate
reviewed
Feb 14, 2026
pirate
approved these changes
Feb 14, 2026
This was referenced Feb 14, 2026
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
2 participants
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.
why
what changed
new
keepAliveoption:keepAliveparameter in the stagehand constructor params.keepAlivedefaults tofalsekeepAliveon the constructor overridesbrowserbaseSessionCreateParams.keepAlivewhen both are provided out-of-process shutdown supervisor (packages/core/lib/v3/shutdown/):keepAliveis set tofalse, the supervisor is spawned duringinit()and performs best-effort cleanup if the parent dies unexpectedly:envis"LOCAL"the supervisor kills the Chrome PID (with SIGTERM → SIGKILL escalation) and removes any temp user-data directoryenvis"BROWSERBASE"& using the stagehand API, the supervisor sendsREQUEST_RELEASEto the browserbase APIkeepAliveis set totrue, no supervisor is spawned, & the browser is left runningdisableAPIis set totrue(direct WS to Browserbase), no supervisor is spawned since the browser will auto release when the WS is droppedrefactored
close()into helpers:shutdown/shutdownBrowser.ts(shutdownBrowserSession,shutdownLocalBrowser)shutdown/shutdownStagehand.ts(shutdownStagehandResources,finalizeStagehandShutdown)-
.close()now skipsapiClient.end()whenkeepAlive: truelocal browser launch changes:
handleSIGINTbased onkeepAliveinpackages/core/lib/v3/v3.tsandpackages/core/lib/v3/launch/local.ts. this prevents chrome from getting killed on Ctrl+C whenkeepAlive: trueunref()the chrome child process after launch so Node can exit while chrome stays open (packages/core/lib/v3/v3.ts)behaviour notes:
keepAlive: truewill keep the browser alive in the following scenarios:unhandledRejection,unhandledException)SIGTERMorSIGINTstagehand.close()test plan
keep-alive.spec.tsthat validate expected behaviour across the following scenarios:unhandledRejection/unhandledException,SIGTERM/SIGINT,stagehand.close()Summary by cubic
Adds a keepAlive option to Stagehand V3 so you control whether the browser/session stays up after stagehand.close(), signals, or uncaught errors. Replaces global process handlers with a per‑instance crash‑only supervisor with a ready handshake and PID polling to avoid race conditions (STG‑1338).
New Features
Migration
Written for commit 3887d8f. Summary will update on new commits. Review in cubic