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(rccl): stop TLS session tickets from breaking bootstrap - #13368
With socket encryption on (ncclSetEncryption, shipped in 10.2.0), communicator creation fails intermittently on 2-rank communicators, including one process driving 2 GPUs, at default settings, and every time with NCCL_BOOTSTRAP_BIDIR_ALLGATHER=0.
Technical Details
src/misc/crypt.cc: the TLS server sends no session tickets. Bootstrap clients that only write (bootstrapSend, the unidirectional ring, and every ring step at 2 ranks) never read them, and closing a socket with unread data sends RST and drops the bytes still queued. From 3 ranks the default ring starts with a duplex step that reads them.
Tests: SocketCrypt.* (loopback socket encryption), BootstrapTlsMPITest.*, and BootstrapStrayPeerMPITest.* (regression for the root-exit fixed in [RCCL] bootstrap socket accept fixes #14450), plus MPI categories running them under both AllGather directions.
tools/scripts/test_runner/configs/mi300x_mellanox_ib.json: runs these suites, the MPI ones on 2 ranks with default settings and on 4 ranks with each AllGather direction. Without -DTLS_BACKEND=OPENSSL3 the encrypted cases skip.
Issue Tracking
JIRA ID: AICOMRCCL-2488
Test Plan
New tests on builds with and without -DTLS_BACKEND=OPENSSL3, and against a library without the fix. rccl-tests with encryption at default settings on 2 to 8 ranks, and multi-node under both AllGather directions.
Test Result
Test
Without fix
With fix
SocketCrypt.WriteOnlyClientCloseKeepsQueuedData
FAIL 8/8
PASS 8/8
rccl-tests, encryption, 2 processes x 1 GPU, default settings
2/10
10/10
rccl-tests, encryption, 1 process x 2 GPUs, default settings
0/5
5/5
rccl-tests, encryption, 4 or 8 ranks, default settings
Ensure ncclGroupEnd runs when send or receive fails
projects/rccl/test/BootstrapTlsMPITests.cpp:78
If ncclSend or ncclRecv fails, short-circuit evaluation skips ncclGroupEnd(), leaving the thread-local group open while the buffers are freed. The caller uses EXPECT_TRUE and continues, so later collective, split, and cleanup calls can be deferred inside that unfinished group. Always call ncclGroupEnd() after a successful ncclGroupStart(), including when an enqueue fails.
Do not treat ncclInProgress as a dropped socket connection
projects/rccl/test/SocketCryptTests.cpp:161
With blocking=false, ncclSocketTryRecv returns ncclInProgress when a connected peer has no data available (src/misc/socket.cc:716). This condition treats that idle, still-open connection as dropped, so TlsListenerDropsPlaintextPeer can pass without observing a disconnect. Continue polling on ncclInProgress; report a drop only on closure or an actual error.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/CHANGELOG.md:52 (Low) CHANGELOG says any non-NCCL connection (port scanner) is dropped, but a peer that connects and stays silent still stalls the bootstrap root
projects/rccl/CHANGELOG.md:53 (Low) Session-ticket entry under Resolved issues fixes ncclSetEncryption, which this same unreleased section introduces under Added
projects/rccl/test/BootstrapTlsMPITests.cpp:59 (Low) Nested or multi-line control-statement bodies without braces in the new test files, which nccl_coding_style.md requires to be braced
projects/rccl/test/BootstrapTlsMPITests.cpp:33 (Low) kTlsBuild block, SetPsk, TearDown and TLS skip are duplicated across the three new test files (~30 lines)
projects/rccl/src/misc/crypt.cc:371 (Nit) WARN for SSL_CTX_set_num_tickets repeats the line 359 text, so the two failures are indistinguishable in logs
Bottom line: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
Bottom line: Worth settling before approval: WriteOnlyClientCloseKeepsQueuedData, the only regression test for the session-ticket fix, skips unless RCCL is built with TLS_BACKEND=OPENSSL3, and no build config in projects/rccl turns that on.
Comment @argus review to re-run, or @argus re-review after pushing.
JIRA ID: AICOMRCCL-2796
## What
Host-only microtests for `src/bootstrap.cc` in a new
`rccl-UnitTestsMicroBootstrap` binary: root rendezvous, socket and net
rings, `bootstrapSplit`, P2P with parked connections, collectives, async
encrypted sends, `NCCL_OOB_NET_IFNAME` and the abort/failure paths.
## How
- The test `#include`s the hipified `bootstrap.cc`. Ranks run as threads
over an in-memory socket layer and net plugin in
`fakes/bootstrap_micro_fakes.cc`, so the real protocol runs end to end;
faults are injected per socket/plugin call.
- Reuses `fakes/os_fakes.cc` (plus `ncclOsSetFilesLimit` and
`ncclOsStrSep`), `fakes/env_fakes.cc`, `fakes/hip_fakes.cc` and the real
`misc/utils.cc`. Own binary because `bootstrap_stubs.cc` fakes this unit
for the others.
- `netGetDevice` caches its device in a function-local static, so those
cases are threadsafe death tests.
## Pinned bugs
Pinned as KnownBug_ tests, not fixed (AICOMRCCL-3002..3004 for the first
four):
- `bootstrapCreateRoot` leaks the listen fd when `ncclSocketGetAddr`
fails.
- `bootstrapInit` and `bootstrapSplit` leak the proxy listen fd on any
failure after the proxy listen.
- `bootstrapClose` with parked connections and no abort leaves
`unexpectedConnections` pointing at the freed list, so a second close
frees it again.
- A grow under `NCCL_OOB_NET_ENABLE=1` + `NCCL_BOOTSTRAP_BIDIR_NET=1`
hangs until abort (the reverse ring never connects).
- `ncclSocketAccept(retry=false)` reports a rejected peer as Accepting,
so `bootstrapAccept` returns an unconnected socket and one stray
connection makes the root exit. Open #13368 fixes this one.
## Coverage
`bootstrap.cc`: 1476/1480 lines, 70/70 functions; the 4 missed lines are
dead code.
## Mutation Testing
46 targeted mutations, 45 killed; the survivor (`rank < D` to `rank <=
D` in `rootIdFromRank`) is equivalent.
## Test Plan
76 tests pass in about 2 s in the standalone build, and `--gtest_shuffle
--gtest_repeat=20` is clean.
---------
Co-authored-by: Claude <noreply@anthropic.com>
i-kosarev
changed the title
fix(rccl): stop TLS session tickets and stray peers from breaking bootstrap
fix(rccl): stop TLS session tickets from breaking bootstrap
Nov 6, 2026
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,050 lines across 10 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
BootstrapStrayPeerMPITest checks that a connection with the wrong socket
magic, or a plaintext peer on an encrypted root, does not end the
bootstrap root before the ranks check in.
OpenSSL's TLS 1.3 server sends session tickets after the handshake by
default. NCCL derives every session from the pre-shared key and never
resumes one, and a client that only writes (bootstrapSend, the
unidirectional bootstrap ring) never reads the tickets. Closing a TCP
socket with unread data sends RST and discards the bytes still queued
for sending, so the peer lost the tail of large messages: with
encryption on, NCCL_BOOTSTRAP_BIDIR_ALLGATHER=0 failed communicator
creation every time, and every encrypted init closed ~100 sockets with
RST. The server context now sends no tickets.
Co-authored-by: Cursor <cursoragent@cursor.com>
SocketCrypt.* (rccl-UnitTestsFixturesDebug, loopback) pins the
ncclSetEncryption contract and, on TLS_BACKEND=OPENSSL3 builds, the
socket paths RCCL's bootstrap drives differently from upstream: the
single-thread ring connect of socketRingConnectStart/Finish, full-duplex
ncclSocketMultiOp on one pair, a wire tap that must not see the hello or
payload in plaintext, mixed-mode and wrong-key rejection, and a
write-only client that closes with data still queued. Without TLS
support a configured key must fail instead of falling back to plaintext.
BootstrapTlsMPITest.* runs init, AllReduce, send/recv and split over
encrypted sockets, and enabling encryption between communicators. The
quick_env_bootstrap_tls_{unidir,bidir} categories run them under both
bootstrap AllGather directions.
Co-authored-by: Cursor <cursoragent@cursor.com>
ncclGetUniqueId and the stray-peer connect ran under rank-0-only ASSERTs, so
a setup failure returned rank 0 while the other ranks waited in MPI_Bcast
until the job timed out. Broadcast the setup result and assert on all ranks.
Co-authored-by: Cursor <cursoragent@cursor.com>
…l disconnect
RingSendRecv skipped ncclGroupEnd when ncclSend or ncclRecv failed, leaving
the group open for later calls. PeerDropped treated ncclInProgress (connected,
no data yet) as a dropped peer, so TlsListenerDropsPlaintextPeer could pass
without seeing the disconnect.
Co-authored-by: Cursor <cursoragent@cursor.com>
…notes
kTlsBuild, SetPsk and ClearPsk move to test/common/EncryptionTestHelpers.hpp
for the three TLS test files. Multi-line and nested control bodies get braces.
The stray-peer test header states the invariant. The num_tickets failure gets
its own WARN text. CHANGELOG: the stray-peer entry is limited to connections
that fail the handshake, and the session-ticket entry is dropped because
ncclSetEncryption itself is new in this release.
Co-authored-by: Cursor <cursoragent@cursor.com>
bootstrapAccept no longer closes a rejected socket that the reset already
closed. The num_tickets comment carries the RCCL: prefix so syncs keep it.
The MPI TLS tests use mpiAllRanks, and PlaintextListenerDropsTlsPeer checks
that the accept only ended through the abort.
Co-authored-by: Cursor <cursoragent@cursor.com>
Builds the OpenSSL 3 TLS backend with the MPI tests and runs SocketCrypt.*,
BootstrapTlsMPITest.* and BootstrapStrayPeerMPITest.* on one node, with the
bidirectional and the unidirectional bootstrap ring AllGather.
Co-authored-by: Cursor <cursoragent@cursor.com>
bootstrapBidirEnabled() is false below 3 ranks, so the 2-rank
bidirectional suite took the unidirectional ring AllGather.
Co-authored-by: Cursor <cursoragent@cursor.com>
…json
Moves SocketCrypt.*, BootstrapTlsMPITest.* and BootstrapStrayPeerMPITest.*
into the MI300X plan, with 4-rank bidirectional and unidirectional ring
AllGather suites, and drops the separate bootstrap_tls.json.
Co-authored-by: Cursor <cursoragent@cursor.com>
With 2 ranks every bootstrap ring AllGather step is one-way, so this is
the default-settings case that hits the TLS session-ticket reset; from 3
ranks the first step is a duplex exchange that reads the tickets.
Co-authored-by: Cursor <cursoragent@cursor.com>
ncclSetEncryption shipped in 10.2.0, so the reset on write-only
bootstrap clients is a user-visible fix.
Co-authored-by: Cursor <cursoragent@cursor.com>
pr-13368_rccl-perf_rocm-10.1-ga vs rocm-10.1-ga_rccl-develop-perf
Readings: flat 2070.
Fails the run: 0 regressed point(s) past 8%, each confirmed by an adjacent size.
No confirmed regressions or improvements.
This branch has not been deployed
No deployments
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
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.
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Motivation
With socket encryption on (
ncclSetEncryption, shipped in 10.2.0), communicator creation fails intermittently on 2-rank communicators, including one process driving 2 GPUs, at default settings, and every time withNCCL_BOOTSTRAP_BIDIR_ALLGATHER=0.Technical Details
src/misc/crypt.cc: the TLS server sends no session tickets. Bootstrap clients that only write (bootstrapSend, the unidirectional ring, and every ring step at 2 ranks) never read them, and closing a socket with unread data sends RST and drops the bytes still queued. From 3 ranks the default ring starts with a duplex step that reads them.SocketCrypt.*(loopback socket encryption),BootstrapTlsMPITest.*, andBootstrapStrayPeerMPITest.*(regression for the root-exit fixed in [RCCL] bootstrap socket accept fixes #14450), plus MPI categories running them under both AllGather directions.tools/scripts/test_runner/configs/mi300x_mellanox_ib.json: runs these suites, the MPI ones on 2 ranks with default settings and on 4 ranks with each AllGather direction. Without-DTLS_BACKEND=OPENSSL3the encrypted cases skip.Issue Tracking
JIRA ID: AICOMRCCL-2488
Test Plan
New tests on builds with and without
-DTLS_BACKEND=OPENSSL3, and against a library without the fix. rccl-tests with encryption at default settings on 2 to 8 ranks, and multi-node under both AllGather directions.Test Result
SocketCrypt.WriteOnlyClientCloseKeepsQueuedDataNCCL_BOOTSTRAP_BIDIR_ALLGATHER=0Through the coco harness, on 1 and 2 nodes with both AllGather directions:
Submission Checklist