Repository navigation
refactor(pipeline): one prefix-reuse policy for native and ORT decode - #1170
Conversation
The decode loop was already shared -- one `run_decode_loop`, one `PipelineDecodeLoopBackend`, both backends behind `&mut dyn PipelineDecoderComponent`. Prefix reuse was not. #1152 added a second implementation inline for the native path, in parallel with `reusable_prefix_len` used by ORT. Two copies of one policy, and they had already drifted apart in opposite directions: the native copy had no `use_kv` gate, and the ORT copy never clamped the retained context's claim against the KV's real length. Each was missing a guard the other had. Neither omission can surface as a crash -- a prefix-reuse bug produces fluent text conditioned on the wrong context. Extract the decision into `pipeline::prefix_reuse::plan_prefix_reuse`, a pure function, and the application into `apply_prefix_reuse` over a `KvPrefixStore` seam. Only the mechanism now differs by backend: a native decoder rewinds its session-resident cache, ORT slices pipeline-held past tensors. Both get both guards. Sharing the application also closes a latent hazard: a declined rewind now always resets. Reporting "reuse nothing" while leaving the previous turn's tokens in the cache would have prefilled the new prompt on top of them. The native path had exactly that shape, unreachable today only because `NativePipelineDecoder::rewind_kv` never returns `false`. The policy is now testable without a model or a device -- 11 unit tests where there were none. Both new guards were falsified: removing the clamp reddens `the_cache_length_bounds_the_reuse_when_the_context_claims_more` and `an_empty_cache_reuses_nothing_however_much_the_prompt_shares`; removing the reset-on-decline reddens `a_declined_rewind_empties_the_cache_rather_than_leaving_its_tail`. No behaviour change on either backend for any input both copies already agreed on. Same work per turn, one added trait dispatch per request.
Extracting the policy stops the two copies that exist. It does not stop the third. Two mechanisms now do. **Compile time.** `ReusedPrefix` is a newtype with a private field whose only constructors live in `prefix_reuse`. A backend cannot produce a reuse length without going through `apply_prefix_reuse`; forging one outside the module is `error[E0603]: tuple struct constructor `ReusedPrefix` is private`. The inline computation #1152 added would not compile today. **Test time.** `the_kv_rewind_primitives_are_only_driven_by_the_shared_policy` scans the crate source and fails if `.rewind_kv(`, `.truncate_past(` or `.current_kv_len(` is called outside the `KvPrefixStore` adapters. A second copy of the policy has to touch one of those primitives, so confining the primitives confines the policy. It runs under `cargo test`, so it cannot be missed by not looking at CI, and it names the offending file and line. The allowlist is deliberately three entries and the failure message says to route the new backend through `apply_prefix_reuse` rather than widen it. Widening the allowlist is how the duplication this module exists to prevent gets back in. Both mechanisms falsified: a probe call in `paged_decode.rs` reddens the guard (`pipeline\paged_decode.rs:311`), and a probe construction fails to compile.
Self-reviewReviewed my own diff against the one failure this change could plausibly introduce, rather than re-describing what it does. The risk: an ordering dependency. The ORT adapter's Traced it:
No path reaches the Behaviour deltas I accepted, deliberately:
Verified: Not verified, stated plainly: this box has no CUDA runtime, so the native adapter was never exercised against a real device-resident KV — only against the trait. And the performance claim is an argument, not a measurement: the work per turn is unchanged and the added cost is one trait dispatch per request, which is not resolvable against a prefill. I did not benchmark it and am not claiming I did. The mechanisms (second commit)Extracting the policy stops the two copies that exist; it does not stop the third. Two mechanisms now do, and both were falsified rather than merely written:
The allowlist is three entries and the failure message tells you to route the new backend through Merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1170 +/- ##
=======================================
Coverage 80.53% 80.54%
=======================================
Files 357 357
Lines 154203 154282 +79
Branches 154203 154282 +79
=======================================
+ Hits 124183 124262 +79
+ Misses 25476 25474 -2
- Partials 4544 4546 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
✅ Benchmarks — No RegressionComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
…ionStore (#1255) ## What & why The engine kept **two parallel session implementations** selected by `if self.decode_backend == EngineDecodeBackend::Native`, one over `self.native_sessions` and one over `self.sessions`. The *policy* around a session — the "session {id} not found" lookup, the rewind bound-checks and their exact error strings, the checkpoint arithmetic, and `truncate`/`clear` — was **backend-independent but duplicated verbatim** across the two arms (the `checked_sub` bound check and its message in `rewind_session_by`, the `position > current` check in `rewind_session_to`, etc.). This is the same class of drift risk that #1170 closed for the KV *length* layer with `KvPrefixStore`. This PR extracts that policy into one place and routes both backends through it, following the `KvPrefixStore` precedent rather than inventing a new pattern. ## The seam New module `crates/onnx-genai-engine/src/engine/session_state.rs`: - **`SessionStore` trait** — mirrors `KvPrefixStore`. Backends implement `logical_len` (via `SessionLen`), `validate_rewind`, `rewind`, `reset`, `close`, with **identical method names/signatures** on both sides, exactly as the `NativeKvStore`/`OrtKvStore` adapters already do. - **Free policy functions** `require_session`, `checkpoint`, `token_count`, `rewind_by`, `rewind_to`, `reset`, `close` own everything that is backend-independent: the lookup + not-found text, both rewind bound-check strings, `SessionPosition::new(len)` checkpoint arithmetic, and the dispatch into the single backend truncation method. - **Adapters in `runtime.rs`**: `OrtSessions`/`OrtSessionsRef` (always compiled) and `NativeSessions`/`NativeSessionsRef` (`#[cfg(feature = "native-backend")]`) implement `SessionStore`. The six public methods (`checkpoint_session`, `rewind_session_by`, `rewind_session_to`, `reset_session`, `close_session`, `session_token_count`) now select an adapter and call `session_state::*`. The `require_ort_backend(...)` guard stays in the ORT arm. Error text is now authored **once**. Rendered strings are byte-identical to the originals (only internal variable names differ, e.g. `{id}` vs `{session_id}`), so behaviour is unchanged. No test asserted these strings before; the ones that assert *backend* rewind errors (`failed_rewind_of_*`) are untouched. ## Asymmetries I deliberately kept (and why) Forcing these into false symmetry would be worse than the small duplication that remains, so — mirroring how `KvPrefixStore`'s `reset` is legitimately different per backend — they stay backend-specific behind the trait: - **`validate_rewind`**: ORT validates the target *and* draft/speculative state and paged-KV page ownership (`validate_target_state_rewind_to_len`) so a rejected rewind leaves the session untouched; the three `failed_rewind_of_*` tests pin this. Native's `validate_rewind` is a deliberate `Ok(())` — it always admits and clamps against its in-process decoder length. These are genuinely different objects: ORT owns paged KV + a draft runner; native owns one persistent in-process decoder. - **`rewind`/`reset`/`close` mechanism**: ORT does validate → `scheduler.complete` → remove → `rewind_target_state_to_len` → reinsert; native truncates its token vec, rewinds/resets the single persistent decoder (gated on `native_active_session`) and touches LRU `last_access`. - **`create_session`** is left backend-specific and is **not** routed through the seam: it is object *construction*, not policy, and the two session value types (`EngineSession` vs `NativeSessionState`) are genuinely different. Unifying it would be a forced abstraction with no shared policy to host. - **Token `truncate` for ORT is intentionally *not* lifted** into the shared policy. It lives inside `rewind_target_state_to_len` (`kv_bridge.rs`), which is **shared with the speculative-decode hot path** (`speculative/mod.rs`). Lifting it would fracture that path for no gain. The shared policy therefore owns the logical-length **bound check** (truly common) and delegates the truncation. ## DRY guard (mandatory, per #1170) Two-part, mirroring `ReusedPrefix` + `KV_REWIND_CALLERS`: 1. **Compile-time** `CheckedPosition` newtype with a private field. It is only ever constructed by `session_state::rewind_to` after the bound check, and it is the required argument to `SessionStore::rewind`/`validate_rewind`. A future backend therefore *cannot* rewind KV without going through the one bound check — there is no other way to obtain the token. 2. **Test-time tripwire** `the_rewind_bound_check_lives_only_in_the_shared_policy` scans the crate sources for the `"cannot rewind session"` fragment and fails if it appears anywhere except `engine/session_state.rs`. A third backend that open-codes a copy turns the suite red. (`fork_session` uses the distinct `"cannot fork session"`, so there is no collision.) ## Performance These are **per-turn / per-request** session ops, not the per-token decode hot path (`generate_incremental_with_callback`). The trait dispatch happens once per rewind/checkpoint/reset, off the hot path, so the dynamic call cannot show up in decode throughput. No hot-path code changed; no per-token dispatch was introduced. (Restructured, not measured — because there is nothing on the hot path to measure.) ## Falsification (the important part) I broke the shared policy deliberately — inverted the one bound check in `session_state::rewind_to` (`requested > current` → `requested < current`) — and confirmed **both** backends go red from that single line: - **ORT** (default backend): `failed_rewind_of_windowed_evicted_position…`, `failed_rewind_of_ort_owned_kv…`, `failed_rewind_of_runner_backed_state…` — all 3 FAILED (they now hit the bound check before their backend validation). - **Native** (`--features native-backend`): `native_session_rewind_by_truncates_logical_length` FAILED with `cannot rewind session 1 to token 3; current length is 5`. Reverting the one line restored both to green. A break in the shared path turns *both* backends red, which is the evidence that the extraction actually unified them rather than leaving two look-alikes. I added `native_session_rewind_by_truncates_logical_length` because there was previously **no** integration test exercising a *valid* explicit rewind on the native backend (the existing native "rewind" test drives prefix-reuse, a different path) — this closes that gap and gives the native half of the falsification. ## Verification - `cargo test -p onnx-genai-engine --lib` → **414 passed, 0 failed, 1 ignored**. - `cargo test -p onnx-genai-engine --features native-backend` → all targets pass **except** the pre-existing `pipeline_load_rejects_kv_page_pool_before_fixed_state_when_host_budget_cannot_fit_either` (a host-RAM KV-budget test — confirmed failing identically on a stashed baseline; it does not touch session code). - `cargo clippy -p onnx-genai-engine --all-targets -- -D warnings` and the same with `--features native-backend` → clean. - **CUDA (RTX 4060, driver 591.55):** `cargo test -p onnx-genai-engine --features native-backend,cuda --test native_engine -- --test-threads=1` ran on the GPU (not ignored): **16 passed, 1 failed, 0 ignored**. Passing includes the new `native_session_rewind_by_truncates_logical_length`, `native_sub4_cuda_fallback_generates_coherent_decode`, and `qwen15b_native_decode_locks_accurate_near_tie_token` (real CUDA decode). The 1 failure, `engine_native_cuda_matches_cpu_tokens` ("full CUDA mask reservation size overflow" during native decoder load), is a pre-existing CUDA mask-workspace sizing issue on this 8 GB card — confirmed failing identically on a stashed baseline, unrelated to session state. ## Behaviour note `close_session`/`reset_session` now run the existence check (via `require_session`) before `scheduler.complete`. Previously ORT `close` called `scheduler.complete` before its remove-based existence check. This is not observable through any error (`scheduler.complete` on a missing id is benign) and `reset` already checked existence first. Do not merge — please review. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
…1271) ## Summary `onnx-genai-ort::cuda_rt::CUDART_CANDIDATES` and `onnx-runtime-ep-cuda::dynamic_library::candidates` were two independent lists of CUDA `cudart` shared-library names. They drifted (the EP listed CUDA 13, `cuda_rt` did not), and CUDA 13 hosts failed to load `cudart` with an error naming a Linux `.so` on Windows (#1178). #1178 fixed the symptom and added a test pinning the two lists together — a weaker mechanism than one table. This PR makes the second copy **unrepresentable** (same shape as the prefix-reuse dedup in #1170): - **One canonical table** now lives in `onnx-genai-cuda-version-guard` — the crate both loaders already depend on: - `CUDART_CANDIDATES_LINUX` / `CUDART_CANDIDATES_MACOS` / `CUDART_CANDIDATES_WINDOWS` - `HostOs` selector + `cudart_candidates_for(HostOs)` (`const fn`) + `cudart_candidates()` (current build target) - Platform-conditional split preserved; the table is the **exact union** of both prior lists, including CUDA 13 (`cudart64_13.dll`, `libcudart.so.13`) and legacy `cudart64_120.dll`, so no host regresses. - **Both loaders consume it**, no second copy: - `onnx-genai-ort::cuda_rt` — deleted the private `CUDART_CANDIDATES` const; `load()` iterates `cudart_candidates()`. - `onnx-runtime-ep-cuda::dynamic_library` — the `Runtime` match arm delegates to `cudart_candidates_for(...)`. - **Deleted the #1178 agreement test** `every_cuda_major_the_ep_loads_is_also_resolvable_here` (the duplication it guarded is gone) and repointed the EP's `generates_linux/windows_cuda_names` tests at the canonical constants. - Added `onnx-genai-cuda-version-guard` as a normal (not just build) dependency of both crates so the runtime table is importable. ## Validation (run locally, default features) - `cargo build -p onnx-genai-cuda-version-guard -p onnx-genai-ort -p onnx-runtime-ep-cuda` — **pass** - `cargo test -p onnx-genai-cuda-version-guard -p onnx-runtime-ep-cuda` — version-guard 4/4 pass; EP `cudart` name tests pass. Two unrelated failures observed and confirmed pre-existing/flaky on clean `origin/main`: `onnx-genai-ort` integration test `real_foundry_package_fails_loudly_when_preprocessing_is_not_executable` (fails identically on baseline; depends on a local `~/.foundry` model) and EP GPU kernel `fp16_gemv_matches_dequant_reference_block128` (flaky — passes on retry and on baseline). - `cargo clippy -p onnx-genai-cuda-version-guard -p onnx-genai-ort -- -D warnings` — **clean**. EP clippy has one pre-existing `unnecessary_unwrap` error in `kernels/matmul_nbits.rs:8751` (untouched by this PR; present on baseline). Closes #1180⚠️ Do not merge — the coordinator admin-merges. ## Reference-oracle cross-check (validation only — no change needed) Sanity-checked our canonical `cudart` names against NVIDIA's `cuda-pathfinder` descriptor catalog (`descriptor_catalog.py`, `cudart` entry), used purely as a reference oracle (no Python/pip dependency added — we stay pure-Rust): - NVIDIA `cudart` Linux sonames: `libcudart.so.12`, `libcudart.so.13` - NVIDIA `cudart` Windows DLLs: `cudart64_12.dll`, `cudart64_13.dll` Our canonical table is a **superset** of both: - Linux: `libcudart.so.13`, `libcudart.so.12`, `libcudart.so` (adds the unversioned dev-symlink fallback) - Windows: `cudart64_13.dll`, `cudart64_12.dll`, `cudart64_120.dll`, `cudart.dll` (adds legacy `cudart64_120.dll` + bare `cudart` fallback) - macOS: `libcudart.dylib` (NVIDIA no longer ships macOS CUDA; harmless legacy retention) No CUDA-13 (or other) name is missing, so no additions were made — coverage already matches the oracle. This confirms the union we preserved is complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The decode loop was already shared. Prefix reuse was not.
What was wrong
One
run_decode_loop, onePipelineDecodeLoopBackend, both backends behind&mut dyn PipelineDecoderComponent— the loop is properly unified. But #1152 added a second prefix-reuse implementation inline on the native path, alongsidereusable_prefix_lenused by ORT.Two copies of one policy, already drifted in opposite directions:
use_kvgatesharedagainst the KV's real lengthEach was missing a guard the other had. Neither omission can surface as a crash: a prefix-reuse bug produces fluent text conditioned on the wrong context, so no test fails and no benchmark moves.
What this does
pipeline::prefix_reuse::plan_prefix_reuse— the decision, as a pure function.apply_prefix_reuseover aKvPrefixStoreseam — the application.rewind_kv), ORT slices pipeline-held past tensors (truncate_past). Both adapters live inflat_autoregressive.rsand are ~20 lines each.Both backends now get both guards.
A latent hazard this closes
Sharing the application means a declined rewind now always resets. Reporting "reuse nothing" while leaving the previous turn's tokens in the cache would prefill the new prompt on top of them. The native path had exactly that shape — unreachable today only because
NativePipelineDecoder::rewind_kvnever returnsfalse, which is a property of the current sole implementor, not of the contract.Validation
cargo test -p onnx-genai-engine --features native-backend --lib— 507 passed, 0 failed, 1 ignored. The pre-existing ORT rewind-semantics tests (failed_rewind_of_ort_owned_kv_leaves_session_unchanged,failed_rewind_of_runner_backed_state_leaves_session_unchanged,failed_rewind_of_windowed_evicted_position_leaves_session_unchanged) still pass — that is the no-regression evidence for the ORT path.cargo clippy -p onnx-genai-engine --features native-backend --all-targets -- -D warnings— clean.Falsified, not just written. Both new guards were deliberately broken and confirmed to redden:
shared.min(current)→the_cache_length_bounds_the_reuse_when_the_context_claims_moreandan_empty_cache_reuses_nothing_however_much_the_prompt_sharesFAILa_declined_rewind_empties_the_cache_rather_than_leaving_its_tailFAILSPerformance
No behaviour change on either backend for any input the two copies already agreed on. Same work per turn; one added trait dispatch per request (not per token), which is unmeasurable against a prefill.