Bound and close HTTP client connection pools

[tgrunnagle]

Summary

Every *http.Client produced by networking.HttpClientBuilder.Build() had a connection pool that could never be drained. IdleConnTimeout was left at its zero value, which in http.Transport means never expire, so a client that performed a single request and was then dropped pinned a socket and its readLoop/writeLoop goroutine pair for the lifetime of the process. The obvious remediation was already unavailable and silently so: Build always wraps the pool in a ValidatingTransport (and, on the authTokenFile path, an oauth2.Transport), neither of which implements CloseIdleConnections, and http.Client.CloseIdleConnections is a type assertion on the outermost transport — so the call returned having done nothing, with no error and no log.

On the auth-server upstream path this compounded into a real leak. authserver.New builds a private HTTP client per configured upstream with keep-alives deliberately on, server.Close() closed storage only, and nothing walked s.upstreams. An embedder that reconstructs the server to change its upstream set — a pattern the API otherwise supports, since New accepts an existing storage.Storage and KeyProvider — leaked one transport per OIDC upstream per reconstruction, unbounded, with no correct call available to reclaim them: Close() would also close the storage the new server was now serving through.

This lands all three changes proposed in the issue:

• Bound the pool in networking.Build — finite IdleConnTimeout, plus MaxIdleConns and MaxIdleConnsPerHost. This alone converts every leak of this shape in the repo from unbounded to self-draining and makes an abandoned transport garbage-collectable.
• Make the pool reachable — CloseIdleConnections forwarders on the transport wrappers, on BaseOAuth2Provider (promoted to OIDCProviderImpl), and a new package-level authserver.CloseIdleConnections(Server) bool kept deliberately distinct from Close(). The split is the load-bearing part for embedders: it is what lets a superseded server release its own upstream connections without touching shared storage. It is a function rather than a Server method so that Server stays source-compatible — see the user-facing-change section.
• Export the upstream provider factory as Config.UpstreamFactory, with DefaultUpstreamFactory exported so a custom factory can delegate. This makes reconstruction structurally free (share a client per issuer host via the already-public WithHTTPClient / WithOAuth2HTTPClient) and makes per-upstream failure isolation possible.

What this closes, and what it does not. #6479 is scoped to HTTP client pools — "HTTP client pools are unbounded and uncloseable" — and all three of its proposed changes land here. Closing it is therefore accurate for its stated scope.

One adjacent resource is deliberately left open and tracked separately: a TrustedIssuers server starts one JWKS refresh worker pool per issuer rooted at context.Background(), released by neither Close nor CloseIdleConnections, so a process that reconstructs the auth server repeatedly still grows goroutines through that path. That is a different resource than the connection pools this issue is about, it is pre-existing, and it is now tracked in #6482 and documented in the Scope: paragraph on Server.CloseIdleConnections so the remaining gap is not silent. #6483 tracks the hand-rolled transports outside networking.Build that share the original defect.

Scope decision. The issue says of its item 3: "This one is a genuine API surface commitment ... so it deserves its own discussion and shouldn't block 1 and 2." This PR lands it anyway, as an explicit choice rather than an oversight: it is the piece that makes reconstruction structurally free rather than merely bounded, which is the scenario the issue is about, so splitting it would leave the headline case only half-addressed across two PRs. It is separable — items 1+2 are a self-contained ~200-line fix — so if a reviewer would rather decide the API question on its own, say so and I will split it out. Note that Config.UpstreamFactory currently has no in-repo caller.

Closes #6479

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test) — full unit suite passes
• E2E tests (task test-e2e)
• Linting (task lint-fix) — 0 issues; task license-check also passes
• Manual testing (describe below)

New tests: TestBuild_BoundsIdleConnectionPool and TestBuild_CloseIdleConnectionsReachesPool (regression guards against the two builder defects, the latter asserting the call reaches the real pool through the wrappers); TestServer_CloseIdleConnections, TestServer_Close_DrainsRealOIDCUpstream (end-to-end against an httptest OIDC issuer — the actual guard on the promoted method), TestNewServer_UpstreamFactoryPrecedence, TestBuildUpstreams_DrainsAlreadyBuiltProvidersOnFailure, TestBuildUpstreams_RejectsNilProvider; TestBaseOAuth2Provider_CloseIdleConnections, TestBaseOAuth2Provider_CloseIdleConnections_InjectedClientNotDrained (ownership), TestNewOIDCProvider_DrainsOwnClientOnFailure.

Changes

File	Change
pkg/networking/http_client.go	Set IdleConnTimeout (90s), MaxIdleConns (100), MaxIdleConnsPerHost (4) in Build. Add (*ValidatingTransport).CloseIdleConnections; add closeIdlerTransport to carry the capability past oauth2.Transport on the authTokenFile path.
pkg/authserver/server.go	Add the package-level CloseIdleConnections(Server) bool and the unexported capability it detects, documenting the Close/CloseIdleConnections split and its scope. Server itself is unchanged. Add the exported UpstreamProviderFactory type.
pkg/authserver/config.go	Add Config.UpstreamFactory with the connection-reuse and failure-isolation rationale and a SECURITY note (the provider validates upstream ID tokens).
pkg/authserver/server_impl.go	Export DefaultUpstreamFactory; extract buildUpstreams / closeUpstreamIdleConnections / resolveUpstreamFactory; drain already-built providers when construction aborts and when any later newServer step fails; Close now drains before closing storage. Reject a nil provider from a custom factory, and name the offending upstream in the error.
pkg/authserver/upstream/oauth2.go	Add the optional IdleConnectionCloser interface and (*BaseOAuth2Provider).CloseIdleConnections. Track ownsHTTPClient so an injected client is never drained. Apply options before building the default client, and warn when a configured CAFilePath is superseded by an injected client.
pkg/authserver/upstream/oidc.go	Same option-ordering and ownership handling; drain the provider's own client if construction fails after discovery. Compile-time capability assertion on *OIDCProviderImpl.
pkg/authserver/server/tokenexchange/multi_issuer_validator.go	CloseIdleConnections forwarder on limitedBodyTransport so the wrapper does not silently swallow the call (no pool today — the client disables keep-alives — but keeps that an optimization, not a correctness dependency).
pkg/authserver/runner/embeddedauthserver.go	Forward CloseIdleConnections to the embedded server via authserver.CloseIdleConnections.
pkg/authserver/upstream/mocks/mock_provider.go	Regenerated: adds MockIdleConnectionCloser.
pkg/authserver/upstream/doc.go	Document IdleConnectionCloser as an optional capability for external OAuth2Provider implementations.

Does this introduce a user-facing change?

Yes, two — both additive. No interface is widened and nothing that compiles today stops compiling, other than case 2 below.

An earlier revision of this PR added CloseIdleConnections() as a required method on the exported authserver.Server interface, which would have been a source-incompatible change for any out-of-tree implementation or test double. That is gone. The drain is now a package-level authserver.CloseIdleConnections(Server) bool, which detects an unexported capability interface — the same shape upstream.IdleConnectionCloser already uses one layer down, so both layers now use capability detection for the same reason and the asymmetry that previously needed explaining is gone.

The compile-time safety that motivated requiring the method is preserved by var _ idleConnectionCloser = (*server)(nil): a server built by New can never silently no-op. The bool return covers the remaining case, so a caller holding an implementation that predates the capability learns it did nothing instead of assuming success. Both new tests assign a capability-free Server implementation to the interface, which is a compile-time proof of the source compatibility claim.

1. Behavioral change in retained connections. MaxIdleConnsPerHost goes from Go's default of 2 to 4 for every client produced by networking.HttpClientBuilder.Build. Unlike IdleConnTimeout and MaxIdleConns (whose zero values are "unlimited", so setting them only imposes bounds), the per-host zero value was already bounded at http.DefaultMaxIdleConnsPerHost — so this is an increase, not a newly imposed cap. A client may now retain up to 4 idle connections per host instead of 2. The trade is against the now-finite 90s IdleConnTimeout: most clients this builder produces are host-scoped and drive a single upstream, so a slightly larger per-host pool serves concurrent requests without dialing, and retention is now time-bounded where before it was forever.

2. A CA bundle configured alongside an injected HTTP client is now an error. Options are applied before the default client is built, so newHTTPClientForHost — and with it the read and parse of config.CAFilePath — is skipped when a caller injects a client via upstream.WithHTTPClient / WithOAuth2HTTPClient. Rather than let a trust-anchor decision be silently dropped, both provider constructors now return upstream.ErrCABundleWithInjectedClient for that combination. An external embedder that today sets both will start failing at construction; the combination was already meaningless (the injected client carries its own TLS trust, and the CA-configured client was built and then discarded), so this converts a silent no-op into a boot-time error. No in-repo caller injects a client.

Special notes for reviewers

Two follow-ups are deliberately out of scope for this PR:

1. Bound the hand-rolled http.Transport pools outside networking.Build #6483 — hand-rolled transports outside networking.Build. Roughly five &http.Transport{} literals elsewhere in the tree — in pkg/auth/discovery, pkg/oauthproto, and pkg/auth/oauth/oidc.go — share the same zero-IdleConnTimeout defect, and four more wrappers (bearerTokenTransport, oauthproto.UserAgentTransport, auth.WrapTransport, pkg/authz/authorizers/http) still swallow CloseIdleConnections on a builder-built client. The right long-term answer is routing them through the builder, which is a larger refactor than this PR should carry.

2. Release per-issuer JWKS refresh workers on auth-server shutdown #6482 — per-issuer JWKS refresh workers. A server configured with TrustedIssuers starts one JWKS refresh worker pool per issuer in MultiIssuerTokenValidator, against context.Background(). These are released by neither Close nor CloseIdleConnections, so an embedder that reconstructs repeatedly still grows goroutines through that path. Documented in the Scope paragraph on Server.CloseIdleConnections so the remaining gap is not silent.

Two points worth extra scrutiny:

• Option-ordering change in both provider constructors. Options are now applied before the default HTTP client is built, so an injected client no longer costs a discarded transport and a CA-bundle read per construction. The side effect is that a configured CAFilePath would no longer be read or validated in that case, so the combination is rejected outright (see user-facing change 2). CAFilePath is the only field needing this treatment: AllowPrivateIPs was already inert with an injected client, and InsecureAllowHTTP still gates config-level scheme validation via ValidateWithInsecure. What the injected client does take over is request-time scheme enforcement and dial-time private-IP blocking, which the option docs now say explicitly.
• Ownership semantics. CloseIdleConnections is a no-op on a caller-injected client, since the caller may be sharing it across providers and draining it would cold-start connections a live provider is using. This is what makes the Config.UpstreamFactory connection-sharing pattern safe, and it is covered by a dedicated test.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 96.80851% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.17%. Comparing base (1af4f80) to head (8badc75).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/upstream/oidc.go	86.66%	2 Missing ⚠️
pkg/authserver/server_impl.go	96.96%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6480      +/-   ##
==========================================
+ Coverage   78.09%   78.17%   +0.07%     
==========================================
  Files         768      768              
  Lines       74551    74638      +87     
==========================================
+ Hits        58220    58347     +127     
+ Misses      16326    16286      -40     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-agent review — 5 specialists (concurrency/lifecycle, security, API design, test coverage, quality/reviewability)

The core fix is correct and well-targeted. Verified independently: closeIdlerTransport reaches the same *http.Transport captured before wrapping; ownsHTTPClient is construction-only write / drain-only read, so no race and a safe zero value; newServer's retErr defer registers strictly after buildUpstreams returns, so no double-drain; Close() drains before closing storage; the pool bounds (90s / 100 / 4) are safe for all ~12 Build() call sites — every one is a single-host control-plane client, none on the MCP data path, and 4 raises the previous effective per-host limit of 2. task gen produces no diff. 9 of the new tests were verified load-bearing by reverting the corresponding production code.

Three things I would want resolved before merge, none of them a defect in the shipped code path:

1. Closes #6479 looks premature. The issue's failure mode is a long-lived process that reconstructs the auth server repeatedly. That still grows goroutines unboundedly through jwk.NewCache(context.Background(), ...) in multi_issuer_validator.go — one worker pool per trusted issuer, rooted at context.Background(), released by neither Close() nor CloseIdleConnections(). This construction happens after buildUpstreams, so the pathological retry loop the new defer targets still leaks a worker pool per attempt once construction gets past the upstream stage. The PR documents this honestly inside the Server.CloseIdleConnections doc comment, which makes it a scoped partial fix rather than an oversight — but the connection half is fixed and the goroutine half is not. Either open the follow-up for shutting down MultiIssuerTokenValidator and reference it, or downgrade to Refs #6479.

2. An injected HTTP client now fails open on CAFilePath. Moving option application ahead of the default-client build means newHTTPClientForHost is skipped for an injecting caller, and with it the read+parse of config.CAFilePath — which previously returned an error on an unreadable or malformed bundle. Now it is a slog.Warn. The effective TLS posture is unchanged (the CA-configured client was already built-then-discarded), and no in-repo caller injects a client, so this is confined to external Go embedders — but it turns a boot-time failure on a trust-anchor decision into a log line, and Config.UpstreamFactory exists precisely to encourage injected clients. Three of five reviewers landed on the same recommendation: treat "injected client + non-empty CAFilePath" as a configuration contradiction and return an error. It is also untested in either direction.

3. Scope. #6479 says of its item 3: "This one is a genuine API surface commitment ... so it deserves its own discussion and shouldn't block 1 and 2." This PR lands all three at 384 added production lines across 9 files — right at the repo's 400-line / 10-file guidance — and item 3 consumes a large share of the reviewer's attention: Config.UpstreamFactory, UpstreamProviderFactory, the DefaultUpstreamFactory export, resolveUpstreamFactory, the nil-provider policy, the constructor reordering, and warnInjectedClientSupersedesCABundle. Items 1+2 alone are a tight ~200-line fix that could land today; item 3 is a policy question (should the component that validates upstream ID tokens be caller-substitutable?) with no in-repo caller. Splitting is a judgment call, but if it stays as one PR that should be a stated decision in the body rather than silent.

Also worth noting

• Breaking change not declared. authserver.Server gains a required method. The single-implementation claim is accurate for production code (only pkg/authserver.server), but this is a compile break for any external implementer or test fake — the PR itself had to update stubServer. Belongs in the "user-facing change" section, along with the CAFilePath behavior change, so both reach the release notes.
• Comment volume. 215 of the 384 added production lines are comments (56%, against a 25–46% baseline in the same files). The optional-capability rationale is restated in 4 places and the injected-client ownership rule in 7; the Scope: paragraph documents an unrelated unfixed leak inside a public method's contract. Stating each rationale once at its natural home and pointing at it elsewhere would cut the diff meaningfully without losing anything.
• Commit history. "Address code review feedback" ×3 are not cosmetic — they add ownership tracking, nil-provider rejection, the constructor reordering, the CA warning, and the failure-path drains, i.e. behavior that changes the first commit's design, with severity labels from review output in the bodies. Worth rebasing into logical commits before merge.
• Coverage gaps. newServer's deferred drain is uncovered (removing it leaves the suite green); warnInjectedClientSupersedesCABundle, limitedBodyTransport.CloseIdleConnections, EmbeddedAuthServer.CloseIdleConnections delegation, and the headline Config.UpstreamFactory shared-client contract are all untested.

No HIGH-severity findings. Nothing here says the code is wrong; the substantive asks are the issue-closure scope, the fail-open CA path, and declaring the breaking change.

🤖 Multi-agent review generated with Claude Code

[jhrozek]

Approved.

Non-blocking completeness note: adding CloseIdleConnections() to the exported authserver.Server interface (pkg/authserver/server.go:62) is a source-incompatible API expansion for out-of-tree implementations and test doubles. We control the current consumers, so this is acceptable for this PR, but it is worth recording as an intentional public API compatibility trade-off.

[tgrunnagle]

Thanks for the approval — but on reflection I'd rather not spend the compatibility break, so I've removed it in 22b52be. authserver.Server is unchanged again.

What replaced it. A package-level function that detects an unexported capability:

type idleConnectionCloser interface{ CloseIdleConnections() }

func CloseIdleConnections(s Server) bool {
	closer, ok := s.(idleConnectionCloser)
	if ok {
		closer.CloseIdleConnections()
	}
	return ok
}

plus var _ idleConnectionCloser = (*server)(nil) in server_impl.go.

Why this is not a downgrade. The reason an earlier review round argued for a required method was compile-time safety — "the call cannot silently degrade to a no-op". That is preserved: the var _ check means a server built by New is guaranteed at compile time to satisfy the capability, and the bool return means a caller holding a foreign implementation learns it did nothing rather than assuming success. So it is source-compatible and no weaker on the property that motivated the original shape.

What tipped it. The interface method bought exactly one in-tree call site — EmbeddedAuthServer, whose field is typed as the interface. Everything else already calls the concrete type. One internal type assertion is a poor return for breaking out-of-tree implementations and test fakes, even at v0.x where we make no semver promise.

Two side benefits: the public API delta is now +1 function instead of +1 required interface method, and both layers use capability detection for the same reason, so the Server-vs-OAuth2Provider asymmetry a previous round asked me to justify is simply gone.

Coverage. TestCloseIdleConnectionsFunction asserts it drains a server from New and reports true, and reports false without panicking for an implementation lacking the capability. TestCloseIdleConnectionsWithoutCapability pins that the runner wrapper degrades quietly and specifically does not fall back to Close — which would tear down storage, the exact thing the split exists to avoid. Both tests assign a capability-free type to Server, so the source-compatibility claim is checked by the compiler rather than asserted in prose.

I kept the interface unexported on purpose: exporting authserver.IdleConnectionCloser would make three types with that name (networking, upstream, authserver), and an earlier round flagged naming the concept twice as a discoverability problem.

PR body updated — the compile break is no longer listed as a user-facing change. task lint-fix clean, full task test passing.