fix(server): speed up Windows diagnostics and editor discovery#3911
fix(server): speed up Windows diagnostics and editor discovery#3911SivanTechDev wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughProcess diagnostics now use platform-specific timeouts and batched Windows performance queries. Editor discovery uses concurrent command resolution, while server configuration applies a timeout and empty fallback for stalled discovery. Tests cover Windows querying, resolver behavior, and non-blocking configuration. ChangesDiagnostics and editor discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerGetConfig
participant EditorDiscovery
participant ExecutableResolver
Client->>ServerGetConfig: request configuration
ServerGetConfig->>EditorDiscovery: resolve available editors
EditorDiscovery->>ExecutableResolver: probe editor commands
ExecutableResolver-->>EditorDiscovery: resolved commands
EditorDiscovery-->>ServerGetConfig: editor IDs or timeout
ServerGetConfig-->>Client: configuration with availableEditors
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/server/src/diagnostics/ProcessDiagnostics.test.ts (1)
223-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest coverage is solid for the bulk query optimization.
The assertions verify each CIM class is queried exactly once, the
$perfByIdjoin expression is present, and-Filter(which would indicate per-process queries) is absent. Consider also asserting the-ErrorAction SilentlyContinueflag is present on the perf query, since silent error handling is the degradation mechanism for environments whereWin32_PerfFormattedData_PerfProc_Processis unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/diagnostics/ProcessDiagnostics.test.ts` around lines 223 - 257, Extend the “queries each Windows CIM class once” test around ProcessDiagnostics.readProcessRows to assert that the generated PowerShell script applies “-ErrorAction SilentlyContinue” to the Win32_PerfFormattedData_PerfProc_Process query. Keep the existing query-count, join-expression, and -Filter assertions unchanged.apps/server/src/server.test.ts (1)
3723-3741: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTest correctly validates non-blocking behavior, but runs for ~5 seconds.
Using
Effect.neverwithTestClock.withLivemeans the test waits the full 5-secondEDITOR_DISCOVERY_TIMEOUTin real time before the timeout fires and the fallback applies. This is functionally correct but adds ~5s to the test suite. If this becomes a concern, consider using a virtualTestClockwith a shorter timeout override, or testing the timeout fallback at theloadServerConfigunit level rather than through the full WebSocket RPC.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/server.test.ts` around lines 3723 - 3741, Shorten this test’s runtime while preserving validation that server.getConfig is not blocked by editor discovery. Replace the real-time five-second wait in the test around buildAppUnderTest and the WebSocket RPC with a virtual TestClock and a shorter editor-discovery timeout override, or move the timeout-fallback assertion to the loadServerConfig unit path rather than the full RPC flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/server/src/diagnostics/ProcessDiagnostics.test.ts`:
- Around line 223-257: Extend the “queries each Windows CIM class once” test
around ProcessDiagnostics.readProcessRows to assert that the generated
PowerShell script applies “-ErrorAction SilentlyContinue” to the
Win32_PerfFormattedData_PerfProc_Process query. Keep the existing query-count,
join-expression, and -Filter assertions unchanged.
In `@apps/server/src/server.test.ts`:
- Around line 3723-3741: Shorten this test’s runtime while preserving validation
that server.getConfig is not blocked by editor discovery. Replace the real-time
five-second wait in the test around buildAppUnderTest and the WebSocket RPC with
a virtual TestClock and a shorter editor-discovery timeout override, or move the
timeout-fallback assertion to the loadServerConfig unit path rather than the
full RPC flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9466a82-f659-433c-9e74-3d47afb862ab
📒 Files selected for processing (6)
apps/server/src/diagnostics/ProcessDiagnostics.test.tsapps/server/src/diagnostics/ProcessDiagnostics.tsapps/server/src/process/externalLauncher.test.tsapps/server/src/process/externalLauncher.tsapps/server/src/server.test.tsapps/server/src/ws.ts
9526c33 to
23ac856
Compare
ApprovabilityVerdict: Approved Performance optimization for Windows diagnostics and editor discovery. Changes batch CIM queries, add concurrency, and include timeout fallbacks for resilience. Well-tested and self-contained with no new functionality. You can customize Macroscope's approvability policy. Learn more. |
What Changed
Why
Fixes #3610.
On the affected Windows machine, process diagnostics and optional editor discovery delayed
server.getConfiglong enough for the client to treat the local environment as disconnected. Project history and provider configuration then failed to load.Windows diagnostics previously queried performance data once per process. Reading both CIM classes once and joining them by process ID reduced the measured query to about 866 ms, so this change gives Windows a five-second budget without changing the POSIX timeout.
The first editor-discovery fallback used 500 ms, but that hid installed editors. A trace on the affected machine showed that its 37-entry PATH still took more than five seconds when scanned separately for every editor, even with four concurrent probes. Windows now reuses the existing executable resolver, which restores VS Code and File Manager discovery without introducing another PATH/PATHEXT implementation.
The Windows resolver uses synchronous filesystem checks, so the surrounding Effect timeout cannot preempt a filesystem call while the event loop is blocked. This PR keeps that existing tradeoff to remain a focused fix; a fully interruptible batch resolver would be broader shell infrastructure work.
Validation
pnpm dev:desktopthat VS Code and File Manager are discovered on the affected machine.apps/server/src/process/externalLauncher.test.tspassed.EPERM).Checklist
Note
Speed up Windows process diagnostics and editor discovery
Get-CimInstance Win32_PerfFormattedData_PerfProc_Processcall and uses a dictionary lookup per process, replacing per-process-FilterCIM queries.SpawnExecutableResolutioninstead of async PATH scans; non-Windows discovery runs probes with bounded concurrency (4).availableEditorslist on timeout instead of blocking.POSIX_PROCESS_QUERY_TIMEOUT_MS(1s) andWINDOWS_PROCESS_QUERY_TIMEOUT_MS(5s), replacing a single shared constant.Macroscope summarized f302154.
Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Touches server config and Windows diagnostics/editor paths with timeout fallbacks that can hide editors if discovery is slow; changes are localized with tests but affect local environment connectivity behavior.
Overview
Fixes slow
serverGetConfigon Windows by cutting process-diagnostics cost and keeping editor discovery from blocking config responses.Windows process diagnostics no longer runs a per-process
Get-CimInstanceperf query. One PowerShell script loads all perf rows into$perfById, joins them when enumeratingWin32_Process, and drops-Filter. Query timeouts are 5s on Windows and 1s on POSIX via a per-calltimeoutMillisonrunProcess.Editor discovery on Windows uses
SpawnExecutableResolutioninstead of serial async PATH scans per editor. On other platforms, probes run with concurrency 4.withEditorDiscoveryTimeoutwraps discovery in config/WebSocket handling soavailableEditorsbecomes[]after 5s instead of hanging the response.Tests cover the bulk Windows CIM script shape, the editor-discovery timeout helper, and updated Windows editor discovery expectations.
Reviewed by Cursor Bugbot for commit f302154. Bugbot is set up for automated code reviews on this repo. Configure here.