fix(filesystem): timeouts + max-visited cap for hangs on lazy provider paths (#4162)

fs.realpath (in validatePath) and recursive fs.readdir (in search_files /
directory_tree) are called with no timeout, and recursive search has no
upper bound on entries visited. On macOS provider-backed paths
(~/Library/CloudStorage/..., ~/Library/Mobile Documents/com~apple~CloudDocs/...,
NFS/SMB) these can block for minutes while the provider fetches metadata; the
host eventually reports the connector unresponsive while the subprocess stays
stuck. Follow-up to #4181, which only skipped excluded entries before realpath.

lib.ts:
- withFsTimeout() wraps fs.realpath and fs.readdir; on timeout rejects with a
  typed FsTimeoutError instead of hanging. A no-op catch on the timeout promise
  avoids spurious unhandled-rejection reports when the operation wins the race.
- FS_SEARCH_MAX_VISITED caps recursive search; abort produces a clear
  "narrow the path / refine the pattern" error.
- FS_SEARCH_EXCLUDE_PREFIXES (opt-in, empty default) refuses recursive
  operations on configured slow roots, via the shared boundary-aware
  assertSearchPathNotExcluded helper (reuses isPathWithinAllowedDirectories,
  expandHome applied symmetrically to the candidate root).
- ENOENT branch in validatePath refactored so a hung parent realpath surfaces
  as FsTimeoutError instead of being masked as "Parent directory does not exist".
- Per-entry catch in search re-throws FsTimeoutError so a search never returns
  silently-partial results.

index.ts:
- directory_tree gets the same timeout + visited-cap treatment and shares the
  exclude-prefix helper; throws immediately on cap-exceed rather than threading
  an aborted flag through unwinding recursion.

Config (all env-var, conservative defaults; invalid values warn + fall back):
  FS_OP_TIMEOUT_MS=15000, FS_SEARCH_MAX_VISITED=50000, FS_SEARCH_EXCLUDE_PREFIXES=(empty)

README documents the three vars, the NFS/SMB tuning note, and that
FS_SEARCH_EXCLUDE_PREFIXES is a lexical prefilter (symlink-aware excludes
tracked in #4208).

Tests: new __tests__/4162-timeouts-and-caps.test.ts (14 cases) using vitest
fake timers + mocked fs/promises, so no real CloudStorage path is needed in CI.
Full suite 160/160. Verified on a real macOS Google Drive tree by @alemuenchen.

Deliberately out of scope: libuv threadpool concurrency, fs.opendir streaming
for huge single directories, symlink-aware excludes (#4208).

Co-authored-by: Alessandro <alemuenchen@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: 3 people

src/filesystem/README.md

@@ -204,6 +204,22 @@ The mapping for filesystem tools is:
 
 > Note: `idempotentHint` and `destructiveHint` are meaningful only when `readOnlyHint` is `false`, as defined by the MCP spec.
 
+## Environment variables
+
+Three environment variables guard against hangs when `allowed_directories` includes a slow or lazy provider-backed path (macOS `~/Library/CloudStorage/...`, `~/Library/Mobile Documents/com~apple~CloudDocs/...`, NFS / SMB mounts, etc.). Defaults are safe for ordinary local trees; tune only if needed.
+
+| Variable | Default | Purpose |
+|---|---|---|
+| `FS_OP_TIMEOUT_MS` | `15000` | Timeout (milliseconds) applied to each `fs.realpath` (in `validatePath`) and `fs.readdir` (in `search_files` and `directory_tree`). On timeout the call rejects with `FsTimeoutError` instead of hanging. |
+| `FS_SEARCH_MAX_VISITED` | `50000` | Hard cap on entries visited by a single `search_files` or `directory_tree` call before aborting with `"Search aborted after visiting N entries..."`. Prevents an unbounded recursion on a large or lazy-materialized tree. |
+| `FS_SEARCH_EXCLUDE_PREFIXES` | (empty) | Comma-separated path prefixes for which recursive search and `directory_tree` are refused outright. `~` is expanded. Matching is boundary-aware: a configured `/data/foo` will not match an unrelated `/data/foobar`. Intended for known-slow roots you'd rather forbid than time out on. |
+
+Invalid numeric values (`"15s"`, `"0"`, negative) print a warning to stderr and fall back to the default, so a typo cannot silently disable a guard.
+
+**Remote-mount tuning:** the 15 s default works well for local trees and warm CloudStorage. On NFS, SMB, or other remote mounts where individual `readdir` calls legitimately take seconds, consider raising `FS_OP_TIMEOUT_MS` (e.g. `30000`–`60000`) to avoid spurious timeouts.
+
+**`FS_SEARCH_EXCLUDE_PREFIXES` is a fast lexical *prefilter*, not a canonical-path exclusion.** The requested root is compared (after `~` expansion and normalisation) against the configured prefixes, with no `realpath` step. A symlink whose target lives under an excluded prefix is therefore **not** blocked — the request would pass the lexical exclude and may still hit the slow provider path through `validatePath`'s realpath. Treat this as a best-effort guard against direct submissions of slow paths, not as a complete symlink-aware exclusion. Security against paths escaping `allowed_directories` is enforced separately by `validatePath`.
+
 ## Usage with Claude Desktop
 Add this to your `claude_desktop_config.json`:
 

src/filesystem/__tests__/4162-timeouts-and-caps.test.ts

@@ -0,0 +1,306 @@
+// Regression tests for issue #4162: recursive search / validatePath can hang
+// for minutes on macOS CloudStorage / lazy provider-backed paths. The patch
+// adds:
+//   - a timeout wrapper around fs.realpath in validatePath
+//   - a timeout wrapper around fs.readdir in searchFilesWithValidation
+//   - a max-visited-entries cap on recursive search
+//   - boundary-aware FS_SEARCH_EXCLUDE_PREFIXES that refuses recursive search
+//     outright on user-configured roots
+//
+// These tests use fake timers to exercise the timeout paths without slow real
+// waits, and mock fs/promises at the module level so no real filesystem state
+// is touched.
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import fs from 'fs/promises';
+import path from 'path';
+import type { Dirent } from 'fs';
+
+vi.mock('fs/promises');
+const mockFs = fs as any;
+
+// The diff reads its env vars at module load. We don't try to retune them per
+// test (which would need module re-import dance); we trust the defaults and
+// drive behaviour by making the mocked fs call hang past the default timeout
+// or by enumerating enough mocked entries to exceed the default cap.
+//
+// Default timeout is 15s (FS_OP_TIMEOUT_MS). With fake timers we just advance
+// past that — no real wait.
+// Default visited cap is 50_000 (FS_SEARCH_MAX_VISITED). We mock readdir to
+// return one directory containing that many synthetic entries.
+
+const ALLOWED = process.platform === 'win32' ? ['C:\\allowed'] : ['/allowed'];
+const ROOT = process.platform === 'win32' ? 'C:\\allowed' : '/allowed';
+const FILE = process.platform === 'win32' ? 'C:\\allowed\\file.txt' : '/allowed/file.txt';
+
+function mkDirent(name: string, isDir = false): Dirent {
+  return {
+    name,
+    isFile: () => !isDir,
+    isDirectory: () => isDir,
+    isSymbolicLink: () => false,
+    isBlockDevice: () => false,
+    isCharacterDevice: () => false,
+    isFIFO: () => false,
+    isSocket: () => false,
+    parentPath: '',
+    path: '',
+  } as unknown as Dirent;
+}
+
+describe('issue #4162: timeouts + max-visited cap', () => {
+  let setAllowedDirectories: typeof import('../lib.js').setAllowedDirectories;
+  let validatePath: typeof import('../lib.js').validatePath;
+  let searchFilesWithValidation: typeof import('../lib.js').searchFilesWithValidation;
+
+  beforeEach(async () => {
+    vi.resetModules();
+    vi.clearAllMocks();
+    const lib = await import('../lib.js');
+    setAllowedDirectories = lib.setAllowedDirectories;
+    validatePath = lib.validatePath;
+    searchFilesWithValidation = lib.searchFilesWithValidation;
+    setAllowedDirectories(ALLOWED);
+  });
+
+  afterEach(() => {
+    setAllowedDirectories([]);
+    vi.useRealTimers();
+    vi.restoreAllMocks();
+  });
+
+  describe('validatePath: fs.realpath timeout', () => {
+    it('rejects with FsTimeoutError when realpath hangs past FS_OP_TIMEOUT_MS', async () => {
+      vi.useFakeTimers();
+      // realpath returns a promise that never settles — the previous code would
+      // hang forever; the patch must surface a timeout.
+      mockFs.realpath.mockImplementation(() => new Promise(() => {}));
+
+      const op = validatePath(FILE);
+      // Attach a no-op catch BEFORE timer fires, so the rejection is "handled"
+      // by the time vi.advanceTimersByTimeAsync flushes microtasks. Without
+      // this, vitest's strict unhandled-rejection checker trips even though
+      // expect.rejects below will observe the same rejection.
+      op.catch(() => {});
+      // Advance past the 15s default. We use 20s for a margin.
+      await vi.advanceTimersByTimeAsync(20_000);
+      await expect(op).rejects.toThrow(/timed out/i);
+    });
+
+    it('preserves ENOENT path: surfaces "Parent directory does not exist" when both realpaths reject ENOENT', async () => {
+      // Regression for the ENOENT-branch refactor in the patch — a non-existent
+      // file with a non-existent parent must still produce the original error,
+      // not a misleading timeout.
+      const enoent = (): NodeJS.ErrnoException => {
+        const e = new Error('ENOENT') as NodeJS.ErrnoException;
+        e.code = 'ENOENT';
+        return e;
+      };
+      mockFs.realpath
+        .mockRejectedValueOnce(enoent())
+        .mockRejectedValueOnce(enoent());
+
+      await expect(
+        validatePath(
+          process.platform === 'win32'
+            ? 'C:\\allowed\\missing\\file.txt'
+            : '/allowed/missing/file.txt',
+        ),
+      ).rejects.toThrow(/Parent directory does not exist/);
+    });
+
+    it('ENOENT-branch realpath timeout surfaces as FsTimeoutError, not masked as missing-parent', async () => {
+      // Verifies the refactor: a hung parent realpath must propagate the
+      // timeout rather than collapse into "Parent directory does not exist".
+      vi.useFakeTimers();
+      const enoent = new Error('ENOENT') as NodeJS.ErrnoException;
+      enoent.code = 'ENOENT';
+      mockFs.realpath
+        .mockRejectedValueOnce(enoent)
+        .mockImplementationOnce(() => new Promise(() => {})); // parent hangs
+
+      const op = validatePath(
+        process.platform === 'win32'
+          ? 'C:\\allowed\\new\\file.txt'
+          : '/allowed/new/file.txt',
+      );
+      op.catch(() => {}); // see note above
+      await vi.advanceTimersByTimeAsync(20_000);
+      await expect(op).rejects.toThrow(/timed out/i);
+    });
+  });
+
+  describe('searchFilesWithValidation: fs.readdir timeout', () => {
+    it('rejects with FsTimeoutError when readdir hangs past FS_OP_TIMEOUT_MS', async () => {
+      vi.useFakeTimers();
+      mockFs.realpath.mockImplementation(async (p: any) => p.toString());
+      mockFs.readdir.mockImplementation(() => new Promise(() => {}));
+
+      const op = searchFilesWithValidation(ROOT, '**/*', ALLOWED);
+      op.catch(() => {}); // see note above
+      await vi.advanceTimersByTimeAsync(20_000);
+      await expect(op).rejects.toThrow(/timed out/i);
+    });
+
+    it('per-entry timeout propagates instead of being swallowed by catch-continue', async () => {
+      // Critical for the patch: the existing per-entry `catch { continue }`
+      // must re-throw FsTimeoutError so a search never returns silently-partial
+      // results. We make readdir succeed once with two entries, the second of
+      // which has a realpath that hangs.
+      vi.useFakeTimers();
+      mockFs.readdir.mockResolvedValueOnce([
+        mkDirent('a.txt'),
+        mkDirent('b.txt'),
+      ]);
+      // First entry's realpath resolves fine, second hangs forever.
+      let call = 0;
+      mockFs.realpath.mockImplementation((p: any) => {
+        call++;
+        if (call === 1) return Promise.resolve(p.toString());
+        return new Promise(() => {});
+      });
+
+      const op = searchFilesWithValidation(ROOT, '**/*', ALLOWED);
+      op.catch(() => {}); // see note above
+      await vi.advanceTimersByTimeAsync(20_000);
+      await expect(op).rejects.toThrow(/timed out/i);
+    });
+  });
+
+  describe('searchFilesWithValidation: FS_SEARCH_MAX_VISITED cap', () => {
+    // Drive the cap via env-var rather than a 50_001-entry fixture: cleaner,
+    // faster, and exercises the same code path. Module is re-imported per test
+    // so the module-level read of FS_SEARCH_MAX_VISITED picks up our value.
+    beforeEach(async () => {
+      vi.resetModules();
+      process.env.FS_SEARCH_MAX_VISITED = '5';
+      const lib = await import('../lib.js');
+      setAllowedDirectories = lib.setAllowedDirectories;
+      searchFilesWithValidation = lib.searchFilesWithValidation;
+      setAllowedDirectories(ALLOWED);
+    });
+
+    afterEach(() => {
+      delete process.env.FS_SEARCH_MAX_VISITED;
+    });
+
+    it('aborts with a clear error once the visited cap is exceeded', async () => {
+      const entries: Dirent[] = [];
+      for (let i = 0; i < 10; i++) entries.push(mkDirent(`f${i}.txt`));
+      mockFs.readdir.mockResolvedValue(entries);
+      mockFs.realpath.mockImplementation(async (p: any) => p.toString());
+
+      await expect(
+        searchFilesWithValidation(ROOT, '**/*', ALLOWED),
+      ).rejects.toThrow(/Search aborted after visiting/);
+    });
+
+    it('returns normally when entry count is well under the cap', async () => {
+      mockFs.readdir.mockResolvedValueOnce([
+        mkDirent('hit.txt'),
+        mkDirent('miss.log'),
+      ]);
+      mockFs.realpath.mockImplementation(async (p: any) => p.toString());
+
+      const out = await searchFilesWithValidation(ROOT, '**/*.txt', ALLOWED);
+      expect(out.some(p => p.endsWith('hit.txt'))).toBe(true);
+      expect(out.some(p => p.endsWith('miss.log'))).toBe(false);
+    });
+  });
+
+  describe('FS_SEARCH_EXCLUDE_PREFIXES', () => {
+    // Set the env var BEFORE importing lib.ts so the module-level read picks
+    // it up. We isolate this in its own describe so vi.resetModules() rewinds
+    // cleanly between tests.
+    const EXCLUDED = process.platform === 'win32' ? 'C:\\allowed\\slow' : '/allowed/slow';
+
+    beforeEach(async () => {
+      vi.resetModules();
+      process.env.FS_SEARCH_EXCLUDE_PREFIXES = EXCLUDED;
+      const lib = await import('../lib.js');
+      setAllowedDirectories = lib.setAllowedDirectories;
+      searchFilesWithValidation = lib.searchFilesWithValidation;
+      setAllowedDirectories(ALLOWED);
+    });
+
+    afterEach(() => {
+      delete process.env.FS_SEARCH_EXCLUDE_PREFIXES;
+    });
+
+    it('refuses recursive search when root is inside an excluded prefix', async () => {
+      await expect(
+        searchFilesWithValidation(EXCLUDED, '**/*', ALLOWED),
+      ).rejects.toThrow(/Recursive search is disabled/);
+    });
+
+    it('does not match an unrelated sibling that merely shares a string head (boundary-aware)', async () => {
+      // /allowed/slowdown must NOT be excluded just because '/allowed/slow' is
+      // a substring of it. This is the containment-via-isPathWithinAllowedDirectories
+      // guarantee from the patch.
+      const sibling = process.platform === 'win32' ? 'C:\\allowed\\slowdown' : '/allowed/slowdown';
+      mockFs.readdir.mockResolvedValueOnce([mkDirent('ok.txt')]);
+      mockFs.realpath.mockImplementation(async (p: any) => p.toString());
+
+      const out = await searchFilesWithValidation(sibling, '**/*', ALLOWED);
+      expect(out.length).toBeGreaterThanOrEqual(0); // did not throw
+    });
+
+    it('shared helper assertSearchPathNotExcluded customises the operation name', async () => {
+      // The same containment check is reused by the directory_tree handler in
+      // index.ts via assertSearchPathNotExcluded(rootPath, 'directory_tree').
+      // We can't unit-test the inline handler directly, but we can exercise
+      // the shared helper to confirm the error wording reflects the opName.
+      const { assertSearchPathNotExcluded } = await import('../lib.js');
+      expect(() => assertSearchPathNotExcluded(EXCLUDED, 'directory_tree'))
+        .toThrow(/directory_tree is disabled.*FS_SEARCH_EXCLUDE_PREFIXES/);
+    });
+
+    it('helper rejects descendants of an excluded prefix, not just the exact root', async () => {
+      const { assertSearchPathNotExcluded } = await import('../lib.js');
+      const descendant = process.platform === 'win32'
+        ? 'C:\\allowed\\slow\\nested\\deep'
+        : '/allowed/slow/nested/deep';
+      expect(() => assertSearchPathNotExcluded(descendant, 'directory_tree'))
+        .toThrow(/directory_tree is disabled/);
+    });
+
+    it('helper allows a sibling that merely shares a textual prefix with the excluded root', async () => {
+      // /allowed/slowdown is NOT a descendant of /allowed/slow even though
+      // the latter is a string prefix -- the containment check is boundary-aware.
+      const { assertSearchPathNotExcluded } = await import('../lib.js');
+      const sibling = process.platform === 'win32' ? 'C:\\allowed\\slowdown' : '/allowed/slowdown';
+      expect(() => assertSearchPathNotExcluded(sibling, 'directory_tree')).not.toThrow();
+    });
+  });
+
+  describe('env-var validation', () => {
+    afterEach(() => {
+      delete process.env.FS_OP_TIMEOUT_MS;
+      delete process.env.FS_SEARCH_MAX_VISITED;
+    });
+
+    it('ignores non-numeric env values and warns to stderr', async () => {
+      const warn = vi.spyOn(console, 'error').mockImplementation(() => {});
+      process.env.FS_OP_TIMEOUT_MS = '15s'; // intentionally invalid
+
+      vi.resetModules();
+      await import('../lib.js');
+
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringMatching(/Ignoring invalid FS_OP_TIMEOUT_MS="15s"/),
+      );
+    });
+
+    it('ignores zero/negative values and warns to stderr', async () => {
+      const warn = vi.spyOn(console, 'error').mockImplementation(() => {});
+      process.env.FS_SEARCH_MAX_VISITED = '0';
+
+      vi.resetModules();
+      await import('../lib.js');
+
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringMatching(/Ignoring invalid FS_SEARCH_MAX_VISITED="0"/),
+      );
+    });
+  });
+});

src/filesystem/index.ts

@@ -26,6 +26,10 @@ import {
   tailFile,
   headFile,
   setAllowedDirectories,
+  // #4162: timeout + cap primitives reused by directory_tree
+  withFsTimeout,
+  FS_SEARCH_MAX_VISITED,
+  assertSearchPathNotExcluded,
 } from './lib.js';
 
 // Command line argument parsing
@@ -548,12 +552,36 @@ server.registerTool(
     }
     const rootPath = args.path;
 
+    // #4162: same defensive shape as searchFilesWithValidation -- honour
+    // FS_SEARCH_EXCLUDE_PREFIXES, bound the recursion, and timeout each
+    // readdir so a lazy provider-backed subtree can't keep the server busy
+    // indefinitely. validatePath already wraps fs.realpath via withFsTimeout
+    // in lib.ts, so only readdir + the entry counter need handling here. We
+    // throw immediately when the cap is hit rather than threading an
+    // `aborted` flag through unwinding recursion -- no caller wants a partial
+    // tree, and the immediate throw makes that contract obvious to future
+    // refactors.
+    assertSearchPathNotExcluded(rootPath, 'directory_tree');
+    let visited = 0;
+
     async function buildTree(currentPath: string, excludePatterns: string[] = []): Promise<TreeEntry[]> {
       const validPath = await validatePath(currentPath);
-      const entries = await fs.readdir(validPath, { withFileTypes: true });
+      const entries = await withFsTimeout(
+        fs.readdir(validPath, { withFileTypes: true }),
+        `readdir ${validPath}`,
+      );
       const result: TreeEntry[] = [];
 
       for (const entry of entries) {
+        if (visited >= FS_SEARCH_MAX_VISITED) {
+          throw new Error(
+            `directory_tree aborted after visiting ${visited} entries ` +
+            `(cap FS_SEARCH_MAX_VISITED=${FS_SEARCH_MAX_VISITED}). ` +
+            `Narrow the search path or use excludePatterns.`
+          );
+        }
+        visited++;
+
         const relativePath = path.relative(rootPath, path.join(currentPath, entry.name));
         const shouldExclude = excludePatterns.some(pattern => {
           if (pattern.includes('*')) {