fix(mcp): guard tab title() with a timeout to prevent CLI-wide hangs on unresponsive tabs#41748
Open
Git-Raini wants to merge 1 commit into
Open
fix(mcp): guard tab title() with a timeout to prevent CLI-wide hangs on unresponsive tabs#41748Git-Raini wants to merge 1 commit into
Git-Raini wants to merge 1 commit into
Conversation
…on unresponsive tabs Fixes microsoft#41714
Contributor
Author
|
@microsoft-github-policy-service agree |
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.
Fixes #41714
Root cause
Tab.headerSnapshot()(packages/playwright-core/src/tools/backend/tab.ts) callsawait this.page.title()inside_raceAgainstModalStates(...), which only races the action against a modal-state event, not a timeout - if the action never resolves and no modal ever appears, it hangs forever.page.title()ultimately doescontext.evaluate(() => document.title)in the page's utility execution context server-side (Frame._title()inserver/frames.ts), which requires the renderer to actually respond to a CDPRuntime.evaluatecall. A tab discarded by Chrome's Memory Saver never answers such calls.Response rendering for every MCP/CLI command calls
headerSnapshot()for all open tabs, not just the current one - so one wedged tab blocks every command, even ones that don't touch it. That's the reported symptom.Fix
raceAgainstTimeout()totools/backend/utils.ts- wraps a promise with a timeout, returns{ timedOut: true }or{ timedOut: false, result }instead of throwing.headerSnapshot()now wrapspage.title()with a 3000ms timeout. On timeout: falls back to the last known title (instead of blank) and marks the tabunresponsive: true.unresponsiveto theTabHeadertype and totabHeaderEquals.renderTabsMarkdown/renderTabMarkdownrender[unresponsive]/- Page status: unresponsive, mirroring the existing[crashed]pattern exactly.Deliberately out of scope: a CDP
Target.getTargetInfofallback for title/url (suggested as an alternative in the issue). The timeout guard + last-known-title fallback fully resolves the hang, which is the critical bug; adding a second CDP session type for a cosmetic title-accuracy improvement felt like unnecessary surface area for this fix. Also did not touchconsoleMessageCount()- tracedpage.consoleMessages()/page.pageErrors()and confirmed they read a server-buffered list, not a live renderer round-trip, so they aren't part of this hang.Testing
The hard part here was proving this against something more than a synthetic timeout - Chrome's real Memory Saver discard isn't reliably triggerable in CI. Simulated it faithfully instead: a page that busy-loops its main JS thread for a bounded 3.5s, which genuinely blocks
Runtime.evaluatefrom completing, the same mechanism as the real bug (verified this is a faithful simulation, not just "a promise that never resolves" - it exercises the actual CDP layer). New test opens that page, confirmsbrowser_tabs listandbrowser_snapshotboth complete in ~3s (not 30s), that the busy tab is marked[unresponsive], and that the other, untouched tab still reports its real title in the same response - directly proving one wedged tab doesn't poison the rest, which is the core of the original report.Ran the new integration test 8x back-to-back (
--repeat-each=8 --workers=1) to check for timing flakiness given the chained timing assumptions (3000ms guard vs 3500ms loop vs staging delays) - all 8 passed with consistent ~5.1-5.3s timing, no flakes observed.Lint / typecheck output
Test output - new tests (with 8x repeat), plus existing crash.spec.ts and tabs.spec.ts (no regressions)