Skip to content

UCT/IB: Add refcounted, TTL-based address handle cache - #11845

Merged
tvegas1 merged 11 commits into
openucx:masterfrom
tvegas1:ah_cache_refcnt
Sep 23, 2026
Merged

tvegas1 merged 11 commits into
openucx:masterfrom
tvegas1:ah_cache_refcnt

Conversation

@tvegas1

@tvegas1 tvegas1 commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

What?

  • AH cache holds a refcnt to each entry; evict (release refcnt and re-query) on TTL expiration, regardless of refcnt.
  • Only ud_v and srd hold a long-term AH reference. The transports (rc_x, ud_x, dc) release any reference right away as they mainly need an AV for connect.
  • ud_v and srd functions is_connected()/ conn_match: compare peer identity (dlid/gid), not AH pointer value.

Important: TTL=0 means full cache bypass, no refcnt.

Why?

The AH cache had no lifecycle, entries were never re-resolved or reclaimed, and is_connected() relied on that by comparing AH pointers directly. Adding refcnt/TTL-based eviction makes pointer comparison unsafe for TTL=0, so identity-based comparison is needed too.

Cache use-case: quickly reconnecting endpoints speedup.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/ud/verbs/ud_verbs.c Outdated
Comment thread src/uct/ib/base/ib_device.c
@svc-nvidia-pr-review

Copy link
Copy Markdown

Coverage note: The added gtests (test_uct_ib_ah_cache, test_ud_verbs_ah_cache, test_ud_verbs_is_connected) cover refcount sharing, TTL reuse/re-query, TTL=0, hold, and ep-churn refcount balance. They do not cover the disconnect-linger resend path that the blocker concerns — a test that destroys a ud_verbs ep with an outstanding/unacked TX window (forcing a resend after destroy) would exercise it.

@svc-ucx

svc-ucx commented Aug 27, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 0) · commit 8247b977

TL;DR: The UD-verbs async progress thread segfaults dereferencing ep->ah_entry (NULL) at ud_verbs.c:119, because the new AH-cache commit releases the endpoint's AH reference in uct_ud_verbs_ep_destroy() while the UD endpoint object deliberately stays alive after uct_ep_destroy() and keeps sending ACKs. Fix: don't NULL the AH in ep_destroy (leave it to the class cleanup func), or guard/short-circuit sends when ah_entry == NULL.

Full analysis

Summary: contrib/test_jenkins.sh exited 139 — ucx_info -epw -u t and examples/ucp_hello_world both crashed with SIGSEGV (signal 11, "address not mapped to object at address (nil)") in uct_ud_verbs_post_send() on the UCS async progress thread.

Root cause: Both backtraces are identical and deterministic:

uct_ud_verbs_post_send()   src/uct/ib/ud/verbs/ud_verbs.c:119
uct_ud_verbs_ep_tx_skb()   ud_verbs.c:149
uct_ud_verbs_ep_send_ctl() ud_verbs.c:187
uct_ud_iface_send_ctl()    ud_iface.h:543
uct_ud_ep_send_ack()       ud_ep.c:1591
uct_ud_ep_do_pending()     ud_ep.c:1695
ucs_arbiter_dispatch_nonempty() ... uct_ud_verbs_iface_async_handler()

Line 119 is wr->wr.ud.ah = ep->ah_entry->ah;. The faulting address is (nil), i.e. ep->ah_entry == NULL (a stale/freed entry would give a non-zero garbage address).

ah_entry is a brand-new field added by the HEAD commit (ud_verbs.h:28). The commit also added:

static void uct_ud_verbs_ep_destroy(uct_ep_h tl_ep)
{
    uct_ud_ep_disconnect(tl_ep);
    uct_ud_verbs_ep_release_ah(ucs_derived_of(tl_ep, uct_ud_verbs_ep_t)); /* sets ah_entry = NULL */
}

(ud_verbs.c:82-86, releasing at ud_verbs.c:63-74)

This is unsafe for UD: uct_ud_ep_disconnect() explicitly does not free the endpoint — see its own comment at src/uct/ib/ud/base/ud_ep.c:1886-1892 "the EP will be destroyed by interface destroy or timeout in uct_ud_ep_timer". The object stays in iface->eps and in iface->tx.pending_q, and dest_ep_id remains set so uct_ud_ep_is_connected() (guard at ud_ep.c:1544) still returns true. Consequently the async thread later dispatches a pending UCT_UD_EP_OP_CTL_ACK on the "destroyed" ep and dereferences the now-NULL ah_entry. Previously the AH lived in the device-wide AH cache and was never dropped per-endpoint, so this path was safe.

Secondary defect: uct_ud_verbs_ep_release_ah() is called from uct_ud_verbs_ep_destroy() outside uct_ud_enter()/uct_ud_leave(), so even with correct lifetime it races with the async progress thread.

Implicated commit: 8247b977 — "UCT/IB: Add refcounted, TTL-based address handle cache", Thomas Vegas (this is the PR HEAD, [REDACTED:Hex High Entropy String])

File: src/uct/ib/ud/verbs/ud_verbs.c:82-86 (premature release) and src/uct/ib/ud/verbs/ud_verbs.c:119 (unguarded deref)

Suggested fix:

  1. Remove the uct_ud_verbs_ep_release_ah() call from uct_ud_verbs_ep_destroy(). The AH reference must be dropped only in the class cleanup func (UCS_CLASS_CLEANUP_FUNC(uct_ud_verbs_ep_t), ud_verbs.c:76-80), which runs when the EP is actually freed by the iface destroy / deferred-timeout path (uct_ud_ep_deferred_timeout_handler, ud_ep.c:284-288). uct_ud_verbs_ep_destroy can then simply be uct_ud_ep_disconnect(tl_ep).
  2. If an early release is genuinely required, it must (a) hold uct_ud_enter(&iface->super) around release_ah, and (b) be paired with a defensive check so no send can occur without an AH — e.g. in uct_ud_verbs_ep_send_ctl()/post_send():
    if (ucs_unlikely(ep->ah_entry == NULL)) {
        return iface->tx.send_sn;  /* no AH: drop ctl packet */
    }
    plus an ucs_assertv(ep->ah_entry != NULL, "ep=%p", ep) in uct_ud_verbs_post_send() so debug builds catch this instead of segfaulting.
  3. Also audit uct_ud_verbs_ep_resolve_peer_address() (ud_verbs.c:661-664): it calls release_ah() before uct_ib_iface_ah_get(), leaving a window where ah_entry == NULL; if ah_get() fails the EP is left permanently AH-less. Get the new entry into a temporary and only release the old one after success.

Related: none found (no existing issue/PR matches this signature)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 9ff409da-aec1-451c-8ca5-9270fe69bbad in the triage console for the audit trail.

@tvegas1 tvegas1 added the WIP-DNM Work in progress / Do not review label Aug 27, 2026
Replace the AH cache's simple create-or-lookup semantics with
refcounted entries that carry an idle timestamp. uct_ib_device_ah_get()
shares an entry while it's referenced and re-queries it once idle past
the new UCX_IB_AH_CACHE_TTL (default inf; 0 disables the cache and
hands out private, unshared AHs). Callers release with ah_put(), or
duplicate a reference with ah_hold() for independently-released
holders.

Port PR openucx#11825's identity-based is_connected()/conn_match for UD verbs
(compare dlid/gid instead of AH pointers) onto the refcounted cache,
and give EFA/SRD the same treatment since it also holds an AH for the
life of the endpoint. Other AH consumers (UD mlx5, RC mlx5 DEVX, DC
pure-grant, mlx5 compact-AV probe) switch to a transient get+put around
uct_ib_mlx5_get_av().
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/base/ib_device.c Outdated
Comment thread src/uct/ib/base/ib_md.c
@svc-ucx

svc-ucx commented Sep 1, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Build build_aarch64 sles15sp7_aarch64) · commit 0df03aae

TL;DR: The build_aarch64 sles15sp7_aarch64 RPM build failed compiling src/uct/ib/base/ib_device.c with -Werror=maybe-uninitialized on ah in the new uct_ib_device_ah_get(); initialize struct ibv_ah *ah = NULL; (or restructure the error paths) to silence the older-GCC false positive.

Full analysis

Summary: Compilation of src/uct/ib/base/ib_device.c aborted the SLES15-SP7 aarch64 RPM build: base/ib_device.c:1530:13: error: 'ah' may be used uninitialized in this function [-Werror=maybe-uninitialized] → cc1: all warnings being treated as errors → make[4]: *** [Makefile:850: base/libuct_ib_la-ib_device.lo] Error 1 → error: Bad exit status from /var/tmp/rpm-tmp.KdfnAE (%build).

Root cause: The new refcounted/TTL AH cache added uct_ib_device_ah_get(), which declares struct ibv_ah *ah; uninitialized (line 1487) and only assigns it indirectly through the inlined helper uct_ib_device_create_ah(..., &ah) (line 1451). On the older GCC in the sles15sp7_aarch64 container, with -O3 plus the multiple goto unlock error paths, the compiler's flow analysis cannot prove that ah is written before the ibv_destroy_ah(ah) call in the entry == NULL branch (line 1530), so it emits -Wmaybe-uninitialized. Since UCX builds with -Wall -Werror, this warning is fatal. It is a compiler false positive (the value is always set when status == UCS_OK), but it still breaks this toolchain — other, newer-GCC jobs would not flag it, which is why only the aarch64/SLES job fails.

Implicated commit: [REDACTED:Hex High Entropy String] — Thomas Vegas, "UCT/IB: Add refcounted, TTL-based address handle cache" (the PR head commit itself)

File: src/uct/ib/base/ib_device.c:1487 (declaration) — warning reported at src/uct/ib/base/ib_device.c:1530

Suggested fix: Initialize the pointer at declaration so the compiler can never see an unset path:

struct ibv_ah *ah = NULL;

Optionally also tidy the duplicated cleanup in that block (lines 1528–1542) so the AH/entry teardown happens in one place, e.g. build the entry with refcount = 1, kh_put first, then bump to 2 on success, and use a single err_destroy_ah: label that calls ibv_destroy_ah(ah)/ucs_free(entry) — fewer paths through ah also removes the source of the false positive. Note the same pattern appears twice (ibv_destroy_ah(ah) at 1500, 1530, 1538), so a single-exit cleanup is the more durable fix.

Related: c8495100 "UCT/IB: Add configurable TTL knob for the AH cache (#11847)" touches the same code; no existing issue found for this warning.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id cbdc5424-d5d3-4dd6-bc07-ff543676fba6 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@tvegas1 tvegas1 removed the WIP-DNM Work in progress / Do not review label Sep 1, 2026
@svc-ucx

svc-ucx commented Sep 1, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (EFA_Tests EFA on rhel90_ib) · commit c4b52d95

TL;DR: The new gtest test_uct_ib_ah_cache.past_ttl_is_requeried fails on the EFA/srd interfaces because it compares a stale struct ibv_ah * pointer across a destroy→create boundary; the freed AH memory is immediately reused by the new ibv_create_ah(), so EXPECT_NE(ah1, ah2) compares equal. Fix the test to assert on an AH-creation counter (or on cache state) instead of on a dangling pointer.

Full analysis

Summary: "Run gtests" step of EFA on rhel90_ib failed with 2 failures out of 8330: srd/test_uct_ib_ah_cache.past_ttl_is_requeried/0 and /1 (both srd/rdmap params); all other AH-cache tests, including the ud_verbs instantiations, passed.

Root cause: In past_ttl_is_requeried (test/gtest/uct/ib/test_ib.cc:386-407) the test:

  1. gets a cached AH and saves ah1 = entry->ah,
  2. drops its reference so only the cache's own hold remains,
  3. forces entry->creation_time = 0 to make the entry stale,
  4. calls uct_ib_iface_ah_get() again and asserts EXPECT_NE(ah1, ah2).

On the stale path in uct_ib_device_ah_get() (src/uct/ib/base/ib_device.c:1494-1507) the refcount drops to 0, so uct_ib_ah_entry_release() (ib_device.c:1468-1475) calls ibv_destroy_ah(entry->ah) and ucs_free(entry) before the new AH is created a few lines later (ib_device.c:1520). ah1 is therefore a dangling pointer, and the provider's ibv_create_ah() allocation immediately reuses the just-freed block, so the new entry->ah has the identical address. The comparison is a use-after-free identity check whose outcome depends on the provider's allocation size/pattern — it happens to be deterministic for the efa provider (both rdmap0 and rdmap1 failed) while it happened to differ for the mlx5-backed ud_verbs instantiation. Corroborating evidence: the sibling test referenced_past_ttl_keeps_old_entry_valid does the same pointer comparison but keeps a reference so the old AH is not destroyed — and it passed.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/IB: Add refcounted, TTL-based address handle cache", Thomas Vegas (also 0df03aa, same series)

File: test/gtest/uct/ib/test_ib.cc:405 (EXPECT_NE(ah1, ah2)); relevant library path src/uct/ib/base/ib_device.c:1468-1520

Suggested fix:

  1. Remove the freed-pointer comparison from past_ttl_is_requeried. Instead observe the re-query through state that stays valid, e.g. add a monotonic counter to uct_ib_device_t (e.g. ah_created / ah_destroyed, bumped in uct_ib_device_create_ah() and uct_ib_ah_entry_release()), and assert ah_created incremented by 1 and ah_destroyed incremented by 1 after the stale get; optionally also assert kh_size(&dev()->ah_hash) == 1 and that the new entry's creation_time != 0.
  2. Alternatively, keep the test observable-safe by comparing only live objects: hold an extra reference before forcing staleness (as referenced_past_ttl_keeps_old_entry_valid does) and cover the "unreferenced ⇒ destroyed" case via the counter above.
  3. Minor hardening in the library while you're there: in uct_ib_device_ah_get() call kh_del() before uct_ib_ah_entry_release() (ib_device.c:1504-1505) so the hash never briefly holds a pointer to a freed entry.

Related: PR #11845 (this PR); prior related PR #11847 "UCT/IB: Add configurable TTL knob for the AH cache" (c849510)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 5f9d7098-db08-48c9-8ff3-a7c0c7c08e27 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@svc-ucx

svc-ucx commented Sep 2, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests roce on worker 2) · commit 3b1176a8

TL;DR: The RoCE gtest shard hung — the very first test's connection handshake (send_recv_am(1)) never completed over rc_x, so ep close/flush spun until the 15‑min gtest watchdog aborted the process; the only functional change in this PR is the new refcounted/TTL address‑handle cache, which now destroys and re‑creates RoCE AHs (default UCX_IB_AH_CACHE_TTL=1s) whose address‑vector snapshots are still in use by connected QPs. Restore the previous lifetime semantics (default TTL inf, and never destroy an AH whose derived AV/QP is still alive) and re‑run the RoCE job.

Full analysis

Summary: rcx/test_ucp_proto_mock_rcx.memtype_copy_enable/0 hung on the RoCE worker: the initial 1‑byte AM never completed, ctx.received stayed false, and cleanup spun in close_all_eps() until the watchdog fired (Connection timed out - abort testing, SIGABRT, core dumped).

Root cause: This is a hang, not a slow test. Timeline from the log: test starts 14:41:23 → 3 min of total silence → 14:44:24 ucp_test.cc:300 UCX ERROR request 0x...1058 did not complete on time → 14:44:34 ctx.received == false → 14:47:34 second request timeout → 14:59:34 watchdog abort, backtrace stuck in ucp_test::progress() from ucp_test_base::entity::close_all_eps() (flush never completes). Nothing was ever delivered over rc_x/RoCE and no transport error was reported — a silent black hole, which on RoCE means the QP was programmed with a bad/stale destination (rmac/rgid/udp_sport). That data comes from an ibv_ah, and this branch is the only thing that changed AH lifetime: uct_ib_device_ah_get() now hands out refcounted entries with a TTL (default AH_CACHE_TTL=1s, src/uct/ib/base/ib_md.c:207, applied at ib_md.c:1408) and destroys the ibv_ah on eviction (ib_device.c:1468-1507), whereas master cached AHs for the whole device lifetime. All mlx5 consumers snapshot the AV and immediately drop their reference (rc_mlx5_devx.c:421-422, ud_mlx5_common.c:60-61, ib_mlx5.c:407-408), so nothing keeps the AH alive for the lifetime of the QP/UD ep that was configured from it. Secondary defect in the same commit: uct_ib_device_cleanup_ah_cached() (ib_device.c:712-725) destroys and frees entries regardless of refcount (only warns), which can leave uct_ud_verbs_ep_t::ah_entry dangling for eps that outlive md close.

Implicated commit: c4b52d9 (with its predecessor 0df03aa), Thomas Vegas — “UCT/IB: Add refcounted, TTL-based address handle cache” (plus the knob added in c849510 / #11847)

File: src/uct/ib/base/ib_device.c:1477-1552 (uct_ib_device_ah_get / TTL eviction), src/uct/ib/base/ib_md.c:207 (default AH_CACHE_TTL=1s), src/uct/ib/mlx5/rc/rc_mlx5_devx.c:414-422

Suggested fix:

  1. Change the default to AH_CACHE_TTL=inf (ib_md.c:207) so the out-of-the-box behavior matches master (AH cached for device lifetime); make the 1s TTL opt-in until it is validated on RoCE.
  2. Never destroy an AH that anything still derives state from: hold the reference for as long as the object built from the AV lives (store the uct_ib_ah_entry_t* in the DEVX QP / UD ep and ah_put() on destroy) instead of putting it right after uct_ib_mlx5_get_av() in rc_mlx5_devx.c:422 and ud_mlx5_common.c:61; on TTL eviction only drop the cache reference and skip re-creation while refcount > 1.
  3. In uct_ib_device_cleanup_ah_cached() (ib_device.c:712-725), don't free entries with refcount != 1 — leak-and-warn or assert instead of freeing memory that eps still point to.
  4. Reproduce/confirm locally on a RoCE node: run rcx/test_ucp_proto_mock_rcx.* with UCX_IB_AH_CACHE_TTL=inf vs 1s; if only the latter hangs, that confirms the eviction path.

Related: PR #11845 (this branch), knob PR #11847

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@svc-ucx

svc-ucx commented Sep 4, 2026

Copy link
Copy Markdown

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

TL;DR: The Coverity stage failed with 2 new CHECKED_RETURN defects at test/gtest/uct/ib/test_ud_ds.cc:30-31, where uct_iface_get_address()'s return value is ignored; wrap both calls in ASSERT_UCS_OK(...).

Full analysis

Summary: coverity devel on coverity_rh7 — the build and cov-analyze succeeded (739/739 compilation units, 1568 total occurrences), but the gate script found nerrors=2 and exited 2: ##[error]Coverity found 2 issues: both CHECKED_RETURN in test/gtest/uct/ib/test_ud_ds.cc.

Root cause: In test_ud_ds::init(), lines 30 and 31 call uct_iface_get_address(m_e1->iface(), ...) / (m_e2->iface(), ...) and discard the ucs_status_t result, while the adjacent uct_iface_get_device_address() calls are checked with ASSERT_UCS_OK. Coverity's statistical CHECKED_RETURN checker reports this because the return value "is checked elsewhere 12 out of 15 times" (evidence cited from examples/uct_hello_world.c:656, src/tools/perf/lib/libperf.c:729, src/ucp/wireup/address.c:1365, test/gtest/uct/ib/test_dc.cc:193, test/gtest/uct/ib/test_ib_pkey.cc:213). The two offending lines are old code, but this PR's branch adds new checked call sites of uct_iface_get_address in the AH-cache tests (test/gtest/uct/ib/test_ib.cc, commits 3b1176a8/0df03aae, Thomas Vegas), which pushed the checked/unchecked ratio over Coverity's anomaly-reporting threshold — so the pre-existing unchecked calls became newly reported defects on this PR. This is a real (if benign-in-test) unchecked-status issue, not a Coverity infrastructure problem.

Implicated commit: Defect site is pre-existing (test_ud_ds.cc last touched by 3c7b2dd7, Yossi Itigin); the change that made it visible is 3b1176a8 / 0df03aae "UCT/IB: Add refcounted, TTL-based address handle cache" by Thomas Vegas (PR branch head [REDACTED:Hex High Entropy String]).

File: test/gtest/uct/ib/test_ud_ds.cc:30-31

Suggested fix: Check the status at both call sites, matching the style used a few lines below:

ASSERT_UCS_OK(uct_iface_get_address(m_e1->iface(),
                                    (uct_iface_addr_t*)(void*)&if_adr1));
ASSERT_UCS_OK(uct_iface_get_address(m_e2->iface(),
                                    (uct_iface_addr_t*)(void*)&if_adr2));

Add this as a small commit on the ah_cache_refcnt branch (or a separate cleanup PR to master) and re-run the Coverity job; no suppression/annotation is needed.

Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache); prior related PR #11847 (AH cache TTL knob).

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 88bf3bda-733f-4a0b-825d-49fdf20cc558 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@svc-ucx

svc-ucx commented Sep 7, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests roce on worker 2) · commit 8845b7d0

TL;DR: The "roce on worker 2" job did not fail on any test or compile error — the Azure DevOps agent on agent-01 received a shutdown signal mid-build while still compiling gtest objects, which cancelled the step. Re-run the pipeline; no code change in PR #11845 is implicated.

Full analysis

Summary: Run ./contrib/test_jenkins.sh (job "roce on worker 2", build 134647) was cancelled during the gtest compile phase by an agent shutdown, before any UCX test executed.

Root cause: Infrastructure/agent-level cancellation, not a product or test failure. The log shows a clean, continuously progressing build (make compiling test/gtest/** objects with per-line gaps of only 1–13 seconds, e.g. uct/ib/test_cq_moderation.o finishing at 10:36:14) and then:

  • 10:36:11.888 ##[error]The agent has received a shutdown signal. This can happen when the agent service is stopped, or a manually started agent is canceled.
  • 10:36:12.228 ##[error]The Operation will be canceled. The next steps may not contain expected logs.
  • 10:36:22.348 ##[error]The operation was canceled. followed by Start cleaning up orphan processes and SIGTERM of test_jenkins.sh, make, g++, cc1plus.

There is no compiler error, no -Werror diagnostic, no gtest output, and no large timestamp gap anywhere in the log — so this is neither a hang nor a legitimate "needs more wall time" case. The build was still in the compile stage (it never reached the ROCE test execution) when the hosting agent went away, meaning the agent service was stopped/restarted or the build was cancelled externally (e.g. superseded by a newer push to ah_cache_refcnt, or agent host maintenance on /scrap/azure/agent-01).

Implicated commit: none — commit [REDACTED:Hex High Entropy String] is not implicated; the failure precedes any code execution.

File: N/A (no source file implicated; cancellation reported in the Run ./contrib/test_jenkins.sh step log at 10:36:11)

Suggested fix: Re-trigger the "UCX PR" pipeline for PR #11845 (or just the "roce on worker 2" job). If it recurs on the same host, ask CI ops to check the health/uptime of the agent-01 Azure DevOps agent service and free space on [REDACTED:Base64 High Entropy String] — repeated mid-compile agent shutdowns on one worker usually indicate agent-service restarts or host-level resource pressure rather than a UCX defect. Do not raise any timeout: the log shows uninterrupted progress up to the kill, so a time-limit change would not address this.

Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache) — the PR under test; no related issue found for this cancellation signature.

🛡️ This comment had 2 potential secret(s) redacted (Base64 High Entropy String, Hex High Entropy String). See request_id ca993cfc-d395-4471-a814-d37327c8742d in the triage console for the audit trail.

@tvegas1
tvegas1 requested a review from brminich September 8, 2026 12:57
@tvegas1

tvegas1 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@yosefe, @brminich, can you please check this PR, it is the generalization of the cached AH TTL for all users.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/base/ib_md.c
Comment thread src/uct/ib/ud/verbs/ud_verbs.c
Comment thread src/ucs/datastruct/conn_match.h
Comment thread src/uct/ib/base/ib_device.c
Comment thread test/gtest/uct/ib/test_ib.cc Outdated
Comment thread test/gtest/uct/ib/test_ud.cc
@svc-ucx

svc-ucx commented Sep 11, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (EFA_Tests EFA on rhel90_ib) · commit d43655f7

TL;DR: No UCX test actually failed — the self-hosted Azure agent running the EFA on rhel90_ib job was shut down mid-run ("The agent has received a shutdown signal"), aborting the gtest step; just re-run the job.

Full analysis

Summary: The "Run gtests" step of the EFA_Tests job was canceled at 21:02:21 by an agent shutdown signal, not by a failing assertion or crash.

Root cause: Infrastructure/agent abort. The log shows continuous, healthy test progress with no stalls — the last ~4 minutes contain back-to-back [ OK ] results (srd/uct_flush_test, srd/uct_cancel_test, srd/test_many2one_am, ud_verbs/uct_p2p_am_test, srd/uct_p2p_am_test), with the largest inter-line gap being ~18 s (srd/uct_cancel_test.am_zcopy/0, which is normal under valgrind). At 21:02:21.55 Azure DevOps emitted ##[error]The agent has received a shutdown signal. This can happen when the agent service is stopped, or a manually started agent is canceled. followed by ##[error]The Operation will be canceled and, 10 s later, ##[error]The operation was canceled. Tests kept passing even after the shutdown signal (e.g. srd/uct_p2p_am_test.am_async_response/3 OK at 21:02:31) until the process tree was killed — the cleanup step reaped test_efa.sh, make, and memcheck-amd64- as orphans. There is zero evidence of a hang, timeout, assertion, valgrind error report, or segfault, and nothing points at the PR's address-handle cache changes (no uct_ib/AH-cache tests failed; all AH-touching srd/ud_verbs suites passed).

Implicated commit: unknown — not a code regression; [REDACTED:Hex High Entropy String] (PR #11845) is not implicated by any log evidence.

File: unknown (no test failure or source line reported)

Suggested fix: Re-run the EFA_Tests / EFA on rhel90_ib job. If it recurs, this is a CI-fleet problem, not a PR problem: check the rhel90_ib EFA agent host for the Azure agent service being restarted/deregistered, VM reclamation or reboot, and OOM kills (valgrind memcheck on the EFA gtest suite is memory-hungry — check dmesg/journalctl on the agent for oom-killer entries around 2026-09-11 21:02 UTC). Also confirm the build wasn't superseded by a newer push to ah_cache_refcnt, since Azure cancels in-flight PR builds on new commits. Do not raise any step timeout — the log shows no stall.

Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache) — the PR under test; no related failure issue found.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 9599b67e-fe5b-4ac1-abcc-8c65a78a080e in the triage console for the audit trail.

- Reuse the existing entry variable in the stale eviction path instead of
  repeating the kh_value() lookup.
- Override create_connected_entities() in test_uct_ib_ah_cache, matching
  test_uct_ib_perf, so init() is not duplicated.
- Drive the AH refcount wait with short_progress_loop(), like
  wait_for_ep_destroyed(), so linger timers are swept by progress.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/efa/srd/srd_ep.c
Comment thread src/uct/ib/base/ib_device.c Outdated
Destroying the AH and freeing the entry unconditionally left any remaining
holder with a dangling pointer, and a double free on its ah_put(). Drop only
the cache's own reference, so a still-referenced entry stays valid.
Now that cached AHs can be destroyed, releasing an AH while the device may
still consume a posted WQE would let the HW read a freed address handle.

Give a send operation ownership of one AH reference whenever it outlives its
sender endpoint:
- ep send op purge takes a reference for each orphaned operation, released
  when its send completion is polled.
- A posted control operation hands its reference to the send op, instead of
  releasing it right after the post.
- iface cleanup releases those references only after the QP is destroyed.
@svc-ucx

svc-ucx commented Sep 14, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests roce on worker 2) · commit 31b9ea10

TL;DR: The roce on worker 2 gtest run failed on exactly one of 8736 tests — rc/test_ucp_am_nbx_seg_size.multi/3 (rc_v/proto_v1/reply) — a randomized multi-fragment AM/reply test; nothing in the log ties it to this PR's IB AH-cache change, so it should be re-run and, if it reproduces, chased in the UCP AM multi-fragment reply path (the assertion text itself is outside the retrievable log tail).

Full analysis

Summary: make test in build-test/test/gtest exited 1 because a single gtest case failed: rc/test_ucp_am_nbx_seg_size.multi/3, where GetParam() = rc_v/proto_v1/reply; the other 8735 tests passed and the suite finished tear-down normally (no hang, no crash, no timeout — the run completed in 3446 s and exited via make: *** [Makefile:4713: test] Error 1).

Root cause: From the evidence available, this is an isolated failure in the multi-fragment eager AM path with UCP_AM_SEND_FLAG_REPLY under proto v1 over rc_verbs, not an infrastructure or build problem. The test is inherently randomized: test_ucp_am_nbx_seg_size::init() picks m_size = ucs_max(UCS_KBYTE, ucs::rand() % (64 * UCS_KBYTE)), re-applies it as IB_SEG_SIZE/MM_SEG_SIZE/TCP_SEG_SIZE, creates a third entity as the new sender() and connects it to receiver(); multi then sends 2 * seg_size bytes, so the exact fragment boundaries (and hence which fragment/footer edge case is exercised) differ on every run.

I could not attribute it to the PR: the changes on ah_cache_refcnt (0df03aae/c4b52d95/d43655f7/31b9ea10, Thomas Vegas) touch only the device-level AH cache (uct_ib_device_ah_get/put/hold, ah_hash keyed on struct ibv_ah_attr, ah_cache_ttl — src/uct/ib/base/ib_device.h:260-263) and IB endpoint peer identity. That cache serves ibv_create_ah users (UD/DC); rc_verbs connects via ibv_modify_qp. In the very same run all AH-sensitive suites passed (ud_mlx5/test_uct_peer_failure, ud_mlx5/test_uct_loopback, dc_ud/test_ucp_sockaddr_with_wakeup, dcx/test_ucp_wireup_errh_peer, rc_verbs/test_uct_peer_failure_multiple), which argues against a broken AH path.

Caveat, stated explicitly: the gtest failure message/assertion for the failing case is not in the retrievable log — the Azure task log is truncated to the last ~500 lines (last ~70 seconds of a 57-minute shuffled run), so I cannot show whether it was a data-pattern mismatch, a counter mismatch, or a wait_receives() timeout.

Implicated commit: unknown — no log evidence links the failure to 31b9ea10 (Thomas Vegas) or the other ah_cache_refcnt commits.

File: test/gtest/ucp/test_ucp_am.cc:1710 (failing case; randomized seg size at test/gtest/ucp/test_ucp_am.cc:1674)

Suggested fix: 1) Re-run the roce on worker 2 job — a single randomized-parameter failure out of 8736 with all related suites green is the signature of a flaky test, not of the AH-cache change. 2) To get an actionable diagnosis if it recurs, capture the assertion: download the full raw task log from Azure DevOps (_apis/build/builds/135935/logs/{logId}) instead of the truncated view, or have CI publish the gtest XML/--gtest_output=xml. 3) If it reproduces, reproduce locally with a pinned seg size and UCX_TLS=rc_v UCX_PROTO_ENABLE=n plus UCP_AM_SEND_FLAG_REPLY (GTEST_FILTER=rc/test_ucp_am_nbx_seg_size.multi/3) and inspect the reply-footer/length accounting in src/ucp/core/ucp_am.c (see prior fixes 328841f7 "Fix calculation of message length in reply handler" and bcc7d08f). 4) Do not raise any time limit — the suite ran to completion with continuous output; there is no hang.

Related: PR #11845 (this PR, "UCT/IB: Add refcounted, TTL-based address handle cache"); no existing issue found for test_ucp_am_nbx_seg_size.multi flakiness.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-ucx

svc-ucx commented Sep 14, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 0) · commit 8e6e3fc6

TL;DR: The RoCE ASan gtest job hung for 11+ minutes inside all/test_ucp_sockaddr_cm_private_data.short_cm_private_data_fallback_to_next_cm/3 — the tag message was sent but never arrived (recv_data == 0), i.e. UD wireup/data traffic was silently dropped, pointing at this PR's new refcounted/TTL AH cache and the removal of the UD endpoint's own peer-address copy.

Full analysis

Summary: test_ucp_sockaddr.cc:594 failed after request ... did not complete on time in the all/tag,sa_data_v1,mt RoCE variant, then the whole gtest binary aborted with "Connection timed out - abort testing" (make: *** [Makefile:4713: test] Aborted).

Root cause: Log evidence: last activity at 12:20:46.397 ("server listening on 1.1.1.1:33487"), then complete silence until 12:32:03.148 when UCX printed ucp_test.cc:300 UCX ERROR request 0x[REDACTED:Hex High Entropy String] did not complete on time, with send_data=1473566375073475996 vs recv_data=0. An 11m17s gap with zero output = a hang, not a slow test; the send request completed locally but the peer never received the data and no transport error was raised, which is the signature of packets being posted to an invalid/stale address handle or of a UD endpoint that is no longer matched by connection-matching (UD is the wireup/aux transport for the IB/RoCE lanes in this test).

That is exactly the machinery this branch rewrites:

  • src/uct/ib/base/ib_device.c:1454-1575 — AHs are no longer immortal per-device; they are refcounted and evicted by TTL, so ibv_destroy_ah() can now run while consumers still use the address.
  • src/uct/ib/mlx5/ud/ud_mlx5_common.c:54-61 and src/uct/ib/mlx5/ib_mlx5.c:401-408 — the AH reference is dropped immediately after the AV is copied out (uct_ib_iface_ah_get() … uct_ib_iface_ah_put()); nothing keeps the entry alive, so a TTL eviction destroys the AH that UD-mlx5 endpoints are still sending with. No error is reported on such sends, matching the silent drop.
  • src/uct/ib/ud/verbs/ud_verbs.c:63-90,641-656 + src/uct/ib/ud/base/ud_iface.c:157-173 — the connection-match (CEP) key is now reconstructed from ep->ah_entry (ucs_assert(ep->ah_entry != NULL)), instead of the endpoint keeping its own peer address. If ah_entry is NULL/replaced, cep_remove_ep() computes the wrong key, the dead endpoint stays in the conn-match table, and a later CREQ is matched to it — again silent packet loss and a hang.

The commit [REDACTED:Hex High Entropy String] UCT/IB: Release cache AH reference on device cleanup (same day as this build) shows the refcounting was still being corrected, so an imbalance/premature destroy in this area is the most probable trigger. Do not raise the gtest timeout — this is a hang.

Implicated commit: 0df03aae / c4b52d95 "UCT/IB: Add refcounted, TTL-based address handle cache", d43655f7 "UCT/IB: Avoid duplicating peer identity in endpoints", [REDACTED:Hex High Entropy String] "UCT/IB: Release cache AH reference on device cleanup" — all Thomas Vegas (branch ah_cache_refcnt, PR #11845)

File: src/uct/ib/base/ib_device.c:1486-1575 (AH get/put/TTL eviction); src/uct/ib/mlx5/ud/ud_mlx5_common.c:54-61; src/uct/ib/ud/verbs/ud_verbs.c:642-656; failure asserted at test/gtest/ucp/test_ucp_sockaddr.cc:594

Suggested fix:

  1. Confirm the trigger cheaply: rerun the failing test with UCX_IB_AH_CACHE_TTL=inf (never evict) and with the cache disabled (=0). If it only hangs with a finite TTL, the eviction/refcount path is the culprit.
  2. Stop dropping the reference for AVs copied out of an AH: in uct_ud_mlx5_iface_get_av() keep the uct_ib_ah_entry_t in the endpoint (as ud_verbs does) and release it in the ep cleanup, or make TTL eviction only unlink entries whose refcount is 1 (cache-only) and never destroy an AH that a consumer copied an AV from.
  3. Don't derive the connection-matching key from ep->ah_entry: restore the endpoint's own immutable peer_address copy (dlid/dgid/is_global/dest_qpn) for cep_insert/remove, or at minimum handle ah_entry == NULL explicitly instead of asserting, so an endpoint can never be left behind in conn_match_ctx.
  4. Add a gtest that creates a UD/RoCE connection, forces AH TTL expiry (small UCX_IB_AH_CACHE_TTL), and then sends on the already-connected endpoints — that reproduces the "AH destroyed while in use" window deterministically.

Related: PR #11845 (this PR), PR #11847 "UCT/IB: Add configurable TTL knob for the AH cache"

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4d247aeb-bf32-4f3e-8aad-7ee6db6adca6 in the triage console for the audit trail.

brminich
brminich previously approved these changes Sep 18, 2026
Comment thread src/uct/ib/efa/srd/srd_ep.c Outdated
Comment thread src/uct/ib/base/ib_device.c Outdated
Comment thread src/uct/ib/base/ib_device.c Outdated
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/base/ib_device.c Outdated
Comment thread src/uct/ib/ud/verbs/ud_verbs.c
Route every AH destruction through a helper that warns on failure, as
destructors cannot propagate a status to the caller.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/ib/ud/verbs/ud_verbs.c
Comment thread src/uct/ib/base/ib_md.c
Comment thread src/uct/ib/efa/srd/srd_def.h
@tvegas1

tvegas1 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Tested gtest on EFA UD/SRD with:

ud*/test_uct_ib_ah_cache.*:srd*/test_uct_ib_ah_cache.*:
ud*/test_ud_verbs_ah_cache.*:ud*/test_ud_verbs_is_connected.*:
test_conn_match.*:ud*/test_ud_ds.*:
ud*/test_ucp_ep.*:srd*/test_ucp_ep.*:
ud*/test_ucp_sockaddr_conn_request.*:srd*/test_ucp_sockaddr_conn_request.*:
ud*/uct_p2p_am_test.*:srd*/uct_p2p_am_test.*:
ud*/uct_p2p_am_misc.*:srd*/uct_p2p_am_misc.*:
ud*/test_many2one_am.*:srd*/test_many2one_am.*:
ud*/test_uct_ib_pkey.*:srd*/test_uct_ib_pkey.*:
ud*/test_uct_ib_sl.*:srd*/test_uct_ib_sl.*:
ud*/test_uct_ib_addr.*:srd*/test_uct_ib_addr.*:
ud*/test_uct_ib_lmc.*:srd*/test_uct_ib_lmc.*:
ud*/test_uct_ib_gid_idx.*:srd*/test_uct_ib_gid_idx.*:
ud*/test_uct_ib_roce.*:srd*/test_uct_ib_roce.*:
ud*/test_uct_ep.*:srd*/test_uct_ep.*:
ud*/uct_flush_test.*:srd*/uct_flush_test.*:
ud*/uct_cancel_test.*:srd*/uct_cancel_test.*

@tvegas1
tvegas1 merged commit f53da7f into openucx:master Sep 23, 2026
162 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants