Skip to content

tests: a crashed server is no longer a refusal in serve_context_limit (engine#1844) - #1859

Merged
bong-water-water-bong merged 1 commit into
mainfrom
os/v3-serve-classify
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
os/v3-serve-classify

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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.sh classified anything that was not 2xx as a refusal:

if not code.startswith("2"):
    print("refusal" if body.strip() else "refusal-empty")

so curl's 000 (no response at all — the server died mid-request) and any 5xx passed the row whose entire purpose is
"the server survives a client hangup". The header said 4xx/5xx passes too, so the intent had the class wrong, not just the code.

Now: 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 named (unclassified-302) instead of silently passing.

2. The fill turn was printed, never asserted

code1 was echoed and never checked, so the over-limit assertion could be made against a server that had already failed the
precondition. It is now asserted 2xx, and the failure says so: "the fill turn returned HTTP $code1, so the over-limit turn
proves nothing"
.

3. --self-test, and it is the point

The 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-test walks the
table: 000, 500, 400, 200 clean, 200 empty+finish_reason: length, 200 degenerate, 302.

Red proof — put the old rule back and the self-test fails on exactly the defect:

FAIL self_test: no response at all (the server died) -> 'refusal-empty', wanted 'died'
FAIL self_test: the server's own 500 -> 'refusal', wanted 'server-error'
FAIL self_test: a redirect nobody follows -> 'refusal-empty', wanted 'unclassified-302'
self-test: 3 failure(s)                                                          rc 1

Restored, green: rc 0, and the two that must stay accepted (400, 200 clean) still are, so the fix does not invert the bug.

Registration, and a placement mistake I made and fixed

serve_context_limit_classify is registered outside if(ONEBIT_SERVE_TEST_GGUF), next to the other script-based
device-free rows. My first attempt put it inside that block beside serve_context_limit_cpu — and ctest -R serve_context_limit_classify printed No tests were found!!!, because the guard's variable is empty by default, so the row
would 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_referenced still rc 0
(the new row names the script), and the script's no-argument usage path is unchanged.

… (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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_classify and 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: 4xx passes (the server refused), 200 + clean passes, 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 5c19d4a-equivalent head eb69d31

Both findings are real and the second is the one that matters: a test that prints its precondition never checks it.

  1. curl's 000 (no response — the server died) and any 5xx were classified as "a refusal", so a row whose entire purpose is the
    server survives a client hangup
    passed when the server had crashed. The header said 4xx/5xx passes, so the intent had the class
    wrong too — the test and its documentation agreed on the wrong thing, which is why review did not catch it. Now 000 -> died,
    5xx -> server-error, both fail with their own message, 4xx stays a refusal, 3xx is named (unclassified-302) instead of
    silently passing.
  2. code1 (the fill turn) was echoed and never asserted, so the over-limit assertion could be made against a server that had already
    failed the precondition — the classic "the test passes because step 1 broke". It is now asserted 2xx, and the failure message says
    what that means: "the fill turn returned HTTP $code1, so the over-limit turn proves nothing".

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
evidence for whoever reads the log; an asserted one is evidence for whoever reads the result.

Head under review: eb69d31

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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.

  1. curl's 000 (no response — the server died) and any 5xx were classified as "a refusal", so a row whose entire purpose is the
    server survives a client hangup
    passed when the server had crashed. The header said 4xx/5xx passes, so the intent had the class
    wrong too — the test and its documentation agreed on the wrong thing, which is why review did not catch it. Now 000 -> died,
    5xx -> server-error, both fail with their own message; 4xx stays a refusal; 3xx is named (unclassified-302) instead of
    silently passing.
  2. code1 (the fill turn) was echoed and never asserted, so the over-limit assertion could be made against a server that had already
    failed the precondition — the classic "the test passes because step 1 broke". It is asserted 2xx now, and the failure says what that
    means: "the fill turn returned HTTP $code1, so the over-limit turn proves nothing".

The general lesson worth spreading: every step a test depends on must be an assertion, not a print. A printed value is evidence for
whoever reads the log; an asserted one is evidence for whoever reads the result.

Head under review: eb69d31

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Print the classification ("crash" / "refusal" / "2xx") on failure so the next person does not re-derive which branch was taken.

@bong-water-water-bong
bong-water-water-bong merged commit 64ac77f into main Nov 10, 2026
9 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the os/v3-serve-classify branch November 10, 2026 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

serve tests: a crashed server passes as a refusal, and the context-limit test never asserts its fill turn

1 participant