[evgeny-leksikov]

What?

Enable the token failover tests.

Why?

Indicator of "token failover" feature completeness

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

PR-level: the description should back the "token failover is complete" claim. The removed comment said "Disabled until token failover is complete," but the diff changes no product code, so the only evidence a reviewer has is a CI run over the newly enabled ~790 cases. Per REVIEW.md, the "Why?" should explain the value and major changes should not be stale. Please add to the description what makes token failover complete now and link a CI run over the newly enabled cases.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Stale comment: the comment claims "Same cases as test_ucp_fault_tolerance", but the instantiation list below is no longer the full TL set used by the base fixture, so a reader will assume TL parity that no longer exists.

Suggestion:

Suggested change

	/* Same cases as test_ucp_fault_tolerance with UCX_FAILOVER_MODE=token. */
	/* Same cases as test_ucp_fault_tolerance with UCX_FAILOVER_MODE=token, but
	* only on transports that support token based retransmits. */

[svc-nvidia-pr-review]

shm_ib likely collapses onto rcx: with FAILOVER_MODE=token, ucp_wireup_filter_token_tls() keeps only TLs with UCT_IFACE_FLAG_V2_QUERY_TOKEN (today only rc_mlx5), so shm/ud/dc/srd/gga/rc_verbs lanes are dropped and "shm,ib" should select the same rc_mlx5 lanes as "rc_x" — for ~72 extra cases (several using LARGE_MSG_SIZE and keepalive wait loops). What extra coverage does this instantiation add over rcx?

[gleon99]

+1

[svc-nvidia-pr-review]

Both instantiations need mlx5 hardware; on nodes without it check_tls() yields no params and the suites silently vanish, so a green CI run does not by itself prove the newly enabled cases executed. This reinforces the already-reported PR-level request for a CI link.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

maybe add an "ib" or "shm,ib" instantiation as well? with only rc_x, all main-lane TLs already pass ucp_wireup_filter_token_tls(), so the filtering path that token mode relies on is never exercised with a mixed set of TLs.

[svc-nvidia-pr-review]

minor: the new comment could name the actual requirement (rc_mlx5 with DEVX, per UCT_IFACE_FLAG_V2_QUERY_TOKEN) instead of "transports that support token based retransmits" — on a non-DEVX rc_x host the suite silently skips as "Unreachable" rather than reporting an unsupported-mode skip.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests BlueField on worker 1) · commit 6d986597

TL;DR: rcx/test_ucp_fault_tolerance_tf.initiator_failure/3 <rc_x/PUT|FLUSH> aborted on ucs_assert(ucp_ep_config(ep)->key.am_lane != UCP_NULL_LANE) in ucp_ep_recovery_progress(), because on this RMA-only (no UCP_FEATURE_AM) endpoint the failover reconfig is allowed to leave key.am_lane = UCP_NULL_LANE once both AM-capable lanes are marked FAILED, while the recovery state machine unconditionally requires an AM/wireup lane. Fix by making recovery handle am_lane == UCP_NULL_LANE gracefully (give up / fail the EP) instead of asserting, or by keeping an AM lane mandatory for failover-mode EPs regardless of requested features.

Full analysis

Summary: gtest crash (core dumped, make: *** [Makefile:4925: test] Aborted) in the newly enabled token-failover suite: assertion failure in ucp_ep_recovery_progress() during keepalive progress, reached from test_ucp_fault_tolerance::do_put_and_wait().

Root cause: Evidence chain from the log:

1. Test injects failure on lane 0 of the RMA-only PUT|FLUSH endpoint (100 MB PUT, rma_msg_size() → LARGE_MSG_SIZE).
2. The second lane then also errors out: ib_mlx5_log.c:174 UCX ERROR Local protection error on mlx5_1:1/RoCE (synd 0x4 vend 0x52) for RC QP 0x50c6 wqe[2]: RDMA_WRITE ... len 104857600 — so after the token-mode failover purge/replay, the 100 MB RDMA_WRITE is (re)posted on lane 1 with an invalidated/stale key, failing that lane too.
3. With all lanes FAILED, ucp_ep_reconfig_internal() sets cfg_key.am_lane = UCP_NULL_LANE and ucp_ep_config_key_find_am_lane() finds no replacement. The guard at src/ucp/core/ucp_ep.c:1753 only returns UCS_ERR_UNREACHABLE when features & UCP_FEATURE_AM; this variant is registered with UCP_FEATURE_RMA only (test_ucp_fault_tolerance.cc:45), so the config is accepted with no AM lane.
4. Next keepalive round calls ucp_worker_do_ep_keepalive() → ucp_ep_recovery_progress(), which asserts key.am_lane != UCP_NULL_LANE (src/ucp/core/ucp_ep.c:2421; same assert also at :2276 in ucp_ep_recovery_send_request()), because recovery sends UCP_WIREUP_MSG_LANES_ADDR_REQUEST over ucp_ep_get_wireup_msg_lane(), which falls back to am_lane. Abort.

So it is a genuine product-code gap in the token-failover recovery path (not a test-harness/timeout issue); PR #12086 merely enables the test_ucp_fault_tolerance_tf variants that expose it.

Implicated commit: PR #12086 branch commits by Evgeny Leksikov expose it — 647e8406 "TEST/UCP: Enable the token failover tests." and 4b564b7b "TEST/UCP: Keep token failover tests on rc_x only" (merge 6d986597). The underlying defect is in the recovery/token-failover code: [REDACTED:Hex High Entropy String] "UCP/FT: Exchange lane tokens on failover (#12005)" / 4f36f859 "UCP/CORE: Purge held outstanding work with the stored RX token (#12008)" / 40d8c4a6 (#12048), all Evgeny Leksikov.

File: src/ucp/core/ucp_ep.c:2421 (assert), with the enabling gap at src/ucp/core/ucp_ep.c:1753-1758 and the duplicate assert at src/ucp/core/ucp_ep.c:2276

Suggested fix:

1. Make recovery tolerate a missing AM lane instead of asserting. In ucp_ep_recovery_progress() replace the assert with an early bail-out, e.g.:

   if (ucp_ep_config(ep)->key.am_lane == UCP_NULL_LANE) {
       /* No lane left to carry the LANES_ADDR_REQUEST - recovery impossible */
       ucs_diag("ep %p: no AM lane, giving up recovery", ep);
       ucp_ep_set_lanes_failed_schedule(ep, 0, UCS_ERR_ENDPOINT_TIMEOUT);
       ret = 1;
       goto done;
   }

   and add the same guard before ucp_ep_recovery_send_request().
2. Preferably also tighten ucp_ep_reconfig_internal(): for UCP_ERR_HANDLING_MODE_FAILOVER EPs require a surviving AM/wireup lane regardless of UCP_FEATURE_AM, since wireup/recovery messages always need one — i.e. drop the features & UCP_FEATURE_AM condition at line 1753 (or OR it with ucp_ep_err_mode_eq(ep, UCP_ERR_HANDLING_MODE_FAILOVER)).
3. Separately investigate the Local protection error on the non-injected lane for the 100 MB RDMA_WRITE replay — the purge/token-replay path appears to re-post a descriptor whose lkey was invalidated; that is what cascades all lanes into FAILED and should not happen when only lane 0 was invalidated.

Related: PR #12086 (this build), PR #12048, PR #12008, PR #12005, PR #11810 (token-based FT protocols)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id a59b0cc3-320b-459d-88a8-5334a946345f in the triage console for the audit trail.

[evgeny-leksikov]

pulled into #12184