Skip to content

test(server): the driver must tell a tensor-bound request it was admitted - #2000

Merged
justinchuby merged 2 commits into
mainfrom
squad/gaff-1995-bound-admission
Aug 24, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/gaff-1995-bound-admission

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Closes #1995.

run_generation's success arm never touches the admission sender — only its error arm does. So the Some(&mut admitted) argument on the (Some(bound), session) arm (driver.rs:1654) is the only thing that resolves that oneshot with Ok on a request that succeeds, and the server is holding the other end waiting to send response headers. Deny it and every successful tensor-bound request 500s at the awaiting end while the generation completes perfectly — the loudest failure in the quietest place.

Nothing covered that arm. One test, +104 lines, no production change.

The gap, measured rather than argued

mutation result
whole admitted closure inert 4 failed — all streaming, all on the (None, None) prompt arm
(Some(bound), session) arm alone passes None 0 failed, 296/296 across all four server targets, tree byte-identical

The engine's own guards — a_tensor_bound_request_signals_admission_exactly_once and its refuse-direction sibling — do not substitute. They assert the engine invokes the callback it is given. This is the driver not giving it one. The engine can be entirely correct and the request still 500s.

That distinction is the reusable part: "is this covered?" and "is this covered on the path I changed?" are different questions. The first answers yes here, and the second answers no.

Why this was writable, contra the note on the engine-side test

a_tensor_bound_request_signals_admission_exactly_once records its limitation as: "an end-to-end version needs a package the interpreter can finish, and that fixture does not exist yet."

It doesn't. The generation is not required to succeed for the driver contract to discriminate, because the admission signal precedes the work and the error arm's take() separates the cases:

  • given the callback — it fires at admission, take()s the sender, so the error arm finds None: the receiver already holds Ok(());
  • denied the callback — the sender survives to the error arm, which sends Err(failure).

So the received value is the observable. Asserting Ok(()) rather than merely "the receiver resolved" is load-bearing: a test that only checked for resolution passes under the defect, since a denied callback still resolves the oneshot — with the wrong answer. That is the falsifier-that-passes-under-the-defect shape, avoided deliberately.

It also means no new fixture: the ordinary tiny-llm package works, because binding any input makes the request non-prompt-only and routes it to the branch under test regardless of decode core.

Mutation battery — 6/6

M0  baseline, no mutation                                   expect PASS  got PASS  (1 passed, 0 failed)
M1  THE DEFECT: bound arm denied the callback               expect FAIL  got FAIL  (0 passed, 1 failed)
M2  superset: the whole closure is inert                    expect FAIL  got FAIL  (0 passed, 1 failed)
M3  specificity: the *prompt* arm denied it instead         expect PASS  got PASS  (1 passed, 0 failed)
M3b that same mutation, seen by the tests that DO cover it  expect FAIL  got FAIL  (254 passed, 3 failed)
M4  whole suite at baseline with the new test present       expect PASS  got PASS  (257 passed, 0 failed)
restore: 7bdf931ebd3a -> 7bdf931ebd3a : identical

M3 + M3b are the arms that matter, and they are the ones I would have skipped a year ago. M1 alone only shows the test fails on something. M3 shows it passes when the neighbouring arm breaks, and M3b shows the three streaming tests fail on that arm and are blind to this one. Together they establish the coverage is complementary rather than overlapping — which is the actual claim, and it is not implied by M1.

M3b's failures:

tests::accepted_streams_preserve_first_chunk_and_chat_protocol_order
tests::accepted_zero_visible_output_stream_returns_headers_and_terminates
tests::streaming_chat_and_completion_chunks_include_logprobs

A correction to my own instrument, since it is the more useful half. M3b first reported PASS where it must report FAIL — I had filtered on admission, which selected 18 tests, none of them the streaming ones that actually cover the prompt arm. The filter matched plenty and still matched the wrong thing, so it read as a clean result rather than a broken instrument. Re-run unfiltered it reports correctly. This is the same species as a filter selecting zero tests and exiting 0 — a battery arm can only falsify what its selector can see, and a plausible-looking selector hides that as effectively as an empty one.

Validation

cargo fmt --all -- --check                                                    clean
cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server -- -D warnings
                                                                              exit 0   (the required lane added by #1973)
cargo test -p onnx-genai-server --all-targets --no-fail-fast                  257 + 0 + 40 = 297 passed, 0 failed

297 = the 296 baseline Pris measured, plus this one.

Limitations, stated rather than implied

  • This pins that the driver passes the callback on the bound arm. It does not pin an end-to-end multimodal generation returning Ok — the tiny-llm fixture has no declared workflow, so the run errors after admission. That is sufficient for this contract and insufficient for a broader one, and it is why the assertion is on the received value.
  • The fourth failure under the whole-closure mutation lives in tests/http.rs, an integration target outside --lib; only three appear in M3b's lib-scoped run. Consistent, not contradictory.
  • MultimodalInput::bind does not validate the bound name against the package, so this proves nothing about tensor-name resolution. Different contract, not covered here either.

Gaff and others added 2 commits August 24, 2026 15:20
…tted

`run_generation`'s success arm never touches the admission sender -- only
its error arm does. So `Some(&mut admitted)` on the `(Some(bound), session)`
arm is the only thing that resolves that oneshot with `Ok` on a request that
succeeds, and the server holds the other end waiting to send headers. Deny
it and every successful tensor-bound request 500s at the awaiting end while
the generation completes perfectly.

Nothing covered that arm. Measured, not argued: replacing `Some(&mut
admitted)` with `None` there alone left all 296 tests across all four server
targets passing, byte-identical to baseline.

The engine's own guards cannot see it -- they assert the engine invokes the
callback it is given, and this is the driver not giving it one.

Closes #1995

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.44%. Comparing base (f89cbe6) to head (f919802).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2000      +/-   ##
==========================================
+ Coverage   80.24%   80.44%   +0.19%     
==========================================
  Files         408      425      +17     
  Lines      189125   208150   +19025     
  Branches   189125   208150   +19025     
==========================================
+ Hits       151768   167439   +15671     
- Misses      31946    35068    +3122     
- Partials     5411     5643     +232     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (?)
cli-ort-windows 72.01% <ø> (?)
mlas 85.20% <ø> (?)
offline 80.57% <ø> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 67 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby
justinchuby merged commit 2c4e603 into main Aug 24, 2026
15 of 17 checks passed
@justinchuby
justinchuby deleted the squad/gaff-1995-bound-admission branch August 24, 2026 16:07
justinchuby added a commit that referenced this pull request Aug 25, 2026
…rong test (#2103)

## What

§9 of `measurement-discipline` guards selector **cardinality** — `--list
| grep -c ': test$'` must resolve to exactly 1, and the run output must
then read `1 passed`. Both checks answer *how many tests ran*. Neither
answers *whether they were the tests that cover the mutated code*, and a
mutation battery only carries its claim if the answer to the second is
yes.

This adds the distinction, the falsifier, and a `**Check:**`.

## The gap, measured

Purpose-built two-test crate; filter `--exact` onto a test that does not
touch the mutated function:

```
precheck  n = 1                                    (the -eq 1 guard is SATISFIED)
mutate    covered(a) -> a + 1  becomes  a + 99
arm       expects FAIL, gets   1 passed; 0 failed  -> reads "survived"
control   unfiltered            1 passed; 1 failed -> the mutant IS caught
```

The guard never fires. The arm reports a clean PASS over a live mutant.

Worth being precise about what §9 already covers, since this was my
first reading and it was wrong: §9's guard is `-eq 1`, **not** `>= 1`,
so it *does* reject the 18-test case on cardinality alone. The residual
gap is narrower and nastier — **identity, not cardinality**. When the
selector resolves to exactly one test and that one is wrong, every
existing check in the section passes.

## Provenance

#2000 (closing #1995) hit the same shape at a larger count: a substring
filter selected 18 tests, none covering the arm under test, and reported
PASS. A non-zero selection *suppresses* the suspicion an empty one would
raise, which makes it strictly harder to catch than the vacuity case
already documented.

I verified the load-bearing half from the tree rather than relaying it:
the three tests that do cover that arm exist, and none of their names
contains the filter word. The count `18` is attributed to that battery's
own output and labelled as such — I did not rebuild the server crate to
re-derive it, and the text says so rather than implying I measured it.

## The snippet is executed, not asserted

The `**Check:**` ships a snippet, so it was run in both directions plus
a firing control:

| case | result |
|---|---|
| wrong filter, mutation live | `ARM-DRIFT`, exit 2 ✅ |
| right filter, mutation live | `arm OK`, exit 0 ✅ |
| right filter, **mutation reverted** (control) | `ARM-DRIFT` ✅ — the
check fires when there is nothing to catch |

Without the third row the first two prove only that the command runs.

## Scope

Docs only — one file, +38/-1. No code, workflow, or test behaviour
changes. The frontmatter `source:` gains `#1995/#2000 selector
identity`, matching the existing `#1619/#1982 selector vacuity` form.

Incidental confirmation while building the falsifier: `cargo test --lib
<bare_name> -- --exact --list` on a test inside `mod tests` printed `0
tests`, exit 0 — §9's own opening claim, reproduced independently.

## Expected CI

This should classify **docs-only** and skip both required checks, which
also makes it a live exercise of the classifier merged in #2081. I will
confirm the skip comes from correct classification rather than from the
fail-closed guard before merging.

Requesting independent Opus review.

---------

Co-authored-by: Holden <holden@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

The tensor-bound arm of run_generation has no test that it signals admission: nulling it alone leaves the server suite green

2 participants