From 2427b7598a1f3526661dddc490b752ef11dac54e Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 11 Feb 2026 16:19:51 +0100 Subject: [PATCH 1/3] fix: propagate cleanup errors from fakeCleanup and recordCleanup Change setupRecordingProxy to return func() error instead of swallowing errors internally, making it consistent with setupFakeProxy. Use a named return in runOrExec so deferred cleanup errors are propagated when no prior error occurred. Fixes #1691 Assisted-By: cagent --- cmd/root/api.go | 10 +++++++--- cmd/root/record.go | 10 +++------- cmd/root/record_test.go | 6 +++--- cmd/root/run.go | 14 ++++++++++++-- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/cmd/root/api.go b/cmd/root/api.go index a9862aa99..62c23bac4 100644 --- a/cmd/root/api.go +++ b/cmd/root/api.go @@ -113,10 +113,14 @@ func (f *apiFlags) runAPICommand(cmd *cobra.Command, args []string) error { }() // Start recording proxy if --record is specified - if _, cleanup, err := setupRecordingProxy(f.recordPath, &f.runConfig); err != nil { + if _, recordCleanup, err := setupRecordingProxy(f.recordPath, &f.runConfig); err != nil { return err - } else if cleanup != nil { - defer cleanup() + } else if recordCleanup != nil { + defer func() { + if err := recordCleanup(); err != nil { + slog.Error("Failed to cleanup recording proxy", "error", err) + } + }() } if f.pullIntervalMins > 0 && !config.IsOCIReference(agentsPath) { diff --git a/cmd/root/record.go b/cmd/root/record.go index b21d58c8a..49e8fd85a 100644 --- a/cmd/root/record.go +++ b/cmd/root/record.go @@ -45,9 +45,9 @@ func setupFakeProxy(fakeResponses string, streamDelayMs int, runConfig *config.R // and normalizes the path by stripping any .yaml suffix. // Returns the cassette path (with .yaml extension) and a cleanup function. // The cleanup function must be called when done (typically via defer). -func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (cassettePath string, cleanup func(), err error) { +func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (cassettePath string, cleanup func() error, err error) { if recordPath == "" { - return "", func() {}, nil + return "", func() error { return nil }, nil } // Handle auto-generated filename (from NoOptDefVal) @@ -67,9 +67,5 @@ func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (ca slog.Info("Recording mode enabled", "cassette", cassettePath, "proxy", proxyURL) - return cassettePath, func() { - if err := cleanupFn(); err != nil { - slog.Error("Failed to cleanup recording proxy", "error", err) - } - }, nil + return cassettePath, cleanupFn, nil } diff --git a/cmd/root/record_test.go b/cmd/root/record_test.go index 7da4924d7..ae8ffe090 100644 --- a/cmd/root/record_test.go +++ b/cmd/root/record_test.go @@ -21,7 +21,7 @@ func TestSetupRecordingProxy_EmptyPath(t *testing.T) { assert.NotNil(t, cleanup) assert.Empty(t, runConfig.ModelsGateway, "ModelsGateway should not be set") - cleanup() + require.NoError(t, cleanup()) } func TestSetupRecordingProxy_AutoGeneratesFilename(t *testing.T) { @@ -31,7 +31,7 @@ func TestSetupRecordingProxy_AutoGeneratesFilename(t *testing.T) { cassettePath, cleanup, err := setupRecordingProxy("true", &runConfig) require.NoError(t, err) - defer cleanup() + defer func() { require.NoError(t, cleanup()) }() assert.True(t, strings.HasPrefix(cassettePath, "cagent-recording-"), "should have auto-generated prefix") assert.True(t, strings.HasSuffix(cassettePath, ".yaml"), "should have .yaml suffix") @@ -46,7 +46,7 @@ func TestSetupRecordingProxy_CreatesProxy(t *testing.T) { resultPath, cleanup, err := setupRecordingProxy(cassettePath, &runConfig) require.NoError(t, err) - defer cleanup() + defer func() { require.NoError(t, cleanup()) }() assert.Equal(t, cassettePath+".yaml", resultPath) assert.True(t, strings.HasPrefix(runConfig.ModelsGateway, "http://"), "ModelsGateway should be HTTP URL") diff --git a/cmd/root/run.go b/cmd/root/run.go index a59c0bd01..fafaab6cd 100644 --- a/cmd/root/run.go +++ b/cmd/root/run.go @@ -119,7 +119,7 @@ func (f *runExecFlags) runRunCommand(cmd *cobra.Command, args []string) error { return f.runOrExec(ctx, out, args, tui) } -func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []string, tui bool) error { +func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []string, tui bool) (retErr error) { slog.Debug("Starting agent", "agent", f.agentName) // Start CPU profiling if requested @@ -193,6 +193,9 @@ func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []s defer func() { if err := fakeCleanup(); err != nil { slog.Error("Failed to cleanup fake proxy", "error", err) + if retErr == nil { + retErr = fmt.Errorf("failed to cleanup fake proxy: %w", err) + } } }() @@ -202,7 +205,14 @@ func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []s return err } if cassettePath != "" { - defer recordCleanup() + defer func() { + if err := recordCleanup(); err != nil { + slog.Error("Failed to cleanup recording proxy", "error", err) + if retErr == nil { + retErr = fmt.Errorf("failed to cleanup recording proxy: %w", err) + } + } + }() out.Println("Recording mode enabled, cassette: " + cassettePath) } From c9116c05b6766daa06b6708afd95f35d49460e12 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 11 Feb 2026 16:21:40 +0100 Subject: [PATCH 2/3] fix: log error on log file close instead of discarding it Fixes #1690 Assisted-By: cagent --- cmd/root/root.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/root/root.go b/cmd/root/root.go index ed3fe819d..ab04c5310 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -65,7 +65,9 @@ func NewRootCmd() *cobra.Command { }, PersistentPostRunE: func(cmd *cobra.Command, args []string) error { if flags.logFile != nil { - _ = flags.logFile.Close() + if err := flags.logFile.Close(); err != nil { + slog.Error("Failed to close log file", "error", err) + } } return nil }, From e49232f3df011d95abb99cdc336dbadc345c3909 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 11 Feb 2026 16:25:46 +0100 Subject: [PATCH 3/3] Fix potential goroutine leak in monitorStdin Add a done channel to ensure the background goroutine that closes stdin exits cleanly when monitorStdin returns, not only when the context is cancelled. This prevents the goroutine from leaking if stdin closes before context cancellation. Fixes #1664 Assisted-By: cagent --- cmd/root/api.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cmd/root/api.go b/cmd/root/api.go index 62c23bac4..ad259eb43 100644 --- a/cmd/root/api.go +++ b/cmd/root/api.go @@ -56,18 +56,26 @@ func newAPICmd() *cobra.Command { // monitorStdin monitors stdin for EOF, which indicates the parent process has died. // When spawned with piped stdio, stdin closes when the parent process dies. +// The caller is responsible for cancelling the context (e.g. via defer cancel()). func monitorStdin(ctx context.Context, cancel context.CancelFunc, stdin *os.File) { - // Close stdin when context is cancelled to unblock the read + done := make(chan struct{}) + + // Close stdin when context is cancelled to unblock the read. + // Also exits cleanly when monitorStdin returns. go func() { - <-ctx.Done() + select { + case <-ctx.Done(): + case <-done: + } stdin.Close() }() + defer close(done) + buf := make([]byte, 1) for { n, err := stdin.Read(buf) if err != nil || n == 0 { - // Only log and cancel if context isn't already done (parent died) if ctx.Err() == nil { slog.Info("stdin closed, parent process likely died, shutting down") cancel()