fix(middleware): budget aliasing, cache key collisions, retry panics, and gateway errors - #10
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideRefactors budget, cache, retry, and adapter middleware to fix subtle correctness bugs (client aliasing, cache key collisions and aliasing, retry backoff panics, and gateway 200-with-error-body handling), introduces observability and configurability for budgets, honors provider Retry-After, and refreshes documentation to match shipped tool support. Sequence diagram for gateway empty-response handling and non-retryable classificationsequenceDiagram
actor Caller
participant Adapter
participant Provider as llmgate.Provider
participant RetryMiddleware as retryClient
participant llmgate
Caller->>RetryMiddleware: Complete(ctx, req)
RetryMiddleware->>Adapter: Complete(ctx, req)
Adapter->>Provider: Complete(ctx, req)
Provider-->>Adapter: resp (Choices may be empty)
alt len(resp.Choices) == 0
Adapter->>Adapter: emptyResponseErr(req)
Adapter-->>RetryMiddleware: *llmgate.LLMError{Class: ErrClassGateway, Message: ..., Provider, RetryAfterDur}
RetryMiddleware->>llmgate: Classify(err)
llmgate-->>RetryMiddleware: ErrClassGateway
RetryMiddleware->>RetryMiddleware: defaultRetryIf(err) == false
RetryMiddleware-->>Caller: error (no retries)
else choices present
Adapter-->>RetryMiddleware: resp
RetryMiddleware-->>Caller: resp
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
29eb794 to
73000c0
Compare
… gateway errors
Four M1 correctness defects. None of them error — they all produce
plausible-looking wrong behaviour.
budget.New built one budgetClient and rewrote its inner pointer on every
application, so wrapping two clients returned the *same* object pointing
at whichever was wrapped last. A router composed the way the README
recommends would send every request to its fallback and never call the
primary. The write also raced the read in Complete. Counters now live in
a shared Ledger and each application gets its own wrapper. Ledger.Spent
and Ledger.Remaining make spend observable, which the dashboard and
control plane both need. Config.RefuseUnpriced closes the related hole
where a budget over unpriced models is silently unlimited, because an
unpriced call debits zero and the cap never trips.
The cache key omitted ToolChoice, Message.ToolCalls and ToolCallID. In a
tool loop the assistant turn requesting a call usually has empty Content
— the calls are the payload — so two different loop states hashed
identically and the second got the first's answer. "required" and "none"
collided for the same reason. Responses were also only shallow-copied,
leaving ToolCalls and their Input bytes aliased to the cached entry
despite a comment claiming otherwise.
sleepBackoff had two reachable panics: base<<attempt overflows int64
around attempt 35 and goes negative, which slips past the `> max` clamp
and reaches rand.Int63n with a non-positive bound; and a BaseDelay under
2ns makes the jitter bound zero, which panics the same way. Backoff now
saturates instead of shifting blind. It also honours Retry-After when the
provider sent one, clamped to MaxDelay so a mistaken header cannot park
the caller for an hour.
An empty response is now ErrClassGateway rather than an unclassifiable
"empty response". OpenAI- and Anthropic-compatible gateways answer HTTP
200 with an error envelope — z.ai returns {"code":500,"msg":"404
NOT_FOUND"} for a base URL missing its version segment — which arrives as
a successful call with zero choices. Classified Unknown, the retry
middleware failed open and burned four paid attempts on a permanent
configuration fault. The error now names the endpoint and model, and says
so explicitly when the base URL has no version segment.
Also fixes the docs drift: README linked a DESIGN.md that does not exist
and pointed the roadmap at another repo, and ROADMAP listed tool calling
as unimplemented three commits after it shipped.
88adb40 to
36bbaf4
Compare
Finishes M1. Four defects that all produce plausible-looking wrong behaviour rather than an error.
HAL-526 —
budget.Newaliased every client it wrappedWrapping two clients returned the same object, pointing at whichever was wrapped last. A router composed the way the README recommends would send every request to its fallback and never call the primary — silently. The write also raced the read in
Complete.Counters now live in a shared
Ledger; each application gets its own wrapper.Ledger.Spent()/Remaining()make spend observable, which the dashboard and control plane both need and which previously had no API at all.Config.RefuseUnpricedcloses the related hole: an unpriced call debits zero, so a budget over unpriced models never trips however much is really spent.HAL-527 — cache keys collided inside tool loops
The key omitted
ToolChoice,Message.ToolCallsandMessage.ToolCallID. In a tool loop the assistant turn requesting a call usually has emptyContent— the calls are the payload — so two different loop states hashed identically and the second request got the first's answer.ToolChoice: "required"and"none"collided for the same reason.Responses were also only shallow-copied, leaving
ToolCallsand theirInputbytes aliased to the cached entry — despite the comment on that line claiming it was a defensive copy. A caller unmarshalling and rewriting arguments in place corrupted every later hit.Key is versioned (
v2) so old entries miss rather than collide.HAL-536 — two reachable panics in
sleepBackoffReachable with
MaxRetries: 40. Separately, aBaseDelayunder 2ns makesd/2 == 0and panics identically — exactly what someone writing a fast test would set.Backoff now saturates instead of shifting blind, and the jitter bound is guarded. It also honours
Retry-Afterwhen present (via a newLLMError.RetryAfterDurandllmgate.RetryAfter), clamped toMaxDelayso a mistaken or hostile header cannot park the caller for an hour.HAL-528 — gateway 200-with-error-body was retried 4×
Compatible gateways answer HTTP 200 with an error envelope. z.ai returns
{"code":500,"msg":"404 NOT_FOUND"}when the base URL lacks its version segment — which reaches the adapter as a successful call with zero choices, became a bare"empty response", classifiedUnknown, and the retry middleware fails open on Unknown. Four paid attempts on a permanent configuration fault, then an error that doesn't say what's wrong.New
ErrClassGateway(non-retryable), and the message now names the endpoint and model, with an explicit hint when the base URL has no version segment. The nine-line explanatory comment invectorless-engine/pkg/config/config.gois the evidence this was worth fixing.HAL-546 — docs drift
README linked a
DESIGN.mdthat doesn't exist and pointed the roadmap at another repo; ROADMAP still described tool calling as unimplemented three commits after it shipped.Verification
go build,go vet,golangci-lint,staticcheck— clean-shuffle=on, zero failuresShuffle earned its keep here: it caught an order dependency I had introduced in #9 —
TestRegisterBeatsNormalizationleft a global price override in place, so it could poisonTestLookupResolvesVariantsdepending on order. Fixed on that branch (29eb794) rather than papered over here.-racestill not runnable locally (no gcc); CI covers itCloses HAL-526
Closes HAL-527
Closes HAL-528
Closes HAL-536
Closes HAL-546
Summary by Sourcery
Fix middleware and error handling defects affecting budgeting, caching, retries, and gateway error classification, and align roadmap/documentation with current tool-calling support.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: