UCP/RMA: Support failover for offloaded GET bcopy protocol

[evgeny-leksikov]

What?

Support failover for offloaded GET bcopy protocol

Why?

Small GET operations below the IB cap.get.min_zcopy bound had no failover-capable protocol, so ucp_get failed to start in FAILOVER error handling mode.

How?

GET is idempotent, so a failed bcopy request can be restarted on another lane and replayed from the beginning.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

No blocking issues found. The refactor of ucp_proto_request_restart_or_complete is clean and the zcopy path is preserved bit-for-bit. Residual coverage note: the new large AM variants rely on RNDV never being selected under failover (asserted in the AM callback); this holds only because RNDV protocols lack UCP_PROTO_COMMON_INIT_FLAG_FAILOVER, which is the intended design.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

test/gtest/ucp/test_ucp_fault_tolerance.cc

SMALL_MSG_SIZE = 8 relies on the assumption that 8 bytes stays below cap.get.min_zcopy so GET picks the bcopy protocol. The test has no assertion that get/bcopy was actually selected, so on any transport with min_zcopy <= 8 the small variant would silently fall back to zcopy and the intended bcopy-restart coverage would be lost without failing. This is a coverage gap rather than a bug; a comment noting the assumption or a light protocol-selection check could harden it.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests new on worker 2) · commit ef2e82d8

TL;DR: The build failed on a single unrelated gtest case, mt/test_profile.log/2 (8 threads), out of 8582 tests — a known-fragile timing/global-state–sensitive UCS profiling test that has nothing to do with this PR's UCT get-failover changes. Re-run the job; optionally harden test/gtest/ucs/test_profile.cc, which asserts exact global profile-file layout and a 1-second per-thread lifetime bound.

Full analysis

Summary: make test in the gtest stage exited with Error 1 because exactly one test failed: [ FAILED ] mt/test_profile.log/2, where GetParam() = 8; everything else (8581 tests) passed.

Root cause: Not the PR. test_profile enables profiling on the process-global default profile context (ucs_profile_default_ctx) and then asserts strict, global invariants that are not robust in a multithreaded, loaded CI process:

• test/gtest/ucs/test_profile.cc:344-407 assumes the profile file contains exactly GetParam() thread contexts and that ptr lands exactly on threads_end / &data[data.size()]. Any other live thread in the gtest process that executes a UCS_PROFILE_*_ALWAYS site while profiling is momentarily on adds an extra thread context (and an extra location from another source file), shifting every offset and breaking EXPECT_EQ(num_threads(), profiled_threads), EXPECT_EQ(NUM_LOCAITONS, num_locations) and the location/file checks. src/ucs/profile/profile.c:161-168 explicitly documents that there is no protection against a thread still producing profiling data during dump.
• test/gtest/ucs/test_profile.cc:269-270 bounds a worker thread's lifetime by 1s * test_time_multiplier * (1 + exp_count). In log mode exp_count == 0, so the bound collapses to ~1 second — the tightest of the three modes (accum and log_accum get ~6 s). The mt/…/8 instantiation is the widest window, which is consistent with log/8 being the only variant that failed.

Log wraparound is ruled out (default PROFILE_LOG_SIZE is far larger than the 60 records/thread expected), and the run shows no hang: the log is continuously active with sub-second test durations up to the summary, and the make failure is purely the gtest exit code.

Caveat: the Azure log only retains the tail (~500 lines), so the exact EXPECT_* line for mt/test_profile.log/2 (which ran ~an hour earlier in this 3706 s run) is not visible; the above narrows it to the non-deterministic assertions in that test.

Implicated commit: unknown for this failure (PR commit ef2e82d — UCT get failover — does not touch src/ucs/profile or the test). The strict layout/offset assertions were introduced by 5bf3df83 "UCS/PROFILE: added ucx env to profiling output, changed profiling file structure" (Roie Danino), and the profiling code itself has not changed since.

File: test/gtest/ucs/test_profile.cc:269 (1-second lifetime bound in log mode) and test/gtest/ucs/test_profile.cc:344-407 (exact thread-count / file-offset assertions)

Suggested fix:

1. Immediate: re-trigger the Azure "Tests new on worker 2" job — this failure is unrelated to the PR and should not block it. To confirm, re-run just the case: ./gtest --gtest_filter='mt/test_profile.log/2' --gtest_repeat=50.
2. Hardening (separate PR):
   • Make the lifetime bound mode-independent, e.g. use (1 + ITER) instead of (1 + exp_count) in test_thread_locations, or drop the upper bound and keep only start_time <= end_time <= ucs_get_time().
   • Derive the number of threads from the file (ucs_profile_calc_num_threads / hdr->threads.size) and ASSERT_EQ it up-front with a clear message, and match threads by TID against m_tids instead of assuming list order/count, so a stray profiled thread produces one clear failure rather than cascading offset mismatches.
   • Better: give this test its own profile context (ucs_profile_init(..., &my_ctx) — supported since 2233c4a2) instead of mutating ucs_profile_default_ctx, so unrelated process threads cannot pollute the dump.

Related: PR #11840 (this build); prior profiling-test/format change 5bf3df83; no existing issue found for mt/test_profile flakiness — worth opening one with a link to build 132820.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

test/gtest/ucp/test_ucp_fault_tolerance.cc

The new small GET variant (SMALL_MSG_SIZE = 1) relies on staying below cap.get.min_zcopy to force bcopy selection, but no assertion verifies get/bcopy was actually chosen — on a transport with a very small min_zcopy/threshold config the intended bcopy-restart coverage could silently be lost. (Already raised in the previous review; remains valid.)

test/gtest/ucp/test_ucp_fault_tolerance.cc

The AM large/RNDV path relies on RNDV never being selected under failover, which was already noted in a prior review.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit e4c14aa9

TL;DR: The Coverity devel stage failed with 1 new defect (NULL_RETURNS) in the new gtest helper check_op_proto_coverage() added by this PR — Coverity doesn't model gtest's ASSERT_NE as a control-flow terminator, so it sees select_elem (possibly nullptr from ucp_proto_select_lookup_slow) dereferenced at test_ucp_fault_tolerance.cc:300. Add an explicit if (select_elem == nullptr) { ... return; } guard, matching the existing pattern in test_ucp_proto.cc.

Full analysis

Summary: Azure job "Coverity coverity devel on coverity_rh7" (build 133040) failed at ##[error]Coverity found 1 issues: — build and cov-analyze succeeded, but the gate [[ nerrors -eq 0 ]] returned non-zero.

Root cause: In the new helper added by this PR, ucp_proto_select_lookup_slow() can return nullptr (src/ucp/proto/proto_select.c:564 explicitly returns null when status != UCS_OK). The code only guards it with ASSERT_NE(nullptr, select_elem). Coverity's checker does not recognize that gtest's ASSERT_* macro expands to a return on failure — the report explicitly traces "path: Falling through to end of if statement" and then "dereference: Dereferencing a pointer that might be nullptr select_elem when calling ucp_proto_select_elem_query" (which dereferences the parameter via ucp_proto_select_thresholds_search, proto_select.inl:35). Coverity even cites 5 supporting examples elsewhere in the tree where the same return value is explicitly NULL-checked (test_ucp_proto.cc:135, 427, 352, 904 and proto_select.inl:102), which is why it flags this one site as inconsistent. Note the defect count is a new-issues gate: the 1564 other occurrences (PASS_BY_VALUE, etc.) are pre-existing/filtered; this one is attributable to the PR.

Implicated commit: [REDACTED:Hex High Entropy String] — Evgeny Leksikov, "UCP/RMA: Support failover for offloaded GET bcopy protocol"

File: test/gtest/ucp/test_ucp_fault_tolerance.cc:289-300 (defect reported at line 300)

Suggested fix: Replace the fatal-assert-only guard with an explicit early return, so the null path is visibly unreachable to Coverity:

    select_elem = ucp_proto_select_lookup_slow(
            worker, proto_select, 0,
            sender().ep(0, INJECTED_EP_INDEX)->cfg_index, rkey_cfg_index,
            &select_param);
    if (select_elem == nullptr) {
        ADD_FAILURE() << ucp_operation_names[op_id]
                      << ": protocol selection is not initialized";
        return;
    }

This is exactly the idiom already used at test/gtest/ucp/test_ucp_proto.cc:135-138 and :426-429, and it keeps the same test semantics (failure reported, function aborted) while eliminating the NULL_RETURNS finding. Avoid ASSERT_NE/ASSERT_TRUE as the sole guard before a dereference in code that goes through Coverity.

Related: none found (searched the project for prior reports of this Coverity signature). Prior commits on the same file for context: db208ee4 (#11563), 9db2e488 (#11562), 6c7c8ff9 (#11545).

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id c4c0accb-bdab-4640-8dd3-ae25655b45a7 in the triage console for the audit trail.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.