Repository navigation
[RCCL] bootstrap socket accept fixes - #14450
Merged
alex-breslow-amd merged 3 commits intoNov 5, 2026
Merged
alex-breslow-amd merged 3 commits into
alex-breslow-amd merged 3 commits into
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
3 more finding(s), not posted inline
projects/rccl/test/host/bootstrap-test.cc:519(Low) Comment at line 519 describes a BadHandshake-only skip contract that bootstrapAccept no longer has; reword it to match the Ready loop. The comment at line 519 says bootstrapAccept was written to skip a peer returned asBadHandshake, but bootstrap.cc:446 now loops on any state until it isReady, so could we reword it to match?projects/rccl/src/transport/net_ib/connect.cc:1173(Low) Follow-up: the connect-side fail labels in ncclIbConnect and IbCastConnect free comm without closing comm->base.sock, the same leak this PR fixes on the accept side. Follow-up, not for this PR: the connect-side fail label also leakscomm->base.sock. Line 1173 and net_ib_cast/connect.cc:1749 only callfree(comm)afterncclSocketConnectopened the fd, so a later QP or dev-base failure leaks it. The samencclSocketClosewould fix it.projects/rccl/test/host/bootstrap-test.cc:685(Nit) The test comment says 'bad-cookie' while the test name and line 539 say 'wrong/bad magic'; use one term.
Bottom line: Worth settling before approval: the rcclNetP2pPolicy cookie fix at net_ib/connect.cc:2038 has no test, so reverting it fails nothing.
Comment @argus review to re-run, or @argus re-review after pushing.
Alkaid-Benetnash
force-pushed
the
gzuo/rccl-socket-accept-fixes
branch
from
October 28, 2026 10:16
fcf8680 to
ee21522
Compare
Contributor
pr-rocm-systems-14450-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14450-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
wenkaidu
approved these changes
Nov 2, 2026
Contributor
|
manually trigger CoCo workflow: https://github.com/ROCm/rocm-systems/actions/runs/41724155315 |
Contributor
pr-rocm-systems-14450-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14450-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
|
@Alkaid-Benetnash I can't trigger on this PR yet -- your association here reads as |
`ncclSocketAccept(..., retry=false)` can return success with the socket still in `Accepting`, including after a bad-cookie peer is rejected. `bootstrapAccept` treated that state as a usable connection. Its caller then failed to receive the bootstrap payload, and a stray peer could end the rendezvous root before valid ranks checked in. Initialize the socket once and continue accept progress until `Ready`. Keep the existing abort check and retry delay. The socket layer owns rejected-peer closure; this does not change its rejection-state contract. Replace two known-bug assertions with readiness/payload and root check-in regressions, using the existing in-memory socket test seams. Test: Build `hipify_all`, then `rccl-UnitTestsMicroBootstrap`. Before this commit (at `1ba70482989b76acd424d7f7745a3a74a216dfc5`): - `Accept_RejectedPeerReportedAsAccepting_RetriesUntilReady`: exit 1. Expected `ncclSocketStateReady` (7), got `sock.state` (2). Expected `ncclSuccess` (0), got `socketRecv(...)` (3). `[ FAILED ] 1 test`. - `Root_StrayPeerWithTheWrongMagicDoesNotPreventCheckIn`: exit 1. `fake socket wait timed out`; valid rank received no ring answer. Expected answer handle 1, got 0. `[ FAILED ] 1 test`. - Existing abort, BadHandshake recovery, and valid-root controls: `[ PASSED ] 3 tests`. With the fix, run the microbootstrap binary: `[==========] 76 tests from 4 test suites ran.` `[ PASSED ] 76 tests.` Both changed tests report `[ OK ]`; exit 0.
The NetIb and IB-CAST accept failure paths free the receive communication object without closing its socket. An accepted TCP descriptor can remain open after an accept or later setup failure. Close the receive socket before freeing its owner. Initialize the socket immediately after allocation, before fallible receive initialization, so early failures see invalid descriptors and initialized socket state. Receive initialization does not overwrite these socket fields. Keep the returned error, listener-stage reset, and pending/success ownership intact. This is limited to existing failure-label cleanup; it does not add general RDMA resource cleanup or alter paths that return directly. Test: Regenerate `hipify_all`, then compile both transport objects: `make -C projects/rccl/build -f src/CMakeFiles/rccl.dir/build.make src/CMakeFiles/rccl.dir/__/hipify/src/transport/net_ib/connect_tmp.cc.o src/CMakeFiles/rccl.dir/__/hipify/src/transport/net_ib_cast/connect_tmp.cc.o -j2` Both objects compile successfully (exit 0). Warnings are in existing headers and the toolchain's unused `--offload-compress` option. Ran `rccl-UnitTestsMicroBootstrap`: `[==========] 76 tests from 4 test suites ran.` `[ PASSED ] 76 tests.` Exit 0. This checks the retained bootstrap port; it does not exercise these transport cleanup paths.
`ncclIbListen` writes `ncclSocketDefaultMagic()` into the connection handle. When `NCCL_SOCKET_MAGIC` overrides the built-in value, `rcclNetP2pPolicy` rejects that valid handle because it compares against the built-in constant. Use the same configured-cookie accessor for validation as for handle creation. Keep null-handle rejection and the P2P flag update unchanged. IB-CAST's cookie policy is not changed. Test: Review the producer `ncclIbListen`, validator `rcclNetP2pPolicy`, and `ncclSocketDefaultMagic`: producer and validator use the same process-cached cookie, including the environment override and built-in fallback. `make -C projects/rccl/build -f src/CMakeFiles/rccl.dir/build.make src/CMakeFiles/rccl.dir/__/hipify/src/transport/net_ib/connect_tmp.cc.o` compiles successfully (exit 0).
Alkaid-Benetnash
force-pushed
the
gzuo/rccl-socket-accept-fixes
branch
from
November 5, 2026 01:05
ee21522 to
f9f77e8
Compare
Contributor
Contributor
pr-rocm-systems-14450-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14450-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-14450-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-14450-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14450-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-14450-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-14450-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
amd-jiali
approved these changes
Nov 5, 2026
1 task done
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
Fix three socket-handling defects in RCCL:
NCCL_SOCKET_MAGICoverrides the built-in cookie.Technical Details
Bootstrap readiness
In
projects/rccl/src/bootstrap.cc,bootstrapAccepttreats every state exceptBadHandshakeas success. However,ncclSocketAccept(..., retry=false)can returnncclSuccesswith the socket still inAccepting, including after rejecting a bad-cookie peer. Accept progress success does not guarantee that the socket is ready for a payload receive.Initialize the socket once and continue accept progress until its state is
Ready. Retain the abort check and retry delay. The socket layer closes rejected peers; this change does not alter its rejection-state contract.Update two existing bootstrap tests to check readiness and payload delivery, and successful rank check-in after a rejected peer. Reuse the existing host-test fixture and in-memory socket seams.
Receive-socket ownership on accept failure
In
projects/rccl/src/transport/net_ib/connect.ccandprojects/rccl/src/transport/net_ib_cast/connect.cc, the accept failure labels callfree(rComm)without closingrComm->base.sock. The receive comm is allocated withncclIbMalloc, and its socket holds a raw descriptor.freereleases the allocation; it does not invoke a destructor or close the descriptor.Call
ncclSocketClosebefore freeing the receive comm. Move socket initialization immediately after allocation, before fallible receive initialization, so early failures have initialized socket state and invalid descriptors. Receive initialization preserves these socket fields.Keep the returned error, listener-stage reset, and pending/success ownership unchanged. This change covers the existing failure labels; it does not add general RDMA resource cleanup or change direct-return paths.
Configured-cookie consistency
ncclIbListenwritesncclSocketDefaultMagic()into the connection handle.rcclNetP2pPolicyinstead compares that handle with the built-inNCCL_SOCKET_MAGICconstant.Use
ncclSocketDefaultMagic()in the validator so handle creation and validation use the same process-cached cookie. Preserve null-handle rejection and the P2P flag update. Do not change IB-CAST's cookie policy.Issue Tracking
No linked issue.
Test Plan
Bootstrap regression checks
Use the same updated tests against the original bootstrap implementation at
1ba70482989b76acd424d7f7745a3a74a216dfc5and against the patched implementation.Build and run from the repository root with configured native RCCL and host-test build directories:
Also run the existing abort-before-accept,
BadHandshakerecovery, and valid-root arrival-order tests as controls against the original implementation.Transport compilation and source review
Regenerate
hipify_alland compile both changed transport objects with the native production build flags:Review
ncclSocketInit, LinuxncclSocketClose, and both receive initializers for socket initialization, ownership, and cleanup. ReviewncclIbListen,rcclNetP2pPolicy, andncclSocketDefaultMagicfor consistent cookie use.No dedicated transport or cookie tests are added. The bootstrap suite does not exercise transport cleanup or P2P cookie validation. A full library build and GPU/RDMA runtime validation were not run.
Test Result
Original bootstrap implementation
Both updated regression tests fail for the intended reason, each with exit code 1:
Accept_RejectedPeerReportedAsAccepting_RetriesUntilReady: expectedReady(7), receivedAccepting(2). Expected payload receive success (0), received an error (3).[ FAILED ] 1 test.Root_StrayPeerWithTheWrongMagicDoesNotPreventCheckIn: the valid rank receives no ring answer after the rejected peer. The fake socket wait times out; expected answer handle 1, received 0.[ FAILED ] 1 test.The three existing controls pass:
[ PASSED ] 3 tests.Patched implementation
The complete bootstrap host suite passes with exit code 0, including both updated regressions:
Both changed transport objects compile successfully. The NetIb object also compiles successfully after the cookie change. Compiler warnings are in existing headers and the unused
--offload-compressoption.Submission Checklist