[william-gallagher-nv]

What?

When enabled, rcache regions of adjacent memory will be merged. Merges only occur when new region is the same size or larger than the existing region. This prevents continually reregistering the same memory region when adding many small regions in a row.

Why?

Reduce the number of memory keys in use for sequences of adjacent memory regions. When using too many memory keys, NIC can run out of resources and fail to register memory.
Fixes #6264

How?

The rcache already has handling for memory regions that overlap with existing registrations. This is extend to handle regions that sit adjacent in memory when enabled. This is a little more difficult. Overlapping regions can be can safely be assumed to be of the same memory type and from the same allocation. This isn't true for adjacent memory regions. This is handled by checking to make sure that the regions have the same memory type, memory flags, and are part of the same allocation.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Residual coverage gap: no test for the gap-fill path (both neighbors present) and no negative test that regions are not merged when the flag is off; both are cheap to add once the tests use their own fixture.

The knob UCX_RCACHE_MERGE_ADJACENT itself (config → rcache_params.flags plumbing) is not exercised anywhere; note uct_rocm_copy_md.c:456 assigns flags = after ucs_rcache_set_params(), so the new flag is silently dropped for rocm_copy.

Growth is unbounded by design (each merge at least doubles the region), so UCX_RCACHE_MAX_SIZE/LRU are the only limits; worth confirming this is intended before merge.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests althca on worker 2) · commit fd32e7c9

TL;DR: The gtest test_rcache.merge/mt_6 (6-thread) case fails because this PR turns on UCS_RCACHE_FLAG_MERGE_ADJACENT unconditionally for every rcache gtest via get_default_rcache_params(); with 6 threads the per-thread mmap buffers land adjacent in the address space, so regions get merged/invalidated across threads and the test's region-identity assertions break. Fix: don't set the flag in the shared default params — put it in a dedicated fixture used only by the new merge_adjacent* tests.

Full analysis

Summary: make test in the "althca on worker 2" gtest job exited non-zero: 3962 tests passed, 1 failed — [ FAILED ] test_rcache.merge/mt_6 — while all the new single-threaded merge_adjacent* tests passed.

Root cause: Commit [REDACTED:Hex High Entropy String] (the PR head) added the adjacent-merge capability and, in the test helper, enabled it globally:

ucs_rcache_params_t params = {sizeof(ucs_rcache_region_t), UCM_EVENT_VM_UNMAPPED, 1000, ops, context,
                              /* Temp for testing merge code. */
                              UCS_RCACHE_FLAG_MERGE_ADJACENT, ULONG_MAX, SIZE_MAX};

(test/gtest/ucs/test_rcache.cc:22-30)

Because get_default_rcache_params() is shared by all test_rcache cases, the pre-existing multi-threaded merge test (UCS_MT_TEST_F(test_rcache, merge, 6), line 363) now runs with adjacent merging on. All 6 threads share one rcache and each alloc_pages() (mmap) a 832-page buffer; the kernel typically places these consecutively, so thread A's region is exactly adjacent to thread B's. ucs_rcache_check_adj_regions() (src/ucs/memory/rcache.c:835-863, invoked from ucs_rcache_check_neighbors at lines 924-929) then looks up start-1/end, picks up the other thread's region, and ucs_rcache_check_overlap_one() merges it and calls ucs_rcache_region_invalidate_internal() on it (line 823) — even though that region is still held (refcount > 1) by the other thread. The victim thread's subsequent get() therefore returns a brand-new region, so the identity assertions fail nondeterministically:

• EXPECT_EQ(region1, region1_2) (line 386)
• EXPECT_NE(region1, region1_2) / EXPECT_EQ(region3, region1_2) (lines 400-401)

Note also that ucs_rcache_check_adj_regions() takes an alignment argument it never uses, and unlike the overlap path it does no ucs_rcache_region_test()/refcount check on the adjacent candidate.

Implicated commit: [REDACTED:Hex High Entropy String] — William Gallagher, "UCS/RCACHE: Adds capability for merging adjacent rcache regions"

File: test/gtest/ucs/test_rcache.cc:27-28 (flag in shared defaults); behavior in src/ucs/memory/rcache.c:835-863 and src/ucs/memory/rcache.c:924-929

Suggested fix:

1. Remove the /* Temp for testing merge code. */ UCS_RCACHE_FLAG_MERGE_ADJACENT from get_default_rcache_params() (pass 0 for flags), and add a dedicated fixture for the new tests:

   class test_rcache_merge_adjacent : public test_rcache {
   protected:
       ucs_rcache_params_t rcache_params() override {
           ucs_rcache_params_t p = test_rcache::rcache_params();
           p.flags |= UCS_RCACHE_FLAG_MERGE_ADJACENT;
           return p;
       }
   };

   and re-declare merge_adjacent, merge_adjacent_high_to_low, etc. under it. This restores the legacy semantics for test_rcache.merge/mt_6 and keeps the new coverage.
2. Independently, harden ucs_rcache_check_adj_regions(): skip adjacent candidates that are in use (region->refcount > 1) or that fail ucs_rcache_region_test(region, prot, alignment), and either use or drop the unused alignment parameter. Silently invalidating a neighbour region that another user still holds is exactly what the MT test exposed, and it will hit real applications whose independent allocations happen to be contiguous.

Related: PR #11958 (this PR, WIP), PR #11803 "UCS/RCACHE: Merge adjacent regions" (earlier attempt at the same feature)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 9abab884-391a-476d-9a9c-afc72d9a4040 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]

Residual coverage note: No test covers the merge_adjacent size heuristic itself (a get that must not merge because the neighbor is larger), which is the rule that makes random_merge order-dependent; a small explicit test for it would pin the intended behavior.

[william-gallagher-nv]

Open Questions:

• Currently merging adjacent regions is opt-in via configuration variable. This feels safer but curious if we want to make it opt-out.
• There is no upper bound for combining regions. There are diminishing returns and increased cost for merging very large regions. Does it make sense to pick a cap beyond which no more merges will occur?
• Do we need to check if two adjacent regions are part of the same map in /proc/self/maps? This would be expense to do each time evaluating a region for merge.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Note: Adjacent merging never pins memory the user did not already register or request (merged span is always left ∪ new ∪ right), so skipping the size check in the gap-fill branch is sound; the heuristic only bounds re-registration work.

Note: Added lines are right at the ~500 line ceiling from REVIEW.md; not over, but no room for more in this PR.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit 545a4d48

TL;DR: The gtest run failed on a single test, rcx/test_ucp_fault_tolerance.probe_gated_recovery/6 (rc_x/AM), which is a race-prone test that asserts it observed a transient aux-probe arming window; it is unrelated to this PR's rcache change, so rerun and make the test deterministic (mock the probe) rather than polling for a transient state.

Full analysis

Summary: make test in the "gpu on worker 3" job exited with error 1 because 1 of 10277 gtests failed: rcx/test_ucp_fault_tolerance.probe_gated_recovery/6, where GetParam() = rc_x/AM (all other 10256 tests passed, 20 skipped).

Root cause: The Azure log tail only preserves the summary block ([ FAILED ] 1 test ... 1 FAILED TEST, then make: *** [Makefile:4787: test] Error 1); the per-test assertion text scrolled out of the 500-line tail, so the exact assertion line is not in the retrievable log. From the source, the only failure-capable check unique to this test is EXPECT_TRUE(probe_armed) << "RC p2p lane recovery completed without arming an aux probe" (test/gtest/ucp/test_ucp_fault_tolerance.cc:941). That flag is only set if a poll iteration happens to catch ep->ext->recovery_arg != NULL and arg->probe[lane].comp.func != NULL — i.e. it samples a transient state. wait_for_cond() evaluates its predicate first, and its exit condition is ucp_ep_get_failed_lanes(ep) == 0; if the target-side-injected lane recovers (or the probe completes and recovery_arg is freed) before/between polls, probe_armed stays false and the test fails. Sibling tests in the same file (teardown_with_outstanding_probe, recovery_retries_exhausted_live_lanes) avoid this by installing a ucs::mock on ops.ep_check to hold/fail the probe deterministically — probe_gated_recovery does not. Nothing in this PR's scope (src/ucs/memory/rcache.c, adjacent-region merging) touches the UCP FT lane-recovery/aux-probe path, and no data-integrity or registration failure appears anywhere in the run, so this is a pre-existing flaky test rather than a regression from commit 545a4d4.

Implicated commit: db208ee "UCP/FT: probe-gated lane recovery via aux uct_ep_check (#11563)", Evgeny Leksikov — the commit that added this test (PR commit [REDACTED:Hex High Entropy String]/[REDACTED:Hex High Entropy String] by William Gallagher on rcache is not implicated)

File: test/gtest/ucp/test_ucp_fault_tolerance.cc:922-943 (assertion at :941)

Suggested fix: Re-run the failed job to confirm flakiness, and harden the test: install a ucs::mock on ops.ep_check (as in teardown_with_outstanding_probe) so the probe is guaranteed to be observed/held, or record arming in a sticky counter incremented inside the UCP recovery code (e.g. a debug/stats counter bumped when probe[lane] is armed) instead of polling for comp.func != NULL. Until then, the EXPECT_TRUE(probe_armed) check should be demoted to a skip (UCS_TEST_SKIP_R("recovery finished before an aux probe could be sampled")) when the recovery completes before any poll observes a probe. Also consider bumping the tail-capture size when triaging this pipeline so the gtest assertion text is retrievable.

Related: PR #11958 (this PR, rcache merge adjacent), PR #11803 (earlier rcache merge attempt), PR #11563 (introduced probe-gated recovery + this test); prior precedent for disabling a flaky FT test: 13ae935 "TEST/GTEST: Disable failing fault-tolerance test (#11397)"

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 7f6fd5b1-7cd4-447b-887b-bec0cc9e8a91 in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-ucx]

🤖 CI Triage Agent — UCX PR (WireCompat Build on althca) · commit 2ee19de7

TL;DR: The build failed to compile src/ucs/memory/rcache.c because GCC (with -Werror=maybe-uninitialized) flags region_left as possibly used uninitialized in the new ucs_rcache_check_adj_regions() helper; initialize region_left/region_right to NULL and gate the list insertions on them being non-NULL.

Full analysis

Summary: "Build ucx artifact" step of the WireCompat build on althca failed — make aborted with a -Werror=maybe-uninitialized compile error in memory/rcache.c (ucs_rcache_create_region/ucs_rcache_check_adj_regions).

Root cause: In ucs_rcache_check_adj_regions() the pointers region_left and region_right are declared without initializers and are only assigned inside if (pgt_left != NULL) / if (pgt_right != NULL). They are later dereferenced via ucs_list_add_tail(list, &region_left->tmp_list) guarded only by can_merge_left / can_merge_right, which are set by an opaque callback rcache->params.ops->can_merge(...). The compiler cannot prove that can_merge_left != 0 implies pgt_left != NULL, so after inlining ucs_list_add_tail (hence the diagnostic pointing at src/ucs/datastruct/list.h:60: elem->next = next;) it emits 'region_left' may be used uninitialized. Since UCX builds with -Wall -Werror, this is a hard build failure. The logic itself is safe at runtime (the flags can only be set when the pointer was assigned), so this is purely a compiler-provability issue — which explains why it only trips on this agent's (older) GCC while other legs may pass.

Exact log evidence:

memory/rcache.c: In function 'ucs_rcache_create_region':
src/ucs/datastruct/list.h:60:16: error: 'region_left' may be used uninitialized in this function [-Werror=maybe-uninitialized]
memory/rcache.c:841:26: note: 'region_left' was declared here
cc1: all warnings being treated as errors
make[3]: *** [memory/libucs_la-rcache.lo] Error 1

Implicated commit: [REDACTED:Hex High Entropy String] — William Gallagher, "UCS/RACHE: Check adjacent regions for memtype, flags and allocation" (introduced the can_merge callback gating; the helper itself was added in [REDACTED:Hex High Entropy String], same author/series, PR #11958)

File: src/ucs/memory/rcache.c:841 (declaration), failure surfaces at src/ucs/memory/rcache.c:864 / 876 via src/ucs/datastruct/list.h:60

Suggested fix: Initialize the pointers and use them as the guard so the compiler can see the correlation, dropping the redundant can_merge_* ints:

static void
ucs_rcache_check_adj_regions(ucs_rcache_t *rcache, void *arg,
                             ucs_pgt_addr_t start, ucs_pgt_addr_t end,
                             ucs_list_link_t *list)
{
    ucs_pgt_region_t *pgt_left, *pgt_right;
    ucs_rcache_region_t *region_left  = NULL;
    ucs_rcache_region_t *region_right = NULL;

    if (rcache->params.ops->can_merge == NULL) {
        return;
    }

    pgt_left  = ucs_pgtable_lookup(&rcache->pgtable, start - 1);
    pgt_right = ucs_pgtable_lookup(&rcache->pgtable, end);

    if (pgt_left != NULL) {
        region_left = ucs_derived_of(pgt_left, ucs_rcache_region_t);
        if (!rcache->params.ops->can_merge(arg, region_left)) {
            region_left = NULL;
        }
    }

    if (pgt_right != NULL) {
        region_right = ucs_derived_of(pgt_right, ucs_rcache_region_t);
        if (!rcache->params.ops->can_merge(arg, region_right)) {
            region_right = NULL;
        }
    }

    /* Fill in a gap between two existing registrations */
    if ((region_left != NULL) && (region_right != NULL)) {
        ucs_list_add_tail(list, &region_left->tmp_list);
        ucs_list_add_tail(list, &region_right->tmp_list);
        return;
    }

    if ((region_left != NULL) &&
        ucs_rcache_check_adj_size(start, end, region_left)) {
        ucs_list_add_tail(list, &region_left->tmp_list);
    }

    if ((region_right != NULL) &&
        ucs_rcache_check_adj_size(start, end, region_right)) {
        ucs_list_add_tail(list, &region_right->tmp_list);
    }
}

This keeps behavior identical and removes the warning on all GCC versions. (Do not silence it with a #pragma — the NULL-init version is cleaner and self-documenting.)

Related: PR #11958 (branch rcache-merge-adjacent); same-series commits [REDACTED:Hex High Entropy String] and [REDACTED:Hex High Entropy String]. No existing issue found for this warning.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 0e0f1735-e486-4ea1-b89e-e7f1e3aa73c1 in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-ucx]

🤖 CI Triage Agent — UCX PR (WireCompat Build on althca) · commit f9641208

TL;DR: The WireCompat Build on althca job failed at compile time: src/ucs/memory/rcache.c:841 declares region_left/region_right without initializing them, and they are later tested against NULL on paths where they were never assigned, tripping -Werror=maybe-uninitialized. Initialize both pointers to NULL at declaration.

Full analysis

Summary: make failed compiling memory/libucs_la-rcache.lo — GCC -Werror=maybe-uninitialized on region_left in ucs_rcache_create_region/ucs_rcache_check_adj_regions.

Root cause: In ucs_rcache_check_adj_regions() (inlined into ucs_rcache_create_region), region_left and region_right are declared uninitialized at line 841. They are only assigned inside if (pgt_left != NULL) / if (pgt_right != NULL) (lines 852–860). Later, lines 869 and 875 do if ((region_left != NULL) && can_merge_left) / if ((region_right != NULL) && can_merge_right) — reading the variables on paths where the pgtable lookup returned NULL and the assignment never happened. GCC flags the subsequent ucs_list_add_tail(list, &region_left->tmp_list) (which expands to elem->next = next; in src/ucs/datastruct/list.h:60) as a use of a possibly-uninitialized value. With -O3 -Wall -Werror in this job's CFLAGS the warning is fatal:

memory/rcache.c: In function 'ucs_rcache_create_region':
.../datastruct/list.h:60:16: error: 'region_left' may be used uninitialized in this function [-Werror=maybe-uninitialized]
memory/rcache.c:841:26: note: 'region_left' was declared here
cc1: all warnings being treated as errors
make[3]: *** [memory/libucs_la-rcache.lo] Error 1

This is not just a warning-pedantry issue — the != NULL guards at lines 869/875 are clearly intended to detect "no adjacent region found", so the missing initialization is a real latent bug (garbage pointer could be treated as a valid region).

Implicated commit: [REDACTED:Hex High Entropy String] — William Gallagher, "UCS/RCACHE: Check adjacent regions for memtype, flags and allocation" (the PR head commit). The enclosing function was introduced by [REDACTED:Hex High Entropy String] "UCS/RCACHE: Adds capability for merging adjacent rcache regions"; f9641208 added the can_merge/NULL-check logic that exposed the uninitialized path.

File: src/ucs/memory/rcache.c:841 (uses at :869 and :875)

Suggested fix: Initialize the pointers at declaration, and drop the now-redundant can_merge flags' dependence on them:

static void
ucs_rcache_check_adj_regions(ucs_rcache_t *rcache, void *arg,
                             ucs_pgt_addr_t start, ucs_pgt_addr_t end,
                             ucs_list_link_t *list)
{
    ucs_pgt_region_t *pgt_left, *pgt_right;
    ucs_rcache_region_t *region_left  = NULL;   /* <-- add */
    ucs_rcache_region_t *region_right = NULL;   /* <-- add */
    int can_merge_left  = 0;
    int can_merge_right = 0;
    ...

Note the codebase is built with -Werror=declaration-after-statement, so keep the initializers in the declaration block (as above) rather than adding assignment statements later. After that, the (region_left != NULL) guards at 869/875 become correct rather than UB; you could simplify them to just if (can_merge_left) since can_merge_left can only be non-zero when region_left was assigned. Consider also adding this compiler/config combo (gcc with -O3 -Werror, no CUDA/ROCm) to a pre-merge smoke build, since the PR's other build variants apparently didn't trigger the warning.

Related: PR #11958 (rcache-merge-adjacent); preceding commits in the same series: [REDACTED:Hex High Entropy String], [REDACTED:Hex High Entropy String].

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 54d00af5-756f-44ec-bc5b-9c76d7757e74 in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Static_check Static checks) · commit d025653b

TL;DR: The Static checks stage failed because clang's static analyzer (core.NullDereference) flagged ucp_memh_rcache_get() dereferencing a NULL mem_info — the new mem_info parameter added by this PR is passed as 0 from ucp_memh_import_slow(). Fix by passing a properly initialized ucs_memory_info_t (or handling NULL) at that call site.

Full analysis

Summary: Azure job UCX PR (Static_check Static checks) build 137136 aborted at the "cstools reports" step: static checkers found errors — one clang analyzer warning reported twice (once per analyzed config) at src/ucp/core/ucp_mm.c:798.

Root cause: The compile itself succeeded; the failure is purely the static-analysis gate. Log excerpt:

Error: CLANG_WARNING:
/src/ucp/core/ucp_mm.c:798:23: warning: Access to field 'mem_flags' results in a
dereference of a null pointer (loaded from variable 'mem_info') [core.NullDereference]
#        .mem_flags  = mem_info->mem_flags,

This PR (rcache-merge-adjacent) extended ucp_mem_rcache_reg_ctx_t with mem_flags/alloc_base/alloc_len and added a ucs_memory_info_t *mem_info parameter to ucp_memh_rcache_get(), which now unconditionally dereferences it:

796:    ucp_mem_rcache_reg_ctx_t reg_ctx = {
797:        .mem_type   = mem_type,
798:        .mem_flags  = mem_info->mem_flags,
799:        .alloc_base = mem_info->base_address,
800:        .alloc_len  = mem_info->alloc_length,

The ucp_memh_get_slow() → ucp_memh_find_slow() path passes &mem_info (fine), but the imported-memh path passes a literal 0 for that new argument:

2045:  status = ucp_memh_rcache_get(rcache, unpacked->address,
2046:                               unpacked->length, UCS_RCACHE_MIN_ALIGNMENT,
2047:                               unpacked->mem_type, 0, 0, 0, "", &memh);

So this is a real NULL dereference reachable whenever ucp_mem_map() is called with an exported memh buffer and context->imported_mem_hash != NULL — not a false positive.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCS/RCACHE: Check adjacent regions for memtype, flags and allocation", William Gallagher (branch head d025653b, "UCS/RCACHE: Initialize pointers with NULL to fix build", same author)

File: src/ucp/core/ucp_mm.c:798 (deref) — bad call site at src/ucp/core/ucp_mm.c:2045-2047

Suggested fix: In ucp_memh_import_slow(), build and pass a real ucs_memory_info_t instead of 0:

ucs_memory_info_t mem_info;
...
mem_info.type         = unpacked->mem_type;
mem_info.sys_dev      = UCS_SYS_DEVICE_ID_UNKNOWN;
mem_info.base_address = unpacked->address;
mem_info.alloc_length = unpacked->length;
mem_info.mem_flags    = UCS_MEM_FLAG_REGISTRABLE;

status = ucp_memh_rcache_get(rcache, unpacked->address, unpacked->length,
                             UCS_RCACHE_MIN_ALIGNMENT, unpacked->mem_type,
                             &mem_info, 0, 0, "", &memh);

Setting base_address/alloc_length to exactly the imported range also keeps ucp_mem_rcache_can_merge_cb() from merging imported regions with unrelated ones. Alternatively, make ucp_memh_rcache_get() tolerate mem_info == NULL by falling back to defaults (e.g. ucs_memory_info_set_host()-style init), but the explicit fill is safer given the new can_merge/merge callbacks read alloc_base, alloc_len and mem_flags. Passing 0 for a pointer parameter should also be changed to NULL style everywhere to avoid this class of mistake.

Related: #11958

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4be7f3ba-8e4e-48b1-a5f2-04527780aef6 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]

src/ucs/memory/rcache.c

Residual note: ucs_rcache_check_adj_regions() does ucs_pgtable_lookup(pgtable, start - 1), which wraps to UCS_PGT_ADDR_MAX if a region ever starts at address 0 — harmless in practice but relies on no region being mapped at the top of the address space.

Residual note: the UCX_*_RCACHE_MERGE_ADJACENT knob is also exposed for gdr_copy/rocm_copy, where it silently does nothing because those ops tables have no can_merge.

[yafshar]

@william-gallagher-nv The last push brought ~20 master commits (#12060, #12154, #12069, #12110, ...) into the branch as new commits, so the PR now shows 74 files and +3221 lines. Merged into current master, the real change is 6 files (rcache.c/h, ucp_mm.c, rocm_copy_md.c, test_rcache.cc, test_ucp_mmap.cc). Can you git merge upstream/master (or rebase onto it) and push, so the diff shows only the rcache change? It also pulled in CODEOWNERS reviewers for files this PR doesn't change.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Build build_x86_64 rocky96) · commit e03cd3f8

TL;DR: The build never got past the "Get sources" step — git fetch against https://github.com/openucx/ucx failed 15 times with fatal: could not read Username for 'https://github.com': terminal prompts disabled, i.e. the GitHub credential/auth header was not supplied to the agent. This is a CI infrastructure/credential problem, not a code regression; re-run the job after the GitHub service connection token is refreshed.

Full analysis

Summary: build_x86_64 rocky96 failed in the Checkout openucx/ucx@refs/pull/11958/merge to s task; all 5 retries (3 fetch attempts each) returned Git fetch failed with exit code: 128.

Root cause: Authentication to GitHub was unavailable to the checkout task. In the log, git config --get-all http.https://github.com/openucx/ucx.extraheader, git config --get-all http.extraheader and git config --get-regexp .*extraheader all return nothing, and the subsequent git --config-env=http.extraheader=env_var_http.extraheader fetch ... +[REDACTED:Hex High Entropy String]:refs/remotes/origin/9fc79054... immediately fails with fatal: could not read Username for 'https://github.com': terminal prompts disabled (20:21:47.9 → 20:23:36.8, every attempt identical, sub-second failures with no stalls). Git fell back to interactive credential prompting because the injected auth header was empty/invalid — consistent with an expired or revoked GitHub service-connection token (or a repo-access/rate-limit denial) on agent swx-rdmz-instinct02-16. No source was ever checked out, nothing was compiled, and no UCX code from this PR was executed, so the PR's rcache changes cannot be implicated. (Secondary note: the fetch targets commit 9fc79054... while the trigger reports e03cd3f8... — expected, as the pipeline resolves refs/pull/11958/merge.)

Implicated commit: unknown — failure is pre-checkout; no repository content was fetched, so no commit is implicated.

File: unknown — no repository file was involved (failure in the Azure Pipelines Get sources task, not in UCX source).

Suggested fix: Have the CI admins rotate/refresh the GitHub service connection (PAT or GitHub App installation token) used by the UCX PR pipeline and verify the token still has repo/read access to openucx/ucx, then re-queue build 142204. If only this agent is affected, check swx-rdmz-instinct02-16 for a stale cached credential or a ~/.gitconfig/insteadOf override, and confirm the container (rocky9.6/builder:mofed-24.10-3.2.5.0) isn't shadowing the agent's credential helper. As hardening, set GIT_TERMINAL_PROMPT=0 expectations aside and add a pre-flight connectivity/auth check so this fails fast with a clear message instead of 15 opaque retries. No code change to the PR is needed; the rcache-merge-adjacent branch should be re-tested once checkout works.

Related: none — searches for this error signature returned only unrelated PRs (no existing issue tracking the checkout auth failure).

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4c3f47dd-7030-4751-a497-f7e32c9de389 in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[william-gallagher-nv]

@william-gallagher-nv The last push brought ~20 master commits (#12060, #12154, #12069, #12110, ...) into the branch as new commits, so the PR now shows 74 files and +3221 lines. Merged into current master, the real change is 6 files (rcache.c/h, ucp_mm.c, rocm_copy_md.c, test_rcache.cc, test_ucp_mmap.cc). Can you git merge upstream/master (or rebase onto it) and push, so the diff shows only the rcache change? It also pulled in CODEOWNERS reviewers for files this PR doesn't change.

@yafshar I accidently squashed a merge commit with master before pushing the last two commits. Fixed now. Sorry for the spam on unrelated changes.

[svc-nvidia-pr-review]

minor: mem_info.type is set to UCS_MEMORY_TYPE_HOST while the same call passes unpacked->mem_type as the memory type. Harmless today (only sys_dev/base_address/alloc_length/mem_flags are read out of mem_info), but the two values contradict each other in one statement.

Suggested change

	.type = UCS_MEMORY_TYPE_HOST,
	.type = unpacked->mem_type,

[svc-nvidia-pr-review]

Residual coverage gaps (previously reported, noted again for completeness): UCX_RCACHE_MERGE_ADJACENT defaults to n with no CI job flipping it, test_ucp_rcache_merge is instantiated only for rcx, "rc_x,cuda_copy" (IB hosts only), and no test asserts that regions are not merged when UCS_RCACHE_FLAG_MERGE_ADJACENT is clear.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit ab90c1b4

TL;DR: The GPU worker-3 gtest run failed on a single unrelated test, tcp/test_uct_sockaddr.cm_open_listen_kill_server/4 (tcp CM over eno1) — a timing-dependent failure in the TCP sockcm disconnect-detection path, not caused by this PR's rcache changes; re-run the job and, if it repeats, investigate the client's disconnect notification after abrupt server teardown.

Full analysis

Summary: make test in build-test/test/gtest exited with error 1 because 1 of 10552 tests failed: [ FAILED ] tcp/test_uct_sockaddr.cm_open_listen_kill_server/4, where GetParam() = /eno1 (all other tests passed; 20 skipped).

Root cause: The only failing test is a UCT TCP connection-manager (tcp_sockcm) test that is completely unrelated to the PR under test. PR #11958 (rcache-merge-adjacent) only touches src/ucs/memory/rcache.c (recent branch commits: 8f6fb0b0 clang-format, 0ee9e0c3 "Skip merge if first region is not registered", b55e24d3 "Skip merge for regions with different protection"), and the rcache gtests themselves passed in this very run (e.g. test_rcache_with_limit.by_size_inuse OK). cm_open_listen_kill_server destroys the server entity (closing its listener and ep sockets abruptly) and then waits for the client's disconnect callback:

• wait_for_bits(&m_state, TEST_STATE_CLIENT_DISCONNECTED) uses the default 10 s deadline (DEFAULT_TIMEOUT_SEC in test/gtest/common/test.h:84), followed by EXPECT_TRUE(m_state & TEST_STATE_CLIENT_DISCONNECTED).
• If the abrupt server-side close is surfaced to the client as an error/EOF instead of a disconnect event (or the fd event is missed), client_disconnect_cb is never invoked, so the bit is never set and the per-ep client_user_data is never freed — which additionally trips ASSERT_TRUE(m_ep_client_data.empty()) in cleanup().
  This is a race/timing-sensitive path on a busy, heavily loaded CI worker (the run took ~48 min with several 25–35 s stress tests in parallel), i.e. classic flakiness rather than a regression from this PR. Note: the fetched log is only the tail of the job, so the exact failing assertion line is not visible — the above is inferred from the test source and the failure summary line.

Implicated commit: unknown — not attributable to the PR commit [REDACTED:Hex High Entropy String] (rcache only). Candidates worth checking in the tcp CM disconnect path: 2faf305a "UCS/ASYNC: Filter stale missed fd events (#11458)" (Leonid Genkin) and b82a9203 "UCT/TCP: Bound negative connect test timeout (#11415)" (Yossi Itigin).

File: test/gtest/uct/test_sockaddr.cc:888-902 (test body; failing wait at line 900-901), with the follow-on cleanup assertion at test/gtest/uct/test_sockaddr.cc:174

Suggested fix:

1. Re-run the "gpu on worker 3" job — this failure is unrelated to the rcache change and should not block PR UCS/RCACHE: Adds capability for merging adjacent rcache regions #11958.
2. To stop the recurring flake, make the test tolerant of the abrupt-teardown path: in cm_open_listen_kill_server, wait for TEST_STATE_CLIENT_DISCONNECTED | TEST_STATE_CLIENT_GOT_SERVER_UNAVAILABLE | TEST_STATE_CLIENT_GOT_ERROR (the client legitimately may observe a connection reset rather than a graceful disconnect when the server entity is destroyed), and release any remaining m_ep_client_data entries (release_user_data()) before cleanup() so the secondary ASSERT_TRUE(m_ep_client_data.empty()) doesn't mask the real cause. Optionally raise the wait timeout above the 10 s default for this test only, since the CI worker is heavily loaded.
3. If it reproduces deterministically, instrument src/uct/tcp/tcp_sockcm_ep.c to confirm whether the client's EPOLLHUP/EOF after server socket close reliably results in the disconnect callback, and check interaction with the stale-fd-event filtering added in 2faf305a.

Related: PR #11958 (the PR under test, rcache merge-adjacent); related earlier attempt PR #11803. No existing issue found for cm_open_listen_kill_server flakiness — worth opening one.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id b1304ff5-61c9-4ef9-a9f4-6afeb8175153 in the triage console for the audit trail.