Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions packages/browseros-agent/apps/server/src/browser/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,20 @@ export class Browser {
return this.pages.get(pageId)
}

private async resolvePageInfo(pageId: number): Promise<PageInfo | undefined> {
const cached = this.pages.get(pageId)
if (cached) return cached

await this.listPages()
return this.pages.get(pageId)
}

private unknownPageError(pageId: number): Error {
return new Error(
`Unknown page ${pageId}. Use list_pages to see available pages.`,
)
}

async refreshPageInfo(pageId: number): Promise<PageInfo | undefined> {
let info = this.pages.get(pageId)
if (!info) {
Expand Down Expand Up @@ -591,12 +605,24 @@ export class Browser {
}

async closePage(page: number): Promise<void> {
const info = this.pages.get(page)
if (!info)
throw new Error(
`Unknown page ${page}. Use list_pages to see available pages.`,
)
await this.cdp.Browser.closeTab({ tabId: info.tabId })
let info = await this.resolvePageInfo(page)
if (!info) throw this.unknownPageError(page)

try {
await this.cdp.Browser.closeTab({ targetId: info.targetId })
} catch (error) {
const refreshed = await this.refreshPageInfo(page)
if (
!refreshed ||
(refreshed.targetId === info.targetId && refreshed.tabId === info.tabId)
) {
throw error
}

info = refreshed
await this.cdp.Browser.closeTab({ targetId: info.targetId })
}

this.consoleCollector.detach(page)
this.pages.delete(page)
this.sessions.delete(info.targetId)
Comment on lines +613 to 628
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stale targetId session entry leaks on retry path

When the retry branch is taken (info = refreshed), the final this.sessions.delete(info.targetId) deletes the new targetId — which almost certainly has no session entry — while the session that was registered under the old targetId is left in the map indefinitely. Every close that hits this retry path leaks one entry in this.sessions. The fix is to capture the original targetId before reassigning info and delete that one.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/browser.ts
Line: 613-628

Comment:
**Stale `targetId` session entry leaks on retry path**

When the retry branch is taken (`info = refreshed`), the final `this.sessions.delete(info.targetId)` deletes the *new* `targetId` — which almost certainly has no session entry — while the session that was registered under the *old* `targetId` is left in the map indefinitely. Every close that hits this retry path leaks one entry in `this.sessions`. The fix is to capture the original `targetId` before reassigning `info` and delete that one.

How can I resolve this? If you propose a fix, please make it concise.

Expand Down
129 changes: 129 additions & 0 deletions packages/browseros-agent/apps/server/tests/browser/browser.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import { describe, it } from 'bun:test'
import assert from 'node:assert'
import type {
CloseTabParams,
GetTabInfoParams,
TabInfo,
} from '@browseros/cdp-protocol/domains/browser'
import type { ProtocolApi } from '@browseros/cdp-protocol/protocol-api'
import type { CdpBackend } from '../../src/browser/backends/types'
import { Browser } from '../../src/browser/browser'

class FakeCdpBackend {
readonly closeTabCalls: CloseTabParams[] = []
tabs: TabInfo[]

Browser = {
getTabs: async () => ({ tabs: this.tabs }),
getTabInfo: async (params: GetTabInfoParams) => {
const tab = this.tabs.find(
(t) => t.tabId === params.tabId || t.targetId === params.targetId,
)
if (!tab) throw new Error('Unknown page')
return { tab }
},
closeTab: async (params: CloseTabParams) => {
this.closeTabCalls.push(params)
const index = this.tabs.findIndex(
(t) => t.tabId === params.tabId || t.targetId === params.targetId,
)
if (index === -1) throw new Error('Unknown page')
this.tabs.splice(index, 1)
},
}

Target = {
on: () => () => {},
}

constructor(tabs: TabInfo[]) {
this.tabs = tabs
}

async connect(): Promise<void> {}
async disconnect(): Promise<void> {}
isConnected(): boolean {
return true
}
async getTargets(): Promise<[]> {
return []
}
session(): ProtocolApi {
throw new Error('session not implemented')
}
onSessionEvent(): () => void {
return () => {}
}
}

function createTab(overrides: Partial<TabInfo>): TabInfo {
return {
tabId: 101,
targetId: 'target-101',
url: 'https://example.com',
title: 'Example',
isActive: false,
isLoading: false,
loadProgress: 1,
isPinned: false,
isHidden: false,
windowId: 1,
index: 0,
...overrides,
}
}

function createBrowser(tabs: TabInfo[]): {
browser: Browser
cdp: FakeCdpBackend
} {
const cdp = new FakeCdpBackend(tabs)
return {
browser: new Browser(cdp as unknown as CdpBackend),
cdp,
}
}

describe('Browser', () => {
it('closes listed pages by target ID', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 401, targetId: 'target-401' }),
])

const pages = await browser.listPages()
assert.strictEqual(pages.length, 1)

await browser.closePage(pages[0].pageId)

assert.deepStrictEqual(cdp.closeTabCalls, [{ targetId: 'target-401' }])
assert.strictEqual(cdp.tabs.length, 0)
})

it('refreshes pages before treating closePage page IDs as unknown', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 402, targetId: 'target-402' }),
])

await browser.closePage(1)

assert.deepStrictEqual(cdp.closeTabCalls, [{ targetId: 'target-402' }])
assert.strictEqual(cdp.tabs.length, 0)
})

it('retries closePage when the tab target changes before close', async () => {
Comment on lines +102 to +113
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Hardcoded page ID 1 makes the test fragile

closePage(1) assumes Browser.nextPageId starts at 1, so the first page produced by listPages() gets assigned ID 1. If the initial counter ever changes, this test silently exercises the "unknown page → listPages() refresh" branch instead of the "cache miss" scenario it intends to cover. Using pages[0].pageId (after calling listPages() explicitly, as done in the other tests) would remove the assumption.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/tests/browser/browser.test.ts
Line: 102-113

Comment:
**Hardcoded page ID `1` makes the test fragile**

`closePage(1)` assumes `Browser.nextPageId` starts at `1`, so the first page produced by `listPages()` gets assigned ID `1`. If the initial counter ever changes, this test silently exercises the "unknown page → `listPages()` refresh" branch instead of the "cache miss" scenario it intends to cover. Using `pages[0].pageId` (after calling `listPages()` explicitly, as done in the other tests) would remove the assumption.

How can I resolve this? If you propose a fix, please make it concise.

const { browser, cdp } = createBrowser([
createTab({ tabId: 403, targetId: 'target-403-old' }),
])

const pages = await browser.listPages()
cdp.tabs[0].targetId = 'target-403-new'

await browser.closePage(pages[0].pageId)

assert.deepStrictEqual(cdp.closeTabCalls, [
{ targetId: 'target-403-old' },
{ targetId: 'target-403-new' },
])
assert.strictEqual(cdp.tabs.length, 0)
})
})
Loading