fix: isolate test HTTP clients#25038
Conversation
|
/coder-agents-review |
There was a problem hiding this comment.
The fix is correct: cloning the transport per coderdtest server eliminates the CloseIdleConnections race on http.DefaultTransport. The newHTTPClientWithTransportFrom factoring, sharing transport but not mutable client state, is clean. Tests verify the right structural invariants (pointer identity, TLS config, field isolation).
One P2, two P3, one P4, two nits, one note.
The P2 is scope: the same codersdk.New() + CloseIdleConnections pattern exists in at least 6 other test files (verified), and two independent workarounds already exist in the codebase with explanatory comments naming this exact root cause (mcpclient.go:265, mockmcp.go:59). A grep -rn 'CloseIdleConnections' --include='*.go' before scoping would have surfaced all siblings. Without a follow-up ticket, the class survives this fix.
The P3s are test gaps: the HTTPS test skips the NotSame(DefaultTransport) assertion that the HTTP test has, and the HTTP test has a dead assertion behind an always-false guard.
"What happens when the next flaky failure comes from these paths instead? The investigation starts over." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All seven R1 findings verified fixed. The class-of-bug sweep is thorough: every codersdk.New() + CloseIdleConnections site in test code now uses NewIsolatedHTTPClient or was already safe (custom transport). Tests are well-structured: direct helper tests (fast, no server) plus integration test for mutation isolation. Two minor P4s remain (both unreachable today).
Two P4 findings, both in unreachable defensive code paths.
"The class-of-bug sweep across six test files is thorough, and the remaining
cli/server_test.gosites were correctly excluded because they already use their own transports." (Kite)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All nine prior findings verified fixed across three rounds. The class-of-bug sweep is complete, test coverage is thorough, and the design is clean. One remaining nit: a test helper duplicates an existing utility.
R3 panel (6 reviewers): 5 found no new issues. Mafu-san noted that DEREM-8 (nil-guard fallback to DefaultTransport) was introduced in the same commit that fixed the same class of bug elsewhere (DEREM-5), a useful process signal but the code is correct now.
"The correction 'every fallback path must avoid http.DefaultTransport' was applied to one function but not generalized to new code written in the same change. This is the gap between fixing an instance and internalizing a principle." (Mafu-san)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 10 prior findings verified fixed. One new P4 in the same class as DEREM-5/8: newHTTPClientWithTransportFrom guards base == nil but not base.Transport == nil. Unreachable today (all callers pass coderdtest clients with isolated transports), but the function's purpose is isolation and it already has one nil guard.
R4 panel (5 reviewers): 3 found no new issues. Hisoka found the nil-Transport edge case. Mafu-san raised useful process observations about the fix progression across rounds (corrections not generalized to sibling functions in the same commit), but the code is correct now.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 11 prior findings verified fixed. One new P3: the CheckRedirect isolation assertion in TestCreateAnotherUserHTTPClient is tautological because it sets CheckRedirect on the parent after CreateAnotherUser returns. Since other.HTTPClient is a separate struct, the field is always nil regardless of what the constructor does.
R5 panel (5 reviewers): 4 found no new issues. Netero clean.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 12 findings across 6 rounds are now fixed. Zero new code findings from any reviewer or Netero. The PR is clean.
12 findings raised and resolved over 6 rounds: 1 P2 (class-of-bug scope), 3 P3 (test gaps, tautological assertion), 4 P4 (unreachable nil-guard fallbacks, untested branch), 2 Nit (naming, duplication), 1 Note (redundant clone), 1 P3 (test ordering). All addressed.
🤖 This review was automatically generated with Coder Agents.
Parallel tests could share
http.DefaultTransportthrough ad hoccodersdk.Newclients. When one test cleaned up withCloseIdleConnections, another in-flight request could fail withhttp: CloseIdleConnections called, as seen in ENG-2179.Add a coderdtest helper for isolated test HTTP clients, use it in coderdtest and sibling test sites that close idle SDK connections, and keep
CreateAnotherUserfrom sharing mutablehttp.Clientfields such asCheckRedirect.Closes coder/internal#1395
Coder Agents notes
Generated by Coder Agents for Linear ENG-2179.
Local validation run before opening and after review follow-up:
go test ./coderd/coderdtestgo test ./coderd -run '^TestOAuth2ProviderResourceIndicators$' -count=20 -parallel=16go test ./coderd -run '^TestOIDCOauthLoginWithExisting$'go test ./coderd/coderdtest ./coderd ./cli ./enterprise/cli -run 'TestNewIsolatedHTTPClient|TestNewIsolatedHTTPSClient|TestCreateAnotherUserHTTPClient|TestDERPForceWebSockets|TestCompositeWorkspaceScopes|TestOAuth2ProviderResourceIndicators|TestDERPHeaders|TestWorkspaceAgent/Headers&DERPHeaders|TestTemplateEdit/AllowAdvancedScheduling|TestTemplateEdit/AllowUserScheduling|TestBoundary' -count=1make lint