Repository navigation
Conversation
New rccl-UnitTestsMicroLinux runs the real os/linux.cc, socket.cc and the plaintext crypt.cc handshake on loopback TCP, socketpairs, temp files and /dev/shm. Libc calls that cannot fail or vary for real go through hooks that default to the real call. Co-Authored-By: Claude <noreply@anthropic.com>
…x_socket_pair.cc Real abstract and path-bound Unix datagram sockets pass descriptors between two handles; the socket pair runs on real pipes. Only socket/sendmsg/recvmsg and pipe are seamed, to fail or to forge a foreign control message. Co-Authored-By: Claude <noreply@anthropic.com>
shmutils runs over the real os/linux.cc shared memory, with local ranks played by threads for ncclShmemAllgather. signals seams only signal(); the handler's exit path runs in a death test. 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 |
CI's gtest predates GTEST_FLAG_SET. Co-Authored-By: Claude <noreply@anthropic.com>
![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/CMakeLists.txt:1084(Low) With BUILD_BFD=ON, signals.cc compiles its libbfd backtrace path into rccl-UnitTestsMicroLinux, which does not link bfdprojects/rccl/test/host/linux-ipcsocket-test.cc:173(Low) SendFd_PassesADescriptorOverAPathSocket leaves /tmp/nccl-socket-{0,1}-<hash> behind on every runprojects/rccl/test/host/linux-test.cc:1773(Low) LastAttachUnlinkFailureIsOnlyLogged leaves a /dev/shm/nccl-XXXXXX segment behind on every runprojects/rccl/test/host/socket-test.cc:996(Low) ASSERT before join of a std::thread terminates the whole binary on failure (socket-test.cc and linux-test.cc, 8 sites)projects/rccl/test/host/shmutils-test.cc:97(Low) Allgather_NullShmemCrashes pins undefined behaviour (NULL deref before the check) in a death test, so the outcome depends on the compiler
This PR changes 3,910 lines across 13 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 crypt is added to the GENERATED list at CMakeLists.txt:877 and Move_TransfersOwnershipAndInvalidatesTheSource gets the same #ifndef NCCL_TLS_BACKEND_OPENSSL3 guard as its siblings.
Comment @argus review to re-run, or @argus re-review after pushing.
Mark the hipified crypt.cc GENERATED for in-build configures, keep the fake TLS pointer out of TLS-backend builds, build signals.cc without libbfd, clean up path sockets and a /dev/shm segment the tests left behind, join threads before any fatal assertion, and drop the death test that depended on undefined behaviour. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
The five non-inline findings are addressed in c7de8b2:
202/202 and 20x shuffled pass. |
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 3,916 lines across 13 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 15, 2026 22:22
…comrccl-2819-os-socket-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 3,916 lines across 13 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: ResetFd_Dup2FailureClosesTheNewSocket assumes fd 1 << 29 is above RLIMIT_NOFILE (linux-test.cc:720), which does not hold where the container soft limit is 1073741816.
Comment @argus review to re-run, or @argus re-review after pushing.
The fixed 1 << 29 target is below the soft limit in containers that run with LimitNOFILE=infinity, so dup2 could succeed there. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 3,925 lines across 13 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.
…Size default Split each pinned bug into its own <Fn>_KnownBug_ test, rename the Close and Initialize tests to say what they check, pin init.cc's SetCpuStackSize default with rccl_assert_source_line, make the ResetFd probe exact, free the handle in CloseReportsAHipUnregisterFailure, and give the real own-binary reason (fakes/os_fakes.cc defines the ncclOs* symbols). Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 3,942 lines across 13 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-2819-os-socket-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md
Member
Author
|
@argus review |
…comrccl-2819-os-socket-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # 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 3,969 lines across 13 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-13587-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13587-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13587-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13587-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13587-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13587-oci-rccl-pr-smokeBuilds
Failures
3 tests blocked and 15 tests skipped. |
Contributor
pr-rocm-systems-13587-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pvallem
reviewed
Oct 25, 2026
…comrccl-2819-os-socket-host-microtests # Conflicts: # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
…y per review develop already lists the binary in that loop. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
pr-rocm-systems-13587-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13587-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 4,041 lines across 22 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-2819-os-socket-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md
…comrccl-2819-os-socket-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/host/fakes/libc_fakes.cc # projects/rccl/test/host/fakes/libc_fakes.h # projects/rccl/test/host/fakes/libc_seam.h # projects/rccl/test/host/fakes/libc_seam_undef.h
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 4,017 lines across 22 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-2819-os-socket-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 4,014 lines across 22 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-microtests Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
|
@aduforest the new head ( |
…host-microtests Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 4,014 lines across 22 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
approved these changes
Nov 6, 2026
…uforest/aicomrccl-2819-os-socket-host-microtests
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 3,999 lines across 20 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.
This branch has not been deployed
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-2819
What
Host-only microtests for
src/os/linux.cc,os/linux_ipcsocket.cc,os/linux_socket_pair.cc,misc/socket.cc,misc/shmutils.ccandmisc/signals.cc(plus theinclude/socket.hsubnet helper), in a newrccl-UnitTestsMicroLinux.include/cpuset.his left to #13579.How
/dev/shmanddlopen, with the realos/linux.cc,socket.ccand plaintextcrypt.cchandshake linked together.fakes/libc_seam.h, andlibc_fakesgains the OS calls these units make (poll,getifaddrs,mmap,signal, ...) plusUseRealLibc(); logging, params, env and HIP come fromnccl_fakes(newg_ncclDebugLogseam),param_redirect.h,env_fakesandhip_fakes(new host-registration seams).fakes/linux_fakes.cconly holds init.cc'sSetCpuStackSizeparam. Hooks go in withScopedHook.fakes/os_fakes.cc, which most targets link, defines thencclOs*symbolsos/linux.ccdefines.KnownBug_tests:ncclOsShmOpenleaves the created file behind when it fails after creating it (AICOMRCCL-2974).ncclIpcSocketClosetreats descriptor 0 as unopened and never closes it (AICOMRCCL-2976).ncclSocketGetAddrFromStringturns a missing IPv4 port into 65535 (AICOMRCCL-2977).Coverage
Lines:
linux.cc696/699,socket.cc570/582,linux_ipcsocket.cc168/175,linux_socket_pair.cc32/32,shmutils.cc43/43,signals.cc26/26 (theHAVE_BFDbacktrace is compiled out),socket.h3/3; the misses aren't reachable from the API.Mutation Testing
105 mutations, 104 killed (12 by a hang or crash); 1 equivalent (
i != comm->localRankinncclShmemAllgather).Test Plan
Standalone build:
rccl-UnitTestsMicroLinux205/205 in about 0.1 s,--gtest_shuffle --gtest_repeat=20clean. Every other micro binary rebuilt against the changed shared fakes and passes once.