test: no test request goes through the process-global default transport (BUG-3008, closes BUG-2949) (#1324)

Closes BUG-3008. Closes BUG-2949, which filed this failure on 09-07 with the
mechanism explicitly undiagnosed and stopped in the right place: it refuted the
obvious candidate, noted a repo-wide grep found no other caller, and listed
three hypotheses. This is hypothesis (1), and the caller is the standard
library rather than this repo.

httptest.Server.Close() calls CloseIdleConnections() on the PROCESS-WIDE
http.DefaultTransport — deliberately, and the standard library says so in its
own comment, calling it "not part of httptest.Server's correctness". So in a
package where many tests each stand up a server, every `defer ts.Close()`
mutates state every other test's requests depend on. CI observed a request
failing in one test while the server that closed belonged to another:

    client_stream_identity_test.go:135: request not sendable:
      Get "http://127.0.0.1:45521/api/v1/events/stream?armed=true":
      net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called

SCOPE is every test request in internal/server, not just the parallel ones.
Three revisions were needed to get the class right, and each earlier boundary
was wrong in a way worth recording:

  - Direct call sites in test bodies missed SHARED HELPERS. connectSSE,
    apiRequest and readRawSSEFramesAuthed sent through the default transport
    and have parallel callers; connectSSE holds its request open for a whole
    test. A helper cannot know which of its callers is exposed, so it no
    longer tries to.
  - "Serial tests are safe" is true but is the wrong boundary. Serial-ness is
    one t.Parallel() away, and CONVE-2086 tells the next author to add exactly
    that line.
  - A nil Transport IS http.DefaultTransport. listen_serve_test.go's health
    probe was exposed identically and appears in no grep for DefaultClient.

internal/cli is deliberately different: its tests drive the PRODUCTION client
from NewClientFromURL, which has a nil Transport by design, so those sends
cannot be isolated without changing the shipped CLI. There the boundary really
is parallel-vs-serial, and the two parallel tests that build their own requests
use srv.Client(). The asymmetry is written into isolatedTestClient's doc
comment rather than left to be inferred.

WHAT IS NOT CLAIMED: Transport.CloseIdleConnections documents that it "does not
interrupt any connections currently in use", and the idle pool is mutated under
idleMu. The interleaving by which errCloseIdleConns reached a caller's Do() is
NOT reconstructed, and the comment says so instead of inventing one. That
unknown is the argument for isolation over a narrower repair: a client with its
own transport is out of reach whichever window it was.

CONVE-2086 amended with the rule, so the convention that creates the exposure
also carries it.

14 files, all _test.go. Codex: four rounds, CLEAN on the fourth; rounds 1-3
each found a real defect in the class boundary or in the comment's claims about
net/http. Gates on 8dfd6ad3: go test ./... 0, go vet 0, gofmt clean, lint 0
issues; all 8 CI checks green.

internal/cli/client_stream_identity_test.go

@@ -95,7 +95,12 @@ func TestNewWatchEventsStreamRequest_UnsendableLabelStillConnects(t *testing.T)
 		t.Fatalf("build request: %v", err)
 	}
 
-	resp, err := http.DefaultClient.Do(req)
+	// srv.Client(), not http.DefaultClient: httptest.Server.Close() closes idle
+	// connections on the PROCESS-WIDE default transport (the standard library
+	// does it deliberately, calling it "not part of httptest.Server's
+	// correctness"), so with several parallel tests in this package any one of
+	// them finishing could break this request mid-flight. BUG-3008.
+	resp, err := srv.Client().Do(req)
 	if err != nil {
 		t.Fatalf("request was not sendable — the monitor would retry forever and deliver nothing: %v", err)
 	}
@@ -130,7 +135,8 @@ func TestNewWatchEventsStreamRequest_ArmedSendsQueryParam(t *testing.T) {
 	if err != nil {
 		t.Fatalf("build request: %v", err)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	// srv.Client() rather than http.DefaultClient, for the reason above (BUG-3008).
+	resp, err := srv.Client().Do(req)
 	if err != nil {
 		t.Fatalf("request not sendable: %v", err)
 	}

internal/server/handlers_collab_test.go

@@ -554,7 +554,7 @@ func TestCollabUpgradeRejectsSchemaVersionMismatch(t *testing.T) {
 	// Server's RoomManager is at "1" (DefaultSchemaVersion); send "9"
 	// to force a mismatch. We hit the HTTP path directly rather than
 	// through dialCollab so we can read the JSON error body.
-	resp, err := http.Get(ts.URL + "/api/v1/collab/" + itemID + "?schema_version=9")
+	resp, err := isolatedTestClient().Get(ts.URL + "/api/v1/collab/" + itemID + "?schema_version=9")
 	if err != nil {
 		t.Fatalf("http get: %v", err)
 	}
@@ -614,7 +614,7 @@ func TestCollabUpgradeMissingItemIDBadRequest(t *testing.T) {
 	ts := httptest.NewServer(srv)
 	defer ts.Close()
 
-	resp, err := http.Get(ts.URL + "/api/v1/collab/")
+	resp, err := isolatedTestClient().Get(ts.URL + "/api/v1/collab/")
 	if err != nil {
 		t.Fatalf("http get: %v", err)
 	}

internal/server/handlers_events_access_epoch_stream_test.go

@@ -90,7 +90,7 @@ func TestSSEStream_AnnouncesAnAccessChangeButStaysQuietOtherwise(t *testing.T) {
 	}
 	req.Header.Set("User-Agent", testSessionUA)
 	req.AddCookie(&http.Cookie{Name: sessionCookieName(srv.secureCookies), Value: token})
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("open stream: %v", err)
 	}

internal/server/handlers_events_cancel_test.go

@@ -91,7 +91,7 @@ func TestDisconnectDuringEstablishmentReleasesTheAdmissionSlot(t *testing.T) {
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := isolatedTestClient().Do(req)
 		if err == nil {
 			_ = resp.Body.Close()
 		}

internal/server/handlers_events_failed_test.go

@@ -59,7 +59,7 @@ func TestAFailedSubscriptionIsRefusedWithARetryableStatus(t *testing.T) {
 		if lastID != "" {
 			req.Header.Set("Last-Event-ID", lastID)
 		}
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := isolatedTestClient().Do(req)
 		if err != nil {
 			t.Fatalf("%s: GET: %v", name, err)
 		}

internal/server/handlers_events_resume_test.go

@@ -34,7 +34,9 @@ func readRawSSEFramesAuthed(t *testing.T, ctx context.Context, url, lastEventID,
 	if token != "" {
 		req.Header.Set("Authorization", "Bearer "+token)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	// isolatedTestClient(), not http.DefaultClient — same helper reasoning as
+	// connectSSE (BUG-3008).
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("connecting: %v", err)
 	}

internal/server/handlers_events_test.go

@@ -41,7 +41,10 @@ func connectSSE(ctx context.Context, t *testing.T, baseURL, workspaceSlug string
 		t.Fatalf("failed to create SSE request: %v", err)
 	}
 
-	resp, err := http.DefaultClient.Do(req)
+	// isolatedTestClient(), not http.DefaultClient: this helper's request lives
+	// for the whole test, and a caller may be parallel (BUG-3008). A helper cannot
+	// know which of its callers is exposed, so it does not try to.
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("failed to connect to SSE: %v", err)
 	}
@@ -102,7 +105,9 @@ func apiRequest(t *testing.T, baseURL, method, path string, body interface{}) *h
 	if body != nil {
 		req.Header.Set("Content-Type", "application/json")
 	}
-	resp, err := http.DefaultClient.Do(req)
+	// isolatedTestClient() for the reason on connectSSE above (BUG-3008): callers
+	// include parallel tests, and the helper cannot tell them apart.
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}
@@ -381,7 +386,7 @@ func TestSSEGlobalConnectionLimit(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -412,7 +417,7 @@ func TestSSEPerWorkspaceLimit(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -446,7 +451,7 @@ func TestSSELimitsExistingConnectionsUnaffected(t *testing.T) {
 
 	// Try (and fail) to get a second connection
 	req, _ := http.NewRequest("GET", ts.URL+"/api/v1/events?workspace="+slug, nil)
-	resp, _ := http.DefaultClient.Do(req)
+	resp, _ := isolatedTestClient().Do(req)
 	resp.Body.Close()
 
 	// The existing connection should still work — publish an event

internal/server/handlers_sessions_test.go

@@ -29,7 +29,7 @@ func getSessions(t *testing.T, baseURL, token string) (int, sessionsResponse) {
 	if token != "" {
 		req.Header.Set("Authorization", "Bearer "+token)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}
@@ -101,7 +101,7 @@ func TestListSessions_IsNotCacheable(t *testing.T) {
 
 	req, _ := http.NewRequest("GET", ts.URL+"/api/v1/sessions", nil)
 	req.Header.Set("Authorization", "Bearer "+tok.Token)
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}

internal/server/handlers_stream_gap_test.go

@@ -351,7 +351,7 @@ func TestRefusedConnectionDoesNotCountASyncRequired(t *testing.T) {
 		t.Fatalf("building the request: %v", err)
 	}
 	req.Header.Set("Last-Event-ID", "not-a-number")
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("connecting: %v", err)
 	}

internal/server/handlers_watch_events_cancel_test.go

@@ -86,7 +86,7 @@ func TestDisconnectDuringTheResumeSettleReleasesTheAdmissionSlot(t *testing.T) {
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := isolatedTestClient().Do(req)
 		if err == nil {
 			_ = resp.Body.Close()
 		}

internal/server/handlers_watch_events_test.go

@@ -81,7 +81,7 @@ func connectWatchStreamWithHeadersAndQuery(ctx context.Context, t *testing.T, ba
 		req.Header.Set(k, v)
 	}
 
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("failed to connect: %v", err)
 	}
@@ -156,7 +156,7 @@ func TestWatchEventsStream_RequiresAuth(t *testing.T) {
 	defer ts.Close()
 
 	req, _ := http.NewRequest("GET", ts.URL+"/api/v1/events/stream", nil)
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}

internal/server/isolated_client_test.go

@@ -0,0 +1,60 @@
+package server
+
+import "net/http"
+
+// isolatedTestClient returns an HTTP client with a transport of its own.
+// Outbound requests in this package's tests go through it — or through the
+// httptest server's own `ts.Client()` where the test holds the server value —
+// and never through `http.DefaultClient`, `http.Get`, or an `http.Client`
+// literal with a nil Transport, which is `http.DefaultTransport` (BUG-3008).
+//
+// WHY THIS EXISTS. `httptest.Server.Close()` reaches into the PROCESS-WIDE
+// default transport — the standard library says so in its own comment, calling
+// it "not part of httptest.Server's correctness" and doing it to help out
+// users who are on the standard transport:
+//
+//	if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
+//	    t.CloseIdleConnections()
+//	}
+//
+// So in a package where many tests each stand up an httptest server, every
+// `defer ts.Close()` mutates state that every other test's requests depend on.
+// What CI observed (BUG-2949, then BUG-3008) is a request in one test failing
+// with "transport connection broken: http: CloseIdleConnections called" while
+// the server that closed belonged to a different test entirely.
+//
+// WHAT IS AND IS NOT CLAIMED HERE. Read out of `net/http/transport.go`,
+// `Transport.CloseIdleConnections` closes each pooled HTTP/1 connection with
+// `errCloseIdleConns` — the error in that message; HTTP/2 connections go
+// through `h2transport.CloseIdleConnections()` separately. It sets
+// `closeIdle`, so a connection going idle afterwards is closed rather than
+// pooled when no request is waiting for one — a waiter is still handed it —
+// until a later `queueForIdleConn` clears the flag (which it does not reach
+// when `DisableKeepAlives` is set). It also cancels the dials in progress that
+// have a `cancelCtx` and are not waiting.
+//
+// Its doc comment states that it "does not interrupt any connections currently
+// in use", and the idle pool is mutated under `idleMu`, so the precise
+// interleaving by which that error reached a caller's `Do()` is NOT
+// reconstructed here.
+//
+// That unknown is the argument for isolation rather than a narrower repair: a
+// client with its own transport is outside the reach of another test's
+// teardown whichever window it was. Guessing at the window and fixing only
+// that would leave the next seat to rediscover this from a worse position.
+//
+// It is deliberately not a shared package-level var: two tests holding one
+// transport would reintroduce a smaller version of the same coupling.
+//
+// NOT A GENERAL RULE FOR EVERY PACKAGE. `internal/cli`'s tests drive the
+// PRODUCTION client from `NewClientFromURL`, which has a nil Transport by
+// design, so its sends cannot be isolated without changing what the shipped
+// CLI does. There the boundary really is parallel-vs-serial: the two
+// `t.Parallel()` tests that build their own requests use `srv.Client()`, and
+// the rest are serial, which a parallel test's teardown cannot overlap.
+func isolatedTestClient() *http.Client {
+	// No Timeout on purpose. These are SSE and long-poll requests; a timeout
+	// here would cancel the stream the test is measuring. A caller that wants
+	// one sets it on the returned client.
+	return &http.Client{Transport: &http.Transport{}}
+}

internal/server/listen_serve_test.go

@@ -42,7 +42,11 @@ func TestListenAndServe_StillServes(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	// The split must not have changed what a bound server does: it answers.
-	client := &http.Client{Timeout: 2 * time.Second}
+	// A nil Transport means http.DefaultTransport, which another test's
+	// httptest server teardown reaches into (BUG-3008). The timeout is the
+	// reason this is not a bare isolatedTestClient() call.
+	client := isolatedTestClient()
+	client.Timeout = 2 * time.Second
 	var resp *http.Response
 	for i := 0; i < 20; i++ {
 		resp, err = client.Get("http://" + ln.Addr().String() + "/api/v1/health")

internal/server/stream_admission_test.go

@@ -30,7 +30,7 @@ func rawWatchStreamStatus(t *testing.T, baseURL, token string) int {
 		t.Fatalf("build request: %v", err)
 	}
 	req.Header.Set("Authorization", "Bearer "+token)
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}
@@ -56,7 +56,7 @@ func holdAuthedSSE(ctx context.Context, t *testing.T, baseURL, slug, token strin
 		t.Fatalf("build request: %v", err)
 	}
 	req.Header.Set("Authorization", "Bearer "+token)
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}
@@ -459,7 +459,7 @@ func rawSSEStatus(t *testing.T, baseURL, slug string) int {
 	if err != nil {
 		t.Fatalf("build request: %v", err)
 	}
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := isolatedTestClient().Do(req)
 	if err != nil {
 		t.Fatalf("request failed: %v", err)
 	}
@@ -724,7 +724,7 @@ func TestStreamLimitRefusalContractIsIdenticalOnBothEndpoints(t *testing.T) {
 			t.Fatalf("%s: build request: %v", tc.name, err)
 		}
 		req.Header.Set("Authorization", "Bearer "+tc.token)
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := isolatedTestClient().Do(req)
 		if err != nil {
 			t.Fatalf("%s: request failed: %v", tc.name, err)
 		}