Skip to content

Commit c684ca7

Browse files
masnwilliamsclaude
masnwilliams
and
claude
authored
fix(cua): make session.stop() idempotent on repeated call (#156)
## Summary Stacks on top of the unified CUA template branch. Addresses the new High-Severity Cursor Bugbot finding on PR #143's latest scan: `stop()` throws on repeated call instead of being a no-op. ## Background PR #155 moved the session-state reset (`_sessionId = null`, …) into the `finally` so that a thrown replay-stop or browser-delete error wouldn't leave stale state behind. That fix was correct, but it exposed a latent bug: `stop()` opens with `const info = this.info` (TS) / `info = self.info` (Py), and the `info` getter delegates to the `sessionId` property, which raises when `_sessionId` is null. So once PR #155 reliably cleared `_sessionId` on the first call, the caller's error-path retry would hit the throwing getter and mask the original exception — exactly the failure mode PR #155 was meant to prevent. ## Fix `stop()` now: 1. Short-circuits at the top with a sentinel `SessionInfo` when no session is active — never touches the throwing getter. 2. Builds the live-session `info` from local fields directly so the body never depends on `this.info` / `self.info` either. Symmetric in TS (`session.ts`) and Python (`session.py`). ## Test plan - [x] `go build ./...` - [x] `go test ./pkg/create/...` - [ ] Force a replay-stop failure (e.g. delete the replay out-of-band, or pass an invalid replay id) and confirm calling `session.stop()` twice from the caller's error path no longer raises and the original error surfaces. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches session shutdown/cleanup logic in both Python and TypeScript templates; while small, mistakes could lead to leaked browser sessions or masked errors during teardown. > > **Overview** > `KernelBrowserSession.stop()` in both `pkg/templates/python/cua/session.py` and `pkg/templates/typescript/cua/session.ts` is updated to be **idempotent**. > > It now short-circuits when no active session exists (returning a sentinel `SessionInfo` without reading `info`/`sessionId`), and builds the returned `SessionInfo` directly from internal fields so teardown can’t fail due to accessing a getter after state has been cleared. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit db0bdea. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f588e58 commit c684ca7

2 files changed

Lines changed: 77 additions & 41 deletions

File tree

‎pkg/templates/python/cua/session.py‎

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -111,28 +111,46 @@ async def start(self) -> SessionInfo:
111111
return self.info
112112

113113
async def stop(self) -> SessionInfo:
114-
info = self.info
114+
session_id = self._session_id
115+
if not session_id:
116+
# Already stopped — return a snapshot of the (cleared) state without
117+
# touching `self.info`, whose `session_id` property raises on None.
118+
return SessionInfo(
119+
session_id="",
120+
live_view_url=self._live_view_url or "",
121+
replay_id=self._replay_id,
122+
replay_view_url=self._replay_view_url,
123+
viewport_width=self.opts.viewport_width,
124+
viewport_height=self.opts.viewport_height,
125+
)
126+
127+
info = SessionInfo(
128+
session_id=session_id,
129+
live_view_url=self._live_view_url or "",
130+
replay_id=self._replay_id,
131+
replay_view_url=self._replay_view_url,
132+
viewport_width=self.opts.viewport_width,
133+
viewport_height=self.opts.viewport_height,
134+
)
115135

116-
if self._session_id:
117-
session_id = self._session_id
118-
try:
119-
if self.opts.record_replay and self._replay_id:
120-
if self.opts.replay_grace_period > 0:
121-
await asyncio.sleep(self.opts.replay_grace_period)
122-
await self._stop_replay()
123-
info.replay_view_url = self._replay_view_url
124-
finally:
125-
# Reset state up front so that if browser deletion or a thrown replay
126-
# error propagates, a follow-up stop() call from the caller's error path
127-
# is a no-op instead of attempting to delete the same session twice.
128-
self._session_id = None
129-
self._live_view_url = None
130-
self._replay_id = None
131-
self._replay_view_url = None
132-
print(f"Destroying browser session: {session_id}")
133-
await asyncio.to_thread(
134-
self.kernel.browsers.delete_by_id, session_id,
135-
)
136+
try:
137+
if self.opts.record_replay and self._replay_id:
138+
if self.opts.replay_grace_period > 0:
139+
await asyncio.sleep(self.opts.replay_grace_period)
140+
await self._stop_replay()
141+
info.replay_view_url = self._replay_view_url
142+
finally:
143+
# Reset state up front so that if browser deletion or a thrown replay
144+
# error propagates, a follow-up stop() call from the caller's error path
145+
# is a no-op instead of attempting to delete the same session twice.
146+
self._session_id = None
147+
self._live_view_url = None
148+
self._replay_id = None
149+
self._replay_view_url = None
150+
print(f"Destroying browser session: {session_id}")
151+
await asyncio.to_thread(
152+
self.kernel.browsers.delete_by_id, session_id,
153+
)
136154

137155
return info
138156

‎pkg/templates/typescript/cua/session.ts‎

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -103,29 +103,47 @@ export class KernelBrowserSession {
103103
}
104104

105105
async stop(): Promise<SessionInfo> {
106-
const info = this.info;
106+
const sessionId = this._sessionId;
107+
if (!sessionId) {
108+
// Already stopped — return a snapshot of the (cleared) state without
109+
// touching `this.info`, whose `sessionId` getter throws on null.
110+
return {
111+
sessionId: '',
112+
liveViewUrl: this._liveViewUrl ?? '',
113+
replayId: this._replayId ?? undefined,
114+
replayViewUrl: this._replayViewUrl ?? undefined,
115+
viewportWidth: this.opts.viewportWidth,
116+
viewportHeight: this.opts.viewportHeight,
117+
};
118+
}
107119

108-
if (this._sessionId) {
109-
const sessionId = this._sessionId;
110-
try {
111-
if (this.opts.recordReplay && this._replayId) {
112-
if (this.opts.replayGracePeriod > 0) {
113-
await sleep(this.opts.replayGracePeriod * 1000);
114-
}
115-
await this.stopReplay();
116-
info.replayViewUrl = this._replayViewUrl || undefined;
120+
const info: SessionInfo = {
121+
sessionId,
122+
liveViewUrl: this._liveViewUrl ?? '',
123+
replayId: this._replayId ?? undefined,
124+
replayViewUrl: this._replayViewUrl ?? undefined,
125+
viewportWidth: this.opts.viewportWidth,
126+
viewportHeight: this.opts.viewportHeight,
127+
};
128+
129+
try {
130+
if (this.opts.recordReplay && this._replayId) {
131+
if (this.opts.replayGracePeriod > 0) {
132+
await sleep(this.opts.replayGracePeriod * 1000);
117133
}
118-
} finally {
119-
// Reset state up front so that if browser deletion or a thrown replay error
120-
// propagates, a follow-up stop() call from the caller's error path is a no-op
121-
// instead of attempting to delete the same session twice.
122-
this._sessionId = null;
123-
this._liveViewUrl = null;
124-
this._replayId = null;
125-
this._replayViewUrl = null;
126-
console.log(`Destroying browser session: ${sessionId}`);
127-
await this.kernel.browsers.deleteByID(sessionId);
134+
await this.stopReplay();
135+
info.replayViewUrl = this._replayViewUrl || undefined;
128136
}
137+
} finally {
138+
// Reset state up front so that if browser deletion or a thrown replay error
139+
// propagates, a follow-up stop() call from the caller's error path is a no-op
140+
// instead of attempting to delete the same session twice.
141+
this._sessionId = null;
142+
this._liveViewUrl = null;
143+
this._replayId = null;
144+
this._replayViewUrl = null;
145+
console.log(`Destroying browser session: ${sessionId}`);
146+
await this.kernel.browsers.deleteByID(sessionId);
129147
}
130148

131149
return info;

0 commit comments

Comments
 (0)