mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(codersdk/workspacesdk): fix leaking AwaitReachable when agent unreachable (#26342)
Noticed when enabling the goleak checker in chatd:
```
=== FAIL: coderd/x/chatd (0.00s)
PASS
goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 108179 in state select, with github.com/coder/coder/v2/tailnet.(*Conn).AwaitReachable on top of the stack:
github.com/coder/coder/v2/tailnet.(*Conn).AwaitReachable(0x2c35e171d760, {0x74b37a8?, 0x2c35f6886330?}, {{0x0?, 0x0?}, {0x2c35def838c0?}})
/home/runner/work/coder/coder/tailnet/conn.go:647 +0x2ae
github.com/coder/coder/v2/codersdk/workspacesdk.(*agentConn).AwaitReachable(0x2c35e4f18440, {0x74b37e0?, 0x2c35f45680a0?})
/home/runner/work/coder/coder/codersdk/workspacesdk/agentconn.go:172 +0x12b
github.com/coder/coder/v2/codersdk/workspacesdk.(*agentConn).apiRequest.(*agentConn).apiClient.func1({0x74b37e0, 0x2c35f45680a0}, {0x61a8946?, 0x60fa2a0?}, {0x2c35e0af7380, 0x2b})
/home/runner/work/coder/coder/codersdk/workspacesdk/agentconn.go:1381 +0x212
net/http.(*Transport).dial(0x2c35e0af6210?, {0x74b37e0?, 0x2c35f45680a0?}, {0x61a8946?, 0xa0e255?}, {0x2c35e0af7380?, 0xa1456f?})
/home/runner/work/_temp/mise-data/installs/go/1.26.4/src/net/http/transport.go:1307 +0xd2
net/http.(*Transport).dialConn(0x2c35f8d8b380, {0x74b37e0, 0x2c35f45680a0}, {{}, 0x0, {0x2c35fc5a3e50, 0x4}, {0x2c35e0af7380, 0x2b}, 0x0}, ...)
/home/runner/work/_temp/mise-data/installs/go/1.26.4/src/net/http/transport.go:1815 +0x847
net/http.(*Transport).dialConnFor(0x2c35f8d8b380, 0x2c35e287a580)
/home/runner/work/_temp/mise-data/installs/go/1.26.4/src/net/http/transport.go:1648 +0xd2
net/http.(*Transport).startDialConnForLocked.func1()
/home/runner/work/_temp/mise-data/installs/go/1.26.4/src/net/http/transport.go:1629 +0x35
created by net/http.(*Transport).startDialConnForLocked in goroutine 107872
/home/runner/work/_temp/mise-data/installs/go/1.26.4/src/net/http/transport.go:1628 +0x112
Goroutine 108180 in state select, with github.com/cenkalti/backoff/v4.(*Ticker).run on top of the stack:
github.com/cenkalti/backoff/v4.(*Ticker).run(0x2c35f8517860)
/home/runner/go/pkg/mod/github.com/cenkalti/backoff/v4@v4.3.0/ticker.go:70 +0x13f
created by github.com/cenkalti/backoff/v4.NewTickerWithTimer in goroutine 108179
/home/runner/go/pkg/mod/github.com/cenkalti/backoff/v4@v4.3.0/ticker.go:49 +0x16c
]
FAIL github.com/coder/coder/v2/coderd/x/chatd 126.492s
```
Closes https://github.com/coder/internal/issues/1595
This commit is contained in:
@@ -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())
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
Reference in New Issue
Block a user