perf: stop tracing every command-path probe and bound git spawns - #4778
perf: stop tracing every command-path probe and bound git spawns#4778arubi9 wants to merge 2 commits into
Conversation
Resolving a command walks every PATH entry against every PATHEXT variant. On Windows that is hundreds of candidates per lookup, and isExecutableFile emitted a span for each one, so a single editor/provider refresh produced tens of thousands of spans -- one 13.7s trace window held 28,698 spans, of which 28,577 were shell.isExecutableFile. Serializing that volume starved the event loop badly enough that unrelated work blew past its own timeouts. Make the per-candidate probe untraced, and cache resolution outcomes (hits and misses alike) for 30s keyed on platform, command, PATH and PATHEXT. Editor detection and provider health checks re-resolve the same commands every refresh; the TTL keeps a newly installed CLI discoverable. Measured on a 57-entry PATH with 14 PATHEXT entries, resolving 21 commands five times: 3809/3566/3700/3369/3401ms before, 3264/1/1/4/1ms after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A workspace refresh asks for refs and status across every project and worktree at once, and each request spawns several git processes. With no bound, one observed refresh issued 1055 git spawns; individual commands that take about a second in isolation took 6.5s under that load, listRefs calls ran 51s, and the local environment endpoint took 14s -- past the client's 10s connection-setup timeout, so the client reconnected and triggered the storm again. Gate the spawn behind a semaphore sized from availableParallelism (4 to 16 permits). The permit is taken around the spawn only, so time spent queued never counts against the command's own timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| options: CommandAvailabilityOptions & { readonly platform: NodeJS.Platform }, | ||
| ): Effect.fn.Return<string, CommandResolutionError, FileSystem.FileSystem | Path.Path> { | ||
| const env = options.env ?? process.env; | ||
| const cacheKey = [ |
There was a problem hiding this comment.
🟡 Medium src/shell.ts:582
resolveCommandPathForPlatform caches results for commands containing / or \ without including the current working directory in the cache key. Those commands are resolved relative to process.cwd(), so if the process changes directories within the 30-second TTL, the cache returns a path from the previous directory (or reuses a cached miss), yielding an incorrect or unusable resolution. Include process.cwd() in the cache key so entries don't leak across directory changes.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/shell.ts around line 582:
`resolveCommandPathForPlatform` caches results for commands containing `/` or `\` without including the current working directory in the cache key. Those commands are resolved relative to `process.cwd()`, so if the process changes directories within the 30-second TTL, the cache returns a path from the previous directory (or reuses a cached miss), yielding an incorrect or unusable resolution. Include `process.cwd()` in the cache key so entries don't leak across directory changes.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This performance optimization adds process concurrency limiting and command path caching. An unresolved review comment identifies a potential caching bug where relative command paths may resolve incorrectly if the working directory changes within the 30-second cache TTL. You can customize Macroscope's approvability policy. Learn more. |
|
I can confirm, 0.0.29 is currently completely unusable on Windows. Seemingly random disconnects from the local server, timeouts stemming from the t3 environment file. |
|
very severe, t3code on windows is virtually almost unusable due to this git call storm. these stats are for just working on 1 thread on 1 project.
-- Each vcs.listRefs call expands into approximately six concurrent Git commands: 21 polls / 5 seconds ≈ 4.2 RPCs/second update: #4727 successfully eliminated the runaway ref-polling/OOM mechanism. However, the remaining run shows:
That maps directly to [PR #4778](#4778), which:
Therefore:
|
Problem
On a Windows host with several projects open, the desktop app became unusable for tens of seconds at a time:
Timed out while checking Codex app-server provider status,Git command timed outwarnings, and a client stuck inFailed to connect. Reconnecting…because/.well-known/t3/environmentexceeded its 10s connection-setup budget.Tracing pointed at two independent causes.
1. A span per command-path probe
resolveCommandPathForPlatformwalks every PATH entry against every PATHEXT candidate, andisExecutableFilewas anEffect.fn— one span per probe. With a 57-entry PATH and 14 PATHEXT entries that is ~800 probes (~1600 with the lower-case variants) per lookup.One 13.7s trace window contained 28,698 spans, of which 28,577 were
shell.isExecutableFile— produced by just 22resolveCommandPathcalls. The trace file rotated at 10 MB every ~40s. Serializing that volume starved the event loop; unrelated fibers then blew past their own timeouts (adiscoverClaudeSkillsspan read 79s while the work it does measures ~93ms).Editor detection and provider health checks re-resolve the same commands on every refresh, and uninstalled CLIs (
cursor-agent,grokhere) pay the full miss every time.Fix: make the per-candidate probe untraced, and cache resolution outcomes — hits and misses — for 30s, keyed on platform, command, PATH and PATHEXT. The TTL rather than a permanent cache so a newly installed CLI is still discovered.
Resolving 21 commands, five times, on the real PATH:
2. Unbounded git spawns
A workspace refresh requests refs and status for every project and worktree at once, and each
listRefsspawns six git processes (branchNoColor,defaultRef,remoteBranches,remoteNames,worktreeList,readBranchRecency).VcsProcesshas timeouts but no concurrency bound.One observed refresh issued 1,055 git spawns. Commands that take ~0.9s in isolation (and stay under 500ms at 12-way concurrency) took 6.5s under that load;
ws.rpc.vcs.listRefsspans ran 51s, and the environment endpoint took 14.2s — past the client's 10s budget, so it reconnected and re-triggered the storm.Fix: gate the spawn behind a semaphore sized from
availableParallelism()(4–16 permits). The permit is taken around the spawn only, so queue time never counts against the command's own timeout.Verification
Both fixes were built into a desktop artifact and run against the same workload that produced the traces above:
shell.isExecutableFilespans/.well-known/t3/environmentTests:
packages/shared/src/shell.test.ts— resolve, delete the file, resolve again (still cached),clearCommandPathCache(), resolve fails.apps/server/src/vcs/VcsProcess.test.ts— 40 competing fibers; peak in-flight stays within the bound. Verified to fail without the semaphore (expected 40 to be 8).vp test apps/server/src/vcs— 71 passed.vp run typecheckandvp checkclean.Six pre-existing Windows test failures (
relayClient×4,logging,t3-sqlite-state) fail identically on an unmodified checkout and are unrelated.Not addressed
The refresh still issues a lot of git work — 384 commands in one window, ~145 of them repeated metadata lookups (
readConfigValue,resolveCurrentUpstream,listRemoteNames,resolveDefaultBranchName). Caching those, or fetching refs only for visible projects, would reduce the work rather than just bounding it. Happy to follow up if that direction seems right.🤖 Generated with Claude Code
Note
Medium Risk
Touches shared spawn/path resolution and global VCS concurrency; mis-tuning could delay refreshes or briefly serve stale “command not found” until cache TTL expires.
Overview
Addresses event-loop starvation that caused provider health checks, HTTP endpoints, and git commands to time out under heavy refresh load.
Command path resolution (
shell.ts) stops emitting a trace span on every PATH/PATHEXT file probe by makingisExecutableFileand the caching wrapper untraced. Adds a 30s TTL cache (platform, command, PATH, PATHEXT) for both hits and misses, plusclearCommandPathCache()for tests.VCS process spawning (
VcsProcess.ts) wraps eachprocessRunner.runin a global semaphore (4–16 permits fromavailableParallelism() / 4) so workspace refreshes cannot launch unbounded git processes; queue time does not count toward per-command timeouts.Tests cover cache reuse after the binary is removed and that 40 concurrent runs stay at or below the spawn cap.
Reviewed by Cursor Bugbot for commit dc7f172. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Cache command path resolutions and bound concurrent git process spawns
resolveCommandPathForPlatformin shell.ts now caches results (hit or not-found) for 30s per(platform, command, PATH, PATHEXT)tuple, avoiding repeated filesystem probes. A newclearCommandPathCache()export forces re-resolution.isExecutableFileis changed to useEffect.fnUntracedto avoid generating a trace span on every probe call.VcsProcess.runin VcsProcess.ts now acquires a semaphore permit before spawning a child process, capping concurrency atmax(4, min(16, floor(parallelism/4)))based onNodeOS.availableParallelism().📊 Macroscope summarized dc7f172. 2 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.