fix(server): close pages by refreshed target id#979
fix(server): close pages by refreshed target id#979Nikhil (shadowfax92) wants to merge 1 commit intodevfrom
Conversation
✅ Tests passed — 1226/1230
|
Greptile SummaryThis PR fixes
Confidence Score: 3/5The retry path introduced to handle stale targetIds leaves the original session entry in The main change is a well-motivated fix for stale-targetId closes, but the session cleanup at the end of packages/browseros-agent/apps/server/src/browser/browser.ts — specifically the cleanup block at the end of Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Browser
participant CDPBrowser as cdp.Browser
participant PagesMap as pages / sessions
Caller->>Browser: closePage(pageId)
Browser->>PagesMap: resolvePageInfo(pageId)
alt cache hit
PagesMap-->>Browser: PageInfo (cached)
else cache miss
Browser->>CDPBrowser: listPages()
CDPBrowser-->>Browser: updated pages
PagesMap-->>Browser: PageInfo (refreshed)
end
Browser->>CDPBrowser: "closeTab({ targetId: info.targetId })"
alt success
CDPBrowser-->>Browser: ok
Browser->>PagesMap: pages.delete(pageId)
Browser->>PagesMap: sessions.delete(info.targetId)
else error (targetId stale)
CDPBrowser-->>Browser: error
Browser->>Browser: refreshPageInfo(pageId)
alt targetId/tabId changed
Browser->>CDPBrowser: "closeTab({ targetId: refreshed.targetId })"
CDPBrowser-->>Browser: ok
Browser->>PagesMap: pages.delete(pageId)
Note over Browser,PagesMap: sessions.delete(refreshed.targetId) — OLD targetId entry leaks
else unchanged
Browser-->>Caller: rethrow error
end
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/server/src/browser/browser.ts:613-628
**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.
### Issue 2 of 2
packages/browseros-agent/apps/server/tests/browser/browser.test.ts:102-113
**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.
Reviews (1): Last reviewed commit: "fix: close pages by target id (bosmain-w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } 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) |
There was a problem hiding this comment.
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.| 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 () => { |
There was a problem hiding this comment.
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.
|
Refinery rejected this merge request after the review gate. Greptile reported an unresolved P1 issue in packages/browseros-agent/apps/server/src/browser/browser.ts: the close_page retry path can leak the stale targetId session entry. Source issue bosmain-wcw has been reopened for rework; branch not merged. |
1 participant
Summary
Automated refinery PR for BrowserOS issue #438 from polecat branch.
Verification
Refinery rebased the branch on current origin/dev and ran the recorded verification commands.
Fixes #438