[jeynmann]

What?

Allow purge CB returning code

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

The return value is not consumed or documented anywhere. The new doc says UCS_OK means the error was handled and otherwise it is "returned back", but: no in-tree caller consumes the status (the only invocation is in test_ib_mlx5_ext.cc:72, which discards the return value); and the uct_ep_outstanding_purge function doc and uct_ep_outstanding_purge_params_t::cb doc are not updated to describe what "returned back" means (e.g. does a non-UCS_OK from the callback abort the purge, and does it propagate out of uct_ep_outstanding_purge?). Please clarify in the API docs how the status flows back to the uct_ep_outstanding_purge caller, and update the test callback so the change is exercised.

[svc-nvidia-pr-review]

gtest no longer compiles with the new callback signature. In test/gtest/uct/ib/test_ib_mlx5_ext.cc:76, purge_cb is declared as returning void and then assigned to params.cb (line 118), whose type is now ucs_status_t ()(const uct_ep_op_info_t, void*). In C++ this is an incompatible function-pointer type and will fail to compile. The typedef change must be accompanied by updating this callback to return ucs_status_t (and having it return UCS_OK).

Note: This is an API signature change to a v2 callback typedef. Worth confirming it does not break existing out-of-tree callback implementations that return void (source compatibility).

[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 (Build build_aarch64 sles15sp7_aarch64) · commit 3ef13045

TL;DR: The aarch64 build failed to compile test/gtest/uct/ib/test_ib_mlx5_ext.cc because this PR changed uct_ep_outstanding_purge_callback_t to return ucs_status_t, but the gtest callback purge_cb still returns void; update the test callback signature.

Full analysis

Summary: make failed in test/gtest — uct/ib/gtest-test_ib_mlx5_ext.o compile error (Makefile:3634: Error 1), aborting the whole build (Bash exited with code '2').

Root cause: Compiler error at 12:15:10 in the build log:

test/gtest/uct/ib/test_ib_mlx5_ext.cc:118:25: error: invalid conversion from
'void (*)(const uct_ep_op_info_t*, void*)' to
'uct_ep_outstanding_purge_callback_t {aka ucs_status_t (*)(const uct_ep_op_info*, void*)}'
[-fpermissive]
     params.cb         = purge_cb;

This PR (branch ft_purge_return_code) changes the outstanding-purge completion callback typedef to return ucs_status_t instead of void. The gtest helper purge_cb in test/gtest/uct/ib/test_ib_mlx5_ext.cc was not updated and is still declared static void purge_cb(const uct_ep_op_info_t*, void *arg) (line 76), so assigning it to params.cb (line 118) is an invalid conversion. UCX builds gtest with -Wall -Werror, so this is fatal. This is a compile-time API mismatch, not an aarch64/SLES-specific or flaky issue — every job compiling this file will fail the same way.

Implicated commit: [REDACTED:Hex High Entropy String] (PR #11882, ft_purge_return_code) — the API change; the test file was introduced by [REDACTED:Hex High Entropy String] / 0826e0b (Zihao Zhao, "UCT: outstanding purge implementation" / "add token query support").

File: test/gtest/uct/ib/test_ib_mlx5_ext.cc:76 (definition) and :118 (assignment); also :72 where the callback's result is discarded.

Suggested fix: Update the test callback to the new signature and check its status:

static ucs_status_t purge_cb(const uct_ep_op_info_t*, void *arg)
{
    *static_cast<bool*>(arg) = true;
    return UCS_OK;
}

and in purge() (line 72) propagate/assert the returned value instead of ignoring it, e.g.:

EXPECT_UCS_OK(params->cb(&op_info, params->arg));

Then grep the tree for any other users of uct_ep_outstanding_purge_callback_t / params->cb (UCT transports, ucp, and other gtests) to make sure all callback definitions and call sites were converted in this PR.

Related: PR #11882 (this change); prior work PR #11668 and PR #11804 added the purge API and this test.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id e76c72f3-3e79-4391-b063-538b29c7893a 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/uct/api/v2/uct_v2.h

This is a source-level signature change to a public v2 callback typedef (uct_ep_outstanding_purge_callback_t changes from returning void to ucs_status_t). Out-of-tree callbacks currently defined as void-returning will need to be updated to return ucs_status_t; this is a source-compatibility point inherent to the intended change.

test/gtest/uct/ib/test_ib_mlx5_ext.cc

No new test exercises the non-OK ("operation not purged, returned to transport") path, since that behavior lives in out-of-tree plugins; the existing test only covers the UCS_OK path. Worth confirming with the author whether a mock exercising the error path is practical, though not required given the in-tree caller ignores the return value.

[evgeny-leksikov]

Suggested change

	* @return @ref UCS_OK - The operation was purged successfully.
	* Otherwise - The operation was not purged and is returned back
	* @return @ref UCS_OK - The purged operation was handled successfully.
	* Otherwise - The purged operation was handled with error and returned back

additional question - how should transport handle this error? what's the side effect of it for user?

[roiedanino]

Should probably stop processing the next wqes?

[evgeny-leksikov]

it may lead to hang of upper layer (UCP), it looks like this status useless, UCT cannot react on this and must continue purging.
if UCP is not able to handle its requests, it looks like unrecoverable error on UCP side.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[evgeny-leksikov]

need to clarify use case / behavior