Skip to content

fix: speed up Windows process diagnostics#4228

Open
StiensWout wants to merge 1 commit into
pingdotgg:mainfrom
StiensWout:fix/windows-process-diagnostics-timeout
Open

fix: speed up Windows process diagnostics#4228
StiensWout wants to merge 1 commit into
pingdotgg:mainfrom
StiensWout:fix/windows-process-diagnostics-timeout

Conversation

@StiensWout

@StiensWout StiensWout commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the Windows per-process performance CIM lookup with one bulk performance query
  • join process and performance snapshots by PID in PowerShell
  • add deterministic coverage for command shape, parsing, timeout cleanup, and bounded error recovery

Root cause

The Windows diagnostics command queried Win32_PerfFormattedData_PerfProc_Process once for every Win32_Process row. That N+1 CIM pattern routinely exceeded the existing 1,000 ms diagnostics deadline.

The optimized command completed in all 25 local fresh-PowerShell samples (674–735 ms in the initial sample; 670 ms min, 701 ms median, 730 ms p95, and 733 ms max across the expanded 20-run sample). None exceeded 1,000 ms. This change intentionally keeps the existing deadline and limits scope to eliminating the expensive query pattern; a timeout increase can be considered separately if cross-machine evidence warrants it.

Impact

Windows process diagnostics now issue two CIM queries regardless of process count while preserving the existing output shape and zero CPU fallback when performance data is unavailable. POSIX behavior and all timeout values are unchanged.

Validation

  • vp test run apps/server/src/diagnostics/ProcessDiagnostics.test.ts (9 passed)
  • vp lint apps/server/src/diagnostics/ProcessDiagnostics.ts apps/server/src/diagnostics/ProcessDiagnostics.test.ts --report-unused-disable-directives
  • vp run typecheck from apps/server
  • targeted vp fmt ... --check
  • git diff --check

Related: #3610


Note

Low Risk
Scoped to Windows diagnostics PowerShell and test coverage; POSIX paths and timeouts are unchanged, with behavior preserved aside from performance.

Overview
Windows process diagnostics no longer run a per-process performance CIM lookup inside the Win32_Process loop. The PowerShell script now loads all Win32_PerfFormattedData_PerfProc_Process rows once into $perfByPid, then builds each process object with CPU from that map (still 0 when perf data is missing). Output shape and the 1s query deadline are unchanged; POSIX ps behavior is unchanged.

Tests pin the new command shape (single perf CIM call, no -Filter), JSON parsing, scoped child cleanup on timeout, and bounded read results when the Windows query hangs. Several integration tests now set HostProcessPlatform explicitly to linux.

Reviewed by Cursor Bugbot for commit d9673af. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Speed up Windows process diagnostics by replacing per-process CIM queries with a bulk query

  • readWindowsProcessRows in ProcessDiagnostics.ts previously issued a separate Get-CimInstance Win32_PerfFormattedData_PerfProc_Process -Filter "IDProcess = ..." call for every process.
  • Now performs a single bulk query of Win32_PerfFormattedData_PerfProc_Process, materializes results into a hashtable keyed by IDProcess, then joins with a single Win32_Process iteration using ContainsKey.
  • New tests cover the bulk query command structure, timeout/deadline behavior with scoped cleanup, and bounded results after a timeout.

Macroscope summarized d9673af.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a4930ba8-716b-47fa-a851-37b4078b34e3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Jul 21, 2026
@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

Performance optimization that batches Windows CIM queries instead of running one per process. The change is small, behavior-preserving, and includes comprehensive test coverage for the new query pattern and timeout handling.

You can customize Macroscope's approvability policy. Learn more.

Copy link
Copy Markdown

Windows cross-machine validation for d9673afe5 against merge-base c0bb23734:

Method

  • Executed the exact PowerShell generated by each revision in a fresh powershell.exe -NoProfile -NonInteractive process.
  • 3 serial samples per revision, with 383–391 process rows.
  • Windows build 26200.8875, AMD64, Windows PowerShell 5.1.26100.8875.
  • Separately called the real ProcessDiagnostics.readProcessRows under a live Effect clock to include the existing 1,000 ms production deadline and cleanup behavior.
Command revision n min median max
base N+1 CIM command 3 197,891.2 ms 198,697.8 ms 203,691.2 ms
PR bulk CIM command 3 1,811.9 ms 1,933.7 ms 2,121.2 ms

The command itself is 102.8× faster at the median (99.03% lower latency). This strongly validates removing the per-process CIM query.

However, the unchanged 1,000 ms deadline still matters on this host. Live readProcessRows results:

  • base: 3/3 ProcessDiagnosticsQueryTimeoutError, observed completion/cleanup at 1,314–1,352 ms;
  • PR: 3/3 ProcessDiagnosticsQueryTimeoutError, observed completion/cleanup at 1,316–1,341 ms.

So the PR removes the pathological ~3.3-minute query, but diagnostics still do not successfully return here because the optimized 1.81–2.12 second command remains above the production deadline. This appears to be the cross-machine evidence mentioned in the PR body for considering a separate timeout adjustment or decoupling diagnostics from latency-sensitive work.

The focused PR suite passes locally: apps/server/src/diagnostics/ProcessDiagnostics.test.ts9/9 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants