Conversation
There was a problem hiding this comment.
Review Summary
Found one minor inconsistency in error handling between the two cleanup approaches. The api.go command silently ignores Close() errors while run.go properly logs them. This is a low-severity code quality issue that could make debugging resource cleanup problems more difficult.
The overall approach of adding Close() methods to properly clean up session store resources is solid and addresses the resource leak issues mentioned in #1671 and #1672.
cmd/root/api.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("creating session store: %w", err) | ||
| } | ||
| defer sessionStore.Close() |
There was a problem hiding this comment.
Missing error handling for sessionStore.Close()
The defer statement ignores any error returned by Close(). This is inconsistent with the error handling pattern in cmd/root/run.go (lines 243-245) where rt.Close() errors are logged.
Consider handling the error to aid debugging:
defer func() {
if err := sessionStore.Close(); err != nil {
slog.Error("Failed to close session store", "error", err)
}
}()While this won't cause runtime failures, it could hide important resource cleanup errors during debugging.
Fixes docker#1672 Fixes docker#1671 Assisted-By: cagent
Fix #1671
Fix #1672