Skip to content

fix(server): do not HTTP-probe unknown local listeners - #9832

Closed
lnieuwenhuis wants to merge 2 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/port-probe-eligibility
Closed

lnieuwenhuis wants to merge 2 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/port-probe-eligibility

Conversation

@lnieuwenhuis

@lnieuwenhuis lnieuwenhuis commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Every discovered local listener gets HTTP-probed, including foreign processes that stall the probe for seconds.

Only probe discovered listeners on curated dev ports or ones owned by a known terminal; configured preview URLs are unchanged.

Ports #8561 as blast-radius reduction. Related to #8407 (not closing it): curated or terminal-owned listeners can still hit the underlying TLS-abort stall, which needs the socket-cleanup follow-up.

Built with muse-spark-1.3-contributor via OpenCode in T3 Code.


Note

Medium Risk
Changes which local listeners get HTTP probes during preview scans; dev servers on non-curated ports may only appear if the user configures a URL or the terminal PID is registered.

Overview
Port discovery no longer sends HTTP(S) probes to every socket lsof (or Windows listener enumeration) finds. Discovered listeners are only probed when the port is in COMMON_DEV_PORTS or the owning PID is tied to a registered T3 terminal (server.terminal); other listeners are skipped so non-HTTP services are not hit with web requests or long probe timeouts.

Configured preview URLs are unchanged—they still go through the existing configured-URL probe path regardless of port. Tests cover skipping a high port with no terminal registration and probing that same port after registerTerminalProcesses.

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

Note

Stop HTTP-probing unknown local listeners in PortDiscovery

Discovered listeners from lsof are now only probed when their port is in COMMON_DEV_PORTS or they are associated with a registered T3 terminal. Other sockets are skipped entirely, avoiding HTTP requests to foreign or unrelated listeners.

  • Adds isEligibleDiscoveredWebProbe predicate in PortScanner.ts and applies it in the scan candidate grouping loop
  • Updates module-level docs to reflect the new eligibility rule
  • Adds tests for both foreign-listener exclusion and registered-terminal high-port probing
  • Behavioral Change: PortDiscovery.scan no longer HTTP-probes or publishes lsof-discovered listeners outside COMMON_DEV_PORTS with no terminal association

Macroscope summarized 4ab29b7.

Summary by CodeRabbit

  • Bug Fixes
    • Improved preview server detection by avoiding HTTP probes for unrecognized listeners.
    • Continued probing high-numbered listeners when they belong to registered terminals, improving detection of eligible preview servers.
  • Tests
    • Added coverage for filtering unrecognized listeners and recognizing eligible terminal-owned listeners.

PortDiscovery no longer sends HTTP(S) probes to every discovered TCP
listener. A discovered listener is probed only on a curated dev port or
when owned by a registered T3 terminal; configured Preview URLs are
probed as before. This avoids writing HTTP onto unrelated binary/RPC
listeners.

Related pingdotgg#8407 (not closing: a curated/terminal binary can still hit the
~10s TLS-abort stall, socket-cleanup/abort follow-up remains open).
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 4, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4ab29b7

Macroscope's review found this PR approvable — This is a focused server bug fix that stops probing unrelated local listeners while preserving curated-port, terminal-owned, and explicitly configured preview behavior. The production logic is localized and accompanied by targeted tests for both skipped and retained listeners.

No code changes detected at c5d8973. Prior analysis still applies.

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

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6be6fe46-c4a7-4f3c-8633-497ba3fa161b

📥 Commits

Reviewing files that changed from the base of the PR and between 6c58362 and c5d8973.

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

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


📝 Walkthrough

Walkthrough

The preview port scanner now filters discovered listeners before HTTP probing. It probes curated development ports and listeners owned by registered T3 terminals, while tests verify both exclusion and inclusion cases.

Changes

Preview port eligibility

Layer / File(s) Summary
Filter discovered listeners before probing
apps/server/src/preview/PortScanner.ts
The scanner skips discovered servers that are not on curated development ports and are not associated with a registered terminal.
Validate listener filtering
apps/server/src/preview/PortScanner.test.ts
Tests verify that unrelated listeners are not probed and that listeners owned by registered terminals are probed and returned.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c5d89

Discovered unrelated listeners are no longer sent HTTP probes, while curated ports, registered-terminal listeners, and configured preview URLs retain intended behavior. The change has focused coverage and is ready to merge.

Suggested reviewers: juliusmarminge

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing HTTP probes to unknown local listeners.
Description check ✅ Passed The description explains what changed, why it changed, scope, behavior for configured preview URLs, and test coverage. The omitted UI Changes section is not applicable, and the checklist requirements …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
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 PR with unit tests

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

@dbiserov

Copy link
Copy Markdown

Another real-world case for this one.

On Windows, pCloud Drive listens on 127.0.0.1:19191 as its single-instance channel. When the preview panel scans, T3 Code sends GET http://localhost:19191/ and then https://localhost:19191/. pCloud treats each connection as a second launch and brings its main window to the front. So pCloud pops up twice, about 10 seconds apart, whenever a preview opens.

Confirmed on current Nightly (Windows 11) by matching the trace ID in server.trace.ndjson (PortDiscovery.probeWebUrl < pollTick < subscribeDiscoveredLocalServers) with the request pCloud logged. It has happened 13 times since Aug 18.

This PR fixes it, since 19191 isn't a curated dev port and pCloud isn't a T3 terminal process. Would be great to see it merged.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Thanks for this! #16687 just landed on main and covers the same ground: discovery now only probes listeners owned by T3 terminals and skips ports with any unowned listener, which is stricter than the curated-ports approach here. Closing this one as superseded.

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants