Skip to content

fix(memory): preserve Phase 3 binding teardown and selection - #1283

Closed
justinchuby wants to merge 1 commit into
justinchuby-1186-memory-binding-phase3from
justinchuby-fix-phase3-binding-races
Closed

justinchuby wants to merge 1 commit into
justinchuby-1186-memory-binding-phase3from
justinchuby-fix-phase3-binding-races

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Summary

  • drop shared-prefix resources before their binding/context pin
  • restore a healthy prior mechanism when a raced selection candidate becomes inactive
  • document Phase-3 drop-driven teardown and the mapped-attribution adoption boundary

Part of #1186; fixes rejected Phase-3 revision #1279

Validation

  • new drop-order and failed-selection regressions pass in isolation; each failed under a controlled test-only inversion/revert (exit 101)
  • cargo test -p onnx-runtime-memory-api: 25 passed
  • cargo test -p onnx-runtime-memory-governor: 63 passed
  • cargo test -p onnx-runtime-ep-cpu --test shared_allocator: 8 passed
  • cargo test -p onnx-runtime-ep-cuda --lib: 382 passed, 22 ignored
  • cargo test -p onnx-runtime-cuda-memory --lib: 8 passed
  • cargo test -p onnx-genai-ort governed_allocator: 20 passed
  • real registry stress: 20/20 runs, 4,800 iterations
  • scoped Clippy and strict memory-API rustdoc pass
  • workspace format, all-target check, and governor rustdoc reproduce exact-base inherited failures only

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.36%. Comparing base (31a8f28) to head (5f1b249).

Files with missing lines Patch % Lines
crates/onnx-runtime-memory-api/src/binding.rs 92.18% 2 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                            Coverage Diff                             @@
##           justinchuby-1186-memory-binding-phase3    #1283      +/-   ##
==========================================================================
+ Coverage                                   79.87%   80.36%   +0.49%     
==========================================================================
  Files                                         363      363              
  Lines                                      159604   159665      +61     
  Branches                                   159604   159665      +61     
==========================================================================
+ Hits                                       127483   128315     +832     
+ Misses                                      27426    26649     -777     
- Partials                                     4695     4701       +6     
Flag Coverage Δ
mlas 85.32% <ø> (+0.26%) ⬆️
offline 80.26% <92.18%> (+0.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/onnx-runtime-memory-api/src/binding.rs 64.04% <92.18%> (+2.44%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

🔴 Benchmark Regression Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
🔴 matmul/medium_generic_f32_threads=8/32x512x512 1.14 ms 1.79 ms +57.2%
🔴 block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 553.88 µs 860.96 µs +55.4%
🔴 matmul/medium_generic_bf16_threads=8/32x512x512 379.47 µs 553.94 µs +46.0%
🔴 matmul/medium_generic_f16_threads=1/32x512x512 32.64 µs 47.00 µs +44.0%
⚠️ gather/large_f32_threads=1-internal/131072 22.63 µs 28.61 µs +26.5%
⚠️ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 5.28 ms 6.41 ms +21.3%
⚠️ add/large_bf16_threads=1-internal/4194304 1.59 ms 1.84 ms +16.0%
✅ sampling_latency/greedy_per_token 2.97 µs 3.22 µs +8.4%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 525.85 µs 569.23 µs +8.2%
✅ add/small_f16_threads=1-internal/1024 418.9 ns 443.7 ns +5.9%
✅ kv_cache/alloc_dealloc_pages 36.21 µs 37.87 µs +4.6%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 1.96 ms 2.04 ms +4.4%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.52 ms 2.60 ms +2.8%
✅ sampling_latency/top_k_per_token 48.60 µs 49.64 µs +2.2%
✅ tokenization/decode_tokens_per_second 5.89 ms 6.00 ms +2.0%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 2.09 ms 2.13 ms +1.9%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 94.47 µs 95.94 µs +1.6%
✅ add/medium_f16_threads=1-internal/262144 97.09 µs 98.45 µs +1.4%
✅ sampling_latency/top_p_per_token 352.65 µs 357.55 µs +1.4%
✅ gather/medium_f32_threads=1-internal/32768 3.46 µs 3.50 µs +1.2%
✅ grammar_masking/llguidance_compute_mask/32 69.99 µs 70.44 µs +0.6%
✅ logit_processing/seven_processor_chain_per_step 297.67 µs 299.56 µs +0.6%
✅ qwen3_sampling_processors/top_k_partial_selection 130.75 µs 131.20 µs +0.3%
✅ gather/small_f32_threads=1-internal/4096 619.5 ns 621.3 ns +0.3%
✅ matmul/medium_generic_f16_threads=8/32x512x512 38.26 µs 38.30 µs +0.1%
✅ add/large_f16_threads=1-internal/4194304 1.56 ms 1.56 ms -0.2%
✅ qwen3_sampling_processors/top_k_top_p_fast 611.31 µs 609.88 µs -0.2%
✅ reduce_mean/medium_f32_threads=1-internal/65536 228.48 µs 227.54 µs -0.4%
✅ gather/small_f16_threads=1-internal/4096 444.5 ns 441.9 ns -0.6%
✅ add/small_bf16_threads=1-internal/1024 415.7 ns 412.0 ns -0.9%
✅ matmul/small_generic_f32_threads=1/1x256x256 34.16 µs 33.85 µs -0.9%
✅ add/medium_bf16_threads=1-internal/262144 97.00 µs 95.48 µs -1.6%
✅ reduce_mean/small_f32_threads=1-internal/4096 14.32 µs 13.99 µs -2.3%
✅ add/medium_f32_threads=1-internal/262144 22.85 µs 22.28 µs -2.5%
✅ matmul/small_generic_f32_threads=8/1x256x256 34.65 µs 33.76 µs -2.6%
✅ sampling_latency/min_p_per_token 202.57 µs 197.14 µs -2.7%
✅ reduce_mean/large_f32_threads=1-internal/262144 942.88 µs 909.82 µs -3.5%
✅ tokenization/encode_tokens_per_second 364.51 µs 349.57 µs -4.1%
✅ gather/medium_f16_threads=1-internal/32768 2.31 µs 2.21 µs -4.2%
✅ gather/small_bf16_threads=1-internal/4096 460.0 ns 439.8 ns -4.4%
✅ add/large_f32_threads=1-internal/4194304 624.78 µs 590.84 µs -5.4%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 10.44 ms 9.82 ms -6.0%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.54 ms 3.29 ms -7.2%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 86.21 µs 78.87 µs -8.5%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 1.74 ms 1.56 ms -10.6%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 546.51 µs 483.75 µs -11.5%
✅ gather/medium_bf16_threads=1-internal/32768 2.56 µs 2.21 µs -13.8%
✅ add/small_f32_threads=1-internal/1024 209.1 ns 179.7 ns -14.1%
🟢 matmul/large_generic_f32_threads=8/32x1024x1024 4.94 ms 3.82 ms -22.6%
🟢 matmul/small_generic_f16_threads=1/1x256x256 37.24 µs 28.38 µs -23.8%
🟢 block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 89.50 µs 61.66 µs -31.1%
🟢 matmul/small_generic_bf16_threads=1/1x256x256 48.23 µs 33.23 µs -31.1%
🟢 gather/large_f16_threads=1-internal/131072 13.88 µs 9.38 µs -32.4%
🟢 gather/large_bf16_threads=1-internal/131072 15.09 µs 10.00 µs -33.7%
🟢 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 108.88 µs 71.21 µs -34.6%
🟢 matmul/small_generic_bf16_threads=8/1x256x256 46.99 µs 30.03 µs -36.1%
🟢 matmul/small_generic_f16_threads=8/1x256x256 45.55 µs 28.40 µs -37.6%
🟢 block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 700.58 µs 365.62 µs -47.8%
🟢 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 128.83 µs 46.28 µs -64.1%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 2.65 3.02 4.52 }
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)

justinchuby added a commit that referenced this pull request Aug 22, 2026
> **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
@justinchuby

Copy link
Copy Markdown
Owner Author

Closing: superseded by #1579, which collapsed phases 1–7 into a single merge — landed as a36964280 (squash, 126 files, +43956 −3286).

This PR was approved by a session that was not its author, and its work is in main.

Containment verified, not assumed:

$ git merge-base --is-ancestor 5f1b249e2 ebceab071   # ebceab071 = #1579 head
→ ancestor

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 main: of the 126 files #1579 touched, exactly one differs from main — crates/onnx-runtime-session/src/executor/tests.rs, where main carries 120 extra lines from #1703 (Expand-broadcast mask tests). That is the conflict resolution correctly preserving the other side, not content loss.

Why this didn't close itself: #1579's body says Closes the stack: #1252 #1263 …, which GitHub does not parse — the closing keyword must be immediately followed by the reference, and in any case closing keywords only ever close issues, never pull requests. So all 15 stayed open by mechanism, not by intent.

#1186 stays open: #1579 explicitly gates it on memory-plugin-provider-wiring (the back half of Phase 6 criterion 8, provider/context pinning), which has not landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant