From 6688f6a026a9b4aa8e3bbeb576276bf84fc175ea Mon Sep 17 00:00:00 2001 From: agenticode <16611333+agenticode@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:28:18 +0900 Subject: [PATCH] feat(recommend): expose production dispositions through a Verdicts seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verdicts reports what the production recommender actually reached, without running pkg/decision a second time — a second evaluation can diverge from the answer production gave. Disposition (recommended / never-observed / insufficient-history / no-significant-change) sits on a separate axis from VerdictState (not-computed / computed). decision.Verdict stays unexported so v.Decision.Action does not compile and a dropped comma-ok yields an empty Action matching no real action. On the wire verdictState is always emitted and the verdict object omitted; UnmarshalJSON re-derives state from presence, so a forged computed still decodes to not-computed. Nothing can set VerdictComputed today. checkNoDisagreement enforces the full biconditional over 200 seeded scenarios in both call orders, including the silent side, and fails if any disposition count hits zero. The random corpus does not land on 29 samples, so TestVerdictsAgreeAtEveryGateBoundary walks each gate across its exact threshold plus a 101-step sweep through churn suppression. 8 of 8 mutations caught, coverage 95.3%. recommend.go is unmodified. Co-authored-by: kording <74226694+kording@users.noreply.github.com> --- pkg/recommend/VERDICT-FINDINGS.md | 379 ++++++++++++++++ pkg/recommend/verdict.go | 300 +++++++++++++ pkg/recommend/verdict_test.go | 695 ++++++++++++++++++++++++++++++ 3 files changed, 1374 insertions(+) create mode 100644 pkg/recommend/VERDICT-FINDINGS.md create mode 100644 pkg/recommend/verdict.go create mode 100644 pkg/recommend/verdict_test.go diff --git a/pkg/recommend/VERDICT-FINDINGS.md b/pkg/recommend/VERDICT-FINDINGS.md new file mode 100644 index 0000000..6a95106 --- /dev/null +++ b/pkg/recommend/VERDICT-FINDINGS.md @@ -0,0 +1,379 @@ +# `Recommender.Verdicts` — the seam, and the verdict it refuses to invent + +Closes the reachability gap named in `cmd/WIRING-FINDINGS.md` §6.4 bullet 1 and +requested by name in `pkg/backtest/backtest.go:416` / `pkg/backtest/FINDINGS.md` +§2. New code lives entirely in `pkg/recommend/verdict.go` (300 lines) and +`pkg/recommend/verdict_test.go` (695 lines). **`recommend.go` was not modified** +— `recommendOne` and `hpaCPUWorkloads` are already package-level, so the seam +needed no edit to the shipped surface at all. `go.mod` and `go.sum` unchanged; +the one new import is intra-repo. + +Package coverage **95.3 %**. `gofmt`, `go vet ./...`, `go build ./...`, +`go test -race -count=1 ./pkg/recommend/...` and `go test -race -short ./...` +(36 packages) all green. No existing test was touched. + +--- + +## 1. The finding: the production path does **not** compute a verdict + +This is the first deliverable and it is a negative result. + +`pkg/recommend` does not evaluate a decision-quality judgement anywhere, and +the gap is not a wiring oversight — it is structural. Evidence, four ways: + +**1.1 No refusal predicate runs.** `pkg/decision` ships eight refusal codes. +Zero of them are evaluated on the production path: + +| `decision.RefusalCode` | Evaluated in `recommend`? | Why not | +|---|---|---| +| `insufficient-history` | **no** — see §1.2 | a silent skip, not a refusal | +| `post-change-soak` | no | `recommend` has no change/deploy events (`Evidence.LastChange`) | +| `class-unstable` | no | `patterns.Detector` yields a class; no stability fraction, no `LastClassFlip` | +| `signal-conflict` | no | `oomCount` exists, `ThrottledInWindow` does not; nothing is compared | +| `regime-change-pending` | no | `forecast.SpikeDetector` is not a CUSUM changepoint | +| `forecast-divergence` | no | one forecaster, no remote to disagree with | +| `sla-degraded` | no | no SLO signal reaches this package | +| `quarantined` | no | no quarantine state | + +**1.2 The one thing that looks like a refusal isn't one.** +`Recommendations` gates on `st.samples < cfg.MinSamples || window < cfg.MinWindow` +and, at default config, those are the same numbers `decision.Config` documents +itself as matching. That is a **coincidence of defaults between two +independently settable Configs**, not a shared computation — and a gate that +`continue`s produces no `Code`, no `Detail`, and no `Until`. A refusal is a +value you can cite; this is a branch you cannot see from outside the package. + +**1.3 No `Action` is chosen.** `decision.Decide` maps confidence onto +act / recommend-only against `Config.ActConfidence`. `recommend` has no such +field. The act threshold lives in `plan.Config.MinConfidence` and is applied +later, in a different package. There is no point on the production path where +"act vs recommend-only" exists to be read out. + +**1.4 Confidence is a bare float.** `Recommendation.Confidence` is +`bySamples × byWindow × (1−volatility)` — the very formula +`decision.Compose` reproduces "by construction" — but it is produced **without +a `Basis`**. There are no named terms on this path, and `pkg/explain` renders +`Confidence.Basis` terms as individually-grounded Drivers. Synthesising a +`decision.Confidence{Score: x}` with an empty basis would put a number in the +payload with nothing behind it. Worse: for churn-suppressed containers +`recommendOne` returns before `r.confidence` is ever called, so for that +disposition **no confidence value exists at all**. + +**Conclusion:** the honest deliverable is §6.4's second branch. The seam +exposes the dispositions production actually reached and reports the +decision-quality verdict as a typed absence. It does not call +`decision.Evaluate`, and it never will from this file — a second evaluation +site is a second answer. + +--- + +## 2. What production *does* reach, and now says out loud + +Per container it considered, `Recommendations` takes exactly one of four +branches. All four are real; three of them were invisible outside the package. + +```go +type Disposition string + +DispositionRecommended // a Recommendation was produced; Rec holds it +DispositionNeverObserved // no observed snapshot ever contained this container +DispositionInsufficientHistory // known, but under MinSamples or MinWindow (Samples 0 included) +DispositionNoSignificantChange // sizing ran; both dimensions inside MinChangeRatio +``` + +`never-observed` and `insufficient-history` are deliberately distinct. +`ObserveSnapshot` registers state for every container of every pod it sees, +with or without usage — so `insufficient-history` with `Samples: 0` is the +**collector gap** (pod known, no telemetry), while `never-observed` means the +snapshot handed to `Verdicts` was never itself observed. Collapsing them would +have hidden a broken collector behind "young workload". + +A `Disposition` is a report of a branch taken. **No `Disposition` is a +`decision.Refusal`**, and the type carries no `Code`/`Detail`/`Until` that +would let one be mistaken for the other. + +--- + +## 3. The seam + +```go +func (r *Recommender) Verdicts(snap *model.ClusterSnapshot) []Verdict + +type Verdict struct { + Key model.ContainerKey + Disposition Disposition + CurrentRequest model.Resources + CurrentLimit model.Resources + Samples int + Window time.Duration + FirstSample time.Time + LastSample time.Time + Rec *Recommendation // non-nil iff Disposition == DispositionRecommended + + state VerdictState // unexported + dec *decision.Verdict // unexported +} + +func (v Verdict) State() VerdictState // "not-computed" | "computed" +func (v Verdict) Decision() (decision.Verdict, bool) // the ONLY way in +``` + +- **Coverage** = `Recommendations`' eligibility filter exactly: Running pods + (empty phase counts as running), excluding `KindBarePod`/`KindJob`/`KindCronJob`, + deduplicated by container key. Ineligible containers are absent, because the + recommender never looked at them. +- **Order** is sorted by `Key.String()`. Deterministic: no map iteration + reaches the output, no `time.Now()` anywhere — everything derives from the + snapshot and the learned state. `Verdicts(nil)` returns nil. +- **Locking**: one `r.mu` acquisition for the whole walk, so no concurrent + `ObserveSnapshot` can split the answer across containers. +- `pkg/recommend` now imports `pkg/decision`, which is the direction + `pkg/decision`'s own package comment sanctions ("pkg/recommend imports this + package; this package never imports pkg/recommend"). No cycle. + +### 3.1 Why it is not `[]decision.Verdict` + +`pkg/backtest` asked for `func (r *Recommender) Verdicts(snap) []decision.Verdict`. +Returning that type would require every element to *be* a verdict, and §1 says +none exists. The returned `recommend.Verdict` **contains** an optional +`decision.Verdict` instead. When the evidence inputs arrive, the element type +does not change — only `state` and `dec` start getting set, in this one place. + +### 3.2 How the two absences are kept apart + +"No verdict was computed" and "a verdict was computed and it is a refusal" are +different facts, and the type is built so a caller cannot merge them: + +- The `decision.Verdict` is **unexported**. `v.Decision.Action` does not + compile; `Decision()`'s comma-ok is the only route. +- A caller who discards `ok` still cannot land anywhere: the zero + `decision.Verdict` has `Action: ""`, which equals none of `ActionAct`, + `ActionRecommendOnly` or `ActionRefuse`. Any `switch` on it falls through to + default. `TestVerdictNotComputedIsNotARefusal` asserts this explicitly. +- The zero `Verdict` (what a lookup miss yields) reports `not-computed`, not + an empty string. +- **On the wire**: `MarshalJSON` always emits `"verdictState"` and omits the + `"verdict"` object entirely when absent, so a JSON consumer reading + `.verdict.action` finds nothing rather than a falsy disposition. There is no + top-level `action`, `refusal` or `confidence` key to misread. +- `UnmarshalJSON` re-derives the state from whether a verdict object is + actually present, so a truncated or hand-edited document claiming + `"verdictState":"computed"` still decodes to `not-computed`. Tested. + +--- + +## 4. How divergence is made impossible + +`Verdicts` and `Recommendations` reach their answer through the **same +`recommendOne` call on the same locked state**, and the contract is that +`Disposition == DispositionRecommended` **iff** `Recommendations` reports that +key on the same snapshot, with `*v.Rec` equal to the served `Recommendation` +value. + +`checkNoDisagreement` (verdict_test.go) enforces the full biconditional on +every scenario — both directions, both call orders, no duplicates, and +critically **the silent side**: every non-recommended disposition must carry +`Rec == nil` *and* be absent from `Recommendations`. It also asserts that no +verdict ever claims a `decision.Verdict`. + +It runs over three corpora: + +1. `TestVerdictsAndRecommendationsCannotDisagree` — 200 seeded scenarios + (multi-workload, mixed kinds and phases, HPA-on-CPU, OOM restarts, sparse + and dense history). Exercised on the last run: **recommended 276, + insufficient-history 401, no-significant-change 103, never-observed 72** — + and the test *fails* if any disposition count is zero, so the proof can + never quietly stop covering the silent cases. +2. `TestVerdictsAgreeAtEveryGateBoundary` — each gate walked across its exact + threshold, because a property corpus samples the space but does not land on + boundaries, and off-by-one drift is how two paths actually come apart: + samples ∈ {0, 1, 28, 29, 30, 31}; window ∈ {MinWindow−1ns, MinWindow, + MinWindow+1ns, 2×MinWindow} (exact spans, asserted exact); and a 101-step + sweep of the current request through the churn-suppression boundary, which + must observe both sides. +3. `TestVerdictsAgreeAfterMutation` — agreement survives state changing + underneath: fresh samples, an OOM bumping the memory floor, and a `GC` that + drops every learned container. + +### 4.1 The proof was mutation-tested + +Each mutation was applied to `verdict.go`, the suite run, then reverted. All +eight were caught, by name: + +| Mutation | Caught by | +|---|---| +| sample gate off by one (`MinSamples-1`) | boundary sweep, `samples=29` | +| window gate off by one nanosecond | boundary sweep, `span=5h59m59.999999999s` | +| eligibility drift (CronJob considered) | property corpus seed 9 + eligible-set test | +| phase filter dropped (Pending considered) | property corpus seed 1 + eligible-set test | +| HPA-on-CPU guard inverted | recommendation value mismatch, seed 1 | +| churn mislabelled as `recommended` | property corpus seed 4 + disposition test | +| `never-observed` collapsed into `insufficient-history` | disposition-coverage assertion | +| fabricated `decision.Verdict` from `Decision()` | 3 tests, incl. the JSON absence test | + +The first two are the important ones: they are precisely the *silent* drift +that a happy-path agreement test would have missed. The corpus alone did **not** +catch the sample-gate mutation — that gap is why the boundary sweep exists. + +### 4.2 The one structural gap left, named + +`Recommendations` still has its own copy of the eligibility walk and the two +history gates; `Verdicts` does not delegate to it (and could not, without +either a second lock acquisition — which reintroduces the split-answer race — +or rewriting `Recommendations`' body, which is outside this unit's additive-only +scope). Agreement is therefore enforced by test, not by construction. + +**The finishing edit, for whoever next owns `recommend.go`** — five lines, +behaviour-preserving, and it makes divergence structurally impossible: + +```go +func (r *Recommender) Recommendations(snap *model.ClusterSnapshot) []Recommendation { + var out []Recommendation + for _, v := range r.Verdicts(snap) { + if v.Rec != nil { + out = append(out, *v.Rec) + } + } + return out +} +``` + +Note this also makes `Recommendations`' order deterministic (it currently +ranges a map). Nothing in the repo depends on that order — **this edit was +applied and verified here, not merely asserted**: `go build ./...` and +`go test -race -count=1 ./...` (36 packages, full not `-short`) both pass with +it in place. It was then reverted, because rewriting a shipped method's body +is outside this unit's additive-only scope while other agents build against +`pkg/recommend`. + +One further invariant `Verdicts` leans on, stated so it cannot rot silently: +**`recommendOne` returns nil exactly at the churn-suppression check and +nowhere else.** A new early return added there must add a `Disposition` with +it. This is documented on `DispositionNoSignificantChange`. + +--- + +## 5. What `cmd/kilter/explain.go` and `pkg/explain` must now do + +`pkg/explain` needs **no change**. `ExplainRequest` already has `Verdict +*decision.Verdict` (payload.go:130), `Explanation` already has `Action`, +`Confidence`, `Refusal` and `Notes` (payload.go:89–110), and `ActionUnknown` +already exists for exactly this case. The whole wiring is in `cmd/`. + +In `cmd/kilter/explain.go`, add the `github.com/agenticode/kilter/pkg/decision` +import (the file does not import it today — only a comment at line 449 mentions +it) and replace the `Recommendations` scan at lines 400–406 with a `Verdicts` +scan: + +```go +var found *recommend.Recommendation +var verdict *decision.Verdict +var note string +for _, v := range rec.Verdicts(series[len(series)-1]) { + if v.Key != key { + continue + } + found = v.Rec // nil unless DispositionRecommended — same value as before + if d, ok := v.Decision(); ok { + verdict = &d + } else { + // Do NOT synthesise one. Say which branch production took. + note = "no decision verdict was computed on the production path; " + + "the recommender's disposition for this container was " + string(v.Disposition) + } + break +} +req := explain.ExplainRequest{ …, Rec: found, Verdict: verdict} +``` + +`ExplainRequest` has no note input, so attach it to the built payload — after +`BuildExplain`, before `Verify`. That is safe: `Explanation.Verify` +(payload.go:399) re-resolves `Citations` only and does not read `Notes`, and +`Prose` already renders notes (payload.go:622): + +```go +if note != "" { + payload.Notes = append(payload.Notes, note) +} +``` + +Three rules for that wiring, all of them load-bearing: + +1. **`Action` stays `unknown` today.** `Decision()` returns `ok == false` for + every verdict this package currently produces, so `Verdict` stays nil and + `BuildExplain` keeps `ActionUnknown`. That is correct and it is the point: + `unknown` is true, and `refuse` would not be. The payload gets *better* + because it can now say **why** it is unknown. +2. **Never map a `Disposition` onto a `decision.Action` or a + `decision.Refusal`.** `insufficient-history` the disposition is not + `CodeInsufficientHistory` the refusal (§1.2). Surface the disposition as a + `Note` — `Explanation.Notes` exists — or as its own field. Do not put it in + `Refusal`. +3. **`Refusal` stays nil until a verdict exists.** `payload.Verify` is the + publish gate, and a refusal with no evidence behind it has nothing to cite. + +The user-visible win now: `kilter explain` on a container the engine said +nothing about stops printing bare `unknown` and starts printing *which of the +four things happened*, with the sample count and window that caused it — +which is what a user running `explain` is asking for. Filling `Action` and +`Refusal` for real needs the evidence inputs in §6. + +--- + +## 6. What `pkg/backtest` can stop working around + +Three of its four open findings close against this seam. **All of them are +`pkg/backtest` edits and none was made here** — that package is another +agent's scope. + +- **§2 `eligibleContainers` (backtest.go:471–500) deletes.** `Verdicts` returns + exactly the eligible set, sorted by `Key.String()` — the same order + `eligibleContainers` sorts into. `TestScoredSetMatchesRecommenderEligibility` + becomes redundant with `TestVerdictsCoverExactlyTheEligibleSet`; keep + whichever, but the duplication that "will drift" is gone. +- **§5 `learnState` (backtest.go:414–422) deletes.** `Verdict.Samples`, + `.FirstSample` and `.LastSample` are the recommender's own counters, past its + own garbage guards, rather than a mirror that has to reproduce them. The + alternative `History(key)` accessor floated there is not needed. +- **§2's third consequence: the harness now knows which containers were + *considered and skipped*, and why** — `decide` can attribute + `NoSnapshot`/`NoHorizon` style skips against a real disposition instead of + inferring silence. +- **§3 does *not* close.** `EnforceDecisionRefusals` and `refusalCode` must + keep calling `decision.Evaluate` themselves, because §1 says there is still + no refusal on the production path to read. What changes is that the harness + can now state this precisely: its refusals are the harness's, not + production's, and `Verdict.State() == VerdictNotComputed` is the machine- + readable proof. **A scorecard must not present harness-evaluated refusals as + engine behaviour** — that is the same lie §6.4 refuses, one layer up. +- §4 (evidence fields nobody can fill) is unchanged: it needs collectors. + +--- + +## 7. What would make `VerdictComputed` real + +Nothing in this unit sets `state = VerdictComputed`; there is no exported +constructor that can, so the package cannot lie about it. The remaining work, +in order: + +1. **Give the recommender its evidence inputs.** `decision.Evidence` needs + `LastChange`, `LastChangepoint`, `ThrottledInWindow`, `HPAThrashPerHour`, + `ClassStability`/`LastClassFlip`, the two forecasts, `SLODegraded`, + `Quarantined`. `recommend` today holds only `Samples`, `Window`, + `LastSample`, `Class`, `OOMsInWindow` and `ShrinkIndicated`. The rest come + from the evidence substrate, so the shape is an optional evidence source on + the `Recommender`, populated by whoever constructs it. +2. **Move confidence to `decision.Compose`.** The three legacy terms + (`history-depth`, `window-span`, volatility) reproduce today's float + exactly, so this is back-compatible by construction — and it is what gives + `pkg/explain` a `Basis` to turn into grounded Drivers. +3. **Give `recommend` an act threshold** (or have `Verdicts` take a + `decision.Config`), so `Decide` can pick an `Action`. It must be the same + threshold `pkg/plan` uses, or `explain` will say "act" about something the + planner declines. +4. **Then, and only then**, `Verdicts` calls `decision.Decide` — once, on the + production path, inside the same lock — and sets `state`/`dec`. Every + consumer above already handles both states, so nothing downstream changes. + +Until step 4, `kilter explain` reports `unknown` — and now says which of four +things production actually did to get there. diff --git a/pkg/recommend/verdict.go b/pkg/recommend/verdict.go new file mode 100644 index 0000000..0d9a0c0 --- /dev/null +++ b/pkg/recommend/verdict.go @@ -0,0 +1,300 @@ +package recommend + +import ( + "encoding/json" + "sort" + "time" + + "github.com/agenticode/kilter/pkg/decision" + "github.com/agenticode/kilter/pkg/model" +) + +// This file implements the `Recommender.Verdicts(snap)` seam that +// pkg/backtest asked for by name (pkg/backtest/backtest.go, FINDINGS.md §2) +// and that cmd/WIRING-FINDINGS.md §6.4 blocked `kilter explain` on. +// +// It deliberately does NOT return []decision.Verdict, and the reason is the +// whole point of the seam. `pkg/recommend` does not compute a +// decision-quality verdict today: not one of pkg/decision's eight refusal +// predicates is evaluated anywhere on the production recommendation path, +// and no Action is chosen (act vs recommend-only is pkg/plan's threshold, +// applied later and elsewhere). Calling decision.Evaluate from here, with +// evidence assembled on the spot, would produce a second, parallel +// evaluation whose answer can differ from the one production actually +// served — an explain payload citing a verdict nobody acted on. §6.4 refuses +// that trade explicitly and so does this file. +// +// What production DOES reach, for every container it considered, is a +// Disposition: it recommended, or it stayed silent for one of three +// specific reasons. Those dispositions are real, they are currently +// invisible outside the package, and they are what this seam exposes. The +// decision-quality verdict is exposed as a typed absence — VerdictNotComputed +// — which a caller cannot accidentally read as "computed, and the answer is +// no". See VERDICT-FINDINGS.md. + +// Disposition is what the production recommendation path actually did with +// one container it considered on a given snapshot. It is a report of a +// branch taken, not a judgement: no Disposition is a decision.Refusal, and +// none of them carries a refusal Code, Detail or Until. +type Disposition string + +const ( + // DispositionRecommended: a Recommendation was produced and Rec holds + // it. Exactly the containers Recommendations reports. + DispositionRecommended Disposition = "recommended" + // DispositionNeverObserved: the container is eligible in this snapshot + // but the recommender holds no record of it at all — no observed + // snapshot has ever contained it. ObserveSnapshot registers state for + // every container of every pod it sees, with or without usage, so this + // means the snapshot handed to Verdicts was not itself observed. A pod + // that was observed but has learned nothing reports + // DispositionInsufficientHistory with Samples 0 instead; that is the + // collector gap, and the two are not the same fact. + DispositionNeverObserved Disposition = "never-observed" + // DispositionInsufficientHistory: the container is known but its + // learned history falls short of Config.MinSamples or Config.MinWindow + // (Samples 0 included), so the recommender stayed silent. This gate resembles decision.CodeInsufficientHistory and at + // default config uses the same numbers, but it is not that refusal: + // the two thresholds live in two independently settable Configs, and + // this path produces no Refusal value at all. + DispositionInsufficientHistory Disposition = "insufficient-history" + // DispositionNoSignificantChange: history was sufficient and sizing ran, + // but both dimensions landed within Config.MinChangeRatio of the current + // request, so the recommendation was suppressed as churn. + // + // Invariant this label depends on: recommendOne returns nil exactly at + // the churn-suppression check and nowhere else. A future early return + // added to recommendOne must add a Disposition here with it. + DispositionNoSignificantChange Disposition = "no-significant-change" +) + +// VerdictState says whether a decision-quality verdict (pkg/decision) exists +// for a container on the production path. It is a separate axis from +// Disposition on purpose: "we never evaluated the refusal predicates" and +// "we evaluated them and refused" are different facts, and a payload that +// cannot tell them apart is a payload that will eventually claim the second +// while meaning the first. +type VerdictState string + +const ( + // VerdictNotComputed: no decision.Verdict exists. This is what every + // Verdict reports today, because pkg/recommend evaluates no refusal + // predicate. It is NOT decision.ActionRefuse and must never be + // rendered as one. + VerdictNotComputed VerdictState = "not-computed" + // VerdictComputed: a decision.Verdict was reached on the production + // path and Decision returns it. Nothing produces this state yet; it is + // the shape the seam commits to, so that when the evidence inputs + // arrive the assignment happens here and in no other call site. + VerdictComputed VerdictState = "computed" +) + +// Verdict is one considered container's readout of the production path: +// which container, what the recommender did, the history it did it on, and +// whether a decision-quality verdict exists for it. +// +// The decision.Verdict is deliberately not an exported field. It is reachable +// only through Decision, whose comma-ok result a caller has to look at — so +// "absent" cannot be silently read as a zero-valued verdict whose Action +// happens to compare unequal to everything. +type Verdict struct { + Key model.ContainerKey `json:"key"` + Disposition Disposition `json:"disposition"` + + // CurrentRequest and CurrentLimit are the container's sizing as the + // snapshot reported it, for every disposition — including the ones with + // no Rec to read them off. + CurrentRequest model.Resources `json:"currentRequest"` + CurrentLimit model.Resources `json:"currentLimit"` + + // Samples, Window, FirstSample and LastSample are the learned history + // the disposition was reached on. Zero for DispositionNeverObserved. + // These are the counters pkg/backtest's learnState mirrors today. + Samples int `json:"samples"` + Window time.Duration `json:"window"` + FirstSample time.Time `json:"firstSample,omitzero"` + LastSample time.Time `json:"lastSample,omitzero"` + + // Rec is non-nil if and only if Disposition is DispositionRecommended, + // and is byte-for-byte the Recommendation Recommendations reports for + // this key on this snapshot. + Rec *Recommendation `json:"recommendation,omitempty"` + + // state and dec are unexported so that the only way to a decision + // verdict is Decision's comma-ok. See the type comment. + state VerdictState + dec *decision.Verdict +} + +// State reports whether a decision-quality verdict exists for this container. +func (v Verdict) State() VerdictState { + if v.state == "" { + return VerdictNotComputed + } + return v.state +} + +// Decision returns the decision-quality verdict the production path reached +// for this container, and whether one exists at all. +// +// ok is false whenever State is VerdictNotComputed, which is every verdict +// this package produces today. When ok is false the returned Verdict is the +// zero value, whose Action is the empty string — equal to none of +// decision.ActionAct, decision.ActionRecommendOnly or decision.ActionRefuse, +// so a caller that ignores ok still cannot land on a disposition by +// accident. Callers rendering a payload should treat !ok as "unknown" +// (explain.ActionUnknown), never as refusal. +func (v Verdict) Decision() (decision.Verdict, bool) { + if v.State() != VerdictComputed || v.dec == nil { + return decision.Verdict{}, false + } + return *v.dec, true +} + +// verdictJSON is the wire shape. verdictState is always present and the +// verdict object is absent unless one was computed, so a JSON consumer +// reading `.verdict.action` gets nothing rather than a falsy disposition. +type verdictJSON struct { + Key model.ContainerKey `json:"key"` + Disposition Disposition `json:"disposition"` + CurrentRequest model.Resources `json:"currentRequest"` + CurrentLimit model.Resources `json:"currentLimit"` + Samples int `json:"samples"` + Window time.Duration `json:"window"` + FirstSample time.Time `json:"firstSample,omitzero"` + LastSample time.Time `json:"lastSample,omitzero"` + Rec *Recommendation `json:"recommendation,omitempty"` + VerdictState VerdictState `json:"verdictState"` + Decision *decision.Verdict `json:"verdict,omitempty"` +} + +// MarshalJSON emits the verdict state explicitly and omits the decision +// verdict unless one exists. +func (v Verdict) MarshalJSON() ([]byte, error) { + out := verdictJSON{ + Key: v.Key, Disposition: v.Disposition, + CurrentRequest: v.CurrentRequest, CurrentLimit: v.CurrentLimit, + Samples: v.Samples, Window: v.Window, + FirstSample: v.FirstSample, LastSample: v.LastSample, + Rec: v.Rec, VerdictState: v.State(), + } + if dec, ok := v.Decision(); ok { + out.Decision = &dec + } + return json.Marshal(out) +} + +// UnmarshalJSON restores a verdict, keeping the two states distinct across +// the wire: a payload carrying no verdict object decodes to +// VerdictNotComputed regardless of what its verdictState field claimed, so a +// hand-edited or truncated document cannot manufacture a disposition. +func (v *Verdict) UnmarshalJSON(b []byte) error { + var in verdictJSON + if err := json.Unmarshal(b, &in); err != nil { + return err + } + *v = Verdict{ + Key: in.Key, Disposition: in.Disposition, + CurrentRequest: in.CurrentRequest, CurrentLimit: in.CurrentLimit, + Samples: in.Samples, Window: in.Window, + FirstSample: in.FirstSample, LastSample: in.LastSample, + Rec: in.Rec, state: VerdictNotComputed, + } + if in.Decision != nil { + dec := *in.Decision + v.state, v.dec = VerdictComputed, &dec + } + return nil +} + +// Verdicts reports what the production recommendation path did with every +// container it considered on snap — the seam pkg/backtest asked for. +// +// "Considered" is exactly Recommendations' eligibility filter: containers of +// Running pods, excluding bare pods and Job/CronJob, deduplicated by +// container key. Ineligible containers are absent from the result, because +// the recommender never looked at them. This is the filter pkg/backtest +// reimplements in eligibleContainers. +// +// The result is sorted by Key.String() and is a pure function of snap and +// the recommender's learned state — no clock, no map-iteration order. A nil +// snapshot returns nil. +// +// Agreement with Recommendations is the contract: for every returned +// Verdict, Disposition == DispositionRecommended if and only if +// Recommendations reports that key on the same snapshot, and Rec then equals +// that Recommendation exactly. Both derive their answer from the same +// recommendOne call on the same locked state, and +// TestVerdictsAndRecommendationsCannotDisagree pins them together. +func (r *Recommender) Verdicts(snap *model.ClusterSnapshot) []Verdict { + if snap == nil { + return nil + } + r.mu.Lock() + defer r.mu.Unlock() + + hpaCPU := hpaCPUWorkloads(snap) + + // Eligibility, byte-for-byte Recommendations'. Later replicas overwrite + // earlier ones for the same key, as there — mid-rollout divergence is a + // known wart (FINDINGS.md), and reproducing it is the point: this seam + // reports what production does, not what it should do. + type current struct{ req, lim model.Resources } + currents := map[model.ContainerKey]current{} + for i := range snap.Pods { + pod := &snap.Pods[i] + if pod.Phase != "" && pod.Phase != "Running" { + continue + } + switch pod.Workload.Kind { + case model.KindBarePod, model.KindJob, model.KindCronJob: + continue + } + for _, c := range pod.Containers { + key := model.ContainerKey{Workload: pod.Workload, Container: c.Name} + currents[key] = current{req: c.Requests, lim: c.Limits} + } + } + + keys := make([]model.ContainerKey, 0, len(currents)) + for key := range currents { + keys = append(keys, key) + } + sort.Slice(keys, func(i, j int) bool { return keys[i].String() < keys[j].String() }) + + out := make([]Verdict, 0, len(keys)) + for _, key := range keys { + cur := currents[key] + v := Verdict{ + Key: key, + CurrentRequest: cur.req, + CurrentLimit: cur.lim, + state: VerdictNotComputed, + } + + st := r.states[key] + if st == nil { + v.Disposition = DispositionNeverObserved + out = append(out, v) + continue + } + window := st.lastSample.Sub(st.firstSample) + v.Samples, v.Window = st.samples, window + v.FirstSample, v.LastSample = st.firstSample, st.lastSample + + if st.samples < r.cfg.MinSamples || window < r.cfg.MinWindow { + v.Disposition = DispositionInsufficientHistory + out = append(out, v) + continue + } + + hpaOwner, hpaOnCPU := hpaCPU[key.Workload] + if rec := r.recommendOne(key, st, cur.req, cur.lim, hpaOnCPU, hpaOwner, window); rec != nil { + v.Disposition, v.Rec = DispositionRecommended, rec + } else { + v.Disposition = DispositionNoSignificantChange + } + out = append(out, v) + } + return out +} diff --git a/pkg/recommend/verdict_test.go b/pkg/recommend/verdict_test.go new file mode 100644 index 0000000..6bae2e3 --- /dev/null +++ b/pkg/recommend/verdict_test.go @@ -0,0 +1,695 @@ +package recommend + +import ( + "encoding/json" + "math/rand" + "strings" + "sync" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/decision" + "github.com/agenticode/kilter/pkg/model" +) + +// verdictScenario builds a pseudo-random cluster that reaches every +// disposition: containers with enough history and an oversized request +// (recommended), containers sized right at their usage (churn-suppressed), +// containers with too few samples or too short a window, ineligible pods +// (Job/CronJob/bare/Pending), HPA-on-CPU workloads, OOM restarts, and +// sometimes a container the recommender has never been shown. +func verdictScenario(t *testing.T, seed int64) (*Recommender, *model.ClusterSnapshot) { + t.Helper() + rng := rand.New(rand.NewSource(seed)) + r := newRec(t) + + snap := &model.ClusterSnapshot{ClusterID: "test", Timestamp: t0.Add(72 * time.Hour)} + n := 4 + rng.Intn(6) + for i := 0; i < n; i++ { + kind, phase := model.KindDeployment, "Running" + switch rng.Intn(10) { + case 0: + kind = model.KindJob + case 1: + kind = model.KindCronJob + case 2: + kind = model.KindBarePod + case 3: + phase = "Pending" + } + ref := model.WorkloadRef{Kind: kind, Namespace: "ns" + itoa(rng.Intn(2)), Name: "wl" + itoa(i)} + key := model.ContainerKey{Workload: ref, Container: "app"} + + baseCPU := int64(50 + rng.Intn(900)) + baseMem := int64(64<<20) * int64(1+rng.Intn(16)) + + var req model.Resources + switch rng.Intn(3) { + case 0: // oversized: a shrink is indicated + req = model.Resources{ + MilliCPU: baseCPU * int64(4+rng.Intn(8)), + MemoryBytes: baseMem * int64(4+rng.Intn(8)), + } + case 1: // already sized at usage+headroom: churn suppression territory + req = model.Resources{ + MilliCPU: ceilInt64(float64(baseCPU) * 1.15), + MemoryBytes: ceilInt64(float64(baseMem) * 1.20), + } + default: // undersized: a growth is indicated + req = model.Resources{MilliCPU: baseCPU / 2, MemoryBytes: baseMem / 2} + } + var lim model.Resources + if rng.Intn(2) == 0 { + lim = model.Resources{MilliCPU: req.MilliCPU * 2, MemoryBytes: req.MemoryBytes * 2} + } + + uid := "pod-" + itoa(i) + snap.Pods = append(snap.Pods, model.PodSpec{ + UID: uid, Name: ref.Name + "-a", Namespace: ref.Namespace, + Workload: ref, Phase: phase, + Containers: []model.ContainerSpec{{Name: "app", Requests: req, Limits: lim}}, + }) + if rng.Intn(6) == 0 { + snap.Workloads = append(snap.Workloads, model.WorkloadInfo{ + Ref: ref, HasHPA: true, HPATargetsCPU: true, + }) + } + + hours := 0 + switch rng.Intn(4) { + case 0: // no usage at all: known container, nothing learned + case 1: + hours = 1 + rng.Intn(5) // under the 6h MinWindow + default: + hours = 8 + rng.Intn(60) + } + for h := 0; h < hours*12; h++ { + snap.Usage = append(snap.Usage, model.Usage{ + Key: key, PodUID: uid, + Timestamp: t0.Add(time.Duration(h*5) * time.Minute), + MilliCPU: baseCPU + int64(rng.Intn(40)), + MemoryBytes: baseMem + int64(rng.Intn(8<<20)), + }) + } + } + r.ObserveSnapshot(snap) + + if rng.Intn(3) == 0 && len(snap.Pods) > 0 { + i := rng.Intn(len(snap.Pods)) + snap.Pods[i].Containers[0].RestartCount = 1 + snap.Pods[i].Containers[0].LastOOMKilled = true + r.ObserveSnapshot(snap) + } + + query := snap + if rng.Intn(3) == 0 { + q := *snap + q.Pods = append(append([]model.PodSpec{}, snap.Pods...), model.PodSpec{ + UID: "pod-fresh", Name: "fresh-a", Namespace: "ns0", Phase: "Running", + Workload: model.WorkloadRef{Kind: model.KindDeployment, Namespace: "ns0", Name: "fresh"}, + Containers: []model.ContainerSpec{{Name: "app", Requests: model.Resources{MilliCPU: 100, MemoryBytes: 128 << 20}}}, + }) + query = &q + } + return r, query +} + +// checkNoDisagreement is the whole unit: Recommendations and Verdicts run +// over the same snapshot and the same locked state, and there is no input on +// which they can report different things — not about what was recommended, +// and not about what was refused. +func checkNoDisagreement(t *testing.T, r *Recommender, snap *model.ClusterSnapshot, label string) map[Disposition]int { + t.Helper() + + recs := r.Recommendations(snap) + vs := r.Verdicts(snap) + // Reverse the call order too: neither may leave state behind that + // changes the other's answer. + vsRev := r.Verdicts(snap) + recsRev := r.Recommendations(snap) + + if len(recs) != len(recsRev) || len(vs) != len(vsRev) { + t.Fatalf("%s: call order changed the answer: recs %d→%d, verdicts %d→%d", + label, len(recs), len(recsRev), len(vs), len(vsRev)) + } + + byKey := make(map[model.ContainerKey]Recommendation, len(recs)) + for _, rec := range recs { + if _, dup := byKey[rec.Key]; dup { + t.Fatalf("%s: Recommendations returned %s twice", label, rec.Key) + } + byKey[rec.Key] = rec + } + + seen := map[model.ContainerKey]bool{} + counts := map[Disposition]int{} + recommended := 0 + for _, v := range vs { + if seen[v.Key] { + t.Fatalf("%s: Verdicts returned %s twice", label, v.Key) + } + seen[v.Key] = true + counts[v.Disposition]++ + + rec, wasRecommended := byKey[v.Key] + switch v.Disposition { + case DispositionRecommended: + recommended++ + if !wasRecommended { + t.Fatalf("%s: %s: Verdicts says recommended, Recommendations never reported it", + label, v.Key) + } + if v.Rec == nil { + t.Fatalf("%s: %s: recommended verdict carries no Recommendation", label, v.Key) + } + if *v.Rec != rec { + t.Fatalf("%s: %s: verdict recommendation diverges from the served one:\n verdict %+v\n served %+v", + label, v.Key, *v.Rec, rec) + } + case DispositionNeverObserved, DispositionInsufficientHistory, DispositionNoSignificantChange: + // The cases that matter: a container the engine stayed silent + // about must not also appear in what it served. + if wasRecommended { + t.Fatalf("%s: %s: Verdicts says %q, Recommendations served %+v", + label, v.Key, v.Disposition, rec) + } + if v.Rec != nil { + t.Fatalf("%s: %s: disposition %q carries a Recommendation", label, v.Key, v.Disposition) + } + default: + t.Fatalf("%s: %s: unknown disposition %q", label, v.Key, v.Disposition) + } + + // No verdict may ever claim a decision production did not compute. + if got := v.State(); got != VerdictNotComputed { + t.Fatalf("%s: %s: state %q — pkg/recommend evaluates no refusal predicate, so no verdict exists", + label, v.Key, got) + } + if dec, ok := v.Decision(); ok { + t.Fatalf("%s: %s: Decision() claims a verdict %+v that the production path never reached", + label, v.Key, dec) + } + } + + if recommended != len(recs) { + t.Fatalf("%s: %d recommended verdicts for %d recommendations", label, recommended, len(recs)) + } + for key := range byKey { + if !seen[key] { + t.Fatalf("%s: %s was recommended but Verdicts never considered it", label, key) + } + } + return counts +} + +// TestVerdictsAndRecommendationsCannotDisagree is the divergence proof. It +// is the reason this seam reads the production path instead of re-evaluating +// it: every container, every disposition, every seed. +func TestVerdictsAndRecommendationsCannotDisagree(t *testing.T) { + total := map[Disposition]int{} + for seed := int64(1); seed <= 200; seed++ { + r, snap := verdictScenario(t, seed) + for d, n := range checkNoDisagreement(t, r, snap, "seed "+itoa(int(seed))) { + total[d] += n + } + } + // A property test that never reached a silent disposition would prove + // only that the happy path agrees. Require every branch. + for _, d := range []Disposition{ + DispositionRecommended, + DispositionNeverObserved, + DispositionInsufficientHistory, + DispositionNoSignificantChange, + } { + if total[d] == 0 { + t.Fatalf("corpus never reached disposition %q; the agreement proof does not cover it", d) + } + } + t.Logf("dispositions exercised: %v", total) +} + +// TestVerdictDispositions pins each branch to a hand-built cause, so a +// mislabelled disposition fails here with a name rather than as a count. +func TestVerdictDispositions(t *testing.T) { + find := func(t *testing.T, vs []Verdict, key model.ContainerKey) Verdict { + t.Helper() + for _, v := range vs { + if v.Key == key { + return v + } + } + t.Fatalf("no verdict for %s (have %d)", key, len(vs)) + return Verdict{} + } + + t.Run("recommended", func(t *testing.T) { + r := newRec(t) + ref := deployRef("web") + snap := mkSnap(ref, model.Resources{MilliCPU: 2000, MemoryBytes: 4 << 30}, model.Resources{}, 24, + func(i int) int64 { return 150 }, func(i int) int64 { return 300 << 20 }) + r.ObserveSnapshot(snap) + v := find(t, r.Verdicts(snap), model.ContainerKey{Workload: ref, Container: "app"}) + if v.Disposition != DispositionRecommended || v.Rec == nil { + t.Fatalf("got %q rec=%v, want recommended with a recommendation", v.Disposition, v.Rec) + } + if v.Samples != v.Rec.Samples || v.Window.Hours() != v.Rec.WindowHours { + t.Fatalf("verdict history %d/%v disagrees with the recommendation's %d/%vh", + v.Samples, v.Window, v.Rec.Samples, v.Rec.WindowHours) + } + }) + + t.Run("no-significant-change", func(t *testing.T) { + r := newRec(t) + ref := deployRef("tight") + // Usage flat at 100m/200Mi; request already at p95×headroom. + req := model.Resources{MilliCPU: 115, MemoryBytes: ceilInt64(float64(200<<20) * 1.20)} + snap := mkSnap(ref, req, model.Resources{}, 24, + func(i int) int64 { return 100 }, func(i int) int64 { return 200 << 20 }) + r.ObserveSnapshot(snap) + if got := len(r.Recommendations(snap)); got != 0 { + t.Fatalf("fixture is wrong: want a suppressed container, got %d recommendations", got) + } + v := find(t, r.Verdicts(snap), model.ContainerKey{Workload: ref, Container: "app"}) + if v.Disposition != DispositionNoSignificantChange { + t.Fatalf("got %q, want no-significant-change", v.Disposition) + } + if v.Samples < DefaultConfig().MinSamples { + t.Fatalf("suppressed container reported %d samples; it had enough history", v.Samples) + } + }) + + t.Run("insufficient-history-samples", func(t *testing.T) { + r := newRec(t) + ref := deployRef("young") + // 8h span (over MinWindow) but only 8 samples (under MinSamples). + key := model.ContainerKey{Workload: ref, Container: "app"} + snap := &model.ClusterSnapshot{ + ClusterID: "test", Timestamp: t0.Add(8 * time.Hour), + Pods: []model.PodSpec{{ + UID: "pod-1", Name: "young-a", Namespace: ref.Namespace, Workload: ref, Phase: "Running", + Containers: []model.ContainerSpec{{Name: "app", + Requests: model.Resources{MilliCPU: 2000, MemoryBytes: 4 << 30}}}, + }}, + } + for i := 0; i < 8; i++ { + snap.Usage = append(snap.Usage, model.Usage{ + Key: key, PodUID: "pod-1", Timestamp: t0.Add(time.Duration(i) * time.Hour), + MilliCPU: 100, MemoryBytes: 200 << 20, + }) + } + r.ObserveSnapshot(snap) + v := find(t, r.Verdicts(snap), key) + if v.Disposition != DispositionInsufficientHistory { + t.Fatalf("got %q, want insufficient-history", v.Disposition) + } + if v.Samples != 8 { + t.Fatalf("samples %d, want 8", v.Samples) + } + }) + + t.Run("insufficient-history-window", func(t *testing.T) { + r := newRec(t) + ref := deployRef("narrow") + // 4h of dense sampling: plenty of samples, window under MinWindow. + snap := mkSnap(ref, model.Resources{MilliCPU: 2000, MemoryBytes: 4 << 30}, model.Resources{}, 4, + func(i int) int64 { return 100 }, func(i int) int64 { return 200 << 20 }) + r.ObserveSnapshot(snap) + v := find(t, r.Verdicts(snap), model.ContainerKey{Workload: ref, Container: "app"}) + if v.Disposition != DispositionInsufficientHistory { + t.Fatalf("got %q, want insufficient-history", v.Disposition) + } + if v.Samples < DefaultConfig().MinSamples { + t.Fatalf("fixture is wrong: %d samples, wanted the window to be the binding gate", v.Samples) + } + if v.Window >= DefaultConfig().MinWindow { + t.Fatalf("window %v is not under MinWindow", v.Window) + } + }) + + t.Run("insufficient-history-no-usage", func(t *testing.T) { + r := newRec(t) + ref := deployRef("silent") + snap := mkSnap(ref, model.Resources{MilliCPU: 500, MemoryBytes: 1 << 30}, model.Resources{}, 0, + func(i int) int64 { return 0 }, func(i int) int64 { return 0 }) + r.ObserveSnapshot(snap) + v := find(t, r.Verdicts(snap), model.ContainerKey{Workload: ref, Container: "app"}) + // Observed, so it is known; nothing learned, so the gate is history. + if v.Disposition != DispositionInsufficientHistory || v.Samples != 0 { + t.Fatalf("got %q with %d samples, want insufficient-history with 0", v.Disposition, v.Samples) + } + }) + + t.Run("never-observed", func(t *testing.T) { + r := newRec(t) + ref := deployRef("fresh") + snap := mkSnap(ref, model.Resources{MilliCPU: 500, MemoryBytes: 1 << 30}, model.Resources{}, 24, + func(i int) int64 { return 100 }, func(i int) int64 { return 200 << 20 }) + // No ObserveSnapshot at all. + v := find(t, r.Verdicts(snap), model.ContainerKey{Workload: ref, Container: "app"}) + if v.Disposition != DispositionNeverObserved { + t.Fatalf("got %q, want never-observed", v.Disposition) + } + if v.Samples != 0 || !v.FirstSample.IsZero() || !v.LastSample.IsZero() { + t.Fatalf("never-observed carried history: %+v", v) + } + }) +} + +// TestVerdictsCoverExactlyTheEligibleSet pins the filter pkg/backtest +// reimplements in eligibleContainers: Running pods only, no bare pods, no +// Job/CronJob, deduplicated by container key. +func TestVerdictsCoverExactlyTheEligibleSet(t *testing.T) { + r := newRec(t) + mk := func(kind model.WorkloadKind, name, phase, uid string) model.PodSpec { + ref := model.WorkloadRef{Kind: kind, Namespace: "default", Name: name} + return model.PodSpec{ + UID: uid, Name: name + "-" + uid, Namespace: "default", Workload: ref, Phase: phase, + Containers: []model.ContainerSpec{{Name: "app", + Requests: model.Resources{MilliCPU: 100, MemoryBytes: 128 << 20}}}, + } + } + snap := &model.ClusterSnapshot{ClusterID: "test", Timestamp: t0, Pods: []model.PodSpec{ + mk(model.KindDeployment, "web", "Running", "a"), + mk(model.KindDeployment, "web", "Running", "b"), // same key: deduplicated + mk(model.KindStatefulSet, "db", "", "c"), // empty phase counts as running + mk(model.KindDeployment, "pending", "Pending", "d"), + mk(model.KindJob, "job", "Running", "e"), + mk(model.KindCronJob, "cron", "Running", "f"), + mk(model.KindBarePod, "bare", "Running", "g"), + }} + r.ObserveSnapshot(snap) + + var got []string + for _, v := range r.Verdicts(snap) { + got = append(got, v.Key.String()) + } + // Verdicts sorts by Key.String(); "Deployment/..." < "StatefulSet/...". + want := []string{"Deployment/default/web/app", "StatefulSet/default/db/app"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("considered %v, want %v", got, want) + } +} + +// TestVerdictNotComputedIsNotARefusal is the anti-collapse test: the two +// facts "no verdict was computed" and "a verdict was computed and it is a +// refusal" must stay distinguishable in Go and over the wire. +func TestVerdictNotComputedIsNotARefusal(t *testing.T) { + r := newRec(t) + ref := deployRef("web") + snap := mkSnap(ref, model.Resources{MilliCPU: 2000, MemoryBytes: 4 << 30}, model.Resources{}, 24, + func(i int) int64 { return 150 }, func(i int) int64 { return 300 << 20 }) + r.ObserveSnapshot(snap) + vs := r.Verdicts(snap) + if len(vs) != 1 { + t.Fatalf("want 1 verdict, got %d", len(vs)) + } + v := vs[0] + + if v.State() != VerdictNotComputed { + t.Fatalf("state %q, want %q", v.State(), VerdictNotComputed) + } + dec, ok := v.Decision() + if ok { + t.Fatalf("Decision() reported a verdict production never computed: %+v", dec) + } + // A caller that drops the ok still cannot land on a disposition: the + // zero Action matches none of the three real ones. + for _, a := range []decision.Action{decision.ActionAct, decision.ActionRecommendOnly, decision.ActionRefuse} { + if dec.Action == a { + t.Fatalf("the absent verdict's Action equals %q — absence collapsed into a disposition", a) + } + } + if dec.Refusal != nil { + t.Fatalf("the absent verdict carries a refusal: %+v", dec.Refusal) + } + + // The wire form: verdictState says so, and there is no verdict object + // to misread. A JSON consumer reading .verdict.action finds nothing. + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatal(err) + } + if string(raw["verdictState"]) != `"not-computed"` { + t.Fatalf("verdictState = %s, want \"not-computed\"", raw["verdictState"]) + } + if _, present := raw["verdict"]; present { + t.Fatalf("a not-computed verdict serialized a verdict object: %s", b) + } + for _, forbidden := range []string{"action", "refusal", "confidence"} { + if _, present := raw[forbidden]; present { + t.Fatalf("not-computed verdict exposes %q at the top level: %s", forbidden, b) + } + } +} + +// TestVerdictJSONRoundTripKeepsAbsenceAbsent: a document with no verdict +// object decodes to not-computed even if its verdictState field says +// otherwise, so a truncated or hand-edited payload cannot manufacture one. +func TestVerdictJSONRoundTripKeepsAbsenceAbsent(t *testing.T) { + r := newRec(t) + ref := deployRef("web") + snap := mkSnap(ref, model.Resources{MilliCPU: 2000, MemoryBytes: 4 << 30}, model.Resources{}, 24, + func(i int) int64 { return 150 }, func(i int) int64 { return 300 << 20 }) + r.ObserveSnapshot(snap) + v := r.Verdicts(snap)[0] + + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + var back Verdict + if err := json.Unmarshal(b, &back); err != nil { + t.Fatal(err) + } + if back.Key != v.Key || back.Disposition != v.Disposition || back.Samples != v.Samples { + t.Fatalf("round trip lost data: %+v vs %+v", back, v) + } + if back.Rec == nil || *back.Rec != *v.Rec { + t.Fatalf("round trip lost the recommendation") + } + if _, ok := back.Decision(); ok || back.State() != VerdictNotComputed { + t.Fatalf("round trip invented a verdict: state %q", back.State()) + } + + // A document that claims "computed" without carrying one stays absent. + lying := strings.Replace(string(b), `"verdictState":"not-computed"`, `"verdictState":"computed"`, 1) + if lying == string(b) { + t.Fatalf("fixture did not contain the verdictState field: %s", b) + } + var forged Verdict + if err := json.Unmarshal([]byte(lying), &forged); err != nil { + t.Fatal(err) + } + if _, ok := forged.Decision(); ok || forged.State() != VerdictNotComputed { + t.Fatalf("a document claiming \"computed\" with no verdict decoded to %q", forged.State()) + } + + // And the zero Verdict — the value a caller gets from a lookup miss — + // must read as not-computed, not as an empty disposition. + var zero Verdict + if _, ok := zero.Decision(); ok || zero.State() != VerdictNotComputed { + t.Fatalf("the zero Verdict reports state %q", zero.State()) + } +} + +// TestVerdictsAreSortedAndRepeatable: Go randomizes map iteration on every +// range, so repeating in one process is the real determinism test. +func TestVerdictsAreSortedAndRepeatable(t *testing.T) { + r, snap := verdictScenario(t, 7) + first, err := json.Marshal(r.Verdicts(snap)) + if err != nil { + t.Fatal(err) + } + for i := 0; i < 8; i++ { + vs := r.Verdicts(snap) + for j := 1; j < len(vs); j++ { + if !(vs[j-1].Key.String() < vs[j].Key.String()) { + t.Fatalf("run %d: unsorted at %d: %s then %s", i, j, vs[j-1].Key, vs[j].Key) + } + } + again, err := json.Marshal(vs) + if err != nil { + t.Fatal(err) + } + if string(again) != string(first) { + t.Fatalf("run %d differs from run 0:\n%s\n%s", i, first, again) + } + } +} + +func TestVerdictsNilSnapshot(t *testing.T) { + r := newRec(t) + if vs := r.Verdicts(nil); vs != nil { + t.Fatalf("nil snapshot returned %v", vs) + } +} + +// TestVerdictsConcurrentWithObserve runs under -race: Verdicts takes the +// same lock ObserveSnapshot and Recommendations do. +func TestVerdictsConcurrentWithObserve(t *testing.T) { + r, snap := verdictScenario(t, 11) + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + for j := 0; j < 20; j++ { + switch i % 4 { + case 0: + r.ObserveSnapshot(snap) + case 1: + _ = r.Recommendations(snap) + case 2: + _ = r.Verdicts(snap) + default: + for _, v := range r.Verdicts(snap) { + if _, ok := v.Decision(); ok { + t.Errorf("%s: verdict appeared under concurrency", v.Key) + return + } + } + } + } + }(i) + } + wg.Wait() +} + +// TestVerdictsAgreeAfterMutation: the agreement must survive state changing +// underneath — new samples, an OOM, and a GC that drops learned state. +func TestVerdictsAgreeAfterMutation(t *testing.T) { + r, snap := verdictScenario(t, 3) + checkNoDisagreement(t, r, snap, "initial") + + for i := range snap.Pods { + snap.Pods[i].Containers[0].RestartCount++ + snap.Pods[i].Containers[0].LastOOMKilled = true + } + r.ObserveSnapshot(snap) + checkNoDisagreement(t, r, snap, "after OOM") + + if n := r.GC(t0.Add(365 * 24 * time.Hour)); n == 0 { + t.Fatalf("GC dropped nothing; the mutation case is not exercised") + } + counts := checkNoDisagreement(t, r, snap, "after GC") + if counts[DispositionNeverObserved] == 0 { + t.Fatalf("after a full GC every container should be unknown again: %v", counts) + } +} + +// mkExactHistory builds a container with exactly `samples` usage points +// spanning exactly `span`, so a gate can be tested on its boundary rather +// than near it. +func mkExactHistory(t *testing.T, name string, req model.Resources, samples int, span time.Duration) (*Recommender, *model.ClusterSnapshot) { + t.Helper() + r := newRec(t) + ref := deployRef(name) + key := model.ContainerKey{Workload: ref, Container: "app"} + snap := &model.ClusterSnapshot{ + ClusterID: "test", Timestamp: t0.Add(span), + Pods: []model.PodSpec{{ + UID: "pod-1", Name: name + "-a", Namespace: ref.Namespace, + Workload: ref, Phase: "Running", + Containers: []model.ContainerSpec{{Name: "app", Requests: req}}, + }}, + } + for i := 0; i < samples; i++ { + off := time.Duration(0) + if samples > 1 { + // Integer math so the last point lands on exactly t0+span. + off = time.Duration(int64(span) * int64(i) / int64(samples-1)) + } + snap.Usage = append(snap.Usage, model.Usage{ + Key: key, PodUID: "pod-1", Timestamp: t0.Add(off), + MilliCPU: 100, MemoryBytes: 200 << 20, + }) + } + r.ObserveSnapshot(snap) + return r, snap +} + +// TestVerdictsAgreeAtEveryGateBoundary walks each gate across its exact +// threshold. A property corpus samples the space; it does not land on +// boundaries, and a gate that drifts by one is exactly how the two paths +// would come apart in practice. +func TestVerdictsAgreeAtEveryGateBoundary(t *testing.T) { + cfg := DefaultConfig() + oversized := model.Resources{MilliCPU: 4000, MemoryBytes: 8 << 30} + + t.Run("sample-count", func(t *testing.T) { + for _, samples := range []int{ + 0, 1, + cfg.MinSamples - 2, cfg.MinSamples - 1, cfg.MinSamples, cfg.MinSamples + 1, + } { + // A window well clear of MinWindow, so samples is the only gate. + r, snap := mkExactHistory(t, "samples", oversized, samples, 12*time.Hour) + label := "samples=" + itoa(samples) + checkNoDisagreement(t, r, snap, label) + + vs := r.Verdicts(snap) + want := DispositionRecommended + if samples < cfg.MinSamples { + want = DispositionInsufficientHistory + } + if vs[0].Disposition != want { + t.Fatalf("%s: disposition %q, want %q", label, vs[0].Disposition, want) + } + } + }) + + t.Run("window-span", func(t *testing.T) { + samples := 4 * cfg.MinSamples // never the binding gate + for _, span := range []time.Duration{ + cfg.MinWindow - time.Nanosecond, cfg.MinWindow, cfg.MinWindow + time.Nanosecond, + 2 * cfg.MinWindow, + } { + r, snap := mkExactHistory(t, "window", oversized, samples, span) + label := "span=" + span.String() + checkNoDisagreement(t, r, snap, label) + + vs := r.Verdicts(snap) + if vs[0].Window != span { + t.Fatalf("%s: window %v, want exactly %v", label, vs[0].Window, span) + } + want := DispositionRecommended + if span < cfg.MinWindow { + want = DispositionInsufficientHistory + } + if vs[0].Disposition != want { + t.Fatalf("%s: disposition %q, want %q", label, vs[0].Disposition, want) + } + } + }) + + // The churn gate has no fixed number to stand on — the target comes out + // of percentile math — so sweep the current request densely through it + // and require that both sides of the boundary were actually reached. + t.Run("change-ratio", func(t *testing.T) { + probe, probeSnap := mkExactHistory(t, "churn", oversized, 4*cfg.MinSamples, 24*time.Hour) + recs := probe.Recommendations(probeSnap) + if len(recs) != 1 { + t.Fatalf("probe produced %d recommendations, want 1", len(recs)) + } + target := recs[0].TargetRequest + + seen := map[Disposition]int{} + for step := 0; step <= 100; step++ { + scale := 0.80 + float64(step)*0.005 // 0.80 … 1.30 + req := model.Resources{ + MilliCPU: ceilInt64(float64(target.MilliCPU) * scale), + MemoryBytes: ceilInt64(float64(target.MemoryBytes) * scale), + } + r, snap := mkExactHistory(t, "churn", req, 4*cfg.MinSamples, 24*time.Hour) + label := "scale=" + itoa(step) + checkNoDisagreement(t, r, snap, label) + seen[r.Verdicts(snap)[0].Disposition]++ + } + if seen[DispositionRecommended] == 0 || seen[DispositionNoSignificantChange] == 0 { + t.Fatalf("sweep never crossed the suppression boundary: %v", seen) + } + }) +}