Fix memory pool index error by Ying1123 · Pull Request #616 · sgl-project/sglang

Ying1123 · 2024-07-13T23:45:06Z

No description provided.

* Add private KimiK2Detector with parser fixes Key fixes: - Support hyphens in function names (e.g., agent__hf-search) - Support newlines in JSON arguments (re.DOTALL) - Fix concatenated tool calls parsing (use [^<]+ to prevent matching across <| token boundaries) - Add reset() method to prevent memory leaks between requests - Disable structural_tag support as workaround for grammar issues * Apply black formatting to kimik2_detector.py --------- Co-authored-by: Chenxi Li <41864925+ConnorLi96@users.noreply.github.com>

…uble-free producers fixed + named free_slot refusal; residual crasher re-scoped to overlap-scheduler cross-stream race)

…named via GPU coredump — int32 accept_index gather, size disagreement not corruption; default-off stability guard + hermetic tests; wedge family proven not LXC-caused on bare-metal host)

Third wedge, first one measured with nvidia-smi while live: all three cards at 100% SM utilisation with 0% memory utilisation and power far below cap (176/130/162 W). That is a spin-wait kernel, not long-running work, which falsifies this note's own section-13 hypothesis that the queue was busy. After the cycle cap expired the cards fell to 0% / ~20 W and Bar1CollectiveAborted fired. All three ranks were host-blocked at the same line, all three GPUs spinning, and all three reported byte-identical barlink last-ops (broadcast:32, broadcast:24, all_gather:192512). Every 'the ranks disagree' explanation is now falsified three ways: census counts, barlink last-ops, and this. What remains is below the Python level - a sequence-number mismatch on the spinning collective, or a BAR1 flag write not becoming visible to the waiting peer - and separating those needs the device flag words, i.e. a coredump at abort time rather than a hot-path probe. Scope: this wedge is pre-existing (hunter-4 saw it; wedge 1 of this window hit the un-fixed build) and is a different failure from the sgl-project#616 index assert. Soak 2 of the fixed build: 15 min, 277 completions, zero index asserts, against a pre-fix envelope of 192 s.

…arried predict via collective pairing shift — fused single broadcast at both sites incl. weightless-KV receive path; 23.5 min soak clean vs 192 s envelope; wedge-family instruments: flag snapshots, 4096-entry collective history, aligner, triage collector)

…lution fixed — bare py-spy via shell PATH silently disabled every automatic wedge dump; wedge harvest: abort gate stages on time but raise is unbounded inside graph replay, single-cell round skew with rank 0 one collective ahead)

…was structurally blind — status staged on the compute stream joined the wedge, and poll_status_word was never defined on the BAR1 transport so the watchdog's private-stream read silently skipped it; bounded staged wait + ported poll + Bar1CollectiveStalled after sustained expiries; wedge evidence: three ranks stuck in the same all_reduce with rank 0 at layer 6/3420160B vs ranks 1/2 at layer 5/12584960B against identical-size pre-wedge control)

…DATED: rank-divergent prefix-cache evolution — evict_from_tree_cache + UnifiedRadixCache.load_back pinned to the binding rank's threshold so match_prefix stays rank-uniform; on-card: combined reuse+pressure load kills unfixed in 91 s with rank 0 at 265 vs peers 2048 tokens and a different op on dcp:0, fixed tree clean for 25 min (16x) with both triggers demonstrably live (459940 cached tokens / 1079 prefills); nine-run validation table incl. the two null arms documented in NOTE_616g)

…p/owner.py:548 inside the collective window wedged all three ranks — build_dcp_weighted_kv_indices blocked on int(full_indptr[bs].item()) with no timeout while peers spun to the BAR1 cycle deadline (crash 2026-08-07 02:02, plus pre-fix specimens 08-06 16:40/17:46; desync falsified: census counters and 4096-entry collective histories byte-identical across ranks); fix wires the dead total_tokens bypass by forwarding the extend_prefix_lens_cpu mirror through update_single_wrapper and deriving the host total via _dcp_host_total_tokens — the crash dump's own locals carried the exact input [39166]; can-fail proven against the crashed tree; 5 owner-rule suites 58 passed/117 subtests identical base-vs-fixed; deployed sgl-project#616-B floor exonerated by diff scope + prior specimens)

…raph replay ISOLATED_PREP moves out-of-graph attention prep onto a private stream. That prep WRITES buffers the captured graph READS -- cuda_graph_kv_indices and kv_indptr. Today the write-after-read ordering is guaranteed for free, because prep and replay share one FIFO compute stream: prep for step N cannot begin before replay for step N-1 has retired. Moving prep off that stream is precisely what removes the guarantee. The resulting failure does not crash and does not hang. It writes attention indices while the previous replay is still reading them, producing silently wrong output. A stability test scores such a build as a complete success, because the wedge it was built to remove is genuinely gone. Stability evidence is therefore necessary and not sufficient, and this bracket is the missing half. Uses the existing sgl-project#616 index_race_guard: snapshot() before the replay launch, check_stable() after. Both are enqueued on the current stream, so they are ordered around the graph by construction; per the guard's own contract a same-stream comparison must report zero, and any non-zero count is positive proof that another stream wrote the tensor in between. Counting is device-side, so the instrument adds no host sync and cannot perturb the ordering it is measuring. Expected readings, the middle arm being the one that makes the instrument trustworthy rather than merely reassuring: baseline, no isolated prep -> 0 (no false positives) isolated prep, WAR event omitted -> >0 (proves it can fail) isolated prep, WAR event in place -> 0 (the fix is ordered) An instrument never shown to fire is not evidence. Inert unless SGLANG_INDEX_RACE_GUARD=1, which defaults to False. Buffer resolution degrades to guarding fewer tensors on any missing attribute rather than raising, since a falsifier that can take down serving is worse than no falsifier. Tested (CPU, no GPU held; all three cards were held by another session): - AST parse + py_compile of the modified module: OK - helper returns [] for no backend and for a backend with no attributes - single-backend shape yields cuda_graph_kv_indices + kv_indptr - multi-backend shape additionally yields per-step kv_indptr[i] - zero-numel tensors and non-tensor attributes are skipped - no degradation path raises NOT yet exercised on a GPU: the three-arm table above is unrun, so this commit adds the instrument only and claims no result from it.

…trol message Reverts the request-injection vehicle entirely. Injection put a SECOND blocking channel -- a control message on the PP point-to-point chain -- alongside the flip's group reduction, with no global order between them, and it deadlocked twice on metal: async forward, arm in the same pass: rank0 on_round -> bounded_collective, ranks1-2 -> _pull_raw_reqs synchronous forward (the "fix"): rank0 -> point_to_point_pyobj send, ranks1-2 -> hidden-states recv Both are instances of the fork's standing collective family (sgl-project#431, sgl-project#616, sgl-project#639): two independent blocking channels with no order between them. Patching the ordering case by case (defer-by-one was the third attempt) fights the pattern; removing the channel ends it by construction. A control message riding the PP data chain can always be scheduled against a rank that is currently blocked in the other channel. The channel was never needed, because THE SIGNAL IS ALREADY REPLICATED. Under PP every stage receives the same request stream over the chain and runs the same scheduling decisions on it, so waiting_queue and running_batch hold the same values on every rank -- visible directly in the scheduler's log, where the three stages print identical #running-req, #queue-req and #pending-token at the same timestamp. decide is a pure function of those values, so every rank computes an identical verdict and arms ITSELF via the same arm_phase_flip the RPC path calls. Zero messages, no delivery, nothing to order. The existing machinery then does what it was built for: ranks arm at slightly different rounds, an armed rank enters the reduction only once locally PARKED (owing no pipeline send), and peers converge on their own arm+drain -- "MIN-skew is legal" (PhaseFlipRuntime.on_round). Skew is absorbed; only DISAGREEMENT is fatal, and disagreement cannot arise from identical inputs. The reduction's equality family (epoch, config_fp, direction once armed) is the standing loud check on that assumption. Evaluated from _phase_flip_on_round, immediately before the consensus round that carries the arm, on every rank and in both loop families. A refused arm releases the dwell clock so the policy can re-decide rather than sit out a window it never used. request_receiver.py and scheduler_pp_mixin.py are byte-identical to their pre-policy state again; a test asserts that by source inspection, so neither can regrow a policy hook or an internal-arm special case. Tests: 313 passed in the sgl-project#631 flip family, CPU-only. New: three ranks with identical replicated inputs produce identical arms; the policy returns no request; a refusal releases the dwell. Operational acceptance under load NOT yet achieved -- boot with this design is running.

…oes not Written against the code with no changes made, so engine work is ready-to-build when the boot cycle frees. ## The membership is not the problem Confirmed from the restored boot: tp_size=1, pp_size=3, dcp_size=1. So world_size=3 and the PP group is built as range(pp_group_idx, world_size, world_size // pp_size) = range(0,3,1) = [0,1,2] (parallel_state.py:3355-3372) -- already every stage. Generally, for tp_size>1 there is one PP group per TP position whose members are that position's pipeline ranks, which is exactly the set across which KV should be token-sharded. No new rank layout has to be invented at any topology. ## But _PP cannot be used as-is, and the reason is silent wrongness Attention backends read attn_dcp_size/attn_dcp_rank ONCE in the constructor and cache them; dcp_enabled is `get_dcp_group_no_assert() is not None and dcp_size > 1` (dcp_group_guard.py:14-19). Given a communicator that is not a DCP-TYPED group they cache dcp_size=1, uneven_dcp_owner_bounds() returns None on EVERY rank, the owner rule is bypassed, and every rank treats every global slot as its own local row -- all ranks write the same token to the same row, last write wins, each reads the whole sequence as local. dcp_group_guard.py:21-27 states it exactly: "The result is silently wrong output. There is no error and no hang to follow." So B1 is a DCP-typed group whose rank sets come from the PP dimension instead of from chunking TP groups. ## Change point and the ordering constraint parallel_state.py:3143-3159 builds _DCP by chunking each TP group; with tp_size=1 every TP group is one rank, so every DCP group is one rank -- which is why PP prefill runs dcp_size=1 with no usable group. LOAD-BEARING ORDERING: _DCP is built at :3152, _PP at :3365, AFTERWARDS. The new construction therefore MUST NOT read _PP; it must recompute the same pure arithmetic inline. Reading a group that does not exist yet is precisely the ordering class the DCP guard exists to catch. The group must exist before attention-backend construction, and server_args.dcp_size must equal the participating stage count, so that assert_dcp_group_formed (dcp_group_guard.py:63) is a no-op -- and if the ordering is ever broken it names the construction step rather than failing downstream. ## Refusal conditions, each traced to an existing guard dcp_size != participating stages -> dcp_group_guard.py:63-100 page_size != 1 -> dcp_group_guard.py:170-177 PD decode without uneven-TP+mooncake -> dcp_group_guard.py:139-166 tp_size>1 with ambiguous shard set -> NEW, no existing guard page_size==1 is already mandatory for Option A independently (sgl-project#706: a multi-token page would span owner ranks), so the two constraints agree rather than compete. ## Recorded as unknown rather than assumed benign The sgl-project#616 group-MIN-floor interaction, whether an extra communicator carries a budgeted per-group cost, and whether any census/registry must be told about a new group. Survey in flight. 190 tests green, hermetic, codespell clean. No code changes to engine paths.

…nd fold the B1 survey ## sgl-project#545 -- attach, resize and detach the HiCache disk tier at runtime mem_cache/disk_tier_control.py, flag-gated, 14 hermetic tests. Built through the existing HiCacheStorage/HiCacheFile abstraction: the controller owns the BUDGET AND LIFECYCLE, the bytes stay with the backend. No second directory layout is introduced, so the sgl-project#706 canonical-page store (not on this branch yet) shares the same backend rather than fighting it. Three rules, each because its violation is SILENT: 1. SHRINK EVICTS DOWN TO THE BOUND; IT NEVER TRUNCATES. A truncated page is not a miss -- it reads back short and wrong, which no cache-hit metric shows. Test: 90 MB across nine pages shrunk to 50 MB evicts four WHOLE pages; every surviving page keeps its full size. 2. DETACH IS REFUSED WHILE ANY PAGE IS ONLY-COPY HERE, naming the count and an example key. Detaching would destroy them and the later miss would have no visible cause. force=True exists for an operator who has decided the loss is acceptable, and says so loudly in the log. 3. A SHRINK THE EVICTOR CANNOT HONOUR IS REFUSED, NOT PARTIALLY APPLIED. Accepting a bound the tier is not under would make the reported capacity a lie, and the only way to reach it would be truncation. Capacity is left unchanged on refusal. Also: a second attach is refused (two directories under one controller would give the evictor two budgets and one bound), operations on an unattached tier refuse, and the flag gates every mutation because attaching or resizing a live L3 changes what the evictor is allowed to delete. ## sgl-project#616 -- B1 survey folded into DESIGN_704, revising three things I verified the survey's two load-bearing claims at file:line before folding. 1. "A DCP-TYPED GROUP" WAS IMPRECISE. There are no typed group classes: every group is a bare GroupCoordinator (parallel_state.py:564) via init_model_parallel_group (:2550). "Typed" here means exactly a module global, a named getter, and a routing flag. B1 subclasses and registers nothing -- a smaller and more precise change than the earlier draft implied. 2. THE PRECEDENT ALREADY EXISTS IN-TREE; REUSE IT. initialize_phase_flip_secondary_groups (:3422-3517) ALREADY creates a DCP group in the same world as a PP group. Verified at :3470-3482: it plans flip_tp -> flip_dcp -> flip_pp in fixed order, then :3484-3497 verifies a world-wide manifest by all_gather with an equality check BEFORE creating anything. My earlier draft proposed reconstructing PP rank arithmetic inline; that is unnecessary -- the precedent computes the same range(idx, world_size, num_pp_groups) layout and already sequences it. 3. THE ORDERING WINDOW IS NARROWER THAN FEARED. _DCP at :3152, _PP at :3365, and only the kvso-spill block (:3185-3188) reads _DCP between them. Routing precedence must also be chosen deliberately: get_dcp_group resolves flip route first (:2709), then kvso-spill (:2718), then primary (:2720). Inheriting whichever branch matches first is how a decoupled read would silently land on the flip's secondary group during a phase change. ## NAMED PREREQUISITE, before B1 rather than inside it pp>1 AND dcp>1 is not refused anywhere in the group path, and ParallelContext.dcp_enabled (runtime_context.py:331-337) returns True whenever a DCP group exists with dcp_size>1. Creating B1's group therefore makes dcp_enabled TRUE ON PP PREFILL RANKS, contradicting the assumption dcp_group_guard.py:38-42 states in prose. The failure shape is what makes this worth naming: the guard itself would still PASS, because both sides would read the same new size. What breaks is every OTHER consumer of the documented invariant. The combination is unreachable today, so the contradiction is latent -- and B1 is exactly the change that makes it live. Decide it before building: either accept pp>1 and dcp>1 deliberately and audit the dcp_enabled consumers, or refuse the combination and give B1 its own flag instead of overloading dcp_size. 35 mem_cache tests green, hermetic, ruff + codespell clean.

…uting slot Built per the corrected spec after the sgl-project#616 survey. Reuses the phase-flip precedent instead of inventing a pattern; 17 hermetic tests (CVD=""), no CUDA, no torch.distributed, no process group. ## Own global + getter + routing flag, NOT dcp_size Per the operator decision. There are no "typed group" classes in this tree -- a type IS a module global, a named getter and a routing flag (sgl-project#616 survey, parallel_state.py:564 / :2550). So B1 adds _DECOUPLED_KV, _DECOUPLED_KV_ACTIVE, get_decoupled_kv_group{,_no_assert} and set_decoupled_kv_active. Overloading dcp_size would have made ParallelContext.dcp_enabled (runtime_context.py:331-337) report True on PP prefill ranks, contradicting the invariant dcp_group_guard.py:38-42 documents -- and the guard would still PASS, so nothing would announce it. The separate flag leaves that invariant TRUE and needs no consumer audit. ## Creation reuses the precedent, verbatim in shape plan -> world-wide manifest all_gather -> equality check -> create, matching initialize_phase_flip_secondary_groups (:3466+). Nothing is created before every rank agrees on the same plan; a divergent create order is the rank-divergent-collective family and dying at the check is the cheap failure. _manifest_salt is carried for the same reason the precedent carries it: so a test can prove the check CAN fail. Membership is the PP layout -- range(idx, world_size, world_size // pp_size) -- computed INLINE. That is load-bearing: _DCP is created at :3152 and _PP only at :3365, so _PP does not exist yet at B1's creation point. A test forces _PP to None and asserts the plan is unchanged. ## The sgl-project#616 labeled gap, closed loudly refuse_pp_dcp_combination fires in the group path BEFORE any group is created: pp>1 with dcp>1 now raises by name instead of staying latent-but-unexercised. ## Routing precedence, pinned rather than read B1 slots deliberately LAST before the primary fall-through: flip (:2709) -> kvso-spill (:2718) -> B1 -> _DCP. It claims only what would otherwise reach _DCP, so the flip keeps the TP decode phase (B1 is a PP PREFILL mechanism and cannot be legitimately active there) and a spill forward keeps its dedicated serial communicator. Inactive is byte-identical to the pre-#704b path. CAN-FAIL PROVEN, not asserted: moving the B1 branch above flip/spill in the source makes exactly three tests fail (flip-outranks, spill-outranks, and the full precedence table); reverting restores 17 passed. First-match routing is the failure mode my own fold named, so the position is pinned by a table rather than by reading order. ## Notes Structure verified by ast membership, not by "it parses" -- the lesson from the sgl-project#464 near-miss where a helper insertion silently turned a method into a nested function. All six new helpers confirmed module-level and the precedent function intact. parallel_state.py deliberately NOT run through ruff format (it reformats unrelated legacy code); diff is 162 insertions, 1 deletion. New annotations match the file's existing typing.List/Optional style rather than mixing in collections.abc. Still NOT wired to a caller: nothing arms set_decoupled_kv_active yet, and the attention path does not read the group. That is the next slice and it needs the #704b pool work (R6) first.

…request Boot 12 (7b855f6) died on PP0 at 21:46:06 with `AssertionError: reusing request must be chunked or have committed KV` (memory_pool.py:395), via get_new_batch_prefill -> prepare_for_extend -> alloc_for_extend -> alloc_req_slots -> HybridReqToTokenPool.alloc. ROOT, from the ledger and not from inference. One second earlier the same rank logged, for the same rid: sgl-project#969 voided-request release failed for 5708abdd579e4f7097ab97ed796481ea: Committed KV cache already freed (self.kv_committed_len=4096) `_release_voided_request`'s never-raise contract caught that, logged it, and fell through to `reset_for_retract`. The contract is right -- an instrument that raises while cleaning up after a divergence turns one defect into two -- but it only decided that this frame does not PROPAGATE the failure. It never decided what the half-released request IS afterwards. The answer was: a request still holding `req_pool_idx`, whose `inflight_middle_chunks` and `kv_committed_len` the reset then zeroes -- and those two fields are the allocator's only evidence that a retained row is legitimate. It was re-queued, re-admitted one second later, and killed rank 0. The assert is correct and is the only thing that caught this. The defect is upstream of it. CLASS: warn-then-continue. The compensator converts a partial failure into a live, corrupt, re-admissible object. This is the catalogued `warn-then-continue swallow` shape, and the cure is not to remove the swallow but to make the post-failure STATE explicit. FIX, at the junction: on a failed release, hand the row back -- mamba slot first, then the req-pool row, the ordering `_release_dynamic_chunk_probe` already documents. The reset below drops prefix, pages and geometry anyway, so the request re-prefills regardless; the row it keeps buys nothing and justifies nothing. `free_slot` carries its own membership scan and REFUSES an already-free row (sgl-project#616), so a release that failed AFTER returning the row is caught by name and logged instead of corrupting the free list -- both outcomes are reported with the rid. NOT CLOSED, and deliberately not guessed at: why `pop_committed_kv_cache` found `kv_committed_freed` already True at `kv_committed_len=4096`. That producer is a second defect. This change contains it and names it in the log (`sgl-project#993 INCOMPLETE RELEASE DISOWNED`), so the next boot measures how often a release fails at all -- a number no boot of this family has ever had. FUTURE CHECK: any new raise inside `release_req` now ends in a disowned row plus a named line, not in an allocator assert three passes later. Evidence: desk. py_compile + import smoke + wiring assertion on `_release_voided_request`. Belegstufe: DESK-BEWIESEN.

fix memory pool

10a4c4d

Ying1123 merged commit 5949b1c into main Jul 13, 2024

Ying1123 deleted the fix-memory-pool branch July 13, 2024 23:45

timethink pushed a commit to timethink/sglang that referenced this pull request Mar 9, 2025

Fix memory pool index error (sgl-project#616)

ebbdd57

efschu added a commit to efschu/htsglang that referenced this pull request Aug 6, 2026

Merge branch 'fix/req-pool-oob-616' (sgl-project#616: two req-pool do…

07b23d9

…uble-free producers fixed + named free_slot refusal; residual crasher re-scoped to overlap-scheduler cross-stream race)

Navigation Menu

Uh oh!

Fix memory pool index error - #616

Fix memory pool index error#616
Ying1123 merged 1 commit into
mainfrom
fix-memory-pool

Ying1123 commented Jul 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Ying1123 commented Jul 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant