Recover accounts broken by #118's config change, and stop hiding it - #144
Merged
OsherElhadad merged 5 commits intoSep 1, 2026
Merged
Conversation
added 2 commits
August 31, 2026 22:08
#118 moved extract_llm's per_output and cold_cache keys to the new extract_llm_sweep component and made config.LoadBytes refuse either one outright — "Breaking existing configs is deliberate... migrated by hand" per that component's own comment. That hand migration never happened for the accounts already running with either key set: on the very next request after the new binary shipped, buildTenantConfig started failing for them, and the proxy's fail-open guarantee took over — every one of their requests kept being forwarded, just with NO compaction applied, silently, until someone happened to read the journal rather than the dashboard (see the paired dash fix in this branch, which makes that failure visible on the page instead). This closes the gap the way it should have shipped with #118: not by loosening the refusal (it is the right call — a silently-reinterpreted cold_cache would be "the most expensive possible misreading of this config", per that same comment) but by performing the exact mechanical translation #118's own migration guidance already names, in code, so it happens once, automatically, at Open, and is provably correct before anything is written back. Nothing is deleted and nothing is guessed: per_output is dropped outright (its presence changed nothing to begin with — the sweep "now IS the warm/tail pass"), and cold_cache's settings are carried onto a new extract_llm_sweep entry, in both the components map and the pipeline list, in the position config.go's own "housellm" preset already uses. Every rewritten document is round-tripped through the same validator a user's own settings-page save already gets rejected by (Options.Validate — reused rather than a new dependency, since `tenant` importing `config` directly would cycle back through config's own tests) before it is ever written; a tenant whose config does not match the exact shape this recognizes is left untouched and logged loudly, never guessed at. Verified against a copy of the production database, through the real Open() -> config.Validate path, not a hand-rolled check: every account that used to fail to build now builds cleanly, with extract_llm_sweep present and the account's own tuning (its min_tokens, its trigger threshold) carried across untouched. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
proxy/tenancy.go's build() fails open on purpose when an account's stored configuration cannot be built into a pipeline — it forwards the request uncompacted rather than taking the account offline over a bad config row, and marks the row's preset "invalid" so the fact is at least recorded. Until now that marker was only ever visible in the proxy's own log: a request tagged this way looked, from the dashboard, exactly like ordinary uncompacted traffic. #118 turned this from a theoretical edge case into a live incident — nine accounts silently lost all compaction for hours because their stored config used a key that release removed, and the first anyone knew was a log line, not this page. Overview now counts InvalidConfigRequests in the same window as everything else (one more query in the errgroup already parallelizing Overview's independent reads — see that function's own comment), and the UI shows an unmissable banner above the headline tiles whenever it's nonzero, naming the count and pointing at Settings. This is not folded into Diagnostics: the fact it reports is "money is being spent right now with none of the savings this page exists to show", which is exactly the class of thing this page must not let go unnoticed again. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
added 3 commits
September 1, 2026 01:34
Independent review of the first draft found the same root cause behind every finding: a regex has no idea what it is looking at, so it broke in a different way for a trailing comma, a shared trigger block another component also owns, a comment line, or a key that sorts into a different position than expected. One of those was a real blocker: a flow-style pipeline where extract_llm was the LAST element has no trailing comma after it, so the substring replace meant to insert extract_llm_sweep was a silent no-op — and because the resulting document was still perfectly valid YAML that still built a pipeline, config.Validate could not catch it. The account's cold_cache setting was deleted, no sweep ever ran in its place, and the row's preset stopped reading "invalid" — silently defeating the dashboard banner this same PR adds to catch exactly this class of problem. Rewritten as a real decode -> structural edit -> encode round trip, the same shape config/form.go already uses for every settings-page save (yaml.NewEncoder with SetIndent(2)) — this is not a new pattern in the codebase, just the first migration to use it instead of hand-rolled text surgery. The rewrite finds extract_llm by walking the parsed components map, not by matching a shared trigger block; finds its pipeline position by comparing list elements, not literal punctuation; and never sees a comment line or a key's stored ordering at all, since the YAML parser has already resolved all of that before this code runs. Also handles cold_cache.enabled: false correctly now: dropped with nothing added, rather than refused as an unrecognized shape — the sweep it would have configured never ran, so there is nothing to migrate forward, per extract_llm.go's own migration note. And a save failure for one candidate (a locked row, a disk error) no longer stops the rest of the batch from getting their own turn. Twelve new or rewritten tests, each a direct regression for one of the shapes review found: extract_llm last in the pipeline (the blocker), a shared trigger block on another component, a multi-key trigger, a disabled cold_cache, a stray "cold_cache:" substring in a comment, an extract_llm_sweep that already exists, and the batch-isolation property under a real failure. Verified end-to-end against a fresh copy of the production database once more, through the same tenant.Open() -> config.Validate path as before: all nine previously- broken accounts still recover cleanly. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
Independent review found one more instance of the same silent-loss class the flow-pipeline bug already was: reading `enabled` out of a map[string]any with a bare `.(bool)` assertion is wrong for a real config. YAML 1.1 accepts yes/Yes/YES/on/On/ON/y/Y as true, but decoding one of those into `any` (rather than into a typed bool field, which is what config.LoadBytes's own pre-#118 struct did) resolves to a plain string, so the assertion silently reads it as false. An account whose sweep was genuinely running under one of those spellings would have had cold_cache dropped and no extract_llm_sweep added — a valid document that quietly stopped doing its job, exactly what this file's own header comment already names as the risk config.Validate cannot catch. Re-marshals the cold_cache sub-map and decodes it through a small typed struct with KnownFields(true) instead — the same decode path config.LoadBytes itself would take. This fixes the bool-word reading by construction and replaces the hand-rolled "count the extra keys" check with the decoder's own unknown-field rejection, so an account with max_calls or min_idle_seconds set is still correctly refused, now for a clearer reason. cold_cache: (null) and cold_cache: {} both resolve to the same zero value a plain enabled: false already did, so both now drop cleanly instead of being refused. Six new tests: all eight YAML 1.1 true/false spellings, null and empty cold_cache, a non-bool enabled value (refused, not coerced), and max_calls/min_idle_seconds (still refused, via the decoder this time). Re-verified end-to-end against a fresh copy of the production database once more: all nine previously-broken accounts still recover. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
TestSpendSurvivesRowEviction failed CI, deterministically, right now — not from anything in this branch, but because spendEvents built its fixture unconditionally `sessions` hours into the past, and this ran in the first few hours of a new calendar month. MonthToDateUSD always asks for the CURRENT real calendar month (time.Now(), not an injectable clock — see its own comment on why the rollup has to be real-time, not simulated), so a 10-hour-wide fixture straddling midnight on the 1st put its oldest sessions in the PREVIOUS month's tenant_spend row and only its newest in this one, and the test's month-to-date assertion only ever saw the smaller, wrong half. Confirmed by reproducing on main with this branch's own changes removed (git stash), independently twice, and by the arithmetic: at the exact times both reproductions ran, the number of sessions that had rolled into the new month matched the shortfall exactly (1 of 10, then 2 of 10, sessions × 4 turns × $0.25 each). Fixed by clamping the fixture's spacing to the room actually available since local UTC midnight on the 1st, so every session lands in the current month regardless of what day it is — exact for the ~99.9% of the month that isn't within a few hours of the boundary (spacing stays exactly one hour, unchanged), and still correct, just more tightly packed, for the sliver that is. Verified passing 5x in a row, with and without -race, at the exact moment (2026-09-01, within hours of midnight) that was failing before this. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
OsherElhadad
deleted the
fix/deprecated-config-migration-and-visibility
branch
September 1, 2026 02:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#118 removed two
extract_llmconfig keys (per_output,cold_cache) with no migration path for accounts already using them — deliberately, per that component's own comment ("Breaking existing configs is deliberate... migrated by hand"), but the hand migration never happened. The moment that binary shipped, every affected account's stored configuration started failing to build, and the proxy's fail-open design took over: their traffic kept flowing, just with zero compaction applied, silently, for as long as nobody happened to read the log line rather than the dashboard.This PR closes both halves of that gap.
1. Recover the accounts (
tenant/configmigrate.go)Performs the exact mechanical translation #118's own migration guidance already names —
cold_cache.min_tokensbecomes a newextract_llm_sweepcomponent with thatmin_tokens, added to both the components map and the pipeline (in the same positionconfig.go's own built-in preset already uses), andper_outputis dropped outright since its presence changed nothing to begin with.cold_cache.enabled: falseis handled too: dropped with nothing added, since the sweep it would have configured never ran. Runs once, automatically, the moment the control-plane database opens.Revision note: the first version of this did the rewrite with regexes over the raw YAML text. Independent review found a real blocker in that approach (a flow-style pipeline with
extract_llmas the last element has no trailing comma, so the substring-insert was a silent no-op — the resulting document was still valid YAML, soconfig.Validatecouldn't catch it, and the account's original setting was destroyed with nothing put in its place) plus several narrower gaps from the same root cause (a regex has no idea what it's looking at — a shared config field another component also owns, a comment line, a differently-ordered key all broke it differently). Rewritten as a real decode → structural edit → encode round trip, the same patternconfig/form.goalready uses for every settings-page save. Nothing is guessed either way: every rewritten document is round-tripped through the exact same validator a user's own settings-page save already gets checked against before it's ever written back, and an account whose config doesn't match a shape this recognizes is left untouched and logged loudly rather than mangled.Verified against a copy of the production database through the real code path (not a hand-rolled check) — every previously-broken account now builds cleanly, with its own tuning (its thresholds, its trigger values) carried across untouched.
2. Stop hiding it next time (
dash/overview.go,dash/ui/app.js)The failure mode that let this run silently for hours is generic, not specific to #118:
proxy/tenancy.go'sbuild()already marks a broken-config request's rowpreset = "invalid"— that data was already being captured, just never surfaced anywhere a human looks. Overview now counts these in the same window as everything else and shows an unmissable banner above the headline tiles whenever the count is nonzero, naming the count and pointing at Settings. This isn't specific to this one incident — it will catch the next config-breaking change too, whatever it is.Verification
go build ./...,go vet ./...clean.go test ./...— every package green.go test -race ./dash/... ./proxy/... ./cmd/... ./tenant/... ./config/...— clean, exceptdash'sTestSpendSurvivesRowEviction, which fails under-raceonmainwith this branch's changes removed too — pre-existing, unrelated, tracked separately, not something this PR introduced or should block on.tenant.Open()exactly as the real binary calls it — not simulated — both before and after the YAML rewrite.extract_llmlast in the pipeline, a sharedtriggerblock on another component, a multi-keytrigger, a disabledcold_cache, a straycold_cache:substring inside a comment, anextract_llm_sweepthat already exists, and the batch's per-tenant failure isolation under a real validation rejection.