libfabric: serialize all endpoint/CQ/AV calls with the rail EP lock

[rongbingzhou]

What?

Hold the rail's ep_mutex_ around every libfabric call on its endpoint, CQ and AV, including the progress-thread (PT) paths drainPostQueue() and pollForCompletions(), which bypassed it. Also lock AV insert/remove and endpoint-bound MR bind/enable/close.

Why?

Rails use FI_THREAD_COMPLETION, so the application must serialize endpoint and CQ access. With the PT enabled, the PT posts transfers and reads the CQ while the caller's thread posts control messages on rail 0 (postSend(), which took the lock). On libfabric 2.7.x (EFA RDM path) this race corrupts provider state and hangs nixlbench --enable-pt VRAM-initiator WRITE.

How?

Lock only the fi_* call itself, never an FI_EAGAIN retry loop, and run completion handling and callbacks outside the lock.

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability of network communication during concurrent operations, including data transfers and completion handling.
  • Improved memory cleanup when registrations cannot provide a usable key.

• Documentation

  • Clarified how concurrent network operations are coordinated.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

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

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

🚀

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: ai-dynamo/nixl/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 893fad85-3fe6-46f1-8c79-cddf2892201c

📥 Commits

Reviewing files that changed from the base of the PR and between 1683cf3 and dbbedae.

📒 Files selected for processing (2)

• src/utils/libfabric/libfabric_rail.cpp
• src/utils/libfabric/libfabric_rail.h

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

The Libfabric rail now uses the endpoint mutex around CQ reads, batched RMA posts, address-vector updates, and endpoint-bound memory-registration operations. Completion handling and callbacks remain outside the lock.

Changes

Libfabric call serialization

Layer / File(s)	Summary
Serialize endpoint operations src/utils/libfabric/libfabric_rail.cpp, src/utils/libfabric/libfabric_rail.h	CQ reads, batched RMA posts, and address-vector insertion and removal now use the endpoint mutex. Completion processing and callbacks remain outside the lock. The mutex comment describes its scope and lock rules.
Serialize endpoint-bound memory registration src/utils/libfabric/libfabric_rail.cpp	Endpoint-bound MR binding, enabling, and deregistration use the endpoint mutex when FI_MR_ENDPOINT is set. Domain-level registration remains unlocked. Cleanup for an unavailable MR key now calls deregisterMemory.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Suggested reviewers: aranadive

Merge Risk: ⚪ Minimal · up to dbbed

The serialization changes appear mergeable after normal checks. Runtime reproduction of the reported provider hang remains unverified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely describes the primary change: serializing libfabric endpoint, CQ, and AV calls with the rail endpoint lock.
Description check	✅ Passed	The description includes complete What, Why, and How sections. It identifies the affected paths, explains the FI_THREAD_COMPLETION race, and describes the lock scope and retry behavior.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

[brminich]

/ok to test dbbedae

[brminich]

/build

[svc-nixl]

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

TL;DR: The Build Docker stage died at Dockerfile STEP 35/38 because the unauthenticated GitHub API call that resolves the "latest vLLM release" tag returned HTTP Error 403: rate limit exceeded; pin VLLM_REF (or authenticate/retry the API call) instead of querying the API at build time.

Full analysis

Summary: nixl-ci-dl-gpu-ep #1399 failed in the Build Docker stage (node 63) at Dockerfile STEP 35/38 — the BUILD_VLLM_ELASTIC_TEST layer — with subprocess exited with status 1.

Root cause: That RUN step resolves the vLLM version dynamically when VLLM_REF is empty:

VLLM_REF="$(python3 -c 'import json, urllib.request; r = urllib.request.Request(
  "https://api.github.com/repos/vllm-project/vllm/releases/latest", ...)')"

The call is unauthenticated, so it is subject to GitHub's 60-requests/hour/IP anonymous limit. The build log shows the traceback ending in urllib.error.HTTPError: HTTP Error 403: rate limit exceeded, which makes VLLM_REF empty/aborts the shell with status 1 and fails the podman/buildah layer. Everything before it succeeded (Mooncake, azure-sdk-for-cpp, azure-cli all built and installed cleanly, only warnings), and the log shows continuous activity with no multi-minute gaps — so this is not a hang and not a timeout. It is also unrelated to the PT_thread_safe branch content (commit dbbedae); the failure is in image construction, before any NIXL code is exercised. Note the CI hosts share an egress IP, so parallel/adjacent EP builds exhaust the anonymous quota quickly, making this intermittent-but-recurring.

Implicated commit: c984ce6 — lishapira, "CI: add vLLM + NIXL EP test to the EP CI job (#2154)" — introduced the unauthenticated releases/latest lookup in the EP CI Dockerfile.

File: .ci/dockerfiles/ — the EP CI Dockerfile, the RUN if [ "${BUILD_VLLM_ELASTIC_TEST}" = "true" ] block, at the VLLM_REF="$(python3 -c ...releases/latest...)" line (Dockerfile STEP 35/38 per the log).

Suggested fix: Stop resolving the vLLM release from the GitHub API at image-build time. Concretely, in order of preference:

1. Pin VLLM_REF to an explicit release tag as a build arg default (this is exactly what PR CI: pin the vLLM release for the elastic EP test #2292 does) and bump it deliberately — reproducible images, no network dependency.
2. If a dynamic lookup must stay, make it resilient: pass a token via --secret and send an Authorization header (raises the limit to 5000/hr), and wrap the call in a bounded retry with backoff plus a hardcoded fallback tag so a 403/5xx degrades instead of failing the build.
3. Fail loudly rather than silently: the current $( ) substitution lets an empty VLLM_REF flow into git clone --branch ""; add an explicit [ -n "$VLLM_REF" ] || { echo "could not resolve VLLM_REF"; exit 1; }.

Re-running #1399 will likely pass once the hourly quota resets, but the class of failure will recur until VLLM_REF is pinned.

Related: PR #2292 "CI: pin the vLLM release for the elastic EP test" (direct fix, still open); PR #2300 "nixl_ep: Test vLLM Elastic EP without patching vLLH" (touches the same block); PR #2291 "CI: retry git fetches and trim gRPC submodules in dependency builds" (same retry-hardening theme); PR #2154 (origin of the lookup).

[akkart-aws]

LGTM — correct fix for the hang, and the rules in the header comment (lock only the fi_* call, never a retry loop, keep it innermost) are the right ones.

One question: do you have a before/after throughput number on nixlbench --enable-pt? The PT's fi_cq_read and the app thread's postSend / drainPostQueue now all serialize on one mutex per rail at completion rate, and the fix is motivated by a hang rather than a measurement. Fine either way, just want to know what it cost.

Longer term I think the real answer is proper FI_THREAD_COMPLETION usage : dedicated CQ and tx/rx contexts per thread, but that's a rail-architecture change and well out of scope here.