Pin npipe round-trip and pipe lifecycle invariants#5216
Open
Conversation
discovery.HTTPClientForURL knew http:// and unix:// only, so a discovery file URL written for a Windows named pipe would be unparseable. Every thv serve startup runs discovery.Discover to detect a previous instance via the discovery URL, which routes through HTTPClientForURL, so without an npipe arm two concurrent thv processes on Windows could not see each other and would silently overwrite the discovery file. Add an npipe:// arm whose transport DialContext goes through winio.DialPipeContext, plus a ParseNamedPipeURL helper that strips the scheme and validates the remaining segment (non-empty, no path separators, no ".."). Split the actual dialer into pipe_windows.go (winio-backed) and pipe_other.go (returns a clear "only supported on Windows" error), so non-Windows builds do not import winio and a misconfigured discovery URL surfaces a useful message instead of an obscure dial syscall failure. go-winio is already a direct dependency at v0.6.2, so no go.mod change is required.
setupUnixSocket assumed the address was a filesystem path -- it
ran os.Stat, os.Remove, os.MkdirAll, and os.Chmod around
net.Listen("unix", ...). None of that applies to a Windows named
pipe (\.\pipe\thv-api), and net.Listen("unix", ...) is not a
substitute since named pipes are a different kernel object. As a
result, toolhive-studio could not run thv with a pipe address on
Windows.
Split setupUnixSocket and cleanupUnixSocket into per-platform
files via build tags, mirroring pkg/container/docker/sdk. On
Windows, branch on the \.\pipe\ prefix and use winio.ListenPipe
with InputBufferSize and OutputBufferSize at 64 KiB; MessageMode
stays at byte-stream because HTTP requires byte framing. Skip
stat/remove/mkdir/chmod for pipes since they are not files;
keep stat/remove/mkdir for the AF_UNIX fallback (Win10 1803+)
but drop chmod because POSIX modes do not apply on Windows.
ListenURL emits npipe://<name> for pipe addresses so the
discovery file URL stays unambiguous. createListener labels the
listener as "Windows named pipe" and rejects pipe addresses on
non-Windows up front, rather than creating a literal-backslash
file via AF_UNIX.
TestParseNamedPipeURL covers the success case plus every
rejection branch (missing scheme, wrong scheme, empty name,
forward slash, backslash, ".."). A non-Windows guard test
asserts CheckHealth("npipe://...") surfaces a clear "Windows"
error rather than a misleading dial syscall failure.
The Windows-only health_windows_test.go spins up a winio
listener with an http.Server.Serve goroutine and asserts
CheckHealth("npipe://<name>", expectedNonce) succeeds with a
matching nonce, plus a not-found case that expects "health
check failed". An atomic counter disambiguates parallel pipe
names, since the Windows pipe namespace is global.
Cover the per-platform refactor of setupUnixSocket. On all platforms, assert socketURL emits unix:// for filesystem addresses and that isNamedPipeAddress detects \.\pipe\ prefixes. On Windows, additionally assert socketURL emits npipe://<name>, that setupUnixSocket against a unique pipe path returns a winio listener that accepts a winio dial within a 2 s timeout (proving the listener is wired to the named-pipe namespace, not AF_UNIX), and that cleanupUnixSocket no-ops for pipe addresses without panicking. Tests are split via build tags so the Windows test file is only compiled on GOOS=windows and pulls in winio there.
Address aponcedeleonch's review on PR #5201: - Update --socket help to mention named pipes and regen CLI docs. - Promote namedPipePrefix to discovery.NamedPipePrefix as the canonical definition; pkg/api re-aliases it locally so listener and dialer cannot drift. - Make isNamedPipeAddress case-insensitive via strings.EqualFold; the Windows pipe namespace is case-insensitive at the kernel layer, so \\.\Pipe\foo must not silently fall through to AF_UNIX. - Collapse stat-then-remove into a single os.Remove that tolerates fs.ErrNotExist on both POSIX and the Windows AF_UNIX fallback. - Close the listener and remove the socket file when Chmod fails, rather than leaking a bound AF_UNIX listener. - Replace stdruntime.GOOS with a per-platform supportsNamedPipe() helper, dropping the runtime-package alias and its collision with pkg/container/runtime. - Rename socket_other.{go,_test.go} to socket_unix.{go,_test.go} to match the client_unix/client_windows convention used by pkg/container/docker/sdk. - Add TestCreateListener_NamedPipe_Unsupported to round out the listener-side reject coverage on non-Windows builds. Co-authored-by: Cursor <cursoragent@cursor.com>
Add three regression tests around the named-pipe transport that the original PR did not cover. None of these reproduce live bugs today; they pin invariants the named-pipe path implicitly relies on so a future change can't silently break them. TestWriteReadServerInfo_NamedPipe (cross-platform) closes the producer/consumer loop for the discovery file: an npipe:// URL written by socketURL must survive WriteServerInfo + ReadServerInfo without being mangled. The individual emit/parse pieces have their own tests; this one pins the seam. TestSetupUnixSocket_NamedPipe_FirstInstanceWins (Windows-only) asserts that two ListenPipe calls on the same name fail the second time. winio sets FILE_FLAG_FIRST_PIPE_INSTANCE so that two thv processes cannot bind the same pipe and race for traffic; if a future winio bump silently relaxed that, this test catches it before the discovery layer does in production. TestCheckHealth_NamedPipe_HungServerCancelsOnContext (Windows-only) covers the StateUnhealthy path: a peer that accepts connections but never responds must not wedge CheckHealth past the caller's context deadline. The existing success and not-found tests don't exercise this case. Co-authored-by: Cursor <cursoragent@cursor.com>
5 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## socker-windows #5216 +/- ##
==================================================
- Coverage 67.81% 67.80% -0.01%
==================================================
Files 610 610
Lines 62256 62244 -12
==================================================
- Hits 42216 42204 -12
+ Misses 16867 16864 -3
- Partials 3173 3176 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samuv
requested review from
ChrisJBurns,
jhrozek,
lujunsan,
rdimitrov and
yrobla
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #5201 (review thread). Stacked on
socker-windows; will rebase ontomainonce #5201 merges. Sibling to #5214 and #5215.Adds three regression tests around the named-pipe transport that the original PR did not cover. None reproduce a live bug today; they pin invariants the named-pipe path implicitly relies on so a future change can't silently break them.
TestWriteReadServerInfo_NamedPipe(cross-platform) — closes the producer/consumer loop for the discovery file: annpipe://URL written bysocketURLmust surviveWriteServerInfo+ReadServerInfowithout being mangled. The individual emit/parse pieces have their own tests; this one pins the seam.TestSetupUnixSocket_NamedPipe_FirstInstanceWins(Windows-only) — asserts that twowinio.ListenPipecalls on the same name fail the second time.winiosetsFILE_FLAG_FIRST_PIPE_INSTANCEso twothvprocesses cannot bind the same pipe and race for traffic; if a futurewiniobump silently relaxed that, this test catches it before the discovery layer does in production.TestCheckHealth_NamedPipe_HungServerCancelsOnContext(Windows-only) — covers theStateUnhealthypath: a peer that accepts connections but never responds must not wedgeCheckHealthpast the caller's context deadline. The existing success and not-found tests don't exercise this case.Addresses inline review comments 3201085442 (round-trip), 3201085446 (first-instance-wins), and 3201085449 (hung-peer).
Type of change
Test plan
go test ./pkg/api/... ./pkg/server/discovery/...— green on macOS (round-trip and Unix tests).task lint-fix— 0 issues.GOOS=windows go vetandGOOS=windows go test -c -o /dev/nullfor both packages — both clean.go test -tags windows -run NamedPipeon a Windows host — pending; deferred to a reviewer with a Windows host. The Windows-only tests are guarded by build tags and do not run on macOS / Linux.Does this introduce a user-facing change?
No.
Made with Cursor