Repository navigation
net-ib: report a failed connection setup instead of crashing on it - #10484
Merged
Merged
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves NetIB/NetIB-CAST failure reporting by ensuring connection-setup failures propagate out of gtest helpers (instead of continuing with an uninitialized connection) and by adding defensive argument validation in the transport plugins to avoid null-communicator crashes.
Changes:
- Wraps
SetupConnectionWithGuard/SetupCastConnectioncall sites withASSERT_NO_FATAL_FAILURE(...)so fatal assertions abort the test correctly. - Adds
comm == NULL(andmhandle == NULL) guards in NetIB and NetIB-CASTregMr/deregMrpaths to returnncclInvalidArgumentwith a warning instead of crashing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/rccl/test/transport/NetIbMPI/StressTests.cpp | Propagates connection-setup fatal assertions to stop tests cleanly. |
| projects/rccl/test/transport/NetIbMPI/NicFusionTests.cpp | Same assertion propagation for vNIC fusion test setup. |
| projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp | Same assertion propagation for general NetIB tests. |
| projects/rccl/test/transport/NetIbMPI/GdrFlushTests.cpp | Same assertion propagation for GDR flush test setup. |
| projects/rccl/test/transport/NetIbMPI/FaultInjectTests.cpp | Same assertion propagation for cast connection setup in fault-injection tests. |
| projects/rccl/test/transport/NetIbMPI/CastTests.cpp | Same assertion propagation for cast connection setup in cast tests. |
| projects/rccl/src/transport/net_ib/reg.cc | Adds null-argument guards in MR register/deregister to avoid null-comm crashes. |
| projects/rccl/src/transport/net_ib_cast/reg.cc | Adds null-argument guards in MR register/deregister to avoid null-comm crashes (cast). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (6)
projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:97
- This propagates a setup assertion only on the rank where it occurred.
SetupConnectioncan return before its finalMPI_Barrier(and a listen failure returns before rank 0 sends the handle), so the peer can remain blocked inMPI_Recv, polling, or the barrier indefinitely. The suite already avoids unilateral fatal assertions for this reason inNicFusionTests.cpp:464-475. Please make the setup protocol exchange/synchronize status so both ranks leave together before relying on this wrapper.
ASSERT_NO_FATAL_FAILURE(SetupConnectionWithGuard(0, pair, connGuard));
projects/rccl/test/transport/NetIbMPI/CastTests.cpp:34
ASSERT_NO_FATAL_FAILUREonly exits the local test.SetupCastConnectionhas unilateral fatal exits before its finalMPI_Barrier; for example, a rank-0 listen failure leaves rank 1 blocked inMPI_Recv, while a later one-sided failure can leave the peer in the barrier. Coordinate setup status between ranks and ensure both ranks take the same exit path, as the explicit pattern inNicFusionTests.cpp:464-475does.
ASSERT_NO_FATAL_FAILURE(SetupCastConnection(/*dev=*/0, &listenComm, &sendComm, &recvComm));
projects/rccl/src/transport/net_ib/reg.cc:77
- The new null-communicator behavior in both
regMrandderegMris not exercised by the existing registration negative tests, so a guard regression would restore the plugin crash unnoticed. Add directnetIbtests that call these operations with a null communicator and verifyncclInvalidArgumentwithout requiring a failed connection setup.
if (comm == NULL || mhandle == NULL) {
WARN("NET/IB: regMr called with comm=%p mhandle=%p", comm, (void*)mhandle);
return ncclInvalidArgument;
projects/rccl/src/transport/net_ib_cast/reg.cc:78
- The cast copy of the new null-communicator guards has no targeted coverage. Add
netIbCastnegative tests for null-comm registration and deregistration so this independently maintained implementation cannot regress to dereferencing the communicator.
if (comm == NULL || mhandle == NULL) {
WARN("NET/IB-CAST: regMr called with comm=%p mhandle=%p", comm, (void*)mhandle);
return ncclInvalidArgument;
projects/rccl/test/transport/NetIbMPI/StressTests.cpp:32
- If setup fails after creating a listen/recv/send object,
SetupConnectionWithGuardreturns from its internal fatal assertion before assigningpairintoguard. This outer assertion then exits the test with those partial connection objects unowned, and fixture finalization does not close them. Have the helper adopt every non-null partial object into the guard (or close it) before reporting the setup failure.
ASSERT_NO_FATAL_FAILURE(SetupConnectionWithGuard(/*dev=*/0, cp, guard));
projects/rccl/test/transport/NetIbMPI/FaultInjectTests.cpp:91
- A fatal setup result now returns from the test before its normal close calls, but
SetupCastConnectionmay already have createdlistenCommor another partial comm. Those raw pointers are not guarded, so failed tests leak plugin/RDMA resources into subsequent tests. Make the setup helper clean partial state on failure or transfer it immediately to an RAII guard.
ASSERT_NO_FATAL_FAILURE(SetupCastConnection(/*dev=*/0, &listenComm, &sendComm, &recvComm));
amd-arozanov
requested changes
Aug 21, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:868
- This accept loop is still unbounded when
AcceptConnectionrepeatedly returnsncclSuccesswith a null communicator, so the newMPI_Allreduceis never reached and the test can still hang on a dead fabric. Apply the samekConnectTimeoutMswatchdog used bySetupConnection.
This issue also appears on line 875 of the same file.
while (!pair.recvComm && r == ncclSuccess)
r = AcceptConnection(pair.listenComm, &pair.recvComm);
projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:876
- The connector has the same unbounded pending path: repeated successful calls returning a null communicator spin forever, preventing both ranks from reaching the failure reduction. Bound and pace this loop with the connection timeout, as
SetupConnectiondoes.
while (!pair.sendComm && r == ncclSuccess)
r = ConnectToRemote(mergedDev, &handshake.handle, &pair.sendComm);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:868
- These connect/accept APIs are asynchronous and may repeatedly return
ncclSuccesswith a null communicator. UnlikeSetupConnection, both loops here remain unbounded, so a stalled fabric can still keep one or both ranks from ever reaching the new reduction and the test hangs until the suite timeout. Apply the same polling deadline to both branches.
ncclResult_t r = ncclSuccess;
while (!pair.recvComm && r == ncclSuccess)
r = AcceptConnection(pair.listenComm, &pair.recvComm);
if (r != ncclSuccess || !pair.recvComm) connectFailed = 1;
projects/rccl/src/transport/net_ib/reg.cc:78
- The new null-communicator contract is not exercised by the existing registration tests:
RegisterMemoryNullPointeruses a live communicator with null data, andDeregisterNullHandlealso uses a live communicator. Since setup failures now stop the test body, the failure scenario will not reach these guards either. Add directregMr/regMrDmaBufand non-null-handlederegMrcalls with a null communicator, assertingncclInvalidArgument.
if (comm == NULL || mhandle == NULL || size == 0) {
WARN("NET/IB: regMr called with comm=%p mhandle=%p size=%zu", comm, (void*)mhandle, size);
return ncclInvalidArgument;
projects/rccl/src/transport/net_ib_cast/reg.cc:78
- The IB-CAST null-communicator paths are not covered by the current CAST tests; all registration calls use the live communicator created by
SetupCastConnection. Add direct null-communicator checks forregMr/regMrDmaBufand non-null-handlederegMr, assertingncclInvalidArgument, so this duplicated implementation cannot drift fromnet_ib.
if (comm == NULL || mhandle == NULL || size == 0) {
WARN("NET/IB-CAST: regMr called with comm=%p mhandle=%p size=%zu", comm, (void*)mhandle, size);
return ncclInvalidArgument;
Contributor
RCCL Perf-Regression Gate:
|
| group | keys | regressions | inconclusive |
|---|---|---|---|
| all_gather_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_gather_perf-d=float-default | 0 | 0 | 0 |
| all_reduce_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_reduce_perf-d=float-default | 0 | 0 | 0 |
| broadcast_perf-d=bfloat16-default | 0 | 0 | 0 |
| broadcast_perf-d=float-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=bfloat16-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=float-default | 0 | 0 | 0 |
amd-arozanov
requested changes
Aug 24, 2026
i-kosarev
requested review from
amd-arozanov
and
a balanced review from Copilot
August 24, 2026 16:39
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
projects/rccl/test/transport/NetIbMPI/CastTests.cpp:1288
- If connection
c > 0fails, this fatal wrapper returns from the test immediately.SetupCastConnectioncleans only the failing connection, while connections[0, c)remain stored in these vectors and the teardown loop at the end is skipped, leaking their listeners/QPs into subsequent tests. Install RAII cleanup for all entries before this loop (or explicitly clean previously established entries before propagating the fatal failure).
ASSERT_NO_FATAL_FAILURE(SetupCastConnection(/*dev=*/0, &listenComms[c], &sendComms[c], &recvComms[c]));
projects/rccl/src/transport/net_ib_cast/reg.cc:134
- The null-communicator guard is bypassed when
mhandleis also null because the existing no-op return runs first. ThusderegMr(nullptr, nullptr)still succeeds silently, contrary to the PR's stated contract thatderegMrrejects a null communicator; checkcommbefore applying the null-handle no-op (which still preserves success for a null handle on a valid communicator).
if (comm == NULL) {
WARN("NET/IB-CAST: deregMr called with a null comm");
return ncclInvalidArgument;
}
projects/rccl/src/transport/net_ib/reg.cc:134
- The null-communicator guard is bypassed when
mhandleis also null because the existing no-op return runs first. ThusderegMr(nullptr, nullptr)still succeeds silently, contrary to the PR's stated contract thatderegMrrejects a null communicator; checkcommbefore applying the null-handle no-op (which still preserves success for a null handle on a valid communicator).
if (comm == NULL) {
WARN("NET/IB: deregMr called with a null comm");
return ncclInvalidArgument;
}
amd-arozanov
approved these changes
Aug 24, 2026
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the new ASSERT_NO_FATAL_FAILURE at CastTests.cpp:1566 wraps a rank-local assertion, so a one-sided IB init failure strands the peer in SetupCastConnection's reduction. The CHANGELOG entry agreed last round is also missing from this head.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new readiness reduction can proceed after failed NetIB initialization because it only considers the device count.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Low-level MPI synchronization and RDMA resource cleanup require final human validation under real multi-node failure conditions.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
alexander-sannikov
approved these changes
Sep 30, 2026
…aBuf/deregMr A caller that lost its connection can reach these entry points with a null communicator. regMr/regMrDmaBuf read the device list off it before checking, and a zero size hits the same null-dereference fate through the page-count math on the way to a release-build assert; deregMr dereferences it directly. All three now return ncclInvalidArgument and log a WARN instead of segfaulting inside the plugin. deregMr keeps its existing null-handle-is-a-no-op short circuit ahead of the new check, so a null handle with a null comm is still ncclSuccess -- that shortcut predates this change and nothing here needed it to move.
… not one SetupConnection, SetupCastConnection and TeardownConnection each had the same shape: a fatal check or an early return on one rank while the peer waited in MPI_Recv or a barrier for a message that a failed rank never sends. Reachable from this branch's own subject -- a null-comm crash inside the plugin -- since before this, a torn-down connection reaching regMr with a null comm on one rank left the other rank blocked rather than reporting anything. SetupConnection and SetupCastConnection now send a status word instead of a bare handle, so a listen failure is reported rather than leaving the peer to interpret a handle nobody filled in. Both close whatever they created on a failure path, since callers now treat the result as recoverable rather than fatal-and-unwind. TeardownConnection downgrades its checks from ASSERT_ to EXPECT_ for the same reason: every rank-local check it makes precedes a barrier, and a fatal one would return from the helper while the peer waited there.
…eListen Same defect as SetupConnection, in a test that cannot use it: this loop builds its own listen/connect handshake rather than the shared helper, so it needed the same fix independently. A listen failure on rank 0 skipped sending the handle, leaving rank 1 in an unbounded MPI_Recv; accept/connect themselves spun without a timeout. The handle now travels with a status word so a listen failure is observable on both ranks, and the accept/connect loops are bounded by the same connect timeout SetupConnection uses. Close on the failure path releases whichever local comms did complete before the peer's did not, since FAIL() would otherwise skip the loop's normal close and leak them. The close-comm checks inside the iteration loop move from ASSERT_ to EXPECT_ for the same reason as TeardownConnection: a fatal exit here would skip the next iteration's handshake and strand the peer.
…AILURE SetupConnectionWithGuard asserts fatally on its own setup call, and a fatal assertion inside a helper returns from the helper alone, not from the caller. Every existing test called it bare, so a setup failure on one rank let that test body carry on into its own logic while its peer -- whose setup succeeded -- waited at whatever collective came next, the same unilateral-exit hang the rest of this branch removes from the shared helpers directly. Wrapping the call is what actually propagates the fatal failure out of the test body. Mechanical across both files: every call site follows the identical two-line shape (construct the guard, then call), so there is nothing here to choose between sites.
…SERT_NO_FATAL_FAILURE Same reasoning as the SetupConnectionWithGuard wrapping: SetupCastConnection now asserts fatally on its own setup, and that assertion returns from the helper rather than the caller, so a bare call let a one-sided setup failure carry the calling test into fault-injection logic while its peer waited elsewhere. Mechanical across all twenty call sites in this file.
…ss test, add a regression test for the null-comm guard Three related changes to this file, all downstream of SetupCastConnection now asserting fatally on its own setup instead of leaving a torn setup for the caller to notice. Every existing call site is wrapped in ASSERT_NO_FATAL_FAILURE -- mechanical, fifteen sites, same reasoning as the other files in this branch. CastStressMultiRoundTwoConns opens a hundred connections and registers memory on each; a setup, registration, or teardown failure partway through used to return with the rest still open and still registered, contaminating the rest of the process. The per-connection state (handles, mhandles, buffers) now lives behind a scope guard that closes and deregisters whatever is still held on any exit, dismissed only after the normal teardown loop succeeds; that teardown loop itself moved from ASSERT_ to EXPECT_ so a rank-local close failure cannot strand the peer at the closing barrier the way ListenCloseListen was rewritten to avoid. CastRegistrationRejectsBadArguments is new: it exercises the production guard this branch adds directly (null comm to regMr/regMrDmaBuf/deregMr, zero size to regMr), pinning that deregMr's pre-existing null-handle-is-a-no-op short circuit still runs ahead of the new comm check.
…usionTests Same defect, same fix, in the two files that had not been touched yet: SetupConnectionWithGuard and GdrFlushTests' own RunRecvFlushBurst both assert fatally on setup now, so a bare call let a one-sided failure carry the caller past its own peer's block. Mechanical across all call sites in both files.
…nfig that carries the Cast set All seven configs list the identical Cast test set, and the guard under test lives in net_ib_cast, so the AINIC and RoCE runs are where it matters most. Registered alongside its siblings in each.
…uisites are absent This test has no useful assertion without its two-rank, two-node launch shape. validateTestPrerequisites already reaches the same verdict on every rank, so SKIP_UNLESS_MPI_PREREQS preserves the other NetIbMPI tests' supported-launch behavior instead of failing the suite. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep failed deregistration and close handles owned by the multi-connection scope guard for a retry after the rank-agreed barrier. Wrap the setup calls develop added in the optional-recv and GID-change tests so a rank-agreed setup failure stops the test body before it can use null communicators. Co-authored-by: Cursor <cursoragent@cursor.com>
sdarisala
approved these changes
Oct 14, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
Motivation
A NetIbMPI test whose connection setup failed did not report that failure, it crashed, and the actual cause scrolled off the top of a page of backtraces. A failed network setup therefore looked like a dozen unrelated crashes.
There are two causes, one on each side of the plugin boundary, and either alone is enough to lose the message.
Technical Details
Three things, in the order a failure hits them.
The setup helpers fail unilaterally, and every early return strands the peer: a listen failure never sent the handle, so the connector waited in
MPI_Recvuntil the suite timeout, and a failure after that skipped the closing barrier, so the other side waited there instead.SetupConnectionandSetupCastConnectionnow agree before anyone gives up. The handle message carries a status word and is sent even when listen failed, so the connector always has something to read, and the closing barrier became anMPI_Allreduceof that status, so both ranks return the same verdict.SetupCastConnectionasserts after the reduction rather than before it, which is what makes its fatal assertion safe. The threaded path already worked this way --SetupConnectionForThreadexchanges a listener-ready flag for exactly this reason -- so this brings the serial helpers to the same contract.SetupConnectionWithGuard,SetupCastConnectionandSetupConnectionare void and assert internally, and a fatal gtest assertion returns from the helper rather than from the test, so the body carried on with a connection that was never established. All 76 call sites now wrap the call inASSERT_NO_FATAL_FAILURE, the gtest idiom for propagating that failure outward. Mechanical, one line each, no signatures changed.The body's next call is
RegisterMemory, andregMrreads the device list straight off the communicator, so a null comm faulted insidelibrccl: observed inncclIbRegMrDmaBufInternalandIbCastRegMrDmaBufInternal.regMr,regMrDmaBufandderegMrinnet_ib_castonly now reject a null communicator withncclInvalidArgumentand aWARN, andregMrrejects a zero size instead of relying on a release-build assert.The
net_ibcopy of that guard was in an earlier revision of this PR and was taken out again on review: the setup helpers this change is about all run overnet_ib_castin this suite, so nothing here exercised thenet_ibcopy. The same crash is reachable throughncclNetIb.regMr/regMrDmaBuf/deregMrand is worth its own change with its own coverage -- it is deliberately not in this one.The first change is what removes the hang, the second stops the body running on a dead connection, and the guards make the invalid argument observable for every caller rather than fatal. That last part is why this is not test-only.
Issue Tracking
JIRA ID: AICOMNET-323
Test Plan
rccl-UnitTestsMPIin a debug configuration with MPI tests enabled.net_ib_transport.jsonconfiguration and compare the non-passing set againstdevelop, since the point is that nothing changes except the shape of a failure.Test Result
Two ranks,
developat6c54d4074c:developproduces. The two non-passing entries are pre-existing:GdrFlush_ForcedScratchpadWrite_ReproducesFaultfails andNetIB_CtsDepthStresserrors.Submission Checklist