v1.0.0: cherry-pick the #5101 roots refresh barrier onto the release branch

src/filesystem/CHANGELOG.md

@@ -25,3 +25,5 @@ The first semantic version, resuming semver after the date-stamped releases (las
 - [#5094](https://github.com/modelcontextprotocol/servers/issues/5094) [`bea1c9a`](https://github.com/modelcontextprotocol/servers/commit/bea1c9a1bd623aea093a4408b0f71e95cd0e1d03) [@cliffhall](https://github.com/cliffhall) - A `roots/list_changed` update that leaves no valid root now revokes access once the client's roots are in force, instead of keeping the previous allowed directories, so a client that withdraws its roots withdraws the server's access too (#5094). Every path is then refused with "Access denied - no allowed directories" until the client exposes a root again. Before any root has been in force, such an update still keeps the command-line directories.
 
 - [#5097](https://github.com/modelcontextprotocol/servers/issues/5097) [`a13e809`](https://github.com/modelcontextprotocol/servers/commit/a13e809d95d6771159483a69e707d0466a9fe1b1) [@cliffhall](https://github.com/cliffhall) - Overlapping roots refreshes now take effect in the order they started: when a client sends `roots/list_changed` again before the previous `roots/list` answer arrived (or while the initial roots are still loading), an answer that arrives after a newer refresh has started is discarded instead of overwriting it, so a stale answer can no longer undo a newer update such as a revocation (#5097).
+
+- [#5101](https://github.com/modelcontextprotocol/servers/issues/5101) [`3f6aaf4`](https://github.com/modelcontextprotocol/servers/commit/3f6aaf4ff6b3d48243f8a94f1ea8dafb574de5ad) [@cliffhall](https://github.com/cliffhall) - Tool calls that arrive while a `roots/list_changed` refresh is fetching the client's roots now wait for it, as they already did for the initial roots, so a call can no longer reach a root the client has just withdrawn while the answer is pending (#5101).

src/filesystem/README.md

@@ -51,6 +51,7 @@ The server's directory access control follows this flow:
    - Tool calls that arrive before the initial roots are loaded wait for them, so they are checked against the client's roots
    - **On runtime updates**: Client can send `notifications/roots/list_changed`
    - Server requests updated roots and replaces allowed directories again
+   - Tool calls that arrive while an update is being fetched wait for it, so none runs against a root the client has just withdrawn
    - Once the client's roots are in force, an update with no valid roots revokes access: every path is refused ("Access denied - no allowed directories") until the client exposes a root again. Before any root has been in force, such an update keeps the command-line directories
 
 4. **Fallback Behavior** (if client doesn't support roots)

src/filesystem/__tests__/server-roots.test.ts

@@ -502,6 +502,206 @@ describe("roots/list_changed", () => {
     );
   });
 
+  // #5101: a call made while a runtime refresh is pending waits for it, so it
+  // cannot reach a root the client has just withdrawn.
+  it("holds tool calls while a runtime refresh is pending (#5101)", async () => {
+    let calls = 0;
+    let releaseUpdate!: () => void;
+    const updateHeld = new Promise<void>(
+      (resolve) => (releaseUpdate = resolve),
+    );
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) return [rootOf(rootDir)];
+        await updateHeld; // the withdrawal is answered slowly
+        return [];
+      },
+    });
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
+    );
+
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(2));
+    let settled = false;
+    const pending = call(client, "list_directory", { path: rootDir }).then(
+      (result) => {
+        settled = true;
+        return result;
+      },
+    );
+    await new Promise((resolve) => setTimeout(resolve, 50));
+    expect(settled).toBe(false);
+
+    releaseUpdate();
+    expect(textOf(await pending)).toMatch(
+      /^Access denied - no allowed directories/,
+    );
+  });
+
+  it("releases a call queued behind a runtime refresh that fails (#5101)", async () => {
+    await fs.writeFile(path.join(rootDir, "a.txt"), "a");
+    let calls = 0;
+    let failUpdate!: () => void;
+    const updateHeld = new Promise<void>((resolve) => (failUpdate = resolve));
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) return [rootOf(rootDir)];
+        await updateHeld;
+        throw new Error("gone");
+      },
+    });
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
+    );
+
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(2));
+    // Queued while the refresh is still pending, before it fails.
+    let settled = false;
+    const pending = call(client, "list_directory", { path: rootDir }).then(
+      (result) => {
+        settled = true;
+        return result;
+      },
+    );
+    await new Promise((resolve) => setTimeout(resolve, 50));
+    expect(settled).toBe(false);
+
+    failUpdate();
+    const result = await pending;
+    expect(result.isError).toBeFalsy();
+    expect(textOf(result)).toBe("[FILE] a.txt");
+    expect(stderr).toHaveBeenCalledWith(
+      "Failed to request roots from client:",
+      expect.stringContaining("gone"),
+    );
+  });
+
+  // A client may announce a newer change while answering an older refresh's
+  // roots/list; the older refresh must not replace the newer one's barrier.
+  it("keeps the newest barrier when a client's roots/list answer announces another change (#5101)", async () => {
+    let calls = 0;
+    let announce: () => Promise<void> = async () => {};
+    let releaseNewest!: () => void;
+    const newestHeld = new Promise<void>(
+      (resolve) => (releaseNewest = resolve),
+    );
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) return [rootOf(rootDir)];
+        if (calls === 2) {
+          // Discarded on purpose: the server handles the notification on its
+          // own schedule, and the test waits for its roots/list instead.
+          void announce();
+          return [rootOf(rootDir)];
+        }
+        await newestHeld; // the newest change withdraws the root, slowly
+        return [];
+      },
+    });
+    announce = () => client.sendRootsListChanged();
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
+    );
+
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(3));
+    let settled = false;
+    const pending = call(client, "list_directory", { path: rootDir }).then(
+      (result) => {
+        settled = true;
+        return result;
+      },
+    );
+    await new Promise((resolve) => setTimeout(resolve, 50));
+    expect(settled).toBe(false);
+
+    releaseNewest();
+    expect(textOf(await pending)).toMatch(
+      /^Access denied - no allowed directories/,
+    );
+  });
+
+  // An update handled before the deferred initial roots/list runs must still
+  // count as newer than the initial load (#5101).
+  it("applies an update announced right after initialize over the initial roots (#5101)", async () => {
+    let calls = 0;
+    let releaseInitial!: () => void;
+    const initialHeld = new Promise<void>(
+      (resolve) => (releaseInitial = resolve),
+    );
+    const conn = connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) {
+          await initialHeld;
+          return [rootOf(rootDir)];
+        }
+        return [rootOf(cliDir)];
+      },
+    });
+    const { client } = await conn;
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(2));
+    releaseInitial();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Discarded a stale roots/list answer: a newer roots update has started",
+      ),
+    );
+    expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
+  });
+
+  // A call waiting on an older refresh must not stay blocked on it once a newer
+  // refresh has applied the client's current roots (#5101).
+  it("wakes a call waiting on an older refresh once a newer one finishes (#5101)", async () => {
+    await fs.writeFile(path.join(rootDir, "a.txt"), "a");
+    let calls = 0;
+    let releaseOlder!: () => void;
+    const olderHeld = new Promise<void>((resolve) => (releaseOlder = resolve));
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) return [rootOf(rootDir)];
+        if (calls === 2) {
+          await olderHeld; // not answered until the end of the test
+          return [];
+        }
+        return [rootOf(rootDir)];
+      },
+    });
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
+    );
+
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(2));
+    const pending = call(client, "list_directory", { path: rootDir });
+    await new Promise((resolve) => setTimeout(resolve, 50));
+    await client.sendRootsListChanged();
+
+    expect(textOf(await pending)).toBe("[FILE] a.txt");
+    expect(calls).toBe(3);
+
+    // Cleanup: the older answer arrives last and is discarded as stale.
+    releaseOlder();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Discarded a stale roots/list answer: a newer roots update has started",
+      ),
+    );
+    expect(await allowedDirectoriesOf(client)).toEqual([rootDir]);
+  });
+
   it("logs and keeps the current directories when the re-fetch fails", async () => {
     let fail = false;
     const { client } = await connectTracked([], {

src/filesystem/server.ts

@@ -189,31 +189,37 @@ export function createServer(
     version: SERVER_VERSION,
   });
 
-  // Settles once the post-initialize setup (fetching the client's initial
-  // roots) has finished, successfully or not; it never rejects. Every tool call
-  // waits for it, so a call that arrives while the initial roots/list is still
-  // outstanding is checked against the client's roots rather than against the
-  // command-line directories (#3204). It is replaced in oninitialized, which
-  // the SDK runs before any request sent after notifications/initialized.
-  let initialization: Promise<void> = Promise.resolve();
-  // True from the initial roots/list until a refresh's answer has been
-  // applied or the newest refresh has failed. A roots/list_changed that
-  // overtakes the initial load while this holds replaces `initialization`, so
-  // waiting tool calls keep waiting for the refresh that will actually apply
-  // rather than running against the command-line directories (#5097).
-  let initialRootsPending = false;
+  // The barrier every tool call waits on, so a call that arrives while roots are
+  // being fetched is checked against the roots that come back rather than the
+  // ones being replaced: the command-line directories while the initial
+  // roots/list is outstanding (#3204), or a root the client has just withdrawn
+  // while a roots/list_changed refresh is pending (#5101). Each refresh (the
+  // initial load included) installs a new barrier before it sends roots/list,
+  // and settles it when it finishes, successfully or not. Installing one also
+  // settles the barrier it replaces, so calls waiting on an older refresh wake
+  // at once and move on to the newer one instead of waiting for the older
+  // one's answer (#5097, #5101).
+  let barrier = { settled: Promise.resolve(), settle: () => {} };
+  function installBarrier(): () => void {
+    let settle!: () => void;
+    const settled = new Promise<void>((resolve) => (settle = resolve));
+    const replaced = barrier;
+    barrier = { settled, settle };
+    replaced.settle();
+    return settle;
+  }
   const registerTool = server.registerTool.bind(server);
   server.registerTool = ((
     name: string,
     config: Parameters<typeof registerTool>[1],
     handler: (...args: unknown[]) => unknown,
   ) =>
     registerTool(name, config, (async (...args: unknown[]) => {
-      let awaited: Promise<void>;
+      let awaited: typeof barrier;
       do {
-        awaited = initialization;
-        await awaited;
-      } while (awaited !== initialization);
+        awaited = barrier;
+        await awaited.settled;
+      } while (awaited !== barrier);
       return handler(...args);
     }) as never)) as typeof server.registerTool;
 
@@ -813,41 +819,41 @@ export function createServer(
     RootsListChangedNotificationSchema,
     async () => {
       const generation = ++rootsGeneration;
-      const refresh = (async () => {
-        try {
-          // Request the updated roots list from the client
-          const response = await server.server.listRoots();
-          /* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the implicit else cannot run */
-          if (response && "roots" in response) {
-            await updateAllowedDirectoriesFromRoots(response.roots, generation);
-          }
-        } catch (error) {
-          console.error(
-            "Failed to request roots from client:",
-            error instanceof Error ? error.message : String(error),
-          );
-        } finally {
-          if (generation === rootsGeneration) initialRootsPending = false;
+      const settle = installBarrier();
+      try {
+        // Request the updated roots list from the client
+        const response = await server.server.listRoots();
+        /* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the implicit else cannot run */
+        if (response && "roots" in response) {
+          await updateAllowedDirectoriesFromRoots(response.roots, generation);
         }
-      })();
-      // Overtaking the initial load: tool calls wait for this refresh instead.
-      if (initialRootsPending) initialization = refresh;
-      await refresh;
+      } catch (error) {
+        console.error(
+          "Failed to request roots from client:",
+          error instanceof Error ? error.message : String(error),
+        );
+      } finally {
+        settle();
+      }
     },
   );
 
   // Handles post-initialization setup, specifically checking for and fetching
-  // MCP roots. Tool calls wait for it (see `initialization` above).
+  // MCP roots. The initial load claims its generation and installs its barrier
+  // here, synchronously, before any roots/list_changed handled afterwards can
+  // claim a newer one.
   server.server.oninitialized = () => {
-    initialization = initialize();
+    const generation = ++rootsGeneration;
+    const settle = installBarrier();
+    // Not awaited: oninitialized is synchronous. Tool calls wait on the
+    // barrier, which settles when initialize() finishes; it never rejects.
+    void initialize(generation).finally(settle);
   };
 
-  async function initialize(): Promise<void> {
+  async function initialize(generation: number): Promise<void> {
     const clientCapabilities = server.server.getClientCapabilities();
 
     if (clientCapabilities?.roots) {
-      const generation = ++rootsGeneration;
-      initialRootsPending = true;
       try {
         const response = await server.server.listRoots();
         /* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the else cannot run */
@@ -863,8 +869,6 @@ export function createServer(
           "Failed to request initial roots from client:",
           error instanceof Error ? error.message : String(error),
         );
-      } finally {
-        if (generation === rootsGeneration) initialRootsPending = false;
       }
     } else {
       if (allowedDirectories.length > 0) {