Skip to content

[Bugfix] Reject invalid GLM kpool indices - #8

Closed
lucamotz wants to merge 1 commit into
ZJY0516:glm-releasefrom
lucamotz:codex/glm53-kpool-index-safety
Closed

lucamotz wants to merge 1 commit into
ZJY0516:glm-releasefrom
lucamotz:codex/glm53-kpool-index-safety

Conversation

@lucamotz

@lucamotz lucamotz commented Aug 27, 2026

Copy link
Copy Markdown

Purpose

GLM-5.3 kpool selection can produce fewer than the requested select_k
entries for short rows. The current destination uses torch.empty, so slots
not written by the top-k backend can retain arbitrary positive values. Pool
expansion then treats any non-negative value as a valid pool ID and can turn an
uninitialized value into an out-of-range sparse-attention gather.

This change adds two defensive layers:

  1. initialize pool-level top-k destinations to -1, preserving the existing
    invalid-index convention for every unwritten slot;
  2. reject pool IDs outside [0, completed_pool_count) during expansion, so a
    malformed positive ID cannot become a token index outside the request's
    completed-pool range.

The regression supplies valid, out-of-range-positive, and negative pool IDs
for a short row and verifies that only the completed pools plus the incomplete
tail are expanded.

This is a focused dependent change for
vllm-project#53906. It targets that PR's source
branch because the GLM kpool implementation is not present on upstream main.
It is independent of #7, which fixes graph
metadata lifetime in different files.

Test Plan

python -m pytest -q tests/kernels/test_kpool_decode_update_batched.py \
  -k expand_pools_rejects_positive_ids_past_completed_pool_count

pre-commit run --files \
  tests/kernels/test_kpool_decode_update_batched.py \
  vllm/model_executor/layers/sparse_attn_indexer_kpool.py \
  vllm/models/glm5next/nvidia/ops/kpool_compress.py

Test Result

  • Applied as one commit to exact glm-release base
    142062f13d16bed254b5d97cc3d371fbd4f7790a.
  • git diff --check: passed.
  • Python syntax parsing for all three files: passed.
  • All configured changed-file pre-commit hooks passed, including Ruff, mypy,
    SPDX, forbidden-import, CUDA-API, and repository-specific validators.
  • The focused pytest passed on two independent NVIDIA GB10 GPUs (compute
    capability 12.1): 1 passed, 30 deselected on each device. The exact
    PR-head test and patched Triton module SHA-256 receipts are recorded in the
    PR conversation.

No private hostnames, paths, credentials, environment files, or raw deployment
logs are included.

This change and PR description were prepared with OpenAI Codex assistance. The
human submitter reviewed the diff and is responsible for the implementation
and reported evidence.


Essential Elements of an Effective PR Description Checklist
  • The purpose and dependent upstream PR are identified.
  • The focused test command is provided.
  • Completed static and two-GB10 CUDA results are explicit.
  • No documentation update is required for this defensive kernel-input fix.

Initialize short-row top-k destinations to -1 and clamp expanded pool IDs to the request completed-pool range before sparse-attention gathers.

Assisted-by: OpenAI Codex
Signed-off-by: Luca Motz <321921718+lucamotz@users.noreply.github.com>
@lucamotz
lucamotz marked this pull request as ready for review August 28, 2026 07:20

Copy link
Copy Markdown
Author

Fresh CUDA regression results from the PR head (fcfcc6ce) on two independent NVIDIA GB10 GPUs (compute capability 12.1).

The exact PR-head test and patched Triton module were mounted read-only into disposable containers based on the qualified GLM-5.3 vLLM image:

python3 -m pytest -q tests/kernels/test_kpool_decode_update_batched.py \
  -k expand_pools_rejects_positive_ids_past_completed_pool_count

Result on each GB10:

1 passed, 30 deselected

Test file SHA-256: 64b8b40993bafaae57eaa9695fad1aa3eb236502487af3d98a751f408725a4b2
Patched kpool_compress.py SHA-256: 40d411d51646a071cf83418cfc731e9a3879fc0e81d4b4b3d4b03b681a8c9d6e

@lucamotz

lucamotz commented Sep 3, 2026

Copy link
Copy Markdown
Author

Now that vllm-project#53906 is merged, I have upstreamed this fix against vllm-project/vllm:main as vllm-project#55201. Closing this dependent fork PR in favor of the upstream PR. Thanks!

@lucamotz lucamotz closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant