[justinchuby]

Closes #1891.

What was wrong

#1723 (faedea4d1) routed every package without a fused decode core through generate_with_pipeline_callbacks, and that drive ran the declared components without ever consulting the scheduler.

I verified all three of @justinchuby's claims against the source rather than taking them on trust; all three hold.

1. runtime.rs:977 — if !self.holds_decode_core() { return self.generate_interpreted(...) } sits above the #[cfg(feature = "native-backend")] block whose comment promises "both cold and session-reusing paths still admit through the scheduler before touching the native backend". The promise is still true of the code it describes. The code is not reached.
2. workflow_api.rs:173-176 — the re-entry branch back to the admitting entry point is guarded on self.holds_decode_core(), the very condition whose falsity routed us there.
3. workflow_api.rs:188-190 — on_admitted() fired unconditionally, with no admission decision in existence.

One refinement to the diagnosis. git log -S 'holds_decode_core' -- crates/onnx-genai-engine/src/engine/workflow_api.rs dates both defects to #1723, and the drive did not previously admit either. So the precise statement is stronger than "the guarantee moved": the interpreted path never had admission wired in at all. Candidate (1) in mechanism, but it is a wiring defect, not stale budget arithmetic.

Two consequences, and the second misreports rather than merely omits:

• An engine with no decode core had no admission path. The KV byte budget did not apply to those packages, so an over-budget request did its work and then failed downstream for an unrelated reason (decoder_kv.0000 — the symptom, not the defect).
• Anything counting admissions was counting requests nothing had admitted.

The fix

Admission goes in the single funnel both entry points reach, rather than being duplicated at each caller:

• after the prompt is encoded, so the scheduler sizes the request it is actually going to run;
• before any component is invoked, which is the ordering the guarantee is about;
• on_admitted() fires only after the scheduler accepts;
• scheduler.complete() runs on both the success and the error path — a request that admits and then fails must not leak its bytes or its running-batch slot.

admit_generate_request_with_scheduler becomes pub(crate). scheduler_seq is the parsed session_id or 0; the workflow session counter starts at 1, so 0 is a free sentinel for "no session".

The re-entry branch is deliberately left in place. It is dead only on the generate_interpreted path. It is live for callers entering the pipeline API directly, and on that route it returns to generate_with_callbacks, which admits — so every route still reaches exactly one admission, never zero and never two.

Not repaired by relaxing the assertion, per the constraint on the issue, and not by fixing decoder_kv.0000, which would have greened the lane with both holes intact.

Coverage, and why the existing test could not carry this alone

A rejection test passes on an engine that never admits anything — including the pre-fix engine. It is structurally blind to the positive half. The ~40-line Engine fixture is now a helper parameterised on the byte budget, shared by the existing rejection test and a new one that asserts the two facts distinguishing a real admission from no admission: the callback fires only after the scheduler accepted, and the reservation is handed back.

The fixture cannot complete a generation, so the new test's observable is scheduler state, not success — which is also the route proof that the assertion is on the path it names.

Both new assertions falsified by mutation:

mutation	test	result
delete self.scheduler.complete(scheduler_seq)	admitted_..._releases_its_reservation	FAILED — left: 1, right: 0
move on_admitted() above the admission call	native_generate_rejects_over_kv_byte_budget_before_backend_run	FAILED — "refused requests must not signal admission"

Validation

• cargo test --locked -p onnx-genai-engine --features native-backend — 766 passed / 0 failed across 80 binaries (lib alone 618/0/1 ignored). Baseline on main was 764 passed / 1 failed.
• cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings — clean.
• cargo clippy --locked --all-targets -p onnx-genai-engine -- -D warnings (default features, no native-backend) — clean. Checked deliberately: my whole local matrix would otherwise be one feature configuration, and a -D warnings lane built without the feature is exactly the blind spot that has cost two people a round this week.
• cargo fmt --all -- --check — clean.
• Merged latest main (6ccf7eec2) before pushing.

Limitations

The interpreted path now admits every request that reaches it, including non-token workflows (diffusion, TTS) whose prompt_tokens is 0 and which therefore reserve nothing. Empirically this caused zero regressions across the crate's 766 tests, but I am flagging that the design intent — whether those packages should pass through the scheduler at all — is inferred from the guarantee's own comment, not confirmed by anyone who owns #1723.

Auto-merge armed, no --admin; waiting on Fast (Linux x86_64) and Rust quality.

[justinchuby]

Closing as superseded by #1900, which merged at cd99e5ceb while this was in flight.

Their fix is better than mine on three counts, and I'd rather say which than wave at it:

• It threads the caller's own workflow session id through admission, so a continuing session accumulates the way the decode-core path already does. Mine collapsed every non-session request onto sentinel 0.
• interpreted_prompt_token_count handles TokenRows as rows.len() * max(row length) — equal-length rows bind into one batched [rows, columns] tensor and run as a single graph step. Mine used max(row length), which undercounts a batched request by a factor of rows.len(). That is a real defect in my version, not a stylistic difference: an over-budget batch would have been admitted.
• It propagates budget_cap onto the result.

On the duplication. I ran gh pr list --state open --search 1891 / admission / generate_with_pipeline_callbacks before committing — the discipline I said I'd adopt after duplicating twice today — and it returned nothing. #1900 was opened 22:33:56Z and merged 22:36:02Z, after that check. So the check worked and still lost, which tells me claiming the issue (which I did, at 21:42Z) is the part that has to be read, not just written. No complaint intended; noting it because "run the search" is evidently not sufficient on its own.

What survives from this branch, as the narrow test-only follow-up #1902:

scheduler.complete() on the interpreted path is not under test. Both tests #1900 ships assert a refusal, and a refusal is equally consistent with an engine that admits nothing and one that admits correctly and never releases. I verified this by mutation rather than by reading: deleting that one line from cd99e5ceb leaves the entire 618-test lib suite green.

One residual I am reporting rather than changing, because it's the #1900 author's scope call: on the !prompt_only (tensor-bound) path, on_admitted() still fires unconditionally with no admission decision behind it. That is exactly the second consequence @justinchuby flagged on #1891 and called the more serious of the two. It cannot simply be deleted, and the PR body's cited precedent is slightly off: onnx-genai-server/src/driver.rs:1609 does pass on_admitted on that path, and its closure sends Ok(()) on a oneshot that unblocks the HTTP response — the Ok(result) arm never takes the sender. So dropping the call would leave a multimodal request's receiver with a RecvError instead of a response. The honest options are "admit tensor-bound too" or "separate the accepted-signal from the admitted-signal", and both are design calls I shouldn't make unilaterally. Left on #1891.

[justinchuby]

Correction: the follow-up is #1904, not #1902 — I wrote the number before the PR existed, which is precisely the class of error I've been flagging to others all day (naming a state the tree isn't in). Apologies for the noise.

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1901      +/-   ##
==========================================
- Coverage   80.71%   80.33%   -0.38%     
==========================================
  Files         415      415              
  Lines      204440   204440              
  Branches   204440   204440              
==========================================
- Hits       165013   164245     -768     
- Misses      33858    34620     +762     
- Partials     5569     5575       +6     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (ø)
cli-ort-windows	72.10% <ø> (ø)
mlas	85.10% <ø> (-0.23%)	⬇️
offline	80.47% <ø> (-0.39%)	⬇️

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