fix(server): close pages by refreshed target id

[Nikhil (shadowfax92)]

Summary

• Close tabs by refreshed BrowserOS target IDs when a listed page's target changes before close.
• Clear every target session touched by a successful close retry so stale target sessions do not leak.
• Add browser regression coverage for the retry path with an attached old-target session.

Test plan

• bun --env-file=.env.development test ./tests/browser/browser.test.ts
• bun run test:browser
• bun run typecheck

Fixes #438

[github-actions]

❌ Tests failed — 1/1231 failed

Suite	Passed	Failed	Skipped
✅ agent	80/80	0	0
✅ build	9/9	0	0
✅ eval	93/93	0	0
✅ server-agent	261/261	0	0
✅ server-api	203/203	0	0
✅ server-browser	8/8	0	0
✅ server-integration	9/10	0	1
✅ server-lib	242/242	0	0
✅ server-root	60/63	0	3
✅ server-skills	31/31	0	0
❌ server-tools	230/231	1	0

Failed tests

• server-tools — search_dom > finds elements by XPath

View workflow run

[greptile-apps]

Greptile Summary

This PR fixes tab-close failures that occur when a page's CDP targetId changes between the time it was listed and when closePage is called. The fix adds a retry path that refreshes page info on first failure and retries with the updated targetId.

• closePage now uses targetId instead of tabId for Browser.closeTab, adds a catch-and-retry block that calls refreshPageInfo when the first attempt fails, and cleans up sessions for every targetId touched.
• resolvePageInfo is extracted as a private helper that auto-hydrates the page cache via listPages() before declaring a page unknown.
• Four new unit tests cover the happy path, lazy resolution, target-change retry, and stale-session cleanup after retry.

Confidence Score: 4/5

Safe to merge; the retry path correctly handles target changes and the session cleanup covers both target IDs.

The core retry logic and session cleanup are sound and well-tested. The only rough edge is the tabId check inside the retry guard: when tabId changes but targetId stays the same, a redundant retry fires with the same targetId that already failed, and the original error is replaced by the new one. This scenario is practically unreachable in real browser behavior, so the change is safe to ship.

packages/browseros-agent/apps/server/src/browser/browser.ts — specifically the retry guard condition in closePage.

Important Files Changed

Filename	Overview
packages/browseros-agent/apps/server/src/browser/browser.ts	Adds retry logic in closePage that refreshes the page's targetId when closeTab fails, and cleans up sessions for all target IDs touched. Also extracts two private helpers (resolvePageInfo, unknownPageError). Minor: retry guard checks tabId unnecessarily alongside targetId.
packages/browseros-agent/apps/server/tests/browser/browser.test.ts	New test file covering four Browser scenarios: normal close by targetId, lazy listPages resolution, target-change retry, and stale session cache cleanup after retry. FakeCdpBackend correctly simulates target mutation and session management.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant closePage
    participant resolvePageInfo
    participant listPages
    participant CDP as cdp.Browser
    participant refreshPageInfo

    Caller->>closePage: closePage(pageId)
    closePage->>resolvePageInfo: resolvePageInfo(pageId)
    resolvePageInfo->>resolvePageInfo: pages.get(pageId)?
    alt not cached
        resolvePageInfo->>listPages: listPages()
        listPages-->>resolvePageInfo: updated cache
    end
    resolvePageInfo-->>closePage: info (or undefined → throw)

    closePage->>CDP: "closeTab({ targetId: info.targetId })"
    alt success
        CDP-->>closePage: ok
        closePage->>closePage: detach console, delete page, delete sessions
    else error (target changed)
        CDP-->>closePage: error
        closePage->>refreshPageInfo: refreshPageInfo(pageId)
        refreshPageInfo->>CDP: "getTabInfo({ tabId: info.tabId })"
        CDP-->>refreshPageInfo: tab with new targetId
        refreshPageInfo-->>closePage: refreshed (new targetId)
        alt no change or page gone
            closePage->>Caller: rethrow error
        else targetId changed
            closePage->>CDP: "closeTab({ targetId: refreshed.targetId })"
            CDP-->>closePage: ok
            closePage->>closePage: detach console, delete page, delete both sessions
        end
    end
    closePage-->>Caller: void

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/browseros-agent/apps/server/src/browser/browser.ts:616-621
The retry guard checks both `targetId` AND `tabId` to decide whether to retry. This means if only `tabId` changes (while `targetId` stays the same) the method retries `closeTab` with the *same* `targetId` that just failed, then surfaces the error from the second attempt rather than the original one. Since `closeTab` is called by `targetId`, only a changed `targetId` can make the retry meaningful. The `tabId` condition should be dropped.

```suggestion
      if (!refreshed || refreshed.targetId === info.targetId) {
        throw error
      }
```

_{Reviews (1): Last reviewed commit: "fix: clear close page retry sessions" | Re-trigger Greptile}

[greptile-apps]

The retry guard checks both targetId AND tabId to decide whether to retry. This means if only tabId changes (while targetId stays the same) the method retries closeTab with the same targetId that just failed, then surfaces the error from the second attempt rather than the original one. Since closeTab is called by targetId, only a changed targetId can make the retry meaningful. The tabId condition should be dropped.

Suggested change

	if (
	!refreshed ||
	(refreshed.targetId === info.targetId && refreshed.tabId === info.tabId)
	) {
	throw error
	}
	if (!refreshed || refreshed.targetId === info.targetId) {
	throw error
	}

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: 616-621

Comment:
The retry guard checks both `targetId` AND `tabId` to decide whether to retry. This means if only `tabId` changes (while `targetId` stays the same) the method retries `closeTab` with the *same* `targetId` that just failed, then surfaces the error from the second attempt rather than the original one. Since `closeTab` is called by `targetId`, only a changed `targetId` can make the retry meaningful. The `tabId` condition should be dropped.

```suggestion
      if (!refreshed || refreshed.targetId === info.targetId) {
        throw error
      }
```

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