UCT/API: Purge outstanding work without an RX token by evgeny-leksikov · Pull Request #12006 · openucx/ucx

evgeny-leksikov · 2026-09-30T22:42:07Z

What?

Purge outstanding work without an RX token. Omitting the RX token means the endpoint is unrecoverable. Every outstanding operation is treated as undelivered.

Why?

When the peer is unavailable this is a way to clean up the EP

svc-nvidia-pr-review · 2026-09-30T22:42:41Z

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

svc-nvidia-pr-review · 2026-09-30T22:48:08Z

+        }
+
+        delivered = 1;
+    } else {


Maybe set receiver_next_psn = first_failed_psn here and keep delivered = 1 before the loop? Then uct_ib_mlx5_wqe_is_delivered() returns false on the first WQE and the existing single code path marks everything undelivered, without the dead receiver_next_psn = 0.

overcomplicating, receiver_next_psn is not used if delivered == 1, it's initialized to suppress compiler warns

svc-nvidia-pr-review · 2026-09-30T22:48:08Z

                                  ctx.num_ops_posted_after_flush);
 }

+UCS_TEST_SKIP_COND_P(test_uct_purge_outstanding, am_short_no_rx_token,


This test body is a copy of am_short; can we extract a helper (e.g. do_test_am_short(bool with_rx_token)) and call it from both tests?

svc-nvidia-pr-review · 2026-09-30T22:48:08Z

+
+    EXPECT_GT(ctx.num_ops_purged_at_completion, 0u);
+    EXPECT_GT(ctx.num_ops_posted_after_flush, 0u);
+    EXPECT_EQ(ctx.num_ops_purged, ctx.num_ops_purged_at_completion +


These are the same expectations as am_short, so the test passes even if delivered operations are skipped. Can we also assert that everything posted is reported, e.g. return num_posted from test_purge_outstanding() and check num_ops_purged == num_posted when rx_token is omitted?

ctx.num_ops_purged_at_completion + ctx.num_ops_posted_after_flush = num_posted

svc-nvidia-pr-review · 2026-09-30T22:48:10Z

Coverage gap worth confirming with CI results: the no-token path is only exercised for am_short on a DEVX-capable rc_mlx5 device (UCT_IFACE_FLAG_V2_QUERY_TOKEN), so the put variants and non-mlx5 hardware do not exercise the new branch.

svc-ucx · 2026-09-30T23:20:19Z

🤖 CI Triage Agent — UCX PR · commit 0149890c

TL;DR: The "Basic compile" stage failed because commit 0149890c left a duplicated, malformed line ((params->rx_token == NULL)) {) inside uct_rc_mlx5_ep_outstanding_purge_check_params(), which breaks brace balance and makes the rest of rc_mlx5_ep.c parse as nested inside that function. Delete the two stray lines (1353–1354) so the RX-token NULL check has a single body.

Full analysis

Summary: Azure Pipelines "Basic compile" job failed with make[5]: *** [Makefile:925: rc/libuct_ib_mlx5_la-rc_mlx5_ep.lo] Error 1 — hundreds of -Werror=shadow / invalid storage class for function / expected declaration or statement at end of input errors in src/uct/ib/mlx5/rc/rc_mlx5_ep.c.

Root cause: A botched edit (leftover text from a copy/paste) in uct_rc_mlx5_ep_outstanding_purge_check_params():

1350:     if ((params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) &&
1351:         (params->rx_token == NULL)) {
1352:         ucs_error("rc mlx5: rx token is NULL");
1353:         (params->rx_token == NULL)) {        <-- stray duplicated line
1354:         ucs_error("rc mlx5: rx token is not set");
1355:         return UCS_ERR_INVALID_PARAM;
1356:     }

Line 1353 is a syntax error and, critically, opens an extra { that is never closed. Every subsequent top-level definition in the file is therefore parsed as nested inside uct_rc_mlx5_ep_outstanding_purge_check_params — which is exactly what the log shows: all errors are reported with the prefix In function 'uct_rc_mlx5_ep_outstanding_purge_check_params', and nested function definitions trigger invalid storage class for function, shadows a global declaration, nested extern declaration, defined but not used, and finally expected declaration or statement at end of input at the last line (2067). The hundreds of shadow/unused errors are pure cascade noise from this one brace imbalance, not independent problems.

Implicated commit: 0149890c — "UCT/RC: err msg in purge outstanding param check", Evgeny Leksikov (the PR HEAD commit; the helper itself was added in the preceding 34b2f462 "UCT/IB/MLX5: Purge outstanding work without an RX token")

File: src/uct/ib/mlx5/rc/rc_mlx5_ep.c:1350-1356 (offending text at 1353–1354)

Suggested fix: Remove the stray lines 1353–1354 so the check has one condition and one body:

    if ((params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) &&
        (params->rx_token == NULL)) {
        ucs_error("rc mlx5: rx token is NULL");
        return UCS_ERR_INVALID_PARAM;
    }

Note this is the semantically correct form for this PR: uct_rc_mlx5_ep_outstanding_purge() at lines 1458–1473 explicitly supports an absent RX token via the else branch (delivered = 0), so the function must only reject a NULL rx_token when UCT_EP_OUTSTANDING_FIELD_RX_TOKEN is actually set — do not add an unconditional "rx token is not set" error. After fixing, rebuild locally with the same flags (-Werror -Wshadow -Wdeclaration-after-statement -Wnested-externs) via contrib/test_jenkins.sh-style basic compile to confirm the whole cascade clears.

Related: PR #12006 (branch uct_purge_no_rx_token); builds on #11874 "UCT/IB/MLX5: support put zcopy in purge outstanding".

svc-nvidia-pr-review · 2026-09-30T23:24:38Z

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

svc-nvidia-pr-review · 2026-09-30T23:29:36Z

    }

-    if (!(params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN)) {
+    if ((params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) &&


blocker: leftover duplicated condition and error line, this does not compile.

Suggested change

if ((params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) &&

if ((params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) &&

(params->rx_token == NULL)) {

ucs_error("rc mlx5: rx token is NULL");

svc-nvidia-pr-review · 2026-09-30T23:29:36Z

-     *  UCT_EP_OUTSTANDING_FIELD_CB must be set. */
+     *  UCT_EP_OUTSTANDING_FIELD_CB must be set. If @ref
+     *  UCT_EP_OUTSTANDING_FIELD_RX_TOKEN is omitted, every outstanding
+     *  operation is treated as undelivered. */


The new "treated as undelivered" rule is only documented on field_mask; the function/callback doc still describes the callback as reporting undelivered operations, so a caller that reposts from op_info can duplicate data that the peer already received. Can we add this note to the @ref uct_ep_outstanding_purge doc as well, and mention that without the token the callback may also report operations the peer already received, so reposting them can duplicate data?

svc-nvidia-pr-review · 2026-09-30T23:29:36Z

                                  ctx.num_ops_posted_after_flush);
 }

+UCS_TEST_SKIP_COND_P(test_uct_purge_outstanding, am_short_no_rx_token,


minor: this is an exact copy of am_short, pls move the body to a helper (e.g. test_am_short(bool with_rx_token)) and call it from both tests.

Omitting the RX token means the endpoint is unrecoverable. Every outstanding operation is treated as undelivered and completed canceled.

svc-nvidia-pr-review · 2026-10-01T09:10:04Z

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

svc-nvidia-pr-review · 2026-10-01T09:16:16Z

                                  ctx.num_ops_posted_after_flush);
 }

+UCS_TEST_SKIP_COND_P(test_uct_purge_outstanding, am_short_no_rx_token,


the new test is a line-for-line copy of am_short except for the last argument; pls extract the body into a helper, e.g. test_am_short(bool with_rx_token), and keep both test cases as one-liners.

svc-nvidia-pr-review · 2026-10-01T09:16:16Z

-     *  UCT_EP_OUTSTANDING_FIELD_CB must be set. */
+     *  UCT_EP_OUTSTANDING_FIELD_CB must be set. If @ref
+     *  UCT_EP_OUTSTANDING_FIELD_RX_TOKEN is omitted, every outstanding
+     *  operation is treated as undelivered. */


minor: pls document the optional case on the rx_token field as well ("Valid when @ref UCT_EP_OUTSTANDING_FIELD_RX_TOKEN is set"), like arg does — the field_mask comment alone is easy to miss.

svc-ucx · 2026-10-01T12:01:05Z

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

TL;DR: The Coverity stage failed with 1 new defect (FORWARD_NULL) introduced by this PR's new purge_outstanding(..., with_rx_token=false) test path, which passes purge_params.rx_token == NULL into uct_ep_outstanding_purge; Coverity can't correlate the UCT_EP_OUTSTANDING_FIELD_RX_TOKEN field-mask bit with the non-NULL pointer (the validation lives in a separate helper), so it reports a null deref at rc_mlx5_ep.c:1458. Fix by always pointing purge_params.rx_token at a real buffer in the test (only toggling the field-mask bit), and/or inlining the NULL check into uct_rc_mlx5_ep_outstanding_purge so Coverity sees the kill-null.

Full analysis

Summary: Azure job "coverity devel on coverity_rh7" failed: build and analysis succeeded, but cov-format-errors reported 1 issue, so the stage exited non-zero (##[error]Coverity found 1 issues: /__w/13/s/test/gtest/uct/test_peer_failure.cc:604 Type: Explicit null dereferenced (FORWARD_NULL)).

Root cause: Not a build/test failure — a static-analysis gate. The reported path is:

test/gtest/uct/test_peer_failure.cc:567 — uct_ep_outstanding_purge_params_t purge_params = {}; ⇒ purge_params.rx_token = NULL (assign_zero).
Line 576 — with_rx_token false, so the block that sets both UCT_EP_OUTSTANDING_FIELD_RX_TOKEN and purge_params.rx_token is skipped (this with_rx_token option is new in this PR).
Line 604 — &purge_params is passed to uct_ep_outstanding_purge, which dispatches via iface->internal_ops->ep_outstanding_purge (uct_iface.c:852) to uct_rc_mlx5_ep_outstanding_purge, where Coverity assumes the branch params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN is taken (rc_mlx5_ep.c:1456) and dereferences rx_token at line 1458.

The runtime code is actually safe: uct_rc_mlx5_ep_outstanding_purge_check_params() (rc_mlx5_ep.c:1350-1354) rejects field_mask & RX_TOKEN with rx_token == NULL, and the test never sets the bit when the pointer is NULL. Coverity simply doesn't propagate the field-mask/pointer correlation across the separate check_params() helper and the function-pointer model, so this is a false positive that the new test path newly exposes.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/IB/MLX5: Purge outstanding work without an RX token", Evgeny Leksikov

File: test/gtest/uct/test_peer_failure.cc:567 (NULL origin) and :604 (reported site); modeled deref at src/uct/ib/mlx5/rc/rc_mlx5_ep.c:1456-1458

Suggested fix: Remove the NULL from the path rather than suppressing it:

In purge_outstanding(), always allocate/point the RX token buffer and only gate the field-mask bit, e.g. query rx_attr.rx_token_length and rx_token.resize(...) unconditionally, set purge_params.rx_token = rx_token.data() unconditionally, and only do purge_params.field_mask |= UCT_EP_OUTSTANDING_FIELD_RX_TOKEN; when with_rx_token is true (ensure the vector is non-empty so data() is non-NULL).
Additionally/alternatively, make the invariant locally visible to Coverity in uct_rc_mlx5_ep_outstanding_purge(): inline the rx_token == NULL check (or add an explicit if (ucs_unlikely(params->rx_token == NULL)) return UCS_ERR_INVALID_PARAM;) inside the UCT_EP_OUTSTANDING_FIELD_RX_TOKEN branch at line 1456, instead of relying only on the out-of-line ..._check_params() helper. That also hardens the non-gtest callers.

Related: PR #12006 (this change); prior work on the same code path: #11874, #11915, #11836, #11969

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 2ff4da67-4125-4d94-b51f-e398e0de8d46 in the triage console for the audit trail.

gleon99 · 2026-10-01T14:36:28Z

@evgeny-leksikov coverity issue seems real?

gleon99 · 2026-10-01T14:44:50Z

-        uct_iface_attr_v2_t               tx_attr      = {};
-        uct_iface_attr_v2_t               rx_attr      = {};
-        uct_ep_attr_t                     ep_attr      = {};
+        std::vector<uint8_t>              tx_token;


Can move to inner scope

Coverity does not connect the field-mask check in the helper with the later dereference. Document the omitted-token purge case and share the am_short test body.

svc-nvidia-pr-review · 2026-10-02T09:36:59Z

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

svc-nvidia-pr-review · 2026-10-02T09:53:50Z

+            uct_iface_attr_v2_t  rx_attr = {};
+            uct_ep_attr_t        ep_attr = {};
+            std::vector<uint8_t> tx_token;
+            std::vector<uint8_t> rx_token;


blocker: rx_token is now declared inside the if (with_rx_token) block, so the vector is destroyed at the closing brace while purge_params.rx_token is still dereferenced by uct_ep_outstanding_purge() afterwards. The transport reads freed heap memory and derives receiver_next_psn from garbage, which can make the token path return UCS_ERR_INVALID_PARAM (psn_diff > num_outstanding_packets) or misclassify delivered WQEs. This affects all four existing token-based tests (am_short, put_short, put_bcopy, put_zcopy), not just the new one — before this PR the vectors lived at function scope.

Suggestion: declare std::vector<uint8_t> rx_token; at function scope (outside the if (with_rx_token) block) and keep only the other locals (tx_attr, rx_attr, ep_attr, tx_token) inside the block.

svc-nvidia-pr-review · 2026-10-02T09:53:50Z

+                        num_outstanding_packets) & UCT_IB_MLX5_PSN_MASK;
+    if (params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) {
+        rx_token = params->rx_token;
+        if (rx_token == NULL) {


minor: the rx_token == NULL validation moved out of uct_rc_mlx5_ep_outstanding_purge_check_params(), which exists for exactly this. Can we keep this NULL check there together with the other parameter validation, so all parameter checks stay in one place and happen before any WQ walking?

svc-ucx · 2026-10-02T10:11:33Z

🤖 CI Triage Agent — UCX PR (Tests roce on worker 0) · commit baa63ab3

[incomplete: stop_reason=max_tokens]

gleon99 · 2026-10-02T11:09:46Z

CI issue seems real. token tests pass freed storage. rx_token is declared inside the if block, but the purge call is after that block.

The vector was destroyed at the end of the if, while purge_params.rx_token still pointed into it.

svc-nvidia-pr-review · 2026-10-02T13:40:51Z

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

svc-nvidia-pr-review · 2026-10-02T14:02:03Z

+                        num_outstanding_packets) & UCT_IB_MLX5_PSN_MASK;
+    if (params->field_mask & UCT_EP_OUTSTANDING_FIELD_RX_TOKEN) {
+        rx_token = params->rx_token;
+        if (rx_token == NULL) {


Parameter validation is now split between uct_rc_mlx5_ep_outstanding_purge_check_params() (NULL params, cb) and the main purge function (rx_token). A reader/maintainer has to look in two places for the purge param contract, and the rx_token check runs after the ucs_assertv_always() and packet-count work instead of with the other early checks.

pls keep this check in uct_rc_mlx5_ep_outstanding_purge_check_params() with the other param validations, so all the purge param contract stays in one place.

svc-nvidia-pr-review · 2026-10-02T14:02:03Z

    }

-    void purge_outstanding(purge_ctx *ctx)
+    void purge_outstanding(purge_ctx *ctx, bool with_rx_token = true)


minor: the = true default value on purge_outstanding() is unused, the only caller passes it explicitly.

Suggested change

void purge_outstanding(purge_ctx *ctx, bool with_rx_token = true)

void purge_outstanding(purge_ctx *ctx, bool with_rx_token)

svc-nvidia-pr-review reviewed Sep 30, 2026

View reviewed changes

UCT/IB/MLX5: Purge outstanding work without an RX token

2799ee8

Omitting the RX token means the endpoint is unrecoverable. Every outstanding operation is treated as undelivered and completed canceled.

evgeny-leksikov force-pushed the uct_purge_no_rx_token branch from 0149890 to 2799ee8 Compare October 1, 2026 09:03

svc-nvidia-pr-review reviewed Oct 1, 2026

View reviewed changes

gleon99 reviewed Oct 1, 2026

View reviewed changes

UCT/IB/MLX5: Reject a NULL RX token next to its dereference

baa63ab

Coverity does not connect the field-mask check in the helper with the later dereference. Document the omitted-token purge case and share the am_short test body.

svc-nvidia-pr-review reviewed Oct 2, 2026

View reviewed changes

TEST/UCT: Keep the RX token alive until outstanding purge returns

4e6b54e

The vector was destroyed at the end of the if, while purge_params.rx_token still pointed into it.

svc-nvidia-pr-review reviewed Oct 2, 2026

View reviewed changes

gleon99 approved these changes Oct 3, 2026

View reviewed changes

gleon99 merged commit 2dbbd8c into openucx:master Oct 4, 2026
162 checks passed

svc-ucx mentioned this pull request Oct 29, 2026

UCT/CUDA: Fix deadlock with concurrent cuInit #12098

Open

	void purge_outstanding(purge_ctx *ctx, bool with_rx_token = true)
	void purge_outstanding(purge_ctx *ctx, bool with_rx_token)

Conversation

evgeny-leksikov commented Sep 30, 2026

What?

Why?

Uh oh!

svc-nvidia-pr-review commented Sep 30, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Sep 30, 2026

Uh oh!

svc-ucx commented Sep 30, 2026

Uh oh!

svc-nvidia-pr-review commented Sep 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svc-ucx commented Oct 1, 2026

Uh oh!

gleon99 commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Oct 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svc-ucx commented Oct 2, 2026

Uh oh!

gleon99 commented Oct 2, 2026

Uh oh!

svc-nvidia-pr-review commented Oct 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants