fix(transport): Track connections from a custom DialTLSContext - #36
Merged
Conversation
net/http dials https through DialTLSContext (or DialTLS) when the caller sets one, bypassing the factory's DialContext wrapper entirely. Those connections were never registered with ownedTransport, so closeAll fell back to CloseIdleConnections, which skips an HTTP/2 connection still winding down a cancelled stream. On a transport with IdleConnTimeout 0 the connection, its readLoop, and the orphaned clone lived until the server hung up: a 40-run soak grew by 111 goroutines. The custom dialer is now wrapped on each clone and its connections tracked unwrapped: net/http only upgrades to HTTP/2 when the dialled value is exactly a *tls.Conn, so a wrapper would silently drop to HTTP/1.1. The address is not rewritten for test_endpoint, because the caller's dialer would verify the certificate against it; the run warns instead of silently ignoring the override (DISC-9). TestNoLeaksAcrossRuns now runs with both dialer kinds. Related: INV-4, DISC-6, DISC-7, DISC-9 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dM7M83LicKy8psVSvjod2
TestNoLeaksAcrossRuns checks the instant Run returns; this checks the long run an embedding agent actually lives in. Three scenarios (completed runs, runs cancelled mid-load, a custom TLS dialer with no idle timeout) loop Run forty times after a warm-up and assert that the goroutine count and post-GC HeapInuse plateau. Skipped under -short. Related: INV-4 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dM7M83LicKy8psVSvjod2
The DialTLSContext case of TestNoLeaksAcrossRuns failed on the minimum-Go job and reproduces on any loaded machine: one socket is still open at the instant Run returns. net/http dials in a goroutine that outlives the cancelled request that started it (startDialConnForLocked -> dialConnFor), so when a phase ends that goroutine can still be inside the caller's dialer. With a custom TLS dialer that window spans the whole handshake rather than a loopback TCP connect, which is why only the new case failed. The socket belongs to the caller's dialer at that moment: the library has not been handed it and cannot close it. The dial context is cancelled with the phase, so the dialer aborts its handshake and closes the socket immediately after. What the library owes is that such a connection dies promptly, not that it was never opened, so that case now polls to zero instead of demanding it at the instant of return. The DialContext case keeps the instant check. ownedTransport also latches closed at teardown, so a dial that does complete after closeAll took its snapshot is closed on arrival rather than filed in a map nobody reads again. This was never observed firing in 72 contended runs; it closes the hole for six lines. Related: INV-4 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dM7M83LicKy8psVSvjod2
TestSoak/default failed on the minimum-Go job at goroutines 4 -> 10, and the printed trend shows why it is not a leak: 10, 10, 7, 4 over the run, with no ramp. The count is process-wide, so it includes the in-process test server's per-connection goroutines. The client aborts its load flows, which is an abortive close, so the server side unwinds on the kernel's schedule rather than ours — INV-4 is a promise about the client's own sockets. A single sample 50ms after the last run catches that transient on a loaded runner. The final check now polls for the plateau, as TestNoLeaksAcrossRuns already does, and tightens the bound it settles to: +2 goroutines and +8MiB rather than +5 and +4MiB. Verified it still catches what it exists for — with the DialTLSContext tracking disabled the same test reports goroutines 16 -> 184. Related: INV-4 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dM7M83LicKy8psVSvjod2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A binary embedding the library leaked a connection per load flow, for ever, if its HTTP client used a custom TLS dialer.
net/httpdials https throughDialTLSContext(orDialTLS) whenever the caller's transport sets one, bypassing the factory'sDialContextwrapper entirely. Those connections were never registered withownedTransport, so teardown fell back toCloseIdleConnections— which by design skips an HTTP/2 connection still winding down a cancelled stream. With the default 90sIdleConnTimeoutthe connection eventually expires; on a hand-built&http.Transport{}(IdleConnTimeout0) it never does, and the connection, itsreadLoop, and the orphaned transport clone survive until the server hangs up. A 40-run soak grew by 111 goroutines and 6 MiB. This breaks INV-4 for the one caller shape most likely to appear in a long-lived agent.Solution
The custom TLS dialer is wrapped on each per-flow clone, so its connections are tracked and torn down with the run.
They are tracked unwrapped:
net/httponly upgrades to HTTP/2 when the dialled value is exactly a*tls.Conn, so a wrapper would silently drop every flow to HTTP/1.1 — a test asserts the negotiated protocol survives tracking. The dial address is not rewritten fortest_endpoint, because the caller's dialer would then verify the certificate against the rewritten address; that limitation was already the behaviour and is now spelled out as DISC-9 and warned about at runtime, alongside the existing DISC-7 proxy warning.TestNoLeaksAcrossRunsruns with both dialer kinds. A newTestSoakcovers what the instant-of-return check cannot: forty runs per scenario (completed, cancelled mid-load, custom TLS dialer with no idle timeout) asserting the goroutine count and post-GCHeapInuseplateau. It is skipped under-short. Before the fix the custom-dialer scenario failed; after, all three are flat.Other Changes
The new INV-4 case failed on the minimum-Go job, and reproduces on any loaded machine.
net/httpdials in a goroutine that outlives the cancelled request that started it, so when a phase ends that goroutine can still be inside the caller's dialer — a socket the library has not been handed and cannot close. With a custom TLS dialer that window spans the whole handshake rather than a loopback TCP connect, which is why only the new case failed. The dial context is cancelled with the phase, so the dialer aborts its handshake and closes the socket immediately after; the check for that case now polls to zero rather than demanding it at the instant of return, and theDialContextcase keeps the instant check.ownedTransportadditionally latches closed at teardown so a dial completing after the snapshot is closed on arrival. That was never observed firing in 72 contended runs — it is six lines against a hole, not a fix for anything seen.Needs a decision before merge: INV-4 reads "when
Runreturns, all goroutines, connections, and timers it created are gone." A socket the caller's own dialer is still handshaking is a documented exception to that sentence. Shoulddocs/product-specs/invariants.mdsay so, or is the test comment enough? I have not edited the spec.Related: INV-4, DISC-6, DISC-7, DISC-9
🤖 Generated with Claude Code
https://claude.ai/code/session_017dM7M83LicKy8psVSvjod2