Repository navigation
fix: reasoning headroom and zero-content fail-closed settlement on the free pool - #1225
Merged
Merged
Conversation
…ement on pooled aliases (#1171) Reasoning members of a load-balanced pool can spend the caller's entire max_tokens on hidden reasoning and answer finish_reason=length with no visible content, which settled as an ordinary full-price success. Two mechanisms close that. Headroom: provider_routes gains reasoning_reserve_tokens (migration 20260826_01); the three reasoning members of the hive-free pool carry 4096, the non-reasoning dots member stays at 0. SelectRoute surfaces the MAX reserve across eligible routes sharing the selected litellm_model_name, and edge-api inflates the completion-limit fields it dispatches upstream by that figure on every endpoint path, so visible content survives inside the caller's own budget. Zero-content guard (sync chat): an empty finish_reason=length completion is retried once against the same pool; if the retry is empty or fails too, the request settles fail-closed by capturing the reservation hold with terminal_usage_confirmed=false (the capture shape #1220 established for streams), raises hive_zero_content_captured_total, and sets the X-Hive-Upstream-Empty-Content response header so no SDK client is left guessing why content is empty. Metering nondeterminism investigated; findings-only in the PR body. Tests: offline selection guards, headroom units, orchestrator retry-success / capture / scope tests, live-Postgres-gated tests over the real seeded rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 54 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 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 |
1 of 2 tasks
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 26, 2026
…ee-lane timeouts (#1230) ## What The deploy SDK replay test `model field shows Hive alias not provider handle` timed out at 60s on the free lane. Since #1225 the free pool retries once on empty content and reasoning members inflate token budgets, so worst-case free-lane latency now exceeds vitest's default 60s per-test timeout (the sibling free-lane test passed at 26.5s). ## Change - The alias-echo test's purpose is the alias echo, not a free-lane exercise, so it now runs on `HIVE_TOOLS_MODEL` (default `deepseek-v4-flash`), mirroring the tools and response_format tests in the same file. - The one remaining test that intentionally uses the free `MODEL` gets an explicit `{ timeout: 120000 }` so future latency growth fails on assertion, not on the runner default. - Assertions are identical; no behavior change to what is verified. ## Test plan - [x] `tsc --noEmit -p tsconfig.json` on `packages/sdk-tests/js`: no new errors (the single pre-existing TS2339 on main reproduces identically, shifted lines only) - [ ] Live replay run against the deployed gateway (requires live stack; suite is deploy-gated) ## Buglog entry None: no bug fixed in shipped code, this is test-infrastructure retargeting.
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 26, 2026
…DeepSeek post-finish chunk (#1222) ## Summary Provider identity was leaking to customers on both `/v1/chat/completions` and `/v1/messages` (Anthropic-compat), streaming and non-streaming, on both OpenRouter-routed (DeepSeek) and Groq-routed aliases. An upstream response id format is provider-identifying by construction (`gen-*` = OpenRouter, `chatcmpl-*` with Groq's own suffix scheme = Groq), the same class of leak CLAUDE.md's provider-blind invariant forbids for error strings. Groq responses additionally carried a raw `system_fingerprint` field. This closes both leaks at every normalize boundary that builds a customer-facing response, and fixes a related DeepSeek stream defect in the same relay code. Evidence: `/tmp/reports/parity-inhive.md` (finding "Wire cosmetics", live parity run 2026-08-26) plus a fresh live negative-control capture against `api-hive.scubed.co` taken during this fix (see Verification below), reproducing the exact defect moments before the fix. ## What leaked and where - **`/v1/chat/completions` non-streaming** (`normalizeChatCompletion`): forwarded `resp.ID` verbatim. - **`/v1/chat/completions` streaming** (`executeStreaming` SSE relay): every chunk carried the upstream's own id, not just the terminal frame; `system_fingerprint` was never stripped. - **`/v1/completions` (legacy) non-streaming** (`normalizeCompletion`): same `resp.ID` passthrough. - **`/v1/messages` (Anthropic-compat) non-streaming** (`FromOAIResponse`): prefixed the raw upstream id with `msg_` instead of minting a fresh one, still shipping the upstream id inside the prefix. - **`/v1/messages` streaming** (`SSETranslator.FeedLine`): same prefix-not-mint bug for `message_start.id`, reused (correctly) for the rest of the stream but wrong at the source. - **DeepSeek-family streams** (both surfaces relay through the same OpenAI-shaped chunk): OpenRouter emits one extra empty `role`/`content` chunk immediately after the real `finish_reason` frame, before `[DONE]`. A strict SSE client that already closed the message on the real finish frame can choke on it. ## Fix - New `mintCompletionID(prefix)` (`apps/edge-api/internal/inference/mint_id.go`) mints a gateway-owned id (`chatcmpl-<uuid>` / `cmpl-<uuid>`), used by every OpenAI-shaped normalize site. `idPrefixForEndpoint` picks the right prefix per endpoint so a stream's minted id matches its non-streaming twin's shape. - `normalizeChatCompletion` and `normalizeCompletion` mint a fresh id and strip `system_fingerprint`; the original upstream id is kept in a local variable purely for the existing usage-clamp log line, never for anything client-facing. - `executeStreaming`'s relay loop mints **once per stream** and reuses that same id on every chunk, including the synthesized terminal usage chunk, so a client-visible id is stable across the whole response (matches its non-streaming twin's shape too). - `FromOAIResponse` and `SSETranslator.FeedLine` (Anthropic-compat) now always mint a fresh `msg_<uuid>`, never derive from the upstream chunk/response id. - `SanitizeVariablePriceFrame` (the map-based fallback sanitizer, shared by the streaming relay's rare unparseable-chunk path and `apps/edge-api/internal/chat`'s own verbatim relay) now also drops `id` and `system_fingerprint` as defense in depth, since it has no per-stream state to mint a stable replacement from. - New `shouldSuppressPostFinishChunk` / `chunkFinished` (pure, unit-tested) gate the DeepSeek post-finish chunk: any chunk arriving after a relayed `finish_reason` is dropped from the wire unless it carries `usage` (the legitimate `stream_options.include_usage` terminal frame, which always forwards). The suppressed chunk's content is still folded into the usage accumulator first, so billing never silently loses data, only the wire write is skipped. ## What this does NOT change (verified explicitly) 1. **Internal correlation.** Nothing reads `resp.ID` / `chunk.ID` for request or attempt correlation. That keys entirely on `attempt.ID`, a UUID this gateway mints independently at dispatch time (`orchestrator.go`/`stream.go` `StartAttempt`), completely decoupled from any upstream or client-facing response id, before and after this change. Confirmed by reading every caller of `RequestAttemptID`/`AttemptID` across `edge-api` and `control-plane`; none of them touch the response body's `id` field. 2. **Billing/settlement.** Settlement (`settleStream`, `ChargeUsage`, reservation release) also keys on `attempt.ID`/`reservation.ID`, never on the response id. The usage-clamp log line is the only place the *old* upstream id was ever read after this change, and it still gets it (via a locally kept `upstreamID`/`upstreamChunkID` variable), so diagnostic log fidelity is unchanged. 3. **Streaming usage semantics.** `NormalizeCacheUsage`'s wire-shape selection (`usage.CacheReadInputTokens != nil`) is untouched; no model-family branch was added anywhere in this diff. 4. **Id stability.** A minted id is generated once per response/stream and reused on every chunk (`mintedID` in `executeStreaming`, `t.messageID` in `SSETranslator`), and the OpenAI-shaped id prefix matches its endpoint's non-streaming shape. ## Verification **Unit tests** (new `mint_id_test.go`, updated `translate_response_test.go`/`stream_test.go` in `internal/anthropic`, updated `upstream_cost_hardening_test.go`): - Non-streaming mint + strip for both an OpenRouter `gen-*` fixture and a Groq `chatcmpl-*` + `system_fingerprint` fixture, asserting the minted id never equals the upstream id and the wire bytes contain neither the upstream id string nor `system_fingerprint`. - Legacy `/v1/completions` mint. - Anthropic non-streaming and streaming: minted id always starts with `msg_`, never equals or contains the upstream id, even when the upstream id already looks `msg_`-shaped. - `shouldSuppressPostFinishChunk`/`chunkFinished` against the **exact byte-for-byte chunk sequence captured live** on `api-hive.scubed.co` (see below): the real finish chunk forwards, the spurious trailing chunk suppresses, a genuine usage-only terminal chunk still forwards. - Full `go test ./apps/edge-api/...` passes (all packages green), run from this worktree's own `deploy/docker` (not the shared checkout, to avoid a stale-image false green). **Live negative control**, captured against the deployed (pre-fix) box moments before this fix, using the existing parity test key (`api_keys.id c4bc000e-3861-4941-8259-68714c1b6d47`, no rotation): - Non-stream `deepseek-v4-flash`: `"id":"gen-1787734315-Jo86gvcpmVkDttYHFhKc"` forwarded verbatim. - Non-stream `hive-free`: `"id":"gen-1787734317-qUFgWPoJsd4J75lctb2B"` forwarded verbatim (this pool member happened to route through a `gen-*` upstream on this call). - Stream `deepseek-v4-flash`: every one of 5 chunks carries the identical upstream id `gen-1787734329-THDkSN71nG5F8uAbPdnB`, AND the exact spurious post-finish chunk reproduces: `{"delta":{"role":"assistant","content":""},"finish_reason":null}` arrives immediately after the `"finish_reason":"length"` chunk, before `[DONE]`. This confirms the defect is real and live, and that the fix (verified in isolation by the unit tests above, against these same captured bytes) targets the actual bug. **What I could not do as builder**: deploy is orchestrator-only (this repo's coding pipeline, stage 10) since a push to `main` auto-deploys the live box. I have not verified the fixed code against the live box post-merge; that is the orchestrator's job at the deploy-confirmation step. The unit tests above exercise the real `mintCompletionID`/`shouldSuppressPostFinishChunk`/`chunkFinished`/`FromOAIResponse`/`SSETranslator` functions directly (not a reimplementation), including against the live-captured byte sequence, so I'm confident in the fix, but I want that boundary stated plainly rather than implied. ## Known residual (found, not fixed here — flagging per project convention rather than burying it) `apps/edge-api/internal/chat/dispatch.go` (the Open WebUI session-chat relay, a separate customer-facing surface from the OpenAI-compat `/v1/chat/completions` this PR fixes) has the same class of leak and is **not touched by this PR**: - For a variable-price alias, it already runs every frame through `SanitizeVariablePriceFrame`, so this PR's `id`/`system_fingerprint` deletion there also happens to close the leak on that path as a side effect. - For a **fixed-price alias** (e.g. Groq/`hive-free`), the relay is `_, _ = w.Write(line)` — genuinely verbatim, no sanitization of any kind, not even the model field. This is a live leak on the primary chat UI surface today. I did not touch `internal/chat` in this PR: it's a different package with its own settlement logic and no verification plan was scoped for it here, and I did not want to expand a security-sensitive diff's blast radius past what was asked and verified. Recommend a dedicated follow-up PR/issue scoped to that file specifically. ## Buglog entry See the commit message on this branch's `HEAD` for the full JSON line (heading "Buglog entry"), to be appended to `.wolf/buglog.jsonl` on `main` per this repo's convention (never on a feature branch). --- ## Update: security review round 2 (blocking findings addressed, rebased onto main) Rebased onto `origin/main` (27a4b31, twelve PRs merged while this branch was open, including #1220's streaming fail-closed settlement fix, #1221 cache affinity, #1225 free-pool headroom). Clean merge, no conflicts; full `edge-api` and `control-plane` suites green post-merge before any of the fixes below were applied. ### Blocking finding 1: `stream.go`'s second verbatim fallback — FIXED When a chunk failed typed decode AND the route was fixed-price, the raw upstream line shipped unsanitized (id, `system_fingerprint`, everything), with no log line. Fixed by collapsing both pricing branches onto the one map-based sanitizer (`SanitizeVariablePriceFrame`) the variable-price case already used: mint the id, strip `system_fingerprint`, drop-and-log anything unparseable, for every route. Verified by full diff review (single call site now, not two) plus the existing `SanitizeVariablePriceFrame` unit test suite, which covers exactly this shape. I did not build a new end-to-end test reaching this specific fallback inside `executeStreaming`: no test in this codebase currently drives that function end to end (confirmed by grep), and building that harness from scratch is a disproportionate lift for this fix; the shared sanitizer this fallback now unconditionally calls is what's under test. ### Blocking finding 2: `apps/edge-api/internal/chat/dispatch.go` fixed-price branch — FIXED Every SSE line was written raw with zero sanitization on the fixed-price branch (D-032 norm, so most aliases, the primary Open WebUI chat surface). Every data frame now runs through the same sanitizer, fixed-price included, with a per-stream minted id threaded through so a client sees one stable id across the whole stream. New test `TestDispatchFixedPriceStreamSanitizesUpstreamID` exercises the real `dispatch.ServeHTTP` end to end with a `FixedPricing` route and a fixture carrying `id`+`system_fingerprint` in the exact shape the live leak had. **Negative control, both findings**: stashed only the fix source files (kept the new/updated tests in place) and re-ran them against the pre-fix code. Both failed exactly as expected, reproducing the leak verbatim (`chatcmpl-8f3a9c2e1b4d` + `system_fingerprint` leaked on the dispatch.go test; the RAG test below leaked `up-1` with no minted id at all). Restored the fix, re-ran, both pass. ### Third leak, found while auditing per the review's instruction to assume one exists `apps/edge-api/internal/rag/chat_handler.go`'s `streamGroundedChat` (the `/v1/rag/chat` streaming path) relays chunks through a generic `map[string]any`, not a typed struct. Its own comments already state a "provider-blind" design intent, and it does drop provider names and `event:` lines, but nothing stripped `id` or `system_fingerprint` from the surviving map keys, so both leaked on every chunk. Fixed with the same mint-once-per-stream, strip-system_fingerprint treatment. Strengthened the existing `TestHandleChat_StreamingRelaysCitationsAndChunks` test (its fixture already carried `"id":"up-1"` on every chunk, but nothing asserted against it) to check the raw upstream id never appears and the same minted id repeats across both chunks. Same stash-based negative control applied and confirmed (see above). ### Full enumeration of every response-emitting path (as requested, so the next reader doesn't have to re-derive it) | Path | Sanitization status | |---|---| | `/v1/chat/completions` non-stream (`normalizeChatCompletion`) | SANITIZED — mint id, strip `system_fingerprint` (commit 1) | | `/v1/chat/completions` stream, typed-decode success (`executeStreaming` primary relay) | SANITIZED — mint once per stream, strip per chunk, post-finish suppression (commit 1, tightened per CodeRabbit) | | `/v1/chat/completions` stream, typed-decode FAILURE fallback | SANITIZED — unified onto the same sanitizer as above (commit 2, blocking finding 1) | | `/v1/chat/completions` stream, synthesized terminal usage chunk (upstream never sent usage) | SAFE by construction — gateway-built, never touches upstream bytes; id field updated for stability only, pre-existing logic otherwise untouched | | Legacy `/v1/completions` non-stream (`normalizeCompletion`) | SANITIZED — mint id (commit 1) | | Legacy `/v1/completions` stream | Same code path as chat completions stream (shared `executeStreaming`) — SANITIZED | | `/v1/responses` (OpenAI Responses API) | ALREADY SAFE pre-PR — mints its own `resp_` id independently, confirmed clean by security review, untouched | | `/v1/messages` (Anthropic-compat) non-stream (`FromOAIResponse`) | SANITIZED — always mints fresh `msg_` id, old code only prefixed the upstream id (commit 1) | | `/v1/messages` (Anthropic-compat) stream (`SSETranslator`) | SANITIZED — same fix, never relays raw bytes at all by construction (commit 1) | | `/v1/rag/chat` non-stream | ALREADY SAFE pre-PR — mints its own `ragchat-` id independently (existing code, confirmed while auditing) | | `/v1/rag/chat` stream (`streamGroundedChat`) | SANITIZED — third leak found this round, fixed (commit 2) | | OWUI session chat (JWT-authenticated `/v1/chat/completions`), variable-price branch | SANITIZED — already routed through the shared sanitizer, now with a stable minted id (commit 1) | | OWUI session chat, fixed-price branch | SANITIZED — blocking finding 2, fixed this round (commit 2) | | Non-data SSE lines (blank, `event:`, `[DONE]`) in any relay | SAFE by construction — no id/model/cost/provider fields on these lines | | `/v1/embeddings`, audio transcription/translation, image generation | SAFE by construction — none of these response types declare an `id` or `system_fingerprint` field at all (checked each typed struct) | | `/v1/batches` object metadata (`Batch.ID`) | SAFE — control-plane's own minted internal id, never an upstream identity (edge-api's batches client talks to control-plane, never to a provider directly) | | `apps/control-plane/internal/batchstore` local batch executor output file (`OutputLine.Response.Body`) | **NOT SANITIZED, known and disclosed, not fixed in this PR** — raw upstream JSON forwarded verbatim into the customer-downloadable batch output. Different service (control-plane, not edge-api), different settlement/output path, no verification plan scoped here. Recommend a dedicated follow-up PR/issue; flagged loudly rather than silently left out of this enumeration. | ### Settlement/billing verification (three specific requirements from the #1220 author) 1. **`HasForwardedChunk` on every forwarded frame, including the new sanitized fallbacks.** Verified by reading every `accumulator.HasForwardedChunk = true` call site in the diff: it is set immediately before every actual client write (primary typed-relay success, the newly-unified fallback's sanitized-success branch), and NOT set on any drop. No site was removed; the only new site is on the fallback's success path, paired 1:1 with its write. 2. **Missing-usage capture path stays `Held()`-full, plus alarm, plus completed status; the minted path cannot skip it.** `accumulator.Accumulate(chunk, aliasID)` and the `ClampUsage`/`RawUsageChunk` capture run unconditionally, BEFORE the post-finish suppression check (`if suppressPostFinish { continue }`). A suppressed (non-forwarded) chunk still fully feeds the settlement accumulator; suppression only skips the client-facing write. `shouldSuppressPostFinishChunk` also explicitly exempts any real usage-only terminal frame (usage set, zero choices) from suppression at all, so the actual terminal usage chunk is never touched by this logic in the first place. 3. **Synthesized terminal usage chunk — not new, not double-billing.** The `includeUsage && !accumulator.HasUsage` synthesis block predates every commit in this PR (present before commit 1). This PR's only change to it is the `ID` field (now reuses the stream's `mintedID` for id-stability rather than a fresh random uuid). The condition that triggers it, and the usage numbers it carries (`accumulator.ToUsageResponse()`), are untouched. This PR touches no settlement arithmetic anywhere: every hunk in `stream.go`'s diff sits inside the SSE relay loop or this synth-chunk block, none inside `settleStream`/`settlementCredits` (confirmed by `git diff origin/main...HEAD -- apps/edge-api/internal/inference/stream.go` hunk ranges). The synthesized chunk is written to the client-facing wire only; settlement's own usage numbers come from the `accumulator` fields directly, never by re-reading what was written to the client, so there is no read-back path for a future consumer (#1226) to double-count. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…1305) Closes #1283. ## What was broken Both defects were confirmed against production with the real OpenAI SDK v7.8.0 (workflow run 33209920195, 2026-08-28), so this is shipped behaviour, not a theoretical gap. 1. **`max_tokens` was not enforced.** `max_tokens: 8` on `hive-free` returned **1650 completion tokens**, and all 1650 were billed. The caller asked for 8 to bound their own spend and was charged roughly 200 times that. 2. **`n: 2` returned a single choice** with HTTP 200 and no indication the parameter had been dropped. ## The billing invariant, stated precisely > **A request that specifies `max_tokens: N` must never be billed for more than N completion tokens.** Scope of the guarantee, so nobody has to re-derive it from the diff: - It applies to every token-priced settlement branch on `/v1/chat/completions`, `/v1/completions` and `/v1/responses`: the measured charge, the content-estimate fallback, the sync zero-content hold capture, and the streaming missing-usage hold capture. - The ceiling is read from the caller's ORIGINAL body, before any outbound rewrite. `EnforceVariablePriceBounds` forces a ceiling Hive chose rather than one the caller asked for, so reading it afterwards would bound the charge by our own number and guarantee the caller nothing. - With both chat spellings present the SMALLER is used. The request is self-contradictory and no reading is provably what the caller meant, so the tie goes to the number that keeps the invariant literally true for whichever field they wrote. That under-charges Hive on a malformed request, the direction every estimate on this path already errs in. - Prompt tokens are never clamped. Only the completion side is bounded. - The bound only ever lowers a charge and never to zero: `CreditsForTokens` floors any non-zero quantity at one credit, so the fail-closed rules in D-034 and D-048 (no served request bills zero) are untouched. - **Carve-out, stated rather than left to be discovered:** a variable-price alias (`Pricing.IsUpstreamActual`) settles on the cost the upstream reported for the generation, not a token count times a catalog rate, so there is no per-token figure to cap. Its spend stays bounded by `EnforceVariablePriceBounds` forcing the outbound ceiling to `VariablePriceMaxCompletionTokens`, and by the hold clamp in control-plane's `finalizeLocked`. No route with a nonzero reasoning reserve is variable-priced today. Arithmetic is unchanged and still runs through `metering.ChargeCredits`, so every figure stays `math/big`. No float64 was introduced. ## Root cause of defect 1 `applyReasoningHeadroom`, shipped by PR #1225 for issue #1171 and live since 2026-08-26, inflated every completion-limit field the caller had set by `provider_routes.reasoning_reserve_tokens` before dispatch. That column is 4096 on three of the four `hive-free` pool members, so `max_tokens: 8` went upstream as `4104`. Its premise was that hidden reasoning would burn the reserve while visible content survived inside the caller's own budget. The premise is not enforceable: nothing in an OpenAI-compatible request tells an upstream that part of a ceiling is earmarked for hidden reasoning, so the model spends the inflated ceiling on whichever it emits first. On the reported request (`Write a long story about the sea.`) that was visible story text. The sharper finding is that the rewrite deliberately skipped a ceiling the caller had NOT set. Its entire effect was therefore overriding ceilings callers HAD set. It had no legitimate operating mode, so it is deleted rather than tuned. This was not a symptom patched at one call site. `applyReasoningHeadroom` had exactly three callers (`orchestrator.go` sync, `stream.go`, `stream_responses.go`); all three are gone, and the settlement bound is applied at each path's single metering seam rather than per branch. ## Root cause of defect 2 `N *int` was declared on both `ChatCompletionRequest` and `CompletionRequest` and read by nothing. The raw body was forwarded to LiteLLM, the parameter was ignored upstream, and one choice came back with HTTP 200. ## Where enforcement lives, and why **Both boundaries.** Either alone is a hope rather than a proof. **Request boundary.** The caller's ceiling now reaches the provider unchanged. This is the only boundary that can stop a caller receiving 1650 tokens they capped at 8, and it is what makes the reported usage honest rather than merely cheap. It also restores OpenAI semantics, where a reasoning model's hidden reasoning counts against the ceiling. **Settlement boundary.** The metered completion count is capped at the caller's own ceiling before anything reads it. It clamps the ONE usage object both the customer response and the charge derive from, and rewrites the already-marshalled body to match, so the number the caller reads and the number they pay for cannot diverge. Request-boundary correctness cannot carry the money guarantee on its own: it would depend on every upstream honouring a parameter we merely forwarded, on no future rewrite of the outbound body, and on no provider counting hidden reasoning into `completion_tokens`. PR #1225 is the proof that the outbound body only has to be wrong once. **A settlement-only fix was also rejected.** The caller would still receive and be shown a completion count they explicitly capped, and every OpenAI-compatible cost dashboard reads `usage.completion_tokens` unconditionally. ## What issue #1171 keeps, and what it loses Its zero-content guard, the fail-closed half, is untouched: an empty-content `finish_reason=length` completion on a reserving pool is still retried once and still captures rather than settling full price. `provider_routes.reasoning_reserve_tokens` keeps a reader as the flag that gates it, so no column is orphaned. What issue #1171 loses is the product half: a reasoning member given a very small ceiling can again answer with empty content. That outcome is now strictly cheaper than before this PR, because the capture that follows it is bounded by the caller's ceiling instead of charging the flat `DefaultHoldText` hold. Routing a small-ceiling request to a non-reasoning member, or sending `reasoning_effort: low`, are the honest ways to recover the product half and are follow-up work, not a silent rewrite of the caller's request. ## Defect 2 decision: reject, not support `n` other than 1 now returns 400 `unsupported_parameter` with `param: "n"` on both `/v1/chat/completions` and `/v1/completions`. Absent and `n: 1` pass through untouched; `n: 0` and negatives are refused too, which OpenAI also rejects. Rejecting rather than honouring, because no route in this catalog can serve `n > 1`: the free pool's Groq members accept only `n=1` on their OpenAI-compatible surface, and OpenRouter does not implement the parameter across the pool. Honouring it would additionally multiply generated tokens against a single per-request `max_tokens` ceiling, which is the invariant above. Silently returning one choice was the only unacceptable option, and a declared 400 is the OpenAI-contract-correct answer for a parameter the upstream genuinely cannot serve. The refusal message names the parameter and nothing about who serves the request, so it stays provider-blind. `support-matrix.json` notes for both endpoints now record the refusal and the settlement bound. ## Tests, written first and mutation-checked Every behavioural test below was written before the fix and observed failing against the unfixed code. Each fix was then reverted individually to prove the test catches its absence rather than merely its deletion, and restored. | Mutation | What was reverted | Result | | --- | --- | --- | | A | reinstate the headroom inflation on the sync path | RED: `TestExecuteSync_CallerCeilingReachesProviderUnchanged` reported `dispatched max_tokens = 4104, want the caller's own 8` | | B | remove the `clampUsageToCeiling` call sites in `executeSync` and `executeStreaming`, leaving the helper intact so this tests the wiring rather than the function | RED: both `TestExecuteSync_BilledCompletionTokensCappedAtCallerCeiling` and `TestExecuteStreaming_BilledCompletionTokensCappedAtCallerCeiling` | | C | sync zero-content capture back to `reservation.Held()` | RED: `TestExecuteSync_ZeroContentCaptureBoundedByCallerCeiling` reported `charged 10000 credits against a ceiling worth 108` | | D | `unsupportedChoiceCount` returns false | RED: all three `TestNGreaterThanOneRejected` subtests | | E | streaming missing-usage capture back to `reservation.Held()` | RED: `TestExecuteStreaming_HoldCaptureBoundedByCallerCeiling` reported `charged 10000 credits against a ceiling worth 32` | Every mutation was reverted and the suite returns green. Negative controls are in the suite too, so the clamp cannot quietly become a discount on every request: `TestExecuteSync_NoCeilingLeavesUsageAlone` pins that a caller who set no ceiling is metered exactly what the provider reported, `TestNOneOrAbsentPassesThrough` pins that only the unservable shape is refused, and `TestSyncOverrunDispatchesOnce` pins that an over-ceiling response does not become a second provider call. The money tests price against the real `hive-free` catalog row (1,000,000 credits per million input, 4,000,000 per million output, D-048 and migration `20260824_02_free_pool_router.sql`) rather than a fixture, so the magnitude they assert is the magnitude the issue measured. `TestExecuteSync_ZeroContentLength_Twice_CapturesHold` was amended rather than deleted: its request now sets no ceiling, which is the condition under which a capture is still at the full hold. Both halves of that behaviour are now pinned, one per test. The three tests that pinned the headroom inflation are removed along with the behaviour they described. ### Verification Run in the repo's Docker toolchain, on a throwaway toolchain container with a worktree-unique `COMPOSE_PROJECT_NAME`. No shared database was touched; these are `-short` unit tests. ``` go vet ./apps/edge-api/... ./apps/control-plane/... go test ./apps/edge-api/... ./apps/control-plane/... -count=1 -short ``` Zero failures across both modules. `gofmt` is clean on every file this PR touches. Two pre-existing `gofmt` misalignments in `chat_completions.go` and `completions.go` were left alone as unrelated noise. ## Review round two, first stream: five defects found in this branch, all fixed An independent adversarial money-path review of a2f4539 reproduced five defects in this branch itself. Every one is answered in 8078f56, and every reproduction is kept as a regression guard in `completion_ceiling_review_test.go`. **Finding 1, HIGH, caller-triggerable revenue bypass.** The ceiling was read as the smaller of `max_tokens` and `max_completion_tokens`, but once the headroom rewrite was deleted the outbound body forwarded both fields verbatim, so the request boundary and the settlement boundary enforced different numbers. OpenAI treats `max_completion_tokens` as authoritative and `max_tokens` as deprecated, and Groq documents the same preference, so a caller pairing `max_tokens: 1` with `max_completion_tokens: 100000` received a full-size generation and paid for one completion token, on the four-to-one expensive output side and unbounded in generation size. `pinCompletionCeiling` now writes the settled minimum back over every ceiling field present, on all three dispatch paths, before `EnforceVariablePriceBounds` so a variable-price alias still gets its own cap applied on top. It only ever narrows: an absent field stays absent, and an unreadable or non-positive one is left as written, because raising it would widen what the provider may generate. **Finding 2, HIGH, overcharge past the ceiling.** `clampUsageToCeiling` is a no-op when the upstream sent no usage block, which is exactly the branch that then prices the request from content length, so a synchronous 200 carrying content and no usage charged far past the ceiling. Streaming survived only by accident, because its unconfirmed branch discards that estimate for a hold capture that is already bounded. The bound went into `settlementCredits`, the single function both settlement paths route through, which also closes the same hole on the session chat surface through `ChatSettlementCredits`. **Finding 3, MEDIUM, variable-price aliases.** On an `upstream_actual` alias the charge is derived from the cost the upstream reported for the generation, which the clamp never touches, so clamping the usage block only advertised a completion count nobody was billed on: the exact divergence that function exists to prevent. The clamp is now gated on the pricing mode, the same test `capCaptureAtCeiling` already applies. `hive-auto` is live in that mode. **Finding 4, MEDIUM, published contract.** The never-billed-past-N sentence is now qualified for the variable-price case on both endpoints that serve `hive-auto`, rather than deleted. **Finding 5, MEDIUM, `best_of`.** The identical unfixed defect `n` had, on the same endpoint: nothing read or refused it, the outbound body is re-marshalled from a map so it reached the provider intact, and the generated OpenAPI contract advertised it. Refused on the same terms as `n`, with the same guard shape and the same provider-blindness assertions. **Finding 6, LOW, documentation only.** That `max_tokens: 0`, negatives, `8.0` and `"8"` all read as no ceiling is now stated where the invariant is stated, with the reasoning. No behaviour change. Finding 7 requested no change and was rebutted on the thread. All five code findings were red before the fix and green after, and the whole of `./apps/edge-api/...` and `./apps/control-plane/...` passes with `go vet` clean and `gofmt` clean on every touched file. ## Review round two, second stream: three more defects, all fixed A second independent adversarial stream ran against 8078f56, the commit that answered the first stream. Codex is exhausted until 2026-09-10 and was replaced by Antigravity rather than skipped, on owner instruction. It found three real defects in code this pull request introduced, all fixed in 446151c, plus one performance observation. Full stream output and the mutation table are in the pull request comment. **Finding 1, HIGH, an unparseable ceiling field survived into the outbound body.** `pinCompletionCeiling` kept any ceiling field it could not parse as an integer, on the reasoning that overwriting it would widen what the provider may generate. That reasoning does not hold for a field this gateway cannot read: it cannot be called smaller either, and the provider may well parse it. So `max_tokens: 1` paired with `max_completion_tokens: 100000.5` reopened the entire first-stream bypass through a spelling the JSON number type happens not to cover. A field is now kept only when it reads as a positive integer at or below the ceiling, which is the rule `clampCompletionLimit` already applied to what it cannot read. **Finding 2, HIGH, the gateway invented a ceiling larger than the one the caller set.** `clampCompletionLimit` fills in every ceiling field the endpoint speaks, and filled them at `VariablePriceMaxCompletionTokens` regardless of a smaller ceiling already present. On chat that wrote `max_completion_tokens: 16384` in beside a `max_tokens: 1`, and since the newer spelling is the authoritative one, a variable-price alias generated four orders of magnitude more than was asked for and billed the upstream cost of it, with no settlement clamp behind it because that mode has none. The filled value is now the lower of that constant and any smaller ceiling the caller set, and the same limit narrows present fields too, so the function is correct independently of call order. **Finding 3, HIGH, hold captures priced the prompt at zero.** Both capture branches bound the charge with `capCaptureAtCeiling`, whose bound is the catalog price of the whole request at the ceiling, prompt included. Both are reached precisely because no usable usage block arrived, so the metered input count is zero, and passing that through priced a large prompt at nothing: a 100,000-token prompt capped at ten output tokens collapsed the capture from the hold to roughly forty credits, a free serve of the expensive half of the request. `captureInputTokens` now falls back to the same content-length prompt estimate `settlementCredits` uses on the same no-usage path. `TestExecuteStreaming_HoldCaptureBoundedByCallerCeiling` was amended rather than deleted, because its upper bound had encoded exactly this zero-priced prompt. **Finding 4, LOW, three JSON decodes of the request body before dispatch.** Partly taken: `pinCompletionCeiling` now settles the common single-ceiling chat request with a byte scan and returns before decoding. The rest was declined on the thread, since threading one shared decoded map through three functions with different contracts, one of which refuses the request outright on a decode error, is a larger and riskier diff on a money path than a call count justifies, on a request already about to make a network call to a language model. Each of the three fixes was mutation-checked: reverting it turns its own test red, and all three were red together before the fixes landed. ## Not in scope - Issues #1282, #1285, #1286, #1288 and #1289 in the same batch are unrelated defects with different root causes: storage writes, voice, images, usage-shape reporting and production route ordering respectively. None share a cause with this one, so none are folded in. #1284 is assigned elsewhere and untouched. - The SDK conformance assertions for these two parameters land with PR #1268, which adds `sampling-params.test.ts`. Adding the same file here would collide with it. - The `WHY` prose in migration `20260826_01_route_reasoning_reserve.sql` still describes the headroom mechanism. Applied migrations are never edited in this repo; the column's surviving purpose is documented at its remaining reader in `zero_content_guard.go`. ## Buglog entry ```json {"id":"bug-2026-08-28-max-tokens-not-enforced","date":"2026-08-28","title":"max_tokens silently inflated by the reasoning reserve, billing 206x the caller's ceiling; n>1 silently truncated to one choice","error_message":"max_tokens: 8 on hive-free returned usage.completion_tokens 1650, all billed; n: 2 returned HTTP 200 with choices.length 1 and no error","root_cause":"applyReasoningHeadroom (PR #1225, issue #1171) inflated every completion-limit field the caller had set by provider_routes.reasoning_reserve_tokens (4096 on three of four hive-free members) before dispatch, so max_tokens 8 went upstream as 4104. Its premise that hidden reasoning would burn the reserve while visible content stayed inside the caller's budget is unenforceable: nothing upstream separates the two, so the model spent the inflated ceiling on visible output. Because the rewrite skipped a ceiling the caller had NOT set, its entire effect was overriding ceilings callers HAD set. Separately, ChatCompletionRequest.N and CompletionRequest.N were declared and read by nothing, so n was forwarded, ignored upstream, and one choice returned with no error. Two hold-capture branches (sync zero-content, streaming missing-usage) also charged the flat DefaultHoldText authorization floor regardless of the caller's ceiling, a far larger breach of the same invariant.","fix":"Deleted applyReasoningHeadroom and its three call sites so the caller's ceiling reaches the provider unchanged. Added completion_ceiling.go: requestedCompletionCeiling reads the ceiling from the caller's original body before any outbound rewrite, clampUsageToCeiling caps usage.CompletionTokens at it on the single usage object both the response and the charge derive from, rewriteNormalizedUsage keeps the response body in step, and capCaptureAtCeiling bounds both hold-capture branches without ever reaching zero. Refused n other than 1 with 400 unsupported_parameter and param n on both chat and legacy completions.","tags":["billing","money-path","overcharge","max_tokens","openai-conformance","issue-1283","issue-1171","edge-api","inference","free-pool"]} ``` A second entry for the three defects the review found in this branch. Both lines go to main together in the buglog-only pull request. ```json {"id":"bug-2026-08-28-completion-ceiling-review-defects","date":"2026-08-28","title":"Completion-ceiling fix shipped a caller-triggerable revenue bypass, an unbounded content estimate, and a usage clamp on aliases it could not bill","error_message":"Request {max_tokens: 1, max_completion_tokens: 100000} was dispatched unchanged and settled at output_tokens 1, actual_credits 24 where the unclamped generation was worth 6620; a synchronous 200 carrying content and no usage block against max_tokens 8 charged 1323 credits versus 32 ceiling-priced; on an upstream_actual alias the caller read completion_tokens 8 while paying the upstream cost of 1650","root_cause":"Three separate causes in one change. First, requestedCompletionCeiling takes the smaller of the two chat spellings but nothing rewrote the outbound body, so after applyReasoningHeadroom was deleted both fields were forwarded verbatim and the request boundary enforced a different number than settlement; OpenAI and Groq both prefer max_completion_tokens, so the larger field governed generation while the smaller governed billing. Second, clampUsageToCeiling returns early when usage is nil, which is the same condition that routes settlement into the content-length estimate in settlementCredits, leaving the one path that guesses the completion count as the one path with no ceiling on it; the streaming path was masked because settleStream overwrites the estimate with a bounded hold capture. Third, clampUsageToCeiling was called unconditionally, but an upstream_actual alias settles from respBody or acc.RawUsageChunk, which the clamp never reads, so it lowered the number the caller reads without lowering the charge.","fix":"Added pinCompletionCeiling, which writes the settled minimum back over every ceiling field present in the outbound body before dispatch and only ever narrows, wired on executeSync, executeStreaming and executeResponsesStreaming ahead of EnforceVariablePriceBounds. Added a ceiling parameter to settlementCredits that bounds the content-length completion estimate, with ChatSettlementCredits reading the ceiling from the request bytes it already parses so the session chat surface is covered too. Gated clampUsageToCeiling on route.Pricing.IsUpstreamActual, the same test capCaptureAtCeiling applies. Refused best_of other than 1 on legacy completions. Qualified the support-matrix sentence for the variable-price case and documented that non-positive and non-integer ceiling spellings read as no ceiling.","tags":["billing","money-path","revenue-bypass","overcharge","max_tokens","max_completion_tokens","best_of","upstream-actual","issue-1283","pr-1305","edge-api","inference","code-review"]} ``` A third entry for the three defects the second adversarial stream found. All three lines go to main together in the buglog-only pull request. ```json {"id":"bug-2026-08-28-completion-ceiling-second-stream-defects","date":"2026-08-28","title":"Ceiling pinning kept unreadable fields, the variable-price bound invented a larger ceiling than the caller set, and hold captures priced the prompt at zero","error_message":"Request {max_tokens: 1, max_completion_tokens: 100000.5} was dispatched with the float intact; a variable-price request sending only {max_tokens: 1} was dispatched with max_completion_tokens 16384 written in beside it; a streaming request with a prompt of roughly 11334 tokens and max_tokens 8 whose upstream sent no usage block settled at 32 credits rather than the price of the prompt","root_cause":"Three causes in the fix for the first adversarial stream. First, pinCompletionCeiling skipped any present ceiling field that failed to unmarshal into int64, on a never-widen rule that does not hold for a value the gateway cannot read at all, since the provider may parse what encoding/json refuses and OpenAI treats max_completion_tokens as authoritative. Second, clampCompletionLimit sets every field the endpoint speaks to VariablePriceMaxCompletionTokens when absent, with no reference to a smaller ceiling already present, and an upstream_actual alias has no settlement clamp behind that. Third, capCaptureAtCeiling computes its bound as the catalog price of the whole request at the ceiling, but both callers pass the metered input count, which is zero exactly because those branches are reached when no usable usage block arrived, so the prompt was priced at nothing and the capture collapsed to the output side alone.","fix":"pinCompletionCeiling now keeps a field only when it reads as a positive integer at or below the ceiling and overwrites everything else present, logging the raw literal truncated. clampCompletionLimit derives an effective limit as the lower of VariablePriceMaxCompletionTokens and any smaller positive ceiling already in the body, and uses it both to fill absent fields and to narrow present ones. Added captureInputTokens, which falls back to the content-length prompt estimate settlementCredits already uses when the upstream reported no usage, and wired it into both capCaptureAtCeiling call sites. Added a byte-scan early-out to pinCompletionCeiling so an ordinary single-ceiling chat request pays no extra decode.","tags":["billing","money-path","revenue-bypass","free-serve","max_tokens","max_completion_tokens","upstream-actual","hold-capture","issue-1283","pr-1305","edge-api","inference","code-review"]} ```
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…ing pool The intermittent failure on the multi-turn tool round trip is a real gateway defect, not a flaky test, and not the pool-member class the top_k defect belonged to. Diagnosed against the live gateway on the demo box rather than inferred. What happens. The second turn of a tool round trip, after the tool result goes back, asks a reasoning model to write the final answer. deepseek-v4-flash spends most of its ceiling on hidden reasoning: measured over ten attempts at the suite's own ceiling of 256, reasoning_tokens ran 22 to 148 out of 49 to 193 total completion tokens. When reasoning overruns the ceiling entirely, the upstream answers content null with finish_reason length. Forcing that case by lowering the ceiling to 120 reproduced it on two of eight attempts, with reasoning_tokens of 117 and 124. So, taking the three readings in the brief in order. The empty content is a legitimate provider response in the narrow sense that the provider really did answer that way, but it is not a usable one: finish_reason is length, the model did not finish, and a caller asking for the final turn of a tool round trip is entitled to text. The assertion is right to demand it. No provider in a pool handles the tool-result turn differently; this is one route, and it is the pinned single-route alias, not hive-free. And the gateway is NOT dropping content the upstream returned: the upstream sends null, and coerceNullContent turns it into an empty string precisely so an SDK that dereferences content unconditionally does not crash. The actual defect is the gate on the existing zero-content guard. That guard already retries an empty length completion once, which is exactly the right answer here, but it only ran when route.ReasoningReserveTokens was positive. That column does not mean "this route reasons": it means "somebody enumerated this route in migration 20260826_01", which covered three free-pool members and nothing else. deepseek-v4-flash carries the column default of zero while reasoning heavily, so the guard sat out the one path where it was needed most. The gate is now the response shape itself, which needs no catalogue column to interpret: an empty length completion is worth one retry whatever route produced it. The test that asserted the opposite is inverted, with the measurement in its comment. Also corrects the max_tokens contract in the conformance suite. It asserted a reserve-inflated ceiling on the pooled alias, citing a mechanism that PR #1225 added and issue #1283 removed. The current code sends the caller's ceiling upstream untouched, which the live probe confirms: a ceiling of 120 came back as exactly 120 completion tokens. Both aliases now assert the ceiling the caller asked for.
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.
Fixes #1171
Root cause
The hive-free alias load-balances four heterogeneous members behind one
litellm_model_name. Two of them reason; a reasoning member can spend the caller's entiremax_tokenson hidden reasoning and answerfinish_reason=lengthwith no visible content, which settled as an ordinary full-price success. Live evidence on #1171: five of six reasoning prompts returned empty content and were billed in full.Fix
1. Per-member headroom (pre-dispatch)
provider_routes.reasoning_reserve_tokens(migration20260826_01_route_reasoning_reserve.sql). The three reasoning members (route-free-pool-gemini,route-free-pool-groq,route-free-pool-groq-2) carry 4096; the non-reasoning OpenRouter dots member stays at 0, so its deployments keep exactly the ceiling the caller asked for.routing.SelectionResultcarries the MAX reserve across eligible routes sharing the selectedlitellm_model_name(LiteLLM load-balances that whole group under one name, so edge-api cannot know which member answers before dispatch). Candidates on other gateway groups are excluded.edge-api/internal/inference.applyReasoningHeadroominflates every completion-limit field the caller actually set (chat: bothmax_tokensandmax_completion_tokens; completions:max_tokens; responses:max_output_tokens) by that reserve on all dispatch paths (sync chat/completions/responses, streaming chat/responses). A ceiling field absent or non-positive is left alone: a caller who set no ceiling declared no budget to protect.max_tokenskeeps its OpenAI meaning: it caps what they SEE.2. Zero-content guard (sync chat path)
Doctrine order followed: retry once first, capture only when the retry does not produce content.
finish_reason=lengthcompletion on a reserving pool is retried once against the same pool (LiteLLM's shuffle makes a repeat member pick possible but not guaranteed, documented honestly).reservation.Held()withterminal_usage_confirmed=false, exactly the capture shape PR fix: streaming settlement fails closed, captures the reservation hold when upstream usage is missing #1220 established for streams, plus:zero_content_capturedand a new Prometheus counterhive_zero_content_captured_total{alias,endpoint},X-Hive-Upstream-Empty-Content: lengthresponse header alongside the upstream body whose finish_reason already explains the emptiness.finish_reason=stopempties are genuine answers and not captured; tool-call messages with null content are spec-correct and never trip the guard; refusals count as visible output.Scoped to the sync path where the live evidence sits; the streaming path gets headroom only, since its settlement side is #1220's freshly merged territory and a mid-stream retry after chunks flowed is not possible.
3. Metering nondeterminism (findings-only)
Identical prompts metered 3 vs 76 prompt tokens across members is upstream variance, not a gateway bug: the four members run different tokenizers and counting rules (gpt-oss harmony BPE vs Gemini SentencePiece-family counting vs the dots model), and LiteLLM may estimate locally for members whose upstream omits usage. Correcting it at our normalize boundary would mean shipping tokenizers or second-guessing provider-reported usage, which settlement treats as truth. Billing flows through alias price x reported tokens, so the variance reaches the customer by design of the free lane. No code change made; revisit only if cross-member variance ever matters on a paid heterogeneous pool.
Test evidence
Red-then-green per requirement:
reasoning_reserve_test.go).zero_content_guard_test.go)..github/ci/test-db-bootstrap.sql+ full migration chain applied in order, including the new migration), integration-tagged routing suite green including the two new live tests (TestFreePoolReasoningReserveLiveRows,TestFreePoolReasoningReserveLiveSelection) proving the real schema, seeded rows, pgx scan, and aggregate end to end. CI runs this same suite against its ephemeral DB.go vet -tags integrationclean.Notes
hive_stream_usage_block_missing_totaland mine).Buglog entry
To be appended to
.wolf/buglog.jsonlin the follow-up buglog-only PR once this merges (#873 protocol):{"id":"1171-zero-content-full-price","ts":"2026-08-26","error_message":"5 of 6 reasoning prompts on hive-free returned finish_reason=length with empty content and settled as ordinary full-price successes; identical prompts also metered 3 vs 76 prompt tokens across pool members","root_cause":"load-balanced heterogeneous pool dispatched the caller's max_tokens verbatim, letting reasoning members spend the whole visible budget on hidden reasoning; settlement had no zero-content verdict and trusted provider-reported usage unconditionally","fix":"per-member reasoning_reserve_tokens inflated into the upstream completion ceiling before dispatch (pool-max surfaced through SelectRoute); sync chat completions retry an empty length-finish once then capture the reservation hold with terminal_usage_confirmed=false plus hive_zero_content_captured_total counter and X-Hive-Upstream-Empty-Content header","tags":["billing","free-pool","reasoning","fail-closed","litellm"]}