From 7daac2b09d7c2ee5cfdae4ad805134f2eb73c165 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 00:11:17 +0100 Subject: [PATCH] fix(cli): only print "CSV report written to" after a complete CSV write writeMultiServiceCSVReport deferred writer.Flush() and never checked writer.Error(), so a failed flush (full disk, failing mount) lost the whole report while returning nil. It also returned nil without creating a file when there were no results. Both callers printed "CSV report written to" on any nil return. Flush explicitly and return writer.Error(); return a file Close error instead of only logging it. Route both callers through a new writeCSVReport helper that says "CSV report not written" for a run with no results, and prints the success line only after a complete write. Closes #2004 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service.go | 12 ++------ cmd/multi_service_csv.go | 39 ++++++++++++++++++++---- cmd/multi_service_csv_test.go | 57 +++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 16 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index d43a39bd1..8b65f1520 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -288,11 +288,7 @@ func confirmPurchaseRun(recs []common.Recommendation, isDryRun bool) bool { func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config, drops *common.DropSummary) { serviceStats := buildServiceStats(passed, allResults) finalCSVOutput := generateCSVFilename(isDryRun, cfg) - if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { - log.Printf("Warning: Failed to write CSV output: %v", err) - } else { - AppLogger.Printf("\nšŸ“‹ CSV report written to: %s\n", finalCSVOutput) - } + writeCSVReport(allResults, finalCSVOutput) printDropSummary(drops) printMultiServiceSummary(passed, allResults, serviceStats, isDryRun) } @@ -812,11 +808,7 @@ func runCSVPurchases(ctx context.Context, awsCfg aws.Config, recs []common.Recom finalCSVOutput := generateCSVFilename(isDryRun, cfg) // Write CSV report - if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { - log.Printf("Warning: Failed to write CSV output: %v", err) - } else { - AppLogger.Printf("\nšŸ“‹ CSV report written to: %s\n", finalCSVOutput) - } + writeCSVReport(allResults, finalCSVOutput) // Print final summary using the post-dedup slice so counts match what was // actually processed, not the pre-dedup input passed into the outer loop. diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index ce2025d8a..0d105f23f 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -254,8 +254,24 @@ func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, tar return nil } -// writeMultiServiceCSVReport writes purchase results to a CSV file. -func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string) error { +// writeCSVReport writes the purchase report and tells the operator whether a +// file exists: "written to" is printed only after a complete, flushed and +// closed write. A run with no results writes no file, so it says so instead. +func writeCSVReport(results []common.PurchaseResult, path string) { + if len(results) == 0 { + AppLogger.Printf("\nšŸ“‹ No purchase results; CSV report not written.\n") + return + } + if err := writeMultiServiceCSVReport(results, path); err != nil { + log.Printf("Warning: Failed to write CSV output: %v", err) + return + } + AppLogger.Printf("\nšŸ“‹ CSV report written to: %s\n", path) +} + +// writeMultiServiceCSVReport writes purchase results to a CSV file. It is a +// no-op returning nil when results is empty (no file is created). +func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string) (err error) { if len(results) == 0 { return nil } @@ -265,13 +281,20 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string return fmt.Errorf("failed to create CSV file: %w", err) } defer func() { - if err := file.Close(); err != nil { - log.Printf("Warning: failed to close CSV file %s: %v", filepath, err) + // A failed close can mean the data never reached the disk (e.g. on + // a network mount), so it fails the write rather than a warning. + if cerr := file.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close CSV file: %w", cerr) } }() - writer := csv.NewWriter(file) - defer writer.Flush() + return writeMultiServiceCSV(file, results) +} + +// writeMultiServiceCSV renders the purchase report to w, flushing before it +// returns so a failed flush is returned rather than lost in a defer. +func writeMultiServiceCSV(w io.Writer, results []common.PurchaseResult) error { + writer := csv.NewWriter(w) // Write header. RecommendedCount shows AWS's pre-sizing count alongside // Count (the post-sizing value); UpfrontPayment is rec.CommitmentCost, @@ -364,6 +387,10 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string } } + writer.Flush() + if err := writer.Error(); err != nil { + return fmt.Errorf("failed to flush CSV file: %w", err) + } return nil } diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index 4ec85e604..a5ed36af7 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -854,3 +854,60 @@ func TestLoadRecommendationsFromCSV_FileErrors(t *testing.T) { }) } } + +// failingWriter rejects every write, standing in for a full disk or a +// network mount that fails when the buffered CSV is flushed. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("disk full") } + +// Issue #2004: csv.Writer buffers everything, so the bytes only reach the +// file at Flush. A Flush failure must surface instead of a nil return. +func TestWriteMultiServiceCSV_FlushErrorIsReturned(t *testing.T) { + results := []common.PurchaseResult{{ + Recommendation: common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 1}, + Success: true, + Timestamp: time.Now(), + }} + err := writeMultiServiceCSV(failingWriter{}, results) + require.Error(t, err) + assert.Contains(t, err.Error(), "disk full") +} + +// Issue #2004: a run with no purchase results writes no file, so it must +// not claim "CSV report written to". +func TestWriteReportAndSummary_NoResultsDoesNotClaimCSVWritten(t *testing.T) { + var logs strings.Builder + previous := AppLogger.Writer() + AppLogger.SetOutput(&logs) + t.Cleanup(func() { AppLogger.SetOutput(previous) }) + + path := filepath.Join(t.TempDir(), "report.csv") + writeReportAndSummary(nil, nil, true, Config{CSVOutput: path}, nil) + + _, statErr := os.Stat(path) + require.ErrorIs(t, statErr, os.ErrNotExist) + assert.NotContains(t, logs.String(), "CSV report written to") + assert.Contains(t, logs.String(), "CSV report not written") +} + +// Issue #2004: a failed write must not be reported as written either. +func TestWriteCSVReport_FailureDoesNotClaimCSVWritten(t *testing.T) { + var logs strings.Builder + previous := AppLogger.Writer() + AppLogger.SetOutput(&logs) + t.Cleanup(func() { AppLogger.SetOutput(previous) }) + + results := []common.PurchaseResult{{ + Recommendation: common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 1}, + Success: true, + Timestamp: time.Now(), + }} + writeCSVReport(results, filepath.Join(t.TempDir(), "missing-dir", "report.csv")) + assert.NotContains(t, logs.String(), "CSV report written to") + + logs.Reset() + path := filepath.Join(t.TempDir(), "report.csv") + writeCSVReport(results, path) + assert.Contains(t, logs.String(), "CSV report written to: "+path) +}