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

[JonoGitty]

Description

Bounds two unbounded code paths in the filesystem server that can hang for minutes on macOS CloudStorage / FileProvider-backed directories: fs.realpath in validatePath, and recursive fs.readdir in search_files / directory_tree. Addresses the structural concerns from #4162 that #4181 (excludePatterns before realpath) left open.

Follow-up design question about symlink-aware excludes is tracked separately in #4208 — intentionally not in scope here.

Server Details

• Server: filesystem
• Changes to: tools (search_files, directory_tree, internal validatePath)

Motivation and Context

fs.realpath and fs.readdir are called with no timeout. On macOS provider-backed paths (~/Library/CloudStorage/..., ~/Library/Mobile Documents/com~apple~CloudDocs/..., and NFS/SMB), these can block while the provider fetches metadata; recursive search additionally has no upper bound on entries visited. The host (e.g. Claude Desktop) eventually reports the connector unresponsive (~4 min) while the subprocess stays alive but stuck. See #4162 for the full report.

The defensive shape here originated from @alemuenchen, who has run equivalent mitigations in production against Google Drive + iCloud Drive for several weeks and contributed the reference diff on #4162.

What this adds

src/filesystem/lib.ts:

• withFsTimeout() wraps fs.realpath (in validatePath) and fs.readdir (in searchFilesWithValidation); on timeout it rejects with a typed FsTimeoutError rather than hanging.
• 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).
• ENOENT branch in validatePath refactored so a hung parent realpath surfaces as FsTimeoutError instead of being masked as "Parent directory does not exist".
• The per-entry catch in search re-throws FsTimeoutError so a search never returns silently-partial results.

src/filesystem/index.ts:

• directory_tree gets the same timeout + visited-cap treatment and shares the exclude-prefix helper.

Configuration

Variable	Default	Purpose
FS_OP_TIMEOUT_MS	15000	Timeout (ms) per fs.realpath / fs.readdir.
FS_SEARCH_MAX_VISITED	50000	Cap on entries visited by a single search_files / directory_tree before aborting.
FS_SEARCH_EXCLUDE_PREFIXES	(empty)	Comma-separated path prefixes recursive ops refuse outright. Lexical prefilter (see #4208 / README).

Invalid numeric values warn to stderr and fall back to the default, so a typo cannot silently disable a guard.

How Has This Been Tested?

• New __tests__/4162-timeouts-and-caps.test.ts (14 cases): realpath/readdir timeouts, ENOENT preservation + ENOENT-realpath-timeout regression, per-entry FsTimeoutError propagation, max-visited abort, exclude-prefix containment (descendant + boundary-aware sibling), env-var validation. Uses vitest fake timers + mocked fs/promises, so no real CloudStorage path is needed in CI.
• Full suite: 160/160 pass, clean build, lib.ts coverage 82% stmts / 88% branch / 94% funcs.
• @alemuenchen independently pulled the branch and tested against a real macOS Google Drive tree with a live MCP client: no regression on pattern matching, cap + exclude-prefix behaviour confirmed, timeout does not misfire on warm provider paths.

Breaking Changes

None. All new behaviour is bounded by env vars that default to safe values for ordinary local trees; existing configurations are unaffected.

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 (via @alemuenchen on a live macOS CloudStorage setup; I verified locally with unit tests + build)
• My code follows the repository's style guidelines
• New and existing tests pass locally (160/160)
• I have added appropriate error handling
• I have documented all environment variables and configuration options

Additional context

Deliberately out of scope (worth their own follow-ups, flagged so as not to muddy this PR):

• libuv threadpool — timed-out fs calls still occupy the threadpool until they settle; a concurrency limiter is a separate mitigation.
• Single huge directory — the cap bounds breadth/depth but one readdir on a directory with millions of entries is still a single allocation; fs.opendir streaming would address this.
• Symlink-aware excludes — FS_SEARCH_EXCLUDE_PREFIXES is a lexical prefilter; a symlink whose target is under an excluded prefix is not blocked. Design discussion in filesystem: FS_SEARCH_EXCLUDE_PREFIXES is lexical; should excludes be canonical/symlink-aware? #4208.

Credit: the core defensive diff is @alemuenchen's, contributed on #4162 with his blessing to take it forward. Tests, directory_tree integration, the shared exclude helper, README, and this PR are mine.

[temurkhan13]

The FsTimeoutError design here is the right shape: a distinct, typed error so the caller can tell a hang apart from EACCES (per the comment on the class) instead of swallowing it as an opaque fs failure. The restructure in validatePath reinforces it well, by pulling realParentPath out of the inner try and explicitly re-throwing FsTimeoutError from parentError so a timeout cannot get masked into a misleading "Parent directory does not exist." That kind of explicit error-shape discipline at the layer where the failure is still structured is what makes the rest of the PR's defensive shape pay off.

The one place the discipline gets dropped is on the cap side of searchFilesWithValidation. When visited >= FS_SEARCH_MAX_VISITED, the inner closure sets aborted = true; return;, and the outer function returns the accumulated results array unchanged. From the caller's perspective, a search that found 30 matches by visiting 49,999 entries and a search that aborted at the 50,000-entry cap with 30 partial matches look identical. The cap-reached signal exists inside searchFilesWithValidation (the aborted flag, the visited counter) but is not visible to anything outside it. A user asking "did this miss results?" cannot answer that question from the return value.

This is the same problem the timeout side carefully avoids. FsTimeoutError is the explicit "I bailed out, not a clean empty result" signal for the timeout path. There is no parallel signal for the cap path. The behavior gap is asymmetric: a hang surfaces as a typed error and is impossible to silently lose; a 50k-entry cap collapses into a partial result that looks complete.

Two shapes that would close it without disturbing the rest of the design:

1. Throw on cap, same as timeout: introduce FsSearchTruncatedError with the visit count and the directory where the cap was hit, mirroring FsTimeoutError. Simplest for callers that already need to handle FsTimeoutError; they catch both as "search did not complete."

2. Return a structured result instead of string[]: { paths: string[]; truncated: boolean; visited: number }. Callers that want partial results get them, and callers that need completeness can branch on truncated. Wider API change, but more recoverable for the search-UI case where partial-with-warning is genuinely useful.

The throw shape is consistent with what the PR already does for timeouts and is probably the lower-friction choice if you want to keep the public signature stable. Either way, the asymmetric shape is the structural thing worth fixing here, because the typed-error discipline elsewhere in this diff is exactly what makes the silent-partial-result on the cap side stand out.

Worth confirming on the test file too. The new 4162-timeouts-and-caps.test.ts covers cap-reached behavior. If the assertion is "result has <= N entries when cap is N," the silent-truncation case is currently passing (correctly bounded, but indistinguishable from "no more matches existed"). A test that asserts the cap-reached signal is observable from outside the function would catch any future regression that silently re-introduces the asymmetry.

[alemuenchen]

@temurkhan13 — thanks for taking the time to read through this so carefully, the framing around FsTimeoutError discipline is well put.

There's one piece I'd gently flag, because I think it might be easy to miss in the diff — the cap path in searchFilesWithValidation actually does surface a cap-reached signal, just not where the inner closure sets the flag.

The flow is:

1. The inner closure sets aborted = true; return; and unwinds the recursion fast (each level re-checks aborted on re-entry, by design).
2. After await search(rootPath) returns, before return results, there's:

if (aborted) {
  throw new Error(
    `Search aborted after visiting ${visited} entries ` +
    `(cap FS_SEARCH_MAX_VISITED=${FS_SEARCH_MAX_VISITED}). ` +
    `Refine the pattern (e.g. '**/name') or narrow the search path.`
  );
}
return results;

So a search that hits the cap throws with a clear message including the visit count — the caller doesn't see a silent partial array. directory_tree in index.ts is even more direct: it throws inline the moment visited >= FS_SEARCH_MAX_VISITED, before incrementing.

On the test side, the cap-reached signal is asserted from outside the function:

it('aborts with a clear error once the visited cap is exceeded', async () => {
  ...
  await expect(searchFilesWithValidation(ROOT, '**/*', ALLOWED))
    .rejects.toThrow(/Search aborted after visiting/);
});

That said, there's a real point in your comment that I do think is worth raising as a separate question for @JonoGitty / the maintainers: the timeout path uses a typed FsTimeoutError, while the cap path uses a plain Error. For callers doing instanceof discrimination rather than string-matching on the message, a typed FsSearchTruncatedError would be more consistent. That's a fair API-shape suggestion — just a different one from "silent truncation," and probably small enough to either fold in here or carry to a follow-up.

Thanks again for the careful read — the instinct that the timeout and cap paths should give callers equivalent visibility is the right one.

[JonoGitty]

Thanks both. @alemuenchen — yes, that's the flow exactly: the cap path already throws after search() unwinds (and directory_tree throws inline), so there was never a silent partial array.

@temurkhan13 — the consistency point is fair and I've folded it in: the cap now throws a typed FsSearchTruncatedError (carrying visited + maxVisited), mirroring FsTimeoutError, from both searchFilesWithValidation and directory_tree. Callers can now instanceof-discriminate a safety-cap abort instead of string-matching the message. Cap test updated to assert the type + fields. 160/160 still green. Pushed.

[temurkhan13]

@alemuenchen — you're right, that's a genuine misread on my end. I grepped the diff for the post-recursion throw and missed it sitting right above return results; the cap was already a hard abort, not a silent partial. Appreciate the correction.

The typed-vs-untyped consistency was the substantive thread of it, and FsSearchTruncatedError carrying visited + maxVisited is exactly the right shape for instanceof discrimination. @JonoGitty — clean fold-in, thanks for picking that up in the same PR rather than punting to a follow-up.