Repository navigation
npu: a wrong-typed max_tokens, stream or tools is a 400 naming the field, not a 500 with no body - #1108
Conversation
…eld, not a 500 with no body
Three fields were read before the route's refusal guard, so a wrong-typed value threw
nlohmann::json::type_error past it and httplib answered **500 with an empty body** -- indistinguishable
from a server fault to a client, and the shape that makes an operator restart a healthy backend:
{"max_tokens": "8"} -> 500, no body (forward_serve.cpp:186, before the try at :191)
{"max_completion_tokens": "8"} -> 500, no body (same site)
{"stream": "yes"} -> 500, no body (forward_serve.cpp:213)
And `tools` of the wrong type was accepted in the default tool_choice mode: npu_tool_grammar.h's
`if (!tools.is_array() || tools.empty()) return "";` folded "wrong type" into "no tools", so the
`tools: must be an array` refusal was only reached for a non-default tool_choice. That one is worse than
cosmetic -- Options::tools_sent is set before the call, so the answer parser still treated <tool_call>
markup as a call in a request whose tools was never valid.
Fixed with two typed readers in app/npu_sampling.h (npu::int_field / npu::bool_field: absent or null is
the default, a number/boolean is taken, anything else throws "<field>: ..." -- the type the handler already
turns into a 400; a fractional max_tokens still truncates, so no caller changes), the two forward_serve
reads routed through them inside the guard, and the tools short-circuit split so only an EMPTY array means
"no tools".
Device-free rows (Qwen3-0.6B, NPU_INFER_HOST_GEMM=1, no ticket): the three fields and both wrong-typed
tools forms now answer 400 with the field named; max_tokens 8 / 8.5 / absent, tools:[] and the pre-existing
refusals (logprobs, stop) are unchanged at 200/400. tests: test_typed_fields + test_tools_type, 253
assertion sites, npu_sampling: ok. docs/npu.md gains the two field rows and names the new messages.
Closes #1106.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 49f436bb — I compiled your test standalone and ran it, then probed the two new helpers myself, then checked the throw actually lands on a 400.
$ g++ -std=c++17 -I app -I <nlohmann> tests/npu_sampling_test.cpp -o /tmp/samp_test && /tmp/samp_test
npu_sampling: ok
My own probe of int_field / bool_field (independent of your 12 assertions, same six shapes):
int_field {"a":"8"} string for an int -> a: must be an integer <- the field is named
int_field {"b":4.7} float -> 4 <- truncation preserved
int_field {"d":null} -> 256 (default)
int_field {} absent -> 256 (default)
bool_field {"c":true} -> true
bool_field {"e":[1]} array for a bool -> e: must be true or false
Two things I care about in that table: the message names the field, which is the whole point of a 400 over a 500, and the float truncation matches what body.value did, so nothing that works today changes behaviour.
And the 500-with-empty-body cannot recur through this path: max_tokens = npu::int_field(…) sits inside the route's try, whose catch is res.status = 400 with "bad request: " + ex.what(). That is the structural fix — the old code threw nlohmann::json::type_error from outside that guard, which is why it escaped as a server fault.
The tools split is right too: if (!tools.is_array() || tools.empty()) return ""; became if (!tools.is_array()) throw …; if (tools.empty()) return ""; — so a wrong-typed tools is now a 400 naming the field, while an empty array is still accepted silently as before. That is the distinction the bug was made of: tools: "x" was being read as "no tools", i.e. a client mistake silently reinterpreted as a different valid request.
Not verified by me: the 253-site before/after HTTP rows. I did not stand up the server and replay them, so those are yours; what I verified is that the failure path they exercise ends in a 400 with the field named, and that the helpers behave identically on every input a good client sends.
Together with #1107 this is a good pair for the NPU route: one fixes a container the converter padded (a silent wrong answer that surfaced as a 500), the other fixes client errors that surfaced as 500s with no body. Both are the same disease — a fault reported as the wrong kind of fault.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Three top-level fields are read before the route's refusal guard, so a wrong-typed value escapes as
nlohmann::json::type_errorand httplib answers 500 with an empty body — which a client (Lemonade's backendwatchdog included) cannot tell from a server fault. And
toolsof the wrong type is accepted in the defaulttool_choicemode and silently installs nothing.Measured on
ad47253, Qwen3-0.6B,NPU_INFER_HOST_GEMM=1(no device, no ticket), one serve,max_tokens2 unless thefield under test is
max_tokensitself:{"max_tokens": "8"}max_tokens: must be an integer{"max_completion_tokens": "8"}max_completion_tokens: must be an integer{"stream": "yes"}stream: must be true or false{"tools": "a string"}(defaulttool_choice)tools_sentstill settools: must be an array{"tools": []}{"max_tokens": 8}/{"max_tokens": 8.5}/ nomax_tokensbody.valuedid){"logprobs": "yes"}/{"top_logprobs": "3"}/{"stop": 5}/{"tool_choice": 7}Root cause, three sites
app/forward_serve.cpp:186—body.value("max_completion_tokens", body.value("max_tokens", 256))ran before thetryat:191.app/forward_serve.cpp:213—body.value("stream", false), same.app/npu_tool_grammar.h:104—if (!tools.is_array() || tools.empty()) return "";folded "wrong type" and "notools" into one early return, so the
tools: must be an arrayrefusal (read_tools) was only reached for anon-default
tool_choice. Worse than cosmetic:Options::tools_sentis set before the call, so the answer parserstill treated
<tool_call>markup as a call in a request whosetoolswas never valid.The change
app/npu_sampling.h:npu::int_field()/npu::bool_field()— absent ornullis the default, a number/boolean istaken, anything else throws
std::runtime_error("<field>: …"), which is the type the handler already turns into a 400.Fractional
max_tokensstill truncates, so no existing caller changes.app/forward_serve.cpp: both reads go through those helpers,max_tokensinside the existing options guard andstreamin its own small guard with the same 400 shape.app/npu_tool_grammar.h:!tools.is_array()throws; only an empty array means "no tools".docs/npu.md:max_tokens/max_completion_tokens/streamrows in the request-field table, and the refusalparagraph names the two new messages.
Tests
tests/npu_sampling_test.cpp:test_typed_fields(integer/bool readers: absent, null, integer, fractional, string,boolean — each refusal asserted by its exact message) and
test_tools_type(string and numerictoolsrefused in thedefault mode, empty array still accepted as "sent, no grammar"). 253 assertion sites,
npu_sampling: ok);tools/copyright.py --checkrc 0;1bitbuilds with-DONEBIT_NPU=ONand the private runtime.Closes #1106.