fix(filesystem): wait for roots initialization before handling tool calls

[majiayu000]

Fixes #3204

Description

Tool calls can arrive before oninitialized finishes fetching roots, so allowedDirectories is empty and every path check returns "Access denied". This adds a Promise-based gate that blocks tool handlers until roots are loaded.

The SDK fires oninitialized without awaiting it (() => this.oninitialized?.() in SDK index.js:53). The filesystem server's handler asynchronously calls server.server.listRoots(), but tool handlers run immediately against an empty allowedDirectories.

Server Details

• Server: filesystem
• Changes to: tools (initialization ordering)

Motivation and Context

When CLI args aren't provided and roots come from the client, there's a window where tool calls hit empty allowedDirectories. This is the race condition described in #3204.

How Has This Been Tested?

• New unit tests in __tests__/roots-gate.test.ts covering: immediate resolution, delayed resolution, timeout, rejection, and concurrent waiters.
• All 105 passing tests in the existing suite still pass. (lib.test.ts has a pre-existing vi.mock failure on main, unrelated to this change.)
• npx tsc --noEmit passes clean.

Breaking Changes

None. If CLI args already provide allowed directories, the gate resolves immediately and there's no behavior change. The 10s timeout only applies when waiting for client-provided roots.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Protocol Documentation
• My changes follows MCP security best practices
• I have updated the server's README accordingly
• I have tested this with an LLM client
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have documented all environment variables and configuration options

Additional context

What changed:

1. roots-gate.ts (~25 lines) - createRootsGate(timeoutMs) factory returning { promise, resolve, waitForReady }. Races the internal promise against a configurable timeout.

2. index.ts - imports the gate, resolves it immediately when CLI args provide directories, resolves it in oninitialized after roots load, and adds await gate.waitForReady() at the top of all 13 tool handlers.

3. __tests__/roots-gate.test.ts - 5 test cases for the gate in isolation.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 3s —— View job

---

PR Review: fix(filesystem): wait for roots initialization before handling tool calls

• Read changed files
• Analyze implementation logic
• Check for correctness and edge cases
• Review tests
• Post full review feedback

---

Summary

This PR correctly identifies and fixes a real race condition: when an MCP client provides roots instead of CLI args, oninitialized fires asynchronously (the SDK never awaits it), so tool calls that arrive during that window see an empty allowedDirectories and get blanket "Access denied" errors. The Promise-gate approach is a clean, idiomatic fix.

The overall implementation is sound. A few issues worth addressing:

---

Issues

1. Timer leak in waitForReady() — no clearTimeout after resolution

src/filesystem/roots-gate.ts:19-29

Every call to waitForReady() creates a fresh setTimeout that is never cleared when the gate resolves. If 13 tool handlers call waitForReady() concurrently and the gate resolves at T=1s, 13 ten-second timers are left dangling until T=11s. In production this is harmless (the server process lives on), but in test environments it can hold open the event loop and cause subtle flakiness.

// Current: timer leaks after gate resolves
function waitForReady(): Promise<void> {
  return Promise.race([
    promise,
    new Promise<void>((_, reject) =>
      setTimeout(() => reject(...), timeoutMs)
    ),
  ]);
}

Suggested fix:

function waitForReady(): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    const timer = setTimeout(
      () => reject(new Error(`Roots initialization timed out after ${timeoutMs}ms`)),
      timeoutMs
    );
    promise.then(
      () => { clearTimeout(timer); resolve(); },
      (err) => { clearTimeout(timer); reject(err); }
    );
  });
}

Or if you prefer keeping Promise.race, at minimum add timer.unref() so the timer doesn't prevent clean process exit in tests.

Fix this →

---

2. Timeout is per-call, not global — inconsistent failure at the boundary

Each call to waitForReady() starts its own independent 10-second countdown. If oninitialized takes 9.5 seconds, a tool call that arrived at T=0 will have its timeout fire at T=10 and be rejected — while a tool call that arrived at T=9 gets a fresh timer expiring at T=19 and will resolve successfully once the gate resolves at T=9.5. Two concurrent tool calls can get different outcomes for the same initialization window.

This is a design trade-off worth acknowledging. If global-gate semantics are preferred (all waiters get the same deadline), the timeout should be placed on the gate itself rather than in each waitForReady() call:

// Alternative: single global timeout on the gate
export function createRootsGate(timeoutMs = 10_000): RootsGate {
  let resolve!: () => void;
  const promise = new Promise<void>((res) => { resolve = res; });
  const timer = setTimeout(
    () => reject(new Error(`Roots initialization timed out after ${timeoutMs}ms`)),
    timeoutMs
  );
  // clear timer on resolution
  ...
}

If the current per-call behavior is intentional, it's worth a comment explaining why.

---

3. promise field exposed on RootsGate interface but never used externally

src/filesystem/roots-gate.ts:8

index.ts only calls gate.resolve() and gate.waitForReady(). The promise field leaks the raw internal Promise and isn't needed by callers. Removing it from the interface narrows the public API.

---

4. PR description says 5 tests, file has 4

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

The PR description says "5 test cases" but only 4 are present (resolves immediately, resolves after delay, rejects with timeout, resolves all concurrent waiters). One scenario missing is the post-resolve case: calling waitForReady() after the gate has already resolved should still succeed instantly (not re-create a new timer or block). This is implicitly covered by test 1, but an explicit test would be cleaner. Minor.

---

What's good

• The finally block in oninitialized ensures the gate is always resolved, preventing permanent hangs even when initialization throws (listRoots fails, no client roots, etc.). This is the correct defensive pattern.
• When CLI args supply directories, the gate resolves immediately at module load (rootsGate.resolve() before oninitialized fires), so there is zero overhead for existing deployments.
• The createRootsGate factory is pure and easily testable in isolation.
• Calling resolve() multiple times (e.g., CLI args path resolves it, then oninitialized resolves it again in finally) is safe — the Promise spec ignores subsequent calls to an already-settled resolver.
• Concurrent waiters all unblock at once when the gate resolves (test 4 verifies this).

---

Verdict

The fix is correct and addresses the described race condition. Issues 1 and 2 are worth resolving before merge; issues 3 and 4 are minor polish. The approach is sound.