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) + } + }) +}