Conversation
Signed-off-by: David Gageot <david.gageot@docker.com>
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Found 1 critical bug and 1 potential issue in this PR.
Critical: The change on line 126 in cmd/root/api.go removes a nil check for recordCleanup, which will cause a nil pointer panic when the --record flag is not specified (the common case).
Medium: The new listenAndCloseOnCancel helper function has a design that could lead to goroutine leaks if used with long-lived contexts, though current usage is safe.
| } else if recordCleanup != nil { | ||
| } else { | ||
| defer func() { | ||
| if err := recordCleanup(); err != nil { |
There was a problem hiding this comment.
🚨 CRITICAL BUG: Nil pointer dereference
The code unconditionally calls recordCleanup() without checking if it's nil. When the --record flag is not specified (the common case), setupRecordingProxy returns a nil recordCleanup function, and calling it here will cause a panic.
Evidence: In cmd/root/run.go (lines 228-236), the same setupRecordingProxy function is used but the defer is guarded with if cassettePath != "" {, indicating that recordCleanup can be nil.
Fix: Restore the nil check:
} else if recordCleanup != nil {
defer func() {
if err := recordCleanup(); err != nil {
slog.Error("Failed to cleanup recording proxy", "error", err)
}
}()
}Or follow the pattern from run.go and check if the cassette path is non-empty before deferring the cleanup.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to listen on %s: %w", addr, err) | ||
| } | ||
| go func() { |
There was a problem hiding this comment.
While the current usage is safe (all callers pass cmd.Context() which is cancelled on command exit), this function design could lead to goroutine leaks if used with long-lived contexts in the future.
The spawned goroutine waits indefinitely for ctx.Done() with no timeout. If a parent context that outlives the listener is passed, the goroutine will leak.
Suggestion: Consider adding a comment documenting that this function requires a context that will be cancelled when the listener is no longer needed, or refactor to use a more robust cleanup pattern.
3 participants
No description provided.