From 0e9cecb8cafe087cdc5efc661c91ee067804dccb Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 04:20:09 +0530 Subject: [PATCH 1/2] fix: suppress baseline comparisons across statistics resets Offline comparisons could expose cumulative deltas or report no material change after detecting a statistics reset or server restart. Apply the existing reset-suppression contract in Compute and terminate rendering after the unavailable-comparison explanation. Regression coverage includes the real SQLite baseline store, CLI/MCP-shared JSON, first and later resets, restarts, normal comparisons and eviction notices. Existing JSON field shapes remain unchanged. Closes #61 --- cmd/pgbot/diff_reset_regression_test.go | 89 +++++++++++++++++++ internal/diff/diff.go | 7 +- internal/diff/reset_regression_test.go | 71 +++++++++++++++ internal/render/diff.go | 5 +- internal/render/diff_reset_regression_test.go | 88 ++++++++++++++++++ 5 files changed, 257 insertions(+), 3 deletions(-) create mode 100644 cmd/pgbot/diff_reset_regression_test.go create mode 100644 internal/diff/reset_regression_test.go create mode 100644 internal/render/diff_reset_regression_test.go diff --git a/cmd/pgbot/diff_reset_regression_test.go b/cmd/pgbot/diff_reset_regression_test.go new file mode 100644 index 0000000..b7a5cfe --- /dev/null +++ b/cmd/pgbot/diff_reset_regression_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "encoding/json" + "path/filepath" + "testing" + "time" + + "github.com/pgrundev/pgbot/internal/model" + "github.com/pgrundev/pgbot/internal/store" +) + +// Exercise the actual SQLite store, offline resolver, and the JSON payload shared +// by the CLI and MCP. This test does not need a live PostgreSQL server. +func TestResolveDiff_ResetSafeJSON(t *testing.T) { + for _, mode := range []string{"first reset", "later reset", "restart", "no reset"} { + t.Run(mode, func(t *testing.T) { + path, fp := writeResetDiffSnapshots(t, mode) + r, err := resolveDiff(path, fp, time.Hour) + if err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(diffJSON(r)) + if err != nil { + t.Fatal(err) + } + var out struct { + Reset string `json:"stats_reset_between"` + Changes json.RawMessage `json:"changes"` + } + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatal(err) + } + if mode == "no reset" { + if out.Reset != "" || r.Deltas == nil || len(r.Deltas.Changes) != 1 { + t.Fatalf("normal stored comparison was lost: %s", raw) + } + return + } + if out.Reset == "" { + t.Fatalf("reset caveat missing: %s", raw) + } + if r.Deltas != nil || string(out.Changes) != "[]" { + t.Fatalf("reset-crossing JSON must contain an empty changes array: %s", raw) + } + }) + } +} + +func writeResetDiffSnapshots(t *testing.T, mode string) (string, string) { + t.Helper() + path := filepath.Join(t.TempDir(), "baselines.db") + st, err := store.Open(path) + if err != nil { + t.Fatal(err) + } + defer func() { + if err := st.Close(); err != nil { + t.Error(err) + } + }() + at := time.Now().UTC().Truncate(time.Second).Add(-3 * time.Hour) + fp := "reset-diff-test" + base := &model.Context{ + SchemaVersion: model.SchemaVersion, Fingerprint: fp, CollectedAt: at, + Server: model.ServerInfo{Database: "synthetic_test_database"}, + Queries: &model.Queries{Enabled: true, Top: []model.QueryStat{{QueryID: 42, MeanMS: 100, TotalMS: 1000}}}, + } + cur := &model.Context{ + SchemaVersion: model.SchemaVersion, Fingerprint: fp, CollectedAt: at.Add(2 * time.Hour), + Server: model.ServerInfo{Database: "synthetic_test_database"}, + Queries: &model.Queries{Enabled: true, Top: []model.QueryStat{{QueryID: 42, MeanMS: 1, TotalMS: 10}}}, + } + old, changed := at.Add(-time.Hour), at.Add(time.Hour) + switch mode { + case "first reset": + cur.Window.StatsResetAt = &changed + case "later reset": + base.Window.StatsResetAt, cur.Window.StatsResetAt = &old, &changed + case "restart": + base.Window.PostmasterStartAt, cur.Window.PostmasterStartAt = &old, &changed + } + for _, c := range []*model.Context{base, cur} { + if _, err := st.Save(c); err != nil { + t.Fatal(err) + } + } + return path, fp +} diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 8b47002..9eb51b1 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -33,11 +33,16 @@ const ( // Compute compares now against the primary baseline (yesterday is accepted for // FirstObserved dating but the primary drives the changes). Returns nil when -// there's nothing to compare against. +// there's nothing to compare against or the statistics reset between snapshots. func Compute(now *model.Context, primary *Baseline, _ *Baseline) *model.Deltas { if primary == nil || primary.Context == nil { return nil } + // Keep every consumer consistent with the baseline suppression contract. + // A reset is an unavailable comparison, not an improvement in counters. + if StatsResetBetween(primary.Context, now) != "" { + return nil + } d := &model.Deltas{Against: primary.CollectedAt, Changes: []model.Delta{}} base := primary.Context diff --git a/internal/diff/reset_regression_test.go b/internal/diff/reset_regression_test.go new file mode 100644 index 0000000..4781f96 --- /dev/null +++ b/internal/diff/reset_regression_test.go @@ -0,0 +1,71 @@ +package diff + +import ( + "testing" + "time" + + "github.com/pgrundev/pgbot/internal/model" +) + +func resetComparisonFixture() (*model.Context, *Baseline) { + at := time.Date(2026, 9, 20, 0, 0, 0, 0, time.UTC) + base := &model.Context{ + CollectedAt: at, + Queries: &model.Queries{Enabled: true, Top: []model.QueryStat{{QueryID: 42, MeanMS: 100, TotalMS: 1000}}}, + } + cur := &model.Context{ + CollectedAt: at.Add(24 * time.Hour), + Queries: &model.Queries{Enabled: true, Top: []model.QueryStat{{QueryID: 42, MeanMS: 1, TotalMS: 10}}}, + } + return cur, &Baseline{CollectedAt: at, Context: base} +} + +func TestCompute_SuppressesResetCrossingComparisons(t *testing.T) { + for _, name := range []string{"first stats reset", "later stats reset", "postmaster restart"} { + t.Run(name, func(t *testing.T) { + cur, base := resetComparisonFixture() + old := base.CollectedAt.Add(-time.Hour) + changed := base.CollectedAt.Add(time.Hour) + switch name { + case "first stats reset": + cur.Window.StatsResetAt = &changed + case "later stats reset": + base.Context.Window.StatsResetAt = &old + cur.Window.StatsResetAt = &changed + case "postmaster restart": + base.Context.Window.PostmasterStartAt = &old + cur.Window.PostmasterStartAt = &changed + } + if StatsResetBetween(base.Context, cur) == "" { + t.Fatal("fixture must contain a detected reset") + } + if got := Compute(cur, base, nil); got != nil { + t.Fatalf("reset-crossing comparison must be unavailable, got %+v", got.Changes) + } + }) + } +} + +func TestCompute_KeepsComparableSnapshots(t *testing.T) { + for _, name := range []string{"no reset metadata", "same reset metadata"} { + t.Run(name, func(t *testing.T) { + cur, base := resetComparisonFixture() + if name == "same reset metadata" { + old := base.CollectedAt.Add(-time.Hour) + base.Context.Window.StatsResetAt, cur.Window.StatsResetAt = &old, &old + base.Context.Window.PostmasterStartAt, cur.Window.PostmasterStartAt = &old, &old + } + got := Compute(cur, base, nil) + if got == nil || len(got.Changes) != 1 || got.Changes[0].ID != "query.mean_ms" { + t.Fatalf("comparable query-mean change was lost: %+v", got) + } + }) + } +} + +func TestCompute_MissingBaselineRemainsUnavailable(t *testing.T) { + cur, _ := resetComparisonFixture() + if Compute(cur, nil, nil) != nil || Compute(cur, &Baseline{}, nil) != nil { + t.Fatal("missing baseline must remain unavailable") + } +} diff --git a/internal/render/diff.go b/internal/render/diff.go index 29c3b8e..6931839 100644 --- a/internal/render/diff.go +++ b/internal/render/diff.go @@ -52,8 +52,9 @@ func DiffReport(w io.Writer, in DiffInput) { // (2) Reset / eviction suppression, reused from the same logic inspect uses. if in.ResetReason != "" { fmt.Fprintln(&b, st.crit("⚠ statistics were reset between these snapshots — "+in.ResetReason)) - fmt.Fprintln(&b, st.dim(" Cumulative deltas are meaningless across a reset; only same-moment gauges below are trustworthy.")) - fmt.Fprintln(&b) + fmt.Fprintln(&b, st.dim(" Comparison unavailable: statistics were reset or the server restarted between snapshots.")) + _, _ = io.WriteString(w, b.String()) + return } if in.PgssEvicted { fmt.Fprintln(&b, st.warn("⚠ pg_stat_statements evicted entries between these snapshots — query-level deltas may be incomplete")) diff --git a/internal/render/diff_reset_regression_test.go b/internal/render/diff_reset_regression_test.go new file mode 100644 index 0000000..163db4a --- /dev/null +++ b/internal/render/diff_reset_regression_test.go @@ -0,0 +1,88 @@ +package render + +import ( + "strings" + "testing" + "time" + + "github.com/pgrundev/pgbot/internal/model" +) + +func resetFixture() DiffInput { + base := time.Date(2026, 9, 20, 0, 0, 0, 0, time.UTC) + return DiffInput{ + Database: "synthetic_test_database", Fingerprint: "fixture123456", + BaselineAt: base, CurrentAt: base.Add(24 * time.Hour), + Requested: 24 * time.Hour, Actual: 24 * time.Hour, + } +} + +func TestDiffReport_ResetDoesNotClaimNothingChanged(t *testing.T) { + for _, tc := range []struct { + name string + deltas *model.Deltas + }{ + {name: "nil", deltas: nil}, + {name: "empty", deltas: &model.Deltas{}}, + } { + t.Run(tc.name, func(t *testing.T) { + in := resetFixture() + in.ResetReason = "server restarted; no comparison available" + in.Deltas = tc.deltas + var out strings.Builder + DiffReport(&out, in) + if !strings.Contains(out.String(), "statistics were reset") { + t.Fatalf("missing reset warning: %s", out.String()) + } + if strings.Contains(out.String(), "nothing material changed") { + t.Fatalf("unavailable comparison is presented as a clean result:\n%s", out.String()) + } + }) + } +} + +func TestDiffReport_ResetDoesNotPresentCumulativeDelta(t *testing.T) { + in := resetFixture() + in.ResetReason = "statistics reset; no comparison available" + in.Deltas = &model.Deltas{Changes: []model.Delta{{ + ID: "query.mean_ms", Subject: "synthetic-query", Before: 100, After: 1, + Note: "CUMULATIVE_DELTA_SENTINEL", Severity: "info", + }}} + var out strings.Builder + DiffReport(&out, in) + if strings.Contains(out.String(), "CUMULATIVE_DELTA_SENTINEL") { + t.Fatalf("invalid cumulative delta is still rendered after the reset warning:\n%s", out.String()) + } +} + +func TestDiffReport_NoResetStillReportsCleanComparison(t *testing.T) { + in := resetFixture() + var out strings.Builder + DiffReport(&out, in) + if !strings.Contains(out.String(), "nothing material changed") { + t.Fatalf("normal empty comparison changed: %s", out.String()) + } +} + +func TestDiffReport_NoResetStillReportsChanges(t *testing.T) { + in := resetFixture() + in.Deltas = &model.Deltas{Changes: []model.Delta{{ + ID: "query.mean_ms", Subject: "synthetic-query", Before: 1, After: 100, + Note: "VALID_DELTA_SENTINEL", Severity: "warn", + }}} + var out strings.Builder + DiffReport(&out, in) + if !strings.Contains(out.String(), "VALID_DELTA_SENTINEL") { + t.Fatalf("valid comparison was suppressed: %s", out.String()) + } +} + +func TestDiffReport_EvictionCaveatStillVisible(t *testing.T) { + in := resetFixture() + in.PgssEvicted = true + var out strings.Builder + DiffReport(&out, in) + if !strings.Contains(out.String(), "evicted entries") { + t.Fatalf("eviction warning disappeared: %s", out.String()) + } +} From 2b35d2caa5125c741c1e7d72da17a99258a0e6c5 Mon Sep 17 00:00:00 2001 From: Alex Shapalov Date: Mon, 12 Oct 2026 15:11:17 -0700 Subject: [PATCH 2/2] render: return the write error from the reset-suppressed diff branch #93 made DiffReport return the writer's error; the early return added here for the reset case predates that and no longer compiled once both landed. Co-Authored-By: Claude Fable 5.1 --- internal/render/diff.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/render/diff.go b/internal/render/diff.go index a6aa8e0..9a6002b 100644 --- a/internal/render/diff.go +++ b/internal/render/diff.go @@ -53,8 +53,7 @@ func DiffReport(w io.Writer, in DiffInput) error { if in.ResetReason != "" { fmt.Fprintln(&b, st.crit("⚠ statistics were reset between these snapshots — "+in.ResetReason)) fmt.Fprintln(&b, st.dim(" Comparison unavailable: statistics were reset or the server restarted between snapshots.")) - _, _ = io.WriteString(w, b.String()) - return + return writeTextReport(w, b.String()) } if in.PgssEvicted { fmt.Fprintln(&b, st.warn("⚠ pg_stat_statements evicted entries between these snapshots — query-level deltas may be incomplete"))