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
NetIbMPI tests multithread mode for fault-injection and recovery - #10541
Last of four stacked PRs (#10538, #10539, #10540 are merged). Adds threaded fault-injection, failover and recovery tests: per-communicator fault hooks make isolation between workers assertable, N simultaneous link failures overlap failovers on one fused device, and concurrent recoveries exercise the process-global recovery thread. Also adds a nightly-only test for an open post-recovery finding.
Technical Details
The threaded fault suites keep RCCL_IB_FAULT_INJECTION=0: they use the per-communicator hooks, and the libibverbs shims would serialize every poll_cq on a global mutex and let one worker's clear disarm another's faults. The nightly test runs only on schedule because a_fault_recovery_nightly has no "smoke": true in the AINIC plan; no workflow change is needed.
Issue Tracking
JIRA ID: AICOMNET-323
Test Plan
Enabled tests
Two and four workers, per-communicator fault hooks:
FaultIsolationAcrossWorkers — new test. One worker breaks its own connection and must observe the failure while every other worker keeps transferring with a fatal count of zero. No serial test can assert this.
FaultInjCastQpErrorIsFatal — each worker arms error injection on its own QPs and must see the failure itself.
FaultInjCastDelayDataIntegrity — each worker slows its own QP 0 and still delivers intact data while the others load the device.
FaultInjCastQpErrorClearRecovers — each worker owns two connections: break the first, clear, then move data cleanly on the second.
FailoverCqeErrorRecovered — N links fail at once, so N failovers overlap on one fused device, each rewriting its own per-communicator resiliency state. Port recovery stays at its default of off in this suite: these tests assert that failover absorbed the error and that device 0 is no longer Ok, and recovery exists to undo exactly that.
FailoverLargeMessageDataIntegrity — 64 KB messages survive their own link failure byte for byte.
FailoverMultiRequestInFlight — several messages are posted first, then QP 0 is driven to error under them.
Two workers:
RecoverySuccessRestoresTraffic — the suite that does enable port recovery, at two workers: concurrent recoveries share one recovery thread, then traffic must resume on every worker. Two deliberate differences from the serial body, both forced: the queue pairs are broken while the connection is idle, because the serial in-flight break relies on an MPI handshake a worker cannot make, and one message is sent after recovery rather than a run of them.
Nightly only, suite a_fault_recovery_nightly (outside the smoke subset):
PostRecoveryPingPongHoldsSync — new test, and the reason the nightly slot exists. It sends ten blocking messages on the recovered connection and asserts they all arrive. The finding it tracks: after recovery reports device 0 healthy on both sides, the two ranks get a message apart, the sender exhausting its FIFO-slot retries on message n+1 while the receiver still waits for n, both devices reported Ok. Seen at two workers, and on every run of a variant that breaks only the sender's queue pair with a request in flight. The serial body sends its own run of messages and does not drift, because its MPI handshakes resynchronize the two sides between phases — which is why this only appears on the threaded path.
No rate is quoted, deliberately. The earlier figure was measured before this test's recovery wait required recoveryCount > 0, when a green run could accept device 0's initial Ok without recovery having been exercised at all, so it says nothing about the corrected test. Establishing the rate is what the nightly runs are for, and until the plugin side is understood the test is expected to fail some of them.
Procedure
Hardware: host build, two ranks.
Rebuild rccl-UnitTestsMPI in a debug configuration with MPI tests enabled.
Run this level's fault, failover and recovery suites at 2 and 4 workers, the nightly drift suite, and everything the three levels below ship.
Run the nightly drift test repeatedly on its own: a test meant to fail sometimes proves nothing from one green run.
Check scope selection through the runner's own config resolver: which suites --scope smoke keeps and which it skips.
Load every runner config through the runner's own resolver and compile the test sources with fault injection off and with MPI tests off.
Test Result
Full net_ib_transport.json: no new failures against develop; every test this PR adds passes at two and four workers.
AINIC plan gtest suites: no new failures against develop; FaultIsolationAcrossWorkers, FaultShimsAbsentUnlessRequested and PostRecoveryPingPongHoldsSync pass.
i-kosarev
changed the title
Run the NetIbMPI fault-injection and recovery tests at several worker counts, and track the post-recovery drift nightly
Run the NetIbMPI fault-injection and recovery tests multithread mode
Aug 21, 2026
Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*). Current: code file(s) changed: projects/rccl/test/transport/NetIbMPI/FaultInjectTests.cpp, projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp, projects/rccl/test/transport/NetIbMPI/StressTests.cpp; no test file found
🚫 Draft PR
🔜 To Be Enabled
—
🚩 Feature Flag
🔜 To Be Enabled
—
📊 Code Coverage
🔜 To Be Enabled
—
🎉 All policy checks passed!
📖 Need help? See the Policy FAQ for details on every check and how to fix failures.
This nightly test also accepts the communicator's initial Ok state as proof of recovery. If QP 0's failure is not observed, the loop exits immediately and the test measures ordinary traffic rather than post-recovery traffic. Require recoveryCount[0] > 0 when the state is Ok (while still accepting the transient Recovered state).
This delay does not keep all requests in flight. The hook sleeps immediately before each wrap_ibv_post_send (src/transport/net_ib_cast/p2p.cc:297-305), so while the second through fourth calls sleep, each earlier 4 KiB request has ample time to complete. Driving the QP to error can therefore still happen after most or all requests retired, making this test pass without exercising multi-request failover. Use a mechanism that holds every posted WR outstanding until the QP transition rather than a per-post pre-call delay.
static constexpr uint32_t kHoldInFlightUs = 200000; // 200ms
if (rank == 1) {
result = WorkerCastFaultSetDelay(pair.sendComm, /*qpIdx=*/0, kHoldInFlightUs);
This comment says the transfer is already posted, but the called helper first drives each side's QP to error and posts the transfer afterward (NetIbMPITestBase.hpp:1652-1662). Update it to reflect the idle break so the nightly test's reproduction sequence is documented accurately.
// Both directions lose QP 0 with a transfer already posted, so both
// sides notice and enter recovery and the payload rides the survivor.
i-kosarev
changed the title
Run the NetIbMPI fault-injection and recovery tests multithread mode
NetIbMPI tests multithread mode for fault-injection and recovery
Aug 21, 2026
Making setup failures non-fatal is unsafe until every caller participates in the coordinated bailout. Most existing call sites still immediately pass the selected communicator to RegisterMemory (for example, CastTests.cpp:39-49 and FaultInjectTests.cpp:151-162); on a failed accept/connect that is null, which can crash in the plugin or let only one rank return while its peer enters later traffic/barriers. Only the shim probe adds the required all-rank check. Return a setup status and update every caller to agree on it before using the communicator, or perform that coordinated early exit in a shared wrapper.
// and both ranks always reach the same collectives, so a caller that checks its
// communicator (they all should) can report and bail out with its peer.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 2,220 lines across 11 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. 5 finding(s) below High not posted this round; comment @argus re-review full for a full pass if you want them.
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
This PR changes 2,220 lines across 11 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.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 2,142 lines across 11 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.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,783 lines across 11 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.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,776 lines across 11 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.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,815 lines across 11 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.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Phase-two traffic can reuse memory still referenced by an abandoned request, and several synchronization and resource-accounting paths do not enforce their intended coverage.
The sender publishes faultArmed before it calls WorkerCastFaultSend. If the victim thread is descheduled here, all bystanders can finish before the victim connection has observed or handled any fault, and the test still passes, so it does not reliably exercise isolation while one connection is broken. Publish the sender-side flag only after the fault outcome has been confirmed (the receiver can still publish after posting its unmatched receive).
This filter already runs in cast_shim_leak_guard (net_ib_transport.json:593-607) and does not use --net_ib_nthreads. Because this parent is extended by both the 2- and 4-worker suites, the change adds two redundant hardware runs without adding threaded coverage. Keep the existing plain CAST guard and remove this duplicate entry.
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,830 lines across 11 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.
The building blocks the threaded fault-injection bodies need, in the
ThreadResult convention the rest of the harness uses: arming and clearing
per-communicator faults, reading the resiliency state and fatal counters,
driving queue pairs to error, and the shared failover/in-flight/abandoned-recv
sequences the tests compose.
Three things in here are less obvious than they look, and each was got wrong
first:
Retirement is caused, not observed. Once the injected fault is counted,
IbCastTest checks the communicator's fatal counter before it looks at the
request at all and returns on the first error CQE, so polling can never report
done however the work is doing -- reading an error as "retired" was wrong.
Driving every queue pair of the communicator to error is what actually retires
outstanding work, with a flush status.
The queue-pair walks are bounded by NCCL_IB_MAX_QPS, the transport's own
maximum, not a number chosen here. A connection's count is qpsPerConnection *
ndevs, so a fused two-device NIC at 64 per connection reaches the full 128; a
smaller bound let a walk stop early and still report success, releasing memory
higher queue pairs could still write into. The receive-side walk also tells
ncclInvalidArgument (past the end) apart from a real ibv_modify_qp failure,
since only the first means the range was fully covered.
Every helper that leaves a request possibly live hands the caller's buffer and
registration back retained rather than freeing memory the NIC may still touch,
and says so in the failure text.
kWorkerGatePolls is declared outside the fault-injection block: the stress
tests use it too and compile whenever MPI tests do, so a -DFAULT_INJECTION=OFF
build had no declaration for it.
…t is read
ThreadedProgressDoesNotSerialize evaluated its scaling checks on every rank
against that rank's own timings. Only rank 0's output is read unless
RCCL_MPI_LOG_ALL_RANKS is set, so a rank-1 failure printed nothing, and the two
ranks measure different things anyway -- rank 1 is the sender and carries the
FIFO-slot waits, rank 0 the receiver. A verdict that differed between them was
reported as whichever rank happened to be looked at.
The timings are now reduced and the verdict is drawn once, on the rank whose
output is actually read, with both ranks reaching the same collectives on the
way there. MrCacheRefCount's small adjustment comes along for the same reason:
its check was rank-local in front of a barrier.
…counts
Threaded bodies for the fault-injection and failover tests: fault isolation
between workers, the failover sequences, recovery, and the post-recovery
ping-pong that tracks an open finding.
What the concurrency is for differs per test and is stated in each. The failover
bodies exercise overlapping failovers on one fused device -- resiliency state is
per communicator, so N workers driving their own links into error at the same
moment is the claim -- while the recovery bodies exercise the process-global
recovery thread facing several broken connections at once. Both need the workers
to reach the failure together, so each uses a bounded rendezvous immediately
before the transition rather than relying on the start gate, which only
synchronises entry to the body: every worker allocates, registers and warms up
first, so without the gate a fast worker finishes failover before a slow one
arrives and the simultaneity the test claims never happens.
RecoverySuccessRestoresTraffic sends two post-recovery messages, not one. The
recovery thread publishes Recovered on its own, but activeQps is not restored
until IbCastResiliencyProgress runs, and that only runs from a request poll --
ncclIbCastGetResiliencyState just reads state. So the first message is a
progress kick that still travels on the failover queue pairs, the state is then
required to be Ok with recoveryCount > 0, and the second message is the one that
actually crosses the restored path. Not more than two: beyond that the two sides
have been seen to drift a message apart, which PostRecoveryPingPongHoldsSync
carries as its own finding and runs nightly only.
FaultIsolationAcrossWorkers' bystanders wait for the victim on the victim's own
hold budget rather than the generic 30 s gate. The victim may legitimately hold
the fault for up to 240 s -- sized from what the bystanders' transfers can take --
so the generic gate would expire while the victim was doing exactly what it is
supposed to, and report it as never finishing. Both derive from one constant now.
Adds the new threaded suites to every config that carries their serial
siblings, marks a_fault_inject_multithread (A19) smoke on the AINIC config, and
leaves a_fault_recovery_nightly (A20) without it: PostRecoveryPingPongHoldsSync
tracks an open finding and is expected to fail some runs until the plugin side
is understood, so it runs nightly rather than gating PRs. A20's description
names that mechanism rather than the old suite-name convention it no longer
selects on.
FaultInjectionShimsAbsentUnlessRequested guarded against SetupCastConnection
returning a null communicator without asserting -- a behavior this branch
carried in an earlier commit, since dropped so #10862 can make that change
once instead of twice. SetupCastConnection is fatal again here (ASSERT_EQ/
ASSERT_NE), so a null comm can no longer reach this point; the guard was
dead code checking a precondition the helper itself now enforces.
…needs
Every comment block this stack adds is now one line, two where the reason cannot fit in one, and the ones that only restated the code are gone. Also splits two statements that had ended up on one line, and moves a comment that had been glued onto the end of a declaration.
Co-authored-by: Cursor <cursoragent@cursor.com>
…to complete
Driving a queue pair to error stops new work but does not retire the request: IbCastTest returns on the fatal-count check before polling, or on the first flush CQE before IbCastRequestComplete, and the device-global MR cache can keep a shared MR alive past our deregistration. WorkerCastFaultSend therefore reports retired only on an observed completion (or no request), and records separately whether every queue pair was driven to error; WorkerCastFlushAbandonedRecv no longer reads a TestRequest error as retirement. Where completion was not seen the host buffer is kept rather than freed, while the MR is still deregistered so the device PD can be released at close. Also corrects the warm-up budget comment: four workers move 16 MB each.
Co-authored-by: Cursor <cursoragent@cursor.com>
A QP reaching ERR stops new work but does not prove a request was completed. If completion was not observed, preserve both the buffer and its registration until the runner's isolated test process exits. The threaded harness tracks this deliberate retention so it still checks QP/CQ cleanup without calling the expected MR/PD hold a leak. Mark the post-recovery drift test as a red nightly canary rather than an expected failure.
Co-authored-by: Cursor <cursoragent@cursor.com>
A faulted request with no observed completion retains both its buffer and MR, so the recovery test must use a separate buffer for its fresh connection. Route all central worker retention helpers through one accounting function, allowing the harness to distinguish deliberate MR/PD retention from teardown leaks while still checking QP/CQ cleanup. Describe the post-recovery drift suite as a red nightly canary rather than an expected failure.
Co-authored-by: Cursor <cursoragent@cursor.com>
The reason will be displayed to describe this comment to others. Learn more.
ARGUS
This PR changes 1,830 lines across 11 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 PostRecoveryPingPongHoldsSync expands RECOVERY_UD_OR_SKIP() and FAILOVER_RDMA_READ_OR_SKIP(), as every other recovery test in that file does.
Comment @argus review to re-run, or @argus re-review after pushing.
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.
github-actions
Bot
added
github actions
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Nikhil-Nunna
left a comment
nikitaxgusev
left a comment
Motivation
Last of four stacked PRs (#10538, #10539, #10540 are merged). Adds threaded fault-injection, failover and recovery tests: per-communicator fault hooks make isolation between workers assertable, N simultaneous link failures overlap failovers on one fused device, and concurrent recoveries exercise the process-global recovery thread. Also adds a nightly-only test for an open post-recovery finding.
Technical Details
The threaded fault suites keep
RCCL_IB_FAULT_INJECTION=0: they use the per-communicator hooks, and the libibverbs shims would serialize everypoll_cqon a global mutex and let one worker's clear disarm another's faults. The nightly test runs only on schedule becausea_fault_recovery_nightlyhas no"smoke": truein the AINIC plan; no workflow change is needed.Issue Tracking
JIRA ID: AICOMNET-323
Test Plan
Enabled tests
Two and four workers, per-communicator fault hooks:
FaultIsolationAcrossWorkers— new test. One worker breaks its own connection and must observe the failure while every other worker keeps transferring with a fatal count of zero. No serial test can assert this.FaultInjCastQpErrorIsFatal— each worker arms error injection on its own QPs and must see the failure itself.FaultInjCastDelayDataIntegrity— each worker slows its own QP 0 and still delivers intact data while the others load the device.FaultInjCastQpErrorClearRecovers— each worker owns two connections: break the first, clear, then move data cleanly on the second.FailoverCqeErrorRecovered— N links fail at once, so N failovers overlap on one fused device, each rewriting its own per-communicator resiliency state. Port recovery stays at its default of off in this suite: these tests assert that failover absorbed the error and that device 0 is no longer Ok, and recovery exists to undo exactly that.FailoverLargeMessageDataIntegrity— 64 KB messages survive their own link failure byte for byte.FailoverMultiRequestInFlight— several messages are posted first, then QP 0 is driven to error under them.Two workers:
RecoverySuccessRestoresTraffic— the suite that does enable port recovery, at two workers: concurrent recoveries share one recovery thread, then traffic must resume on every worker. Two deliberate differences from the serial body, both forced: the queue pairs are broken while the connection is idle, because the serial in-flight break relies on an MPI handshake a worker cannot make, and one message is sent after recovery rather than a run of them.Nightly only, suite
a_fault_recovery_nightly(outside the smoke subset):PostRecoveryPingPongHoldsSync— new test, and the reason the nightly slot exists. It sends ten blocking messages on the recovered connection and asserts they all arrive. The finding it tracks: after recovery reports device 0 healthy on both sides, the two ranks get a message apart, the sender exhausting its FIFO-slot retries on message n+1 while the receiver still waits for n, both devices reported Ok. Seen at two workers, and on every run of a variant that breaks only the sender's queue pair with a request in flight. The serial body sends its own run of messages and does not drift, because its MPI handshakes resynchronize the two sides between phases — which is why this only appears on the threaded path.No rate is quoted, deliberately. The earlier figure was measured before this test's recovery wait required
recoveryCount > 0, when a green run could accept device 0's initialOkwithout recovery having been exercised at all, so it says nothing about the corrected test. Establishing the rate is what the nightly runs are for, and until the plugin side is understood the test is expected to fail some of them.Procedure
Hardware: host build, two ranks.
rccl-UnitTestsMPIin a debug configuration with MPI tests enabled.--scope smokekeeps and which it skips.Test Result
net_ib_transport.json: no new failures againstdevelop; every test this PR adds passes at two and four workers.develop;FaultIsolationAcrossWorkers,FaultShimsAbsentUnlessRequestedandPostRecoveryPingPongHoldsSyncpass.Submission Checklist