Skip to content

fix: give each profile runtime an unleased anchor page - #290

Open
ayushsingh82 wants to merge 1 commit into
agentrhq:mainfrom
ayushsingh82:fix/276-profile-runtime-anchor-page
Open

fix: give each profile runtime an unleased anchor page#290
ayushsingh82 wants to merge 1 commit into
agentrhq:mainfrom
ayushsingh82:fix/276-profile-runtime-anchor-page

Conversation

@ayushsingh82

Copy link
Copy Markdown

Summary

CloakSessionManager.getPage() adopted the context's initial about:blank page as an ordinary leased user page the first time a session asked for one. Releasing that final leased page could then begin browser/context shutdown; a command arriving before that shutdown finished propagating would fail on its first navigation with Target page, context or browser has been closed.

Implements the anchor-page approach from the issue:

  • The context's initial page is reserved as ProfileRuntime.anchorPage at launch (adopted synchronously, no extra page creation on the common path) instead of being adopted into the pages map — so it's automatically excluded from listPages, tab selection, page IDs, and network capture, since those all iterate the pages map.
  • Every getPage()/newPage() lease now always creates its own page — no more adopting a leftover tab.
  • If the anchor was never created (the practically nonexistent case of a context launching with zero pages) or gets unexpectedly closed while the context is still healthy, it's recreated — under the existing per-profile page-creation lock — before the next leased-page close, so closing the last leased page can never drop the context to zero pages.
  • The anchor is per ProfileRuntime, so separate profiles (e.g. peer / actor) stay independent.

Fixes #276

Test plan

  • npx tsc --noEmit
  • Added a runtime-owned anchor page describe block to session-manager.test.ts covering: the anchor stays open when the only leased page is released, immediate reuse after release doesn't relaunch the context, the anchor is recreated if unexpectedly closed before the next page close, the anchor is never exposed via listPages, each profile gets its own independent anchor, and freshPage never leases the anchor out.
  • Updated 3 pre-existing tests whose expected newPage/CDP call counts changed now that the first lease always creates its own page instead of adopting the anchor (this is the actual behavior fix, not a test artifact — verified by reverting the fix and confirming these tests, old and new, fail as expected).
  • Updated provider.test.ts's shared page-mock fixture to model the anchor correctly (own initial tab, never leased); fixed 2 tests whose hardcoded page-array indices shifted because the first lease now creates a real second page instead of adopting the anchor.
  • Full local run of src/browser/**/*.test.ts plus the existing unit project: no failures beyond the pre-existing, unrelated src/hosted/* baseline and two files that need a locally installed Chromium binary this sandbox doesn't have.

CloakSessionManager.getPage() used to adopt the context's initial
about:blank page as an ordinary leased user page the first time a
session asked for one. Releasing the final leased page could then
begin browser/context shutdown; a command arriving before that
shutdown finished propagating would fail on its first navigation with
"Target page, context or browser has been closed".

Reserve the initial page as ProfileRuntime.anchorPage instead — never
leased out, excluded from listPages/tab selection/page IDs since it's
never added to the pages map. Every getPage()/newPage() lease now
always creates its own page. If the anchor is ever unexpectedly
closed while the context is still healthy, it's recreated (under the
existing per-profile page-creation lock) before the next leased page
close, so closing the last leased page can never drop the context to
zero pages.

Fixes agentrhq#276
@github-actions

Copy link
Copy Markdown
Contributor

🟢 No documentation gap found — medium confidence

The automated review found no documentation gap in the supplied changes.

This review is advisory and does not block merging.

@ayushsingh82

Copy link
Copy Markdown
Author

Fixes #276@rishabhraj36 @beubax implements the anchor-page approach from the issue. Ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: releasing the final leased page can invalidate the shared profile runtime

1 participant