fix(vscode): Prevent timeout in binary search for large pnpm monorepos#17580
fix(vscode): Prevent timeout in binary search for large pnpm monorepos#17580MIreland wants to merge 3 commits intooxc-project:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses timeout issues in the VS Code extension when searching for binaries in large pnpm monorepos by implementing a hybrid search strategy with caching, fast-path file system operations, depth-limited searching, and timeout protection.
Key Changes:
- Implements a three-tier progressive search strategy: direct file system checks, depth-limited recursive search, and timeout-protected
findFilesfallback - Adds binary path caching to avoid repeated expensive searches
- Introduces timeout protection (5 seconds) for the
workspace.findFilesAPI to prevent indefinite hangs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix cache check to use Map.has() instead of !== undefined - Clear cache when binary path settings change in onVscodeConfigChange - Fix comment accuracy about search direction - Fix misleading comment about deep recursion - Make Windows drive letter regex case-insensitive - Skip .pnpm directories in file system search - Fix timeout leak by clearing timeout ID in finally block - Improve error handling in catch block
|
Based on the PR description I presume this was - at least in large part - generated using AI tooling. Please update the PR description to note any and all AI usage in accordance with our policy, preferably including models/tools used and a confirmation that you have personally reviewed the code generated and ensured it works/makes sense: https://oxc.rs/docs/contribute/introduction.html#ai-usage-policy If the PR description is not updated in a timely manner, we will opt to close this PR to avoid maintainer fatigue and prevent spending too much time on contributions not thoroughly reviewed by the contributor. |
|
It may also be worth checking #17345 to see if that fixes the problem for your large monorepo as an alternative, although it will mostly depend on Sysix/camc's preference here - assuming both fix the problem adequately. |
|
@connorshea Added an AI disclaimer with context to the description. |
Thank you! |
| pattern: RelativePattern, | ||
| exclude: string | null, | ||
| maxResults: number, | ||
| timeoutMs: number = 5000, |
There was a problem hiding this comment.
I know of some cases (mainly networked drives) where searching for files could take at least a few seconds, and so we may want to consider bumping this up a bit. We could make it configurable, but it's not my preference.
|
#17345 also fixes the issue if oxfmt is installed. I don't currently plan to use oxfmt, although I'll stick it in the node_modules if I need to to make the extension work 😂 I'll call this out in that PR as well. |
lol I guess that's okay as a temporary workaround until we ship a fix, but yeah it's definitely not intentional that the extension locks up for users with only oxlint installed |
|
Oh also #17511 probably improves the handling of a missing oxfmt binary and has already been merged, will go out in the release on Monday. |
|
At first glance it doesn't look like that code would help handle the situation when |
Sysix
left a comment
There was a problem hiding this comment.
Thanks for working on this 🫶 Did you try to use tinyglobby npm package and avoid workspace.findFiles completely? I am confused why this is really slow.
|
|
||
| // Skip .pnpm directories to avoid traversing into pnpm's internal structure | ||
| // Check if we're inside a .pnpm directory path | ||
| if ( |
There was a problem hiding this comment.
I do not like it, that we have a special check for only .pnpm.
Can the search really ends up in a .pnpm directory?
| } | ||
|
|
||
| // Check with .exe extension on Windows | ||
| if (process.platform === "win32") { |
There was a problem hiding this comment.
For windows, we do not use the .exe. Depending on the package manager, there is no exe.
That's why we use shell: true for windows, to target the same binary :)
oxc/editors/vscode/client/tools/lsp_helper.ts
Lines 26 to 38 in f02c0e7
| * Searches for a binary in node_modules/.bin directories using file system operations. | ||
| * This is much faster than workspace.findFiles in large monorepos. | ||
| * Limits search depth and scope to avoid timeouts. | ||
| * Note: This method searches upward from the start path toward the filesystem root. |
There was a problem hiding this comment.
I do not like this too. We have validateSafeBinaryPath which explicit disallow parent directory access.
| maxResults: number, | ||
| timeoutMs: number = 5000, | ||
| ): Promise<Uri[]> { | ||
| const searchPromise = workspace.findFiles(pattern, exclude, maxResults); |
There was a problem hiding this comment.
workspace.findFiles has a fourth parameter to cancel the search. Use this parameter instead of Promise.race :)
| 1, | ||
| // Check cache first | ||
| const cacheKey = `auto:${defaultPattern}`; | ||
| if (this.binaryPathCache.has(cacheKey)) { |
There was a problem hiding this comment.
searchBinaryPath will only be called at the start of the extension. Can you tell me why a cache is needed?
| return result; | ||
| } catch (error) { | ||
| // Timeout or other error - log and cache undefined to avoid retrying | ||
| // Note: Error logging is intentionally minimal to avoid noise in production |
There was a problem hiding this comment.
can we add a window.showErrorMessage so the user knows the search failed?
Probably suggest setting the oxc.path.* configuration to avoid automatic search
There was a problem hiding this comment.
I'm just going to shut this PR down in favor of #17345, which works also works for my giant monorepo.
|
|
||
| // Fast path: Check common locations first using file system operations | ||
| // This is much faster than recursive findFiles in large monorepos | ||
| const workspaceFolders = Array.from(this.workspaceConfigs.keys()); |
There was a problem hiding this comment.
The old implementation used only the first workspace folder. I guess this could lead to performance problems too. Specially with setups like in this example:
#17515
|
I'm going to close this in favor of #17345 |
AI Disclaimer:
The code changes in this PR was written entirely by Cursor.
I opened this issue: #17578
Then I started debugging the extension locally, adding logs with cursor and tracking down where my plugin was failing to instantiate.
I walked through the entire process with cursor, feeding it logs and manually walking through several permutations of trial and error on my local instance of vscode. The code and traversal patterns themselves were written by AI.
I have a large local pnpm monorepo that does NOT work with the published vscode extension. It does work with this branch.
Problem
This is a fix for #17578
The VS Code extension was timing out when searching for binaries in large pnpm monorepos. The
workspace.findFilescall with a recursive pattern**/node_modules/.bin/${defaultPattern}was searching through potentially thousands of nestednode_modulesdirectories, causing indefinite hangs.Solution
Implemented a hybrid approach that prioritizes fast file system operations and only uses
findFilesas a last resort with timeout protection:node_modules/.bin/in each workspace folder firstfindFileswith a 5-second timeout usingPromise.raceto prevent indefinite hangingChanges
searchBinaryInNodeModules()method for depth-limited file system searchfindFilesWithTimeout()helper method with timeout protectionsearchBinaryPath()to use progressive search strategyPerformance Benefits
findFilesin large monoreposfindFilesonly if needed, with timeout protectionTesting