Repository navigation
Conversation
Host-only microtests for src/bootstrap.cc in a new rccl-UnitTestsMicroBootstrap binary. Ranks run as threads over an in-memory socket layer and net plugin defined in the test, so the root rendezvous, the socket and net rings in both directions, split, grow, P2P send/recv and the async encrypted sends run for real. Known production bugs are pinned as observed, not fixed. Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Member
Author
|
@argus review |
aduforest
changed the title
![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
5 more finding(s), not posted inline
projects/rccl/test/host/bootstrap-test.cc:1667(Low) Split_EachSetupFailureIsReturned: fault lambda's loop-local std::mapcallsis also reached from the detached root threadprojects/rccl/test/host/bootstrap-test.cc:1743(Low) Grow_NetBidir sets a 300 ms watchdog_ that also applies to the bidir=0 runs expected to succeedprojects/rccl/test/host/bootstrap-test.cc:1963(Low) FailFirstOfTwoSends waits on gateCv_ and asyncSendCond with no deadline, so a regression hangs the binaryprojects/rccl/test/host/bootstrap-test.cc:1210(Low) Pinned production-bug tests carry no tracking ticket, so whoever fixes the bug sees an unexplained test failureprojects/rccl/test/host/bootstrap-test.cc:189(Low) g_rejectedState default copies socket.cc's accept behaviour, so the #13368 pinned tests won't flip when socket.cc is fixed
This PR changes 2,094 lines across 8 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: Worth settling before approval: Barrier_EveryRankLeavesAfterAllArrive only checks return codes, so a barrier that returns without waiting still passes.
Comment @argus review to re-run, or @argus re-review after pushing.
Hold one rank back in the barrier test so an early exit fails it, keep fault hooks out of reach of the root thread, bound the async-send waits, give the grow test's succeeding runs the full watchdog, and say in each pinned-bug comment what a fix flips instead of citing PR history. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Addressed in 3e01295. On the non-inline findings:
@argus re-review |
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,116 lines across 8 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: Worth settling before approval: Root_RankGoneBeforeItsAnswer_MakesTheRootGiveUp and IntraNodeAllGather_EncryptedConnectsOnAHelperThread still pass with the behaviour they name removed.
Comment @argus review to re-run, or @argus re-review after pushing.
Assert the root leaves rank 1 unanswered once its send to rank 0 fails, and that the encrypted intra-node connects run on a thread other than the rank's. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,137 lines across 8 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: Worth settling before approval: connectThreads.size() at bootstrap-test.cc:1944 can come up short when helper thread ids are reused, so the encrypted allgather test can flake in pre-checkin.
Comment @argus review to re-run, or @argus re-review after pushing.
Compare each gathered proxy and P2P address with the listener the owning rank created, require distinct UDS ids, and mark rank threads with a thread_local so the helper-thread check does not depend on thread ids. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,144 lines across 8 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 once BcastGrowHandle_CoordinatorReachesOnlyTheBoundaryRanks asserts nothing is pending on rank 2's peerSocket.
Comment @argus review to re-run, or @argus re-review after pushing.
Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,146 lines across 8 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.
aduforest
marked this pull request as ready for review
October 16, 2026 00:19
…comrccl-2796-bootstrap-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/host/run_host_tests.sh # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,146 lines across 8 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.
- NetMultiOp_RejectsEachMalformedOpBeforePostingAny counts isend/irecv on a real loopback comm instead of g_netConns, which no op could change. - Split_EachSetupFailureIsReturned closes each iteration's parent and child states (Split overwrote parents_, leaking them) and checks no listener is left. - TearDown resets the seams; RAS_ENABLE goes through g_params; the stagger test uses a real delay and checks it. - Pinned production bugs are named <Fn>_KnownBug_<What> with one-line comments. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,150 lines across 8 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.
…comrccl-2796-bootstrap-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,194 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.
pvallem
reviewed
Oct 24, 2026
…comrccl-2796-bootstrap-host-microtests
Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,187 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.
Host CI runs the binary through run_host_tests.sh. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
pr-rocm-systems-13601-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13601-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13601-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,170 lines across 9 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.
Contributor
pr-rocm-systems-13601-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13601-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13601-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13601-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13601-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13601-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13601-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13601-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
pvallem
approved these changes
Oct 25, 2026
pvallem
pushed a commit
that referenced
this pull request
Oct 25, 2026
…GIN (#13840) JIRA ID: AICOMRCCL-2813 ## What Host-only microtests for `src/transport/net_ib_cast/{init,common,capability,reg,gdr,multiplane,gin,net_ib_cast_gid_inspect}.cc`, in a new `rccl-UnitTestsMicroNetIbCastInit` with one test TU per unit. ## How - The verbs, mlx5/ionic, socket and neighbouring net_ib_cast seams are shared fakes. The ones the other net_ib_cast PRs also use are byte-identical copies; this PR owns the scheduler, p2p_resiliency_recovery, gdr_peermem and gdr_probe fakes. Fake ibv contexts route post_send/poll_cq back to the test. `xml.cc`, `utils.cc` and `net_telemetry.cc` are the real ones. The GDR answers latched by `std::call_once` are tested in isolated processes. - `transport_stubs.cc` gains net.cc's `NET_OPTIONAL_RECV_COMPLETION` param, and `os_fakes.cc` gains `ncclOsStrSep`, the same hunk as #13601. - Pins 10 production bugs as `KnownBug_` tests: - AINIC fuses NICs by default, although the code means to require `NCCL_IB_MERGE_NICS=1` (AICOMRCCL-3088). - A GDR probe that could not run is latched as "no GDR" (AICOMRCCL-3089). - A data-direct lookup error skips the cleanup (AICOMRCCL-3090). - A failed device init turns the next one into a silent no-op (AICOMRCCL-3091). - Deregistering unsorts the MR cache, so a cached range gets registered twice (AICOMRCCL-3092). - A regMr failure on a later device keeps the earlier devices' MRs (AICOMRCCL-3093). - GIN connect, context and registration failures leak comms and MRs (AICOMRCCL-3094). - The RMA proxy's error report indexes devices by completion slot, and can crash (AICOMRCCL-3095). - IPutSignal with data but no destination posts an empty WR instead of the signal (AICOMRCCL-3096). - A plane-index failure frees the ibv device list twice (AICOMRCCL-3143). - Not pinned, as the outcome depends on uninitialized memory: `IbCastMakeVDevice` compares and logs PCI roots it never set when a path has none (AICOMRCCL-3097). ## Coverage Lines: init.cc 642/652, common.cc 185/185, common_cast.h 57/57, capability.cc 162/162, reg.cc 116/117, gdr.cc 73/73, multiplane.cc 186/189, gin.cc 491/491 (the GDAKI arm is compiled out, as in the shipping build), net_ib_cast_gid_inspect.cc 102/102. The misses are calloc failures, a capability probe that cannot fail, a telemetry slot that is never out of range, and the return after reg.cc's loop. ## Mutation Testing 172 mutations on the hipified sources, all killed; 5 of them needed a sharper test first. 1 more against the review changes, also killed. 11 more for #12749's runtime uDMA count and its mlx5-only data-direct checks, all killed. ## Test Plan Standalone build: `rccl-UnitTestsMicroNetIbCastInit` 275/275 in about 0.4 s, and `--gtest_shuffle --gtest_repeat=20` is clean. Every other host binary was rebuilt against the changed fakes and run once. DdaColl and Rocmwrap fail as they do on develop (the break #13819 fixes) and link none of the changed files. --------- Co-authored-by: Claude <noreply@anthropic.com>
…comrccl-2796-bootstrap-host-microtests # Conflicts: # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
Member
Author
|
@argus review |
Contributor
pr-rocm-systems-13601-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13601-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 2,165 lines across 7 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.
aduforest
deleted the
users/aduforest/aicomrccl-2796-bootstrap-host-microtests
branch
October 26, 2026 09:56
Contributor
TheRock Submodule Bump ActivityNewest first
|
1 task done
i-kosarev
added a commit
that referenced
this pull request
Oct 29, 2026
The accept and root pins added in #13601 recorded the bug this branch fixes. A peer rejected in the Accepting state is now skipped and the next connection accepted, and the root keeps accepting after a wrong-magic peer and answers a valid check-in. Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
added a commit
that referenced
this pull request
Oct 30, 2026
The accept and root pins added in #13601 recorded the bug this branch fixes. A peer rejected in the Accepting state is now skipped and the next connection accepted, and the root keeps accepting after a wrong-magic peer and answers a valid check-in. Co-authored-by: Cursor <cursoragent@cursor.com>
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.
JIRA ID: AICOMRCCL-2796
What
Host-only microtests for
src/bootstrap.ccin a newrccl-UnitTestsMicroBootstrapbinary: root rendezvous, socket and net rings,bootstrapSplit, P2P with parked connections, collectives, async encrypted sends,NCCL_OOB_NET_IFNAMEand the abort/failure paths.How
#includes the hipifiedbootstrap.cc. Ranks run as threads over an in-memory socket layer and net plugin infakes/bootstrap_micro_fakes.cc, so the real protocol runs end to end; faults are injected per socket/plugin call.fakes/os_fakes.cc(plusncclOsSetFilesLimitandncclOsStrSep),fakes/env_fakes.cc,fakes/hip_fakes.ccand the realmisc/utils.cc. Own binary becausebootstrap_stubs.ccfakes this unit for the others.netGetDevicecaches 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):
bootstrapCreateRootleaks the listen fd whenncclSocketGetAddrfails.bootstrapInitandbootstrapSplitleak the proxy listen fd on any failure after the proxy listen.bootstrapClosewith parked connections and no abort leavesunexpectedConnectionspointing at the freed list, so a second close frees it again.NCCL_OOB_NET_ENABLE=1+NCCL_BOOTSTRAP_BIDIR_NET=1hangs until abort (the reverse ring never connects).ncclSocketAccept(retry=false)reports a rejected peer as Accepting, sobootstrapAcceptreturns an unconnected socket and one stray connection makes the root exit. Open fix(rccl): stop TLS session tickets from breaking bootstrap #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 < Dtorank <= DinrootIdFromRank) is equivalent.Test Plan
76 tests pass in about 2 s in the standalone build, and
--gtest_shuffle --gtest_repeat=20is clean.