cudarc fork: PR #585 is alive and blocked on us — answer the maintainer's thread-safety question #260

New Issue

grenade · 2026-08-15T20:42:14Z

grenade commented

2026-08-15 20:42:14 +00:00

Correction (2026-08-15): the original version of this issue claimed
no upstream PR had ever been filed. That was wrong. I searched
coreylowman/cudarc, which has been renamed to chelsea0x3b/cudarc;
gh pr list does not follow the rename redirect and returned []. The
Cargo.toml comment saying "pending upstream review" was accurate all
along. Everything below is the corrected picture.

The fork (verified 2026-08-15)

Pin: grenade/cudarc @ 6904cf2e651a0384eac318f003ce6417f47f5d94,
branched from v0.19.8. Three commits, all 2026-06-08, all in
src/nccl/safe.rs:

commit	change	lines
`d34b54a`	expose `Comm::abort`, `get_async_error`, raw comm accessor	+38
`2ece895`	don't panic in `Comm::drop` when abort returns non-success	+6 −1
`6904cf2`	make `Comm` Send+Sync and abort exactly once	+38 −7

Needed for #17 Stage 2 TP hang-recovery: aborting a wedged collective
and rebuilding the comm.

Upstream: chelsea0x3b/cudarc#585

state:      OPEN        mergeable: MERGEABLE
created:    2026-06-08  updated: 2026-08-11
diff:       +76 -3, 1 file
reviews:    8

Not ignored — actively reviewed, and the exchange has already improved
the patch:

2026-06-08 — review flags a double-free (ncclCommAbort frees the
communicator, so abort() + Drop would abort twice) and that Comm
was !Send + !Sync so a cross-thread abort was unusable anyway. Fixed
in 63327a2 with an AtomicBool swapped exactly once, making
abort() idempotent.
2026-06-18 — maintainer asks for cu_comm() naming consistency
(done) and, more importantly, for sources on ncclComm_t thread
safety.
2026-06-24 — we conceded the SAFETY wording overshot, and restated
the accurate contract: a comm may be used from multiple threads only
one at a time, and safe abort requires the comm created nonblocking
with no thread inside an NCCL op when ncclCommAbort is called.

What is blocking it — an unanswered question, 4 days old

2026-08-11, chelsea0x3b:

Based on the nvidia nccl docs, a comm is not thread safe based on my
read. This doesn't seem related to the description.

@grenade where in this page
https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/usage/threadsafety.html
do you gather that it is supposed to be safe to use a comm from
multiple threads?

The maintainer is pushing on the unsafe impl Send + Sync for Comm, and
the question is a fair one. NCCL's own threadsafety page says primitives
are "generally not thread-safe, however … reentrant", and that operations
must not be issued to a single communicator in parallel from multiple
threads.

The distinction that needs making in the reply:

Send — moving a Comm between threads, used by one at a time,
is consistent with the quoted contract.
Sync is the contested half: it permits &Comm shared
concurrently, which is exactly what the abort use case wants (one
thread inside a collective, another calling abort). Justifying it
rests on the Fault Tolerance docs rather than the thread-safety page —
ncclCommAbort exists precisely to be called out-of-band — and on the
nonblocking-comm requirement we already conceded on 2026-06-24.

Possible outcomes worth weighing before replying:

Cite Fault Tolerance and narrow the SAFETY comment to the
nonblocking + no-thread-inside-an-op contract, keeping Send+Sync.
Implement Send only, and reach abort through a separately-held
raw ncclComm_t rather than a shared &Comm. Weaker claim, likely
easier to land, and possibly sufficient for our device-worker design
where the comm already lives on one owning thread.
Ask the maintainer which they would accept — the question is
genuinely about what invariant the crate wants to promise.

Option 2 deserves a hard look: our TP layer already pins each comm to a
context-owning thread, so we may not need Sync at all.

Also true

Gap is one release: we branched at v0.19.8, upstream is at v0.19.9.
Rebasing now is nearly free.
The Cargo.toml comment is accurate and needs no correction.

Sibling: #261 (candle fork — different story; that PR really has had zero
engagement).

**Correction (2026-08-15):** the original version of this issue claimed no upstream PR had ever been filed. That was wrong. I searched `coreylowman/cudarc`, which has been **renamed to `chelsea0x3b/cudarc`**; `gh pr list` does not follow the rename redirect and returned `[]`. The `Cargo.toml` comment saying "pending upstream review" was accurate all along. Everything below is the corrected picture. ## The fork (verified 2026-08-15) Pin: `grenade/cudarc @ 6904cf2e651a0384eac318f003ce6417f47f5d94`, branched from **v0.19.8**. Three commits, all 2026-06-08, all in `src/nccl/safe.rs`: | commit | change | lines | |---|---|---| | `d34b54a` | expose `Comm::abort`, `get_async_error`, raw comm accessor | +38 | | `2ece895` | don't panic in `Comm::drop` when abort returns non-success | +6 −1 | | `6904cf2` | make `Comm` Send+Sync and abort exactly once | +38 −7 | Needed for #17 Stage 2 TP hang-recovery: aborting a wedged collective and rebuilding the comm. ## Upstream: chelsea0x3b/cudarc#585 ``` state: OPEN mergeable: MERGEABLE created: 2026-06-08 updated: 2026-08-11 diff: +76 -3, 1 file reviews: 8 ``` Not ignored — actively reviewed, and the exchange has already improved the patch: - **2026-06-08** — review flags a double-free (`ncclCommAbort` frees the communicator, so `abort()` + `Drop` would abort twice) and that `Comm` was `!Send + !Sync` so a cross-thread abort was unusable anyway. Fixed in `63327a2` with an `AtomicBool` swapped exactly once, making `abort()` idempotent. - **2026-06-18** — maintainer asks for `cu_comm()` naming consistency (done) and, more importantly, for sources on `ncclComm_t` thread safety. - **2026-06-24** — we conceded the SAFETY wording overshot, and restated the accurate contract: a comm may be used from multiple threads only one at a time, and safe abort requires the comm created **nonblocking** with no thread inside an NCCL op when `ncclCommAbort` is called. ## What is blocking it — an unanswered question, 4 days old **2026-08-11, chelsea0x3b:** > Based on the nvidia nccl docs, a comm is not thread safe based on my > read. This doesn't seem related to the description. > @grenade where in this page > https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/usage/threadsafety.html > do you gather that it is supposed to be safe to use a comm from > multiple threads? The maintainer is pushing on the `unsafe impl Send + Sync for Comm`, and the question is a fair one. NCCL's own threadsafety page says primitives are "generally not thread-safe, however … reentrant", and that operations must not be issued to a single communicator in parallel from multiple threads. The distinction that needs making in the reply: - **`Send`** — moving a `Comm` between threads, used by one at a time, is consistent with the quoted contract. - **`Sync`** is the contested half: it permits `&Comm` shared concurrently, which is exactly what the abort use case wants (one thread inside a collective, another calling abort). Justifying it rests on the Fault Tolerance docs rather than the thread-safety page — `ncclCommAbort` exists precisely to be called out-of-band — and on the nonblocking-comm requirement we already conceded on 2026-06-24. Possible outcomes worth weighing before replying: 1. **Cite Fault Tolerance and narrow the SAFETY comment** to the nonblocking + no-thread-inside-an-op contract, keeping Send+Sync. 2. **Implement `Send` only**, and reach abort through a separately-held raw `ncclComm_t` rather than a shared `&Comm`. Weaker claim, likely easier to land, and possibly sufficient for our device-worker design where the comm already lives on one owning thread. 3. Ask the maintainer which they would accept — the question is genuinely about what invariant the crate wants to promise. Option 2 deserves a hard look: our TP layer already pins each comm to a context-owning thread, so we may not need `Sync` at all. ## Also true - Gap is **one release**: we branched at v0.19.8, upstream is at v0.19.9. Rebasing now is nearly free. - The `Cargo.toml` comment is accurate and needs no correction. Sibling: #261 (candle fork — different story; that PR really has had zero engagement).

grenade referenced this issue

2026-08-15 20:42:44 +00:00

return the candle fork upstream — or stop calling it temporary; 3 of its 4 commits were never offered #261

grenade changed title from ~~return the cudarc fork upstream — 3 commits, 82 lines, and a PR that was never actually filed~~ to cudarc fork: PR #585 is alive and blocked on us — answer the maintainer's thread-safety question

2026-08-15 20:46:24 +00:00

grenade referenced this issue

2026-08-17 14:20:11 +00:00

a wedged NCCL collective hangs neuron indefinitely — exit non-zero and let systemd restart it #265

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: helexa/helexa#260