UCP/WIREUP: Add a token trailer and an ACK message to the lane address exchange

[evgeny-leksikov]

What?

Append an optional token trailer to the LANES_ADDR wireup messages and add the
LANES_ADDR_ACK message which closes the exchange. The trailer is framing only:
its token sections are present but empty, and no token is queried or consumed yet.

Why?

Groundwork for hardware PSN fault tolerance. Purging a failed UCT lane requires the
PSN tokens of both directions, which have to travel with the lane addresses that
recovery already re-exchanges. Splitting the wire format out of the feature keeps the
protocol change reviewable on its own and lets the token query, per-lane storage and
their consumer land in a follow-up.

How?

Recovery today is a two-legged exchange over the AM lane. The endpoint which detects
a lane failure sends LANES_ADDR_REQUEST, packing the addresses of the lanes it
rebuilt in provided_lane_map; the peer replies with LANES_ADDR_REPLY, packing the
addresses of the lanes it managed to rebuild in turn.

The trailer goes between the fixed ucp_wireup_msg_lanes_info_t, which UCX 1.22
already ships, and the packed addresses:

[ucp_wireup_msg_t][lanes_info][request_id][TX lengths][RX lengths][addresses]

Each token section holds one length byte per lane followed by the tokens themselves,
so a lane which has no token costs one zero byte. The sections take their lanes from
the maps already in lanes_info rather than from maps of their own: the TX section
belongs to provided_lane_map, the lanes the message carries addresses for, and the
RX section to requested_lane_map, redefined as the lanes the message answers. The
addresses stay last, so they need no length of their own.

Both peers decide on the trailer from the peer release version in
ucp_ep_config_key_t::dst_version, gated by UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION,
so nothing is added to the fixed header and older peers keep parsing the addresses
where they are today. request_id carries the recovery generation, echoed by the
peer, for the follow-up to tell answers of different rounds apart.

Why a third leg: an RX token is derived from the TX token of the remote side, so
each peer can only produce the RX tokens of the other one. The request carries the
initiator's TX tokens, and the reply carries both the responder's TX tokens and the
RX tokens it derived for the initiator. Nothing has yet delivered the RX tokens the
initiator derives from the reply, which is what LANES_ADDR_ACK is for. It exists
only to carry the trailer, so it is withheld from peers below the minimum version.

Testing: gtest coverage of the section parser, including truncated sections which the
empty sections of the wire itself never produce, plus the fault-tolerance suite on
ConnectX-7 (test_ucp_fault_tolerance and test_ucp_wireup, no failures).

[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]

Coverage note: The gtest (test_ucp_wireup_token_section) only exercises ucp_wireup_skip_token_section in isolation. There is no end-to-end test that a v24<->v24 recovery exchange round-trips the trailer, nor a mixed-version test confirming an older peer still parses a trailer-less LANES_ADDR. The mixed-version path is the one the version gate above protects, so if practical it would be worth confirming via an existing recovery/fault-tolerance job.

[svc-ucx]

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

TL;DR: The WireCompat Build on althca job failed to compile, not to test: src/ucp/wireup/wireup.c triggers -Werror=maybe-uninitialized on the local lanes_info pointer in ucp_wireup_msg_handler. Fix by initializing lanes_info = NULL at its declaration (line 1314).

Full analysis

Summary: make aborted with wireup/wireup.c:1373:44: error: 'lanes_info' may be used uninitialized in this function [-Werror=maybe-uninitialized] while building wireup/libucp_la-wireup.lo; cc1: all warnings being treated as errors → make[2]: *** [wireup/libucp_la-wireup.lo] Error 1.

Root cause: In ucp_wireup_msg_handler, const ucp_wireup_msg_lanes_info_t *lanes_info; (wireup.c:1314) is declared without an initializer and is only ever assigned indirectly, via the out-parameter of ucp_wireup_parse_lanes_addr(), inside the guarded block if (ucp_wireup_msg_is_lanes_addr(msg->type)) (lines 1336–1343). It is then read at lines 1370 and 1373 under the separate conditions msg->type == UCP_WIREUP_MSG_LANES_ADDR_REQUEST / ..._REPLY.

The code is functionally correct — ucp_wireup_msg_is_lanes_addr() is true for exactly the LANES_ADDR_REQUEST/REPLY/ACK types, so the pointer is always set before use — but GCC's flow analysis at -O3 cannot correlate the predicate ucp_wireup_msg_is_lanes_addr(msg->type) with the later equality tests on msg->type, especially since the value is written through a pointer in an inlined static helper. The compiler on the althca agent is stricter/older than on the other build nodes, so this warning only fires there. The build uses -O3 -g -Wall -Werror (see the configure summary in the log), which turns the diagnostic into a hard failure.

Note the helper also has a real (though currently unreachable) hazard that reinforces the warning: the truncation check at wireup.c:1251–1255 returns UCS_ERR_MESSAGE_TRUNCATED before *lanes_info_p is assigned at line 1261, so on that path the caller's variable genuinely stays uninitialized. The caller does goto out on error, so it isn't dereferenced today, but the pattern is fragile.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCP/WIREUP: Add a token trailer and an ACK message to the lane address exchange", Evgeny Leksikov (2026-08-26). This commit introduced ucp_wireup_parse_lanes_addr() and moved the lanes_info assignment out of the handler into an out-parameter, which is what defeats GCC's initialization analysis. The PR head commit is [REDACTED:Hex High Entropy String] on pr-wireup-tokens-framing (PR #11843).

File: src/ucp/wireup/wireup.c:1314 (declaration) — error reported at src/ucp/wireup/wireup.c:1373; secondary issue at src/ucp/wireup/wireup.c:1251-1261.

Suggested fix: Initialize the pointer at its declaration so no code path can leave it indeterminate:

const ucp_wireup_msg_lanes_info_t *lanes_info = NULL;

This is a one-line, zero-cost change that silences the diagnostic on every compiler without restructuring the control flow. Two recommended hardening follow-ups in the same patch:

1. Move *lanes_info_p = lanes_info; (and *request_id_p = 0;) in ucp_wireup_parse_lanes_addr to before the truncation check at line 1251, or explicitly set *lanes_info_p = NULL on that early-return path, so the helper never returns leaving the caller's pointer untouched.
2. Optionally add ucs_assert(lanes_info != NULL); at the top of the LANES_ADDR_REQUEST/REPLY branches (lines 1369–1374) to document and check the invariant that ucp_wireup_msg_is_lanes_addr() guarantees.

Avoid "fixing" this by dropping -Werror or adding a #pragma GCC diagnostic ignored — the warning is pointing at a real initialization gap in the helper's error path.

Related: PR #11843 (this build). Prior work in the same series that introduced the LANES_ADDR exchange and its handlers: 6c7c8ff9 (#11545), 9db2e488 (#11562), db208ee4 (#11563). No existing issue matches the lanes_info may be used uninitialized signature.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 312b23c3-63e1-4a22-80fe-6c58d90b9571 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/ucp/wireup/wireup.c

(question) Trailer presence relies on symmetric dst_version: both peers independently decide whether the token trailer is present using their own ucp_ep_config(ep)->key.dst_version (ucp_wireup_ep_supports_tokens). If the two ends hold asymmetric config views of the peer release version, the sender may add a trailer the receiver doesn't expect (or vice versa), causing the receiver to drop the message or misparse addresses. For recovery both directions are established, so dst_version should be consistent — but please confirm key.dst_version is guaranteed symmetric on both peers when LANES_ADDR messages flow. If yes, a note in the parse function would help; if not, this could silently stall recovery.

No test/coverage gap for the parser itself; the runtime path (real recovery with tokens-capable peers exchanging ACKs) is exercised only by the existing recovery/fault-tolerance suites — worth confirming one of those runs with dst_version >= 24 peers so the ACK path is actually hit, since the trailer is version-gated.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (jucx JUCX Test aarch64 java11) · commit 06585cb5

TL;DR: The aarch64 java11 job failed in the "Build UCX" step, not in any Java test: src/ucp/wireup/wireup.c fails to compile with -Werror=maybe-uninitialized because lanes_info is only assigned inside a conditional block. Initialize it to NULL at declaration (and/or assert non-NULL at use).

Full analysis

Summary: make aborted while compiling wireup/libucp_la-wireup.lo — GCC (aarch64) reported ‘lanes_info’ may be used uninitialized in this function [-Werror=maybe-uninitialized], so the JUCX aarch64/java11 job never got to run tests.

Root cause: In ucp_wireup_msg_handler(), const ucp_wireup_msg_lanes_info_t *lanes_info; (line 1315) is declared without an initializer and is only written by ucp_wireup_parse_lanes_addr() inside if (ucp_wireup_msg_is_lanes_addr(msg->type)) { ... } (lines 1337–1344). It is later read unconditionally in the UCP_WIREUP_MSG_LANES_ADDR_REQUEST / ..._REPLY branches (lines 1371 and 1374). The invariant "message type is LANES_ADDR_REQUEST/REPLY ⇒ ucp_wireup_msg_is_lanes_addr() was true" is real, but GCC cannot prove it across the helper predicate, so it emits the warning — and this build uses -Werror, turning it into a hard error. (The same pattern applies to request_id, which is initialized to 0 at line 1314 and therefore does not warn — evidence the omission for lanes_info was an oversight.)

This is a new-code regression from the PR itself: the refactor that moved lanes-info parsing into ucp_wireup_parse_lanes_addr() with an out-parameter (**lanes_info_p) is what broke GCC's ability to see the definite assignment. It is compiler/arch-sensitive, which is why only the aarch64 GCC leg tripped on it.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCP/WIREUP: Add a token trailer and an ACK message to the lane address exchange", Evgeny Leksikov (introduced ucp_wireup_parse_lanes_addr() and the out-param indirection); still present at PR head 06585cb5 — "UCP/WIREUP: Gate LANES_ADDR token trailers on release 1.24", same author.

File: src/ucp/wireup/wireup.c:1315 (declaration); error reported at src/ucp/wireup/wireup.c:1371, same issue at :1374

Suggested fix: Initialize the pointer at its declaration so the definite-assignment analysis is trivially satisfied:

    const ucp_wireup_msg_lanes_info_t *lanes_info = NULL;

Optionally add ucs_assert(lanes_info != NULL); at the top of the two LANES_ADDR branches (lines 1371/1374) to keep the real invariant documented and checked in debug builds, rather than silently relying on NULL. Avoid "fixing" this by relaxing -Werror or the warning flag — the diagnostic is a legitimate false positive but the one-line initializer is the cheap, standard remedy used elsewhere in this same function (request_id = 0).

Related: PR #11843 (#11843) — the failure is self-contained in this PR; no pre-existing issue found.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 234cdd97-b301-4aa8-9e52-0c15496ee943 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-ucx]

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 0) · commit 6b7c7bf9

TL;DR: The only failure in this GPU run is the GDA device test rc_gda/test_ucp_device_xfer.counter/4, whose host-side poll (wait_for_counter, 10 s default deadline, no worker progress) did not see the expected counter value — a timing/flake in the fire-and-forget (nodelay_without_req) GDA path, not something the PR's wireup-token diff can reach; re-run the job and, if it reproduces, make wait_for_counter progress the worker with an explicit longer timeout.

Full analysis

Summary: make test in the gtest stage exited with Error 1 because 1 of 10227 tests failed: [ FAILED ] rc_gda/test_ucp_device_xfer.counter/4, where GetParam() = rc,rc_gda,cuda_copy,rocm_copy/host/thread/nodelay_without_req (all other 10206 tests passed, 20 skipped).

Root cause: The counter test issues remote counter increments from a CUDA kernel in nodelay_without_req mode (with_no_delay=true, with_request=false, i.e. no completion object to wait on — test/gtest/ucp/test_ucp_device.cc:616-619), then verifies on the host with wait_for_counter(), which polls dst_counter_read() via wait_for_cond(cond, [] {}) — an empty wait lambda (no progress()) with the base-class default deadline of DEFAULT_TIMEOUT_SEC = 10.0 (test/gtest/common/test.h:84,100-108) — and then asserts EXPECT_EQ(num_iters * multiplier, ...) = 10 × 8 = 80 (test/gtest/ucp/test_ucp_device.cc:712-727). The counter never reached the expected value inside that 10 s window. The run was also heavily loaded/slow (TOP-20 list in the log shows single tests such as rcx/test_ucp_atomic32.fetch/1 taking 150032 ms, and 5061749 ms total test time), which is consistent with a short-deadline polling flake. Nothing in the PR under test can plausibly reach this path: commits [REDACTED:Hex High Entropy String] / 06585cb / 6b7c7bf only add the token trailer + LANES_ADDR_ACK to the lane-address exchange (ucp_wireup_pack_token_payload, ucp_wireup_parse_lanes_addr, ucp_wireup_skip_token_section in src/ucp/wireup/wireup.c:287-1304), which is exercised only during EP failover/recovery — this test performs no failover — and the GDA/device code (src/uct/ib/mlx5/gdaki, src/ucp/api/device/ucp_device_impl.h) is untouched by the PR (last GDA change is [REDACTED:Hex High Entropy String], 2026-08-20). Note: the exact actual-vs-expected counter value is in the earlier, truncated part of the console log (only the tail is retrievable), so the distinction between "lost increments" and "not yet arrived" cannot be closed from the fetched log alone.

Implicated commit: none in the PR diff — 6b7c7bf (Evgeny Leksikov, "UCP/WIREUP: Fix maybe-uninitialized lanes_info and shrink request_id") and its predecessors 06585cb / 532b94c touch only wireup.c/.h, ucp_ep.c/.h, ucp_request.h LANES_ADDR recovery paths, not the GDA device path that failed.

File: test/gtest/ucp/test_ucp_device.cc:712-727 (wait_for_counter, assertion at line 724), test invoked from test/gtest/ucp/test_ucp_device.cc:775-791

Suggested fix:

1. Re-run the gpu on worker 0 job to confirm it is a flake; the PR diff does not touch the device/GDA or non-recovery wireup path.
2. Harden the test so it stops failing under load and gives usable diagnostics: in wait_for_counter, progress the worker while polling and use an explicit, generous timeout, e.g. wait_for_cond(cond, [this] { progress(); }, 60.0);, and include the observed counter value in the failure message (<< "value: " << list.dst_counter_read(...)) so the next occurrence pinpoints whether increments were lost or merely late.
3. If it does reproduce with a value stuck below 80, escalate to the GDA owners as a real completion/atomic-delivery bug in the with_request=false (no-request, no-delay) path rather than a test timeout.

Related: PR #11843 (the PR under test, "UCP/WIREUP: Add a token trailer and an ACK message to the lane address exchange"); no existing issue found for test_ucp_device_xfer.counter flakiness.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 5d89fba2-42fa-412f-9eb1-eb0d539787f1 in the triage console for the audit trail.

[roiedanino]

Suggested change

	if (request_id == 0) {
	if (!ucp_wireup_ep_supports_tokens(ep)) {

and perhaps assert(request_id != 0)

[roiedanino]

Minor: maybe add another case for make_section({0, 0, 0});?