[rwgk]

Description

Prevent a newly created graph branch from adopting an older graph's metadata when CUDA reuses a native handle address.

A node's native destruction can finish before Core retires its child-graph metadata. During that window, an independent graph can receive the same raw CUgraph address. The old registry lookup accepted that address without checking its parent or owner, and later cleanup could invalidate the new branch or erase its replacement registry entry. This is consistent with the Windows free-threaded CI failure, where a fresh conditional branch was already invalid before the test destroyed its owner; the exact address reuse was not captured in that run.

The root cause is structural. The process-wide graph_registry mapped a raw CUgraph to a weak_ptr that aliases the hierarchy's single control block, so a lockable entry proved only that the hierarchy was alive, not that the child graph was. CUDA destroys child graphs inside cuGraphDestroyNode, cuGraphNodeSetParams, and capture invalidation, and Core retires the box only after the driver call returns.

The fix removes the shared map instead of guarding it:

• Each GraphBox holds the registry of its direct child graphs, next to its existing node registry. create_child_graph_handle, the only reader, always has the parent handle, and roots are never looked up. A recycled address can no longer resolve to another hierarchy's metadata.
• Within a parent, entries are keyed by (CUgraph, owner node), the pair that identifies a child graph. If CUDA frees a child before Core retires its box, a newer child at the recycled address has a different owner node and its own entry, so the late retire cannot erase it.
• The static graph_registry, the hierarchy deleter's unregister loop, register_handles, and the conditional unregister_handle overload are gone. HandleRegistry is unchanged; the key is a struct with operator== and a hash functor, like ExportDataKey in memory.cpp.

Three deterministic Cython regressions exercise the real metadata API with synthetic child/owner keys and real empty parent graphs. The synthetic keys never reach CUDA. The tests cover foreign parents, different owners within one hierarchy, stale child retirement, and stale hierarchy cleanup. The Cython test builder supplies Core's source include path and the C++17 flag on Linux and Windows. The registry and hierarchy design notes describe the resulting invariants.

History

The first version of this PR (46bab1f) kept the process-wide registry and added a parent-and-owner check on lookup plus a compare-and-erase unregister_handle(key, expected). The design discussion in the comments led to the per-parent registry (974a7ad) and the pair key (9b5d126), which supersede that approach. The original description is in the edit history of this text.

Validation

• Unfixed main: all three regression cases fail at the foreign-generation assertions.
• The two revision commits: build, import, full test collection, the Cython test extension, and pre-commit locally; the graph and cython test directories on a GPU, 565 passed, 9 skipped, all four Cython cases passing.
• Hosted CI on 9b5d126: pending at the time of this edit.

This is one shared Core implementation for CUDA 12 and CUDA 13. The opaque CUgraph and CUgraphNode declarations in the CUDA 12 and CUDA 13 cydriver.pxd files already match, so neither bindings root needs a source or generation change. Callers still synchronize mutations within a shared graph hierarchy.

Suggested Review Order

1. cuda_core/cuda/core/_cpp/rt/graph.cpp: ChildGraphKey, the children registry on GraphBox, create_child_graph_handle, register_child_handles, and retire_graph.
2. cuda_core/cuda/core/_cpp/rt/internal.hpp: the removed overloads; HandleRegistry is back to its original interface.
3. cuda_core/tests/cython/test_graph_registry.pyx: deterministic reuse and cleanup cases.
4. The Cython test builder/module registration and the two runtime design notes (REGISTRY_DESIGN.md, GRAPH_ATTACHMENTS.md).

Checklist

• I have read the Contributing Guidelines.
• New and existing tests cover these changes.
• The documentation is up to date with these changes.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/cuda-python
• Branch: rwgk/fix-graph-handle-reuse
• File: .coderabbit.yaml
• Commit: 9b5d126
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: d8238462-6b84-43df-bde5-1d5c918b2ecf

Commits

Reviewing files that changed from the base of the PR and between 46bab1f and 9b5d126.

Files selected for processing: 5

• cuda_core/cuda/core/_cpp/rt/GRAPH_ATTACHMENTS.md
• cuda_core/cuda/core/_cpp/rt/REGISTRY_DESIGN.md
• cuda_core/cuda/core/_cpp/rt/graph.cpp
• cuda_core/cuda/core/_cpp/rt/internal.hpp
• cuda_core/tests/cython/test_graph_registry.pyx

Files with no reviewable changes: 1

• cuda_core/cuda/core/_cpp/rt/internal.hpp

Files skipped from review as they are similar to previous changes: 3

• cuda_core/cuda/core/_cpp/rt/GRAPH_ATTACHMENTS.md
• cuda_core/cuda/core/_cpp/rt/REGISTRY_DESIGN.md
• cuda_core/tests/cython/test_graph_registry.pyx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Improved graph handle reuse and cleanup to prevent recycled handles from being associated with stale metadata across graph hierarchies.
  • Preserved replacement mappings when an older graph hierarchy is invalidated.
• Documentation
  • Clarified how graph handle reuse and cleanup behave within graph hierarchies.
• Tests
  • Added coverage for handle reuse across parents and owners, invalidation, and hierarchy destruction.

Child graph handles are now registered and resolved within their immediate parent, using both the child graph resource and owner node as the key. The Cython test build adds the Core source root and C++17 flags. New tests cover handle reuse and hierarchy cleanup.

Changes: Child graph registry

Layer / File(s)	Summary
Parent-scoped registration and cleanup cuda_core/cuda/core/_cpp/rt/graph.cpp, cuda_core/cuda/core/_cpp/rt/internal.hpp, cuda_core/cuda/core/_cpp/rt/GRAPH_ATTACHMENTS.md, cuda_core/cuda/core/_cpp/rt/REGISTRY_DESIGN.md	Child graph lookup and registration use a key containing the child graph resource and owner node in the immediate parent’s registry. Publication and retirement use that registry. The documentation describes the per-GraphBox registries. HandleRegistry::register_handles is removed.
Cython registry tests and build cuda_core/tests/cython/test_graph_registry.pyx, cuda_core/tests/cython/build_tests.py, cuda_core/tests/cython/test_cython.py, cuda_core/tests/cython/test_get_cuda_native_handle.pyx	New tests cover handle reuse and separation by parent and owner, plus preservation of replacement mappings after hierarchy deletion. The test build adds the Core source root and C++17 flags. The native-handle test removes its local C++17 flag.

Possibly related issues

• NVIDIA/cuda-python#3156: The issue describes raw CUgraph handle reuse. This change scopes child metadata lookup and retirement to the parent and owner.
• NVIDIA/cuda-python#2438: The change registers cloned child handles with their immediate parents, but it does not add the public cloning API.

Change: Bug fix

Merge Risk: Minimal · up to 9b5d1

No merge-blocking regression was established in the child-graph registry changes.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Doc Preview CI
🚀 View preview at https://nvidia.github.io/cuda-python/pr-preview/pr-3294/
https://nvidia.github.io/cuda-python/pr-preview/pr-3294/cuda-core/
https://nvidia.github.io/cuda-python/pr-preview/pr-3294/cuda-bindings/
https://nvidia.github.io/cuda-python/pr-preview/pr-3294/cuda-pathfinder/
Preview will be ready when the GitHub Pages deployment is complete.

[Andy-Jost]

Thanks, Ralf. This is an important catch, and the diagnosis in the description is right. The registry's weak_ptr aliases the hierarchy's control block, so a lockable entry proves only that the hierarchy is alive, not that the child graph is. CUDA frees a child inside cuGraphDestroyNode or cuGraphNodeSetParams, and cuda.core retires the box only after the driver call returns. In that window, a process-wide map keyed by a raw CUgraph is an ABA hazard across hierarchies.

I prefer a slightly different fix that scopes the lookup map rather than a change to the HandleRegistry interface. The only reader of graph_registry is create_child_graph_handle, and every call has the parent handle. Roots are registered but never looked up. So the child registry can live in the parent GraphBox, next to node_handles, which already scopes node lookups to one graph:

• GraphBox gains a children registry.
• create_child_graph_handle looks up and registers through parent->children. The owner-node check can stay as a cheap guard.
• retire_graph unregisters from graph->parent->children.
• The clone and update publish paths register each new box into its parent.
• The static graph_registry, the deleter's unregister loop, register_handles, and the two-argument unregister_handle go away. HandleRegistry is unchanged.

A recycled address can then collide only inside one hierarchy. There, the documented external synchronization covers it, and every destroy and its retire happen inside one cuda.core call. Cross-hierarchy confusion, the case the CI failure hit, becomes impossible by construction rather than caught by a check.

Your tests 1 and 3 carry over unchanged; I traced both. Test 2 models two live boxes at one address under one parent, which the synchronization contract excludes, so it would go. The Cython test scaffolding is valuable either way.

If you prefer, I can make the change and update this PR.

[rwgk]

From the same codex session that produced this PR:

codex GPT-6.1-Sol ultra:

I agree with Andy and would prefer his design for #3294. He also posted a more detailed proposal on the PR, which I checked against the implementation.

His key observation is correct: the global graph registry has only one reader, and that reader already knows the parent graph. Roots are registered but never looked up. Putting a child registry on each parent GraphBox therefore fits the actual usage better.

That makes the fix simpler:

• Independent graph hierarchies cannot accidentally share metadata through a recycled address.
• Cleanup in an old hierarchy cannot erase another hierarchy’s entries.
• The new conditional unregister_handle overload and global cleanup loop become unnecessary.

I also agree about dropping test 2. It manufactures two active child generations at the same address under one parent. Supported usage requires mutations within a hierarchy to be externally synchronized; the implementation retires old metadata before publishing its replacement. That synthetic overlap should not dictate the design. Tests 1 and 3 still cover the cross-hierarchy problem and can remain unchanged.

The main implementation detail to check carefully is cloning and replacement: each nested child must register with its immediate parent, and retirement must still invalidate existing wrappers correctly.

I’d take Andy up on his offer to revise the PR, then validate the retained regressions, existing clone/replacement tests, and full CI.

[Andy-Jost]

I pushed the rework to this branch: two commits on top of 46bab1f.

974a7ad scopes the registry as discussed. GraphBox gains a children registry next to node_handles; create_child_graph_handle looks up and registers through the parent; retire_graph unregisters from its parent; the clone and update publish paths register each new box with its immediate parent. The static graph_registry, the hierarchy deleter's unregister loop, register_handles, and the two-argument unregister_handle are gone.

9b5d126 keys the per-parent registry by (CUgraph, owner node), the pair that identifies a child graph within its parent. The owner check becomes part of the lookup instead of a guard after it. This is defense in depth: if CUDA frees a child before cuda.core retires its box (the one such path we know of is an invalidated conditional-body capture, #2918), a newer child at the recycled address has a different owner node and therefore its own entry, so the late retire cannot erase it. HandleRegistry is still unchanged; the key is a struct with operator== and a hash functor, like ExportDataKey in memory.cpp.

That second commit is why test 2 is back, contrary to what we both said above. Under the pair key the scenario it models, two owners at one address under one parent, is exactly what the key protects, and the test passes unchanged. Tests 1 and 3 are unchanged. Both design notes are updated.

Validated locally: build, import, full collection, the Cython test extension, pre-commit, and the graph and cython test directories on a GPU (565 passed, 9 skipped; all four Cython cases pass). The PR description still describes the first approach; I can update it, or leave that to you.