PLUGINS/UCX: Do not use strict order key for PUT/GET

[tvegas1]

What?

Do not use strict order key in NIXL. Related: openucx/ucx#11636

Why?

Can provide 20% throughput gain when UCX rndv/put/mtype is used for instance.

How?

AM already does not use strict order key, even when DDP/AR is used. So any uct_ep_fence() ends-up having unrelated PUT use strict order key. Fence is not used in NIXL, flush used is LOCAL, and DirectNIC read on flush call does not use fencing explicitly.

Summary by CodeRabbit

• Bug Fixes
  • Improved UCX configuration to improve Active Message visibility after earlier transfers, reducing the likelihood of missed updates in certain communication flows.
  • Enhances reliability during sequences involving prior PUT operations without requiring strict ordering behavior.

[github-actions]

👋 Hi tvegas1! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 2e834a34-f681-4dc9-9c67-6d51a4178961

📥 Commits

Reviewing files that changed from the base of the PR and between 81a04fa and dd6a206.

📒 Files selected for processing (1)

• src/plugins/ucx/ucx_utils.cpp

---

📝 Walkthrough

Walkthrough

Adds an RC_FENCE UCX configuration setting with value "none" during nixlUcxContext construction, along with a comment describing Active Message visibility after earlier PUT operations.

Changes

UCX Context Configuration

Layer / File(s)	Summary
Add UCX_RC_FENCE config knob src/plugins/ucx/ucx_utils.cpp	Sets RC_FENCE to "none" during nixlUcxContext setup and documents its relationship to Active Message visibility after prior PUTs.

Estimated code review effort: 1 (Trivial) | ~3 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main UCX plugin change to avoid strict order key usage for PUT/GET.
Description check	✅ Passed	The description follows the required What/Why/How template and explains the change, motivation, and approach.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/plugins/ucx/ucx_utils.cpp`:
- Around line 435-438: The RC fence setting is being written with the wrong key
in ucx_utils.cpp, so the wrapper’s env-prefix logic causes it to miss the
intended UCX setting. Update the config.modify call in the UCX config setup path
to use the unprefixed RC_FENCE key so the wrapper maps it correctly before
calling ucp_config_modify(), and keep the surrounding fence-related logic in the
same UCX configuration block.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 7d54b2cb-a897-4757-8c88-222383fe7765

📥 Commits

Reviewing files that changed from the base of the PR and between c7ea0c6 and 9f977a8.

📒 Files selected for processing (1)

• src/plugins/ucx/ucx_utils.cpp

[brminich]

/build

[brminich]

/ok to test dd6a206

[ColinNV]

/build

[svc-nixl]

🤖 CI Triage Agent — nixl-ci-dl-gpu · commit d09a7477

TL;DR: The Run DL CPP tests stage was SIGTERM-killed (exit 143) because ucx/TestTransfer.ListenerCommSize/0 hung for ~11.5 minutes; the underlying cause is a UCX UD wireup failure (wireup message size 1017 exceeds max bcopy size 1016 on ud_mlx5/mlx5_16:1) that also made the preceding notification tests fail. Fix by constraining/disabling the problematic UD transport (e.g. set UCX_TLS/exclude ud_mlx5 on this GB200 node) or adding a hard per-test timeout so a broken transport fails fast instead of hanging.

Full analysis

Summary: CPP test stage aborted after the gtest ucx/TestTransfer.ListenerCommSize/0 hung following repeated UCX UD wireup errors that prevented notification delivery.

Root cause: UCX cannot complete endpoint wireup on the UD Mellanox transport: wireup.c:183 UCX ERROR ep ...: wireup message size 1017 exceeds max bcopy size 1016 on ud_mlx5/mlx5_16:1. With wireup failing, all AM sends are canceled (UCX AM send failed with status -16 (Request canceled)), so NotificationOnly/0, SelfNotification/0, and EmptyNotificationPayload/0 fail at test_transfer.cpp:333 (notif_list.size()==0), and ListenerCommSize/0 then blocks forever waiting on a transfer that can never connect. The wall-clock kill (exit 143) is only the symptom of that hang.

Implicated commit: Not a source regression — this is an environment/transport (UCX + mlx5_16 UD) issue on node gb200-nvl4-ts2-78. The sibling ucx-master variant (stage 313) passed CPP tests in ~1004s, indicating the failure is specific to this ucx-v1.22.x run/node, not commit d09a747. test_transfer.cpp assertion at line 333 is from c7e97a41fee1 (Colin Hirsch, "TEST/GTEST: Fail on unexpected error or warning log") but that only surfaces the error; it is not the cause.

File: test/gtest/test_transfer.cpp:333 (assertion) — the failure originates in the UCX ud_mlx5/mlx5_16:1 transport, not in NIXL source.

Suggested fix:

1. Immediate: on the GB200 test node, restrict the UCX transports so the broken ud_mlx5 device isn't used for wireup — e.g. export UCX_TLS to exclude ud/ud_mlx5 (or point tests at the working HCA), or investigate why mlx5_16:1 produces a 1017-byte wireup that exceeds the 1016-byte UD bcopy limit (device/firmware/UCX v1.22.x mismatch on this port).
2. Robustness: add a per-test hard timeout to the CPP gtest runner (or run tests under timeout) so a hung test like ListenerCommSize/0 fails fast and reports the culprit instead of consuming the whole stage budget and being SIGTERM-killed. Do not simply raise the stage time limit — the work was hung, not slow.

Related: none found (search returned only unrelated tracing/telemetry commits on this file).

[svc-nixl]

🤖 CI Triage Agent — nixl-ci-dl-gpu-ep · commit d09a7477

TL;DR: The "Run DL EP elastic tests" stage failed because the expansion_fault_contraction fault-injection test killed rank 2 as planned, but the surviving ranks 0/1/3 crashed with a CUDA "illegal memory access" (exit code 1) instead of gracefully tolerating the peer's death — a fault-tolerance bug in the EP dispatch/combine CUDA-IPC/NVLink path, not a flaky infra issue.

Full analysis

Summary: Jenkins nixl-ci-dl-gpu-ep #395 failed in stage 220 ("Run DL EP elastic tests"); elastic.py --plan expansion_fault_contraction.json raised RuntimeError: Worker processes failed: worker 0 (exit code 1), worker 1 (exit code 1), worker 3 (exit code 1).

Root cause: The plan [[0,1],[0,1,2,3],[0,1,-2,3],[0,3]] deliberately kills rank 2 during phase 2 (the -2 entry). Rank 2 (pid 1659423) received its planned SIGTERM correctly ("Killing rank during dispatch/combine" → "SIGTERM (15) received … releasing rank and exiting"), which the harness tolerates (p.exitcode in (0, -signal.SIGTERM)). But the three surviving ranks then hit CUDA warning: an illegal memory access was encountered (function destroyEvent) followed by cuMemFree_v2(...device_ep) failed and cuStreamDestroy_v2 failed in UCX's cuda_ipc_ep.c / cuda_iface.c, and exited with code 1. The dispatch/combine kernels on surviving ranks accessed the killed peer's CUDA-IPC/NVLink mapping after it was torn down, corrupting the CUDA context — so the fault-tolerant contraction path did not cleanly handle the peer's departure. This is a real crash (log timestamps are continuous 14:52:07→14:52:26 with no gaps — not a hang/timeout).

Implicated commit: Not definitively pinpointable from logs alone. The EP low-latency NVLink / CUDA-IPC path most recently changed in [REDACTED:Hex High Entropy String] (Itay Alroy, "nixl_ep: Optimize low-latency NVLink path", #1751) and the fault/phase semantics in a39434189f94 (#1693). The failure is in code exercised by PR #1917 — compare against the last green run of stage 220 to confirm whether #1917's diff or one of these EP kernel commits introduced it.

File: examples/device/ep/csrc/kernels/nixl_ep_ll.cu (and the CUDA-IPC teardown path in examples/device/ep/csrc/nixl_ep.cpp / buffer destroy); test driver examples/device/ep/tests/elastic/elastic.py:692; plan examples/device/ep/tests/elastic/expansion_fault_contraction.json:4 ([0, 1, -2, 3]).

Suggested fix: Make the surviving ranks tolerate a peer dropping mid dispatch/combine: guard the dispatch/combine kernels and buffer/stream teardown so they do not dereference or cuMemFree/cuStreamDestroy a dead peer's CUDA-IPC handles (detect the fault and skip/quiesce the removed rank's endpoint before contraction). As a starting point, reproduce locally with python3 examples/device/ep/tests/elastic/elastic.py --plan .../expansion_fault_contraction.json --num-processes 4 ... --validate-phase-failures and bisect against the last passing #395-equivalent build; inspect the EP buffer destroy / endpoint-release logic that runs after a peer SIGTERM. Do not treat this as flaky — it is reproducible fault-handling breakage.

Related: none (no matching open issues/PRs found; searches returned only unrelated telemetry/UCX PRs).

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 807ebfe8-b560-4bf4-b6c0-cd67cbad35c0 in the triage console for the audit trail.