fix: give each profile runtime an unleased anchor page - #290
Open
ayushsingh82 wants to merge 1 commit into
Open
Conversation
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
Contributor
🟢 No documentation gap found — medium confidenceThe automated review found no documentation gap in the supplied changes. This review is advisory and does not block merging. |
Author
|
Fixes #276 — @rishabhraj36 @beubax implements the anchor-page approach from the issue. Ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CloakSessionManager.getPage()adopted the context's initialabout:blankpage 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 withTarget page, context or browser has been closed.Implements the anchor-page approach from the issue:
ProfileRuntime.anchorPageat launch (adopted synchronously, no extra page creation on the common path) instead of being adopted into the pages map — so it's automatically excluded fromlistPages, tab selection, page IDs, and network capture, since those all iterate the pages map.getPage()/newPage()lease now always creates its own page — no more adopting a leftover tab.ProfileRuntime, so separate profiles (e.g.peer/actor) stay independent.Fixes #276
Test plan
npx tsc --noEmitruntime-owned anchor pagedescribe block tosession-manager.test.tscovering: 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 vialistPages, each profile gets its own independent anchor, andfreshPagenever leases the anchor out.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).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.src/browser/**/*.test.tsplus the existingunitproject: no failures beyond the pre-existing, unrelatedsrc/hosted/*baseline and two files that need a locally installed Chromium binary this sandbox doesn't have.