Repository navigation
fix(memory): revert unsound capability-identity/atomic-release apparatus (#1186 corrective) - #1237
Closed
justinchuby wants to merge 1 commit into
Closed
justinchuby wants to merge 1 commit into
justinchuby wants to merge 1 commit into
Conversation
Corrective follow-up to the #1186 Phase 2 capability split, replacing an unsound "unforgeable mechanism identity + atomic release" design that review rounds 5-6 (on the now-draft PR #1233) found could still be defeated (split-inner-wrapper composition, self-reported identity, and a CUDA release path whose "atomicity" claim did not match what the code actually guarantees). Rather than continuing to patch that apparatus, this removes it and restores the round-1 shape while keeping round-1 findings 2-5: - Delete `MechanismId` and `capability_shares_mechanism` entirely (no self-reported identity token remains anywhere in the public API). `DeviceAllocator::mechanism_id()` and `HostAllocator`/`CudaVmmAllocator` overrides are removed; no new base-trait `'static`/`Any` bound is introduced (`DeviceAllocator` still supports non-'static borrowed implementors). - `CudaExecutionProvider::with_memory` keeps only its real, honest device-key sanity checks (rejecting a capability whose device disagrees with its own allocator) and drops the identity-based rejection it could not actually deliver on. - `SharedMapping::release_shared_mapping`'s doc no longer claims a driver-level atomic release; it now honestly describes the bookkeeping-level exact-once guarantee this crate's locking already provides, separately from true CUDA-driver atomicity (deferred to Phase 4's owning-token/generation design). - `vmm_allocator.rs`'s partial-unmap/free-list gap in `deallocate_span`/`release_granules_report` is documented as a pre-existing property that predates Phase 2 (not introduced or altered by it), deferred to Phase 4 rather than patched with more unproven mechanism. `main` never merged `ReleaseReport` or a generic `with_memory<T>` (those only existed on the un-merged PR #1233 branch), so blockers 3 and 5 from the round-6 review required no code changes here. Validation: `cargo check --workspace --tests`, `cargo clippy` (deny warnings) on onnx-runtime-memory-api/-memory-governor/-cuda-memory/ -ep-cuda with zero new findings (all remaining clippy/rustdoc issues confirmed pre-existing on `main` via stash comparison), `cargo test` on the same four crates (all green, 0 failures), and `cargo doc` (deny warnings) clean for memory-api and cuda-memory, with zero new rustdoc regressions for memory-governor/ep-cuda (identical pre-existing error sets diffed against `main`). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b2f3675-eff5-46ec-b60a-74f24d52984b
🔴 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
|
justinchuby
marked this pull request as draft
August 18, 2026 14:12
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
## Summary - Reverts the exact merged squash commit `5c181b49ed9ee073639f1ee3df11ef7c597eec46` from #1192. - Withdraws the bundled #1186 Phase 1 + Phase 2 memory capability split rather than attempting another fix-forward patch. - Preserves unrelated mainline work merged after the squash. Part of #1186. ## Why revert first Subsequent high-effort review found the merged release/identity design structurally unsafe: - shared-mapping release can race free-list address reuse; - `0` conflates a legitimate no-op/refund result with failure; - partial CUDA unmap and handle-release failures can leave software and driver state divergent; - pointer-only identity is vulnerable to ABA/address reuse; - the contract makes mechanism-identity guarantees it cannot actually prove. These are lifecycle and ownership hazards, not isolated implementation bugs. Leaving the contract on `main` while iterating would expose callers to unsafe semantics, so this PR withdraws it completely. ## Related work - #1192 merged the Phase 1 branch into `main` as squash `5c181b49`. - #1206 merged Phase 2 into the Phase 1 branch and was therefore bundled into that squash. - Draft #1233 attempted to structurally pair capability allocation/release. - Draft #1237 attempted a narrower corrective rollback of the identity/atomic-release apparatus. Those drafts do not eliminate the need to withdraw the already-merged contract. A safe Phase 1 and a deliberately narrowed Phase 2 will be re-landed as a new reviewable stack after redesign. ## Validation - `cargo metadata --locked --no-deps --format-version 1` - `cargo check --locked --all-targets -p onnx-runtime-memory-governor -p onnx-runtime-cuda-memory -p onnx-runtime-ep-api -p onnx-runtime-ep-cuda -p onnx-genai-ort` - `cargo test --locked -p onnx-runtime-memory-governor --lib` — 64 passed - `cargo test --locked -p onnx-runtime-cuda-memory --lib` — 8 passed - `cargo test --locked -p onnx-runtime-ep-api --lib provider::tests::` — 6 passed - `cargo test --locked -p onnx-runtime-ep-cuda --lib provider::tests::` — 8 passed, 6 GPU-gated tests ignored - `cargo test --locked -p onnx-genai-ort --lib governed_allocator::tests::` — 20 passed - `git diff --check` Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Superseded by the safety revert in #1247. Phase 1 and a narrower Phase 2 will be re-landed as a fresh reviewed stack; this corrective branch targeted code no longer present on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1186
Context
PR #1233 (Phase 2 of #1186, branch
justinchuby-1186-memory-api-phase2-round5-fix) went through six review rounds trying to make an "unforgeable mechanism identity + atomic release" apparatus sound enough to safely bind capability allocation/release to one mechanism. Round 6 found it was still unsound (a#[repr(C)]split-inner wrapper could forward a foreign capability from an offset-zero field and defeat any self-reported identity check; the CUDA release path's "atomicity" claim did not match what the code actually does; the design kept adding machinery rather than closing the gap). The coordinator ordered a full stop: PR #1233 is now marked draft and is being left completely untouched by this PR — no commits, pushes, or state changes were made to it or its branch.This PR is a corrective, design-only response, on a fresh branch off
main, that removes the unsound apparatus rather than patching it further, and is scoped to what actually needs fixing onmaintoday.What this does
MechanismIdandcapability_shares_mechanismentirely. No self-reported identity token remains anywhere in the public API.DeviceAllocator::mechanism_id()and theHostAllocator/CudaVmmAllocatoroverrides are removed. No new'static/Anybound is added to the baseDeviceAllocatortrait — non-'staticborrowed implementors remain valid (a dedicated compile/runtime test proves this).CudaExecutionProvider::with_memorykeeps only its real checks. It still rejects an injected allocator whose device disagrees with the provider's own device, and still rejects injection once a VMM arena is already selected (unchanged, real, narrower guarantee). It no longer claims to reject foreign-capability composition via identity — because no self-attested identity check can actually prove that (a type can honestly returnSome(self)from a capability accessor while its methods secretly operate on unrelated state; closing this for real needs a registry that issues capability access rather than trusting self-report, which is [memory] Separate allocator, backing, and lifetime capabilities #1186 Phase 3's "provider/context pinning and binding identity" work, not Phase 2's).SharedMapping::release_shared_mapping's doc no longer overclaims atomicity. It now honestly describes the bookkeeping-level exact-once guarantee this crate's own locking already provides (repeated/unmatched calls on the sameptrrefund zero rather than double-counting), explicitly distinguished from genuine CUDA-driver-level atomicity, which is deferred to Phase 4's owning-token/generation design.deallocate_span/release_granules_reportpartial-unmap gap. If a partial CUDA release fails for one granule run, the address range is still handed back to the free list unconditionally, which can let a later allocation overlap granules the bookkeeping still believes are committed. This predates Phase 2 by months and is not something Phase 2 introduced or altered — it's called out explicitly as deferred to Phase 4 (owning allocation token / generation counter to keep a VA range out of the free list until release reaches a consistent terminal state), instead of being patched with more unproven machinery under time pressure.Why blockers 3 and 5 needed no fix here
Two of the five round-6 blockers (
ReleaseReport.completebeing discarded by callers, and a genericwith_memory<T>regression) only existed on the un-merged PR #1233 branch.mainnever hadReleaseReport— release methods here are still bareu64— andwith_memoryis already the trait-objectArc<dyn DeviceAllocator>form. No code changes were needed onmainfor those two.What's preserved (round-1 findings 2-5, from the original Phase 2 work)
as_shared_mapping()still returnsNonefor detached/pool-less allocators.incremental_owned_bytes_for_shared_prefixstill checks device/authority before treating a foreign prefix as zero-cost.mapped_bytes_for_allocation_ranges's default is still a real, granularity-aware conservative estimate (not a naive sum).deallocate_with_unmappedmapped-release reporting still lives onVirtualBacking/SharedMappingrather than silently defaulting to0for capability-bearing mechanisms.OnceLock-style selection remains in place).Validation
cargo check --workspace --tests --exclude onnx-genai-bench— clean (only pre-existing unrelated warnings inonnx-genai-ort).cargo clippy -p onnx-runtime-memory-api -p onnx-runtime-memory-governor -p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda --tests -- -D warnings— zero new findings in any file this PR touches; all remaining errors are confirmed pre-existing onmainviagit stashcomparison (all inonnx-runtime-ep-cpu/onnx-runtime-ep-cudakernel/optimizer files unrelated to memory code).cargo test -p onnx-runtime-memory-api -p onnx-runtime-memory-governor -p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda— all green, 0 failures (381 tests in the ep-cuda unit binary, 13 in memory-api, 61 in memory-governor, remaining GPU-gated binaries correctlyignoredin this no-CUDA sandbox).cargo doc --no-deps -p onnx-runtime-memory-api -p onnx-runtime-cuda-memorywithRUSTDOCFLAGS="-D warnings"— clean.cargo doc --no-deps -p onnx-runtime-memory-governor -p onnx-runtime-ep-cudawithRUSTDOCFLAGS="-D warnings"— remaining errors are byte-for-byte identical tomain's baseline (diffed the exact error/line sets against agit stashd tree); this PR introduces zero new rustdoc regressions.Scope discipline
main(notjustinchuby-1186-memory-api-phase-1/PR fix(memory): structurally pair capability allocation/release (#1186 Phase 2 round-5 fix) #1233's branch), since this is a corrective fix to what's already merged, not a continuation of the draft.ProcessMemoryManager, no RAII allocation handle/deferred-free queue, no plugin ABI, noExecutionProvider::allocator(), no Phase 3/4 work started.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com