You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(cuda.core): serialize pinned-memory kernel-access test on Windows - #3436
Add @thread_unsafe_on_windows to the pinned-memory kernel-access test. The test accesses host memory while parallel workers may execute GPU work; synchronizing its own stream does not coordinate those workers.
This follows the guard used by similar tests on Windows and WSL. Windows CI is needed to confirm it resolves the reported crashes. The separate OOM failures remain under investigation.
closesNVIDIA#3426
Add @thread_unsafe_on_windows to the pinned-memory kernel-access test.
The test accesses host memory while parallel workers may execute GPU
work; synchronizing its own stream does not coordinate those workers.
This follows the guard used by similar tests on Windows and WSL.
Windows CI is needed to confirm it resolves the reported crashes.
The separate OOM failures remain under investigation.
Validation: Ruff and git diff --check pass.
jpascucci-nv
changed the title
fix(cuda.core): serialize pinned-memory kernel-access test on Windows
fix(cuda.core): serialize pinned-memory kernel-access test on Windows
Nov 10, 2026
jpascucci-nv
changed the title
fix(cuda.core): serialize pinned-memory kernel-access test on Windows
fix(cuda.core): serialize pinned-memory kernel-access test on Windows
Nov 10, 2026
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Updated cleanup behavior for host and NUMA memory pools to preserve peer access; other pool types and IPC-imported pools retain their existing cleanup behavior.
Tests
Updated Windows handling for a pinned-memory test and marked a memory-pool reset test as thread-unsafe.
Improved CUDA test cleanup so the current context is cleared after synchronization, including when synchronization raises an error. Added coverage for cleanup when the test wrapper has already removed the context.
The changes adjust peer-access cleanup for HOST and HOST_NUMA memory pools, update CUDA fixture teardown to synchronize through the bound device and attempt context cleanup, and add thread-safety markers to two memory tests.
The pool wrapper now accepts a flag that controls peer-access revocation. HOST and HOST_NUMA pools disable revocation during destruction. Other pool locations and IPC-imported pools retain revocation.
CUDA fixture teardown
Layer / File(s)
Summary
Synchronize and unset the current context cuda_core/tests/conftest.py, cuda_core/tests/memory/test_init_cuda_collect_gate.py
The teardown calls device.sync() and attempts to unset the current context in a finally block. Tests cover intact and already-popped fixture contexts, and verify that a synchronization CUDAError propagates while cleanup runs.
Memory test thread-safety guards
Layer / File(s)
Summary
Mark thread-sensitive memory tests cuda_core/tests/test_memory.py
The pinned-memory kernel-access test receives a Windows thread-safety decorator. The watermark-reset test receives a thread-unsafe marker for a legacy-default-stream mempool teardown and synchronization race.
Possibly related issues
NVIDIA/cuda-python#3426: The change adds the suggested Windows guard to the pinned-memory kernel-access test. The separate pinned-memory OOM failures remain outside this change.
NVIDIA/cuda-python#3414: The change affects the Windows execution of the pinned-memory kernel-access test, but does not add the broader kernel-test parametrization described by the issue.
NVIDIA/cuda-python#3350: The watermark-reset test receives a thread-unsafe marker for a related mempool teardown and synchronization race.
Suggested reviewers:mdboom
Change: Bug fix
Merge Risk: Low · previous review e2512
The change is mergeable with awareness that simultaneous fixture teardown failures can obscure the original test failure. The reported Windows crashes still need CI confirmation.
Security architecture risk: Low · up to e2512
Device-pool and imported-pool cleanup retain their existing controls. Host-pool destruction now skips device-access revocation on every platform, but the effect on access permissions after handle reuse remains unverified. No cross-process compromise is established.
Retained concerns
Low · security · inferred: Owned HOST and HOST_NUMA pools no longer revoke device access before destruction. Because creation grants the current device READWRITE access, a subsequent pool could inherit an unintended device grant if host handles exhibit the recycling behavior documented for device pools. The change applies without a platform guard. Host-specific inheritance remains unverified, so this is an isolation-contract concern rather than a demonstrated vulnerability.
Security review details
Security Blast Radius
inferred — The changed policy reaches newly created owned HOST and HOST_NUMA pools on all platforms using this creator, not just the marked Windows test. The supported concern is device-access state across local pool lifetimes. The evidence does not establish an attacker-controlled remote entry point, cross-tenant exposure, or automatic cross-process access.
Security Findings and Attack Paths
inferred — The conditional failure path is: a host pool grants device access, closes without revocation, and its handle is reused for a pool with different intended device access. An old device grant could then outlive its intended pool lifetime. Actual host-pool inheritance, GPU reachability of later allocations, and attacker control are not established; the existing reuse regression test exercises device pools only.
Trust Boundaries and Controls
observed — Device-created pools and IPC-imported pools retain revocation before destruction. IPC access also requires an exported pool capability and allocation descriptor. Existing device-memory IPC tests show that an importing process must establish its own peer grants; those tests do not resolve host-specific IPC behavior.
Resilience and Maintainability Implications
observed — Ownership remains shared-pointer managed, with destruction attempted on final owning-reference release. Creation and import failures still return before wrapping an owned handle. Explicit close and outstanding asynchronous-allocation handling are unchanged, so their unresolved driver semantics are not identified as new PR risks.
Hardening Proposals
proposed — Validate HOST and HOST_NUMA destruction and handle reuse with differing device grants, including driver support for revocation. Use the result to establish whether destruction alone preserves the access-reset invariant or an alternative cleanup control is required.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between bcfd4c4 and e251232.
Files selected for processing: 1
cuda_core/cuda/core/_cpp/rt/memory.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Check the cuCtxSynchronize result in init_cuda so asynchronous errors
surface during the triggering test's teardown instead of going unnoticed
until a later test. Always attempt context cleanup via finally.
Related to NVIDIA#3426.
Reviewing files that changed from the base of the PR and between aa83d4b and 730fcbe.
Files selected for processing: 1
cuda_core/tests/conftest.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Use device.sync() because the test wrapper may pop the current context
before init_cuda teardown. This avoids spurious INVALID_CONTEXT errors
while preserving synchronization error reporting and context cleanup.
Add regression coverage for a missing current context and cleanup after
synchronization failures.
Related to NVIDIA#3426.
Mark test_mempool_watermark_reset thread_unsafe, matching test_mempool_trim.
Both exercise the pool teardown and synchronization sequence associated
with the suspected driver race in NVIDIA#3350.
254-276: Trivial · Maintainability & Code Quality · Heavy lift
suggestion: Assert that fixture-context work completes
When the context is already popped, this test checks only context cleanup. A regression can leave work undrained and still pass. Queue controlled work in the fixture context and assert its completion after teardown.
Reviewing files that changed from the base of the PR and between 587c111 and bcfd4c4.
Files selected for processing: 1
cuda_core/tests/test_memory.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Skip peer-access revocation for locally created HOST and HOST_NUMA pools
to investigate pinned-pool teardown OOMs and crashes. Continue destroying
the pools and preserve cleanup for device, managed, and imported pools.
This is a diagnostic workaround pending Windows CI validation.
Related to NVIDIA#3426.
Keep setup or synchronization failures primary and attach subsequent
cleanup failures as exception notes. Always attempt to pop the context.
Add regression coverage for combined failures, including interrupts.
@jpascucci-nv@Andy-Jost — I'd like to get this merged before I rebase and run final CI for the #3322 stack tonight, starting around 5pm PST. Today's Pathfinder release CI only went green on the seventh attempt, and I'd really like to avoid repeating that while landing the stack.
An earlier codex's CI log review already found 88 direct pinned-memory kernel-test crashes across 39 runs, concentrated in three free-threaded Windows configurations. The latest candidate here passed all 21 Windows cuda.core jobs on their first attempt, including those three configurations. That's encouraging evidence for a substantial reduction in this crash family.
Codex also flagged a remaining scope issue here: the host-pool cleanup bypass also affects host-preferred managed-memory pools, beyond the stated pinned-memory scope. Since I already have the investigation and review context in a Codex session, I'll add follow-up fix commits here, in hopes that we can get the updated diff reviewed and merged this afternoon.
HOST and HOST_NUMA also describe managed-pool preferences. Check the allocation type so the pinned-memory workaround preserves managed-pool peer-access revocation.
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
test_pinned_memory_resource_kernel_access allocates through _allocate_pinned_buffer_or_xfail (cuda_core/tests/test_memory.py:1278), which turns CUDA_ERROR_OUT_OF_MEMORY into pytest.xfail. The out-of-memory reason checker added for #2381 only looks at failed reports, so those errors produce no diagnostics.
On Windows L4 (MCDM) with Python 3.14 and CUDA 13.5.0, kernel_access[current] ends in that xfail, and so do the two older tests that construct PinnedMemoryResource() without options: test_non_managed_resources_report_not_managed[pinned] and test_pinned_memory_resource_initialization. This happens in passing runs too. The push run at #3405's commit passed with all three recorded as xfail for CUDA_ERROR_OUT_OF_MEMORY, and in the last nightly before #3405, which also passed, the two older tests show the same xfail in all five repetitions. So allocating from the default host pool already ran out of memory on this configuration before #3405, and the TODO(#9999) xfail has been hiding it.
This matters for the PR in two ways. First, [current] stops at the allocation on this configuration, before the kernel launch, so only [new] and [new_numa] validate kernel access there. Second, the xfails at 49% on the PR's L4 job aren't what starts its cascade at 52%. They also occur in runs that pass, and in the first failing nightly after #3405 the first out-of-memory error came at 15%, before any of these tests ran. The checker's "first CUDA_ERROR_OUT_OF_MEMORY" label is wrong, but the error it reports is still the first one that isn't this known error.
Calling mr.allocate directly here would make the diagnostics worse. [current] would then fail on this configuration, and because the checker captures only once per session, it would spend that capture on this known error whenever this test runs before the cascade begins. The default-host-pool error needs its own issue, which the TODO(#9999) placeholder can then reference.
Edit: rewritten after checking more runs. The xfail is a pre-existing out-of-memory error, so I've withdrawn my suggestion to call mr.allocate directly.
juenglin
added a commit
to juenglin/cuda-python
that referenced
this pull request
Nov 12, 2026
…op redundant gh install from build-wheel.yml
test_pinned_memory_resource_kernel_access crashes under pytest-run-parallel
on free-threaded Windows builds (py3.14t, py3.15t) with a Windows fatal
access violation. The fix is in progress in NVIDIA#3436; skip on Windows until
it lands.
gh is pre-installed on all runners that build-wheel.yml uses (GitHub-hosted
Windows and NVIDIA self-hosted Linux/aarch64), so the install-gh-cli step
there was always a no-op. CI on the latest commit confirms this. Drop it.
test-wheel-linux.yml keeps its step because the ubuntu:24.04 container
does not ship gh.
and root-causing a second flake we hit here ("watermark/OOM cascade")
at the same time, before I got a chance to ask it to separate the concerns.
I'm holding off stopping it for the minute, to see how far it gets with what it started already.
This PR should be back on track now. The root-causing of the watermark/OOM cascade flakiness continues separately. Sorry for conflating those things accidentally.
@rwgk our updates crossed: I rewrote my comment above before I saw 8812d2b, and the rewrite withdraws the suggestion that commit implements. The L4 job at c4f114a13a5 shows why. With mr.allocate called directly, kernel_access[current] fails at 49% with the known default-host-pool error, and the checker spends its one capture there, with the verdict "inconclusive: all probes succeeded". The cascade still starts at 52%, in the teardown of test_mempool_watermark_reset[PinnedMR-used], and runs to an access violation at 93% without diagnostics.
9bdfbe3 moved the rest of 8812d2b out of this PR, but its test_memory.py line is still here. Could you revert that line too?
@jpascucci-nv@Andy-Jost are you still around, I asked codex if the PR title is still suitable, because the changes in cuda_core/cuda/core/_cpp/rt/memory.cpp look like a production code bug fix. Here is the response:
codex GPT-6.1-Sol ultra:
The title describes the original test change, but now understates the PR’s scope. My earlier framing was too focused on test mitigation.
The runtime change changes teardown for real applications: locally created pinned host/NUMA pools skip GPU-access revocation before attempting destruction. That applies on all platforms.
I’d suggest:
fix(cuda.core): avoid revoking access during pinned host pool teardown
The description should lead with that production workaround, then explain the test serialization and fixture improvements. “Workaround” reflects the evidence accurately while the underlying crash mechanism remains incompletely understood.
On the [current] round trip: my original suggestion was meant to expose what the xfail hid, in case it was the cascade's real first out-of-memory error. [current] fails the same way on six Windows jobs (WDDM and MCDM; RTX 4090, RTX 2080, A100, L4), those jobs finish with that one failure and no cascade, and where the checker could probe, every probe succeeded with about 93% of device memory free. So it's a separate default-host-pool problem, and restoring the xfail keeps the checker's one capture for the cascade.
Heads up: after consulting with @Andy-Jost, we decided to backtrack even more here, by backing out the production code changes. Most likely we'll still get a substantial reduction in flakiness. The other changes need more thought and/or root causing.
I just checked: @jpascucci-nv's test and fixture changes are unchanged after today's back and forth. Apart from merging main, the only net change is removal of the production workaround in memory.cpp, leaving this PR test-only.
The reason will be displayed to describe this comment to others. Learn more.
To alleviate the very substantial CI flakiness, I'm approving this based on intense reviewing with codex, and consultation with @Andy-Jost; offline Andy wrote:
The test changes seem safe enough.
All other changes were backed out, pending follow-on investigations.
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
bugSomething isn't workingCI/CDCI/CD infrastructurecuda.coreEverything related to the cuda.core moduleP1Medium priority - Should dotestImprovements or additions to tests
4 participants
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.
github-actions
Bot
added
the
cuda.core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
rwgk
left a comment
Description
related to #3426
Add @thread_unsafe_on_windows to the pinned-memory kernel-access test. The test accesses host memory while parallel workers may execute GPU work; synchronizing its own stream does not coordinate those workers.
This follows the guard used by similar tests on Windows and WSL. Windows CI is needed to confirm it resolves the reported crashes. The separate OOM failures remain under investigation.
Validation: Ruff and git diff --check pass.
Checklist