Fix responses goroutine leak#874
Merged
Merged
Conversation
NewRouter now returns a RouterResult that tracks closable resources (currently the responses Store cleanup goroutine). Service gains a Close() method that delegates to RouterResult.Close(). Callers (e.g. Docker Desktop / pinata) must call Service.Close() on shutdown. Without this, the responses Store background goroutine leaks and triggers goroutine-leak detectors in integration tests.
Ensures the responses Store cleanup goroutine is stopped when the standalone model-runner process exits.
Add testmain_test.go with goleak.VerifyTestMain to detect goroutine leaks in the routing package. Add router_test.go with tests that verify: - NewRouter with IncludeResponsesAPI + Close does not leak goroutines - NewRouter without IncludeResponsesAPI + Close is safe (no-op) Verified that without calling Close, goleak catches the leaked responses.Store.cleanupLoop goroutine, confirming these tests serve as a regression guard.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider making
RouterResult.Closeidempotent (e.g., by clearing or guarding theclosersslice after the first call) so that accidental double-closing does not invoke cleanup functions multiple times. - In
main.go, you might want to callsvc.Close()withdeferimmediately after successful service creation so that resources are reliably cleaned up on all exit paths, including early returns due to errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `RouterResult.Close` idempotent (e.g., by clearing or guarding the `closers` slice after the first call) so that accidental double-closing does not invoke cleanup functions multiple times.
- In `main.go`, you might want to call `svc.Close()` with `defer` immediately after successful service creation so that resources are reliably cleaned up on all exit paths, including early returns due to errors.
## Individual Comments
### Comment 1
<location path="pkg/routing/router.go" line_range="20-22" />
<code_context>
+// resources that require cleanup.
+type RouterResult struct {
+ Mux *NormalizedServeMux
+ // closers collects io.Closer values that must be closed when the
+ // router is no longer needed (e.g. the responses Store goroutine).
+ closers []func()
+}
+
</code_context>
<issue_to_address>
**suggestion:** Align the `closers` field type with the comment (io.Closer vs func()).
The field’s behavior and its comment don’t match. Please either change the type to `[]io.Closer` and call `.Close()` in `RouterResult.Close`, or update the comment to describe that this holds arbitrary cleanup functions rather than `io.Closer` instances.
Suggested implementation:
```golang
"io"
"github.com/docker/model-runner/pkg/responses"
```
```golang
// RouterResult is the output of NewRouter, bundling the mux with any
// resources that require cleanup.
type RouterResult struct {
Mux *NormalizedServeMux
// closers collects io.Closer values that must be closed when the
// router is no longer needed (e.g. the responses Store goroutine).
closers []io.Closer
}
```
To fully align behavior with the new `[]io.Closer` type:
1. Update `RouterResult.Close` (wherever it is defined) to iterate over `r.closers` and call `Close()` on each element, e.g.:
-
```go
func (r *RouterResult) Close() {
for _, c := range r.closers {
if c != nil {
_ = c.Close()
}
}
}
```
Optionally clear the slice afterward (`r.closers = nil`) if the router can be reused.
2. Update all code that constructs a `RouterResult` to append `io.Closer` implementations to `closers` instead of `func()` cleanup callbacks. Any existing `func()`-based cleanup should be wrapped in an adapter that implements `io.Closer` if still needed.
</issue_to_address>
### Comment 2
<location path="pkg/routing/router_test.go" line_range="12" />
<code_context>
+// IncludeResponsesAPI enabled and then calling Close does not leak
+// goroutines. The goleak detector in TestMain will catch any leak.
+func TestNewRouter_WithResponsesAPI_Close(t *testing.T) {
+ log := slog.New(slog.DiscardHandler)
+
+ result := NewRouter(RouterConfig{
</code_context>
<issue_to_address>
**issue (testing):** Tests use `slog.DiscardHandler`, which does not exist in the standard slog API and will prevent the tests from compiling
`log/slog` doesn’t define `DiscardHandler`, so this test won’t compile. To discard logs while keeping a valid logger, you can do something like `slog.New(slog.NewTextHandler(io.Discard, nil))` (or JSON handler). Any handler that writes to `io.Discard` is sufficient.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a resource cleanup mechanism to prevent goroutine leaks by introducing a RouterResult struct and Close methods for the router and service. Feedback highlights the need for idempotency in the Close methods to prevent panics on multiple calls and the addition of nil checks to avoid potential pointer dereferences in main.go and service.go.
- RouterResult.Close: nil out closers slice after execution to prevent double-close panics (responses.Store.Close closes a channel). - Service.Close: guard against nil receiver and nil routerResult; nil out routerResult after closing for idempotency. - main.go: use defer svc.Close() right after creation so cleanup runs on all exit paths. Remove the redundant explicit call at the bottom. - Fix closers field comment to accurately describe []func() (not io.Closer).
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
RouterResult.CloseandService.Closemethods are documented as idempotent but are not concurrency-safe; if there’s any chance of concurrent shutdown paths, consider guarding theclosersslice androuterResultwith a mutex orsync.Onceor clarify in the doc comments that they are not safe for concurrent use. - Since
RouterResultis essentially a resource bundle with aClosemethod, consider having it implementio.Closer(and documenting that), which would make it easier to integrate with existing cleanup patterns and tooling in the rest of the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `RouterResult.Close` and `Service.Close` methods are documented as idempotent but are not concurrency-safe; if there’s any chance of concurrent shutdown paths, consider guarding the `closers` slice and `routerResult` with a mutex or `sync.Once` or clarify in the doc comments that they are not safe for concurrent use.
- Since `RouterResult` is essentially a resource bundle with a `Close` method, consider having it implement `io.Closer` (and documenting that), which would make it easier to integrate with existing cleanup patterns and tooling in the rest of the codebase.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
doringeman
approved these changes
Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces proper resource cleanup for the router and service layers to prevent goroutine leaks, especially when the optional Responses API is enabled. The changes ensure that any background goroutines started by the router (such as those for the responses Store) are properly stopped when the service shuts down. It also adds tests to verify that resource cleanup is performed correctly and integrates a goroutine leak detector into the test suite.