Skip to content

fix(cache): retain shared SWA slots until final free - #35501

Closed
Arist12 wants to merge 43 commits into
sgl-project:sglang-milesfrom
Arist12:fix/swa-allocator-deduplicate-free
Closed

Arist12 wants to merge 43 commits into
sgl-project:sglang-milesfrom
Arist12:fix/swa-allocator-deduplicate-free

Conversation

@Arist12

@Arist12 Arist12 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Problem

Different full-cache indices can reference the same physical SWA slot. Freeing one mapping previously returned that slot while another mapping still referenced it, allowing premature reuse and a later double free.

Change

Track a reference count per physical SWA slot whenever mappings are written, replaced, or cleared. Free token slots or pages only after their final mapping is removed, while preserving PureSWA's direct identity mapping.

Validation

  • full diff passes repository pre-commit hooks
  • focused duplicate-free and cross-call token/page lifetime tests pass on MI350X, including page_size=4
  • combined Inkling LoRA E2E completes two rollouts without the shared-slot illegal address

CI States

Latest PR Test (Base): ❌ Run #32773003173
Latest PR Test (Extra): ❌ Run #32773002775
Latest PR Test (AMD ROCm 7.2): ➖ No AMD PR run found for this commit.

ishandhanani and others added 30 commits July 22, 2026 15:18
…ect#31076) (sgl-project#32074)

Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Co-authored-by: Connor Carpenter <connorc@nvidia.com>
…yer mtp draft extend cuda graph (sgl-project#32254) (sgl-project#32260)

Co-authored-by: Qiaolin Yu <liin1211@outlook.com>
…isoning spec verify capture (trtllm_mla + tc_piecewise) (sgl-project#32288) (sgl-project#32346)

Co-authored-by: Qiaolin Yu <liin1211@outlook.com>
…ense) (sgl-project#18639)

Squashes the qwen_dense on-policy follow-up into the original FSDP2 commit:
the follow-up rewrote the norm_kwargs / rl_on_policy_target call sites this
commit introduced, so they cannot be applied independently.

Rebased onto v0.5.16: rl_on_policy_target predicates now go through the
sglang.srt.true_on_policy helpers, and every legacy get_global_server_args()
call site uses runtime_context.get_server_args() so v0.5.16's
test_legacy_global_ratchet baseline still holds.

Co-authored-by: maocheng23 <35615230+maocheng23@users.noreply.github.com>
…h/RL fixes (sgl-project#25141, sgl-project#29874, sgl-project#31251)

Squashes the two follow-up PRs into the MoE-LoRA base commit: both rewrite the
mem_pool sharding helpers this commit introduces and cannot be applied
independently.

Rebased onto v0.5.16:
  - the shard probes keep v0.5.16's shared-MoE-over-full-TP carve-out at EP=1
    (`is_shared_moe_module`) and only fall through to the probed shard for
    non-MoE modules;
  - sgl-project#29874's `free_lora` is dropped -- v0.5.16 already releases the slot on
    unload through `LoRAMemoryPool.remove_lora()`, which additionally zeroes the
    buffers for graph-captured replay, so `lora_manager.unload` needs no second
    call;
  - sgl-project#31831's expected_checksums check is likewise already upstream in
    `tp_worker.load_lora_adapter_from_tensors`.

Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai>
…psert (sgl-project#27268, sgl-project#31759, sgl-project#30913)

Squashes the two follow-ups into the API commit: sgl-project#31759 fixes this commit's
HTTP handler (the LoRAUpdateOutput was returned unserialized) and sgl-project#30913
rewrites its registry-registration block to support in-place upsert, so
neither applies on its own.

Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai>
Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…ct#21278, sgl-project#22663)

Includes making Cohere2MoeConfig a dataclass so parameter mapping can
introspect config fields for P2P weight update.

Rebased onto v0.5.16, which extracted the remote-instance transfer engine out
of ModelRunner into RemoteInstanceWeightTransporter: the per-rank
RankParallelismConfig is now built in `init_engine()` and published from
`maybe_register_and_publish_weight_info()`, instead of the ModelRunner methods
this commit originally added. The hoisted deepseek expert_params_mapping also
picks up v0.5.16's broader `is_wint4afp8_or_wint4a16_config` predicate rather
than the old `quant_config.get_name() == "w4afp8"` check.

Co-authored-by: JensenFire <elevenji@outlook.com>
… for spec draft worker(s) (sgl-project#27749, sgl-project#28575, sgl-project#18565, sgl-project#22663, sgl-project#28001, sgl-project#29675, sgl-project#27750)

Squash of the spec-draft distributed-update work and the weight-processing
session work: the latter rewrites iter_draft_runners()/get_model_runners() the
former introduces, so they cannot be applied independently.

  - Draft runners never join the update group, so the target receives the
    broadcast once and the weights are loaded into every selected runner.
  - Begin/EndWeightUpdate session chain: engine / http_server /
    tokenizer_control_mixin (pause-aware locking) -> scheduler ->
    SchedulerWeightUpdaterManager, with {target,draft,all} runner selectors.
  - loader.py: public post_load_weights plus restore_weight/postprocess_weight
    via _apply_quant_method_hook (skips LoRA wrappers).
  - weight checker: skip_tensor_list + role-prefixed overall_checksum over
    get_model_runners(selector), replacing _get_draft_model_runner.

Reimplemented onto v0.5.16, which had already extracted weight updating into
ModelRunner.weight_updater (WeightUpdater): the receive/load split lands there
as receive_weights_from_distributed() + load_weights() rather than on
ModelRunner, and the scheduler fan-out drives runner.weight_updater.*. The
worker-level update_weights_from_{distributed,tensor} entry points on
TpModelWorker / EAGLEWorkerV2 / NGRAMWorker are dropped: the scheduler now owns
the fan-out, and leaving them would be a second path that updates one runner
only. iter_runners() replaces the ad-hoc draft-runner discovery on every spec v2
worker (incl. DFlashWorkerV2). The unit test moves to test/registered/rl/, since
v0.5.16 retired test/srt/ from CI collection.

Co-authored-by: Yueming Yuan <yym022502@gmail.com>
Co-authored-by: JD-ETH <jaedon.guo@gmail.com>
Co-authored-by: maocheng23 <35615230+maocheng23@users.noreply.github.com>
…ht updates (sgl-project#30421)

The per-rank attn_sink slice is built once and cached; an RL weight update
rewrites attn_sink in place, so decode keeps serving the pre-update sink.
Refresh the cache from post_load_weights instead.

Extended past the original PR for v0.5.16, which added deepseek_v4_dspark.py
with the same lazily-cached slice: refresh_attn_sink_cache moves from MQALayer
up to MqaAttentionBase (with an overridable _attn_sink_pad_width, since DSpark
pads q to _PAD_NUM_HEADS rather than padded_num_heads), and
DeepseekV4ForCausalLMDSpark gains the matching post_load_weights hook. Without
this, DSpark + RL would keep the same stale-sink bug this commit fixes for the
non-DSpark path.
… into a host-local checkpoint (sgl-project#30366, sgl-project#28524)

Folds in the two fixes sgl-project#28524 made to this endpoint -- Annotated[..., Body()]
on the handler and keyword construction of PullWeightsReqOutput, which is a
kw_only msgspec Struct and would raise on the positional form. The rest of
sgl-project#28524 (dumper / dump-comparator) is already upstream in v0.5.16 and is dropped.
…h the merged per-role payload

`_merge_checksum_payloads` folds each runner's payload into one, tagging every
entry with its role, so `parallelism_info` is a LIST of per-role entries. That
shape was never validated on v0.5.15 -- v0.5.16 added the msgspec wire structs
plus `msgspec.convert(p, ChecksumInfo)` in `check_weights`, and the wire struct
still declared a single `ParallelismInfo` with no `role`, so every
check_weights('checksum') call would have raised on conversion.

This is the shape miles already consumes: `checksum_utils._gpu_rank` iterates
`parallelism_info` and reads `role_info["rank"]` per role, and its tests cover
the target+draft case sharing one GPU rank.
…t to v0.5.16's parallel state

Two follow-ups to [14/24]'s P2P weight-update support, both caused by v0.5.16
retiring the module-level dp-attention accessors. Found by CI: every GPU stage
that initialises the remote-instance transfer engine died at startup with

    remote_instance_weight_transporter.py:56 init_engine
      -> RankParallelismConfig.from_parallel_state
    ImportError: cannot import name 'get_attention_cp_rank'
                 from 'sglang.srt.layers.dp_attention'

1. `from_parallel_state` imported get_attention_{tp,dp,cp}_{rank,size} from
   dp_attention. v0.5.16 removed all six; the attention-side ranks and sizes now
   live on the runtime ParallelState, so read them from `get_parallel()`.

2. `ParallelismContext` set `dp_attention._ENABLE_DP_ATTENTION_FLAG`, which
   v0.5.16 replaced with `get_flags().dp.enabled`. That assignment would not have
   raised -- it would have created a stale attribute nobody reads, leaving
   `is_dp_attention_enabled()` reporting the ambient value instead of the
   context's and silently building a replica with the wrong sharding. The flag is
   now saved, set and restored on the runtime flags instead, and dropped from
   _DA_GLOBALS.
Three stale references left by the rebase. The cherry-picks applied cleanly
because the surrounding lines matched, so nothing surfaced until the code ran.

1. model_runner.forward: `decode_cuda_graph_runner.num_tokens_per_bs` ->
   `.captured_req_width`. v0.5.16 renamed the per-request capture width (and
   `ModelRunner.decode_num_tokens_per_bs` -> `decode_num_tokens_per_req`), so
   every decode with a captured graph raised AttributeError.
   From [2/25], R3 DeepEP/MTP support.

2. lora/backend/base_backend: `dp_attention.get_attention_tp_size()` ->
   `get_parallel().attn_tp_size`. Same removal as [24/25]; this one is a lazy
   import so it only fired on the MoE-LoRA DP-attention path.

3. lora/backend/base_backend: `layers.utils.cp_utils` ->
   `layers.cp.padding` for get_cp_padding_align_size. The module moved in
   v0.5.16; every other caller in the tree already uses the new path.

Found 1 and 2 in CI; found 3 by then resolving every sglang symbol imported by
lines this branch adds against the v0.5.16 tree, which is how it should have been
checked in the first place.
@hnyls2002

Copy link
Copy Markdown
Collaborator

Thanks @Arist12 for digging into this. Closing, for three reasons:

  1. On current main no production path leaves two full indices pointing at one physical SWA slot. Allocation pairs the two pools one-to-one from a single seq_lens, and the locked-full recovery paths re-point the kept node first, clear the incoming mapping, and only then free the incoming full slots (swa_radix_cache.py:1328-1330, unified_cache/components/swa_component.py:1476-1478). The new tests here construct the shared slot by hand via set_full_to_swa_mapping(full, swa_page.repeat(2)) rather than through a caller.
  2. The illegal-address symptom this PR was written against matches the grouped-free ownership race fixed by Fix SWA ownership across grouped frees #36381 (a deferred free_swa resolved the mapping at flush time, after a cache action had re-pointed the same full index) and extended to free() in [misc] Resolve SWA ownership at enqueue time for grouped free() #36646. Since then, ownership is resolved at enqueue (allocator/swa.py:451).
  3. The reference counting reintroduces host syncs on the hot path: _write_full_to_swa_mapping runs torch.unique plus a read-back on every mapping write, which alloc_extend / alloc_decode call each step, and free_swa gains a torch.unique again. [mem_cache] Make free_swa sync-free on page_size == 1 #36723, [mem_cache] Drop the torch.unique sync from the SWA page expansion #37463 and [mem_cache] Free hybrid SWA pages by one representative per page on page_size > 1 #38159 removed exactly these data-dependent-shape ops from the SWA free path.

If you still hit a shared-slot double free on main after #36381, a repro through a real allocation/cache sequence would be very welcome in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.