Skip to content

perf(server): avoid expensive Windows listener probes - #19299

Closed
daniyal-sh wants to merge 1 commit into
pingdotgg:mainfrom
daniyal-sh:perf/windows-listener-discovery-sol61
Closed

daniyal-sh wants to merge 1 commit into
pingdotgg:mainfrom
daniyal-sh:perf/windows-listener-discovery-sol61

Conversation

@daniyal-sh

Copy link
Copy Markdown
Contributor

Problem

Windows preview discovery starts PowerShell/WMI to enumerate TCP listeners and resolve process names on each scan. On this Windows machine, the original command took a median 3,706 ms with 200 added listeners. Preview discovery needs PID ownership; the expensive process-name resolution is unnecessary for finding terminal-owned web servers.

Change

Use one netstat.exe -ano call for IPv4/IPv6 listeners, identify listening rows by their empty remote endpoint so parsing works with localized state labels, and keep process names nullable. Cache the raw table for three seconds while applying current terminal ownership on every read. Skip process discovery when no terminal PIDs are registered, retain configured-URL probes, and back off failed/truncated/timed-out scans from five seconds to one minute. Unowned ports remain excluded from HTTP probing.

Scope and approval

This is a focused performance fix for the existing Windows preview scanner, limited to its implementation and regression tests. It is submitted under the focused obvious-defect exception: recurring scans launch an expensive WMI query for data the scanner does not require. No new capability or setting is introduced; no prior maintainer approval is claimed.

Related open alternative: #18524. This version uses one netstat call, avoids extra tasklist name lookups, and adds raw-table coalescing, current-owner filtering, and failure backoff.

Verification

  • vp test run apps/server/src/preview/PortScanner.test.ts: 28 passed on Windows, including a real owned HTTP listener through the process runner. Coverage includes localized IPv4/IPv6 rows, shared/unowned ports, zero spawns without owners, cache coalescing, changed ownership, and retry recovery.
  • Three interleaved command samples per listener count, including process startup: about 50 existing listeners plus 0/30/200 real Node TCP listeners. At 200 added listeners, median 3,706.46 → 35.90 ms, approximately 103× for this command. This is not a whole-app speedup claim.
  • Raw measurements and methodology. Timing baseline was 107f6024 on Windows, Node 24.19.0; the submitted source is based on current main 8b460a3c.

Post-rebase validation on this branch at upstream main 8b460a3c (Effect 4.0.4): 28 scanner tests passed; vp run --filter t3 typecheck, changed-file lint, vp fmt --check, and git diff --check passed.

Implemented and validated using GPT-6.1 Sol in T3 Code through the Codex harness.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 8124efb

Macroscope's review found this PR approvable — This is a contained Windows-only performance fix that replaces costly listener probing with a lightweight netstat path and bounded caching/backoff, while preserving configured URL probing. The production logic stays within the existing scanner and is accompanied by targeted unit and Windows integration coverage.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ab847c81-449a-49b0-bf82-2730b9845f95
📥 Commits

Reviewing files that changed from the base of the PR and between 8855a6d and 8124efb.

📒 Files selected for processing: 2
  • apps/server/src/preview/PortScanner.test.ts
  • apps/server/src/preview/PortScanner.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Windows listener discovery now uses netstat.exe -ano instead of PowerShell. The scanner parses listener ownership, caches output for one poll interval, and retries failed scans with capped exponential delays. Tests cover parsing, ownership, caching, retries, configured URLs, and a real Windows listener.

Changes: Windows port scanning

Layer / File(s) Summary
Parse Windows listener rows
apps/server/src/preview/PortScanner.ts, apps/server/src/preview/PortScanner.test.ts
The parser reads TCP rows from netstat output, accepts wildcard-zero remote endpoints, validates local ports, and records PID ownership without process names. Tests cover owned IPv4 and IPv6 listeners and exclude ineligible or malformed rows.
Run and retry Windows scans
apps/server/src/preview/PortScanner.ts, apps/server/src/preview/PortScanner.test.ts
Windows discovery skips scans when no terminal PIDs are registered or a retry delay is active. It caches successful output for one poll interval, applies current terminal ownership when parsing cached output, and retries failures with capped exponential delays. Tests cover configured URLs, cache behavior, retries, and a real listener.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 8124e

No actionable issue remains in the Windows listener-scanning change; it is ready to merge after normal checks.

Security architecture risk: 🔵 Low · up to 8124e

The change is limited to Windows preview discovery, but a recently closed listener can briefly remain eligible for automatic probing if another local service takes its port. Existing ownership checks, probe caching, and the short cache lifetime limit the exposure.

Retained concerns

  • Low · security · inferred: A cached listener row can continue to authorize automatic HTTP/TLS probes after its registered process releases the port and an unrelated local service binds it. This extends the pre-existing probe race across subsequent scans, subject to cache timing.
Security review details

Security Blast Radius

  • inferred — The identified exposure is bounded to loopback ports on the Windows server where an owned listener is released and another service takes its port during cache validity; scanner-derived endpoints are then offered to the existing discovery subscriber.

Security Findings and Attack Paths

  • inferred — A replacement local service could receive an automatic HTTP/TLS request when a stale listener row still maps to a registered terminal PID and the independent HTTP probe result has expired. No production occurrence or exploit outcome is established.

Trust Boundaries and Controls

  • observed — Removing terminal registration removes its ownership from later cached-table reads, and scans with no registered PIDs skip listener discovery. These controls do not establish whether a still-registered PID continues to own a cached socket.

Resilience and Maintainability Implications

  • observed — Incomplete process results are not cached, concurrent scans share a serialized path, and failure backoff preserves configured-URL discovery rather than authorizing unowned listener probes.

Hardening Proposals

  • proposed — Before a cached row triggers a fresh automatic HTTP/TLS request, verify that its listener and PID still match a current OS snapshot, or otherwise avoid using that stale row to authorize the request.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Description check Passed The description includes all required sections. It clearly explains the Windows performance problem, implementation changes, scope exception, related issue, focused verification, measured results, and…
Title check Passed The title is concise, specific, and accurately summarizes the main change: reducing the cost of Windows listener probes in the server.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks for investigating Windows listener discovery. The netstat replacement, cache, and retry backoff target scan performance, while the timings measure the command rather than the preview-discovery user path. Right now we are only taking focused bug fixes from outside contributors, as allowed by our limited review capacity in Read this first. Please preserve the slow-discovery case in an issue and agree on a focused scope with us before requesting reconsideration with before/after evidence through preview discovery.

@maria-rcks maria-rcks closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 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