From a75de0f5ba7a537a7a395447e913c7ce0b8cd75c Mon Sep 17 00:00:00 2001 From: Osher Elhadad Date: Thu, 3 Sep 2026 06:23:08 +0000 Subject: [PATCH] fix(dash): let the invalid-config banner clear itself once the config is fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The banner above the headline reads present-tense — "Open Settings, fix the configuration, and save it again" — but it was driven by a count over the FILTER'S WINDOW, and the dashboard's default view has no window at all. So it reported an incident forever after it was resolved. On this deployment that is exactly what happened. All 1,752 invalid-config requests fall between 2026-08-31 17:56 and 2026-09-01 02:07 — a single afternoon, caused by a config key removed with no migration for the accounts already using it, and fixed since. Zero in the last 24 hours. Yet every viewer of the default view was still being told, days later, to go and repair a configuration that was already correct. An alarm that cannot switch itself off is one people learn to scroll past, which costs precisely what it was built to buy — and it was built after a real incident where nine accounts' compaction silently went to zero for hours. So the banner now reads InvalidConfigRecent: the same fact over the last hour, scoped to the account and to nothing else. An hour because a config that cannot build affects EVERY request for that account while it lasts, so the signal appears within seconds on any account with traffic — long enough that a quiet account still trips it, short enough that it clears on its own once someone fixes the config, without anyone needing to know that clearing it is a thing that needs doing. Facets are ignored as well as the time range, deliberately: filtering to one model must not be able to hide a live breakage showing up on another. InvalidConfigRequests is unchanged and still reported — the historical fact is worth having, it just is not a call to action. The new test asserts both halves: resolved history raises nothing, and a breakage happening now still raises it. Verified to fail if the recency term is dropped. Signed-off-by: Osher Elhadad --- dash/overview.go | 42 ++++++++++++++++++++++++++++++++++++++++++ dash/store_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ dash/ui/app.js | 12 +++++++++--- 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/dash/overview.go b/dash/overview.go index bb5226af..e3a6cc0a 100644 --- a/dash/overview.go +++ b/dash/overview.go @@ -3,6 +3,7 @@ package dash import ( "database/sql" "fmt" + "time" "golang.org/x/sync/errgroup" ) @@ -49,6 +50,16 @@ type WaterfallStep struct { // Overview is the payload behind the dashboard's headline. Every percentage here // is derived at read time from stored absolutes, so a rate change never rewrites // history and a filter change never needs a rebuild. +// invalidConfigRecentWindow is how recently a request must have run with a failed configuration +// for the dashboard to still call it a live problem. +// +// An hour, because the thing being detected is a stored config that cannot build: while it lasts +// EVERY request for that account is affected, so on any account with traffic the signal appears +// within seconds and keeps appearing. An hour is therefore long enough that a quiet account still +// trips it, and short enough that the banner clears on its own once someone fixes the config — +// without anyone having to know that clearing it is a thing that needs doing. +const invalidConfigRecentWindow = time.Hour + type Overview struct { Since int64 `json:"since"` Until int64 `json:"until"` @@ -65,6 +76,21 @@ type Overview struct { // zero before anyone noticed from a log line rather than from here. Nonzero here means the // account's Settings page needs attention now, not "check the logs". InvalidConfigRequests int64 `json:"invalid_config_requests"` + // InvalidConfigRecent is the same count over the last invalidConfigRecentWindow, scoped to + // the account but NOT to the filter's time range or any of its facets. It exists because + // InvalidConfigRequests alone cannot answer the question the banner above the headline asks, + // which is "is compaction broken RIGHT NOW". + // + // The dashboard's default view has no time filter at all, so a window-scoped count keeps + // reporting an incident forever after it is over: this deployment carried 1,752 such requests + // from a single afternoon (a config key removed with no migration, since fixed), and the page + // went on telling every viewer to "open Settings and fix the configuration" for days, about a + // configuration that was already correct. An alarm that cannot switch itself off is one people + // learn to scroll past, which costs exactly what it was built to buy. + // + // Facets are deliberately ignored, not just the time range: filtering to one model must not be + // able to hide a live breakage that happens to be showing up on another. + InvalidConfigRecent int64 `json:"invalid_config_recent"` TokensBefore int64 `json:"tokens_before"` TokensAfter int64 `json:"tokens_after"` @@ -680,6 +706,7 @@ func (d *DB) Overview(f Filter) (*Overview, error) { accountingM, cacheMissM, uncompressedM map[string]int64 p95cg, p95up float64 invalidConfigRequests int64 + invalidConfigRecent int64 ) var g errgroup.Group // See InvalidConfigRequests' own comment: preset is set to "invalid" by @@ -691,6 +718,20 @@ func (d *DB) Overview(f Filter) (*Overview, error) { return d.sql.QueryRowContext(d.readCtx(), `SELECT COUNT(*) FROM requests r WHERE `+cond+` AND r.preset = 'invalid'`, args...).Scan(&invalidConfigRequests) }) + // The same fact over a fixed recent window, which is what the banner is actually about. Built + // here rather than by copying f and overriding Since, so that it demonstrably carries the + // TENANT scope and nothing else: a facet leaking in could hide a live breakage, and dropping + // the tenant term would show one account another's problem. + g.Go(func() error { + rcond := "r.keepalive = 0 AND r.ts >= ? AND r.preset = 'invalid'" + rargs := []any{time.Now().Add(-invalidConfigRecentWindow).UnixMilli()} + if !f.TenantAll { + rcond += " AND r.tenant_id = ?" + rargs = append(rargs, f.Tenant) + } + return d.sql.QueryRowContext(d.readCtx(), + `SELECT COUNT(*) FROM requests r WHERE `+rcond, rargs...).Scan(&invalidConfigRecent) + }) // The replay ceiling's raw form, corrected below by the inflation query once both are in — // see the comment above `splitMoved` usage earlier in this function for the ceiling's own // derivation, and see the correlated-vs-window-function tradeoff explained where this query @@ -906,6 +947,7 @@ func (d *DB) Overview(f Filter) (*Overview, error) { o.Uncompressed = uncompressedM o.CGLatencyMsP95, o.UpstreamMsP95 = p95cg, p95up o.InvalidConfigRequests = invalidConfigRequests + o.InvalidConfigRecent = invalidConfigRecent o.SafetyCost.FrozenTokens = o.FrozenTokens o.SafetyCost.RestoredTokens = o.ExpandTokens diff --git a/dash/store_test.go b/dash/store_test.go index 16d8d7f7..cb55d658 100644 --- a/dash/store_test.go +++ b/dash/store_test.go @@ -489,6 +489,52 @@ func TestOverviewCountsInvalidConfigRequests(t *testing.T) { } } +// A resolved config incident must stop raising the alarm, which is what InvalidConfigRecent is for. +// +// The dashboard's banner is present-tense — "open Settings and fix the configuration" — but it was +// driven by a count over the filter's window, and the default view has NO window. So on this +// deployment 1,752 invalid-config requests from a single afternoon kept the banner up for days +// after the config was fixed, telling every viewer to go and repair something already correct. An +// alarm that cannot switch itself off is one people learn to ignore. +func TestInvalidConfigRecentIgnoresResolvedHistory(t *testing.T) { + db := openTestDB(t) + now := time.Now() + // A burst of breakage well in the past, and nothing wrong since. + old := mkEvent(now.Add(-48*time.Hour).UnixMilli(), "s-old", "m", 100, 100) + old.Preset = "invalid" + fine := mkEvent(now.Add(-time.Minute).UnixMilli(), "s-now", "m", 100, 90) + fine.Preset = "codesmart" + if err := db.insertBatch([]*Event{old, fine}); err != nil { + t.Fatal(err) + } + o, err := db.Overview(Filter{}) + if err != nil { + t.Fatal(err) + } + if o.InvalidConfigRequests != 1 { + t.Fatalf("invalid_config_requests = %d, want 1 (the historical fact must still be reported)", + o.InvalidConfigRequests) + } + if o.InvalidConfigRecent != 0 { + t.Errorf("invalid_config_recent = %d, want 0: a config fixed two days ago is not a live "+ + "problem, and the banner keyed on this must clear itself", o.InvalidConfigRecent) + } + + // ...and a breakage happening NOW must still raise it, or the fix has removed the alarm. + live := mkEvent(now.Add(-2*time.Minute).UnixMilli(), "s-live", "m", 100, 100) + live.Preset = "invalid" + if err := db.insertBatch([]*Event{live}); err != nil { + t.Fatal(err) + } + if o, err = db.Overview(Filter{}); err != nil { + t.Fatal(err) + } + if o.InvalidConfigRecent != 1 { + t.Errorf("invalid_config_recent = %d, want 1: a configuration failing to build right now is "+ + "exactly what this banner exists to surface", o.InvalidConfigRecent) + } +} + // TestNewInputRatioNeverDividesSavingsByThemselves is the guard the issue calls // non-negotiable: with no provider usage data the denominator would be `saved` // alone and the ratio would read ~100%. It must read n/a. diff --git a/dash/ui/app.js b/dash/ui/app.js index 2fc2e2b3..2ec10a8e 100644 --- a/dash/ui/app.js +++ b/dash/ui/app.js @@ -1477,10 +1477,16 @@ function renderTiles(o) { // proxy's own log. Shown above the headline, not folded into Diagnostics: the number it // reports is "your bill was bigger than it needed to be, right now", which is exactly what // this page exists to prevent from going unnoticed. - if (o.invalid_config_requests > 0) { + // Gated on the RECENT count, not the window's total, and that distinction is the whole point. + // The default view has no time filter, so a total keeps reporting an incident forever after it + // is fixed: this deployment carried 1,752 such requests from one afternoon and then told every + // viewer for days to go and fix a configuration that was already correct. The banner asks a + // present-tense question, so it reads a present-tense number and clears itself once someone + // fixes the config — see InvalidConfigRecent in dash/overview.go. + if (o.invalid_config_recent > 0) { host.appendChild(el('div', { class: 'banner bad', 'data-testid': 'invalid-config' }, - el('div', {}, el('strong', {}, num(o.invalid_config_requests) + ' request' - + (o.invalid_config_requests === 1 ? '' : 's') + ' ran with NO compaction. '), + el('div', {}, el('strong', {}, num(o.invalid_config_recent) + ' request' + + (o.invalid_config_recent === 1 ? '' : 's') + ' ran with NO compaction in the last hour. '), 'This account’s stored configuration failed to build, so every one of them was ' + 'forwarded as-is instead — traffic kept working, but none of it was compacted. ' + 'Open Settings, fix the configuration, and save it again.')));