From 5dc825afa99fbf34974b5136b3a7b315a85caae4 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 22:00:54 +0000 Subject: [PATCH] fix: exorcise goroutine-leaking util.After from the codebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit util.After spawned a goroutine that sent on an unbuffered channel. If the caller's select picked a different branch (e.g. ctx.Done()), the goroutine blocked forever on the send — leaking one goroutine per cancellation. Replace all 5 call sites with inline clock.NewTimer + explicit Stop(), then delete util.After entirely so nobody can reintroduce the leak. Fixes #210 --- cmd/attach/attach.go | 5 +++-- lib/screentracker/pty_conversation.go | 10 ++++++++-- lib/termexec/termexec.go | 8 ++++++-- lib/util/util.go | 19 ------------------- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/cmd/attach/attach.go b/cmd/attach/attach.go index 10fbdccd..7658ec68 100644 --- a/cmd/attach/attach.go +++ b/cmd/attach/attach.go @@ -14,7 +14,6 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/coder/agentapi/lib/httpapi" - "github.com/coder/agentapi/lib/util" "github.com/coder/quartz" "github.com/spf13/cobra" sse "github.com/tmaxmax/go-sse" @@ -239,9 +238,11 @@ func runAttach(remoteURL string) error { } p.Send(finishMsg{}) + graceTimer := quartz.NewReal().NewTimer(1 * time.Second) + defer graceTimer.Stop() select { case <-pErrCh: - case <-util.After(quartz.NewReal(), 1*time.Second): + case <-graceTimer.C: } return err diff --git a/lib/screentracker/pty_conversation.go b/lib/screentracker/pty_conversation.go index 0e309fec..cce8c677 100644 --- a/lib/screentracker/pty_conversation.go +++ b/lib/screentracker/pty_conversation.go @@ -428,11 +428,14 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me }, func() (bool, error) { screen := c.cfg.AgentIO.ReadScreen() if screen != screenBeforeMessage { + stabilityTimer := c.cfg.Clock.NewTimer(1 * time.Second) select { case <-ctx.Done(): + stabilityTimer.Stop() return false, ctx.Err() - case <-util.After(c.cfg.Clock, 1*time.Second): + case <-stabilityTimer.C: } + stabilityTimer.Stop() newScreen := c.cfg.AgentIO.ReadScreen() return newScreen == screen, nil } @@ -458,11 +461,14 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me return false, xerrors.Errorf("failed to write carriage return: %w", err) } } + crTimer := c.cfg.Clock.NewTimer(25 * time.Millisecond) select { case <-ctx.Done(): + crTimer.Stop() return false, ctx.Err() - case <-util.After(c.cfg.Clock, 25*time.Millisecond): + case <-crTimer.C: } + crTimer.Stop() screen := c.cfg.AgentIO.ReadScreen() return screen != screenBeforeCarriageReturn, nil diff --git a/lib/termexec/termexec.go b/lib/termexec/termexec.go index 05403690..e83c63d7 100644 --- a/lib/termexec/termexec.go +++ b/lib/termexec/termexec.go @@ -127,7 +127,9 @@ func (p *Process) ReadScreen() string { return state } p.screenUpdateLock.RUnlock() - <-util.After(p.clock, 16*time.Millisecond) + t := p.clock.NewTimer(16 * time.Millisecond) + <-t.C + t.Stop() } return p.xp.State.String() } @@ -152,9 +154,11 @@ func (p *Process) Close(logger *slog.Logger, timeout time.Duration) error { close(exited) }() + timeoutTimer := p.clock.NewTimer(timeout) + defer timeoutTimer.Stop() var exitErr error select { - case <-util.After(p.clock, timeout): + case <-timeoutTimer.C: if err := p.execCmd.Process.Kill(); err != nil { exitErr = xerrors.Errorf("failed to forcefully kill the process: %w", err) } diff --git a/lib/util/util.go b/lib/util/util.go index 9e4d5847..b89bf4d6 100644 --- a/lib/util/util.go +++ b/lib/util/util.go @@ -99,22 +99,3 @@ func OpenAPISchema[T ~string](r huma.Registry, enumName string, values []T) *hum } return &huma.Schema{Ref: fmt.Sprintf("#/components/schemas/%s", enumName)} } - -// After is a convenience function that returns a channel that will send the -// time after the given duration has elapsed using the provided clock. -// If clk is nil, a real clock will be used by default. -// Note that this function spawns a goroutine that will remain alive until the -// timer fires. -func After(clk quartz.Clock, d time.Duration) <-chan time.Time { - if clk == nil { - clk = quartz.NewReal() - } - timer := clk.NewTimer(d) - ch := make(chan time.Time) - go func() { - defer timer.Stop() - defer close(ch) - ch <- <-timer.C - }() - return ch -}