fix(recall): overlap the shadow arm, split the confidence histogram by choice - #587
Merged
Merged
Conversation
…hadow arm, split the confidence histogram Three findings from the review of #581, all on the instant-recall path an operator hits the moment they enable the decision model. The spend ceiling skipped the free decider. affordRerank gates every backend on the LLM's token estimate, but the jev backend spends no LLM tokens (rankJev is unaccounted by design). On the outcomeFallback path, whose second rank call runs against real prior spend, an over-budget incident was refused the recall the free decider would have produced and then stopped by the same ceiling in the loop — neither recall nor investigation. The gate now consults effectiveBackend, the one resolution of Backend and Decider that rank also switches on, and lets jev through. Shadow still pays for the LLM and stays gated. Shadow mode doubled recall latency. The decider was asked only after the LLM had answered, so a slow or black-holed endpoint added up to its client timeout to every instant recall — twice per investigation on the fallback path — for a verdict already in hand. The shadow arm now runs while the LLM does and is joined after, so shadow costs max(LLM, decider) rather than their sum. Joined rather than abandoned, because the comparison is written to spend, which the loop reads once rank returns. Pinned by a model fake that refuses to answer until the decider has been asked: serial, it deadlocks into the context deadline. The confidence histogram mixed two distributions. It is the series the promotion procedure reads a fire bar off, and it recorded the decider's confidence in "none of these" alongside its confidence in a named candidate. On a corpus where most incidents have no runbook the none answers carry the mass, so a bar set "from data" landed above every candidate answer and in jev mode nothing fired. The rerank consumer now labels each sample choice=none|candidate, and the observability page says to read the bar off the candidate series. Refs #581
…n the overlap for real Review of the first commit, and the simplify pass, on the same branch. The jev free pass is reverted. The premise "jev spends no LLM tokens" holds for the rank call only: the recall it produces runs confirmRecall and verifyFindings, paid completions the ceiling never checks, so letting a crossed ceiling fire a free rank call would have it buy a verify it was about to refuse. affordRerank gates every backend again and says why. The gap that remains — a jev recall could be free if verify were budget-gated — is a design question for the ceiling, not a rerank change, and is filed separately. fireThreshold was the third copy of the rule effectiveBackend was introduced to own; it resolves through it now, and effectiveBackend is one expression. The shadow arm is its own method, rankShadow, so rank stays a three-line dispatcher; the join is a closure over two variables and a done channel rather than a struct on a buffered channel; and its comment no longer claims the loop reads spend.shadow — nothing outside tests does — but joins because the metric and the log line are what the arm exists to record. "The decider named a candidate" is one predicate, named, at the three sites that turned on it. The overlap test is a two-way rendezvous. The first version passed a serial decider-first order (the decider signalled, the model then proceeded); now the decider also waits for the model to have been entered, so either serial order deadlocks into the context deadline. Mutation-tested: serial decider first fails it in two seconds. The rendezvous decider embeds fakeDecider rather than re-implementing it, the histogram assertion compares counts and sums within an epsilon instead of exact float equality, and the two metrics test helpers in this package share one install-and-collect pair. The configuration page's promotion path now says to read the bar off the choice="candidate" series, the same correction the observability page got. Refs #581
Smana
changed the title
Smana
added a commit
that referenced
this pull request
Sep 26, 2026
…nt's body read, fix the backend docs (#588) * fix(curator): meter and guard the dedup decider, cap the decider client's body read, fix the backend docs Follow-up to the review of #581, the half that is not on the recall path. The decider client read an operator-configured endpoint's response with an unbounded io.ReadAll where httpx.ReadBody (2 MiB, ErrResponseTooLarge) is what every other backend client uses. A misconfigured base_url behind a proxy that streams an unbounded body was allocated in full on the recall path for a call that is a few KB of JSON. It also went through httpx.DoWithRetry at one attempt, which is a plain Do behind a retry loop that never loops, and read as though retries happened; it is a plain Do now, with the no-retry rationale kept. The curator's dedup decider calls were invisible in metrics. The reranker's land on runlore_model_requests_total{provider="decide"} with their result and latency; the curator's recorded only the confidence histogram, so a decider returning 401 on every curation fell back to BM25 behind a Warn line while the series the observability page points operators at stayed flat. Both consumers now record through telemetry.Metrics.RecordModelRequest, nil-safe, so the pair cannot be instrumented differently again. (The reranker's inline copy is swapped for the helper once #587 lands, to keep the two PRs from colliding.) The curator asked the decider on every curation that had ANY BM25 hit. A 200-entry catalog returns some hit for every query — at 0.02, an entry nobody would call related — so every filed finding made a paid third-party call, and carried the finding's text off-host, to ask whether it duplicated an obviously unrelated entry. The reranker guards its call with rerank_min_score; the curator now guards with the floor relatedEntries already applies, below which a hit is not even shown to the reviewer. The three places an operator reads about the backends had drifted, each differently. config.go omitted "shadow" from rerank_backend's values while config.Validate accepts it; config.go, configuration.md and values.yaml said the reranker's backend answers "same incident pattern?", which is the dedup question (the reranker asks which candidate, if any, is the runbook); and all three said the annotate tier files "with the suspect named in the body", after the fix moved the suspect into the PR/MR description only. All six are corrected, and TestDecisionBackendsReadTheSameInEveryPlaceAnOperatorLooks pins them: each source must name every value config.Validate accepts (probed, not trusted: a value outside the list must be refused by name), and neither stale phrase may return. Refs #581 * fix(curator): answer-per-question at the client, a dedup floor field, backend sets from config Review of the first commit, and the simplify pass, on the same branch. Decider client. The status is checked BEFORE the body is read, so a misrouted base_url's multi-MB error page reports its status and request id rather than "response too large". The too-large error is built from the sentinel with the remedy that applies (decision_model.base_url), instead of chaining the capped reader's "narrow the query" hint, which has no query to point at here. A 200 whose payload lacks a question that was asked is an error from the client — so every consumer sees the same failure and none re-detects it — replacing the weaker "no answers at all" check. Curator. The decider floor is a field, DedupMinScore, defaulting to the related-entries floor rather than the constant compared in place: a spend floor and a display floor will want to move apart, and the config knob is now one wiring change away. The dedup decider's missing-answer case counts as an error on the request series, since the fallback is identical and an ok would hide a vendor schema drift. Both change nothing for a curator built as before. Config. The accepted backend values are exported (RerankBackends, DedupBackends) and Validate ranges over them and builds its error message from them, so the docs guard reads the parser's own set rather than a copy — a copy is how `shadow` went undocumented, and the guard's earlier version would not have noticed a value added to the switch. Docs guard. Its config.go leg was mutation-tested and found weak: a window around the yaml tag was satisfied by Validate's own error string, and "naming the suspect in the body" did not match the "named in the body" needle. The config.go passage is now the field's doc comment, delimited exactly, and the stale claim is a regexp over both spellings. Against main's config.go the guard now reports all three drifts. Tests slimmed onto what the packages already had: the client tests use fixtureServer through one decideAgainst helper; the curator's metrics test reads two series through one collector with per-subtest provider install and absolute assertions, and pins the latency histogram as well as the counter; fakeDecider grew a noAnswer case instead of a third fake. Also: the observability page's provider="decide" row names both consumers, and the three backend descriptions state the dedup floor. Deferred, tracked in the PR: the five inline model-request metric sites (rerank ×2, summarize, loop, embed's recordRequest) onto RecordModelRequest; the seven per-package copies of the manual-reader test helper into one telemetry test package; embed's own unbounded body read. Refs #581
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #581, from its review: two of the three findings on the instant-recall path an operator hits the moment they enable the decision model. The third, the spend ceiling skipping the free
jevdecider, turned out to be the ceiling doing its job (see below).Shadow mode doubled recall latency
The decider was asked only after the LLM had answered, so a slow or black-holed endpoint added up to its client timeout to every instant recall, twice per investigation on the fallback path, for a verdict already in hand. The shadow arm is now its own method,
rankShadow, which asks the decider while the LLM runs and joins after: shadow costs max(LLM, decider) rather than their sum.Pinned by
TestRerankShadowAsksTheDeciderWhileTheLLMRuns, a two-way rendezvous: the model answers only once the decider has been asked, and the decider answers only once the model has been entered, so either serial order deadlocks into the context deadline. Mutation-tested: a serial decider-first arm fails it in two seconds. Race detector clean.The confidence histogram mixed two distributions
runlore_decision_model_confidenceis what the promotion procedure reads a fire bar off, and it recorded the decider's confidence in "none of these" alongside its confidence in a named candidate. On a corpus where most incidents have no runbook thenoneanswers carry the mass, so a bar set "from data" landed above every candidate answer and injevmode nothing fired.The
rerankconsumer now labels each samplechoice=none|candidate, and both the observability page and the configuration page's promotion path say to readrerank_threshold_jevoff thecandidateseries only. Pinned byTestDecisionConfidenceSeparatesNoneFromACandidate. Thededupconsumer is unchanged (a noul has no choice).Not changed: the ceiling still gates
jevThe review of #581 read the gate as skipping a call that costs nothing. The rank call does cost nothing, but the recall it produces runs
confirmRecallandverifyFindings, paid completions the ceiling never checks, so an over-budget investigation let through the gate would buy a verify it was about to refuse.affordReranknow says so. Whether ajevrecall should be free when verify is budget-gated is a ceiling design question, filed separately.Also in this PR
fireThresholdwas a third copy of the backend-resolution rule; it,rankand the newrankShadowall resolve through oneeffectiveBackend. "The decider named a candidate" is one predicate at the three sites that turned on it. The package's two metrics test helpers share one install-and-collect pair.Verified
TDD: each test failed first for its stated reason, then passed; the overlap pin was mutation-tested.
go test -raceon the package, then the full gate:go build,go vet,gofmt -lclean,go test ./...,hack/lint.shat 0 issues.