From ddcb642bba1b2e1c228ef8c2835cda8ce0160d79 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 12 Aug 2026 07:36:37 +0000 Subject: [PATCH 1/4] test(testutil): add NewUnstartedHTTPServer helper Introduce testutil.NewUnstartedHTTPServer, wrapping httptest.NewServer with keep-alives disabled by default. Tests that proxy or pool connections to a bare httptest.Server intermittently fail on Windows with a bare EOF when a stale pooled connection is reused, because net/http does not retry a non-replayable request on a closed pooled connection. Disabling keep-alives forces a fresh connection per request. Refactor the aibridgeproxyd hot-reload harness to use it in place of a hand-rolled httptest.Server with SetKeepAlivesEnabled(false). Refs https://github.com/coder/internal/issues/1564 (AIGOV-430) --- enterprise/aibridgeproxyd/reload_test.go | 12 +++--- testutil/http_server.go | 23 ++++++++++++ testutil/http_server_test.go | 48 ++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 testutil/http_server.go create mode 100644 testutil/http_server_test.go diff --git a/enterprise/aibridgeproxyd/reload_test.go b/enterprise/aibridgeproxyd/reload_test.go index f07462d970c..ae83a00a07c 100644 --- a/enterprise/aibridgeproxyd/reload_test.go +++ b/enterprise/aibridgeproxyd/reload_test.go @@ -149,17 +149,15 @@ func newReloadTestHarness(t *testing.T) *reloadTestHarness { t.Helper() recorder := &aibridgedRecorder{} - bridged := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Keep-alives are disabled so the proxy cannot reuse a stale pooled + // connection to aibridged, which would surface as a bare EOF on + // Windows (see AIGOV-430). + bridged := testutil.NewUnstartedHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { recorder.record(r.URL.Path) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("aibridged")) })) - // The proxy reuses pooled connections to aibridged, but net/http will - // not retry a POST on a closed pooled conn, so a stale reuse fails - // with a bare EOF on Windows. Force a fresh conn per request. - // https://github.com/coder/internal/issues/1564 (AIGOV-430) - bridged.Config.SetKeepAlivesEnabled(false) - t.Cleanup(bridged.Close) + bridged.Start() store := &providerStore{} metrics := aibridgeproxyd.NewMetrics(prometheus.NewRegistry()) diff --git a/testutil/http_server.go b/testutil/http_server.go new file mode 100644 index 00000000000..a4fb5e9a47c --- /dev/null +++ b/testutil/http_server.go @@ -0,0 +1,23 @@ +package testutil + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// NewUnstartedHTTPServer is httptest.NewUnstartedServer with keep-alives +// disabled by default. Tests that proxy or pool connections to a bare +// httptest.Server can intermittently fail on Windows with a bare EOF when +// a stale pooled connection is reused, because net/http does not retry a +// non-replayable request (e.g. a POST) on a closed pooled connection. +// Disabling keep-alives forces a fresh connection per request, eliminating +// that class of flake. Call Start before sending requests. See +// https://github.com/coder/internal/issues/1564 (AIGOV-430). +func NewUnstartedHTTPServer(t testing.TB, handler http.Handler) *httptest.Server { + t.Helper() + srv := httptest.NewUnstartedServer(handler) + srv.Config.SetKeepAlivesEnabled(false) + t.Cleanup(srv.Close) + return srv +} \ No newline at end of file diff --git a/testutil/http_server_test.go b/testutil/http_server_test.go new file mode 100644 index 00000000000..ec1fe9097d4 --- /dev/null +++ b/testutil/http_server_test.go @@ -0,0 +1,48 @@ +package testutil + +import ( + "bufio" + "fmt" + "net" + "net/http" + "testing" +) + +// TestNewUnstartedHTTPServerDisablesKeepAlives verifies the helper +// disables keep-alives on the underlying server, which is the property +// that prevents stale pooled-connection reuse (see AIGOV-430). A server +// with keep-alives disabled closes the connection after a response, so a +// second request on the same raw connection must fail. +func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { + t.Parallel() + + srv := NewUnstartedHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + srv.Start() + + conn, err := net.Dial("tcp", srv.Listener.Addr().String()) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + + send := func() error { + _, err := fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: x\r\n\r\n") + if err != nil { + return err + } + _, err = http.ReadResponse(bufio.NewReader(conn), nil) + return err + } + + // First request on this connection succeeds. + if err := send(); err != nil { + t.Fatalf("first request: %v", err) + } + // Keep-alives disabled means the server closes the conn, so the + // second request on the same conn fails. + if err := send(); err == nil { + t.Fatal("expected second request on reused connection to fail") + } +} \ No newline at end of file From 2d022e113f9ab1987130beb2168ba8dd9db59861 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 12 Aug 2026 07:46:27 +0000 Subject: [PATCH 2/4] test(testutil): use require instead of t.Fatalf in helper test --- testutil/http_server_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/testutil/http_server_test.go b/testutil/http_server_test.go index ec1fe9097d4..fe6292c0045 100644 --- a/testutil/http_server_test.go +++ b/testutil/http_server_test.go @@ -6,6 +6,8 @@ import ( "net" "net/http" "testing" + + "github.com/stretchr/testify/require" ) // TestNewUnstartedHTTPServerDisablesKeepAlives verifies the helper @@ -22,9 +24,7 @@ func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { srv.Start() conn, err := net.Dial("tcp", srv.Listener.Addr().String()) - if err != nil { - t.Fatalf("dial: %v", err) - } + require.NoError(t, err, "dial") defer conn.Close() send := func() error { @@ -37,12 +37,8 @@ func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { } // First request on this connection succeeds. - if err := send(); err != nil { - t.Fatalf("first request: %v", err) - } + require.NoError(t, send(), "first request") // Keep-alives disabled means the server closes the conn, so the // second request on the same conn fails. - if err := send(); err == nil { - t.Fatal("expected second request on reused connection to fail") - } + require.Error(t, send(), "second request on reused connection must fail") } \ No newline at end of file From 11398f0f84c26870207749199a38dd517be814b3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 12 Aug 2026 07:50:48 +0000 Subject: [PATCH 3/4] test(testutil): add trailing newline to helper files --- testutil/http_server.go | 2 +- testutil/http_server_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/testutil/http_server.go b/testutil/http_server.go index a4fb5e9a47c..84d8036a130 100644 --- a/testutil/http_server.go +++ b/testutil/http_server.go @@ -20,4 +20,4 @@ func NewUnstartedHTTPServer(t testing.TB, handler http.Handler) *httptest.Server srv.Config.SetKeepAlivesEnabled(false) t.Cleanup(srv.Close) return srv -} \ No newline at end of file +} diff --git a/testutil/http_server_test.go b/testutil/http_server_test.go index fe6292c0045..4a55107be7d 100644 --- a/testutil/http_server_test.go +++ b/testutil/http_server_test.go @@ -41,4 +41,4 @@ func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { // Keep-alives disabled means the server closes the conn, so the // second request on the same conn fails. require.Error(t, send(), "second request on reused connection must fail") -} \ No newline at end of file +} From 41ab821d1c30f6f0fc4ac7edcddebc3f6d430d06 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 12 Aug 2026 07:53:06 +0000 Subject: [PATCH 4/4] test(testutil): use external test package and close response body --- testutil/http_server_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/testutil/http_server_test.go b/testutil/http_server_test.go index 4a55107be7d..fb7fc2e2a72 100644 --- a/testutil/http_server_test.go +++ b/testutil/http_server_test.go @@ -1,4 +1,4 @@ -package testutil +package testutil_test import ( "bufio" @@ -8,6 +8,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/testutil" ) // TestNewUnstartedHTTPServerDisablesKeepAlives verifies the helper @@ -18,7 +20,7 @@ import ( func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { t.Parallel() - srv := NewUnstartedHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := testutil.NewUnstartedHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) srv.Start() @@ -32,8 +34,12 @@ func TestNewUnstartedHTTPServerDisablesKeepAlives(t *testing.T) { if err != nil { return err } - _, err = http.ReadResponse(bufio.NewReader(conn), nil) - return err + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + return err + } + _ = resp.Body.Close() + return nil } // First request on this connection succeeds.