Repository navigation
filesystem: recursive search can hang on macOS CloudStorage / lazy provider paths #4162
Description
Activity
Thanks @Sean-Kenneth-Doherty for picking this up.
Being able to exclude a known-slow folder before the server tries to touch it is genuinely useful — on my setup the slow paths are always the same Google Drive and iCloud Drive folders, so even this first step would help in practice.
And thanks for being upfront that it doesn't cover everything: the timeout, the cap on how many entries get visited, the abort path, and
directory_treeare still where the real hangs come from for me. For several weeks I've been running a patched build with timeout + max-entry mitigations and Google Drive / iCloud paths inallowedDirectories, and it's been stable. Happy to share what I did if it helps a follow-up, and happy to test #4181 on my CloudStorage setup.Hi @alemuenchen, @Sean-Kenneth-Doherty — I can reproduce this reliably on macOS with both
~/Library/CloudStorage/OneDrive-...and~/Library/Mobile Documents/com~apple~CloudDocs/...inallowedDirectories.search_fileshangs for minutes on lazy-materialized subtrees;directory_treeis worse.#4181 (excludePatterns before
validatePath) is a useful first cut but, as @alemuenchen noted, doesn't address the underlying timeout / abort / max-entry gaps. To avoid stepping on that PR I'd like to scope a follow-up to the parts not covered:- Timeout wrapper around
fs.realpathinvalidatePath()(configurable via env, sane default e.g. 5s) returning a structured error rather than hanging. - Timeout wrapper around
fs.readdirinsearchFilesWithValidation()anddirectory_tree. - Configurable max-visited-entries cap for
search_filesanddirectory_tree, returning a clear "search aborted after N entries — narrow the path or pattern" error. - Tests using a mocked slow
realpath/readdirso CI doesn't depend on a real CloudStorage path.
Happy to drop any of these from scope if maintainers prefer them split into separate PRs, or if there's existing design preference on config surface (env vars vs constructor options). Will hold off coding until either a maintainer or @Sean-Kenneth-Doherty signals this scope is fine.
- Timeout wrapper around
Thanks @JonoGitty — your four-point scope matches exactly where the real hangs come from on my setup, so from the reporter's side this is the right follow-up to #4181.
Two things that might save you time:
- I've been running a patched build with mitigations 1–3 (realpath/readdir timeouts + a max-visited-entries cap) for several weeks, with Google Drive and iCloud Drive paths in
allowedDirectories, and it's been stable. Happy to share the diff so you can compare design choices — I went with env-var config and conservative defaults, which matches your instinct. - On point 4: a mocked slow
realpath/readdiris the way to go. The hang is 100% reproducible with a real CloudStorage path, but that's obviously unusable in CI.
I'm just the reporter, not a maintainer, so I can't green-light the scope — but for what it's worth, consider this a +1 that splitting this out from #4181 is the right call. Happy to test your branch on my CloudStorage setup once it's up.
- I've been running a patched build with mitigations 1–3 (realpath/readdir timeouts + a max-visited-entries cap) for several weeks, with Google Drive and iCloud Drive paths in
Thanks @alemuenchen — yes please, I'd love to see the diff. Saves me re-deriving design decisions you've already validated in real use. Happy to credit your work in the PR description if it ends up materially shaping the patch.
Agreed the scope split makes sense regardless of which way maintainers go on config surface. I'll hold off coding until a maintainer chimes in, but the diff will let me have a draft ready to put up the moment they do — and your offer to test on a live CloudStorage setup is exactly the validation this fix needs.
@JonoGitty — here's the diff, against current
main(src/filesystem/lib.ts). Treat it as a reference, not a PR: take whatever design decisions are useful and drop the rest.What it does (your scope points 1–3, search side):
withFsTimeout()wraps everyfs.realpathinvalidatePath()and everyfs.readdirinsearchFilesWithValidation(). On timeout it rejects with a typedFsTimeoutError.FS_SEARCH_MAX_VISITEDcaps recursive search; hitting the cap aborts with a clear "narrow the path / refine the pattern" error.FS_SEARCH_EXCLUDE_PREFIXES— opt-in, empty by default — lets a user refuse recursive search on known-slow roots outright. Containment reusesisPathWithinAllowedDirectories, so it's boundary-aware (a prefix can't match an unrelated sibling that just shares a string head).- Config is env-var driven, defaults conservative (15s timeout, 50k entries). Invalid numeric values are ignored with a stderr warning rather than silently disabling a guard.
Design notes / things I deliberately left for your PR:
FsTimeoutErrorpropagates from any depth — the per-entrycatchin search re-throws timeouts instead of swallowing them, so a search never returns silently-partial results. Tradeoff: one genuinely-hung subtree aborts the whole search. If you'd rather collect skipped paths and return partial results + a warning, that needs an API change (search_filescurrently returnsstring[]) — your call.directory_treeis not touched here (it lives inindex.ts, notlib.ts). Your point 2 includes it; the samewithFsTimeout+ cap pattern applies.- The cap bounds recursion breadth/depth, not a single
fs.readdir: one directory with millions of entries is still read in a single call (the timeout bounds the wait, not the allocation). Switching tofs.opendirstreaming would fix that but is a larger structural change — flagging it as a known limitation. - The libuv threadpool angle from the issue (slow calls still stuck in the pool after a timeout) isn't addressed — a concurrency limiter is a separate mitigation.
- No tests — your point 4 (mocked slow
realpath/readdir) is the right approach; I left it to the PR.
Env-var names and defaults (
FS_OP_TIMEOUT_MS,FS_SEARCH_MAX_VISITED,FS_SEARCH_EXCLUDE_PREFIXES) are just my picks — rename freely to whatever config surface the maintainers settle on.This diff was independently code-reviewed before posting. Happy to test your branch on a live Google Drive / iCloud setup once it's up.
--- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -38,6 +38,86 @@ export interface SearchResult { path: string; isDirectory: boolean; +} + +// --------------------------------------------------------------------------- +// Mitigations for issue #4162: recursive search / validatePath can hang for +// minutes on macOS CloudStorage / lazy provider-backed paths, because +// fs.realpath and fs.readdir have no timeout and recursive search has no +// upper bound. All knobs below are env-var configurable and default to +// behavior that is safe for ordinary local trees. +// +// FS_OP_TIMEOUT_MS timeout (ms) per fs.realpath / fs.readdir +// call. Default 15000. Positive integer. +// FS_SEARCH_MAX_VISITED max entries a single recursive search may +// visit before aborting. Default 50000. +// Positive integer. +// FS_SEARCH_EXCLUDE_PREFIXES comma-separated path prefixes that recursive +// search refuses outright. Empty by default +// (no behavior change). '~' is expanded. +// +// Invalid numeric values are ignored (a warning is logged) and the default is +// used, so a typo cannot silently disable a guard. +// --------------------------------------------------------------------------- + +// Raised when a wrapped filesystem call exceeds FS_OP_TIMEOUT_MS. Distinct +// from ordinary fs errors so callers can tell a hang apart from e.g. EACCES +// and avoid silently swallowing it. +class FsTimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = 'FsTimeoutError'; + } +} + +// Reads a positive-integer env var, falling back to `defaultValue` (with a +// warning) when unset or invalid. Uses Number() rather than parseInt() so that +// partially-numeric garbage such as "15s" is rejected instead of truncated. +function readPositiveIntEnv(name: string, defaultValue: number): number { + const raw = process.env[name]; + if (raw === undefined || raw.trim() === '') return defaultValue; + const parsed = Number(raw); + if (!Number.isInteger(parsed) || parsed < 1) { + console.error(`[filesystem] Ignoring invalid ${name}="${raw}"; using default ${defaultValue}.`); + return defaultValue; + } + return parsed; +} + +// Timeout (ms) applied to individual fs.realpath / fs.readdir calls. +const FS_OP_TIMEOUT_MS = readPositiveIntEnv('FS_OP_TIMEOUT_MS', 15000); + +// Hard cap on entries visited by a single recursive search before it aborts. +const FS_SEARCH_MAX_VISITED = readPositiveIntEnv('FS_SEARCH_MAX_VISITED', 50000); + +// Opt-in: comma-separated path prefixes that recursive search refuses outright. +// Empty by default -- no behavior change unless the user sets it. Intended for +// known-slow provider-backed roots (e.g. a CloudStorage folder). Each entry is +// expanded ('~') and normalized so the containment check below is boundary- +// aware: "/data/foo" must not match an unrelated "/data/foobar" root. +const FS_SEARCH_EXCLUDE_PREFIXES = (process.env.FS_SEARCH_EXCLUDE_PREFIXES ?? '') + .split(',') + .map(prefix => prefix.trim()) + .filter(prefix => prefix.length > 0) + .map(prefix => normalizePath(path.resolve(expandHome(prefix)))); + +// Races a filesystem promise against a timeout. On timeout it rejects with an +// FsTimeoutError instead of letting the caller hang. Note: the underlying fs +// operation cannot be cancelled and may still settle in the background; the +// timeout only unblocks the caller. +async function withFsTimeout<T>(operation: Promise<T>, label: string): Promise<T> { + let timer: ReturnType<typeof setTimeout>; + const timeout = new Promise<never>((_, reject) => { + timer = setTimeout( + () => reject(new FsTimeoutError(`Filesystem operation timed out after ${FS_OP_TIMEOUT_MS}ms: ${label}`)), + FS_OP_TIMEOUT_MS, + ); + }); + try { + return await Promise.race([operation, timeout]); + } finally { + clearTimeout(timer!); + } } // Pure Utility Functions @@ -112,8 +192,10 @@ // Security: Handle symlinks by checking their real path to prevent symlink attacks // This prevents attackers from creating symlinks that point outside allowed directories + // #4162: fs.realpath is wrapped with a timeout so a lazy provider-backed path + // cannot block validation indefinitely. try { - const realPath = await fs.realpath(absolute); + const realPath = await withFsTimeout(fs.realpath(absolute), `realpath ${absolute}`); const normalizedReal = normalizePath(realPath); if (!isPathWithinAllowedDirectories(normalizedReal, allowedDirectories)) { throw new Error(`Access denied - symlink target outside allowed directories: ${realPath} not in ${allowedDirectories.join(', ')}`); @@ -124,16 +206,21 @@ // This ensures we can't create files in unauthorized locations if ((error as NodeJS.ErrnoException).code === 'ENOENT') { const parentDir = path.dirname(absolute); + // #4162: only the realpath call is inside this try, so a timeout + // surfaces as FsTimeoutError and the access-denied check below is not + // masked into a misleading "Parent directory does not exist". + let realParentPath: string; try { - const realParentPath = await fs.realpath(parentDir); - const normalizedParent = normalizePath(realParentPath); - if (!isPathWithinAllowedDirectories(normalizedParent, allowedDirectories)) { - throw new Error(`Access denied - parent directory outside allowed directories: ${realParentPath} not in ${allowedDirectories.join(', ')}`); - } - return absolute; - } catch { + realParentPath = await withFsTimeout(fs.realpath(parentDir), `realpath ${parentDir}`); + } catch (parentError) { + if (parentError instanceof FsTimeoutError) throw parentError; throw new Error(`Parent directory does not exist: ${parentDir}`); } + const normalizedParent = normalizePath(realParentPath); + if (!isPathWithinAllowedDirectories(normalizedParent, allowedDirectories)) { + throw new Error(`Access denied - parent directory outside allowed directories: ${realParentPath} not in ${allowedDirectories.join(', ')}`); + } + return absolute; } throw error; } @@ -377,13 +464,47 @@ allowedDirectories: string[], options: SearchOptions = {} ): Promise<string[]> { + // #4162: refuse recursive search outright on user-configured slow roots. + // Containment reuses isPathWithinAllowedDirectories so it is boundary-aware: + // a prefix cannot match an unrelated sibling that merely shares a string head. + if (FS_SEARCH_EXCLUDE_PREFIXES.length > 0) { + const normalizedRoot = normalizePath(path.resolve(rootPath)); + if (isPathWithinAllowedDirectories(normalizedRoot, FS_SEARCH_EXCLUDE_PREFIXES)) { + throw new Error( + `Recursive search is disabled for this path by FS_SEARCH_EXCLUDE_PREFIXES: ${rootPath}. ` + + `Recursive search over lazy provider-backed trees (macOS CloudStorage / FileProvider) ` + + `is unreliable; use list_directory on specific subfolders, or narrow the search root.` + ); + } + } + const { excludePatterns = [] } = options; const results: string[] = []; + // #4162: bound the recursion so a large / lazy-materialized tree cannot keep + // the server busy indefinitely. The search aborts once FS_SEARCH_MAX_VISITED + // entries have been visited; `aborted` is checked at every re-entry. + let visited = 0; + let aborted = false; async function search(currentPath: string) { - const entries = await fs.readdir(currentPath, { withFileTypes: true }); + if (aborted) return; + // #4162: fs.readdir is wrapped with a timeout. A timeout (FsTimeoutError) + // propagates to the caller from any depth: the per-entry catch below + // deliberately re-throws timeouts instead of swallowing them, so a search + // never returns silently-partial results. + const entries = await withFsTimeout( + fs.readdir(currentPath, { withFileTypes: true }), + `readdir ${currentPath}`, + ); for (const entry of entries) { + if (aborted) return; + if (visited >= FS_SEARCH_MAX_VISITED) { + aborted = true; + return; + } + visited++; + const fullPath = path.join(currentPath, entry.name); try { @@ -403,13 +524,24 @@ if (entry.isDirectory()) { await search(fullPath); + if (aborted) return; } - } catch { + } catch (error) { + // #4162: a timeout is a real hang, not an inaccessible entry -- surface + // it instead of continuing with partial results. + if (error instanceof FsTimeoutError) throw error; continue; } } } await search(rootPath); + 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; }
Thank you @alemuenchen — this is a serious gift, well beyond what I expected. The design choices are all reasonable to me and several are sharper than what I'd have arrived at independently: re-throwing
FsTimeoutErrorfrom the per-entry catch (so a search never silently returns partial results), the boundary-aware containment forFS_SEARCH_EXCLUDE_PREFIXESviaisPathWithinAllowedDirectories, and warn-and-default on bad env values are all the right calls.I also ran the diff through a cross-vendor code-review pass (GPT-5-mini) as a sanity check; it concurred on the design and didn't surface anything beyond what's already mitigated, with one note worth carrying into the test plan: defaults that work well for local trees and CloudStorage may be tight on NFS / SMB / other remote mounts where individual
readdircalls legitimately take seconds. Worth documenting in the README alongside the env-var knobs.What I'll be picking up from here, assuming maintainers green-light the direction:
- Vitest tests with mocked slow
fs.realpath/fs.readdir— cap fires, timeout fires, ENOENT path preserved, env-var validation behaviour, exclude-prefix boundary cases. - The same
withFsTimeout+ cap pattern applied todirectory_tree(inindex.ts, orlib.tsif refactor(filesystem): extract buildDirectoryTree to lib.ts #4055 lands first). - README updates documenting the three env vars + the remote-mount note.
- PR description crediting the diff + your weeks of real-world testing.
I'll flag the limitations you noted (libuv threadpool, single-directory allocation under
fs.readdir, search aborting on first hang vs collecting partial results) in the PR description as deliberately out-of-scope — worth their own follow-ups but conflating them muddies this one.Still holding off on opening the PR until a maintainer signals on scope and config-surface preferences. Will ping you to test on a live CloudStorage branch the moment one's up.
- Vitest tests with mocked slow
Thanks @JonoGitty — glad the diff lands well and that the design holds up under the second-opinion review. The NFS/SMB caveat is a fair catch; agreed it belongs in the README next to the env-var knobs, and the out-of-scope split for the libuv / partial-results items sounds right to me.
No rush on the PR from my side — happy to wait for a maintainer to weigh in on scope and config surface. Whenever there's a live branch, I'm set up to test it against real macOS CloudStorage paths, so just ping me.
Thanks again for picking this up.
Reacted by JonoGittyQuick note for @alemuenchen — the branch is up at https://github.com/JonoGitty/servers/tree/fix/4162-filesystem-cloudstorage-hangs if you want to pull it for that CloudStorage test you offered. Test suite is green (157/157, incl. 11 new in
__tests__/4162-timeouts-and-caps.test.ts),lib.tscoverage 82%. Still holding off opening the PR until a maintainer weighs in on scope.@JonoGitty — pulled
fix/4162-filesystem-cloudstorage-hangsand tested it on my macOS setup.Build & tests: clean build, your
4162-timeouts-and-caps.test.tsis 11/11 green, full suite 157/157.Real Google Drive path (~900 dehydrated entries):
search_filesreturns correct results, no regression on pattern matching, and — worth noting — the 15s default timeout does not misfire on a warm provider path.FS_SEARCH_MAX_VISITED: aborts deterministically at the cap with a clear error.FS_SEARCH_EXCLUDE_PREFIXES: refuses recursive search as documented; boundary-aware containment confirmed (a/x/fooprefix does not match/x/foobar).What I could not reproduce: the multi-minute hang itself. My provider metadata cache is currently warm across the whole tree — I probed ~6000 directories under Google Drive + iCloud Drive and every
readdirwas <500ms. The hang needs cold provider metadata; "free up space" evicts file content but not directory listings, so it doesn't recreate the cold state on demand. So the timeout's real-world firing on a slowfscall is still only exercised by your mocked unit test — which is the right approach for CI anyway; just being transparent that my live test couldn't add empirical coverage there.One real issue (low severity): the README states
FS_SEARCH_EXCLUDE_PREFIXESapplies to bothsearch_filesanddirectory_tree, but the exclude-prefix check is only implemented insearchFilesWithValidation.directory_treegets the timeout + visited-cap but not the exclude-prefix — docs and code disagree. Either wire the check intobuildTreetoo, or narrow the README.Net: from the reporter's side this looks solid. Thanks for the careful scoping.
Thanks @alemuenchen — sharp catch. Fixed across three commits on the same branch:
- 16c959e — extracted containment into a shared
assertSearchPathNotExcluded(rootPath, opName)helper used by bothsearchFilesWithValidationanddirectory_tree.directory_treenow produces adirectory_tree is disabled...error from the same template. - 7409bfd —
expandHomeapplied torootPath(prior asymmetry meant a~/fooroot wouldn't match a~/fooprefix). Two extra helper tests: descendant-of-prefix, sibling-with-shared-string-head. 160/160 locally. - b871b4f — README now documents the exclude as a fast lexical prefilter, not a canonical-path exclusion: a symlink whose target lives under an excluded prefix is not blocked and the request would still hit the slow path via
validatePath's realpath. Inherited from the existing search check, not regressed by this PR.
Open design question for maintainers: the symlink bypass is a real performance hole, not just a security one. Hardening options I considered:
realpath-before-compare — risky, that's the very call this PR is bounding with timeouts because it can hang- Two-stage (lexical first → bounded canonical second) — viable but a real design decision with implications for
search_filestoo
I'd rather raise this than silently pick a direction. Happy to do the hardening as a follow-up if you'd like a particular shape — or leave it as documented prefilter behaviour, your call.
- 16c959e — extracted containment into a shared
@JonoGitty thanks — re-verified the three follow-up commits (
16c959e,7409bfd,b871b4f) onfix/4162-filesystem-cloudstorage-hangs:- Build clean, 160/160 tests across 8 files locally.
- No dependency changes —
package.jsonuntouched, so no supply-chain surface to review. - The shared
assertSearchPathNotExcludedhelper reusesisPathWithinAllowedDirectories, so containment stays boundary-aware, and theexpandHome-on-rootPathsymmetry fix only ever tightens the guard.validatePathand its realpath-based symlink check are untouched.
On the open design question — my vote is to leave it as the documented lexical prefilter for this PR. The symlink-into-an-excluded-prefix bypass is pre-existing behaviour, not a regression introduced here, and
validatePath's realpath still enforces the actualallowed_directoriesboundary. The only cost of such a symlink is hitting the slow provider path — which is exactly what this PR's timeout + visited-cap now bound.realpath-before-compare would reintroduce the very hang this PR exists to fix; the two-stage (lexical → bounded canonical) approach is the right long-term shape but it's a genuine design decision that also touchessearch_files, so it belongs in a separate follow-up rather than as scope creep here. I'd suggest opening that follow-up issue so it isn't lost, and otherwise this looks good to me from the reporter side.Thanks @alemuenchen — appreciate the re-verification and the substantive read on the symlink question. That matches my understanding too: this branch stays focused on the timeout/cap/prefilter fix, and the canonical-exclude question is its own design conversation. Opened the follow-up: #4208.
I can investigate a narrow fix for this if it is not already covered by the fork branch mentioned in #4208.
To keep the first PR reviewable, I’d avoid adding a broad configuration system. I’m thinking about:
- adding bounded traversal behavior for
search_files; - preventing one slow subtree from making the entire server unresponsive;
- preserving current behavior for normal local paths;
- adding tests with mocked slow filesystem operations or a bounded traversal fixture.
Before I start, would maintainers prefer:
- returning partial results with warnings, or
- returning a bounded error when traversal exceeds the limit/deadline?
I can start with the least invasive option.
- adding bounded traversal behavior for
@Kakarottoooo thanks for looking into this. The fork branch mentioned in #4208 is now PR #4212, opened 2026-05-20.
It may cover the same ground: bounded traversal via
FS_SEARCH_MAX_VISITED, per-call timeouts viaFS_OP_TIMEOUT_MS, defaults intended to preserve current behavior for normal local trees, and tests using mocked slow fs operations.On the partial-results-vs-bounded-error question, #4212 currently uses option 2: a bounded error raised as
FsSearchTruncatedError, carryingvisitedandmaxVisited. That came out of discussion with @temurkhan13 on the PR. #4208 is tracking the separate symlink-aware-exclude design question.If you're interested, review or feedback on #4212 would be very welcome.
Reacted by ZIWEI GUOThanks for the pointer — that makes sense. I’ll avoid duplicating #4212.
I’ll review #4212 locally instead and focus on whether the timeout / max-visited behavior is observable, deterministic in tests, and clear for MCP clients. If I find anything concrete, I’ll leave focused feedback on the PR rather than opening overlapping work.
If #4212 lands cleanly, I may look at one of the explicitly out-of-scope follow-ups, such as symlink-aware excludes,
fs.opendir-based streaming for huge single directories, or a small docs/test improvement.
Hi — thanks for maintaining the filesystem server.
I'm filing this against
@modelcontextprotocol/server-filesystemafter several weeks of production use on macOS with provider-backed directories inallowedDirectories,especially:
~/Library/CloudStorage/GoogleDrive-.../...~/Library/Mobile Documents/com~apple~CloudDocs/...Symptom
When
allowedDirectoriesincludes macOS CloudStorage / lazy provider paths,search_filesand other operations that callvalidatePath()can intermittently hang for minutes.Claude Desktop eventually reports the MCP server as unresponsive after roughly 4 minutes. The subprocess remains alive, but subsequent filesystem calls may continue to hang until
the server is restarted.
This may overlap with reports like #3281, but I'm filing separately because this issue points to specific filesystem code paths.
Code paths
In
src/filesystem/lib.ts,validatePath()calls:and, in the ENOENT branch:
These calls have no timeout. On macOS provider-backed paths that are not locally materialized,
realpathcan block while the provider fetches metadata.searchFilesWithValidation()then recursively does:As far as I can tell, recursive search currently has no upper bound on visited entries, no timeout around
readdir, and no abort path. A large or lazy-materialized tree cantherefore keep the server busy for a long time.
Why this matters
This is especially visible with CloudStorage / FileProvider-backed folders such as Google Drive and iCloud Drive, where metadata for subdirectories may be fetched lazily. It can
also affect very large local trees.
The libuv threadpool angle may make the symptom worse: filesystem operations use libuv's threadpool, whose default size is 4. A small number of slow filesystem calls can
therefore degrade unrelated filesystem operations in the same process. I mention this as a contributing factor, not necessarily as the main fix.
Suggested mitigations
The two minimal mitigations I'd suggest are:
Add a timeout wrapper around
fs.realpathinvalidatePath()and aroundfs.readdirin recursive search.Add a configurable maximum visited-entry count for
search_files, returning a clear error such as:Optional follow-ups:
UV_THREADPOOL_SIZEtuning for users who intentionally expose large / slow provider-backed trees.I have tested equivalent mitigations locally in a patched installed build for several weeks with Google Drive and iCloud Drive paths in
allowedDirectories. I'm not opening a PRbecause the exact configuration surface — env vars vs config object, defaults, and docs placement — seems like a maintainer policy decision, but I'd be happy to test any
proposed fix.