Repository navigation
Send an output cap, and shrink it when the completion reserve overflows the window - #36
solvyxtech wants to merge 2 commits into
Conversation
…ws the window
GMI Cloud serves qwen/qwen3-235b-a22b-2507 with a 131072-token total
window. The OpenAI-shaped request carried no max_tokens, so the router
asked for the model's whole listed output (131072), and even a
2,454-token prompt was refused. contextOverflow then shed history, which
cannot fix a completion overflow, and 11 of 34 Qwen runs ended "too large
and nothing left to shed".
- completionOverflow reads the window, input and completion counts from
the vLLM/OpenAI wordings ("You requested a total of X tokens: A tokens
from the input messages and B tokens for the completion", and "(A in
the messages, B in the completion)"). When lowering the completion
alone fits (window - input - margin >= 1024), max_tokens is lowered to
that and the same request is sent again, with no shed; the cap is kept
for the session. A prompt that is itself too big still sheds.
- The OpenAI-shaped request now always sends max_tokens: the default is
DEFAULT_MAX_TOKENS (32768), doubled up to 131072 only when a reply is
cut off at it; --max-tokens still wins.
- A provider that refuses the field itself (max_completion_tokens
models) gets requests without it after one retry. The output-ceiling
retry now runs on both request shapes and reads OpenAI's and
DeepSeek's wordings too.
- contextOverflow reads "maximum context length of W", and takes the
input count, not the total, as what the server counted.
solvyxtech
left a comment
There was a problem hiding this comment.
Grok review: changes requested (posted as a comment because GitHub won't let the shared account request changes on its own PR). Two small fixes, then this is good to go.
Reviewed only the new commit a48570ddb (everything under it is #24–#31, reviewed in their own PRs). Pairing rules are clean: no subscription backends, no competitor names in product copy (the new user-facing info line names no product), nothing touching fine-tune data, and the base is claude/sync-local-work, not main. CI (typecheck-and-test) was still pending when I looked.
The diagnosis and the fix are right: a completion-reserve overflow can't be fixed by shedding, so refitting max_tokens and resending the same history is the correct move, and keeping the prompt-too-big case on the shed path is the right split. Taking the input count as sent in contextOverflow is also a real fix, since that's the number Maat's estimate is compared with.
Please fix
-
The truncation note and nudge report the doubled cap, not the cap the reply hit. In the
finishReason === "length"branch,outputCapis doubled before thelog.appendnote, the[molt: that reply hit the output ceiling of ${this.maxTokensFor()} tokens…]transcript nudge and theinfoline are built. So on the first cut-off at 32,768 all three say 65,536. The nudge goes to the model, and the note goes into the run log Maat's receipts rely on, so it should be accurate. Captureconst hit = this.maxTokensFor()before doubling and usehitin those three strings (and maybe add "next request asks for N" to the note). Worth a one-line assertion in the "doubles only when a reply hits it" test. -
refusedMaxTokensis broader than its doc comment says. The comment says it must name the field and say it's unsupported, but the last alternative,|max_completion_tokens, matches any body that mentions both fields, with no "unsupported" wording needed. (It also makesuse 'max_completion_tokens'redundant.) A value error like "max_tokens / max_completion_tokens must be ≤ N" that slips pastoutputCeilingwould drop the cap for the session. The one-retry-then-revert guard limits the damage, but I'd drop the bare alternative so the code matches the comment.
Suggestions (non-blocking)
- Always sending
max_tokenschanges behavior for every OpenAI-shaped endpoint, not only GMI. Servers that count the reserve against the window but word the refusal differently will now fall through to the shed path, which is the exact failure this PR fixes. Hugging Face TGI-style servers are the one I'd check (their refusal reads roughly "inputstokens +max_new_tokensmust be <= N"). Either add that wording tocompletionOverflow, or at least make sure a refusal that namesmax_tokens/max_new_tokensnever goes to shed. outputCapisn't reset insetModel, whilemodelMaxTokens,fittedCompletionandmaxTokensUnsupportedare. A cap doubled to 131,072 on one model carries over to the next. That's self-correcting through the refusal paths, but it costs a refused request, and resetting it toDEFAULT_MAX_TOKENSon a model change would match the comment above it.- Reasoning models that refuse
max_tokensend up uncapped, which is the pre-PR behavior. Sendingmax_completion_tokens: this.maxTokensFor()on the retry would keep a cap there too. Fine as a follow-up. - Tests: the refused-field test could also assert that the second turn doesn't pay the refusal again, and that a retry-without-field that also fails puts
max_tokensback (the revert path has no test). - Minor: the two-pass refit loop doesn't check the abort signal between sends. That's harmless at two iterations; I'm only noting it.
Not merging from here. Once 1 and 2 are in and CI is green, this is an approve from me.
solvyxtech
left a comment
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
(Posted as a comment review: GitHub won't let this shared account request changes on its own PR.)
Fresh review at head a48570d. This is the same commit my 22:37 UTC review looked at, so neither of the two items from that review has been fixed. I re-read the whole commit and probed it with mock providers built on the test/completion-reserve.test.ts harness.
Checks run: typecheck is clean. The full unit suite passes 2024/2024 on Linux with node 20.19. The runner still exits 1, but only because node's coverage reporter crashes; that is unrelated to this PR.
Pairing terms: clean. Vendor names appear only in code comments and regexes, there are no subscription backends, and fine-tune data is untouched.
Blocking
1. Always sending max_tokens: 32768 breaks endpoints whose refusal wording isn't one of the two parsed ones. src/engine.ts:6157 and src/engine.ts:356 (completionOverflow). High confidence: reproduced with mocks.
- OpenRouter. When a request doesn't fit, OpenRouter answers "This endpoint's maximum context length is 32768 tokens. However, you requested about 34668 tokens (1000 of text input, 900 of tool input, 32768 in the output)…".
completionOverflowreturns null,refusedMaxTokensreturns false, andcontextOverflowsheds withsent: 0. On turn 1 the run then ends with "this endpoint serves 32768 tokens of context and there is nothing left to shed … Restart the server with a larger context". In the mock, the first request was refused and the turn never answered. - Current vLLM (wording since vllm#36197). It answers "This model's maximum context length is 32768 tokens. However, you requested 32768 output tokens and your prompt contains 1000 input tokens, for a total of 33768 tokens…". That hits the same dead end on turn 1. It is worse, because
contextOverflow's/you requested (?:a total of )?(\d+)/reads the output count assent, sotokenScalejumps to about 8x and every later decision about what to shed is skewed. - Why it matters: before this PR no
max_tokenswas sent, and both servers fit the output themselves. OpenRouter's documented maximum is the context minus the prompt, and vLLM defaults tomax_model_len - input. So this is a regression for every OpenRouter model with a context of about 32k–64k (the bench routes through OpenRouter) and for self-hosted vLLM at the common--max-model-len 32768. Both fail on the first request. - Fix:
- Teach
completionOverflowthese wordings:(\d+) of text input, an optional(\d+) of tool input,(\d+) in the output, andyou requested (\d+) output tokens and your prompt contains (\d+) input tokens. - Make
contextOverflowstop reading an output count assent. - As a fallback: when a 400 is about the context window and the
max_tokenswe sent was our default (not--max-tokens), retry once withoutmax_tokensbefore shedding. - Add mocks for all three wordings.
- Teach
2. The vLLM 0.8–0.16 wording only works by accident, through the over-broad refusedMaxTokens. src/engine.ts:373. High confidence: reproduced.
- What happens: vLLM 0.8–0.16 answers "'max_tokens' or 'max_completion_tokens' is too large: 32768. This model's maximum context length is 32768 tokens and your request has 1000 input tokens (32768 > 32768 - 1000)."
completionOverflowmisses it. The bare|max_completion_tokensalternative matches, so the session dropsmax_tokensentirely and journals "provider refused max_tokens", which is false. The session also runs uncapped from then on. - Correction to my earlier advice: in my last review I asked for that alternative to be removed. Doing that alone would turn this case into the turn-1 failure from item 1. Please add this wording to
completionOverflowfirst, then narrowrefusedMaxTokensto what its comment says it matches.
3. The cut-off note and the model nudge still report the doubled cap. src/engine.ts:7197, 7200 and 7207. High confidence: reproduced.
- What happens:
outputCapis doubled before the note, theinfoline and the[molt: that reply hit the output ceiling of N tokens…]nudge are built. With a reply cut at 32,768, the mock printed "cut off at the 65536-token output ceiling", and the transcript carries "output ceiling of 65536". - Why it matters: the model is told the wrong number, and so are the receipts that read the journal.
- Fix: capture
const hit = this.maxTokensFor()before doubling and use it in all three strings.
Non-blocking
- The fitted cap is sticky after the prompt shrinks.
src/engine.ts:2046and6323. Medium confidence. Say the fit is taken near a full window: 125k input on a 131k window gives 4,761. After a shed or compaction the prompt shrinks, but the cap never comes back up until the model changes. BecausemaxTokensFor() !== outputCap, the doubling path never fires either. A largewrite_filereply then truncates on every try. The comment "the prompt only grows" isn't true once shedding exists. Store the window and recompute from the current estimate, or clearfittedCompletionafter a shed. outputCapis not reset insetModel.src/engine.ts:2358. Medium-high confidence. This was raised before and is still open. A cap of 131,072 carried over to a model whose limit is lower costs a refused request.- One extra refused round trip every time the prompt grows about 1% past the fit. Low confidence. The fit is computed against the old input count, so each turn that grows past the 1% margin pays a refusal and a resend. Computing the fit from the window and the current estimate would avoid it.
- Test gaps: the refused-field revert path has no test, and neither do the three wordings above.
Nothing merged.
…hedding
Sending max_tokens on every request broke endpoints whose refusal wording
was not one of the two parsed ones: OpenRouter ("1000 of text input, 900 of
tool input, 32768 in the output") and current vLLM ("you requested 32768
output tokens and your prompt contains 1000 input tokens") fell through to
the shed path and ended turn 1 with nothing to shed.
- completionOverflow reads OpenRouter, current vLLM, vLLM 0.8-0.16
("'max_tokens' or 'max_completion_tokens' is too large: N ... your request
has M input tokens"), OpenAI (including "N in the functions"), Anthropic
("input length and `max_tokens` exceed context limit: A + B > W") and TGI.
llama.cpp never refuses for the reserve, so its refusal stays a prompt
overflow.
- contextOverflow takes the prompt count from those wordings and never reads
an output count as what was sent (current vLLM scaled every estimate 8x);
it also reads Anthropic's "prompt is too long: N tokens > W maximum".
- A window refusal in wording none of these read, from a request with a cap
above 8192, gets one request with a smaller cap (fitted from the window
and the estimate when the server named a window) before shedding; kept
only if it works, and dropped if a reply is later cut off at it. A prompt
known to be too big goes straight to the shed path.
- refusedMaxTokens no longer matches a body that merely names
max_completion_tokens; it needs the field refused.
- The cut-off note, the nudge and the info line report the cap the reply
hit, not the doubled one; the note says what the next request asks for.
- A model change resets the doubled output cap; a shed clears the fitted
cap, since the smaller prompt leaves room for a larger reply.
- Tests for each wording, end to end on turn 1, the llama.cpp shed path,
the fallback and its revert, and the refused-field revert.
|
Addressed in Blocking
Non-blocking, also done
Tests added ( Not done (follow-up): sending The stacked branches (#32, #33, #34) merge cleanly onto the new head ( |
Builds on #31: the stack is linear, so the #24–#31 commits show here until they merge. Only the top commit is new.
The bug
GMI Cloud serves
qwen/qwen3-235b-a22b-2507with a 131,072-token total window. The OpenAI-shaped request carried nomax_tokens, so the router asked for the model's whole listed output (131,072). Even a 2,454-token prompt was refused:contextOverflow()then shed history, which can never fix a completion-reserve overflow. The run ended "too large and nothing left to shed" in 11 of 34 Qwen runs.The fix
completionOverflow()reads the window, the input count and the completion count from this wording and from the older OpenAI/vLLM one ("(A in the messages, B in the completion)"). When lowering the completion alone is enough (window − input − margin ≥ 1024, margin = max(256, 1% of the window)),max_tokensis lowered to that and the same request is sent again, with no shed. The lowered cap is kept for the session and reset on a model change.max_tokens. The default is the existingDEFAULT_MAX_TOKENS(32,768), not the model's listed maximum. It doubles, up to 131,072, only when a reply is actually cut off at it.--max-tokensstill wins.max_tokensand say it is unsupported. OpenAI'smax_completion_tokensmodels are the case.outputCeiling) now runs on both request shapes, and also reads OpenAI's wording ("supports at most N completion tokens") and DeepSeek's ("valid range of max_tokens is [1, N]").contextOverflowreads "maximum context length of W" too. It now takes the input count, not the total, as what the server counted, which is the number Maat's estimate is compared with.Tests
test/completion-reserve.test.ts:max_tokens(≤ 131072 − 2454 − 256) and resends the same history. Nothing is shed, and the turn answers. The next turn uses the lowered cap with no second refusal.max_tokensis unchanged.max_tokens. Its requests go out without the field.npm run checkis green: 2024 tests, e2e and self-check, with ~/.local/bin off PATH.