Conversation
|
[automated reply by timer's agent] The Go and E2E workflow runs report The focused concurrency tests pass 30 shuffled runs with the race detector. The full local suite has the macOS certificate assertion failure described above, which also fails on untouched upstream code. No test or CI setting was disabled. |
There was a problem hiding this comment.
🟡 Changes recommended
The new test file contains compilation errors (e.g., sync.WaitGroup.Go usage and an invalid pointer slice literal) that must be fixed before the PR can be safely validated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an isolated retryablehttp.Client factory option so each SDK client instance (including token refresh and message sessions) gets its own retryable client, http.Client, and *http.Transport, avoiding shared-client races and shared TLS configuration mutation.
Changes:
- Adds
WithRetryableHTTPClientFactoryand updates client construction to prefer the factory when provided. - Deprecates
WithRetryableHTTPClintand clarifies precedence rules between legacy custom client vs. factory. - Clones transport/TLS-related state (TLS config, certificate slice header, protocol handler map, and
NextProtos) before applying SDK options.
File summaries
| File | Description |
|---|---|
| http_client_factory_test.go | Adds coverage for factory isolation, precedence, token-refresh concurrency, and TLS/protocol handler copying. |
| common_client.go | Implements the factory option, precedence with legacy option, and transport/TLS copying to reduce cross-client sharing and races. |
Review details
Suppressed comments (4)
http_client_factory_test.go:220
sync.WaitGroupdoes not have aGomethod, so this goroutine setup will not compile. UseAdd(1)anddefer Done()around a normalgo func()instead.
workers.Go(func() {
for range sessions {
// The HTTP transport clones the TLS config before a handshake.
assert.False(t, parentConfig.Clone().InsecureSkipVerify)
}
})
http_client_factory_test.go:247
sync.WaitGroupdoes not have aGomethod, so this test will not compile. UseAdd+go+Done(and capture the loop index) when spawning the goroutines.
var workers sync.WaitGroup
for i := range sessions {
workers.Go(func() {
// MessageSessionClient copies the parent options before overrides.
children[i] = opts
WithTLSClientCertificate(tls.Certificate{Certificate: [][]byte{{byte(i)}}})(&children[i])
})
}
http_client_factory_test.go:274
sync.WaitGroupdoes not have aGomethod, so this will not compile. UseAdd+go+Donewhen runningCloseIdleConnectionsconcurrently.
var workers sync.WaitGroup
for range 16 {
client, err := opts.newRetryableHTTPClient()
require.NoError(t, err)
workers.Go(client.HTTPClient.CloseIdleConnections)
}
http_client_factory_test.go:203
sync.WaitGroupdoes not have aGomethod, so this loop will not compile. Switch toAdd+go+Doneand captureito avoid closure issues.
var workers sync.WaitGroup
for i := range sessions {
workers.Go(func() {
childOpts := opts
WithoutTLSVerify()(&childOpts)
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| func TestRetryableHTTPClientFactoryRejectsNil(t *testing.T) { | ||
| for _, client := range []*retryablehttp.Client{nil, {}} { |
There was a problem hiding this comment.
[automated reply by timer's agent]
This literal is valid Go. The composite literal rules allow a pointer element to omit its type and address operator. Here, {} means &retryablehttp.Client{}. The test covers a nil client and a non-nil client with a nil HTTPClient.
I verified commit 58cf292 with Go 1.26.3. All packages pass three shuffled race runs and go vet ./... on Linux. No change is needed for this finding.
| var workers sync.WaitGroup | ||
| errs := make([]error, inflight) | ||
| for i := range inflight { | ||
| workers.Go(func() { errs[i] = generate(t.Context(), fmt.Sprintf("inflight-%d", i)) }) | ||
| } |
There was a problem hiding this comment.
[automated reply by timer's agent]
sync.WaitGroup.Go was added in Go 1.25. This repository declares Go 1.26.3 in go.mod, so this API is supported.
I verified commit 58cf292 with that toolchain. All packages pass three shuffled race runs and go vet ./... on Linux. The factory tests also pass ten shuffled race runs on macOS. The full macOS suite still hits the previously documented certificate error-string assertion; it does not have a compilation error. No change is needed for this finding.
Port the reproduction from #128, but share one HTTP client between every request instead of handing out a per-client factory. Sixteen requests are parked inside the JIT handler while the admin token expires, so the token refresh overlaps them with nothing synchronizing the two. Run against the code before this change, it reports the race in getActionsServiceAdminConnectionRequest, which confirms the test is sensitive to the defect rather than merely passing. #128 needs a factory to avoid that race; a shared client is safe here because the retry policy is copied per request rather than stored on the client. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a factory that gives each SDK HTTP client its own retry client and transport.
Authored by Timer's automated agent.
Problem
WithRetryableHTTPClintreturns the same client during token refresh and message-session creation. Token refresh changesCheckRetryandErrorHandleron that client. Concurrent JIT requests can read those fields at the same time. The race detector confirms this interleaving.Session-specific HTTP options can also change settings on a shared client. A fresh transport alone does not isolate a shared TLS config or its certificate array.
Change
WithRetryableHTTPClientFactory. The factory returns a fresh retry client,http.Client, and*http.Transportfor each SDK client.WithRetryableHTTPClintfor compatibility, but deprecate it. Callers must use the factory to avoid the shared-client race.Verification
golang:1.26.3image. The source mount was read-only.TestServerWithSelfSignedCertificates/client_without_ca_certs. The same assertion fails on untouched upstream commitcb0405b2d874. This PR does not change that test.Docs/knowledge: API comments describe the new option and its ownership contract.