Repository navigation
Fail WebMCP invocations fast with tool_page_unloaded when their page is replaced - #498
Conversation
|
Picked tnsardesai to review these related PRs (kernel/kernel#5165, kernel/docs#738): touched the changed code in |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2938c84. Configure here.
| } | ||
| c.removeSessionToolsLocked(message.SessionID) | ||
| c.stateMu.Unlock() | ||
| c.signalStateChanged() |
There was a problem hiding this comment.
Crash handler never receives CDP events
High Severity
The new Inspector.targetCrashed path never runs against a real browser because nothing in webmcpclient or browsersurface sends Inspector.enable. Chromium only delivers that event after the domain is enabled; cdpmonitor already does this on its own sessions. Fake CDP tests inject the event directly, so they pass while production renderer crashes still wait out the full timeout instead of returning tool_page_unloaded.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2938c84. Configure here.
There was a problem hiding this comment.
Not an issue: Chromium sends Inspector.targetCrashed to every session attached to the target whether or not Inspector.enable was called. InspectorHandler::TargetCrashed() emits the event unconditionally; Enable() only re-reports a crash that happened before enable.
Checked against a production browser (Chromium 155): attached a flattened session, sent only Page.navigate and Page.crash, never Inspector.enable, and received Inspector.targetCrashed on that session. The end-to-end run in the PR description used the same unmodified webmcpclient connection, which is why the crash case dropped the tools and failed in 1.7s instead of 15s.


Summary
When the document that owns a pending WebMCP invocation is reloaded or navigated by anything other than the tool itself,
POST /webmcp/invokewaits out the fulltimeout_sec(up to 120s) before returningoutcome_unknown, even though no result can ever arrive. This fails those invocations fast with a distinct410 tool_page_unloaded, and marks both invoke failures that may have had side effects as non-retryable for SDKs.Chromium only answers a pending invocation across a navigation when the tool started that navigation (
WebMCPHandler::DidFinishNavigationchecksGetScriptToolInvocationId()): cross-origin gets an immediateError, same-origin gets a result from the new document around itsDOMContentLoaded. A reload or navigation from Playwright, the user, or unrelated page script never producesWebMCP.toolResponded.webmcpclientdropped the frame's tools on a document change but kept the invocation waiting.Changes
Fail fast (
lib/webmcpclient)EventDocumentChanged).Page.frameStoppedLoadingon the same session and frame), wait 500ms more, then fail the invocation withErrToolPageUnloaded(keepsinvocation_id).Renderer crashes (
lib/webmcpclient)timeout_sec(theWebMCP.invokeToolcommand to a dead renderer never answers), and an invocation pending at crash time also waited out the timeout.Inspector.targetCrashed, drop that session's tools and fail its pending invocations withErrToolPageUnloadedafter the same 500ms grace. The session stays enabled, so tools registered after the tab is reloaded or navigated are picked up.API (
cmd/api/api/webmcp.go,openapi.yaml, regeneratedlib/oapi/oapi.go)ErrToolPageUnloaded→410withcode: "tool_page_unloaded",message, andinvocation_id.410 tool_page_unloadedand the existing504 outcome_unknownboth setX-Should-Retry: false. Kernel SDKs retry 408/409/429/5xx by default unless that header says otherwise, so a default-configured client could re-invoke a tool after a 504 even though it may already have run. 410 isn't retried by the SDKs even without the header. Both headers are declared in the spec.410reports a known cause, not a known outcome: the tool may have had side effects before its page went away, so callers must not retry it automatically.webmcp.invokeTool) anddocs/repl.mdmention the new code.Deploy order
kernel/kernel#5165 must deploy first. Until metro-api allows the new code, its
/browser/kernelpassthrough rewrites410 tool_page_unloadedtosession_gone. That PR also adds the header and 410 passthrough on the control-plane path. Docs: kernel/docs#738.Tests
reloadWithoutResultin the fake CDP server replays the event sequence observed from Chromium for an external reload (frameStartedLoading→frameNavigated→frameStoppedLoading, notoolResponded).TestInvocationFailsFastWhenToolDocumentReloadsWithoutResultwaited the full 10s before this change and returnsErrToolPageUnloadedin about 1s after.frameStoppedLoadingbefore the late result, covering the grace window.TestInvocationFailsFastWhenToolRendererCrashes(crash while pending, tools removed, stale ref returnsErrToolNotFound) andTestInvocationFailsFastWhenCrashedToolPageReloads(crash, then a recovery commit that never finishes loading) both waited the full 10s before the fix.X-Should-Retry, and body for both the 504 and the 410.go vetclean;go test -racepasses for all non-e2e packages exceptlib/devtoolsproxyTestUpstreamManagerDetectsChromiumAndRestart, which fails the same way onmainin my environment (temp dir cleanup with local Chromium) and doesn't depend on this change.webmcpclient.Managerwith the fail-fast change against a production Chromium 155 browser over CDP (20s timeout):Page.navigate/ page-script reloadoutcome_unknownCompletedwith outputCompletedwith outputErrorErrorRenderer crash via CDP
Page.crash(15s timeout):outcome_unknowntool_page_unloadedoutcome_unknownNot included
dispatchedfield on failures.WebMCP.invokeTooldispatch and its response isn't tracked and still waits for the timeout.Reviewed with sol-loop: 5 rounds with openai-codex/gpt-6.1-sol on extra-high reasoning. Sol verdict: approved.