fix(server): discover Windows ports without PowerShell [WIP] - #9520
fix(server): discover Windows ports without PowerShell [WIP]#9520UtkarshUsername wants to merge 13 commits into
Conversation
|
Note 🤖 GPT-6 Astra (preview) responding on behalf of Theo This note is part of an automated cleanup pass. Carryover from #6254 at 920f8251d9: retain listener-to-PID/process-name mapping and the interrupted-scan case, where a later scan must run normally. Check degraded discovery with a previously found non-common port so it does not disappear only because fallback probes common ports. Its old single-flight test does not start the first fork before awaiting the second scan, so do not copy that test unchanged. Keep terminal ownership fresh when reusing listener data. |
c9abc35 to
674d991
Compare
SunkenInTime
left a comment
There was a problem hiding this comment.
Tested this draft on Windows x64 in a dedicated worktree at c27fa4e1b131311051d609a344ca2c6f5e7262a2, including its incremental diff over #9476. Requesting changes for finding 1.
-
P2: reject failed or truncated PowerShell results before replacing the snapshot. At PortScanner.ts:547, stdout is parsed without checking
result.codeorresult.stdoutTruncated. ProcessRunner returns nonzero exits as ordinary results, and this call uses truncation mode. Starting with a valid listener, an exit-1/empty-stdout result clears it and resets cooldown; the next immediate scan launches PowerShell again. Truncated output has the same problem. Two added regression cases fail: expected one retained listener and two total launches, received zero listeners and three launches. Validate exit status and completeness before parsing, route invalid results through the existing failure branch, and add both cases to the suite. -
Separate existing Windows issue: rapid terminal restart followed by close crashes with
Signals not supported on windows.The adapter, node-pty dependency, and explicit signal path predate these PRs; a standalone reproduction also fails. This needs a separate close-before-ready fix and is not the reason for requesting changes here. -
Nonblocking cleanup: add direct response, timeout, interruption, and restart coverage for the new client command. Consider sharing its repeated request lifecycle machinery. Also consider matching #9476's backoff when native discovery fails but PowerShell succeeds: this scanner resets failures and resumes PowerShell every scan, which may be an intentional compatibility choice.
Existing validation passes: bun fmt, bun lint, bun typecheck, 144 focused TypeScript tests, 18 Rust tests, Rust formatting, and release build. Native probes verified IPv4/IPv6 ownership and closed-listener removal; real PTY/RPC checks verified Node activity, terminal ownership, Ctrl+C, and port removal. The two additional failure cases above fail. This was not an installer, sustained CPU, or separate #9520 frontend acceptance run.
Blueprint review and design report includes numbered findings, the required correction, architecture, and full evidence. Download the evidence bundle and regression cases.
c27fa4e to
c948897
Compare
|
Addressed the review follow-ups in ee94393:
The separately reproduced Focused verification passed: 37 port scanner and native client tests, plus 3 manager fallback and pacing tests. The broader manager run had one Windows temp-file |
SunkenInTime
left a comment
There was a problem hiding this comment.
Re-reviewed on Windows x64 at b58b4cbc43edf9a73f95a388be9ea6dc2bcc8c85, against base bde39d4d7977ce85d6ea396a983d6b6a25bf7e07.
-
Resolved: the original failed/truncated-result finding. Both regression cases from my previous review now pass. The shared request helper is also in place, and a new test verifies that a 5-second successful fallback waits 20 seconds after completion before retrying.
-
P2: preserve the last native snapshot when discovery degrades. PortScanner.ts:619-623 returns native listeners without updating retained state. If the next native request fails and PowerShell also fails,
lastSnapshotis still null, so the scanner probes only common ports. The added reproduction discovers a native listener on port 43123, then makes both discovery paths unavailable while the HTTP fixture remains healthy. The next scan returns zero servers. Retain the latest authoritative native result too, including successful empty results, and refresh terminal ownership when reusing it. This issue remains in the PR; it is not introduced specifically by the latest fix commit. -
P2: start failure cooldown when the failed probe completes. PortScanner.ts:592 uses the timestamp captured before launching PowerShell. With the new 15-second timeout, a slow failure can consume the entire initial 3-, 6-, or 12-second retry delay. A test advancing the clock by 15 seconds inside the failing runner observes two launches across two immediate scans instead of one. The regular polling loop still supplies its own 3-second interval, but the intended additional failure cooldown has expired. Compute this deadline from completion, as the successful path already does.
-
Nonblocking coverage clarification. The four new request-lifecycle tests exercise the extracted helper. The restart case manually fails a Deferred and starts another helper call; it does not exercise sidecar exit handling, pending-map draining, a replacement process, or response dispatch. Useful coverage, but please either add that client-level integration coverage or narrow the claim of directly tested post-restart behavior.
Validation: bun fmt, bun lint, and clean-head bun typecheck pass; 150 focused TypeScript tests and 18 Rust tests pass; native formatting and release build pass. Twelve process-table probes and six IPv4/IPv6 listener snapshots pass, including listener removal. Live PTY/RPC checks pass for child activity, listener ownership, Ctrl+C, and port removal. Windows computer use loaded the isolated app and opened its PowerShell terminal drawer after a transient reconnect. Five additional review cases yield three passes and the two failures above.
The previously documented PTY restart/close issue remains a separate follow-up and was not re-exercised here. No installer, ARM64, POSIX, sustained CPU comparison, or soak claim is made.
Updated Blueprint report and downloadable evidence with all five review cases.
|
Addressed the two P2 findings in f690dbe. Native listener snapshots are now retained as the fallback cache, including empty snapshots, and cached ownership is re-resolved on reuse. Failed PowerShell probes now start their cooldown when they complete. Added regression coverage for both cases. I also narrowed the PR description: the request-lifecycle tests exercise the helper, not a full sidecar-restart integration. |
|
Added the requested client-level lifecycle coverage in 9329deb: an in-flight process-table request fails when the sidecar exits, the supervisor starts a replacement after its backoff, and the next request is dispatched and answered by that replacement. |
There was a problem hiding this comment.
reviewed at 4c5178e. no new blocking findings; no code changes needed from this review.
verified repairs:
- native listener results now populate the retained snapshot, including empty results. reuse refreshes terminal ownership.
- failed fallback cooldown begins after the probe completes; failed and truncated results preserve the previous snapshot.
- the new client-level test exercises sidecar exit, pending-request failure, supervisor restart, and a response from the replacement through the real client with a mocked process spawner.
local validation on linux: 153 tests passed across the scanner, native client, diagnostics, telemetry and terminal manager suites; the native-client file also passed independently, 12 tests. server typecheck and diff whitespace check passed. tracked files remain unchanged.
limits: this was not a native windows execution, installer test, or sustained cpu benchmark. the restart integration uses processTable, not windowsListeners; direct client-level coverage of windowsListeners success/error dispatch would be a useful nonblocking addition.
ci checks are green with optional checks skipped. the branch is mergeable, but this remains a draft. posting a comment rather than approval; the earlier changes-requested reviews belong to their original reviewer.
What Changed
Why
Windows preview discovery previously ran
Get-NetTCPConnectionevery three seconds and then started oneGet-Processcall per listener. On affected systems, those WMI-backed commands could overlap and cause sustained CPU usage.The resource-monitor sidecar already provides native process telemetry, so reading the Windows TCP table there removes PowerShell from the normal discovery path. The fallback keeps discovery working when native telemetry is unavailable, while bounded retries prevent a failed probe from repeatedly spawning PowerShell. Keeping the last good snapshot also avoids making already-discovered servers disappear during a temporary outage, and refreshing terminal ownership keeps cached results accurate as terminals change.
Closes #5900.
Checklist
Verification
vp test run apps/server/src/preview/PortScanner.test.ts apps/server/src/resourceTelemetry/NativeTelemetryClient.test.ts apps/server/src/diagnostics/ProcessDiagnostics.test.ts apps/server/src/resourceTelemetry/Model.test.ts apps/server/src/resourceTelemetry/ResourceTelemetry.test.ts apps/server/src/resourceTelemetry/ResourceTelemetryHistory.test.ts(71 passed)git diff --checkModel: GPT-5 | Harness: Codex / T3 Code