Repository navigation
fix(memory): settle prepared releases on CUDA device loss - #1349
justinchuby wants to merge 1 commit into
Conversation
The CUDA deferred release queue's three device-loss retention sites moved the whole unexecuted action into `RetainedOwnership.keep_alive`. For a `PreparedReleaseAction` that froze a live `PreparedAllocationRelease`: its request was never executed, never quarantined, and never dropped, so the binding never recorded the allocation as retained, `queued_releases` and `active_operations` never settled, `confirm_context_terminated` and `remove` stayed impossible, and a permanent cycle formed — queue -> retained record -> request -> binding -> mechanism -> provider context -> context pin -> queue. `DeferredReleaseAction` gains a consuming `settle_device_lost` hook. The default keeps retaining the whole action, which is right for an action whose ownership is purely physical (a weight page's allocator/allowance, a reservation ticket). `PreparedReleaseAction` overrides it: it consumes its request through the existing device-loss settlement path — no allocator call, no refund — and hands back only the pinned allocator, the same residual the normal quarantine path retains and the one piece that does not pin the provider context. All three sites (already-lost enqueue/poll, concurrent-loss carry, retain_all_pending) now share one helper. `PreparedAllocationRelease::quarantine_device_lost` exposes the settlement `execute` already performs behind a device-lost release gate. It is needed because the queue can learn the context is unusable before the mechanism lifecycle is invalidated, and `execute` would then still be permitted to call the allocator. The new portable tests use the production path — real `enqueue_prepared` with a host-backed `MemoryBinding` and a provider-context pin shaped like the CUDA one — and assert zero allocator releases, device-lost quarantine against the exact allocation identity, zero queued releases and active operations, and that the queue's strong count returns to one after the documented context teardown. Refs #1341 Part of #1186 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Final independent review: approved; #1341 device-loss rejection resolved. Verified all three loss sites consume through one settlement hook; prepared releases record exact DeviceLost quarantine without allocator/device calls; queued/active pins settle; observer refunds zero; residual allocator ownership breaks the queue→binding→context cycle while retaining physical safety; races/outstanding/fence retention remain balanced; and production |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## justinchuby-1186-memory-deferred-release-phase-4 #1349 +/- ##
====================================================================================
- Coverage 79.53% 79.52% -0.01%
====================================================================================
Files 365 365
Lines 162318 162327 +9
Branches 162318 162327 +9
====================================================================================
- Hits 129100 129096 -4
- Misses 28501 28513 +12
- Partials 4717 4718 +1
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 |
Corrective PR for the confirmed high defect in #1341. Targets
justinchuby-1186-memory-deferred-release-phase-4at4d2b2cc5. Part of #1186.Do not merge this or any memory PR automatically.
The defect
CudaDeferredReleaseQueuehas three device-loss retention sites — already-lost enqueue/poll, the concurrent-loss carry path, andretain_all_pending. All three moved the whole unexecuted action intoRetainedOwnership.keep_alive.For a
PreparedReleaseActionthat froze a livePreparedAllocationReleasewithrequest: Some(..). It was never executed, never quarantined, and never dropped, so:quarantinedentry at all;queued_releasesandactive_operationsnever settled;confirm_context_terminatedfailed onContextNotQuiescent, andremovestayed impossible;Arccycle formed:queue.retained→ prepared request →MemoryBinding→MechanismEntry→ProviderContextEntry→CudaProviderContextPin→queue.The queue kept itself, its CUDA context, and its streams alive forever after any device loss with work in flight.
The fix
DeferredReleaseActiongains a consumingsettle_device_losthook.WeightPageRelease,ReservationTeardownAction, and the synthetic test actions.PreparedReleaseActionoverrides it: consumes its request through the existing device-loss settlement path (no allocator call, no refund, no device touch), then hands back only the pinnedArc<dyn DeviceAllocator>— the same residual the normal quarantine path retains, and the one piece that does not pin the provider context. The observer still sees the real terminal outcome; it refunds only the reported unmapped bytes, which are zero, and counts no free.All three sites now go through one
settle_lost_entryhelper, so they cannot drift apart again.PreparedAllocationRelease::quarantine_device_lost(memory-api) exposes the settlementexecutealready performs behind a device-lost release gate. It is needed because the queue can learn the context is unusable before the mechanism lifecycle is invalidated, andexecutewould then still be permitted to call the allocator. It is deliberately distinct fromquarantine(QuarantineReason::DeviceLost), which records the genericQuarantinedstate rather than theDeviceLostterminal state that confirmed context termination discharges.After settlement
binding.quarantinedholds exactly one record: the exactAllocationIdentity,state: DeviceLost,reason: DeviceLost, exact bytes and retained bytes.queued_releases == 0andactive_operations == 0.== 0.invalidate_device→confirm_context_terminated→remove→remove_provider_contextall succeed, per the documented quarantine discharge.Arc::strong_count(&queue) == 1after that teardown — the cycle is gone.Tests
Three new production-path tests in
crates/onnx-runtime-ep-cuda/tests/deferred_release_queue.rs. They use realenqueue_preparedwith a realMemoryBindingover a host-backed allocator and a provider-context resource shaped exactly likeCudaProviderContextPin, notCountingReleaseor a toy action:device_loss_settles_a_pending_prepared_release_and_frees_the_contextretain_all_pending(the reported pending path)device_loss_settles_a_prepared_release_a_poller_was_holdingconcurrent_loss_settles_every_prepared_release_whichever_path_takes_itThe third site cannot be forced deterministically without adding a test hook to production code:
polltakes the execution gate before its per-entry device-loss check, andmark_device_lostneeds that same gate, so any in-loop trigger would deadlock. It is covered by the racing test, which asserts the same invariants for every request regardless of which site settled it, and all three sites share one helper.Controlled revert: restoring the pre-fix behaviour (override replaced by the default) makes all three new tests fail on
the binding records exactly one retained allocation: left 0, right 1. With the fix, all 20 tests in the file pass.Validation
cargo test -p onnx-runtime-ep-cudacargo test --test deferred_release_queuecargo test -p onnx-runtime-memory-api -p onnx-runtime-memory-governorcargo fmt(scoped)deferred_release.rs/deferred_release_queue.rs-D warnings, changed filesFailures compared against the exact #1341 base
4d2b2cc5:cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warningsfails at base and at head with the same two errors in the untouchedonnx-runtime-ep-cpu(manual RangeInclusive::contains,collapsible_if).RUSTDOCFLAGS=-D warnings cargo docfails at base and at head on untouched items (shareability.rsKvLayout,weight_paging.rs,cudnn, and thedeferred_release.rsline-29 module header this PR does not touch).verify_cuda_test_honesty.pyfails identically at base and head for the same three portable files; onlydeferred_release_queue's count changes 17 → 20.No new failure class is introduced.
Scope
deferred_release.rs(CUDA),deferred_release_queue.rs(tests), and one additive method in memory-apideferred.rs. No rearchitecture, no Phase-5 work, no change to the CUDA partial-release state machine or its accounting, no unrelated cleanup.cargo fmt --allalso wanted to reformatonnx-genai-server/src/routes/completions.rs; that pre-existing deviation was reverted out of this diff.Head
9ac2b41b, 3 files changed, +502 / −68.