Skip to content

fix: exorcise goroutine-leaking util.After from the codebase#211

Merged
johnstcn merged 1 commit intomainfrom
fix/goroutine-leak-util-after
Mar 31, 2026
Merged

fix: exorcise goroutine-leaking util.After from the codebase#211
johnstcn merged 1 commit intomainfrom
fix/goroutine-leak-util-after

Conversation

@johnstcn
Copy link
Copy Markdown
Member

Summary

  • Replace all 5 util.After call sites with inline clock.NewTimer + explicit Stop()
  • Delete the leaky util.After function entirely

Problem

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.

Changes

  • lib/screentracker/pty_conversation.go — Phase 1 stability timer + Phase 2 carriage-return timer now use inline NewTimer with Stop() on both cancel and normal paths
  • lib/termexec/termexec.goReadScreen vsync timer + Close timeout timer replaced with inline NewTimer
  • cmd/attach/attach.go — Grace period timer replaced, removed unused util import
  • lib/util/util.go — Deleted After function
Implementation context

The goroutine leak was originally flagged by Copilot in PR #208. The After helper used make(chan time.Time) (unbuffered) + a goroutine doing ch <- <-timer.C. If nobody drained ch, the goroutine blocked on send forever. Inline NewTimer is the idiomatic quartz pattern already used by WaitFor itself.

Fixes #210

🤖 Written by a Coder Agent. Will be reviewed by a human.

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
@github-actions
Copy link
Copy Markdown

✅ Preview binaries are ready!

To test with modules: agentapi_version = "agentapi_211" or download from: https://github.com/coder/agentapi/releases/tag/agentapi_211

@johnstcn johnstcn self-assigned this Mar 31, 2026
Copy link
Copy Markdown
Member Author

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-reviewed

@johnstcn johnstcn marked this pull request as ready for review March 31, 2026 22:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the goroutine-leaking util.After helper and replaces its usage with inline quartz.Clock.NewTimer timers that are explicitly Stop()’d, preventing goroutine leaks when a select takes a non-timer branch (e.g., ctx.Done()).

Changes:

  • Deleted util.After from lib/util/util.go.
  • Replaced all previous util.After call sites with clock.NewTimer(...) + Stop() patterns (including ctx-cancel paths where applicable).
  • Cleaned up cmd/attach imports after removing the last util.After usage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
lib/util/util.go Removes the leaky After helper entirely.
lib/termexec/termexec.go Replaces the vsync sleep and Close timeout with NewTimer and Stop().
lib/screentracker/pty_conversation.go Replaces Phase 1 stability + Phase 2 carriage-return delays with explicit timers that are stopped on both normal and cancel paths.
cmd/attach/attach.go Replaces grace-period wait with NewTimer and removes now-unused util import.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@johnstcn johnstcn merged commit d42b9ac into main Mar 31, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: potential goroutine leak in util.WaitFor

3 participants