Skip to content

Commit a9a8b72

Browse files
samuvcursoragent
andcommitted
Pin npipe round-trip and pipe lifecycle invariants
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>
1 parent 785ddc3 commit a9a8b72

3 files changed

Lines changed: 91 additions & 0 deletions

File tree

pkg/api/socket_windows_test.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,3 +87,25 @@ func TestCleanupUnixSocket_NamedPipe_NoOp(t *testing.T) {
8787
// cleanly.
8888
cleanupUnixSocket(`\\.\pipe\thv-api-cleanup-noop`)
8989
}
90+
91+
// TestSetupUnixSocket_NamedPipe_FirstInstanceWins pins winio's first-wins
92+
// semantics: ListenPipe sets FILE_FLAG_FIRST_PIPE_INSTANCE, so a second
93+
// listener targeting the same name must fail. This is the safety net the
94+
// discovery layer relies on to detect a stale-PID conflict; if a future winio
95+
// bump silently relaxed it, two thv processes could bind the same pipe and
96+
// quietly race for traffic.
97+
func TestSetupUnixSocket_NamedPipe_FirstInstanceWins(t *testing.T) {
98+
t.Parallel()
99+
pipePath := uniqueTestPipe()
100+
101+
first, err := setupUnixSocket(pipePath)
102+
require.NoError(t, err)
103+
t.Cleanup(func() { _ = first.Close() })
104+
105+
second, err := setupUnixSocket(pipePath)
106+
require.Error(t, err, "second ListenPipe on the same name must fail")
107+
if second != nil {
108+
_ = second.Close()
109+
}
110+
assert.Contains(t, err.Error(), "failed to create named pipe listener")
111+
}

pkg/server/discovery/discovery_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,31 @@ func TestWriteReadServerInfo_UnixSocket(t *testing.T) {
5454
assert.Equal(t, info.Nonce, got.Nonce)
5555
}
5656

57+
// TestWriteReadServerInfo_NamedPipe pins the producer/consumer contract for
58+
// npipe:// discovery URLs end to end. The individual pieces (socketURL emit,
59+
// HTTPClientForURL dispatch, ParseNamedPipeURL parse) are covered in their
60+
// own tests; this test asserts that an npipe URL survives the discovery
61+
// file's JSON serialization round-trip without being mangled.
62+
func TestWriteReadServerInfo_NamedPipe(t *testing.T) {
63+
t.Parallel()
64+
dir := t.TempDir()
65+
66+
info := &ServerInfo{
67+
URL: "npipe://thv-api",
68+
PID: 99999,
69+
Nonce: "test-nonce-pipe",
70+
StartedAt: time.Date(2026, 5, 7, 14, 0, 0, 0, time.UTC),
71+
}
72+
73+
require.NoError(t, writeServerInfoTo(dir, info))
74+
75+
got, err := readServerInfoFrom(dir)
76+
require.NoError(t, err)
77+
assert.Equal(t, info.URL, got.URL)
78+
assert.Equal(t, info.PID, got.PID)
79+
assert.Equal(t, info.Nonce, got.Nonce)
80+
}
81+
5782
func TestReadServerInfo_NotFound(t *testing.T) {
5883
t.Parallel()
5984
dir := t.TempDir()

pkg/server/discovery/health_windows_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"net/http"
1212
"sync/atomic"
1313
"testing"
14+
"time"
1415

1516
"github.com/Microsoft/go-winio"
1617
"github.com/stretchr/testify/assert"
@@ -51,3 +52,46 @@ func TestCheckHealth_NamedPipe_NotFound(t *testing.T) {
5152
require.Error(t, err)
5253
assert.Contains(t, err.Error(), "health check failed")
5354
}
55+
56+
// TestCheckHealth_NamedPipe_HungServerCancelsOnContext pins that a peer which
57+
// accepts the connection but never responds does not wedge CheckHealth: when
58+
// the caller's context expires the dial / read returns and CheckHealth surfaces
59+
// a wrapped error. This is the discovery StateUnhealthy path; without this
60+
// guarantee a hung peer would block the previous-instance probe forever.
61+
func TestCheckHealth_NamedPipe_HungServerCancelsOnContext(t *testing.T) {
62+
t.Parallel()
63+
64+
pipeName := fmt.Sprintf("thv-test-hung-%d", pipeNameSeq.Add(1))
65+
pipePath := `\\.\pipe\` + pipeName
66+
67+
listener, err := winio.ListenPipe(pipePath, &winio.PipeConfig{})
68+
require.NoError(t, err)
69+
t.Cleanup(func() { _ = listener.Close() })
70+
71+
// Drain accepts so the dial succeeds, but never write anything back. The
72+
// goroutine exits when the listener is closed via t.Cleanup.
73+
go func() {
74+
for {
75+
conn, acceptErr := listener.Accept()
76+
if acceptErr != nil {
77+
return
78+
}
79+
// Hold the connection open without responding so CheckHealth's
80+
// HTTP read blocks until the context deadline fires.
81+
t.Cleanup(func() { _ = conn.Close() })
82+
}
83+
}()
84+
85+
ctx, cancel := context.WithTimeout(context.Background(), 250*time.Millisecond)
86+
defer cancel()
87+
88+
start := time.Now()
89+
err = CheckHealth(ctx, "npipe://"+pipeName, "")
90+
elapsed := time.Since(start)
91+
92+
require.Error(t, err)
93+
// The CheckHealth call must return promptly after the context expires
94+
// rather than blocking on the hung peer indefinitely. healthTimeout is
95+
// 5 s, so anything within ~2 s of the 250 ms ctx is the context path.
96+
assert.Less(t, elapsed, 2*time.Second, "CheckHealth wedged on a hung peer")
97+
}

0 commit comments

Comments
 (0)