mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
Fixes: https://github.com/coder/internal/issues/1451 ## Problem Flaky test `TestStreamAgentReinitEvents/doesn't_transmit_events_if_the_transmitter_context_is_canceled` (coder/internal#1451): ``` agentsdk_test.go:84: Error: Received unexpected error: Get "http://127.0.0.1:XXXXX": net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called ``` ## Root cause The subtests used `client := &http.Client{}`. A client with a nil `Transport` uses the process-global `http.DefaultTransport`, which is shared by every parallel test in the test binary. `httptest.Server.Close()` calls `http.DefaultTransport.CloseIdleConnections()`. When any other parallel test closes its `httptest.Server` while this test's request is in flight, the shared transport tears the connection down and `client.Do(req)` fails with `http: CloseIdleConnections called`. This is the same class of flake already documented/fixed in `testutil/oauth2.go` and the `mcphttpclient` helpers, and related to coder/internal#1020. ## Fix Give each client a dedicated `*http.Transport` (`&http.Client{Transport: &http.Transport{}}`) so cross-test `CloseIdleConnections` calls cannot break its requests. The construction is extracted into a small `newReinitTestClient()` helper used by all three subtests, with a comment documenting the reason. ## Verification `go test ./codersdk/agentsdk -run TestStreamAgentReinitEvents -count=20` passes. The flake was reproduced against the exact failing subtest logic (real `NewSSEAgentReinitTransmitter` with a pre-canceled transmit context, same client pattern) under a `CloseIdleConnections` stress loop: - Fix reverted to `&http.Client{}`: reliably FAILs (e.g. 15 broken requests in 10s). - Fix present: 0 broken requests across repeated runs. <details> <summary>Optional stress harness to reproduce/verify locally (not committed)</summary> Drop this into `codersdk/agentsdk/` as a throwaway `*_test.go` file. It runs the verbatim body of the failing subtest in a loop while parallel goroutines call `CloseIdleConnections` (exactly what `httptest.Server.Close()` does). With the fix present it reports `closeIdleErrs=0`; revert `newReinitTestClient` to `&http.Client{}` to reproduce. ```go package agentsdk_test import ( "context" "net/http" "net/http/httptest" "strings" "sync" "sync/atomic" "testing" "time" "github.com/google/uuid" "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/codersdk/agentsdk" ) func TestFlakeReproRealSubtest(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() var wg sync.WaitGroup var closeIdleErrs int64 var sample atomic.Value defaultTransport := http.DefaultTransport.(*http.Transport) for range 8 { wg.Add(1) go func() { defer wg.Done() for ctx.Err() == nil { defaultTransport.CloseIdleConnections() } }() } for range 32 { wg.Add(1) go func() { defer wg.Done() for ctx.Err() == nil { // Verbatim body of the failing subtest. eventToSend := agentsdk.ReinitializationEvent{ WorkspaceID: uuid.New(), Reason: agentsdk.ReinitializeReasonPrebuildClaimed, } events := make(chan agentsdk.ReinitializationEvent, 1) events <- eventToSend transmitCtx, cancelTransmit := context.WithCancel(context.Background()) cancelTransmit() transmitErrCh := make(chan error, 1) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { transmitter := agentsdk.NewSSEAgentReinitTransmitter(slogtest.Make(t, nil), w, r) transmitErrCh <- transmitter.Transmit(transmitCtx, events) })) req, err := http.NewRequestWithContext(ctx, "GET", srv.URL, nil) if err != nil { srv.Close() continue } client := newReinitTestClient() // revert to &http.Client{} to reproduce resp, err := client.Do(req) if err != nil { if strings.Contains(err.Error(), "CloseIdleConnections called") { atomic.AddInt64(&closeIdleErrs, 1) sample.CompareAndSwap(nil, err.Error()) } srv.Close() continue } resp.Body.Close() srv.Close() } }() } wg.Wait() t.Logf("closeIdleErrs=%d", atomic.LoadInt64(&closeIdleErrs)) if n := atomic.LoadInt64(&closeIdleErrs); n > 0 { t.Fatalf("reproduced coder/internal#1451 on the real subtest: %d requests broken (e.g. %v)", n, sample.Load()) } } ``` Example output with the fix reverted to `&http.Client{}`: ``` flakerepro_test.go:88: closeIdleErrs=15 flakerepro_test.go:90: reproduced coder/internal#1451 on the real subtest: 15 requests broken (e.g. Get "http://127.0.0.1:43755": net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called) --- FAIL: TestFlakeReproRealSubtest (10.14s) ``` With the fix present: `closeIdleErrs=0` and PASS. </details> --- Generated by Coder Agents on behalf of @mtojek.
202 lines
6.6 KiB
Go
202 lines
6.6 KiB
Go
package agentsdk_test
|
|
|
|
import (
|
|
"context"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"net/url"
|
|
"testing"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/stretchr/testify/require"
|
|
"tailscale.com/tailcfg"
|
|
|
|
"cdr.dev/slog/v3/sloggers/slogtest"
|
|
"github.com/coder/coder/v2/codersdk/agentsdk"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
// newReinitTestClient returns an http.Client with a dedicated transport.
|
|
//
|
|
// The tests must not use &http.Client{} (which shares the process-global
|
|
// http.DefaultTransport with every other parallel test in the binary).
|
|
// httptest.Server.Close() calls http.DefaultTransport.CloseIdleConnections(),
|
|
// so a server closed by an unrelated parallel test can break an in-flight
|
|
// request here with "http: CloseIdleConnections called". A dedicated transport
|
|
// is not shared, so those cross-test calls cannot affect it. See
|
|
// coder/internal#1451.
|
|
func newReinitTestClient() *http.Client {
|
|
return &http.Client{Transport: &http.Transport{}}
|
|
}
|
|
|
|
func TestStreamAgentReinitEvents(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
t.Run("transmitted events are received", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
eventToSend := agentsdk.ReinitializationEvent{
|
|
WorkspaceID: uuid.New(),
|
|
Reason: agentsdk.ReinitializeReasonPrebuildClaimed,
|
|
OwnerID: uuid.New(),
|
|
}
|
|
|
|
events := make(chan agentsdk.ReinitializationEvent, 1)
|
|
events <- eventToSend
|
|
|
|
transmitCtx := testutil.Context(t, testutil.WaitShort)
|
|
transmitErrCh := make(chan error, 1)
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
transmitter := agentsdk.NewSSEAgentReinitTransmitter(slogtest.Make(t, nil), w, r)
|
|
transmitErrCh <- transmitter.Transmit(transmitCtx, events)
|
|
}))
|
|
defer srv.Close()
|
|
|
|
requestCtx := testutil.Context(t, testutil.WaitShort)
|
|
req, err := http.NewRequestWithContext(requestCtx, "GET", srv.URL, nil)
|
|
require.NoError(t, err)
|
|
client := newReinitTestClient()
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
receiveCtx := testutil.Context(t, testutil.WaitShort)
|
|
receiver := agentsdk.NewSSEAgentReinitReceiver(resp.Body)
|
|
sentEvent, receiveErr := receiver.Receive(receiveCtx)
|
|
require.Nil(t, receiveErr)
|
|
require.Equal(t, eventToSend, *sentEvent)
|
|
})
|
|
|
|
t.Run("doesn't transmit events if the transmitter context is canceled", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
eventToSend := agentsdk.ReinitializationEvent{
|
|
WorkspaceID: uuid.New(),
|
|
Reason: agentsdk.ReinitializeReasonPrebuildClaimed,
|
|
}
|
|
|
|
events := make(chan agentsdk.ReinitializationEvent, 1)
|
|
events <- eventToSend
|
|
|
|
transmitCtx, cancelTransmit := context.WithCancel(testutil.Context(t, testutil.WaitShort))
|
|
cancelTransmit()
|
|
transmitErrCh := make(chan error, 1)
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
transmitter := agentsdk.NewSSEAgentReinitTransmitter(slogtest.Make(t, nil), w, r)
|
|
transmitErrCh <- transmitter.Transmit(transmitCtx, events)
|
|
}))
|
|
|
|
defer srv.Close()
|
|
|
|
requestCtx := testutil.Context(t, testutil.WaitShort)
|
|
req, err := http.NewRequestWithContext(requestCtx, "GET", srv.URL, nil)
|
|
require.NoError(t, err)
|
|
client := newReinitTestClient()
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
receiveCtx := testutil.Context(t, testutil.WaitShort)
|
|
receiver := agentsdk.NewSSEAgentReinitReceiver(resp.Body)
|
|
sentEvent, receiveErr := receiver.Receive(receiveCtx)
|
|
require.Nil(t, sentEvent)
|
|
require.ErrorIs(t, receiveErr, io.EOF)
|
|
})
|
|
|
|
t.Run("does not receive events if the receiver context is canceled", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
eventToSend := agentsdk.ReinitializationEvent{
|
|
WorkspaceID: uuid.New(),
|
|
Reason: agentsdk.ReinitializeReasonPrebuildClaimed,
|
|
}
|
|
|
|
events := make(chan agentsdk.ReinitializationEvent, 1)
|
|
events <- eventToSend
|
|
|
|
transmitCtx := testutil.Context(t, testutil.WaitShort)
|
|
transmitErrCh := make(chan error, 1)
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
transmitter := agentsdk.NewSSEAgentReinitTransmitter(slogtest.Make(t, nil), w, r)
|
|
transmitErrCh <- transmitter.Transmit(transmitCtx, events)
|
|
}))
|
|
defer srv.Close()
|
|
|
|
requestCtx := testutil.Context(t, testutil.WaitShort)
|
|
req, err := http.NewRequestWithContext(requestCtx, "GET", srv.URL, nil)
|
|
require.NoError(t, err)
|
|
client := newReinitTestClient()
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
receiveCtx, cancelReceive := context.WithCancel(context.Background())
|
|
cancelReceive()
|
|
receiver := agentsdk.NewSSEAgentReinitReceiver(resp.Body)
|
|
sentEvent, receiveErr := receiver.Receive(receiveCtx)
|
|
require.Nil(t, sentEvent)
|
|
require.ErrorIs(t, receiveErr, context.Canceled)
|
|
})
|
|
}
|
|
|
|
func TestRewriteDERPMap(t *testing.T) {
|
|
t.Parallel()
|
|
// This test ensures that RewriteDERPMap mutates built-in DERPs with the
|
|
// client access URL.
|
|
dm := &tailcfg.DERPMap{
|
|
Regions: map[int]*tailcfg.DERPRegion{
|
|
1: {
|
|
EmbeddedRelay: true,
|
|
RegionID: 1,
|
|
Nodes: []*tailcfg.DERPNode{{
|
|
HostName: "bananas.org",
|
|
DERPPort: 1,
|
|
}},
|
|
},
|
|
},
|
|
}
|
|
parsed, err := url.Parse("https://coconuts.org:44558")
|
|
require.NoError(t, err)
|
|
client := agentsdk.New(parsed, agentsdk.WithFixedToken("unused"))
|
|
client.RewriteDERPMap(dm)
|
|
region := dm.Regions[1]
|
|
require.True(t, region.EmbeddedRelay)
|
|
require.Len(t, region.Nodes, 1)
|
|
node := region.Nodes[0]
|
|
require.Equal(t, "coconuts.org", node.HostName)
|
|
require.Equal(t, 44558, node.DERPPort)
|
|
}
|
|
|
|
func TestExternalAuthRequestQuery(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
t.Run("IncludesGitRefFieldsAndOmitsWorkdir", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
require.Equal(t, "/api/v2/workspaceagents/me/external-auth", r.URL.Path)
|
|
require.Equal(t, "true", r.URL.Query().Get("listen"))
|
|
require.Equal(t, "main", r.URL.Query().Get("git_branch"))
|
|
require.Equal(t, "https://github.com/coder/coder.git", r.URL.Query().Get("git_remote_origin"))
|
|
require.Equal(t, "test-chat-id", r.URL.Query().Get("chat_id"))
|
|
require.False(t, r.URL.Query().Has("workdir"))
|
|
_, _ = w.Write([]byte(`{"type":"github","access_token":"token"}`))
|
|
}))
|
|
defer srv.Close()
|
|
|
|
parsedURL, err := url.Parse(srv.URL)
|
|
require.NoError(t, err)
|
|
|
|
client := agentsdk.New(parsedURL, agentsdk.WithFixedToken("token"))
|
|
_, err = client.ExternalAuth(testutil.Context(t, testutil.WaitShort), agentsdk.ExternalAuthRequest{
|
|
Match: "github.com",
|
|
Listen: true,
|
|
GitBranch: "main",
|
|
GitRemoteOrigin: "https://github.com/coder/coder.git",
|
|
ChatID: "test-chat-id",
|
|
})
|
|
require.NoError(t, err)
|
|
})
|
|
}
|