Skip to content

fix: isolate test HTTP clients#25038

Merged
ThomasK33 merged 6 commits into
mainfrom
fix/eng-2179-coderdtest-http-client
May 11, 2026
Merged

fix: isolate test HTTP clients#25038
ThomasK33 merged 6 commits into
mainfrom
fix/eng-2179-coderdtest-http-client

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented May 7, 2026

Copy link
Copy Markdown
Member

Parallel tests could share http.DefaultTransport through ad hoc codersdk.New clients. When one test cleaned up with CloseIdleConnections, another in-flight request could fail with http: 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 CreateAnotherUser from sharing mutable http.Client fields such as CheckRedirect.

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/coderdtest
  • go test ./coderd -run '^TestOAuth2ProviderResourceIndicators$' -count=20 -parallel=16
  • go 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=1
  • make lint

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread coderd/coderdtest/coderdtest.go Outdated
Comment thread coderd/coderdtest/httpclient_test.go Outdated
Comment thread coderd/coderdtest/httpclient_test.go Outdated
Comment thread coderd/coderdtest/coderdtest.go Outdated
Comment thread coderd/coderdtest/coderdtest.go Outdated
Comment thread coderd/coderdtest/httpclient_test.go Outdated
Comment thread coderd/coderdtest/coderdtest.go Outdated
@ThomasK33 ThomasK33 changed the title fix(coderd/coderdtest): isolate test HTTP clients fix: isolate test HTTP clients May 7, 2026

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go sites were correctly excluded because they already use their own transports." (Kite)

🤖 This review was automatically generated with Coder Agents.

Comment thread coderd/coderdtest/coderdtest.go Outdated
Comment thread coderd/coderdtest/httpclient_test.go

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread coderd/coderdtest/httpclient_test.go Outdated

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread coderd/coderdtest/coderdtest.go

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread coderd/coderdtest/httpclient_test.go Outdated

Copy link
Copy Markdown
Member Author

/coder-agents-review

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ThomasK33
ThomasK33 marked this pull request as ready for review May 7, 2026 17:12
@ThomasK33
ThomasK33 merged commit 4a6756a into main May 11, 2026
31 checks passed
@ThomasK33
ThomasK33 deleted the fix/eng-2179-coderdtest-http-client branch May 11, 2026 09:03
@github-actions github-actions Bot locked and limited conversation to collaborators May 11, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flake: TestOAuth2ProviderResourceIndicators/ResourceInAuthOnly

2 participants