Repository navigation
ci(quality): lint onnx-genai-engine and -server at default features too - #1973
Merged
Merged
Conversation
`Check the native backend compiles` is the only `-D warnings` gate in this file that names these two crates, and it turns the feature on. `Run clippy on all offline crates` cannot list them because they pull in `ort-sys`, and `CLI ORT`'s clippy steps name `onnx-genai-cli`, so `-D warnings` does not reach a dependency -- and dependencies do not build test targets anyway. So the *default* configuration of both crates was compiled by `Test ORT-backed workspace crates` and linted by nothing. Measured rather than assumed: #1964 merged with `warning: function `test_session_decoder_runtime` is never used` sitting in a green `CLI ORT` log. That warning is harmless. A real unused-code defect at default features has the same signature and the same fate -- a line in a log that is only read once something else has already gone red. Add the mirror-image step, and fix the one warning it finds: `test_session_decoder_runtime`'s only caller is `#[cfg(feature = "native-backend")]`, so the fixture is gated the same way. This is the other half of the hole #1961 closed. That one was `native-backend` code no fast lane compiled, which let a compile error sit on `main`; this one is default-feature code no lane lints. Both configurations are checked separately rather than as a union, because default is what ships. Most of the new step is cache-warm from the one above, which shares the whole dependency graph minus the feature. Closes #1969 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1973 +/- ##
==========================================
- Coverage 80.43% 80.40% -0.04%
==========================================
Files 409 423 +14
Lines 198253 207429 +9176
Branches 198253 207429 +9176
==========================================
+ Hits 159467 166775 +7308
- Misses 33345 35014 +1669
- Partials 5441 5640 +199
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…rries (#1982) ## What this adds A test that a **continuing** turn is charged for the conversation it carries, and a fresh turn is not. `admit_interpreted_generate_request` (`runtime.rs:846`) computes: ```rust let carried = if self.workflow_sessions.contains_key(&session_id) { self.workflow.session_prepended_prompt_len(&session_id.to_string()) } else { 0 }; let prompt_tokens = self.interpreted_prompt_token_count(prompt)? + carried; ``` The scheduler id chosen in `workflow_api.rs:206-216` is the whole input to that lookup. A **fresh** id makes `carried = 0`, so turn N is admitted as though it were turn 1, under-reserving by the entire conversation prefix — verbatim what the function's own comment warns about. ## Why it did not already exist I measured this, I did not infer it. Reviewing #1951 I ran a mutation battery on the merged tree; **M7 — disabling the session-reuse arm entirely — SURVIVED at 622 passed / 0 failed.** The only test reaching that branch (`native_generate_in_workflow_session_rejects_over_kv_byte_budget`) asserts a **refusal**, and a refusal arrives identically whether the id was reused or freshly minted. The observable it needs is on the *admitted* side. This also corrects the review record on #1951, which stated that coverage "returns when that fixture is repaired." The fixture is repaired — I repaired it in #1964 — and the gap is still open. ## Design **Two directions on one engine, path held fixed.** A single `interpreted_conversation_engine_with_byte_budget(30)` builds two sessions: one that has heard nothing, one seeded with 3 tokens. Same budget, same prompt, same call. Only the carry differs, so the fresh arm is the control that stops the test from passing by refusing everything. **Non-vacuity asserted, not hoped for.** The test asserts the seed is visible through `session_prepended_prompt_len` — the exact accessor the accounting reads — *before* exercising either arm. Without that, a seed that silently failed would make both arms cost the same and the test would be pinning the budget rather than the carry. **The refusal is matched specifically** (`scheduler admission failed: KV byte budget`). The fixture's interpreted decoder cannot finish a generation, so the admitted arm still returns `Err`; matching the admission refusal by name is what keeps "was not refused" from being satisfied by any other failure. ## A third fixture shape was required Neither existing fixture can express this. `Stateless` has no session state; `LoopCarriedSession`'s state is threaded by the loop. The shape the scheduler actually charges for is a **session-scoped cell the workflow never reads**, which the request binding prepends to each turn's prompt — and the validator refuses a cell that is both loop-carried and a continuation ("the lease and an SSA carry are two answers about the same value"), so it has to be its own cell. Hence `TestWorkflowShape::PromptPrefixConversation`, built against the constraints enumerated at `validation.rs:2540+`, with five construction-time `ensure!` self-checks. The test-only `seed_session_conversation` seeds through the **declared** continuation cell rather than a name spelled in the test, so a fixture that declares no continuation fails loudly instead of writing an entry nothing will ever read. ## Mutation battery: 3/3 caught | mutation | result | caught by | |---|---|---| | **M7** session-reuse arm unreachable (a continuing turn gets a fresh id) | **CAUGHT** | the continuing arm, both assertions | | **M8** `carried` always 0 | **CAUGHT** | the continuing arm, both assertions | | **M9** vacuity: admission refuses every workflow-driven turn | **CAUGHT** | the **fresh** arm only — opposite polarity | Tree verified byte-identical after the battery (`workflow_api.rs 3976c9c7a664`, `runtime.rs bf4d8e20ff56`), post-battery baseline PASS. **M9 is the arm that earns the fresh session.** It is the direction a one-sided test cannot see, and it is caught by the *opposite* arm from M7/M8 — which is the point of running the guard against the case it must accept as well as the case it must reject. ### The battery's first run reported 0/3, and that is why M9 exists Including M9, which refuses everything and therefore *must* fail. A vacuity arm reading SURVIVED is impossible, so the battery itself was broken: `cargo test --lib <NAME> -- --exact` with a **bare** test name matches nothing, exits 0, and is textually indistinguishable from a survived mutant. Repaired with the full module path plus an assertion that the run actually selected one test. Anyone running mutation testing here: **a filter that selects zero tests reads exactly like SURVIVED.** Assert your run selected something. ## Default-feature dead code, and a convention adopted from #1973 While validating I found `cargo clippy -p onnx-genai-engine --all-targets -- -D warnings` (default features) failing on `origin/main` — measured by checking `origin/main` out and running it, 1 error: `test_session_decoder_runtime` never used, because its only consumer is gated on `native-backend`. A landmine I introduced in #1964. This branch would have added two more. **#1973 landed the same finding while this PR was open**, and cites that exact warning as its motivating evidence. It added `Check the default features compile clean` to `rust-quality` (`ci.yml:517`) — a **required** job — so this is now enforced rather than latent. I had fixed it with `#[cfg_attr(not(feature = "native-backend"), allow(dead_code))]`. Main fixed it with `#[cfg(all(test, feature = "native-backend"))]`. Both work; **main's is now the convention, so I took main's** and applied it to my two new items as well. Consistency here is worth more than my marginal preference for keeping the items type-checked in both configurations. Verified with the exact command the new required lane runs: `cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server -- -D warnings` → clean. ## Validation | check | result | |---|---| | `cargo fmt --all -- --check` | clean | | `clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server -- -D warnings` (the new required lane from #1973) | clean | | `clippy -p onnx-genai-engine --features native-backend --all-targets` | clean | | `cargo test -p onnx-genai-engine --features native-backend --lib` | **623 passed / 0 failed** | | mutation battery | **3/3 caught**, tree byte-identical, baseline PASS | Run under `hostlock.sh run` with `taskset` **outermost**, on latest `main`. ## Limitation, stated plainly **No required CI lane runs these tests.** `onnx-genai-engine` is not in `workspace_test_packages.py` offline-linux. `rust-quality` is required and, since #1973, does now *compile and lint* this test at both default and `native-backend` features — but clippy does not run tests, so a test that compiles and **fails** is still structurally invisible to it. The engine `native-backend` tests run in `cli-ort` (`ci.yml:889`), which is advisory. So **a required green on this PR is not evidence for the test.** The mutation battery is the evidence. I would rather say that than let a green tick imply something it does not cover. Scope: 3 files, +289/-11, **tests and test fixtures only** — no production behaviour change. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 24, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…tted (#2000) 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. Co-authored-by: Gaff <gaff@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 24, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…e check (#2022) ## What this is #1868 corrected the spin-window deadline at **two** sites — it kept the stride in the pure-`spin_loop` phase and dropped it in the yield phase, in both `decode_spmd`'s readiness barrier and `task_runtime::pool`'s worker loop. **Only the `decode_spmd` site got a test.** This adds the missing guard for the other half. I found this while validating merged code, not while reading the diff. The finding is a mutation result, not an opinion about coverage. ## The gap, measured Baseline on `main`: `cargo test -p onnx-runtime-ep-cpu` → **1718 passed / 0 failed**. | mutation (reintroduce the stride in the yield phase) | result | |---|---| | both sites (`decode_spmd` + `pool.rs`) | **1717 / 1** — `decode_spmd::dispatch_claim_tests::the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride` | | `pool.rs` only (`decode_spmd` restored) | **1718 / 0 — SURVIVED** | One test covers both mutations only because one of them was never covered. The `task_runtime::pool` half of #1868 was held in place by nothing but the comment beside it. ## Why it is worth a test rather than a comment `SPIN_LOOP_BUDGET` (4096) is an exact multiple of `CLOCK_CHECK_STRIDE` (64), so the yield phase begins **on** a stride boundary and the next evaluation is 64 yields away. `MAX_SPIN`'s stated contract is ~0% CPU within a millisecond of going idle. Under the contention that makes a yield expensive — microseconds to milliseconds rather than the ~1.2us an uncontended one costs — a strided check holds a core for most of a second. On a shared box that cost lands on a co-tenant, i.e. it fails hardest in exactly the regime it exists for. ## How it is tested `spin_for_dispatch` is split out of `worker_loop` verbatim (the loop body is unchanged except `break` → `return SpinOutcome::*` and `thread::yield_now()` → `spin_yield()`, which *is* `thread::yield_now()` in a non-test build). The split is what makes the policy drivable: a real worker runs this loop on a thread the test does not own, so the only observable from outside the pool is wall-clock idle CPU, which does not discriminate on this box. Yield cost is injected through a `#[cfg(test)]` thread-local — the same technique `decode_spmd`'s test uses. Thread-scoping is what makes it sound: a real pool's workers never set it and always read `0`. **The observable is the yield count, deliberately.** It is monotone in the right direction under load — a starved thread accumulates wall time faster per yield, so it crosses the deadline in **fewer** yields, never more. Contention can therefore never turn a real failure into a pass. A wall-clock observable ("was it parked at T?") is not monotone that way and goes flaky beside 1700 siblings. `TaskPool::new(1)` spawns no threads, so the `Shared`'s epoch cannot move under the test. ## Falsified in both directions, on this exact tree - correct code → `test result: ok. 15 passed; 0 failed` (`--lib task_runtime::pool`) - stride reintroduced at the yield site → **14 passed / 1 failed**: > left the yield phase after **65** yields against a 200ms window and 10ms yields, i.e. the clock was not re-read on every yield **65 is the predicted number**, not a threshold tuned to fail: spins resume at 4097 and the next multiple of 64 is 4160. The every-yield form exits at ~20 (200ms / 10ms). The test also asserts **non-vacuity explicitly** (`yields >= 2`, with an "it is not a pass" message). If the window had already elapsed when the yield phase began, both the strided and unstrided forms exit on the first yield and the test discriminates nothing — that state must be reported as inconclusive, not green. This is the #1817 class, so the guard should not be able to join it. ## Local validation Rebased onto `1bf87c86a`, run under `scripts/hostlock.sh run --wait` (live PID anchor, no TTL), `taskset -c 8-15`, `CARGO_INCREMENTAL=0`: ``` cargo fmt --all -- --check -> clean cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warnings -> 0 cargo clippy -p onnx-runtime-ep-cpu --all-targets --features mlas -- -D warnings -> 0 cargo test -p onnx-runtime-ep-cpu -> 1719 passed / 0 failed / 24 ignored (+ 11 smaller suites green) ``` 1719 = the 1718 baseline + this test. Clippy is run on **both** feature arms because a `#[cfg(test)]` helper used by only one arm is dead code in the other, and #1973 made default-feature linting a required-lane concern. Local validation is necessary and not sufficient — this waits for the required GitHub checks and merges by normal auto-merge. No admin bypass. Refs #1868, #1825, #1817. --- ## Update: the Miri lane caught this, and it caught it the right way The first CI run went red on `Miri unsafe-crate soundness` — **on this test's own non-vacuity assertion**, not on a passing-but-empty green: ``` ---- task_runtime::pool::tests::the_spin_window_deadline_is_evaluated_on_every_yield_not_on_a_stride ---- inconclusive: left the yield phase after 0 yield(s), so the 200ms window had already elapsed before the second check. This test cannot tell a strided clock read from an unstrided one in that regime — it is not a pass ``` **Miri makes the test's premise false rather than its assertion wrong.** The test needs the spin phase to be short against the window — 4096 `spin_loop`s measure 128us natively against a 200ms window, so the yield phase is reached with nearly the whole window left. Under the interpreter those 4096 iterations and their strided `Instant::now()` calls outlast the window, so the yield phase is entered *already expired* and the strided and every-yield forms both exit at yield 0. That is precisely the regime the `yields >= 2` assertion was written to refuse, and without it this run would have been a **silent green in the lane where the test discriminates nothing** — the #1817 shape, in the guard for an #1817 instance. I would rather report the mechanism than the outcome: I did not predict Miri specifically, I asserted the condition the test depends on, and the condition is what failed. Fixed with `#[cfg_attr(miri, ignore = "spin-vs-window ratio is wall-clock, not emulated")]`, matching the precedent directly below it in the same module — `workers_park_when_idle_and_wake_again`, ignored under Miri for the same class of reason (a wall-clock policy, not a memory-model one). **It costs no coverage, checked rather than assumed:** ``` $ python3 .github/scripts/workspace_test_packages.py cargo-args offline-linux | tr ' ' '\n' | grep -x onnx-runtime-ep-cpu onnx-runtime-ep-cpu ``` `ci.yml:224` runs that package set in **`Fast (Linux x86_64)`, a required lane**, natively. The guard runs where its premise holds. Reproduced locally with the exact lane command (`miri.yml:137`), under hostlock and `taskset -c 8-15`: ``` MIRIFLAGS=-Zmiri-disable-isolation cargo +nightly miri test --locked -p onnx-runtime-ep-cpu --lib task_runtime:: -> 30 passed; 0 failed; 2 ignored (was 30 passed; 1 failed; 1 ignored) cargo test -p onnx-runtime-ep-cpu --lib task_runtime::pool -> 15 passed; 0 failed (the guard still runs, and still passes) ``` --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
… testing (#2039) `#1868` made **two** separable corrections at the CPU worker spin loops. Deleting either one should have reddened the suite. Only one did. This is the follow-up to my review of #1868 (comment `5401429803`) and a sibling of #2022, which closed the other half of the same gap. ## The finding Mutating each site independently, one at a time, full crate per mutant, restore verified by an empty `git diff` (baseline at the time: **1723 passed / 0 failed**): | | mutation | result | | |---|---|---|---| | M1 | put the stride back on `decode_spmd`'s **yield** check | 1722 / 1 | ✅ killed | | M2 | put the stride back on `pool.rs`'s **yield** check | 1722 / 1 | ✅ killed (#2022) | | M3 | *inverted*: drop the stride from `pool.rs`'s **spin** phase | 1723 / 0 | ✅ correctly not over-constrained | | **M4** | **delete the spin-phase check `#1868` added** | **1723 / 0** |⚠️ **SURVIVED** | M4 is the subject of this PR. ## Why the surviving half is the one that matters when the box is quiet The spin window is `(w*2).min(MAX_SPIN)` on catch and `(w/2).max(MIN_SPIN)` on park, so an idling process converges *down* to `MIN_SPIN` = **20us**. The pure-spin phase runs `SPIN_LOOP_BUDGET` = 4096 `spin_loop` iterations, which I measure at **130us** (median of 200 reps, `taskset`-pinned, under `hostlock run`; p10 129.4us, p90 139.0us). With no clock read inside that phase, a worker told to release its core after 20us **cannot look at the clock until 130us have passed** — a **6.5x overshoot at exactly the idle floor the window exists to bound**. On this box the surplus is spent on a co-tenant. That is the same shape as the gap #2022 closed, on the other half of the same PR. Which is the general lesson: **a fix applied at N sites has to be mutated at each site independently.** The site that survives is systematically the one hardest to reach from a test — which is usually also the one with the worse failure mode, because "hard to reach" and "only reachable when the system is in an unusual state" are the same property. ## The guard Calls `spin_for_dispatch` directly with a `MIN_SPIN` window on a width-1 pool. Width 1 spawns no workers (`requested = width.saturating_sub(1)`), so the epoch cannot move underneath it and the deadline is the only exit — asserted, not assumed, via `outcome == SpinOutcome::Expired`. The observable is the **spin count**, not elapsed time, for the same reason #2022's is the yield count: it is *monotone in the safe direction*. A preempted thread accumulates wall time faster per iteration and therefore crosses the deadline in **fewer** spins, never more — so load beside 1700 sibling tests can only push this towards passing. A wall-clock assertion here would be flaky on precisely the shared runner it is meant to protect. It is recorded into a `#[cfg(test)]` thread-local **on exit rather than per iteration** — a per-iteration `Cell` bump is a sizeable fraction of a ~31ns `spin_loop` and would distort the very phase under test. That is the only reason the loop is rewritten from `return` to `let outcome = loop { … break … }`. Both bounds are stated as **premises, not just outcomes**: - `spins >= CLOCK_CHECK_STRIDE` — `spins` is incremented *before* the check, so the earliest possible spin-phase exit is at 64. Anything below that means the loop did not leave through the spin-phase deadline at all, and the test says so instead of passing vacuously. - `spins < SPIN_LOOP_BUDGET` — the actual defect. ## Falsified in both directions, in both lanes | lane | tree | result | |---|---|---| | native | correct code | **1729 / 0** (25 ignored) | | native | M4 applied | **1728 / 1** — `ran the whole 4096-iteration spin budget against a 20µs window` | | Miri | correct code | **31 passed / 0 failed / 2 ignored** (was 30 / 0 / 2) | | Miri | M4 applied | **FAILS**, same message | The Miri row is worth noting: this guard is live in the interpreter lane too, unlike its yield-phase sibling. Under Miri the interpreted spin phase vastly outlasts a 20us window, so the correct code exits at the first stride boundary and the defective code still runs the whole budget — the discrimination survives, it just moves. ## Two corrections that came out of the same measurements **1. The yield-phase comment overclaimed its scope.** It said the stride bites "the whole grown range: … every window above the floor reaches the yield phase". Measured, that is too broad. The reachable window values are {20, 31, 40, 62, 80, 125, 160, 250, 320, 500}us, and against a 130us spin phase the five values **31–125us are above the floor and expire inside the spin phase**, never reaching the yield branch. Corrected scope: **~160us and up** — which still includes the `MAX_SPIN` 500us ceiling a busy steady state converges on, so #1868's justification is unchanged. But an over-wide claim in a comment is what the next reader checks the code against, and this one would have made the M4 gap *harder* to see, since it asserts the spin phase is never where a deadline lands. The same comment said a yield "costs microseconds to milliseconds under contention" without a number. Now it carries one: **11.2ms** with four runnable siblings pinned to one core (~9400x the uncontended 1193ns). So a 64-yield stride holds the core **717ms past a 500us window** — a 1434x overshoot. The claim was if anything understated. **2. `decode_spmd`'s sibling guard is a CI landmine, and is now defused.** `the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride` **fails under Miri** — verified under the lane's own flags, not assumed: `inconclusive: left the yield phase after 1 yield(s)`. Miri makes the test's *premise* false rather than its assertion wrong (4096 interpreted `spin_loop`s outlast the deadline, so the yield phase is entered already expired and the strided and every-yield forms become indistinguishable). It is green today only because `miri.yml` selects `decode_spmd::tests::a_panic_in_the_dispatcher` and **not** `decode_spmd::dispatch_claim_tests::`. It escapes **by filter, not by design** — so anyone widening that filter reds the lane with a message that reads like a defect in the code under test. Marked `#[cfg_attr(miri, ignore)]`, same spelling and same reasoning as the `pool.rs` sibling and as the pre-existing `workers_park_when_idle_and_wake_again` precedent. No native coverage is lost: `onnx-runtime-ep-cpu` is in the `offline-linux` set that the required `Fast (Linux x86_64)` lane runs natively (`ci.yml:224`). ## Scope Tests, comments, and one control-flow rewrite (`return` → `break`) needed to observe the spin count. **No behaviour change.** ## Validation run locally All under `scripts/hostlock.sh run --wait`, `taskset -c 8-15`, `CARGO_INCREMENTAL=0`, on `6a4b22eb1`: ``` cargo fmt --all -- --check clean cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warnings 0 warnings cargo clippy -p onnx-runtime-ep-cpu --all-targets --features mlas -- -D warnings 0 warnings cargo test -p onnx-runtime-ep-cpu 1729 / 0 MIRIFLAGS=-Zmiri-disable-isolation cargo +nightly miri test --locked -p onnx-runtime-ep-cpu --lib task_runtime:: 31 / 0 / 2 ignored ``` Both clippy arms are run because a `#[cfg(test)]` helper reachable from only one feature arm is dead code in the other (#1973). ## Note for #2027 @roy — #2027 also touches `crates/onnx-runtime-ep-cpu/src/task_runtime/pool.rs`. The hunks look disjoint (yours at `:108`, `:471–498` and the end of `mod tests`; mine inside `spin_for_dispatch` and mid-`mod tests`), but `:471` is adjacent to my last non-test hunk, so whichever lands second may need a trivial rebase. Flagging rather than serialising. Refs #1868, #2022, #1817 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 26, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 27, 2026
…it (#2278) Closes #2251. ## The defect `admit_interpreted_generate_request` gated the carried prefix length on one map and read its value from another: ```rust let carried = if self.workflow_sessions.contains_key(&session_id) { // engine-side, SessionId self.workflow.session_prepended_prompt_len(&session_id.to_string()) // pipeline-side, (String, cell) } else { 0 }; ``` `session_prepended_prompt_len` is **already total** — `let Some(cell) = … else { return 0 }` and `.unwrap_or(0)`, so it answers `0` for a session it does not know. That makes the gate's `else` arm identical to the callee's own answer: - maps **agree** → the gate is redundant, the callee returns the same value; - maps **diverge** (workflow holds a prefix, engine map does not) → the gate discards a correct non-zero count and forces `carried = 0`. **No input exists for which the gate improves the outcome.** Its only reachable effect is to under-reserve — exactly what the function's own comment warns about: > Admitting on the request alone under-reserves for exactly the turns that need the most. ## The fix Read it unconditionally. The sole caller is the workflow path, so no non-workflow hot path pays for the removal. Production change is 6 lines out, 3 lines in. ## Why this needed a new test rather than an existing one This is **latent, not live** — `close_session` maintains both sides, and the caller-side hazard is already closed. So the agreeing case cannot distinguish the two versions, which is precisely why deleting the gate leaves the rest of the suite green. A change that no test can fail is a change with no evidence. The new test builds the divergence directly: create a session, seed its conversation, then drop it from the engine-side map only — the state any future close/reset/eviction path that skipped `forget_session` would leave behind. **Confirmed failing before the fix**, on the same commit: ``` a forgotten session holding 3 tokens: the carried conversation was not charged, so a turn over budget was admitted; got: <admitted> test result: FAILED. 0 passed; 1 failed; 668 filtered out ``` A turn needing 5 tokens of budget was admitted against a budget of 3. The failure mode is silent by nature — an admission that should have been a refusal, not an error. Two controls make the arms mean something, both asserted rather than assumed: - `workflow_sessions.remove(...).is_some()` — `create_session` is what inserted the id, so the removal is real. A silently-failed removal would send both arms down the agreeing path and pass while proving nothing. - `session_prepended_prompt_len(...) == seed.len()` — the conversation is still readable from the map the carry is read from, per arm. **Each arm gets its own engine.** `admit_*` takes a reservation that only `complete()` returns, so a shared engine would let the admitted arm's leftover reservation pay for the other arm's refusal — and the test would pass with the carry ignored. ## Verification | check | result | |---|---| | new test, **before** fix | **FAILED** (the falsifier above) | | new test, after fix | ok — `1 passed; 668 filtered out` | | `a_continuing_turn_is_admitted_for_the_conversation_it_carries` (#1982, the agreeing case) | ok — behaviour-identical today | | full engine lib suite | **668 passed; 0 failed; 1 ignored** | | `cargo fmt --check` | clean | | `cargo clippy --all-targets -D warnings` | clean | | `cargo check` at **default features** (#1973 lane) | clean | Both targeted runs used `--exact` and report `1 passed / 668 filtered out`, so the filter demonstrably matched — per the empty-filter hazard `scripts/test_step.sh` guards against. ## Limitations The divergence is constructed by the test, not reachable through any current public path. This does not fix a live bug; it removes a gate whose correctness depended on an invariant maintained in a different module from the one reading it, with nothing asserting that invariant — and it adds the assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1969.
The gap
Check the native backend compiles(ci.yml:493) is the only-D warningsgate inci.ymlthat namesonnx-genai-engineoronnx-genai-server— and it turns the feature on.Run clippy on all offline cratesort-sysClippy onnx-genai-cli(CLI ORT)-p onnx-genai-cli;-D warningsapplies to named packages, and dependencies don't build test targetsClippy ... with native CUDAnative-cudaimpliesnative-backendTest ORT-backed workspace cratesSo the configuration that ships was compiled and linted by nothing.
Measured, not assumed
#1964 merged with this in a green
CLI ORT (Linux x86_64)log,Test ORT-backed workspace cratesstep:The warning is harmless — a
#[cfg(test)]fixture whose only caller is#[cfg(feature = "native-backend")]. What is not harmless is that nothing could turn it red. A genuine unused-code defect in either crate at default features has exactly the same signature, lands in the same log, and that log is only read once something else has already failed.This PR
The mirror-image step. Same two crates, no features,
-D warnings. Separate from the native-backend step rather than a union, because default is what ships and a union stops checking it on its own — same argument the existingClippy onnx-genai-cli/... with native CUDApair already makes two steps below.The one warning it finds, gated to match its only caller:
The gate is not vacuous — falsified in both directions
The step is worth nothing unless it fails on the defect it was added for, so I ran it against the pre-fix tree:
cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server -- -D warnings#[cfg(test)], as merged onmain)error: function `test_session_decoder_runtime` is never usedThat is the whole argument for the step: it goes red on the exact state
mainis in right now, and green after a two-word change.Validation
Linux x86_64,
CARGO_INCREMENTAL=0,taskset -c 16-23, underscripts/hostlock.shwith a declared reason. Not claiming an idle host — pass/fail results, not timings.cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server -- -D warnings(the new step)cargo clippy ... --features onnx-genai-engine/native-backend,onnx-genai-server/native-backend -- -D warnings(existing step, unchanged)cargo test --locked -p onnx-genai-engine --features native-backend --lib621 passed; 0 failedcargo test --locked -p onnx-genai-engine(default features)cargo fmt --all -- --checkci.ymlparses; new step present inrust-qualityBackups restored with
cp+touch, nevermv—mvgives the restored file the backup's older mtime and cargo then silently reuses the artifacts built from the mutant.Cost
One
cargo clippyover two crates on a job that already builds both. It shares the entire dependency graph with the step above minus one feature, so almost all of it is cache-warm; the incremental run measured 10.9s here after the native-backend step.Scope
Deliberately just these two crates — they are the ones with the demonstrated hole and the ones #1961 already pairs. #1969 raises the general question (for each feature configuration a crate ships or tests, which lane goes red if it breaks?) for the rest of the workspace; that is a survey, not this change.