-
Notifications
You must be signed in to change notification settings - Fork 294
fix: clear session screenshots in the mcp #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ import { randomUUID } from "crypto"; | |||||||||||||||||||||||||||
import { Stagehand, Page } from "@browserbasehq/stagehand"; | ||||||||||||||||||||||||||||
import { StagehandSession, CreateSessionParams } from "./types/types.js"; | ||||||||||||||||||||||||||||
import type { Config } from "../config.d.ts"; | ||||||||||||||||||||||||||||
import { clearScreenshotsForSession } from "./mcp/resources.js"; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Store for all active sessions | ||||||||||||||||||||||||||||
const store = new Map<string, StagehandSession>(); | ||||||||||||||||||||||||||||
|
@@ -110,6 +111,18 @@ export const create = async ( | |||||||||||||||||||||||||||
const disconnectHandler = () => { | ||||||||||||||||||||||||||||
process.stderr.write(`[StagehandStore] Session disconnected: ${id}\n`); | ||||||||||||||||||||||||||||
store.delete(id); | ||||||||||||||||||||||||||||
// Purge by internal store ID and Browserbase session ID | ||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||
clearScreenshotsForSession(id); | ||||||||||||||||||||||||||||
const bbId = session.metadata?.bbSessionId; | ||||||||||||||||||||||||||||
if (bbId) { | ||||||||||||||||||||||||||||
clearScreenshotsForSession(bbId); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} catch { | ||||||||||||||||||||||||||||
process.stderr.write( | ||||||||||||||||||||||||||||
`[StagehandStore] Error clearing screenshots for session ${id}\n`, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+120
to
+125
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Silent error handling might hide important failures during screenshot cleanup.
Suggested change
|
||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
browser.on("disconnected", disconnectHandler); | ||||||||||||||||||||||||||||
|
@@ -158,6 +171,18 @@ export const remove = async (id: string): Promise<void> => { | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await session.stagehand.close(); | ||||||||||||||||||||||||||||
process.stderr.write(`[StagehandStore] Session closed: ${id}\n`); | ||||||||||||||||||||||||||||
// Purge by internal store ID and Browserbase session ID | ||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||
clearScreenshotsForSession(id); | ||||||||||||||||||||||||||||
const bbId = session.metadata?.bbSessionId; | ||||||||||||||||||||||||||||
if (bbId) { | ||||||||||||||||||||||||||||
clearScreenshotsForSession(bbId); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} catch { | ||||||||||||||||||||||||||||
process.stderr.write( | ||||||||||||||||||||||||||||
`[StagehandStore] Error clearing screenshots for session ${id}\n`, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||||||
process.stderr.write( | ||||||||||||||||||||||||||||
Comment on lines
+182
to
187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Silent error handling might hide important failures during screenshot cleanup.
Suggested change
|
||||||||||||||||||||||||||||
`[StagehandStore] Error closing session ${id}: ${ | ||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Screenshot cleanup is called in a disconnect handler that runs during session creation, but the session might not be fully established yet. This could lead to premature cleanup of screenshots that are still being used.