diff --git a/codersdk/workspacesdk/agentconn.go b/codersdk/workspacesdk/agentconn.go index 6882ff0d91..a47a19db26 100644 --- a/codersdk/workspacesdk/agentconn.go +++ b/codersdk/workspacesdk/agentconn.go @@ -505,7 +505,7 @@ func (c *agentConn) WatchContainers(ctx context.Context, logger slog.Logger) (<- url := fmt.Sprintf("http://%s%s", host, "/api/v0/containers/watch") conn, res, err := websocket.Dial(ctx, url, &websocket.DialOptions{ - HTTPClient: c.apiClient(), + HTTPClient: c.apiClient(ctx), // We want `NoContextTakeover` compression to balance improving // bandwidth cost/latency with minimal memory usage overhead. @@ -541,7 +541,7 @@ func (c *agentConn) WatchGit(ctx context.Context, logger slog.Logger, chatID uui host := net.JoinHostPort(c.agentAddress().String(), strconv.Itoa(AgentHTTPAPIServerPort)) dialOpts := &websocket.DialOptions{ - HTTPClient: c.apiClient(), + HTTPClient: c.apiClient(ctx), CompressionMode: websocket.CompressionNoContextTakeover, } c.headersMu.RLock() @@ -583,7 +583,7 @@ func (c *agentConn) ConnectDesktopVNC(ctx context.Context) (net.Conn, error) { host := net.JoinHostPort(c.agentAddress().String(), strconv.Itoa(AgentHTTPAPIServerPort)) dialOpts := &websocket.DialOptions{ - HTTPClient: c.apiClient(), + HTTPClient: c.apiClient(ctx), CompressionMode: websocket.CompressionDisabled, } c.headersMu.RLock() @@ -696,7 +696,7 @@ func (c *agentConn) ExecuteDesktopAction(ctx context.Context, action DesktopActi } c.headersMu.RUnlock() - resp, err := c.apiClient().Do(req) + resp, err := c.apiClient(ctx).Do(req) if err != nil { return DesktopActionResponse{}, xerrors.Errorf("action request: %w", err) } @@ -1352,12 +1352,14 @@ func (c *agentConn) apiRequest(ctx context.Context, method, path string, body in } } - return c.apiClient().Do(req) + return c.apiClient(ctx).Do(req) } // apiClient returns an HTTP client that can be used to make -// requests to the workspace agent's HTTP API server. -func (c *agentConn) apiClient() *http.Client { +// requests to the workspace agent's HTTP API server. The client is +// scoped to a single request: its transport cancels in-flight dials +// once reqCtx ends. +func (c *agentConn) apiClient(reqCtx context.Context) *http.Client { return &http.Client{ Transport: &http.Transport{ // Disable keep alives as we're usually only making a single @@ -1378,6 +1380,18 @@ func (c *agentConn) apiClient() *http.Client { return nil, xerrors.Errorf("request %q does not appear to be for http api", addr) } + // http.Transport detaches ctx from the request context so + // a pending dial can outlive its request and serve future + // requests. This client is request-scoped with keep-alives + // disabled, so a detached dial can never be reused. Without + // re-linking cancellation, a canceled request would leave + // the dial blocked in AwaitReachable until the agent becomes + // reachable, which may be never. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + stop := context.AfterFunc(reqCtx, cancel) + defer stop() + if !c.AwaitReachable(ctx) { return nil, xerrors.Errorf("workspace agent not reachable in time: %v", ctx.Err()) } diff --git a/codersdk/workspacesdk/agentconn_test.go b/codersdk/workspacesdk/agentconn_test.go new file mode 100644 index 0000000000..9a5e3a93bb --- /dev/null +++ b/codersdk/workspacesdk/agentconn_test.go @@ -0,0 +1,66 @@ +package workspacesdk_test + +import ( + "context" + "net/netip" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" + + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/tailnet" + "github.com/coder/coder/v2/testutil" +) + +// TestAgentConn_DialBoundedByRequestContext verifies that the +// transport dial behind the agent HTTP API stops when the request +// context ends. http.Transport detaches dial contexts from the +// request context so a pending dial can outlive its request and +// serve future ones, but the agent API client is request-scoped +// with keep-alives disabled, so a detached dial can never be +// reused. If the transport does not re-link cancellation, the dial +// goroutine stays blocked in AwaitReachable pinging an unreachable +// agent forever, even after the tailnet conn is closed, and leaks. +// +//nolint:paralleltest // goleak.IgnoreCurrent requires this test to run non-parallel. +func TestAgentConn_DialBoundedByRequestContext(t *testing.T) { + // goleak.IgnoreCurrent snapshots running goroutines, so this + // test must not run in parallel with other tests. + logger := testutil.Logger(t) + + // Snapshot before the tailnet conn exists so everything spawned + // below, including the transport dial goroutine, is verified. + ignoreCurrent := goleak.IgnoreCurrent() + + tailnetConn, err := tailnet.NewConn(&tailnet.Options{ + Addresses: []netip.Prefix{tailnet.TailscaleServicePrefix.RandomPrefix()}, + Logger: logger.Named("client"), + }) + require.NoError(t, err) + t.Cleanup(func() { + _ = tailnetConn.Close() + }) + + conn := workspacesdk.NewAgentConn(tailnetConn, workspacesdk.AgentConnOptions{ + AgentID: uuid.New(), + }) + + // No agent exists, so the transport dial blocks in + // AwaitReachable until the request context expires. The timeout + // only needs to be long enough for the dial goroutine to start; + // its expiry is the behavior under test. + ctx, cancel := context.WithTimeout(context.Background(), testutil.IntervalSlow) + defer cancel() + _, err = conn.ListeningPorts(ctx) + require.Error(t, err) + + // Close the conn like test teardown would. The conn's own + // goroutines exit on close; the dial goroutine must have already + // exited when the request context expired. + err = tailnetConn.Close() + require.NoError(t, err) + + goleak.VerifyNone(t, ignoreCurrent) +}