From 147ccf66282495a7f5be2d387f19d85aa16b9a73 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Fri, 17 Jul 2026 10:07:57 +0200 Subject: [PATCH] fix(mcp): give allow-private-ips OAuth clients a private connection pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit oauthHTTPClientForAllowPrivateIPs(true) returned a client with a nil Transport, i.e. http.DefaultTransport. Anything pruning that shared pool via CloseIdleConnections — notably httptest.Server.Close, run by every finishing parallel test — could break another client's in-flight request ("transport connection broken: http: CloseIdleConnections called"), the same flake f7b5c105f fixed for the transports' base RoundTripper. Clone the default transport instead: same proxy/HTTP2/timeout behavior, private pool. TestMain's swapped-in client and the remaining nil-Transport test clients now get private pools too. Assisted-By: Claude --- pkg/tools/mcp/main_test.go | 10 +++++----- pkg/tools/mcp/oauth_helpers.go | 9 ++++++++- pkg/tools/mcp/oauth_server_test.go | 2 +- pkg/tools/mcp/oauth_test.go | 2 +- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/pkg/tools/mcp/main_test.go b/pkg/tools/mcp/main_test.go index 034d3e4ae7..b72a703395 100644 --- a/pkg/tools/mcp/main_test.go +++ b/pkg/tools/mcp/main_test.go @@ -3,15 +3,15 @@ package mcp import ( "os" "testing" - "time" - - "github.com/docker/docker-agent/pkg/httpclient" ) // TestMain swaps the OAuth helpers' SSRF-safe HTTP client for the // loopback-allowing variant so tests can hit httptest.NewServer (which -// binds to 127.0.0.1). Production code keeps the safe client. +// binds to 127.0.0.1). Production code keeps the safe client. The variant +// carries its own connection pool, so parallel tests closing httptest +// servers (which prunes http.DefaultTransport's pool) can't break its +// in-flight requests. func TestMain(m *testing.M) { - oauthHTTPClient = httpclient.NewSafeClient(30*time.Second, true) + oauthHTTPClient = oauthHTTPClientForAllowPrivateIPs(true) os.Exit(m.Run()) } diff --git a/pkg/tools/mcp/oauth_helpers.go b/pkg/tools/mcp/oauth_helpers.go index 31fb0f2136..90cd8d8324 100644 --- a/pkg/tools/mcp/oauth_helpers.go +++ b/pkg/tools/mcp/oauth_helpers.go @@ -37,7 +37,14 @@ var oauthHTTPClient = httpclient.NewSafeClient(30*time.Second, false) func oauthHTTPClientForAllowPrivateIPs(allowPrivateIPs bool) *http.Client { if allowPrivateIPs { - return &http.Client{Timeout: 30 * time.Second} + // Clone keeps the default proxy/HTTP2/timeout behavior but gives the + // client its own connection pool: a nil Transport would share + // http.DefaultTransport's pool, which third parties may prune via + // CloseIdleConnections (httptest.Server.Close does). + return &http.Client{ + Timeout: 30 * time.Second, + Transport: http.DefaultTransport.(*http.Transport).Clone(), + } } return oauthHTTPClient } diff --git a/pkg/tools/mcp/oauth_server_test.go b/pkg/tools/mcp/oauth_server_test.go index 8f662d9f56..0b7427fd11 100644 --- a/pkg/tools/mcp/oauth_server_test.go +++ b/pkg/tools/mcp/oauth_server_test.go @@ -99,7 +99,7 @@ func TestCallbackServer_DuplicateCallbacksDoNotBlock(t *testing.T) { cs.SetExpectedState("expected-state") callbackURL := cs.GetRedirectURI() + "?code=authcode&state=expected-state" - client := &http.Client{Timeout: 2 * time.Second} + client := &http.Client{Timeout: 2 * time.Second, Transport: newTestTransport(t)} // Fire several callbacks back-to-back. Each one must complete (so the // handler goroutine isn't stuck) regardless of whether anyone is diff --git a/pkg/tools/mcp/oauth_test.go b/pkg/tools/mcp/oauth_test.go index 5f42056efe..5af3e00e25 100644 --- a/pkg/tools/mcp/oauth_test.go +++ b/pkg/tools/mcp/oauth_test.go @@ -28,7 +28,7 @@ import ( // test's in-flight request ("transport connection broken"). func newTestTransport(t *testing.T) *http.Transport { t.Helper() - tr := &http.Transport{} + tr := http.DefaultTransport.(*http.Transport).Clone() t.Cleanup(tr.CloseIdleConnections) return tr }