Repository navigation
fix(server): update the native-backend test call site for generate's third argument - #1944
Merged
Merged
Conversation
…third argument
`EngineDriver::generate` grew an `input: Option<MultimodalInput>` parameter, but
the one call site in `tests.rs` behind `#[cfg(feature = "native-backend")]` was
not updated, so `onnx-genai-server` fails to compile with that feature:
error[E0061]: this method takes 3 arguments but 2 arguments were supplied
--> crates/onnx-genai-server/src/tests.rs:2601:29
This is a compile error on `main`, not a test failure. It takes down the
`CLI ORT` lane at its last step, `cargo clippy -p onnx-genai-cli
-p onnx-genai-server --features .../native-cuda --all-targets -- -D warnings`,
because `native-cuda` implies `native-backend`.
It survived because no default lane compiles this test: the feature is off
everywhere except `CLI ORT`, so an API change to a `pub(crate)` method could
not be caught at the point it was made. Passing `None` restores the previous
behaviour of the call, which supplied no multimodal input.
Measured, same commit, `59f6c6cde`:
unmodified clippy exit 101 error[E0061]
with fix clippy exit 0
-p onnx-genai-server --features native-backend --lib: 262 passed, 0 failed
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 #1944 +/- ##
==========================================
+ Coverage 80.19% 80.30% +0.10%
==========================================
Files 400 422 +22
Lines 186471 206747 +20276
Branches 186471 206747 +20276
==========================================
+ Hits 149540 166020 +16480
- Misses 31557 35098 +3541
- Partials 5374 5629 +255
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
…t missed it (#1961) ## `main` does not compile with `native-backend` Two independent breaks, both in code that **no default lane builds**. `Fast`, `Rust quality` and every coverage lane are green right now with both present. ### 1. `paged_gqa.rs` — the urgent one #1956 threaded a `head_sink: Option<f32>` parameter through `sdpa_decode_row` and `sdpa_decode_row_accessor`. Two call sites in `crates/onnx-genai-engine/src/native_decode/paged_gqa.rs` were not updated: ``` paged_gqa.rs:164: error[E0061]: this function takes 10 arguments but 9 were supplied paged_gqa.rs:247: error[E0061]: this function takes 11 arguments but 10 were supplied ``` **`None` is not a guess.** `PagedGqaConfig` has no sink term, `head_sink` appears nowhere in this crate, and `None` reproduces exactly what these calls did before the parameter existed. It also matters that this is right rather than merely compiling: this paged path is the **parity oracle** for the flat one, so a wrong sink value would silently desynchronise the two while still building. `paged_matches_flat_*` is the test that would catch that, and all seven pass. Note this is the **third** call site #1956 missed — #1958 is fixing the ep-cpu bench, and neither it nor any other open PR touches `paged_gqa.rs`. ### 2. The check that should have caught #1944 covered only half the surface The "Check the native backend compiles" step builds `-p onnx-genai-engine` alone. A `#[cfg(feature = "native-backend")]` test in `onnx-genai-server` called `EngineDriver::generate` with its pre-third-parameter arity, and `main` sat uncompilable while every default lane stayed green (#1944). The only job that did compile it is `CLI ORT`, which is slow and has been red for unrelated reasons — so the signal existed but never arrived anywhere useful. Add the server crate to the same step. That step's own comment already names this exact failure mode: > *onnx-genai-engine's native decode path only compiles when this feature is on. > Without this step a refactor can leave it uncompilable while every other job > still passes.* The gap was never the idea — only its coverage. ## Verification Locally on a CUDA host, reproducing before and confirming after: | Command | before | after | |---|---|---| | `clippy --locked --all-targets -p onnx-genai-engine --features native-backend` (today's CI step, unchanged) | 2x `E0061` | **exit 0** | | the same, widened to `-p onnx-genai-server` (this PR's step) | 2x `E0061` | **exit 0** | | `cargo test --locked -p onnx-genai-engine --features native-backend paged` | — | **12 passed**, including all seven `paged_matches_flat_*` | `ci.yml` parses under `yaml.safe_load`. ## What I did not verify - I did not measure what the widened step adds to that lane's wall time. The server crate's dependency tree overlaps the engine's heavily, so the marginal cost should be small, but that is reasoning rather than a measurement. - I did not audit the rest of the workspace for further #1956 call sites beyond the two here and the bench in #1958. The widened step will now surface any that live behind `native-backend`; ones behind other unbuilt features would not be covered by this change. - This does not address the general class — features that no default lane compiles. It closes the two instances that are breaking `main` today and the one lane gap that let the second through. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
This was referenced Aug 24, 2026
Closed
Merged
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…admission (#1891) (#1951) A request that binds a tensor never passed through scheduler admission. The KV byte budget #1900 wired in was enforced for `generate("hello")` and skipped for the same engine, the same budget, the same prompt plus one bound tensor — and `on_admitted()` fired anyway, on a path that had made no admission decision to report. This is the residual of #1891. Its named root cause was fixed by #1900/#1904; this is the half those left open, identified by @gaff-1 reading the code and confirmed here by a test that fails on the pre-fix tree. ## The defect `generate_with_pipeline_callbacks` splits on `prompt_only`: ```rust let prompt_only = request.inputs.is_empty() && request.component_overrides.is_empty(); if prompt_only && self.holds_decode_core() { ... } // decode core: admits if !prompt_only { if let Some(on_admitted) = on_admitted.as_mut() { on_admitted(); } return self.run_declared_workflow_generation(request, callback); // <- returns } // only prompt-only, no-decode-core requests reach the admitting block below ``` The `!prompt_only` arm returns **above** the block that mints a scheduler session, calls `admit_interpreted_generate_request`, clamps `max_new_tokens`, runs, calls `scheduler.complete()`, and reports `budget_cap`. So a tensor-bound request got none of it: - **no byte budget.** The request is never refused at the door and never clamped. - **no reservation lifecycle.** `scheduler.complete()` is not called because nothing was started. - **no `budget_cap` in the result.** The caller is told nothing about a cap that was never applied. - **`on_admitted()` fires with nothing behind it.** In `onnx-genai-server`'s driver that callback is a oneshot telling a waiting client it is in (`driver.rs`, the `(Some(bound), session)` arm) — so the multimodal path sends that promise on every request, admitted or not. The `!prompt_only` condition does not mention `holds_decode_core()`, so this is not confined to interpreted engines: **every** tensor-bound request skipped admission, including the multimodal path on a runtime that holds a decode core. ## Why no lane caught it Every admission test written for #1900/#1904 sends a *prompt*: | test | request | |---|---| | `native_generate_rejects_over_kv_byte_budget_before_backend_run` | prompt | | `native_generate_in_workflow_session_rejects_over_kv_byte_budget` | prompt | | `an_admitted_interpreted_request_releases_its_reservation_for_the_next_one` | prompt | The absence of a tensor-bound admission test **is** the defect, so a green suite was never evidence the hole was closed. Deleting the entire `!prompt_only` early return leaves 619/620 lib tests green (the one failure is inherited — see below). ## The test fails today for the admission reason Not for the `decoder_kv.0000` interpreter symptom #1891 opens with. On the pre-fix tree: ``` one bound tensor: over-budget request was not refused at the door; got: workflow request supplied undeclared application inputs: ["pixel_values"] one bound tensor: on_admitted() fired with no admission decision behind it one component override: over-budget request was not refused at the door; got: workflow component 'decoder' does not allow application replacement one component override: on_admitted() fired with no admission decision behind it ``` The `admitted` assertion is the load-bearing one: it does not depend on any error string, and it is decisive *because of the control*. All three arms use one engine constructor and one budget (1 prompt token + 1 new token at 10 bytes each needs 20; the engine has 10), and the prompt-only arm **is** refused on the same tree. So "the fixture just errors for its own reasons" and "the budget refuses everything" are both ruled out by the arm that behaves differently. The component-override arm is there because `!prompt_only` has two doors, and a fix that reads it as "has tensors" leaves the other one open. ## Mutation battery — 4/4 caught, including the opposite polarity `.validation-worktrees/pris_1891_admission_mutations.py`. Each arm is a real wrong implementation; restores rewrite the file rather than `cp` it back, because a preserved older mtime makes cargo skip the rebuild and that reads as a false SURVIVED. | mutation | caught by | |---|---| | **M1** restore the `!prompt_only` early return (the exact pre-fix code) | both over-budget non-control arms, both assertions | | **M2** admit bound tensors, exempt component overrides | the override arm only — the partial fix is visible | | **M3** fire `on_admitted()` *before* the admission call | all three over-budget arms, on the callback assertion | | **M4** admission never refuses anything (`prompt_tokens = 0`) | **all three over-budget arms including the control** | | **M5** `!prompt_only` always refuses | **the within-budget arm only** — added after review, see below | M4 and M5 are the two vacuity checks, in opposite directions. Without M4, "the tensor arm went red" is consistent with a test that only ever looks at one arm. Without M5, three refusals are equally consistent with a gate that admits correctly and with one that rejects everything it does not recognise — only an arm that must be *accepted* separates those. M4's output also shows exactly the trap #1891 warns about — with admission broken, the control arm's error becomes: ``` prompt-only (control): ... got: workflow component 'decoder' input 'past.key' references unavailable value 'decoder_kv.0000' ``` That is the symptom the issue is titled after. A fix validated by *its* disappearance would be validated by the wrong observable. Post-battery the tree is byte-identical to the start and the baseline passes again. ## The fix Six deleted lines. The `!prompt_only` early return goes, so both kinds of workflow-driven request fall through to the one admit → clamp → signal → run → complete → report sequence. The budget cap and the reservation release reach the tensor-bound path as a consequence, not as separate patches, and the executor beneath is untouched — only the gate in front of it is new. Not deleted: the `holds_decode_core()` branch above it. @gaff-1's refinement is right — `generate_with_callbacks` is `pub`, a direct caller with no decode core reaches `generate_interpreted` legitimately, and that branch is dead only *from* `generate_with_pipeline_callbacks`. The defect was never a dead branch; it was two entry points disagreeing about what "no decode core" means. ## Review, and the finding that changed the diff An independent Opus review returned **APPROVE WITH NON-BLOCKING FINDINGS**, and the first finding was a real defect in my own change, so it is recorded here rather than summarised away. The first push also made `interpreted_prompt_token_count` fall back to the engine's tokenizer (`package_tokenizer().or(self.tokenizer.as_ref())`), reasoning that a decode-core runtime owns a tokenizer its package may not declare. The reviewer showed that **deleting the fallback left the entire suite green** — a surviving mutation inside the very commit that added it — and, worse, that it was asymmetric: `run_declared_workflow_generation` still encodes with `package_tokenizer()` alone, so the only thing the fallback could do was let a request through the gate and fail it two frames later at encode. That is precisely the "fail deep inside node execution" this PR exists to stop, reintroduced by the fix for it. Dropped in `0b0422735`. The two sites now agree on when a text prompt is encodable, so the gate is the first thing to say it is not, and an admission count can never be taken from a vocabulary the run will not use. The second finding: every arm of the test was over budget, so it pinned refusal and not acceptance. Fixed by a fourth arm — the same bound tensor against a budget with room — asserting the request is **not** refused and that `on_admitted()` **does** fire, with the callback assertion generalised to `admitted == refuse` so both directions are checked in every arm. That arm is the only thing that catches M5. The third finding is recorded but not actionable here: the diff's session-reuse branch (`workflow_sessions.contains_key(...)`) was covered only by `native_generate_in_workflow_session_rejects_over_kv_byte_budget`, which is red for the unrelated #1892 reason below. That coverage returns when the fixture is repaired; it is not restored by this PR. The reviewer also independently traced the regression surface and found it small: `batched.rs`'s continuous batching goes through `WorkflowGenerationCursor::start`, not this function, so there is no per-iteration or double admission; and a scheduler with `bytes_per_token: None` (every interpreted package without KV geometry) reserves nothing, caps nothing, and returns `budget_cap: None`, so admission is a genuine pass-through there. Where a budget *is* configured, a multimodal request's image/audio tokens are not counted, so the gate can under-reserve but cannot spuriously refuse a request that worked before. ## Validation All under `scripts/hostlock.sh run` (no `--ttl`, per #1869), `taskset -c 8-15`, `CARGO_INCREMENTAL=0`, on `683861ff5`. ``` cargo fmt --all -- --check -> clean RUSTFLAGS="-D warnings" cargo clippy -p onnx-genai-engine \ --features native-backend --all-targets -> 0 warnings RUSTFLAGS="-D warnings" cargo clippy -p onnx-genai-server -p onnx-genai-cli \ --all-targets -> 0 warnings cargo test --no-fail-fast -p onnx-genai-engine \ --features native-backend --all-targets -> 86 targets, 783 passed, 1 failed (inherited, below), 19 ignored cargo test -p onnx-genai-server -> 256 + 40 passed, 0 failed ``` `--no-fail-fast` is not decoration: without it cargo abandons the remaining 85 targets the moment the lib target reports the inherited failure, and the run reports one number that describes one target. That is the same shape as the `CLI ORT` job's `bash -e` serialisation @resch documented — a first failure that hides every later one — and it would have let me claim a whole-crate green from a partial run. The server run matters more than its size: `driver.rs` is the one production caller that passes a real `on_admitted`, and the multimodal arm is the one whose behaviour changes. ## The one failure is inherited, and I checked rather than asserting it ``` engine::runtime::tests::native_generate_in_workflow_session_rejects_over_kv_byte_budget ``` Reproduced on **clean `origin/main` with this branch's commit absent** — `git switch --detach origin/main`, same command, same failure. Introduced by #1892, which made `create_session()` require the package to declare session-scoped state; the `test_decoder_runtime()` fixture declares none, so the test now dies at `create_session()` *before reaching the admission assertions it exists to make*. Diagnosed independently by @gaff-1 at the bottom of #1944. Not fixed here — it needs a fixture that declares the state, and it is not this PR's concern. ## Worth recording: no required lane runs these tests `onnx-genai-engine` is not in `workspace_test_packages.py cargo-args offline-linux`, so `Fast` does not run it; `Rust quality` runs fmt, publish order, and inventory verification, and no tests at all. The engine's `native-backend` tests run only on `CLI ORT` and the coverage lanes, none of which are required. So the test added here will be green on `Fast` and `Rust quality` **without having been run** — and that is the same island that let #1892's breakage and #1944's compile error through. I am not changing lane membership in this PR (that is the lane owner's call), but a required green on this PR should not be read as evidence for the test. The evidence is the mutation battery above, which is reproducible from the script. Refs #1891, #1944. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 24, 2026
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
2 participants
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.
maindoes not compile withnative-backendenabled. This is one of the two things keeping theCLI ORTlane red; the other is separate and described at the bottom.The failure
EngineDriver::generategrew aninput: Option<MultimodalInput>parameter. The one call site intests.rswas not updated. PassingNonerestores exactly what the call did before, since it supplied no multimodal input.This is a compile error, not a test failure — it takes the lane down at the last step,
cargo clippy -p onnx-genai-cli -p onnx-genai-server --features onnx-genai-cli/native-cuda,onnx-genai-server/native-cuda --all-targets -- -D warnings, becausenative-cudaimpliesnative-backend.Why it survived
The enclosing test is
#[cfg(feature = "native-backend")], and no default lane compiles it.Fast,Rust quality, and the coverage lanes are all green onmainright now with this error present. The only job that enables the feature isCLI ORT, so an API change to apub(crate)method could not be caught anywhere near the point it was made.That is worth noting beyond this one fix: a
pub(crate)signature change is normally caught by the compiler, which is why it needs no test. That guarantee silently does not hold for call sites behind a feature no routine lane builds.Validation
Same commit (
59f6c6cde), baseline and fix measured back to back, with atouchbetween them so neither run could reuse the other's artifacts:mainerror[E0061]The previously-uncompilable test itself now runs and passes:
This does not fully green the lane
Being explicit so the PR is not read as more than it is.
CLI ORThas a second, unrelated blocker in a different step:Deterministic on
59f6c6cde, introduced by #1892, which madecreate_session()require the package to declare session-scoped state. The test's fixture does not, so it now fails atcreate_session()before reaching the admission assertions it exists to make — the same "fixture quietly lost its premise" shape as #1879. Filing that separately; it needs a fixture that declares the state, not a relaxed assertion.Also worth recording against #1891: the test that issue named,
native_generate_rejects_over_kv_byte_budget_before_backend_run, passes on currentmain. That root cause is fixed. #1891's own warning — "it may be a different red" — turned out to apply to itself twice over.Refs #1891.