Repository navigation
feat(memory): pin the provider/context across a plugin's deferred release - #1725
Merged
Merged
Conversation
…ease Closes the back half of #1186's Phase 6 criterion 8. The module half was already enforced; the provider/context half had nothing to attach to. A stream-ordered free retires against whoever owns the device resources. If that owner finished teardown first, the plugin unmaps handles the teardown already released. Nothing on the Rust side reports this: release_completed touches only host state, which is deliberately built to outlive the allocator, so the host stays quiet while the driver is handed a double free. That silence is why the gate is on the way in rather than on the completion. - memory-api gains two object-safe traits so the plugin host can hold a pin without depending on the governor. - The governor implements them over ContextRecord::begin_transaction, so a plugin release is one entry in the same count remove_provider_context already waits on rather than a parallel mechanism. - PluginFactory::open_with_provider_context binds a context; enqueue_release takes a pin before any counter moves and refuses outright once a context stops accepting work, leaving the allocation live for the canonical path. The pin lives in HostBridge, not AllocatorCore, because AllocatorCore::drop deliberately leaks that block precisely when releases are outstanding. - open() stays the standalone case: a mechanism with no provider behind it has no context to outlive. Verified by injection, not by a first green run. With pin acquisition disabled, teardown returns while a release is outstanding and a retiring context accepts the enqueue; both restored tests pass, and the two control tests stay green either way so the block is attributable to the pin. Not wired: no in-tree execution provider opens a plugin allocator yet, so nothing in production passes a pin source. Recorded in the ABI doc. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
74 of 100 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1725 +/- ##
==========================================
+ Coverage 80.03% 80.43% +0.39%
==========================================
Files 410 411 +1
Lines 198269 198336 +67
Branches 198269 198336 +67
==========================================
+ Hits 158685 159529 +844
+ Misses 34195 33413 -782
- Partials 5389 5394 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
…1762) **`main` does not compile in the CUDA lane, and my own #1725 is what broke it.** Flagging that first because it gates every PR currently open. ``` error[E0599]: no method named `device` found for struct `RegisteredMemoryContext` --> crates/onnx-runtime-ep-cuda/src/provider.rs:1163:30 error: could not compile `onnx-runtime-ep-cuda` (lib) exit=101 ``` First red at `a2cd1e7bf`; `a2a61656f` immediately before it is green. `git show ab0880b` (#1725) contains the removal. ## What happened #1725 rewrote the `use` list in `manager.rs` to pull in the pin-source types, and `pub fn device` was deleted along with it. Nothing in that PR required its removal — the underlying `RegisteredProviderContext::device` is untouched — so this restores the forwarder verbatim. ## Why nothing caught it, which is the part worth keeping Three independent things had to line up, and each is individually reasonable: 1. **`onnx-runtime-ep-cuda` has exactly one caller of this method, and it compiles in exactly one job.** Every lane that ran on #1725 was blind to it by construction. 2. **I verified #1725 with `cargo clippy --all-targets -- -D warnings` on the three crates I had edited.** That is a real check, and it is exactly the crate set that cannot observe this break. Editing crate A and verifying crate A is the natural thing to do and it is not sufficient when A is a library. 3. **A `pub` method with no in-crate caller draws no `dead_code` warning.** So deleting it was silent going out, and the breakage was silent coming back — until a job that most PRs never reach. This is the same shape as the `kv_commits_on_demand` finding in #1631/#1632: several individually-sound reasons not to look, converging. I had that lesson in hand and still shipped this, so the lesson evidently was not "run clippy" — it was **verify the reverse dependencies of a library change, not the library.** ## Verification — the lane's own command, not a proxy ``` cargo check -p onnx-runtime-ep-cuda -p onnx-runtime-python --features onnx-runtime-python/cuda main -> exit 101 (the E0599 above) fixed -> exit 0 ``` Negative control taken deliberately: I stashed the fix and re-ran on clean `main` to confirm the error returns, so this is demonstrably the cause rather than a change that merely happens to compile. Both results reproduced **locally on macOS aarch64 with no CUDA toolkit and no GPU.** That matters — this was catchable on any developer machine, which means it was not a CUDA-hardware coverage gap. Also clean: `cargo fmt --all -- --check`; `clippy -D warnings` on the governor; 67 + 34 + 5 governor tests; and the native-CUDA clippy lane from #1632. ## Not fixed here `Mobius metadata packages` is also red, with `The process '/usr/bin/git' failed with exit code 1` and no compile error. Different failure, unrelated to this change, and I have not diagnosed it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
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.
Closes the back half of #1186's Phase 6 criterion 8:
The module half was already enforced. The provider half was not, and
docs/memory/MEMORY_PLUGIN_ABI.mdsaid why in as many words: "No execution provider is wired to this ABI in this phase, so … has nothing to pin."Why this needs a structural gate
A stream-ordered free retires against whoever owns the device resources. If that owner finished teardown first, the plugin unmaps handles the teardown already released.
Nothing on the Rust side reports this.
host_release_completedtouches onlyHostBridge— a mutex, some atomics — and that block is deliberately built to outlive the allocator (AllocatorCore::dropmem::forgets it when releases are outstanding, so a plugin thread cannot write into freed memory). So the host stays quiet while the driver is handed a double free. There is nothing to observe after the fact, which is why the gate is on the way in rather than on the completion.What changed
memory-apigainsProviderContextPin/ProviderContextPinSource. Object-safe, becauseonnx-runtime-memory-hostcannot depend on the governor.ContextRecord::begin_transaction. A plugin release becomes one entry in the same transaction countremove_provider_contextalready waits on, rather than a second mechanism that could disagree with the first.PluginFactory::open_with_provider_contextbinds a context.enqueue_releasetakes a pin before any counter moves, and refuses outright once a context stops accepting work — the allocation stays live so the canonical path can still take it. Queuing unpinned would be the same defect with a narrower window.HostBridge, notAllocatorCore, for the same reasonmoduledoes:AllocatorCore::dropleaks that block precisely when releases are outstanding, so a pin dying with the allocator would stop guarding the exact window it exists for.open()is unchanged and stays the standalone case. A mechanism with no provider behind it has no context to outlive, so requiring one would be a regression rather than a tightening.Verification — injection, not a first green run
The path this guards was unreachable before, so any implementation goes green on the first try. With pin acquisition disabled (
&& false):Restored →
4 passed. The two control tests stay green in both runs, which is what makes the block attributable to the pin rather than towait_quiescentbeing slow in general:The blocking test also asserts teardown resumes after the drain. Asserting only that it blocks would pass for a deadlock — a pin that never releases reads as a successful pin.
Local gates:
cargo fmt --all -- --checkclean;clippy --all-targets -- -D warningson the three crates exit 0;memory-api/memory-governor/memory-host/ep-api/sessionall green, 0 failed.What this does not do
No in-tree execution provider opens a plugin allocator yet, so nothing in production passes a pin source today —
onnx-runtime-memory-hoststill has no dependents. The mechanism and its guarantees exist and are tested; adopting them from the CUDA EP is the remaining half ofmemory-plugin-provider-wiringand needs GPU verification I cannot do locally. Recorded in the ABI doc rather than claimed as done.#1186 should stay open until that adoption lands. Checkbox audit for the rest of the issue is in this comment (74/100 ticked).