Repository navigation
feat(kv): one KV capacity mechanism for native CUDA and ORT - #367
Conversation
Justin hit "CUDA KV capacity exceeded: requested context length 4097, configured max_len 4096" on the native backend and asked why it was so conservative when he had VRAM to spare, and whether we did not already have dynamic KV allocation. We did -- on one path only. The ORT shared-buffer GQA path grew in power-of-two buckets via kv_capacity_bucket, while native CUDA pre-allocated once at a hardcoded DEFAULT_CUDA_KV_MAX_LEN of 4096 and never grew. Two mechanisms for the same resource, which is the shape that has produced several bugs here recently; a bigger constant would have left the divergence in place for someone to hit from another angle. The growth policy now lives once, in onnx-genai-kv: kv_capacity_bucket plus a KvCapacityGrowthBackend seam and a shared ensure_kv_capacity driver. Both ORT and native CUDA consume it. The backend seam carries only primitives that genuinely differ -- allocate buffers, copy the prefix, build the mask, invalidate capture, commit -- so adding a backend means implementing primitives rather than re-deriving policy. Growth and CUDA graph capture coexist, as they already did on the ORT side: grow to the next bucket, device-to-device copy the valid prefix, rebuild the mask, release the capture, re-capture at the new size. The comment claiming capture required a frozen physical capacity described the fixed-capacity design rather than a real constraint, and is corrected. Capacity derives from model metadata and is clamped by what the device can actually afford, rather than by a constant. The clamp initially checked only the final KV size, which missed the transient peak during growth where the old and new buffers are both live: on an 8 GB RTX 4060 with DeepSeek at 28,680 bytes/token, the 65536 to 131072 transition needs 5.64 GB against a 5.93 GB budget. It now clamps to the worst transition, giving hard_max_len 110080 for that model on that card instead of the metadata's 131072. Verified on the GPU rather than reasoned about: forced 4 to 8 to 16 growth with real reallocation and re-capture, output stayed coherent -- a corrupted prefix copy shows up as wrong tokens rather than an error, so coherence is the assertion that matters. Reaching the ceiling now produces the capacity error before allocation rather than a CUDA OOM. The error text names where the limit came from, the metadata value, device free and total, KV bytes per token, and the available overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
Justin hit "CUDA KV capacity exceeded: requested context length 4097, configured max_len 4096" on the native backend and asked why it was so conservative, and whether we did not already have dynamic KV allocation. We did -- on one path only. The ORT shared-buffer GQA path grew in power-of-two buckets via kv_capacity_bucket, while native CUDA pre-allocated once at a hardcoded 4096 and never grew. Two mechanisms for the same resource, which is the shape behind several bugs here recently. A bigger constant would have left the divergence for someone to hit from another angle. The growth policy now lives once, in onnx-genai-kv: kv_capacity_bucket plus a KvCapacityGrowthBackend seam and a shared ensure_kv_capacity driver, consumed by both backends. The seam carries only primitives that genuinely differ -- allocate, copy the prefix, build the mask, invalidate capture, commit -- so a new backend implements primitives rather than re-deriving policy. Growth and CUDA graph capture coexist, as they already did on the ORT side: grow to the next bucket, device-to-device copy the prefix, rebuild the mask, release the capture, re-capture at the new size. The comment claiming capture required a frozen physical capacity described the fixed-capacity design rather than a real constraint, and is corrected. There is no derived VRAM ceiling. An earlier revision predicted a safe maximum from free memory times a fraction; that is guesswork dressed as policy, since the snapshot is stale as soon as another process allocates and the fraction is the kind of constant RULES.md rule 2 exists to prevent. The only remaining ceilings are facts: the model's declared max_sequence_length, and an explicit user cap. Unset means grow until the model's own limit or until the device refuses. That puts the weight on failing well. Growth is transactional: a failure at any fallible step leaves capacity, logical length, buffers and capture state exactly as they were, and the session stays usable. The commit phase is infallible assignment after all fallible work, which is why it has no failure case to cover. Failures are translated rather than passed through, naming the old and new bucket, the transient peak, bytes per token, device free and total, and the recovery levers. The transient peak is worth stating because it is unintuitive: growth holds the old and new buffers at once, so the requirement is roughly (old + new) * bytes_per_token. A grow can fail with what looks like ample free memory. Verified on an RTX 4060: forced 4 to 8 to 16 growth with real reallocation and re-capture, output stayed coherent -- a corrupted prefix copy produces wrong tokens rather than an error, so coherence is the assertion that matters. A real allocator OOM cannot be forced on a WDDM device, which virtualises and pages GPU memory; that is documented so it is not rediscovered. Coverage for the failure path is instead model-free injected failure at the shared seam -- allocation, prefix copy, mask allocation and capture invalidation each fail in turn, and each asserts no commit and an unchanged session, then proves a retry succeeds. Those run in CI on every PR rather than only on machines with a GPU. Also makes the Qwen CUDA smoke fixture env-overridable via ONNX_GENAI_QWEN_CUDA_SMOKE_MODEL; it previously skipped on a hardcoded Linux path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
|
| Status | Scenario | Base | PR | Change |
|---|---|---|---|---|
gather/large_f16_threads=1-internal/131072 |
9.23 µs | 10.95 µs | +18.7% | |
gather/large_f32_threads=1-internal/131072 |
22.54 µs | 26.67 µs | +18.3% | |
| ✅ | gather/large_bf16_threads=1-internal/131072 |
9.97 µs | 10.95 µs | +9.8% |
| ✅ | matmul/large_generic_bf16_threads=8/32x1024x1024 |
1.26 ms | 1.33 ms | +5.3% |
| ✅ | matmul/small_generic_f32_threads=8/1x256x256 |
31.99 µs | 33.24 µs | +3.9% |
| ✅ | gather/medium_f32_threads=1-internal/32768 |
3.39 µs | 3.52 µs | +3.8% |
| ✅ | matmul/medium_generic_bf16_threads=8/32x512x512 |
360.12 µs | 372.57 µs | +3.5% |
| ✅ | add/medium_bf16_threads=1-internal/262144 |
2.45 ms | 2.51 ms | +2.1% |
| ✅ | matmul/medium_generic_f16_threads=8/32x512x512 |
27.63 µs | 28.21 µs | +2.1% |
| ✅ | add/medium_f32_threads=1-internal/262144 |
2.32 ms | 2.37 ms | +1.9% |
| ✅ | matmul/small_generic_f16_threads=8/1x256x256 |
28.06 µs | 28.56 µs | +1.8% |
| ✅ | matmul/large_generic_bf16_threads=1/32x1024x1024 |
1.81 ms | 1.84 ms | +1.7% |
| ✅ | matmul/small_generic_bf16_threads=1/1x256x256 |
28.71 µs | 29.16 µs | +1.6% |
| ✅ | matmul/large_generic_f32_threads=1/32x1024x1024 |
8.67 ms | 8.80 ms | +1.5% |
| ✅ | matmul/large_generic_f32_threads=8/32x1024x1024 |
3.64 ms | 3.68 ms | +1.3% |
| ✅ | add/medium_f16_threads=1-internal/262144 |
2.40 ms | 2.43 ms | +1.2% |
| ✅ | matmul/medium_generic_bf16_threads=1/32x512x512 |
486.15 µs | 491.25 µs | +1.0% |
| ✅ | matmul/small_generic_f32_threads=1/1x256x256 |
34.03 µs | 34.34 µs | +0.9% |
| ✅ | add/large_f32_threads=1-internal/4194304 |
37.37 ms | 37.62 ms | +0.7% |
| ✅ | gather/medium_f16_threads=1-internal/32768 |
2.25 µs | 2.26 µs | +0.6% |
| ✅ | add/large_f16_threads=1-internal/4194304 |
38.30 ms | 38.48 ms | +0.5% |
| ✅ | matmul/small_generic_bf16_threads=8/1x256x256 |
29.06 µs | 29.16 µs | +0.4% |
| ✅ | matmul/small_generic_f16_threads=1/1x256x256 |
27.94 µs | 27.96 µs | +0.1% |
| ✅ | reduce_mean/large_f32_threads=1-internal/262144 |
908.00 µs | 907.32 µs | -0.1% |
| ✅ | matmul/medium_generic_f32_threads=1/32x512x512 |
2.15 ms | 2.14 ms | -0.3% |
| ✅ | gather/small_f16_threads=1-internal/4096 |
436.9 ns | 435.0 ns | -0.4% |
| ✅ | matmul/medium_generic_f16_threads=1/32x512x512 |
27.67 µs | 27.55 µs | -0.5% |
| ✅ | reduce_mean/small_f32_threads=1-internal/4096 |
13.88 µs | 13.79 µs | -0.6% |
| ✅ | matmul/large_generic_f16_threads=8/32x1024x1024 |
78.57 µs | 78.02 µs | -0.7% |
| ✅ | add/large_bf16_threads=1-internal/4194304 |
39.38 ms | 39.00 ms | -1.0% |
| ✅ | gather/small_f32_threads=1-internal/4096 |
625.8 ns | 619.6 ns | -1.0% |
| ✅ | gather/small_bf16_threads=1-internal/4096 |
439.9 ns | 433.9 ns | -1.4% |
| ✅ | matmul/large_generic_f16_threads=1/32x1024x1024 |
74.00 µs | 72.99 µs | -1.4% |
| ✅ | grammar_masking/llguidance_compute_mask/32 |
71.75 µs | 70.67 µs | -1.5% |
| ✅ | tokenization/decode_tokens_per_second |
5.97 ms | 5.86 ms | -1.9% |
| ✅ | reduce_mean/medium_f32_threads=1-internal/65536 |
229.87 µs | 225.32 µs | -2.0% |
| ✅ | matmul/medium_generic_f32_threads=8/32x512x512 |
899.64 µs | 878.66 µs | -2.3% |
| ✅ | gather/medium_bf16_threads=1-internal/32768 |
2.31 µs | 2.25 µs | -2.6% |
| ✅ | tokenization/encode_tokens_per_second |
363.16 µs | 352.49 µs | -2.9% |
| ✅ | sampling_latency/min_p_per_token |
328.23 µs | 313.72 µs | -4.4% |
| ✅ | sampling_latency/top_k_per_token |
462.72 µs | 441.29 µs | -4.6% |
| ✅ | kv_cache/alloc_dealloc_pages |
36.95 µs | 35.12 µs | -4.9% |
| ✅ | sampling_latency/greedy_per_token |
3.16 µs | 2.99 µs | -5.2% |
| ✅ | sampling_latency/top_p_per_token |
973.07 µs | 921.61 µs | -5.3% |
| ✅ | logit_processing/seven_processor_chain_per_step |
1.16 ms | 1.08 ms | -6.3% |
| ✅ | add/small_f16_threads=1-internal/1024 |
13.32 µs | 12.23 µs | -8.2% |
| ✅ | add/small_f32_threads=1-internal/1024 |
204.9 ns | 186.8 ns | -8.9% |
| ✅ | add/small_bf16_threads=1-internal/1024 |
13.58 µs | 11.92 µs | -12.2% |
Visual flags:
Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.4.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.82 3.48 3.98 }
What this cannot catch
- Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
- Sub-threshold regressions that compound over multiple PRs
- Performance changes that only manifest under GPU execution
- Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)
There was a problem hiding this comment.
Pull request overview
This PR unifies KV-cache capacity growth behavior between ORT shared-buffer GQA and the native CUDA path by centralizing the bucketing/growth policy and implementing backend-specific primitives on both sides, including transactional failure handling and capture invalidation/recapture behavior.
Changes:
- Introduces a shared KV growth policy + driver (
kv_capacity_bucket,ensure_kv_capacity) inonnx-genai-kv, consumed by both ORT and native CUDA. - Refactors ORT shared-buffer KV growth to use the shared driver and adds richer, actionable failure diagnostics (including device memory info when available).
- Adds native CUDA KV bucket growth (realloc + prefix D2D copy + mask rebuild + capture boundary) and expands test coverage with injected failure seams; also makes CUDA smoke fixture path env-overridable.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/native-ort-kv-capacity.md | New design note documenting unified KV growth policy/capture boundary behavior. |
| crates/onnx-runtime-session/src/executor/build.rs | Adjusts expose_logical_input_shape detection logic when allocating device bindings. |
| crates/onnx-genai-ort/src/lib.rs | Exposes cuda_rt module publicly under the cuda feature. |
| crates/onnx-genai-ort/src/decode/kv_growth.rs | Switches ORT bucketing helper to re-export the shared onnx-genai-kv policy. |
| crates/onnx-genai-ort/src/decode/dynamic.rs | Moves ORT shared-buffer growth orchestration to onnx_genai_kv::ensure_kv_capacity via a backend trait impl. |
| crates/onnx-genai-ort/src/cuda_rt.rs | Adds cudaMemGetInfo support and makes CUDA runtime helpers public for cross-crate use. |
| crates/onnx-genai-ort/Cargo.toml | Adds workspace dependency on onnx-genai-kv. |
| crates/onnx-genai-kv/src/lib.rs | Adds shared KV bucketing policy, backend trait, shared growth driver, and unit tests (including transactional/failure-order assertions). |
| crates/onnx-genai-engine/src/native_decode/tests.rs | Adds/updates tests for native CUDA KV capacity resolution and makes CUDA smoke model path env-overridable. |
| crates/onnx-genai-engine/src/native_decode/mod.rs | Extends CUDA options with metadata_max_len. |
| crates/onnx-genai-engine/src/native_decode/load.rs | Threads metadata-derived max sequence length into native CUDA options and capacity resolution. |
| crates/onnx-genai-engine/src/native_decode/cuda.rs | Implements native CUDA KV bucket growth via shared driver; adds capacity diagnostics and D2D prefix copy path. |
| crates/onnx-genai-engine/src/engine/load.rs | Plumbs model metadata max_sequence_length into native decode session creation and updates KV-cap comment. |
| Cargo.lock | Locks in the new workspace dependency wiring. |
| .squad/agents/leon/history.md | Internal agent log updates describing the work sequence and validations. |
| /// Load with an explicit CUDA KV capacity. `None` uses | ||
| /// `ONNX_GENAI_CUDA_KV_MAX_LEN`, then the 4096-token default. | ||
| /// `ONNX_GENAI_CUDA_KV_MAX_LEN`, then derives a default from model metadata | ||
| /// clamped by queried CUDA free memory. |
There was a problem hiding this comment.
Correct, and fixed in 3531331c.
The comment described the removed free-memory clamp. It now states the actual resolution order: ONNX_GENAI_CUDA_KV_MAX_LEN -> model.max_sequence_length from inference metadata -> unbounded growth until the device refuses. No ceiling is derived.
Worth flagging why this was more than a typo: a stale comment describing a removed mechanism is how the mechanism gets reintroduced. This PR already corrected one instance of exactly that -- a comment claiming CUDA graph capture required a frozen physical capacity, which is what justified the fixed 4096 buffer in the first place. So I swept the whole branch for the same class rather than fixing the one line.
The sweep found nothing else stale. The remaining free-memory references are diagnostics in failure messages -- reporting the device snapshot at the moment of failure -- not ceiling derivations, and the 4096 values left in tests are fixtures for model.max_sequence_length, exercised by uses_metadata_as_default_fact and without_metadata_is_unbounded_until_growth_fails.
…kv_max_len
The old comment said None "derives a default from model metadata clamped
by queried CUDA free memory". That mechanism was removed by this PR: free
memory no longer supplies a ceiling. The new resolution order is:
explicit override → ONNX_GENAI_CUDA_KV_MAX_LEN
→ model.max_sequence_length
→ unbounded growth until allocation fails
Update the doc comment to match.
Sweep of all changed files found nothing else stale: no free-memory-derived
ceiling, 85%-fraction, hardcoded-4096 default, or pre-allocated-KV-buffer
language in any changed source or doc file. engine/load.rs mentions
"pre-allocating" in the past tense as context for why callers may still
want an env cap; that phrasing is accurate. docs/native-ort-kv-capacity.md
already correctly describes the current design.
Failure messages reviewed against RULES.md §1:
- cuda_kv_capacity_exceeded_message: names requested length, configured
max_len with its source, and user actions. Device free/total is
diagnostic context, not the origin of the cap. ✓
- growth_failed_error: names the growth range, transient peak, device
memory at failure, and retry levers. Does not reference a removed
ceiling. ✓
Gates: cargo fmt --check ✓ · clippy -D warnings ✓ · cargo test -p onnx-genai-kv (101+1 passed) ✓
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Conflicts:
crates/onnx-genai-engine/src/engine/load.rs
crates/onnx-genai-engine/src/native_decode/load.rs
## engine/load.rs
The call to load_with_weight_offload_host_cache had two sides:
- ours: passes metadata.model.max_sequence_length as the metadata_max_len arg
for the CUDA KV capacity resolution chain (override -> metadata -> unbounded)
- main: passes crate::decode::key_sequence_lengths_policy(&metadata) as a new
key_sequence_lengths_policy arg for GroupQueryAttention contract validation
Both are needed. Resolution: pass both arguments.
## native_decode/load.rs
Region 1 — load_with_weight_offload_host_cache parameter:
- ours: metadata_max_len: Option<usize>
- main: key_sequence_lengths_policy: crate::decode::KeySequenceLengthsPolicy
Both parameters serve orthogonal purposes. Resolution: keep both.
Region 2 — function body, new function, doc comment:
- main added �alidate_key_sequence_lengths_contract (new GQA contract check)
and changed the body to call it before delegating.
- main's doc comment on load_with_cuda_kv_max_len read "then the 4096-token
default" — stale, describes a removed mechanism.
- ours has the corrected doc comment (override -> model metadata -> unbounded).
Resolution: integrate main's �alidate_key_sequence_lengths_contract and its body
call; keep our corrected doc comment on load_with_cuda_kv_max_len.
## Auto-merged ORT files — semantic check
onnx-genai-ort/src/decode/dynamic.rs: main added
ew_with_io, explicit
oken_input/�ttention_mask_input/position_ids_input/logits_output fields,
and exact-name I/O binding (replacing name-heuristic lowercase scans). None of
this overlaps with our KV capacity changes; auto-merge is semantically correct.
onnx-genai-ort/src/lib.rs: cuda_rt narrowed from pub to pub(crate);
pub mod io_roles added. No KV impact.
## Flagged for Justin: engine/load.rs ORT shared-buffer comment
main's diff wanted the comment at engine/load.rs:~388 (the ORT shared-buffer
ONNX_GENAI_KV_MAX_LEN rationale) to say "pre-allocates a KV buffer of exactly
that many tokens up front". Our side (and docs/native-ort-kv-capacity.md) says
"grows by buckets instead of pre-allocating that full length". The auto-merge
landed on our side. If the ORT shared-buffer path actually still pre-allocates
rather than growing by buckets, our comment and the doc are wrong for that path.
This is a design question that needs a deliberate decision, not a merge choice.
Justin should verify which description is accurate before #367 lands.
## Staleness sweep on main's incoming code
No free-memory-derived ceiling, 85%-fraction, "4096-token default", or
pre-allocated-KV-buffer language introduced by main's new code in the changed
files. The only "free-memory ceiling" match is our own corrected doc comment
explicitly stating that no such ceiling is derived.
## Gates
Commands run verbatim:
cargo fmt --all -- --check exit 0
cargo clippy --locked --all-targets -p onnx-genai-kv \
-p onnx-genai-metadata -p onnx-genai-runtime-config \
-p onnx-genai-scheduler -p onnx-runtime-ir -p onnx-runtime-ep-api \
-p onnx-runtime-session -p onnx-runtime-tracer -p onnx-std \
-- -D warnings exit 0
cargo clippy --locked --all-targets \
-p onnx-genai-engine --features native-backend -- -D warnings exit 0
cargo test -p onnx-genai-kv (101 unit + 1 integration, 0 failed) exit 0
cargo test -p onnx-genai-engine --lib (267 passed, 1 ignored, 0 failed) exit 0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
- Coverage 81.28% 80.54% -0.75%
==========================================
Files 309 310 +1
Lines 120384 120650 +266
Branches 120384 120650 +266
==========================================
- Hits 97860 97180 -680
- Misses 18554 19506 +952
+ Partials 3970 3964 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…cket
The comment in engine/load.rs and the prose in docs/native-ort-kv-capacity.md
both said the shared-buffer path "grows by buckets instead of pre-allocating
that full length", which is true but leaves a reader unsure whether anything
is allocated at session start.
Verified against source before writing:
- Initial allocation: kv_capacity_bucket(0, max_length) in dynamic.rs:397
which evaluates to the minimum bucket (256 by default), not max_length.
- Default floor: const MIN_BUCKET_DEFAULT: usize = 256 in onnx-genai-kv/src/lib.rs:64.
- Env var: ONNX_GENAI_KV_MIN_BUCKET in lib.rs:68; the existing kv_capacity_bucket
docstring already says it can "raise or lower" the floor, so "overridden by"
is used here rather than "raised by".
The update says both things explicitly:
"pre-allocates at an initial power-of-two bucket (256 tokens by default,
overridden by ONNX_GENAI_KV_MIN_BUCKET) and grows on demand up to the
model's declared max_length; it does not pre-allocate the full context."
Gates (verbatim from .github/workflows/ci.yml):
cargo fmt --all -- --check exit 0
cargo clippy --locked --all-targets -p onnx-genai-kv \
-p onnx-genai-metadata -p onnx-genai-runtime-config \
-p onnx-genai-scheduler -p onnx-runtime-ir -p onnx-runtime-ep-api \
-p onnx-runtime-session -p onnx-runtime-tracer -p onnx-std \
-- -D warnings exit 0
cargo test -p onnx-genai-kv (101 unit + 1 integration, 0 failed) exit 0
cargo test -p onnx-genai-engine --lib (267 passed, 0 failed) exit 0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Documentation only — Leon's investigation record for the *"DeepSeek repeats its thinking and won't stop"* report on native + CUDA. **The finding: it is not a backend bug.** Native CUDA and ORT fall into the **identical verbatim repetition loop** on the hard prompt, diverging only at one `,`/`.` token at char 325 — an fp16-GPU vs fp32-CPU tie-break, not the cause. Neither emits EOS. It is DeepSeek-R1-Distill degenerating under greedy decoding, which is why its authors ship `do_sample: true, temperature: 0.6`. That reframes the `CUDA KV capacity exceeded (4097 > 4096)` error that opened this thread as a **downstream symptom** — the non-terminating loop grew KV into the native path's cap. ORT had no cap, so it looped on silently instead of erroring. The root cause is fixed in #392 and #385; #367 removed the arbitrary cap. The record also corrects a wrong intermediate conclusion — that native CUDA was unrunnable here for want of CUDA 13 redistributables. **The CUDA driver API (`nvcuda.dll`) ships with the display driver, not the toolkit**, and both the EP loader and cudarc fall back `*_13` → `*_12`. The real blockers were two NVRTC *header* gaps (`cuda_fp16.h`, `crt/mma.h`), host provisioning rather than code, and the recipe for resolving them is written down for the next engineer. Kept deliberately: an inferred capability claim that turns out to be wrong costs more than the investigation it prevents. --------- Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Recovered decision records from agents' per-worktree inbox files, which were deleted before Scribe ran. Substance survived in history.md and PR descriptions; durable rules now appended to decisions.md: From Rachael (PR #372): - Probe the stream you are writing to, not a convenient nearby one - Run the gate command CI runs, verbatim — local subset validation fails - Terminal behaviour requires PTY-driven tests; piped I/O cannot cover it From Zhora (PR #393): - Type-ahead is not lost during generation on Unix or Windows - PTY-harness hazards that present as swallowed input - Prefer an idle timeout to a total one when child is silent before first output From Leon (PRs #367/#385/#392/#395): - DeepSeek "repeats thinking" is not a backend bug, but greedy-decoding degeneration - Model-declared generation defaults are canonical; our constants are fallback, not override - CUDA driver API ships with display driver, not toolkit; inferred bad facts must be verified From Coordinator: - Standing Operational Rule: worktree lifecycle and decision merging — do not delete a worktree before Scribe has merged its decision inbox Before: 27,360 bytes | After: 34,662 bytes | Added: 7,302 bytes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…red native-CUDA parity test) (#1264) ## What this fixes `main` currently has a **red end-to-end native-CUDA parity test**: `native_cuda_pipeline_decoder_matches_cpu_token_ids` fails during `DecodeCudaState::new()` with `full CUDA mask reservation size overflow`. ### Root cause `DecodeCudaState::new` computed the mask *reservation* extent unconditionally: ```rust let full_mask_bytes = batch .checked_mul(capacity.max_len) // <-- capacity.max_len == usize::MAX .and_then(|e| e.checked_mul(size_of::<i64>())) .context("full CUDA mask reservation size overflow")?; ``` For a model with **no `max_sequence_length` metadata**, `resolve_cuda_kv_capacity` returns `capacity.max_len == usize::MAX` — the deliberate *"unbounded until growth fails"* sentinel (added in #367, asserted by `cuda_kv_capacity_without_metadata_is_unbounded_until_growth_fails`). So the multiply overflows and aborts construction — **even on the non-VMM decode path, whose mask binding is a plain `[batch, max_len]` allocation that never reserves the full logical extent and never reads `full_mask_bytes` at all.** The `tiny-gemma4-vlm-cuda` fixture used by the parity test is exactly such a metadata-less model, which is why the test is red. ### First bad commit Bisected to **#682 (`9fd7ee56`, "Reserve CUDA KV with VMM committed ranges")**, which introduced the unconditional `full_mask_bytes`. The `usize::MAX` sentinel predates it (#367). My earlier suspects **#1221** (two-pass workspace reservation) and **#1228** (KV mask growth) were both **cleared with evidence** — they are much later and touch different code (#1228 only added the `grow_vmm_mask_in_place` shape assertion). ### The fix Compute `full_mask_bytes` (and the `mask_committed_bytes` it feeds) **only inside the `kv_commits_on_demand` (VMM) branch that actually consumes them.** The `usize::MAX` sentinel is intended design and is left untouched. Behaviour is **byte-identical on every previously-working path**: the non-VMM branch always committed `mask_bytes`, and `seq_major_fixed` implies `kv_commits_on_demand`, so `full_mask_bytes` was only ever live on the VMM path. ### Captured operands at the failure (RTX 4060, non-VMM path) ``` batch = 1 capacity.max_len = 18446744073709551615 (= usize::MAX) source = "unbounded (model.max_sequence_length unavailable)" bytes_per_token = 40 kv_commits_on_demand = false (non-VMM path -- full_mask_bytes is dead here) ``` ## Verification (RTX 4060, driver 591.55, CUDA 13 runtime) - **Regression test** `native_cuda_decoder_constructs_with_unbounded_metadata_less_capacity` (CUDA-gated, `ONNX_GENAI_RUN_CUDA_SMOKE=1`): constructs a metadata-less non-VMM decoder and asserts `new()` succeeds + `capacity.max_len == usize::MAX`. - **GREEN** with the fix. - **RED** with the exact `full CUDA mask reservation size overflow` when the computation is hoisted back out of the branch (falsified by revert). - **Parity test** `native_cuda_pipeline_decoder_matches_cpu_token_ids`: **RED on `main` → GREEN with this fix.** - Engine lib suite (`--features "cuda,native-backend" -- --test-threads=1`): **548 passed, 0 failed, 2 ignored** (default). ## Scope note — the VMM half is NOT fixed here This fix makes only the **non-VMM** path correct. On the **VMM** path (`ONNX_GENAI_CUDA_VMM=1`) a metadata-less model would still overflow for real, because `full_mask_bytes` — and `full_vmm_kv_allocation_bytes(..., capacity.max_len)` — become *live* values that genuinely need a concrete bound. Resolving the unbounded sentinel to a real capacity before the VMM path can reserve is a design decision, not an obvious mechanical fix, so it is **filed as a separate issue** rather than papered over here (see linked issue). ## Not covered by this PR Running the lib suite with `ONNX_GENAI_RUN_CUDA_SMOKE=1` surfaces **3 pre-existing failures** (`native_cuda_capture_replay_...`, `native_cuda_symbolic_batch_...`, `native_cuda_symbolic_total_seq_...`) that fail with `cannot resolve model.io.token_input ... 3 ports match`. These are a pre-existing test-harness `model.io` ambiguity unrelated to this change (this PR does not touch graph building or io resolution); they only surface because CI never sets the smoke env. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
… path (closes #1266) (#1281) ## What this fixes Closes #1266 — the **VMM** half of the metadata-less mask/KV reservation overflow that #1264 deliberately left for a design decision. On the VMM native-CUDA decode path (`ONNX_GENAI_CUDA_VMM=1`), `DecodeCudaState::new` reserves the full mask island (`batch × capacity.max_len × i64`) and the full KV extent (`full_vmm_kv_allocation_bytes(..., capacity.max_len)`) up front. For a model with **no `max_sequence_length` metadata**, `resolve_cuda_kv_capacity` returns `capacity.max_len == usize::MAX` (the deliberate "unbounded until growth fails" sentinel from #367). Unlike the non-VMM path — where those values are *dead* (fixed in #1264) — on the VMM path they are **live**, so the reservation arithmetic overflows and aborts construction with `full CUDA mask reservation size overflow`. ## The decisive fact: the VMM reservation is virtual-only `allocate_device_binding_committed(..., allocation_bytes, committed_ranges)` → `CudaVmmAllocator::allocate_committed(bytes, align, ranges)` only **carves `bytes` out of a fixed 64 GiB virtual arena** (`RESERVATION_BYTES = 64 << 30`, `arena.spans.carve`). Physical granules are claimed **only** for `committed_ranges`. So the up-front reservation costs address space, not memory. ## Options weighed - **(a) Derive a concrete bound from device memory — CHOSEN.** Resolve the sentinel *only for the VMM reservation* to `device_free / bytes_per_token` (the largest sequence the device could ever physically hold). Because the reservation is virtual-only, an over-large-but-finite bound is nearly free (≈`device_free` of VA + a ~1.5 GB mask, well inside the 64 GiB arena), and a decode that outgrows it still fails at the same physical ceiling the sentinel implied. - **(b) Configurable default ceiling + bucket growth.** Rejected: it invents a second policy knob and a magic default when option (a) already has the exactly-right, self-tuning ceiling for free (the reservation is virtual, so there is no reason to pick anything smaller than the physical maximum). - **(c) Refuse VMM for metadata-less models, fall back to eager.** Rejected: silently strips VMM's whole benefit (in-place growth, no bind churn, capture stability) from an entire class of models — every model without declared `max_sequence_length` — for no gain, since (a) makes VMM work. - **(d) Resolve the sentinel at capacity-resolution time.** Rejected: cleanest-looking but rewrites `capacity.max_len` itself, changing the deliberate `usize::MAX` contract (#367) that the non-VMM grow-until-OOM path and `cuda_kv_capacity_without_metadata_is_unbounded_until_growth_fails` depend on. Not worth touching a deliberate contract when a local resolution suffices. The fix therefore resolves the bound **only locally, only for the VMM reservation**; `capacity.max_len` and the non-VMM path are untouched (the regression test asserts the sentinel survives). ## Falsification (RTX 4060, driver 591.55, CUDA 13 runtime) New CUDA-gated regression test `native_cuda_vmm_decoder_constructs_with_unbounded_metadata_less_capacity` drives real VMM-path construction with a metadata-less capacity, asserts it is *actually* on the commit-on-demand path (`kv_commits_on_demand()`), and asserts `capacity.max_len == usize::MAX` still holds. - **Fix in place → GREEN.** - **Revert the bound resolution (`vmm_reserved_max_len = capacity.max_len`) → RED with the real failure:** `Caused by: full CUDA mask reservation size overflow`. - **Restore → GREEN.** The VMM path is proven live, not a proxy: the arena log reports `device allocations go through a VMM arena over 68719476736 bytes of reserved address space`, and the assertion catches any silent fall-back to the eager path. The existing real-model growth test `native_cuda_vmm_kv_grows_in_place_and_commits_more_granules` (qwen05b-q4, `ONNX_GENAI_KV_MIN_BUCKET=8`) also passes — it calls `ensure_capacity(8193)` and asserts committed bytes grow by the exact bucket delta with `device_ptrs` unchanged (grown in place), confirming this change does not disturb the metadata-having VMM growth path. ## Test isolation note The test deliberately does **not** set `ONNX_GENAI_CUDA_GRAPH`: that is a frozen `RuntimeConfig` knob (#804), so mutating it after any earlier test froze the process-wide config trips the freeze-guard. Construction never captures a graph, so the knob is irrelevant here; `vmm_enabled()` reads `ONNX_GENAI_CUDA_VMM` directly from the environment, so setting only VMM is safe regardless of freeze order. Also folds in a rustfmt fix for an overlong line #1264 introduced in the same file. ## GPU verification (RTX 4060, `--features "cuda,native-backend" -- --test-threads=1`) - Engine lib suite (default): **559 passed, 0 failed, 2 ignored.** - Engine lib suite (`ONNX_GENAI_RUN_CUDA_SMOKE=1`): **556 passed, 3 failed, 2 ignored.** The 3 failures are the **known pre-existing** `model.io.token_input` harness ambiguities (`native_cuda_capture_replay_...`, `native_cuda_symbolic_batch_...`, `native_cuda_symbolic_total_seq_...`), unrelated to this change and identical to the set #1264 documented. Both new metadata-less tests pass; zero new failures. ## Known caveat The derived bound is `device_free / bytes_per_token`; the summed VA reservation is ≈`1.2 × device_free`. On this 8 GB card that is ~8 GB of the 64 GiB arena. On a very large GPU (≳~53 GiB free) the metadata-less VMM reservation could exceed the 64 GiB arena and fail the carve **loudly** at construction (no silent corruption). That is an acceptable, visible failure mode for a narrow combination (metadata-less **and** VMM **and** a huge idle GPU); a follow-up could cap the bound to the arena if it ever bites. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
The report
We did have dynamic allocation — on one path only.
kv_capacity_bucketTwo mechanisms for the same resource. That is the shape behind several bugs here recently, and a bigger constant would have left the divergence for someone to hit from another angle. Justin's instruction was explicit: "我们native的要和ort的整合。机制和能力应该要一样".
One mechanism
The growth policy now lives once in
onnx-genai-kv:kv_capacity_bucketplus aKvCapacityGrowthBackendseam and a sharedensure_kv_capacitydriver. Both backends consume it. The seam carries only primitives that genuinely differ — allocate, copy prefix, build mask, invalidate capture, commit — so a new backend implements primitives rather than re-deriving policy.Growth and CUDA graph capture coexist, as they already did on the ORT side: grow to the next bucket, D2D-copy the prefix, rebuild the mask, release the capture, re-capture at the new size. The comment claiming capture required a frozen physical capacity described the fixed-capacity design, not a real constraint — corrected, since a stale comment like that is what would make someone reintroduce the fixed buffer.
No derived VRAM ceiling
An earlier revision predicted a safe maximum from free memory × a fraction. Justin rejected it — "太hardcode了。天花板我认为是不需要。只是增长如果失效,优雅报错就行了" — and he is right: the snapshot is stale the moment another process allocates, and the fraction is exactly the constant
RULES.mdrule 2 exists to prevent.The only remaining ceilings are facts, not guesses: the model's declared
max_sequence_length, and an explicit user cap. Unset means grow until the model's own limit or until the device refuses.Which puts the weight on failing well
Growth is transactional. A failure at any fallible step leaves capacity, logical length, buffers and capture state exactly as they were, and the session stays usable. The commit phase is infallible assignment after all fallible work — which is why it has no failure case to cover.
Failures are translated, not passed through: old and new bucket, the transient peak, bytes per token, device free/total, and the recovery levers.
The transient peak is worth stating because it is unintuitive — growth holds the old and new buffers at once, so the requirement is roughly
(old + new) * bytes_per_token. A grow can fail with what looks like ample free memory. That insight came from an earlier clamp attempt and survives as documentation.Verification
On a real RTX 4060: forced
4 → 8 → 16growth with genuine reallocation and re-capture, and DeepSeek output stayed coherent. Coherence is the assertion that matters — a corrupted prefix copy produces wrong tokens, not an error.A real allocator OOM cannot be forced on a WDDM device — Windows virtualises and pages GPU memory, so driving free to zero still allocates. That is documented so nobody re-burns the time.
Coverage for the failure path is therefore model-free injected failure at the shared seam: allocation, prefix copy, mask allocation and capture invalidation each fail in turn; each asserts no commit and an unchanged session, then proves a retry succeeds. These run in CI on every PR rather than only where a GPU exists — strictly better coverage than the GPU test could give.
Also
The Qwen CUDA smoke fixture is now env-overridable via
ONNX_GENAI_QWEN_CUDA_SMOKE_MODEL; it previously skipped on a hardcoded Linux path, and a silently skipping CUDA test is the category we spent yesterday eliminating.Gates
Build,
onnx-genai-kv101 tests, engine lib 253/0/1, fmt, clippy, CUDA feature check — all pass.Review round
Copilot caught a stale doc comment on
load_with_cuda_kv_max_lenstill describing the removed free-memory clamp. Fixed, and swept the whole branch for the same class rather than the one line — a stale comment describing a removed mechanism is how the mechanism gets reintroduced, and this PR had already corrected one instance of exactly that. The sweep found nothing else: remaining free-memory references are failure-time diagnostics, not ceiling derivations, and the 4096 values left in tests aremax_sequence_lengthfixtures.A merge with
mainsurfaced a design-fact conflict rather than a text one.main's comment said the ORT shared-buffer path "pre-allocates a KV buffer of exactly that many tokens up front"; ours said it grows by buckets. Both cannot be true, and getting it wrong inside a PR about comments that describe things that are not true would be self-defeating — so it was verified empirically instead of decided.Verdict: ours is correct,
main's is stale.dynamic.rs:386is the only shared-buffer init path and callskv_capacity_bucket(0, max_length), which peronnx-genai-kv/src/lib.rs:73yields 256 — the minimum bucket, notmax_length. Growth is lazy viaensure_kv_capacity.Both comments now state the whole truth — it does pre-allocate, at the minimum bucket — since ""grows by buckets"" alone left readers unsure whether anything is allocated at session start. The 256 default and the
ONNX_GENAI_KV_MIN_BUCKETname were verified in code before being written down, and the proposed wording ""raised by"" was corrected to ""overridden by"": the env var accepts any positive value, so it can lower the floor too.main's new GQA contract validation (validate_key_sequence_lengths_contract) and this PR's KV capacity parameter are orthogonal; the merge keeps both.Gates
15/15 green — full platform matrix, three coverage flags, Miri, benchmarks, audit.