fix: prevent out of memory crash on cyclic symbolic links - #1396
fix: prevent out of memory crash on cyclic symbolic links#1396superphosphate wants to merge 3 commits into
Conversation
fast-glob follows symbolic links while walking the workspace, so a cyclic symbolic link made the language server read the same directory over and over again until the process ran out of memory. Read every directory by its real path only once, which breaks cycles while keeping symbolic links working.
There was a problem hiding this comment.
🟡 Changes recommended
The cycle-breaking strategy can silently omit matches for certain glob patterns when the same real directory is reachable via multiple non-cyclic symlink paths, and the intended semantics should be clarified/adjusted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents an out-of-memory crash during workspace globbing when the repository contains cyclic symbolic links by adding cycle detection to the fast-glob filesystem adapter used by getFilePaths.
Changes:
- Added a cycle-safe
fast-globfilesystem adapter that tracks visited directories by realpath and stops traversing repeated realpaths. - Wired the adapter into
getFilePathswhile keepingfollowSymbolicLinks: true. - Added Jest tests covering basic glob matching, following symlinks, cyclic symlinks, and the
maxItemslimit.
File summaries
| File | Description |
|---|---|
| server/src/util/fs.ts | Adds a cycle-safe fast-glob FS adapter and uses it in getFilePaths to prevent infinite traversal via symlink cycles. |
| server/src/util/tests/fs.test.ts | Adds regression tests for symlink traversal behavior (including cycles) and maxItems limiting. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fs.realpath(directoryPath, (realPathError, realPath) => { | ||
| if (realPathError == null && !isFirstReadOf(realPath)) { | ||
| done(null, []) | ||
| return | ||
| } |
Skipping every already-read real path also dropped matches for the same
directory reachable through several distinct symbolic links. Compare the
real path with the real paths of the ancestor directories instead, so cycles
are still broken while `{a,b}/**` matches both prefixes.
|
Addressed the review comment in e6e9f64. The adapter now skips a directory only when its real path equals the real path of one of its ancestors, instead of skipping every already-read real path. That still breaks cycles (the walk can only descend, so an infinite walk has to revisit an ancestor) but no longer drops matches for the same directory reachable through several distinct symbolic links, for example Added two regression tests:
Verification: |
|
Feels odd that we need to do a fix on our side, did you investigate if there are any fast-glob issues related to this or workarounds? Also fine to switch library if there are better options now. |
Point at mrmlnc/fast-glob#74 in the adapter doc so the reason for the cycle guard (and why the upstream workarounds were not used) is recorded in the code.
|
@skovhus good question — I checked before hand-rolling the adapter.
So the adapter keeps If you'd prefer migrating to |
Summary
fast-globfollows symbolic links while walking the workspace (followSymbolicLinks: true). A repository that contains a cyclic symbolic link (for examplefwupd'ssrc/tests/sys) therefore makes the server walk the same directory over and over again until Node runs out of memory:This adds a
fast-globfile system adapter that records the real path of every directory that is read and returns an empty directory when the same real path is encountered again. Symbolic links keep working, cycles are broken.Related issues
fixes #1235
What changed
server/src/util/fs.ts:getFilePathsnow passes a cycle-safefsadapter tofast-glob.server/src/util/__tests__/fs.test.ts: new tests for glob matching, following symbolic links, cyclic symbolic links and themaxItemslimit.Verification
pnpm exec tsc -bpasses.pnpm exec jest --runInBand server/src/util/__tests__/fs.test.ts: 4/4 pass.does not follow cyclic symbolic linkstest fails with paths such asloop/loop/loop/.../script.sh, which is exactly the runaway behaviour from the issue.pnpm exec eslint server/src/util/fs.ts server/src/util/__tests__/fs.test.tspasses.server.test.ts,analyzer.test.ts,sourcing,sh,shfmt,executables,shellcheck) fail identically on a clean checkout in this environment, so they are unrelated to this change.Notes