fix(curator): meter and guard the dedup decider, cap the decider client's body read, fix the backend docs - #588
Merged
Merged
Conversation
…nt'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
… 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: the half that is not on the recall path (that half is #587).
Decider client
httpx.ReadBody(2 MiB cap) instead of an unboundedio.ReadAll, after the status check, so a misroutedbase_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), not the capped reader's "narrow the query" hint.httpx.DoWithRetryat one attempt is a plainDobehind a loop that never loops; it is a plainDonow, rationale kept.Pinned by
TestDecideRefusesAnOversizedResponse,TestDecideReportsTheStatusBeforeTheBodySizeandTestDecideRequiresAnAnswerToEveryQuestionAsked.Curator
telemetry.Metrics.RecordModelRequest. Before, a decider returning 401 on every curation fell back to BM25 behind a Warn line while the documented series stayed flat. A missing answer counts as an error, since the fallback is identical. Pinned byTestDedupDeciderCallsAreMetered, counter and histogram both.DedupMinScore, a field defaulting to the related-entries floor. Before, every curation with any BM25 hit made a paid off-host call. A field rather than the constant compared in place, because a spend floor and a display floor will want to move apart; the config knob is one wiring change away. Pinned byTestDedupDeciderSkipsNoiseFloorHits.Config and docs
config.RerankBackendsandconfig.DedupBackendsare exported andValidateranges over them, building its error message from them, so the docs guard reads the parser's own set rather than a copy. A copy is howshadowwent undocumented.shadowwas missing from the field doc, the reranker's question was described as the dedup question in three places, all three said the suspect goes in the body, and none mentioned the dedup floor. Pinned byTestDecisionBackendsReadTheSameInEveryPlaceAnOperatorLooks, which was mutation-tested against main'sconfig.goand now reports all three drifts (its first version reported one).provider="decide"row names both consumers.Deferred
rerank.go×2,summarize.go,loop.go,embed.go'srecordRequest) ontoRecordModelRequest. Kept out of this PR so it does not collide with fix(recall): overlap the shadow arm, split the confidence histogram by choice #587 inrerank.go.internal/embedstill reads its response body unbounded before its status check, the same shape fixed here.Verified
TDD: each new test failed first for its stated reason, then passed. Full gate:
go build,go vet,gofmt -lclean,go test ./...,hack/lint.shat 0 issues.