fix: do not block tool calls on a roots/list the client is slow to answer (#2477)

`getContext()` awaits `updateRoots()` while `ToolHandler.handle()` holds
the tool mutex, so a client that negotiated `roots` and never answers
blocks every tool for the SDK's 60s default.

This bounds the one awaited listing rather than `updateRoots()` as a
whole, since the other two call sites are already fire-and-forget and
bounding them would only throw away roots a slow client was about to
send. The last listing is then kept and handed to each new `McpContext`.
Roots are client state rather than browser state, so a reconnect does
not invalidate them, and real changes still arrive through
`roots/list_changed`.

Both halves turn out to be needed, which took me a couple of tries to
see. Bound every listing and a client answering in 8s never gets roots
at all, because each of its listings times out. Leave the background
ones unbounded but keep no cache and they are lost anyway, since that
listing can land while `context` is still undefined and
`context?.setRoots()` drops it. Either way, file writes stay confined to
the temp directory for the rest of the session, where today that client
works and merely waits.

It does mean a client declaring `roots` without `listChanged` can see
the previous list on its first call against a new context, until the
background refresh lands.

Tests for each half:

- `does not block tools if the client never answers roots/list` fails on
`main` and passes here. It raises the client-side timeout above the SDK
default so the stall surfaces as the assertion rather than as a test
timeout

```
✖ does not block tools if the client never answers roots/list (60902.121334ms)
  AssertionError [ERR_ASSERTION]: list_pages took 60438ms, expected the bounded roots request to settle well before the 60s SDK default
```

- `still applies roots from a client slower than the bound` passes on
`main` and here, and fails if the bound covers every listing rather than
the awaited one

`npm run test` shows four failures here, but the same four fail on
unmodified `main`.

#2285 changes the `catch` inside `updateRoots()` that this rewrites, so
whichever lands second needs a rebase. Happy to do it if this goes last.

Fixes #2476

Co-authored-by: Natasha Gorshunova <47688881+nattallius@users.noreply.github.com>

src/index.ts

@@ -16,6 +16,7 @@ import {FilePersistence} from './telemetry/persistence.js';
 import {
   McpServer,
   type CallToolResult,
+  type Root,
   SetLevelRequestSchema,
   ListRootsResultSchema,
   RootsListChangedNotificationSchema,
@@ -29,6 +30,16 @@ import {VERSION} from './version.js';
 
 export {buildFlag} from './ToolHandler.js';
 
+/**
+ * Timeout for a `roots/list` that a tool call is waiting on, matching the 5s
+ * default used for page operations. `getContext()` awaits it while
+ * `ToolHandler` holds the tool mutex, so leaving it unbounded lets a client
+ * that negotiates `roots` but does not answer block every tool for the SDK's
+ * default of 60s. Background refreshes are not bounded by this, so roots a
+ * slow client sends late still land.
+ */
+const ROOTS_REQUEST_TIMEOUT = 5_000;
+
 export async function createMcpServer(
   serverArgs: ReturnType<typeof parseArguments>,
   options: {
@@ -58,7 +69,15 @@ export async function createMcpServer(
     return {};
   });
 
-  const updateRoots = async () => {
+  // Roots are client state rather than browser state, so the last listing stays
+  // valid across browser reconnects and only the client can invalidate it, via
+  // the `roots/list_changed` notification handled below
+  let lastRoots: Root[] | undefined;
+
+  // `timeout` is only passed where a tool call is waiting on the result – the
+  // background refreshes below block nobody, so bounding them would just discard
+  // roots a slow client was about to send
+  const updateRoots = async (timeout?: number) => {
     if (!server.server.getClientCapabilities()?.roots) {
       return;
     }
@@ -66,8 +85,10 @@ export async function createMcpServer(
       const roots = await server.server.request(
         {method: 'roots/list'},
         ListRootsResultSchema,
+        timeout === undefined ? undefined : {timeout},
       );
-      context?.setRoots(roots.roots);
+      lastRoots = roots.roots;
+      context?.setRoots(lastRoots);
     } catch (e) {
       logger?.('Failed to list roots', e);
     }
@@ -158,7 +179,16 @@ export async function createMcpServer(
         // Surfaces a one-time note in the next response after a reconnect.
         reconnected: context !== undefined,
       });
-      await updateRoots();
+      if (lastRoots === undefined) {
+        // Nothing listed yet, so this call has to wait – bounded, since it is
+        // holding the tool mutex, and a later background refresh still lands
+        await updateRoots(ROOTS_REQUEST_TIMEOUT);
+      } else {
+        // Carry the known roots over and refresh out of band, so a reconnect
+        // never pays for a client round-trip
+        context.setRoots(lastRoots);
+        void updateRoots();
+      }
     }
     return context;
   }

tests/index.test.ts

@@ -9,6 +9,7 @@ import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
 import {describe, it} from 'node:test';
+import {pathToFileURL} from 'node:url';
 
 import {Client} from '@modelcontextprotocol/sdk/client/index.js';
 import {StdioClientTransport} from '@modelcontextprotocol/sdk/client/stdio.js';
@@ -285,6 +286,89 @@ describe('e2e', () => {
     );
   });
 
+  it('does not block tools if the client never answers roots/list', async () => {
+    await withClient(
+      async client => {
+        // A client that negotiates roots but never responds. getContext()
+        // awaits updateRoots() while holding the tool mutex, so an unbounded
+        // request would stall this call for the SDK default of 60s.
+        client.setRequestHandler(ListRootsRequestSchema, () => {
+          return new Promise<never>(() => {
+            // Intentionally never settles
+          });
+        });
+
+        const start = Date.now();
+        // Raise the client-side timeout above the SDK default so an unbounded
+        // roots request surfaces as the assertion below rather than a timeout
+        const result = await client.callTool(
+          {
+            name: 'list_pages',
+            arguments: {},
+          },
+          undefined,
+          {timeout: 90_000},
+        );
+        const elapsed = Date.now() - start;
+
+        assert.strictEqual(result.isError, undefined);
+        // Bounded roots request plus browser launch settles well under this,
+        // leaving room for a slow CI runner while still catching the 60s stall
+        assert.ok(
+          elapsed < 45_000,
+          `list_pages took ${elapsed}ms, expected the bounded roots request to settle well before the 60s SDK default`,
+        );
+      },
+      [],
+      {
+        capabilities: {
+          roots: {listChanged: true},
+        },
+      },
+    );
+  });
+
+  it('still applies roots from a client slower than the bound', async () => {
+    const workspace = await fs.promises.mkdtemp(
+      path.join(os.homedir(), '.roots-slow-client-'),
+    );
+    try {
+      await withClient(
+        async client => {
+          // Answers after the bound the blocking call uses, so the roots only
+          // arrive via the background listing
+          client.setRequestHandler(ListRootsRequestSchema, async () => {
+            await new Promise(resolve => setTimeout(resolve, 8_000));
+            return {
+              roots: [{uri: pathToFileURL(workspace).href, name: 'workspace'}],
+            };
+          });
+
+          await client.callTool({name: 'list_pages', arguments: {}});
+          await new Promise(resolve => setTimeout(resolve, 5_000));
+
+          const result = await client.callTool({
+            name: 'take_screenshot',
+            arguments: {filePath: path.join(workspace, 'shot.png')},
+          });
+
+          // Asserted before isError so a denial reports the path it rejected
+          const content = result.content as TextContent[];
+          assert.match(content[0].text, /Saved screenshot to/);
+          assert.strictEqual(result.isError, undefined);
+        },
+        [],
+        {
+          capabilities: {
+            roots: {listChanged: true},
+          },
+        },
+      );
+    } finally {
+      await fs.promises.rm(workspace, {recursive: true, force: true});
+    }
+  });
+
   describe('Dialogs', () => {
     async function createNewPageAndTriggerDialog(client: Client) {
       // Navigate to a page with a button that triggers a dialog on click