From d77c6fb1cad6b1458cb208216051ec5ff3bcf749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Fri, 25 Sep 2026 15:08:04 +0200 Subject: [PATCH] Don't resolve a navigation queued on a free connection as loaded The easy-handle pool is shared by every client in the process. When other sessions hold all of it, a new navigation's request sits in pending_queue, but Runner resolved a frame still in `.pre` as soon as in-flight activity was zero, which ignores pending transfers. goto then reported success on an empty document. Wait on `activity.idle()` instead, and make a goto that times out before any response arrives a NavigationTimeout error rather than a soft timeout. Fixes #3636 --- src/browser/Runner.zig | 4 ++-- src/browser/tools.zig | 29 +++++++++++++++++++++++++++++ src/mcp/tools.zig | 2 +- src/network/HttpClient.zig | 1 - 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/browser/Runner.zig b/src/browser/Runner.zig index c2fb72857..48f8a03ba 100644 --- a/src/browser/Runner.zig +++ b/src/browser/Runner.zig @@ -231,7 +231,6 @@ fn _tick(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa const activity = http_client.activity(); const total_http_activity = activity.http; - const total_network_activity = activity.total(); const network_idle = activity.idle(); const is_done = browser.hasMacrotasks() == false and network_idle; @@ -283,7 +282,8 @@ fn _tick(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa condition.status = .complete; }, .pre, .raw, .text, .image, .download => { - if (total_network_activity == 0) { + // Includes pending: another client may hold every connection. + if (network_idle) { condition.status = .complete; } else { want_http_tick = true; diff --git a/src/browser/tools.zig b/src/browser/tools.zig index d70f19860..5650d0bcb 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -795,6 +795,7 @@ pub const ToolError = error{ InvalidParams, NodeNotFound, NavigationFailed, + NavigationTimeout, Cancelled, Timeout, InternalError, @@ -807,6 +808,7 @@ pub fn errorMessage(err: ToolError) []const u8 { return switch (err) { error.NodeNotFound => "NodeNotFound: the selector or backendNodeId matched nothing on the current page. Re-inspect the page (tree/interactiveElements) for fresh node ids, or omit backendNodeId to target the document root.", error.FrameNotLoaded => "FrameNotLoaded: no page is loaded — call goto (or pass a url) first.", + error.NavigationTimeout => "NavigationTimeout: no response arrived before the timeout, so the page is empty. Other sessions may be holding every connection (see --http-max-concurrent); retry goto or close idle sessions.", else => @errorName(err), }; } @@ -2455,6 +2457,7 @@ fn performGoto(session: *lp.Session, registry: *NodeRegistry, url: [:0]const u8, // re-fetch frame, navigate might have changed it. const frame = page.frame() orelse return ToolError.NavigationFailed; if (frame._last_navigate_error != null) return ToolError.NavigationFailed; + if (result == .timeout and frame._parse_state == .pre) return ToolError.NavigationTimeout; return result; } @@ -2702,6 +2705,32 @@ test "tree and nodeDetails read the node's own frame" { try std.testing.expect(std.mem.indexOf(u8, details.text, "child-label") != null); } +test "goto: a navigation stuck waiting for a connection is an error" { + var registry: NodeRegistry = .init(std.testing.allocator); + defer registry.deinit(); + + const network = &testing.test_app.network; + var held: std.ArrayList(*@import("../network/http.zig").Connection) = .empty; + defer held.deinit(std.testing.allocator); + defer for (held.items) |conn| network.releaseConnection(conn); + while (network.getConnection()) |conn| try held.append(std.testing.allocator, conn); + + const session = testing.test_session; + defer if (session.primaryPage()) |page| page.close(); + + const aa = testing.arena_allocator; + const args = try std.json.parseFromSliceLeaky(std.json.Value, aa, + \\{"url":"http://localhost:9582/src/browser/tests/mcp_actions.html","timeout":300} + , .{}); + try std.testing.expectError(error.NavigationTimeout, call(aa, session, ®istry, "goto", args, .{})); + + for (held.items) |conn| network.releaseConnection(conn); + held.clearRetainingCapacity(); + + const r = try call(aa, session, ®istry, "goto", args, .{}); + try std.testing.expectEqualStrings("Navigated successfully.", r.text); +} + test "parseValue: zero-filled optional backendNodeId treated as omitted" { var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); defer arena.deinit(); diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index c6edfe97e..5a0ba03ca 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -166,7 +166,7 @@ fn dispatchBrowserTool( error.FrameNotLoaded => .FrameNotLoaded, error.NodeNotFound, error.InvalidParams => .InvalidParams, error.Cancelled => .Cancelled, - error.Timeout => .Timeout, + error.Timeout, error.NavigationTimeout => .Timeout, error.NavigationFailed, error.InternalError, error.OutOfMemory => .InternalError, }; return server.sendError(id, code, browser_tools.errorMessage(err)); diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index 245d4255e..5f62fb0bf 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -802,7 +802,6 @@ pub fn _tick(self: *Client, timeout_ms: u32, mode: DrainMode) !bool { // we're about to tell our caller not to call us again without it // doing some work (e.g. running tasks). Let's assert that we were // right in doing that, else we'll likely introduce latency. - std.debug.assert(self.pending_queue.first == null); std.debug.assert(self.delayed_queue.first == null); std.debug.assert(self.dispatch_queue.first == null); std.debug.assert(self.ws_dispatch_queue.first == null);