fix(coderd/x/chatd): synchronize aibridgeTestFactory recorded fields (#28031)

Fixes the data race in the chatd test helper `aibridgeTestFactory`
reported in CODAGT-917 (`test-go-race-pg` flake in
`TestAwaitSubagentCompletion/Timeout`).

`TransportFor` recorded `providerName` and `source` with plain field
writes. Tests that start the chat worker share one factory between
concurrently running chat runners (parent chat and spawned subagent), so
two runners resolving models at the same time raced on those writes.

The fix guards the recorded fields with a mutex and reads them through a
locked `recorded()` accessor at the three asserting call sites.

Verified with a red-green repro: concurrent `TransportFor` calls on one
factory failed `go test -race` with the exact CI signature (lines 37-38)
before the fix and pass after it. Also ran `go test -race
./coderd/x/chatd -run TestAwaitSubagentCompletion -count=10` and the
full `go test -race ./coderd/x/chatd` package, both clean.

Audited every other `aibridge.TransportFactory` implementation and
`aibridgeTestFactory` use site for the same defect:
`chattest.MockAIBridgeTransport` is already mutex-guarded,
`stubTransportFactory` (coderd/aibridge_test.go) records via a channel,
`providerRoutedTransportFactory` (chatd_test.go) is a stateless lookup,
and the production factories keep no recorded state. No other occurrence
exists.

Closes CODAGT-917.

> Mux acted on Mike's behalf to create this PR.
This commit is contained in:
Michael Suchacz
2026-08-11 23:51:00 +02:00
committed by GitHub
parent 5fab238ed1
commit bde38e9d10
+19 -6
View File
@@ -7,6 +7,7 @@ import (
"io"
"net/http"
"strings"
"sync"
"sync/atomic"
"testing"
@@ -27,6 +28,7 @@ import (
)
type aibridgeTestFactory struct {
mu sync.Mutex
providerName string
source aibridge.Source
err error
@@ -34,6 +36,8 @@ type aibridgeTestFactory struct {
}
func (f *aibridgeTestFactory) TransportFor(providerName string, source aibridge.Source) (http.RoundTripper, error) {
f.mu.Lock()
defer f.mu.Unlock()
f.providerName = providerName
f.source = source
if f.err != nil {
@@ -42,6 +46,12 @@ func (f *aibridgeTestFactory) TransportFor(providerName string, source aibridge.
return f.rt, nil
}
func (f *aibridgeTestFactory) recorded() (providerName string, source aibridge.Source) {
f.mu.Lock()
defer f.mu.Unlock()
return f.providerName, f.source
}
type roundTripFunc func(*http.Request) (*http.Response, error)
func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
@@ -617,8 +627,9 @@ func TestAIBridgeGatewayProviderTypesPreserveSlashModelID(t *testing.T) {
got := <-seen
require.NotEmpty(t, got.path)
require.Equal(t, modelName, got.model)
require.Equal(t, tt.providerName, factory.providerName)
require.Equal(t, aibridge.SourceAgents, factory.source)
gotProvider, gotSource := factory.recorded()
require.Equal(t, tt.providerName, gotProvider)
require.Equal(t, aibridge.SourceAgents, gotSource)
})
}
}
@@ -655,8 +666,9 @@ func TestAIBridgeComputerUseModelUsesRoute(t *testing.T) {
require.False(t, debugEnabled)
require.EqualValues(t, codersdk.ChatComputerUseProviderOpenAI, resolvedProvider)
require.Equal(t, modelName, resolvedModel)
require.Equal(t, "primary-openai", factory.providerName)
require.Equal(t, aibridge.SourceAgents, factory.source)
gotProvider, gotSource := factory.recorded()
require.Equal(t, "primary-openai", gotProvider)
require.Equal(t, aibridge.SourceAgents, gotSource)
}
// The computer-use model is a hardcoded default with no config of its own, so
@@ -776,8 +788,9 @@ func TestAIBridgeDelegatedContextPropagation(t *testing.T) {
require.NoError(t, err)
got := <-seen
require.Equal(t, "primary-openai", factory.providerName)
require.Equal(t, aibridge.SourceAgents, factory.source)
gotProvider, gotSource := factory.recorded()
require.Equal(t, "primary-openai", gotProvider)
require.Equal(t, aibridge.SourceAgents, gotSource)
require.True(t, got.ok)
require.Equal(t, "/v1/responses", got.path)
require.Equal(t, apiKeyID, got.apiKeyID)