From e02d9adc115f531cf4573022f1e8921bc92c29a1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 12 Aug 2026 15:22:38 +0100 Subject: [PATCH] chore: add NewUnstartedHTTPServer helper to disable keep-alives on test servers (#28052) 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. net/http will not retry a non-replayable request (e.g. a POST) on a closed pooled connection, so forcing a fresh connection per request eliminates the failure class. This is the same mechanism fixed in #28016 (AIGOV-430 / internal#1564), now expressed as a reusable, behavior-preserving helper. This PR adds `testutil.NewTestHTTPServer`, a known-good wrapper around `httptest.NewServer` that applies some defaults. Currently the only default is disabling keep-alives by default. - `testutil/http_server.go`: `NewHTTPServer(t, handler, opts)` started, with documented defaults, starts automatically, and handles `t.Cleanup`. - `testutil/http_server_test.go`: unit test for defaults and overriding defaults. - `enterprise/aibridgeproxyd/reload_test.go`: refactor the harness's hand-rolled server to the helper. ## Future Work - Functional options are exposed but not explicitly defined. This can be done later as required. - No lint rule or broader migration. A forcing-function analyzer covering more packages, plus wider adoption, belongs in a separate follow-up. ## Verification - `testutil` and `enterprise/aibridgeproxyd` suites pass under `-race`. - `TestProxy_HotReloadRouting` and `TestProxy_StaleTunnel` pass 10x under `-race`. - New helper unit test passes under `-race`. > Generated by a Coder agent. --- enterprise/aibridgeproxyd/reload_test.go | 11 ++-- testutil/http_server.go | 25 +++++++++ testutil/http_server_test.go | 65 ++++++++++++++++++++++++ 3 files changed, 94 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 f07462d970..8d3aeeef1e 100644 --- a/enterprise/aibridgeproxyd/reload_test.go +++ b/enterprise/aibridgeproxyd/reload_test.go @@ -149,17 +149,14 @@ 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.NewHTTPTestServer(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) store := &providerStore{} metrics := aibridgeproxyd.NewMetrics(prometheus.NewRegistry()) diff --git a/testutil/http_server.go b/testutil/http_server.go new file mode 100644 index 0000000000..3751110ea9 --- /dev/null +++ b/testutil/http_server.go @@ -0,0 +1,25 @@ +package testutil + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// NewHTTPTestServer return a *httptest.Server with the following +// defaults set: +// - keep-alives disabled by default to prevent stale connection reuse (AIGOV-430). +// +// Override these defaults via opts if needed. +// The server is started and will be closed when the test ends. +func NewHTTPTestServer(t testing.TB, handler http.Handler, opts ...func(*httptest.Server)) *httptest.Server { + t.Helper() + srv := httptest.NewUnstartedServer(handler) + srv.Config.SetKeepAlivesEnabled(false) + for _, opt := range opts { + opt(srv) + } + srv.Start() + t.Cleanup(srv.Close) + return srv +} diff --git a/testutil/http_server_test.go b/testutil/http_server_test.go new file mode 100644 index 0000000000..45ad503d21 --- /dev/null +++ b/testutil/http_server_test.go @@ -0,0 +1,65 @@ +package testutil_test + +import ( + "bufio" + "fmt" + "net" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/testutil" +) + +func TestNewUnstartedHTTPServer(t *testing.T) { + t.Parallel() + + send := func(conn net.Conn) error { + _, err := fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: x\r\n\r\n") + if err != nil { + return err + } + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + return err + } + _ = resp.Body.Close() + return nil + } + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + + srv := testutil.NewHTTPTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + conn, err := net.Dial("tcp", srv.Listener.Addr().String()) + require.NoError(t, err, "dial") + defer conn.Close() + + // Keepalives should be disabled: first request succeeds, second request on the same conn fails. + require.NoError(t, send(conn), "keepalives disabled: first request on reused connection must succeed") + require.Error(t, send(conn), "keepalives disabled: second request on reused connection must fail") + }) + + t.Run("override", func(t *testing.T) { + t.Parallel() + + srv := testutil.NewHTTPTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }), func(srv *httptest.Server) { + srv.Config.SetKeepAlivesEnabled(true) + }) + + conn, err := net.Dial("tcp", srv.Listener.Addr().String()) + require.NoError(t, err, "dial") + defer conn.Close() + + // Keepalives should be enabled: multiple requests on the same conn succeed. + require.NoError(t, send(conn), "keepalives enabled: first request on reused connection must succeed") + require.NoError(t, send(conn), "keepalives enabled: second request on reused connection must succeed") + }) +}