v1.0.0: cherry-pick the #5097 roots refresh ordering fix onto the release branch

src/filesystem/CHANGELOG.md

@@ -23,3 +23,5 @@ The first semantic version, resuming semver after the date-stamped releases (las
 - [#5009](https://github.com/modelcontextprotocol/servers/pull/5009) [`f621c75`](https://github.com/modelcontextprotocol/servers/commit/f621c75a92347738f28f26c9461286764beba3c8) [@cliffhall](https://github.com/cliffhall) - Paths that differ from the names on disk only in Unicode form now resolve ([#1970](https://github.com/modelcontextprotocol/servers/issues/1970)). An NFC spelling of an allowed directory whose name is stored NFD (macOS "Capture d’écran", Japanese dakuten) is no longer refused as outside the allowed directories, and a request that spells a U+202F or U+00A0 in a name as a plain space (the macOS screenshot "Screenshot … at 2.40.40 PM.png") finds the file instead of failing with ENOENT. The exact spelling still wins when it exists, and a name matching two entries is refused as ambiguous.
 
 - [#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).

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

@@ -345,6 +345,163 @@ describe("roots/list_changed", () => {
     expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
   });
 
+  // #5097: the answers to overlapping refreshes can arrive out of order; only
+  // the newest refresh's answer is applied, so a stale one cannot undo it.
+  it("applies the newest of two overlapping updates when their answers arrive out of order (#5097)", async () => {
+    let calls = 0;
+    let releaseStale!: () => void;
+    const staleHeld = new Promise<void>((resolve) => (releaseStale = resolve));
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) return [rootOf(rootDir)]; // the initial load
+        if (calls === 2) {
+          await staleHeld; // the older update, answered last
+          return [rootOf(rootDir)];
+        }
+        return []; // the newer update withdraws the root
+      },
+    });
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
+    );
+
+    await client.sendRootsListChanged();
+    await vi.waitFor(() => expect(calls).toBe(2));
+    await client.sendRootsListChanged();
+    await vi.waitFor(async () =>
+      expect(await allowedDirectoriesOf(client)).toEqual([]),
+    );
+
+    releaseStale();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Discarded a stale roots/list answer: a newer roots update has started",
+      ),
+    );
+    expect(await allowedDirectoriesOf(client)).toEqual([]);
+  });
+
+  it("discards the initial answer when an update overtakes it (#5097)", async () => {
+    let calls = 0;
+    let releaseInitial!: () => void;
+    const initialHeld = new Promise<void>(
+      (resolve) => (releaseInitial = resolve),
+    );
+    const { client } = await connectTracked([], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) {
+          await initialHeld;
+          return [rootOf(rootDir)];
+        }
+        return [rootOf(cliDir)];
+      },
+    });
+    await vi.waitFor(() => expect(calls).toBe(1));
+    await client.sendRootsListChanged();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Updated allowed directories from MCP roots: 1 valid directories",
+      ),
+    );
+
+    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]);
+  });
+
+  // #5097: a call waiting for the initial roots must not be released by the
+  // stale initial answer; it waits for the update that overtook it.
+  it("holds waiting tool calls until the update that overtook the initial load applies (#5097)", async () => {
+    let calls = 0;
+    let releaseInitial!: () => void;
+    let releaseUpdate!: () => void;
+    const initialHeld = new Promise<void>(
+      (resolve) => (releaseInitial = resolve),
+    );
+    const updateHeld = new Promise<void>(
+      (resolve) => (releaseUpdate = resolve),
+    );
+    const { client } = await connectTracked([cliDir], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        await (calls === 1 ? initialHeld : updateHeld);
+        return [rootOf(rootDir)];
+      },
+    });
+    await vi.waitFor(() => expect(calls).toBe(1));
+    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",
+      ),
+    );
+    // Asked before the update has answered: it must not run against the
+    // command-line directory in the meantime.
+    let settled = false;
+    const pending = call(client, "list_directory", { path: cliDir }).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/);
+    expect(await allowedDirectoriesOf(client)).toEqual([rootDir]);
+  });
+
+  it("releases waiting tool calls when the update that overtook the initial load fails (#5097)", async () => {
+    let calls = 0;
+    let releaseInitial!: () => void;
+    const initialHeld = new Promise<void>(
+      (resolve) => (releaseInitial = resolve),
+    );
+    const { client } = await connectTracked([cliDir], {
+      capabilities: ROOTS,
+      listRoots: async () => {
+        calls += 1;
+        if (calls === 1) {
+          await initialHeld;
+          return [rootOf(rootDir)];
+        }
+        throw new Error("gone");
+      },
+    });
+    await vi.waitFor(() => expect(calls).toBe(1));
+    await client.sendRootsListChanged();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Failed to request roots from client:",
+        "MCP error -32603: gone",
+      ),
+    );
+
+    // Asked while the initial roots/list is still unanswered: the failed update
+    // releases waiting calls instead of leaving them blocked on it.
+    expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
+    // Let the stale initial answer finish before teardown removes its roots.
+    releaseInitial();
+    await vi.waitFor(() =>
+      expect(stderr).toHaveBeenCalledWith(
+        "Discarded a stale roots/list answer: a newer roots update has started",
+      ),
+    );
+  });
+
   it("logs and keeps the current directories when the re-fetch fails", async () => {
     let fail = false;
     const { client } = await connectTracked([], {

src/filesystem/server.ts

@@ -177,6 +177,12 @@ export function createServer(
   // withdraws its roots withdraws the server's access too (#5094). Until then an
   // update with no valid roots keeps the command-line directories.
   let rootsInForce = false;
+  // Each roots refresh (the initial load and every roots/list_changed) takes
+  // the next generation before it asks for roots. The SDK runs notification
+  // handlers without waiting for earlier ones, so answers can come back out of
+  // order; one is applied only if no newer refresh has started since, so a
+  // stale answer cannot undo a newer update, such as a revocation (#5097).
+  let rootsGeneration = 0;
 
   const server = new McpServer({
     name: "secure-filesystem-server",
@@ -190,14 +196,24 @@ export function createServer(
   // 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;
   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[]) => {
-      await initialization;
+      let awaited: Promise<void>;
+      do {
+        awaited = initialization;
+        await awaited;
+      } while (awaited !== initialization);
       return handler(...args);
     }) as never)) as typeof server.registerTool;
 
@@ -763,9 +779,19 @@ export function createServer(
     },
   );
 
-  // Updates allowed directories based on MCP client roots
-  async function updateAllowedDirectoriesFromRoots(requestedRoots: Root[]) {
+  // Updates allowed directories based on MCP client roots, unless a newer
+  // refresh started after this one's `generation` was taken (#5097).
+  async function updateAllowedDirectoriesFromRoots(
+    requestedRoots: Root[],
+    generation: number,
+  ) {
     const validatedRootDirs = await getValidRootDirectories(requestedRoots);
+    if (generation !== rootsGeneration) {
+      console.error(
+        "Discarded a stale roots/list answer: a newer roots update has started",
+      );
+      return;
+    }
     if (validatedRootDirs.length > 0) {
       allowedDirectories = [...validatedRootDirs];
       rootsInForce = true;
@@ -786,19 +812,27 @@ export function createServer(
   server.server.setNotificationHandler(
     RootsListChangedNotificationSchema,
     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);
+      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;
         }
-      } catch (error) {
-        console.error(
-          "Failed to request roots from client:",
-          error instanceof Error ? error.message : String(error),
-        );
-      }
+      })();
+      // Overtaking the initial load: tool calls wait for this refresh instead.
+      if (initialRootsPending) initialization = refresh;
+      await refresh;
     },
   );
 
@@ -812,11 +846,13 @@ export function createServer(
     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 */
         if (response && "roots" in response) {
-          await updateAllowedDirectoriesFromRoots(response.roots);
+          await updateAllowedDirectoriesFromRoots(response.roots, generation);
         } else {
           console.error(
             "Client returned no roots set, keeping current settings",
@@ -827,6 +863,8 @@ 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) {