diff --git a/cmd/root/api.go b/cmd/root/api.go index a9862aa99..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() @@ -113,10 +121,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/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 }, 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) }