Stop browser evals from hanging a tool call forever - #299
Merged
Conversation
get_tab_dom could sit unanswered for minutes. executeJavaScript on a WebContentsView with no live renderer — one whose main frame never committed, or whose renderer process crashed — neither resolves nor rejects, so the existing try/catch could not rescue the caller, and neither the control-server handler nor the MCP bridge had a deadline of its own. Every eval now goes through evalInTab: it refuses upfront when the tab's renderer is gone (naming the crash reason so the caller knows to reload) and otherwise races the eval against a 5s timer. Both backends share the helper, and the bridge gets a 60s socket timeout as a backstop so no future endpoint can wedge a tool call indefinitely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
It was the only browser tool with no output bound — console logs cap at 200 entries, clickables at 500 items, screenshots default to JPEG q70, but outerHTML came back verbatim. A heavy page is 1-5MB of markup in a single tool result, which blows the calling agent's context. Truncates to 100KB by default with a notice naming the real size and the max_bytes escape hatch. Also makes the bridge's request timeout env-overridable so the never-responds path is testable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
get_tab_domcould sit unanswered for ten minutes and never return or error — the user had to interrupt it.executeJavaScripton aWebContentsViewwith no live renderer — one whose main frame never committed, or whose renderer process crashed — neither resolves nor rejects. It queues for a frame that never arrives, so the existingtry/catchcould not rescue the caller (confirmed by the missinggetDom failedlog line). Three layers deep, nothing had a deadline:BrowserManager.getDom, the/browser/domcontrol-server handler, andcallControlin the MCP bridge (Node's http client has no default request timeout).getClickables,scrollTab, andshowCursorhad the identical flaw — all four are awaited behind control-server handlers.The trigger in the reported incident was a renderer crash, not a dead URL: the page loaded and ran script, then
SpeechRecognition.available({processLocally: true})killed the renderer process, which Electron surfaces as a lateERR_FAILED (-2). AgetURL()-based precondition would not have caught it, so detection is crash-aware.Changes
src/main/browser-eval.ts(new, runtime-agnostic, shared by both backends):evalWithTimeoutraces the eval against a 5s timer, clearing it in afinally.evalBlockedReasonrefuses upfront with an actionable message — crash first (tab renderer crashed (reason: crashed) — reload the tab), then never-committed-document. A tab merely mid-first-load returnsnull, so "create tab, immediately read DOM" still queues as it does today.BrowserManager.evalInTabroutes all four eval callers through the helper. Crash state comes from a newrender-process-gonelistener plus a liveisCrashed()check; a newdid-fail-loadlistener records the load error (there wasn't one before — theloadURL failedlog came from theloadURLpromise's.catch);dom-readyclears both.null, so the control server's existing top-level catch returns a 500 carrying the real reason rather than a genericdom read failed.about:blank, so the frameless hang can't occur there — noted in a comment.callControlgets a 60s socket timeout withreq.destroy(err)(thetimeoutevent alone doesn't abort) as a backstop, so no future endpoint can wedge a tool call indefinitely.POST /worktreesgets 300s since it legitimately blocks on git fetch / PR checkout.Second commit: cap
get_tab_domoutputIt was the only browser tool with no output bound — console logs cap at 200 entries, clickables at 500 items, screenshots default to JPEG q70, but
outerHTMLcame back verbatim. A heavy page is 1–5MB of markup in one tool result. Now truncated to 100KB by default with a notice naming the real size and amax_bytesoverride.Test plan
npm run typechecknpx electron-vite buildnpx vitest run— 5 failures, all pre-existing flakes ingit-ops-state/path-fix/worktree-watcher.integrationunder suite contention; verified identical on a stashed clean treebrowser-eval.test.ts— never-settles eval rejects on a timer, timer cleared on the success path, each blocked-reason branch including crash-beats-ERR_FAILEDprecedencecontrol-server.test.ts— crash / timeout / dead-URL each come back as a 500 with the message intactmcp-bridge.test.js— end-to-end regression test: a stub server that never responds now errors instead of hanging; plus DOM truncation casesSpeechRecognition.available({langs:['en-US'], processLocally:true}).🤖 Generated with Claude Code