fix(agent): restore access permissions and unblock collective multicast calls - #133
Open
galletas1712 wants to merge 2 commits into
Conversation
cuMemSetAccess only recorded its descriptors when the requested range matched a tracked mapping's (address, size) pair exactly. Any caller that maps at one granularity and sets access at another -- which PyTorch's caching allocator does routinely, and which is legal CUDA -- fell through to the passthrough and left access_count at 0. restore_mappings replays access only when access_count != 0, so those mappings came back mapped but with no device access. The VA resolved, and the first kernel to touch it died with CUDA_ERROR_ILLEGAL_ADDRESS. Measured on an 8-rank GLM-5.2 TEP8 checkpoint: 192 of 336 recorded mappings had access_count == 0 (64 each at 2 MiB, 8 MiB and 24 MiB, i.e. 8 buffers per rank). Restoring that checkpoint produced an immediate illegal address on the first inference request on every rank, while the identical configuration cold-started without checkpoint/restore served correctly. Record access against every tracked mapping the range fully covers, and fail closed with CUDA_ERROR_NOT_SUPPORTED on a range that only partially overlaps a tracked mapping rather than passing it through and silently losing the access across restore. Classification runs before the driver call so an unrepresentable range is rejected without mutating device state, and a range covering nothing we track stays a pure passthrough. Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
cuMulticastCreate and cuMulticastAddDevice ran the real driver call while
holding the process-global state_lock. Both contend the NVIDIA RM top lock
and are NVSwitch-team collectives, so they can block for a long time during
concurrent multi-rank startup.
serve() needs the same lock to answer a peer's export request, and the
accept loop served connections inline, so a creator stuck in one of those
calls could not respond. The peer hit the 30 s request_export deadline and
got CUDA_ERROR_INVALID_HANDLE while other ranks succeeded.
That per-rank, timing-dependent failure is pathological for collective
frameworks. Observed on an 8-rank GLM-5.2 TEP8 startup: 2 of 8 ranks timed
out and fell back to a different FlashInfer allreduce backend while the
other 6 stayed on mnnvl. The fallback path runs its own collective, which
the 6 never joined, and the tensor-parallel group deadlocked for 15+
minutes until killed.
bind_memory and bind_address already drop the lock around their driver
call for exactly this reason. Extend the same treatment to create and
add_device, revalidating the phase (and, for add_device, the handle) on
reacquire via a new state_is_active callback.
Also:
* make the export and control-socket deadlines configurable via
DYN_SNAPSHOT_EXPORT_TIMEOUT_SECONDS and
DYN_SNAPSHOT_CONTROL_TIMEOUT_SECONDS, and raise the default from 30 s
to 300 s. The unicast import path has the same exposure.
* serve each control connection on a detached thread so a slow request
no longer blocks accept(), which previously left peers stuck in
connect() burning their deadline before they were ever heard.
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
galletas1712
requested review from
Ronkahn21,
danbar2,
dfeigin-nv,
hhzhang16,
hutm,
julienmancuso,
oleg-kushniriov,
shayasoolin and
shmuel-runai
as code owners
August 30, 2026 02:52
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent defects in the cuinterpose shim, both found while bringing up
GLM-5.2-NVFP4 TEP8 (vLLM, TP=8, 8x B200) with GMS V1 + CRIU checkpoint/restore.
Each one on its own made that configuration unusable. Both are fixed here, on
top of #79, as two separate commits.
Stacked on #79 (
feat/snapshot-cuda-vmm-multicast-posix), which is itself on #78.1.
CUDA_ERROR_ILLEGAL_ADDRESSon the first request after restoreCommit:
fix(agent): restore CUDA access permissions for all covered mappingsSymptom
A restored engine came up healthy — CRIU restore clean,
wake_up()in 0.9 s,endpoints registered — and then every one of the 8 ranks died with
CUDA_ERROR_ILLEGAL_ADDRESSon the first inference request. The same model andthe same engine flags, cold-started without checkpoint/restore, served
correctly. So the fault was introduced by checkpoint/restore.
Root cause
cuMemSetAccessrecorded its descriptors only when the requested range matched atracked mapping's
(address, size)pair exactly:Any caller that maps at one granularity and sets access at another — which is
legal CUDA, and which PyTorch's caching allocator does routinely — took that
passthrough and left
access_countat 0.restore_mappingsthen replays access only whenaccess_count != 0:So those mappings were restored mapped but with no device access. The VA
resolves, nothing looks wrong, and the first kernel to dereference it faults.
Evidence
The
cuinterposer.statesidecar recordsaccess_countper mapping, so this ismeasurable directly from a checkpoint, before any restore. Decoding the sidecar
of the failing 8-rank GLM-5.2 checkpoint:
access_count == 0The 192 broken ones were exactly 64 each at 2 MiB, 8 MiB and 24 MiB — 8 buffers
per rank across 8 ranks, i.e. the non-GMS scratch allocations. GMS's own weight
mappings were never affected, because GMS maps and sets access with an identical
(base, aligned_size)and so always hit the exact-match path.Fix
Record access against every tracked mapping the range fully covers, and fail
closed with
CUDA_ERROR_NOT_SUPPORTEDon a range that only partially overlapsa tracked mapping, rather than passing it through and silently losing the access
across restore. Classification runs before the driver call, so an unrepresentable
range is rejected without mutating device state, and a range that covers nothing
we track stays a pure passthrough.
Result
Same workload, same record counts, before and after the fix:
Restore then serves correctly:
finish_reason: stop, content"2, 3, 5, 7, 11",and three follow-up requests all returning the right answer, with zero CUDA
errors and no container restarts.
2. Rank-divergent hang during multi-rank startup
Commit:
fix(agent): do not hold state_lock across collective multicast callsSymptom
An 8-rank startup wedged for 15+ minutes inside
torch.compile. py-spy showedthe tensor-parallel group split across two different code paths in vLLM's
get_fi_ar_workspace: 6 ranks still attempting themnnvlallreduce workspace,2 ranks already fallen back to
trtllm. The fallback path runs its owncollective, which the other 6 never joined, so the group deadlocked.
Root cause
cuMulticastCreateandcuMulticastAddDeviceran the real driver call whileholding the process-global
state_lock. Both contend the NVIDIA RM top lock andare NVSwitch-team collectives, so under concurrent multi-rank startup they can
block for a long time.
serve()needs that same lock to answer a peer's export request, and the acceptloop served connections inline. So a creator stuck inside one of those calls
could not respond, the peer hit the hard-coded 30 s
request_exportdeadline,and got
CUDA_ERROR_INVALID_HANDLE— while other ranks, which happened to askearlier, succeeded.
A per-rank, timing-dependent failure is pathological for a collective framework:
which ranks lose is pure timing, and any framework that treats workspace creation
failure as a rank-local decision will diverge and hang.
Note the codebase already knew this shape —
bind_memory/bind_addressdrop thelock around their driver call, with the comment "Bind waits for the complete
multicast team, so it must not block the creator endpoint under state_lock."
The same treatment was simply never applied to create and add_device.
Fix
state_lockaround thecuMulticastCreate/cuMulticastAddDevicedriver calls, mirroring
bind_memory. Revalidate the phase on reacquire (and,for
add_device, re-look up the handle, which may have been released whileunlocked) via a new
state_is_activecallback.DYN_SNAPSHOT_EXPORT_TIMEOUT_SECONDSandDYN_SNAPSHOT_CONTROL_TIMEOUT_SECONDS, and raise the default from 30 s to300 s. The unicast import path had the same exposure.
blocks
accept().serve()still takesstate_lockfor its whole body, sorequests stay serialised — the win is that peers are no longer stuck in
connect()burning their deadline before they are even heard.Note for reviewers
The immediate trigger for issue 2 is arguably a vLLM bug: its
mnnvl->trtllmfallback is a rank-local decision with a collective inside it and no agreement
step, so any transient per-rank failure deadlocks the group. FlashInfer ships
the correct primitive for this (
all_ranks_support_mnnvl, which AND-reduces theper-rank probe and documents exactly this hazard), but vLLM bypasses it by
pre-resolving
"auto"locally. That is worth reporting upstream separately.It is still right to fix the shim: it should not manufacture per-rank,
timing-dependent CUDA failures during a collective startup.
Worth a separate discussion: #79 also made
cuMulticastGetGranularityforward tothe driver, where #78 returned
CUDA_ERROR_NOT_SUPPORTED. That is the capabilityprobe, so #79 advertises multicast support and opens the whole multicast path.
A uniform, early, deterministic capability denial is much easier for frameworks
to handle than a late per-rank timeout. Gating multicast behind an env switch
(defaulting to the #78 stub) may be worth considering — at the cost of losing
NVSwitch multicast allreduce, and requiring
NCCL_NVLS_ENABLE=0andTORCH_SYMM_MEM_DISABLE_MULTICAST=1to degrade gracefully rather than fail hard.Validation
Both commits build independently and cleanly under
-Wall -Wextra -Werror(the stack is bisect-clean).
End-to-end on four models, all vLLM TP=8 / TEP8 on 8x B200 with GMS V1 +
CRIU. Every one captures cleanly with no allreduce deadlock, restores with 0
restarts, and serves correct inference after restore — the check that
actually matters here, since the illegal-address fault only surfaced on the
first request:
criu_restorecuda_restorewake_upInference check was a deterministic prompt (
"2, 3, 5, 7, 11", and 3x"6 times 7" -> 42 on GLM-5.2), with zero CUDA errors in the engine logs.
Before the fix, GLM-5.2 died with
CUDA_ERROR_ILLEGAL_ADDRESSon all 8 ranksat exactly this point.
Sidecar verification before/after, decoded from
cuinterposer.state:GLM-5.2 192 mappings without access -> 0 (336/336 now carry access);
Qwen3.8 336/336; DSV4-Pro 144/144.
Shadow-engine promotion (kill the active engine, promote the paused restored
shadow in the same pod) also works on the patched shim: verified on
DSV4-Flash, 21 consecutive correct responses on the promoted engine. It
exposes a separate, unrelated defect in GMS V1 lock admission — the
reclamation of a SIGKILLed peer's epoch blocks new-writer admission and is
O(GPU bytes held) — which is being fixed independently in the dynamo repo and
is not a cuinterpose issue.
Follow-ups not addressed here
build via
LD_PRELOADfrom a shared volume. The image needs rebuilding fromthese commits.
WORLD_SIZE=2, which cannot see either of thesebugs (one needs a non-exact
cuMemSetAccessrange, the other an 8-rank timingrace). Regression tests for both would be valuable.