diff --git a/internal/config/patrol_run_tally_test.go b/internal/config/patrol_run_tally_test.go index ceeb4122f..fa8ce231f 100644 --- a/internal/config/patrol_run_tally_test.go +++ b/internal/config/patrol_run_tally_test.go @@ -1,6 +1,8 @@ package config import ( + "errors" + "strings" "testing" "time" ) @@ -97,3 +99,44 @@ func TestPatrolRunTallyPrunesOldDays(t *testing.T) { t.Fatal("yesterday's tally day was pruned") } } + +// A failed read of the existing history must not block the save — the tally is +// telemetry — but the tally reset it causes has to be visible in the log, not +// silent. (A genuinely missing file loads as empty data with no error, so it +// never reaches the warning path.) +func TestPatrolRunTallyReadErrorWarnsAndKeepsSaving(t *testing.T) { + p := NewConfigPersistence(t.TempDir()) + now := time.Now().UTC() + + // Seed history whose tally holds a run that has already fallen off the + // capped run list: only a preserved tally would still count it. + runA := PatrolRunRecord{ID: "a", StartedAt: now.Add(-2 * time.Hour), CompletedAt: now.Add(-2 * time.Hour)} + if err := p.SavePatrolRunHistory([]PatrolRunRecord{runA}); err != nil { + t.Fatalf("SavePatrolRunHistory seed: %v", err) + } + + logs := captureConfigLogs(t) + mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")} + p.SetFileSystem(mfs) + + runB := PatrolRunRecord{ID: "b", StartedAt: now.Add(-1 * time.Hour), CompletedAt: now.Add(-1 * time.Hour)} + if err := p.SavePatrolRunHistory([]PatrolRunRecord{runB}); err != nil { + t.Fatalf("SavePatrolRunHistory with failing read must still save: %v", err) + } + if !strings.Contains(logs.String(), "daily run tally restarts") { + t.Fatalf("no tally-restart warning was logged; logs: %s", logs.String()) + } + + mfs.readError = nil + loaded, err := p.LoadPatrolRunHistory() + if err != nil { + t.Fatalf("LoadPatrolRunHistory: %v", err) + } + if len(loaded.Runs) != 1 || loaded.Runs[0].ID != "b" { + t.Fatalf("saved runs = %+v, want just run b", loaded.Runs) + } + // The tally restarted from the saved window: run a is gone, run b counted. + if got := loaded.PatrolRunsSince(now.AddDate(0, 0, -30)); got != 1 { + t.Fatalf("PatrolRunsSince = %d, want 1 after the tally restart", got) + } +} diff --git a/internal/config/persistence.go b/internal/config/persistence.go index c1b87c370..49d743ded 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -2480,9 +2480,16 @@ func (c *ConfigPersistence) SaveAIFindings(findings map[string]*AIFindingRecord) // SaveAIFindingsWithSuppression persists AI findings + explicit suppression rules to disk. // If suppressionRules is nil, existing suppression rules (if any) are preserved. func (c *ConfigPersistence) SaveAIFindingsWithSuppression(findings map[string]*AIFindingRecord, suppressionRules map[string]*AISuppressionRuleRecord) error { - // Preserve suppression rules if caller didn't provide them. + // Preserve suppression rules if caller didn't provide them. A missing file + // loads as empty data with no error, so a load error here is a real read + // failure: abort rather than rewrite the file without the user-authored + // rules it still holds. if suppressionRules == nil { - if existing, err := c.LoadAIFindings(); err == nil && existing != nil && existing.SuppressionRules != nil { + existing, err := c.LoadAIFindings() + if err != nil { + return fmt.Errorf("load existing AI findings to preserve suppression rules: %w", err) + } + if existing != nil && existing.SuppressionRules != nil { suppressionRules = existing.SuppressionRules } } @@ -3588,7 +3595,12 @@ func (c *ConfigPersistence) SavePatrolRunHistory(runs []PatrolRunRecord) error { LastSaved: now, Runs: runs, } - if existing, err := c.LoadPatrolRunHistory(); err == nil && existing != nil { + // A missing file loads as empty data with no error, so a load error here is + // a real read failure. The tally is telemetry, so keep the save — but say + // the tally is restarting rather than resetting it silently. + if existing, err := c.LoadPatrolRunHistory(); err != nil { + log.Warn().Err(err).Str("file", c.aiPatrolRunsFile).Msg("Failed to read existing patrol run history; daily run tally restarts from this save") + } else if existing != nil { data.DailyRuns = existing.DailyRuns data.RunTallyThrough = existing.RunTallyThrough } diff --git a/internal/config/persistence_ai_suppression_test.go b/internal/config/persistence_ai_suppression_test.go index c6696eef0..04abc9df2 100644 --- a/internal/config/persistence_ai_suppression_test.go +++ b/internal/config/persistence_ai_suppression_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "testing" "time" @@ -45,3 +46,54 @@ func TestSaveAIFindings_PreservesSuppressionRulesWhenNotProvided(t *testing.T) { require.Contains(t, loaded.SuppressionRules, "rule1") require.Equal(t, "Ignore for now", loaded.SuppressionRules["rule1"].Description) } + +// A failed read of the existing findings file must abort the save: proceeding +// would rewrite the file without the user-authored suppression rules it still +// holds. (A genuinely missing file loads as empty data with no error, so it +// never reaches this path.) +func TestSaveAIFindings_ReadErrorAbortsInsteadOfDroppingSuppressionRules(t *testing.T) { + tempDir := t.TempDir() + cp := NewConfigPersistence(tempDir) + + findings := map[string]*AIFindingRecord{ + "f1": {ID: "f1", Title: "High CPU", DetectedAt: time.Now(), LastSeenAt: time.Now()}, + } + rules := map[string]*AISuppressionRuleRecord{ + "rule1": {ID: "rule1", ResourceID: "res-1", Description: "Ignore for now", CreatedAt: time.Now()}, + } + require.NoError(t, cp.SaveAIFindingsWithSuppression(findings, rules)) + + mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")} + cp.SetFileSystem(mfs) + + err := cp.SaveAIFindings(findings) + require.Error(t, err) + require.ErrorContains(t, err, "transient read failure") + + // The aborted save must have left the file untouched: once reads work + // again, the rules are still there. + mfs.readError = nil + loaded, err := cp.LoadAIFindings() + require.NoError(t, err) + require.Contains(t, loaded.SuppressionRules, "rule1") +} + +// Explicit suppression rules need no read of the existing file, so a failing +// read must not block that save. +func TestSaveAIFindings_ExplicitRulesSaveDespiteReadError(t *testing.T) { + tempDir := t.TempDir() + cp := NewConfigPersistence(tempDir) + + mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")} + cp.SetFileSystem(mfs) + + rules := map[string]*AISuppressionRuleRecord{ + "rule1": {ID: "rule1", ResourceID: "res-1", Description: "Ignore for now", CreatedAt: time.Now()}, + } + require.NoError(t, cp.SaveAIFindingsWithSuppression(nil, rules)) + + mfs.readError = nil + loaded, err := cp.LoadAIFindings() + require.NoError(t, err) + require.Contains(t, loaded.SuppressionRules, "rule1") +}