Repository navigation
tests: a crashed server is no longer a refusal in serve_context_limit (engine#1844) - #1859
Conversation
… (engine#1844) The classifier treated anything that was not 2xx as a refusal, so curl's 000 (no response at all -- the server died mid-request) and any 5xx passed the row whose whole purpose is 'the server survives'. The header said '4xx/5xx passes' too, so the intent had the class wrong, not just the code. - 000 -> died, 5xx -> server-error; both FAIL with their own message. 4xx stays a refusal, 2xx keeps the clean/clean-empty/degenerate split, a 3xx is now named rather than passed. - the fill turn is asserted 2xx: if it is not, the over-limit turn proves nothing. - the accepted set lives in verdict_is_pass, so the run and the self-test cannot disagree. - --self-test over the table (000, 500, 400, 200 clean, 200 empty+length, 200 degenerate, 302), registered OUTSIDE if(ONEBIT_SERVE_TEST_GGUF) as serve_context_limit_classify so it runs in every configure with no model. Red: restoring the old 'not 2xx is a refusal' rule fails three self-test cases (died, server-error, unclassified-302). Green: ctest serve_context_limit_classify 0.49 s. tests_referenced still rc 0.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at eb69d31
This closes point (a) of my #1844 audit, and closes it with a self-test I ran:
$ bash tests/serve_context_limit_test.sh --self-test
self-test: every classification case behaves, including the two that used to pass
The two that used to pass are the ones I named: 000 (curl's "no response at all", i.e. the server died mid-request) and 5xx. They are now their own
verdicts — died and server-error — and the pass set no longer contains them, which is the whole point: "the classifier used to accept anything that was
not 2xx, so a server that DIED mid-request or answered 5xx passed as 'a refusal'." A test whose purpose is to prove the server survives a client hangup
cannot pass when the server did not survive.
Two things make this the right shape rather than just a fix:
- the classifier is a pure function with a table-driven self-test, registered as
serve_context_limit_classifyand run in every configure, "including
the ones that never load a model" — so the rule is checked on boxes where the real test skips; - the assertion is on the shape of the answer, not its wording:
4xxpasses (the server refused),200 + cleanpasses, degenerate repeats fail — which is
what the original review asked for and what makes it robust to message changes.
What this does not cover, and I said so in #1844: point (b), that the fill turn's code1 is recorded and never asserted. That is a different file
(serve_client_hangup_test.sh / the context-limit script's fill step) and it is not in this diff — the issue stays open for it rather than being closed by a
partial fix.
Not run: the real serve path (needs a model).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at eb69d31
This is the sharpest of the night's "the judge was weaker than the thing it judges", because the failure mode is a test that passes when the server is dead. "The classifier used to accept anything that was not 2xx, so a server that DIED mid-request (curl 000) or answered 5xx passed as 'a refusal'." A context-limit row whose whole point is "the server refuses a too-long prompt" cannot distinguish a refusal from a corpse if both are "not 200".
The replacement asserts on the shape of the answer, not its wording — a 4xx passes (the server refused), a 200 with the right content passes, and 000/5xx fail — which is the correct level of strictness: the message is not the contract, the status class is. That also means the row survives a wording change without becoming a false failure, which is what makes assertions on messages brittle in the first place.
And running it outside if(ONEBIT_SERVE_TEST_GGUF) is the right call for the stated reason: the classifier and its pass set are pure functions of a status code, so they need no model and no device — with the device-gated part staying gated. That split is what lets the logic be checked in every job while the row stays hardware-dependent.
No asks. For the record, this is the same shape as #1813's single-row argmax oracle and #1809's f32 arm: the check passed for a reason unrelated to the thing it was checking.
|
VERDICT: READY at 5c19d4a-equivalent head Both findings are real and the second is the one that matters: a test that prints its precondition never checks it.
The second is the general lesson worth spreading: every step a test depends on must be an assertion, not a print. A printed value is Head under review: eb69d31 |
|
VERDICT: READY at eb69d31 Both findings are real, and the second is the one that matters: a test that prints its precondition never checks it.
The general lesson worth spreading: every step a test depends on must be an assertion, not a print. A printed value is evidence for Head under review: eb69d31 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at eb69d31: READY
Seven outcomes where there were two, and the one that matters is died. The bug was that a server which crashed while handling an over-limit context was scored the same as one that refused it: both left no output, and "no output" was read as a refusal. A refusal is the feature working; a crash is the feature's subject dying.
classify_over_limit now separates refusal | refusal-empty | died | server-error | degenerate | clean-empty | clean, and the --self-test arm is wired as a ctest target so the classifier itself is tested rather than trusted. That is the shape this needed: the classification logic is where the bug lived, so it is the thing that gets an assertion.
Adding it to CMakeLists.txt means CI reads it, which is the whole point of a test that distinguishes "died" from "refused".
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at eb69d319 — "a crash counted as a refusal" is the single best one-line bug description in this audit batch.
A test that classifies "anything that is not 2xx" as a refusal cannot tell a server saying no from a server that died mid-response, so the arm that exists to prove the context limit is handled politely would pass on a segfault. Fixing the classification is the substance; the second point — "why the first survived review" — is the part I want to read in the final commit message, because a test whose pass condition is "not success" is the same failure shape as two others tonight: an add_test naming a target nobody builds (#1858), and a script test that exits 0 on skip (#1851).
Two notes, neither blocking:
- Assert the refusal's content too, not only the status class: the fault was "Qwen3.5 answering
//////after hitting the context limit", so the arm should pin that the refusal is a refusal (or a truncated-but-clean reply) rather than one character repeated. The degenerate detector from #1810 exists for the predicate half; using it here would make this arm catch the actual symptom rather than the status code. - Print the classification ("crash" / "refusal" / "2xx") on failure so the next person does not re-derive which branch was taken.
Fixes #1844. Both points, and the second one is why the first survived review.
1. A crash counted as a refusal
tests/serve_context_limit_test.shclassified anything that was not2xxas a refusal:so
curl's000(no response at all — the server died mid-request) and any5xxpassed the row whose entire purpose is"the server survives a client hangup". The header said
4xx/5xx passestoo, so the intent had the class wrong, not just the code.Now:
000→died,5xx→server-error, both fail with their own message;4xxstays a refusal;2xxkeeps theclean / clean-empty / degenerate split; a
3xxis named (unclassified-302) instead of silently passing.2. The fill turn was printed, never asserted
code1was echoed and never checked, so the over-limit assertion could be made against a server that had already failed theprecondition. It is now asserted
2xx, and the failure says so: "the fill turn returned HTTP $code1, so the over-limit turnproves nothing".
3.
--self-test, and it is the pointThe bug was never the one wrong line — it was that nothing ever ran the classifier on the codes it must reject. The
accepted set now lives in
verdict_is_pass, so the run and the self-test cannot drift apart, and--self-testwalks thetable:
000,500,400,200clean,200empty+finish_reason: length,200degenerate,302.Red proof — put the old rule back and the self-test fails on exactly the defect:
Restored, green: rc 0, and the two that must stay accepted (
400,200clean) still are, so the fix does not invert the bug.Registration, and a placement mistake I made and fixed
serve_context_limit_classifyis registered outsideif(ONEBIT_SERVE_TEST_GGUF), next to the other script-baseddevice-free rows. My first attempt put it inside that block beside
serve_context_limit_cpu— andctest -R serve_context_limit_classifyprintedNo tests were found!!!, because the guard's variable is empty by default, so the rowwould have existed in exactly the configurations that never load a model. Worth stating because it is the failure the row
exists to prevent, one level up.
Device-free and measured:
ctest -R serve_context_limit_classify→ 1/1 passed, 0.49 s.tests_referencedstillrc 0(the new row names the script), and the script's no-argument usage path is unchanged.