Skip to content

Move pipe and socket URL handling to net/url#5215

Open
samuv wants to merge 1 commit intomainfrom
windows-pipe-net-url
Open

Move pipe and socket URL handling to net/url#5215
samuv wants to merge 1 commit intomainfrom
windows-pipe-net-url

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented May 7, 2026

Summary

Follow-up to #5201 (review thread). Stacked on socker-windows; will rebase onto main once #5201 merges. Sibling to #5214.

Replaces prefix-and-trim string handling with net/url.Parse on both producer (socketURL) and consumer (HTTPClientForURL, ParseNamedPipeURL, ParseUnixSocketPath) sides so pipe and socket addresses round-trip through one library and one set of rules.

The change includes one real bug fix: socketURL previously emitted unix://C:\path\thv.sock for Windows AF_UNIX paths, which url.Parse rejects with invalid port ":\\path\\thv.sock". The dispatcher only worked because it used strings.TrimPrefix. New behavior emits unix:///C:%5Cpath%5Cthv.sock, which round-trips cleanly. Round-trip is now exercised by TestSocketURL_RoundTrip_Unix and TestSocketURL_RoundTrip_NamedPipe.

Other improvements that fall out of the migration:

  • Case-insensitive scheme dispatchNPIPE://x routes the same arm as npipe://x because url.Parse lowercases the scheme during parsing. Pinned by TestHTTPClientForURL_SchemeDispatchCaseInsensitive.
  • Case-insensitive pipe-name normalizationnpipe://Thv-API resolves to \\.\pipe\thv-api via strings.ToLower(u.Hostname()); matches the kernel's case-insensitive pipe namespace.
  • Length cap on pipe names (247 chars, the CreateNamedPipeW lpName limit after the \\.\pipe\ prefix).
  • Reserved-name rejection for legacy Windows device names (CON, NUL, PRN, AUX, COM1-9, LPT1-9).
  • Positive charset of [A-Za-z0-9._-]+ for pipe names — replaces the over-eager strings.Contains(name, "..") guard which rejected legitimate names like my..api.
  • Stricter URL shape: ParseNamedPipeURL rejects pipe URLs with non-empty path / query / fragment / userinfo / port; ParseUnixSocketPath rejects unix URLs with non-empty authority.

Addresses inline review comments 3201085366 (ParseNamedPipeURL migration), 3201085375 (dispatcher migration), and 3201085383 (socketURL / ParseUnixSocketPath migration + Windows AF_UNIX bug).

Type of change

  • Bug fix (Windows AF_UNIX URL round-trip)
  • Code quality / hardening

Test plan

  • Unit tests (go test ./pkg/api/... ./pkg/server/discovery/...) — green on macOS; new round-trip tests, scheme-case test, reserved-name tests, length-cap test, and shape-rejection cases all pass.
  • task lint-fix — 0 issues.
  • GOOS=windows go vet and GOOS=windows go test -c -o /dev/null for both packages — both clean.

Does this introduce a user-facing change?

Yes (Windows-only). On Windows, socketURL for AF_UNIX paths now emits a percent-encoded URL form. The discovery file is local per-user and overwritten on every thv serve startup, so there is no on-disk migration concern. POSIX paths like /tmp/thv.sock produce identical output before and after.

Made with Cursor

@samuv samuv requested review from JAORMX and amirejaz as code owners May 7, 2026 13:09
@samuv samuv self-assigned this May 7, 2026
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.77%. Comparing base (d5337ec) to head (24b1dfe).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/server/discovery/health.go 76.47% 4 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5215      +/-   ##
==========================================
- Coverage   67.79%   67.77%   -0.03%     
==========================================
  Files         608      610       +2     
  Lines       62234    62288      +54     
==========================================
+ Hits        42191    42214      +23     
- Misses      16869    16899      +30     
- Partials     3174     3175       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from socker-windows to main May 7, 2026 13:42
Replace prefix-and-trim string handling with net/url.Parse on both
the producer (socketURL) and the consumer (HTTPClientForURL,
ParseNamedPipeURL, ParseUnixSocketPath) sides so pipe and socket
addresses round-trip through one library and one set of rules.

socketURL now emits unix:///<path> via (&url.URL{}).String(),
which fixes a real bug on Windows AF_UNIX paths: the previous
concatenation form produced unix://C:\path\thv.sock, which
url.Parse rejects with "invalid port :\\path\\thv.sock". The
round-trip is exercised by new tests on both platforms.

HTTPClientForURL now dispatches on url.Parse + switch u.Scheme,
which gives case-insensitive scheme matching (NPIPE://x routes the
same as npipe://x) for free and matches what the http arm already
does via ValidateLoopbackURL.

ParseNamedPipeURL gains explicit checks that net/url alone cannot
do: a 247-character cap (CreateNamedPipeW's lpName limit minus the
\\.\pipe\ prefix), the legacy reserved Windows device names
(CON, NUL, COM1-9, LPT1-9, ...), and a positive charset of
[A-Za-z0-9._-]+. Names are lowercased via strings.ToLower because
the pipe namespace is case-insensitive at the kernel layer; the
old strings.Contains(name, "..") guard is removed because it
rejected legitimate names like my..api.

ParseUnixSocketPath rejects URLs with non-empty authority, query,
fragment, or userinfo; it also strips the synthetic leading slash
that url.URL inserts in front of Windows drive letters so
unix:///C:%5Cpath%5Cthv.sock round-trips back to C:\path\thv.sock.

Co-authored-by: Cursor <cursoragent@cursor.com>
@samuv samuv force-pushed the windows-pipe-net-url branch from 9a83a9d to 24b1dfe Compare May 7, 2026 13:54
@samuv samuv requested a review from aponcedeleonch May 7, 2026 13:55
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant