fix(browser): don't reuse a dead page/context lease in browser run/exec - #324
Open
Kaushik2003 wants to merge 1 commit into
Open
fix(browser): don't reuse a dead page/context lease in browser run/exec#324Kaushik2003 wants to merge 1 commit into
Kaushik2003 wants to merge 1 commit into
Conversation
page.isClosed() can keep reporting false even after the underlying CDP connection has actually died (crashed renderer, killed process, dropped pipe), so getPage() was handing the same broken lease back out to every browser run/exec command on a Session, matching the repro in agentrhq#314. - getPage()'s reuse fast path now wraps its existing liveness probe (assertOwnedWindow, which already makes a real CDP round trip) in a try/catch. On a closed-context error it invalidates the Profile runtime and falls through to acquire a fresh page instead of returning the stale one. - runBrowserProgram()'s post-run snapshot capture can hit the same closed-context error while still reporting ok: true (the script may have genuinely succeeded). It now signals that upward via a new onStaleContext option so the run action can invalidate the runtime instead of silently downgrading it to a warning. - Added CloakSessionManager.invalidateIfClosedContext() as the public hook actions.ts uses to wire that signal through. - Hoisted isClosedContextError() into run/types.ts as a shared helper instead of a private duplicate in session-manager.ts. Fixes agentrhq#314
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. This review is advisory and does not block merging. |
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.
Description
Related issue: Fixes #314
When you run
webcmd browser runagainst an existing Session, WebCMD triesto reuse the browser tab (page) it already has open for that session instead
of opening a new one every time — that's normal and good for performance.
The one check it leans on to decide "is this tab still good to use?" is
page.isClosed(). I found that this check can lie. If the browser'sconnection dies in a weird way (crashed tab, killed process, connection
dropped from outside Playwright's normal close path),
page.isClosed()keepssaying "still fine!" even though the tab is actually dead. Every real command
sent to it then fails with:
And because nothing told WebCMD "this tab is actually dead, stop using it,"
the next
browser runpicks the same broken tab again. And the one afterthat — it just keeps handing out the same dead lease, which is exactly what
the reporter saw.
I also found a second, sneakier spot with the same root cause: after a script
finishes running, WebCMD takes a snapshot of the page to build a before/after
diff. If that snapshot step hits the same "connection is dead" error,
WebCMD was shrugging it off as a warning and reporting the run as successful
— without ever flagging that the tab needs to be thrown away. So even a run
that did detect the dead connection wasn't fixing anything for next time.
What I changed
I didn't invent anything new here — the codebase already has a working
pattern for this exact problem in a couple of other places
(
newPageAttempt,navigatePageAttempt): make a real round trip to checkthe connection is alive, and if that fails, throw away the broken session and
start a fresh one. I applied that same pattern to the two spots that didn't
have it yet:
When reusing a tab (
getPage()insession-manager.ts): right beforehanding back a "looks fine" tab, I now check the result of the existing
round-trip call to the browser (it was already being made, I just now act
on whether it fails). If it fails with the "connection closed" error, I
throw away the broken session, close the dead tab, and transparently
acquire a fresh tab instead — the caller never sees the difference, they
just get a working page.
When the after-run snapshot fails (
runner.ts): if that step hits thesame "connection closed" error, I still report the run as successful (the
user's script may well have actually worked), but I now also flag the
session as needing a reset, so the next command on that session gets a
clean tab instead of the dead one.
Net effect: a dead tab now gets detected and replaced automatically, instead
of being handed out over and over.
Files touched
src/browser/runtime/local-cloak/session-manager.ts— the fix for point 1above (reused-tab liveness check + fallback to a fresh tab). I also added a
small helper method other code can call to say "this session's connection
looks dead, throw it away."
src/browser/run/runner.ts— the fix for point 2 (flagging a deadconnection after a successful run, instead of silently swallowing it).
src/browser/run/types.ts— small plumbing: I pulled the "is this aconnection-closed error" check into one shared place instead of it living
only inside
session-manager.ts, and added the option that letsrunner.tssignal "this session's connection looks dead" upward.src/browser/runtime/local-cloak/actions.ts— wires the signal fromrunner.tsback to the session manager's new helper, so a run thatdetects a dying connection actually triggers the cleanup.
src/browser/run/runner.test.tsandsrc/browser/runtime/local-cloak/session-manager.test.ts— new tests(see below).
Tests
session-manager.test.ts: simulates the exact bug — a tabwhere
isClosed()says "still open" but the underlying connection checkfails. Confirms WebCMD notices, throws away the broken session, and hands
back a tab from a fresh session instead of the same broken one.
runner.test.ts:closed" error, the run still succeeds but the "this connection is
dying" signal fires.
signal — I only want to reset the session for the specific
"connection is actually dead" case, not for every hiccup.
npm run typecheckandnpm run build— both clean.What this does not do (on purpose)
This is intentionally a small, focused fix. It does not:
mid-run — if a script has already clicked buttons or submitted forms,
blindly replaying it could cause duplicate side effects. I only fixed the
lease, not the in-flight script.
navigatePagepath used by adapter commands (issue Browser runtime does not recover when context closes during initial adapter navigation #293,already assigned to another contributor) — different code path, same
family of error message, but a separate fix.
— this fix only kicks in when a connection is actually used and found to
be dead, which is enough to close out browser run: recover when an apparently open page belongs to a dead context #314.
Type of Change
Screenshots / Output