fix(engine): repair the session test #1892 broke, and cover both directions of what it guards

[justinchuby]

main is red, and has been since #1892

On a pristine bb39883e3, clean git status:

$ cargo test --locked -p onnx-genai-engine --features native-backend --lib -- --test-threads=1
test result: FAILED. 618 passed; 1 failed; 1 ignored
    engine::runtime::tests::native_generate_in_workflow_session_rejects_over_kv_byte_budget

Attribution by build, not by git log:

revision	result
387f840b0 (parent of #1892)	1 passed
08bc957f5 (#1892)	1 failed

The test was added by #1900 (cd99e5ceb). #1892 (08bc957f5) merged later and broke it. It runs in cli-ort (ci.yml:889), a lane that has been red for long enough that its colour stopped being read — and rust-quality never caught it because that job runs cargo clippy, so a compiling test that fails is invisible to the required check.

What actually broke, and why the fix is not to relax the test

The test fails at engine.create_session(), before reaching the assertion it exists for. #1892 added a gate:

if publishes_tokens && !workflow_carries_session_state(workflow) {
    return Err(PackageCapabilityError::NoSessionState.into());
}

That gate is correct, and the fixture is honestly caught by it: test_decoder_runtime builds a bare decoder whose state is all scope: invocation, which is a package that restarts from its own prompt every turn. Opening a session over it would be meaningless. #1900's test simply predates the rule.

So the fix gives the session test a package that declares a conversation. test_session_decoder_runtime is the same canonical decoder with its loop-carried cells promoted to scope: session:

• it is the shape a real multi-turn decoder declares — the cells the loop already threads between iterations are the ones a session threads between turns;
• it is one flag away from the stateless fixture, so a session test and a stateless test differ in exactly the declared property rather than in two unrelated fixtures;
• the builder asserts workflow_carries_session_state afterwards — the same predicate create_session reads — so the fixture cannot silently degrade into the stateless one if the promotion ever stops matching.

The repaired test now reaches admission and is refused there, which is the thing #1900 wrote it to prove. Its assertions are untouched.

Two tests added, and the mutant each one kills

Everything below is one revision, one command, --test-threads=1.

tree	result	sole failure
pristine bb39883e3	618 / 1 failed	the #1900 test
+ fixture repair	620 / 0	—
+ both new tests	621 / 0	—
NoSessionState gate disabled	620 / 1	create_session_refuses_a_package_that_declares_no_conversation
on_admitted() deleted from !prompt_only	620 / 1	a_tensor_bound_request_signals_admission_even_though_it_takes_no_reservation

1. create_session_refuses_a_package_that_declares_no_conversation — the reject direction for the fixture the repaired test accepts. A fixture that grants the property under test is worth nothing until the ungranted case is shown to be refused, and both fixtures come from one builder differing in one flag, so this pins that the flag is what the refusal turns on.

It is also the only unit test of #1892's gate. There is a server-side test of the status mapping, but nothing asserted the engine refuses. How the gate was discovered to be load-bearing is that it silently broke a test written for something else — which is a slower way to find out, and the reason this PR exists.

2. a_tensor_bound_request_signals_admission_even_though_it_takes_no_reservation — the accept direction for the !prompt_only branch, which had neither direction under test. This one needs its asymmetry stated, because the obvious falsifier is backwards:

on_admitted carries two contracts at that call site. To the engine it means "the scheduler admitted this" — false there: the branch takes no reservation and calls no complete(). To the server it means "you may begin streaming", and run_generation's success arm never touches the admission sender (only its error arm does), so this callback is the only thing that resolves that oneshot with Ok on a request that succeeds. Delete the call and every successful tensor-bound request becomes a 500 at the awaiting end (completions.rs) while the generation itself completes perfectly.

So asserting !admitted here — the natural reading of "this branch does not admit" — passes under exactly the change that breaks serving. The test asserts the contract a mutation can actually violate.

Limitation, stated rather than implied

The engine fixture's interpreted decoder cannot complete a generation, so what test 2 pins is that the branch signals admission, not that it does so on a run returning Ok. The signal is unconditional and precedes the work, so it is the same line either way — but a genuinely end-to-end version needs a package the interpreter can finish, and no such fixture exists in this crate yet. The server crate has no Engine fixture at all, so it cannot host that test today either.

Gate

On this branch, from clean:

cargo fmt --all --check                                                     -> 0
cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server
    --features onnx-genai-engine/native-backend,onnx-genai-server/native-backend
    -- -D warnings                                                          -> 0
cargo test --locked -p onnx-genai-engine --features native-backend --lib    -> 621 passed / 0 failed
cargo test --locked -p onnx-genai-engine   (default features)               -> 0 failed

Unrelated to the head_sink build break (#1961, #1958) that was red on main at the same time; that one is fixed and verified separately, and I deliberately kept the two apart.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.75%. Comparing base (2f2eb6d) to head (8a2036d).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
+ Coverage   80.64%   80.75%   +0.11%     
==========================================
  Files         408      423      +15     
  Lines      197607   207429    +9822     
  Branches   197607   207429    +9822     
==========================================
+ Hits       159359   167513    +8154     
- Misses      32818    34281    +1463     
- Partials     5430     5635     +205     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (?)
mlas	85.33% <ø> (?)
offline	80.90% <ø> (+0.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 40 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]

Reviewed. This is right, and it is better than the PR I had open for the same defect (#1954, now closed in favour of this one). The fixture repair, the reject-direction test, and the on_admitted accept-direction test are all correct, and the reasoning about why the obvious falsifier is backwards is the part I'd have got wrong.

Three things: one real finding measured from this PR's own CI, one place where a guard is weaker than its comment claims, and one confirmation I ran.

1. test_session_decoder_runtime is dead code under default features — and this PR's CI already says so

Its only caller is runtime.rs:3009, inside interpreted_engine_inner, which is #[cfg(feature = "native-backend")]. The function itself is only #[cfg(test)]. So a default-feature test build compiles it with no reference to it. From this PR's CLI ORT (Linux x86_64) log, in the Test ORT-backed workspace crates step (default features, and onnx-genai-engine is in that list):

2026-08-24T09:29:56Z warning: function `test_session_decoder_runtime` is never used
2026-08-24T09:29:56Z     = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default

One line:

#[cfg(all(test, feature = "native-backend"))]
pub(crate) fn test_session_decoder_runtime() -> anyhow::Result<WorkflowRuntime> {

The reason I'd fix it rather than shrug is not the warning. It is that the job stayed green with it. onnx-genai-engine has no -D warnings gate at default features anywhere in ci.yml — the only -D warnings clippy that names it is Check the native backend compiles, which turns the feature on. So the default-feature configuration of this crate emits warnings into a log nobody reads and cannot fail a lane. A genuine unused-code defect there has the same signature as this one.

That is the mirror image of #1944: native-backend code that no default lane compiles, and now default-feature code that no lane lints. Both halves of the crate have a hole; #1961 closed one. Worth an issue rather than scope creep here — I'll file it unless you'd rather.

I hit exactly this and only caught it because I ran both configurations, so this is a shared trap and not a criticism.

2. loop_carried_cell_names's _ => {} is a silent-miss arm

match step {
    WorkflowStep::Sequence { .. } => ...,
    WorkflowStep::Loop { .. }     => ...,
    WorkflowStep::Branch { .. }   => ...,
    _ => {}
}

A future WorkflowStep variant that nests steps falls into _ and its carries are missed. The three ensure!s catch total failure (carried empty, promoted == 0, predicate false) but not a partial one: if a new nesting variant holds some carried cells and the top-level loop still holds others, the fixture promotes a subset, all three guards pass, and it declares session state that is missing a cell — a package subtly unlike the one it claims. Anti-correlated in the usual direction: it fails only in the case a reader would most want it to catch.

Exhaustive matching makes the compiler point at this function when a variant is added:

WorkflowStep::Invoke { .. } | WorkflowStep::Emit { .. } | ... => {}

Not a blocker — today's WorkflowStep has no other nesting variant, so the set is currently complete. It is a future silent miss, and the cost of removing it is listing the leaf variants once.

Credit where due: ensure!(workflow_carries_session_state(&workflow)) — asserting the fixture satisfies the same predicate create_session reads, not a lookalike — is the single best line in this diff, and it is what my version was missing.

3. Independently confirmed the lane this repairs

Before this PR existed I replayed the CLI ORT steps locally against the same fixture repair (Linux x86_64, CARGO_INCREMENTAL=0, taskset -c 16-23, under hostlock.sh with a declared reason — not claiming an idle host):

lane step	result
cargo test --locked -p onnx-genai-engine --features native-backend -- --test-threads=1	exit 0, 619 passed lib + all integration suites
... --test onnx_genai_workflow_conformance	exit 0, 15 passed
cargo clippy --locked -p onnx-genai-cli --all-targets -- -D warnings	exit 0
cargo clippy ... --features onnx-genai-cli/native-cuda,onnx-genai-server/native-cuda --all-targets -- -D warnings	exit 0
cargo fmt --all -- --check	exit 0

And this PR's own CLI ORT (Linux x86_64) is now green on every step, including both clippy steps — which is the first time that lane has run them at all in this sequence, since they were skipped behind the earlier failure on #1944 and red on #1954.

On the on_admitted asymmetry

Nothing to add except that I want it on record, because I reached the wrong falsifier first and someone else will too. assert!(!admitted) is the natural encoding of "this branch takes no reservation", it is true of the engine, and it passes under the deletion that turns every successful tensor-bound request into a 500. A test can be a correct statement about the code and still be green precisely when the system is broken. The limitation paragraph — that the interpreted fixture cannot complete a generation, so this pins the signal and not the Ok-path — is exactly the right thing to state rather than imply.

Verdict: approve on the substance. Item 1 is a one-line change I'd make before merging; item 2 is optional and worth its two lines. Neither changes the correctness of what is here.