Repository navigation
fix(memory): correct Phase 7's unverifiable claims and anchor count_code - #1468
justinchuby wants to merge 1 commit into
Conversation
Phase 7's production change was reviewed and found correct. This revises the claims that stood in place of verification that cannot be done on a host with no CUDA, and pins the one test helper that was still unpinned. - Anchor `count_code` in `no_built_in_eager_allocator.rs`. Every assertion built on it is an `is_empty()`, so blinding the helper to `.filter(|_line| false)` left the file 6/6 green and let a resurrected `CUDA_VMM_ENV` back into production undetected. The existing anchor test covers `count` only, and the prose/code companion stays green when the helper is blinded. Add a positive `count_code` assertion against a constant that lives on a code line now. - Drop the granularity capability claim. `allocation_granularity` substitutes 2 MiB for a driver refusal or a reported zero, so the arena builder's `granularity == 0` guard is unreachable from the CUDA provider and `cuMemAddressReserve` is the sole init-time detector. Corrected in the provider diagnostic and in both design-doc passages, and recorded at the two code sites so it is not re-derived. - Correct the retained physical-handle pool rows. It is on at 256 MiB by default on the standalone/plugin path and on the governed lending path; the env var overrides that default rather than enabling a pool. Both the design doc and the Chinese wiki table said it was off by default. - Cover `production_physical_pool_enabled`, which had no coverage at all and whose meaning changed in this phase. - Fix the drop ordering in the GPU-gated late-injection test: `with_memory` takes `mut self`, so a refused injection consumed and dropped the provider while a buffer was still outstanding. No production behaviour changes. Part of #1186 — Phase 7 only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## justinchuby-sturdy-potato #1468 +/- ##
=============================================================
+ Coverage 79.38% 79.55% +0.17%
=============================================================
Files 373 374 +1
Lines 165958 168905 +2947
Branches 165958 168905 +2947
=============================================================
+ Hits 131740 134376 +2636
- Misses 29372 29669 +297
- Partials 4846 4860 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison 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
|
> **A100 safety update (head `13c37a7f`):** expected shared-prefix admission failures are now all-or-private: K/V commit is transactional, a failed V share rolls K back before enqueue, and only a successful rollback permits private-KV fallback. Fatal kernel faults remain process-fatal; the new cross-process VMM probe proves a worker fault does not affect an actively executing peer, while the current server is still in-process and does not yet consume physical shared-prefix metadata. See the [final verified conclusion](#1579 (comment)). Collapses phases 1–7 of the #1186 memory architecture rework onto current `main` as a single merge. The stack forked 270 commits ago (176 of them touching `crates/`), so rebasing it layer by layer would mean solving the same 17 conflicts fourteen times, with no reviewer ever looking at the intermediate states. Closes the stack: #1252 #1263 #1279 #1283 #1301 #1341 #1349 #1426 #1440 #1448 #1454 #1462 #1465 #1468 #1533. **A review guide is in the first comment.** It is the part worth reading — this diff is 103 files, but only three decisions in it are ones a compiler cannot check. ## What was already reviewed, and what wasn't Every phase was reviewed and approved by a session that was not its author, under the rejection-lockout rule (a rejected author never writes the next revision). | Phase | PR | Verdict | |---|---|---| | 1–5 | #1252 → #1426 | approved | | 6 | #1440, #1448, #1454 | **rejected** ×3 | | 6 | #1462 | approved | | 7 | #1465 | **rejected** | | 7 | #1468 | approved | | 7 test fixes | #1533 | approved, A100-verified | The four rejected rounds were not replaced — the later rounds are stacked **on top of** them. So the tree here is the approved state, but the history contains the rejected commits. **#1440 / #1448 / #1454 / #1465 should not be reviewed individually**; they close automatically. **Not reviewed anywhere:** the conflict resolutions themselves. That is what this PR is for. ## Verification Run here, on this merged tree: - `cargo check --workspace --all-targets` — clean - `cargo check -p onnx-genai-engine --features cuda,native-backend --all-targets` — clean. Worth calling out: the default feature set does **not** compile the `cfg(cuda)` code, which is where the riskiest edits are. Checking only the default set would have missed a real break (see the guide). - `cargo test` over 7 crates — **1105 passed, 2 failed, 88 ignored** - `cargo clippy --workspace --all-targets` - `cargo fmt` **The 2 failures and the 1 clippy error are pre-existing and proven so, not merge damage:** - `platform_capacity::{disk_capacity_is_measured_for_the_working_directory, an_explicit_byte_limit_is_honored_without_a_device_query}` — `platform_capacity.rs` is byte-identical to `main` (`git diff origin/main HEAD -- ` that file is empty). The cause is a macOS-only FFI layout bug: `fsblkcnt_t` is 4 bytes on macOS (confirmed: `sizeof(struct statvfs)=64`, `sizeof(fsblkcnt_t)=4`) while the Rust struct declares `f_blocks`/`f_bfree`/`f_bavail` as `u64`. CI is Linux, where it is 8 bytes. Left alone: it is a real bug but not this PR's. - `optimizer.rs` clippy `approx_constant` — present on both parents; the merged file is byte-identical to `main`. - Three files had `rustfmt` drift already present on `main` (`matmul_nbits.rs`, `normalization.rs`, `optimizer.rs`). Reverted rather than swept in, to keep the diff readable. **Final A100 revalidation (head `13c37a7f`):** full CUDA-memory GPU suite passed; CUDA EP default-parallel lib suite passed (**488 passed / 17 ignored**); real fp16 GQA covered three requests × two interleaved decode steps with two shared peers and one transactional private fallback, byte-identical to independent GPU and CPU references. A device-started/event-gated worker remained healthy through a peer process `CUDA_ERROR_ILLEGAL_ADDRESS`, owner exit, replacement worker, and process restart. Governor tests, targeted Miri, CI Clippy, fmt, and CUDA honesty also passed. ## Still open after this merges - `memory-plugin-provider-wiring` — the back half of Phase 6 criterion 8 (provider/context pinning), which fell in the gap between phases. **#1186 must not be closed until it lands.** This merge makes it more tractable, not less: see decision 3 in the guide. - `memory-deferred-invariant-asserts` — two surviving mutants found by #1462's reviewer, adjudicated NON-BLOCKING and not defects. - #1533's CUDA honesty guard warns on a legitimately portable anchor. Should be moved out of the CUDA test binary or allow-listed rather than left warning. --------- Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Copilot-Session: 46c5d75b-8146-489c-b82f-08ee29c27ce4 Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100 Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
|
Closing: superseded by #1579, which collapsed phases 1–7 into a single merge — landed as This PR was approved by a session that was not its author, and its work is in Containment verified, not assumed: Every one of the 15 stack heads (#1252 → #1533) is a literal ancestor of the merged head, and I separately confirmed the merged head's content reached Why this didn't close itself: #1579's body says #1186 stays open: #1579 explicitly gates it on |
Part of #1186 — Phase 7 only. Base is
justinchuby-sturdy-potato(Phase 6, head21370921, approved).Supersedes #1465, which stays open as the review record. Nothing was pushed to it.
Phase 7 removes the built-in CUDA eager allocator (
CudaDeviceAllocator,cuMemAlloc) and makes the VMM arena the sole built-in CUDA memory mechanism. #1465's production change was reviewed and no production defect was found. It was rejected on three factual claims that stand in place of verification that cannot be performed on this host, plus one missing test anchor. This revision fixes exactly those. There are no production behaviour changes; the single production string touched is the diagnostic atprovider.rs:408.This host is macOS/arm64 with no CUDA and no NVIDIA driver, so every behavioural CUDA path is GPU-gated and cannot run — 459 tests are
ignored. Because so much is unverifiable here, the written claims carry the weight the tests cannot, which is why their accuracy is the acceptance surface.What changed
1.
count_codeis anchored against vacuity (blocking)crates/onnx-runtime-ep-cuda/tests/no_built_in_eager_allocator.rsthe_removed_type_and_flag_are_absent_from_production_codeis built entirely on thecount_codehelper, and every one of its assertions ishits.is_empty(). Replacing the helper's comment filter with.filter(|_line| false)yields a scanner that reads no lines at all, which satisfies every emptiness assertion trivially — the file stayed 6/6 green and the crate pair stayed at 464/0.I reproduced the reviewer's compound exploit: with the helper blinded, reintroducing
pub const CUDA_VMM_ENV: &str = "ONNX_GENAI_CUDA_VMM";into productionvmm_allocator.rskept the whole file green (M2 below). Without the blinding, that same regression is killed by two tests (M3). So the test works today; the helper it reads through was unpinned.Correction to #1465's disclosures #1 and #2. They named
the_scan_can_observe_an_eager_call_site_that_is_known_to_existas the protecting anchor. That was wrong: at #1465 that test usedcountonly and never calledcount_code. The companionthe_removal_stays_explained_in_prose_and_the_code_scan_can_tell_the_differencedid not anchor it either — it assertscountseesONNX_GENAI_CUDA_VMMandcount_codedoes not, and both remain true whencount_codeis blinded (verified: that test stayed green under M1).The fix extends the anchor test so it now pins both helpers, and its doc comment states why they must be pinned separately. The new assertion is
count_code(&crate_src("onnx-runtime-cuda-memory"), "CUDA_PHYSICAL_HANDLE_POOL_BYTES_ENV")being non-empty — a constant declared on a code line that is not going away, since it is the surviving pool-bound variable that the shipped-constraints table documents.Also added, as a scope note in the module docs: the eager-site allowlist counts
malloc_sync/free_synconly.cudnn/mod.rs:621reaches the device eagerly via.alloc_zeros::<u8>(...)— a third eager device allocation, pre-existing, untouched, and outside theDeviceAllocatorseam, so criterion 2 is unaffected. The point is that the allowlist should not read broader than it is.2. The granularity capability claim is dropped (blocking)
allocation_granularity()(virtual_memory.rs:2079-2093) ends:A driver refusal or a reported zero is silently replaced with 2 MiB. Both callers —
CudaVirtualBacking::granularity()(:2290) andPhysicalHandlePool(:1378) — go through it, so thegranularity == 0early-return inbuild()(vmm_allocator.rs:1160) is unreachable from the CUDA provider. Independently verified.Criterion 3's substance survives.
reserve()(virtual_memory.rs:2312) propagatescuMemAddressReservefailures throughcheck()with no fallback, so an unsupported device still fails fatally at construction with the intended diagnostic. The probe simply stands on one leg, not two.Per the review, I took the smaller in-scope fix — state honestly which call detects the unsupported device — rather than rewriting
allocation_granularity(). That would be a production behaviour change to a pre-existing untouched function, and it cannot be exercised on this host.The brief listed three claim sites. There is a fourth, which I fixed too:
provider.rs:408diagnosticcuMemAddressReserveas the detector and says the granularity query is not a capability checkMEMORY_MANAGEMENT_MODEL_DESIGN.md:734(table)cuMemAddressReserveand not here"MEMORY_MANAGEMENT_MODEL_DESIGN.md:708(prose, not in the brief)cuMemAddressReserve… and the driver's reported allocation granularity, both of which a device without VMM support refuses"cuMemAddressReserveis the single init-time detector; the granularity query is explicitly not a second onebuild()already callsgranularity()(errors on zero)"Leaving
:708would have left the document contradicting its own table.I also recorded the fact at the two code sites — a doc comment on
allocation_granularityand a comment on the now-unreachablegranularity == 0guard. This matters because the error will not self-correct: a CUDA host following the "what a CUDA host must check" item 3 will observe the diagnostic working viareserve()and conclude the documented boundary was accurate.3. The retained physical-handle pool rows are corrected (blocking)
As shipped in
provider.rs:Some(DEFAULT_STANDALONE_PHYSICAL_POOL_BYTES)at:841, const256 << 20at:277auto_dynamic_lending.then_some(256usize << 20)at:821The arena constructors do
physical_handle_pool_bytes().or(default_pool_bytes)(vmm_allocator.rs:942, 979, 1086, 1116), soONNX_GENAI_CUDA_PHYSICAL_HANDLE_POOL_BYTESoverrides a default that is already present — it does not enable one.Two documents said the opposite, so this was systematic rather than a typo. Both corrected:
docs/memory/MEMORY_MANAGEMENT_MODEL_DESIGN.md:735— was "Off unless…POOL_BYTESis a positive byte count"wiki/memory/Memory Management for Beginners.md:334— was "默认关闭;…设为正整数字节数开启". Kept in Chinese, matching the surrounding register.Criterion 11 requires retained-pool bounds documented as shipped constraints, and the table's own framing is "the first things worth knowing when diagnosing it" — so an operator asking "is device memory being retained?" was getting the wrong answer on the most common path. Both rows now also state that zero or unparseable means "fall back to the path default", never "a pool of zero".
4. Removal-record line count corrected (non-blocking)
device_allocator.rswas 304 lines, not 206. No figure matched 206:non-comment 181; non-blank non-comment 166; diffstat agrees at −304. Full record below.
5. Two cheap items (optional, both done)
production_physical_pool_enabled()(vmm_allocator.rs:122) had no coverage at all — mutating its body totruesurvived everywhere including the engine suite. Its semantics changed in this PR: it no longer requires the removed flag, soONNX_GENAI_CUDA_PHYSICAL_HANDLE_POOL_BYTESset alone was previously ignored and is now honoured. That change is correct and was disclosed — its sole consumer isengine/load.rs:608→uses_governed_physical_pool→cuda_weight_startup_reservation, and since the arena now always appliesphysical_handle_pool_bytes().or(default), leaving the predicate gated on a deleted flag would make the engine mispredict whether an authority-owned pool exists. Behaviour kept; now covered.The new test computes its expectation from the already-pinned
parse_physical_handle_pool_byteshelper rather than restating it, so it asserts the composition — that the predicate asks the environment exactly one question and applies no second condition. Its doc comment states the scope limit plainly: nothing in the workspace callsset_varfor this variable, so it is absent when the suite runs and the expectation isfalse, which is what kills atruebody; a developer who has exported the variable will still see it pass, because it checks agreement rather than a fixed answer.device_allocator_gpu.rs,injection_is_refused_once_the_live_mechanism_has_served_memory. Confirmed:with_memorytakesmut self, so the failing call consumes and drops the provider, tearing down the arena whilebufferis still outstanding; the test then built a second provider,let _ = provider;, and droppedbuffer. The stated intent was not demonstrated, and the drop could hit a teardown assertion or a use-after-free on a real device.I fixed it, and the fix required a judgement I should flag: with
mut selfthere is no way to hold a provider across a failedwith_memoryat all, so "the provider is unchanged by the refusal" is not expressible against this API — reordering alone cannot rescue it. The test now releases the buffer through the original provider first (which is what actually demonstrates "the mechanism that served the pointer releases it"), then attempts the injection, which is still refused becauseep_allocationsis monotonic soserved > 0stays true. Nothing is outstanding when the provider is consumed. The reasoning is written into the test's doc comment. This is GPU-gated so it cannot run here, but it type-checks under--features gpu-tests --all-targets.Criterion-by-criterion
device_allocator.rsdeleted (−304);the_cuda_memory_crate_has_no_eager_allocation_sitescuMemAllocthe_removed_type_and_flag_are_absent_from_production_code, now anchored at depth 1. Scope of the allowlist (malloc_sync/free_synconly;cudnnalloc_zerosexcluded and why) disclosed in the module docscuMemAddressReserveinCudaVirtualBacking::reserveis the sole init-time detector — failure propagates throughcheck()with no fallback. The granularity probe is not a second leg:allocation_granularitysubstitutes 2 MiB for a refusal or a zero, making thegranularity == 0guard unreachable from the CUDA provider. Corrected at all four sites; regraded from #1465DeviceAllocatorcontract unchanged; injection still supportedwith_memoryunchanged and still honouredONNX_GENAI_CUDA_VMM/CUDA_VMM_ENVdeleted; absence pinned in code, presence pinned in prosethe_memory_crate_provides_exactly_one_built_in_mechanism(exactly 1, and it isvmm_allocator.rs)with_memoryretires the arena; refused only for foreign device or already-served memory, before the offered allocator is usedno_built_in_eager_allocator.rs, 6/6, runs everywhereignored; no CUDA device and no NVIDIA driver on macOS/arm64vmm_allocator.rsCriterion 8 is kept split into its met and unmet halves, and criterion 10 stays unticked with no number invented — that handling was explicitly endorsed in review.
Timemachine removal record (criterion 12)
crates/onnx-runtime-cuda-memory/src/device_allocator.rs— 304 lines (181 non-comment; 166 non-blank non-comment)CudaDeviceAllocator,QuarantinedCudaAllocation, andimpl DeviceAllocator for CudaDeviceAllocatorONNX_GENAI_CUDA_VMM(env var) and its constantCUDA_VMM_ENVpub mod device_allocator;export fromonnx-runtime-cuda-memory/src/lib.rs; thedevice_allocatorre-export fromonnx-runtime-ep-cuda/src/lib.rs; and the CUDA EP's VMM→eager fallback inprovider.rs4d2b2cc5— feat(memory): add stream-ordered owning release, the last commit to touch the file before removal21370921(Phase 6 head, this PR's base)DeviceAllocatoris unchanged and a caller who wants eagercuMemAllocinjects it throughCudaExecutionProvider::with_memory.git show 21370921:crates/onnx-runtime-cuda-memory/src/device_allocator.rsWhat a CUDA host must still check before merge
Nothing below can be executed on macOS/arm64.
with_memoryinjection is honoured, is authoritative, and is refused in exactly the two intended cases.cuMemAddressReserveand only that. Observing the diagnostic fire does not confirm the granularity probe contributes — it cannot, since a refusal or a zero is replaced with 2 MiB.injection_is_refused_once_the_live_mechanism_has_served_memorypasses with its new ordering, and no teardown assertion fires.ONNX_GENAI_CUDA_PHYSICAL_HANDLE_POOL_BYTESon the governed non-lending path newly gets an authority-owned pool — the predicate no longer requires the deleted flag — and therefore newly facesadopt_memory_governor's authority-match check. Confirm that a mismatched authority produces the intended error rather than a surprise at load.Mutation table
Every mutation used Python exact-string replacement with a
count == 1guard, printed the mutated line, and was re-confirmed withgit diff -U1. Nosed -i '' '<N>s/...'line-number substitution was used, per the two harness hazards recorded on this task.count_code:.filter(|line| !line.trim_start().starts_with("//"))→.filter(|_line| false)— atec75da9d, before my fixthe_removal_stays_explained_…also stayed green, confirming it does not anchor the helperpub const CUDA_VMM_ENV: &str = "ONNX_GENAI_CUDA_VMM";into productionvmm_allocator.rsCUDA_VMM_ENVreintroduction alone, helper healthythe_removed_type_and_flag_are_absent_from_production_code+the_removal_stays_explained_…. Confirms the test works and only the helper was unpinnedthe_scan_can_observe_an_eager_call_site_that_is_known_to_existfails: "the code scan found no occurrence of a constant that is declared on a code line right now … everycount_code(..).is_empty()assertion below is vacuous: {}""CUDA_PHYSICAL_HANDLE_POOL_BYTES_ENV"→"…_ENV_NOPE"production_physical_pool_enabled()body →trueec75da9d, including the engine suiteAll mutations restored and each restore re-verified with
git diff.A third harness hazard, hit and caught. After M4 the naive restore needle
.filter(|_line| false)occurred twice — once in code and once in my new doc comment, which quotes the mutation to explain it. My harness'scount == 1guard refused the restore rather than silently mutating the wrong line. This is exactly hazard #2 from the brief (the reviewer'scount == 1ontruefinding 2 and leaving a file mutated), arriving from the opposite direction. I restored using a unique two-line anchor (.lines()+ the filter) and verified withgit diff. Recording it because the guard is what caught it: a restore step needs the same discipline as the mutation step.Validation
Baseline established at
ec75da9dbefore any edit.cuda-memory,ep-cuda,memory-abi,memory-governor,memory-host,memory-testplugin,virtual-memory)production_physical_pool_enabledtestmemory-abi55 +memory-host55 +memory-testplugin6)no_built_in_eager_allocator.rscargo fmt -p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda -- --checkcargo clippy … --all-targetsonnx-runtime-cuda-memory(where every new doc comment lives)cargo check -p onnx-runtime-ep-cuda --features gpu-tests --all-targetsonnx-genai-engine(consumer of the changed predicate)Clippy's one
error(approximate value of f32::consts::PI) is incrates/onnx-runtime-ep-cuda/src/kernels/standard_attention.rs, which this PR does not touch — pre-existing. Every clippy diagnostic sits inkernels/,optimizer.rs,ep-cpu, or two unrelated GPU test files; none are in the five files changed here.ep-cuda's rustdoc warnings are the pre-existing intra-doc-link set inkernels/anddeferred_release.Known pre-existing failures were not touched and are not reported.
Corrections to the brief
Stated with evidence rather than complied with silently.
The granularity claim had a fourth site, not three.
MEMORY_MANAGEMENT_MODEL_DESIGN.md:708says the capability is exercised by "cuMemAddressReserve… and the driver's reported allocation granularity, both of which a device without VMM support refuses" — the same wrong claim in prose, a few lines above the table row the brief did list. Fixing only the three listed sites would have left the document contradicting its own corrected table. Fixed.Edit 5b could not be fixed by reordering. The brief framed it as "fix the ordering if you can do so confidently". Reordering alone cannot work:
with_memory(mut self, …)consumes the provider on the error path too, so there is no arrangement in which a provider survives a refused injection. The test's stated intent is not expressible against this API. I rewrote it to assert what is true and safe — release through the original mechanism first, then observe the refusal, which still fires becauseservedis monotonic. Flagging it because it is a slightly larger change than "reorder two lines", though still test-only.The baseline crate set needed pinning down.
-p onnx-runtime-cuda-memory -p onnx-runtime-ep-cudaalone gives 464/0/458, and all eleven memory+CUDA crates give 785/0/459. The brief's 696/0/459 is the seven-crate set named in the table above (40+424+55+95+55+6+21 = 696; ignored 68+390+1 = 459). The brief's numbers were right; I note the composition so the next author reproduces the same set rather than a different one.464is separately correct as the two-crate figure quoted in the Edit 1 blinding description.I found nothing wrong with the substance of Edits 1–4, and I verified each independently rather than taking them on report: the blinding survival, the compound exploit, the 2 MiB substitution and the resulting unreachable guard, all three pool-default paths and the
.or(default)override semantics, and the 304-line count.Do not merge, do not enable auto-merge, do not enqueue. This stays open for human review, like every PR in this stack. CI will sit pending for hours due to the Actions backlog; that says nothing about this change, and local validation is above.