Skip to content

cuda pagedattention seqlens shape fix - #31651

Open
apsonawane wants to merge 5 commits into
mainfrom
msrc/cuda-pagedattention-seqlens-shape-fix
Open

cuda pagedattention seqlens shape fix#31651
apsonawane wants to merge 5 commits into
mainfrom
msrc/cuda-pagedattention-seqlens-shape-fix

Conversation

@apsonawane

Copy link
Copy Markdown
Contributor

This pull request introduces improved validation for input tensors in the Paged Attention CUDA operator, ensuring that sequence lengths and block table values are properly checked before computation. It adds a new comprehensive validation function, integrates it into the main compute path, and provides unit tests to verify correctness. Additionally, a minor bug in shape checking is fixed.

Input validation improvements:

  • Added a new function CheckBlockTableAndPastSeqLensValues in paged_attention_helper.h to validate that cumulative_sequence_length, past_seqlens, and block_table values are within expected ranges, preventing invalid memory accesses and logical errors.
  • Integrated this validation into the main compute function in paged_attention.cc, copying relevant device data to CPU and running the checks before computation proceeds.
  • Fixed a bug in CheckSequenceLengthTensors where the logical condition for validating the shape of seqlens was incorrect (&& replaced with ||).

Testing:

  • Added a new test file paged_attention_helper_test.cc with unit tests covering valid and invalid cases for the new validation logic, ensuring robustness against edge cases.

Correct CheckSequenceLengthTensors logic to reject tensors that are not rank-1 or do not match batch_size.

Add CUDA internal helper tests for valid and invalid seqlens lengths.
Add host-side value checks for block_table and past_seqlens before CUDA kernel launch to prevent out-of-range page indexing.

Also require block_table dim1 > 0 and add helper-level regression tests for invalid and valid value cases.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens runtime validation for the CUDA PagedAttention contrib op by adding host-side checks for cumulative_sequence_length, past_seqlens, and block_table, fixes a shape-check bug in CheckSequenceLengthTensors, and adds unit tests for the helper validation.

Changes:

  • Fix CheckSequenceLengthTensors shape validation logic (&&||) for seqlens.
  • Add CheckBlockTableAndPastSeqLensValues and invoke it from PagedAttention<T>::ComputeInternal after copying device inputs to host.
  • Add CUDA-kernel unit tests for sequence-length and block-table/past-seqlens validation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h Fixes seqlens shape guard and adds new value-level validation helper for paged attention inputs.
onnxruntime/contrib_ops/cuda/bert/paged_attention.cc Integrates the new validation helper into the CUDA PagedAttention compute path via D→H copies + sync.
onnxruntime/test/contrib_ops/cuda_kernels/paged_attention_helper_test.cc Adds unit tests covering the new helper validation (plus the seqlens shape fix).
Suppressed comments (1)

onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h:220

  • The past_seqlens range check currently rejects past_length == max_num_blocks_per_seq * block_size even when q_len == 0. The paged attention implementation explicitly supports batches with zero new tokens, so a “no-write” sequence with a full cache can be a valid edge case. Consider allowing past_length == max_cache_sequence_length when q_len == 0, while still rejecting it when q_len > 0.
    const int32_t q_len = q_end - q_start;
    const int32_t past_length = past_seqlens[b];
    if (past_length < 0 || static_cast<int64_t>(past_length) >= max_cache_sequence_length) {
      return ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT,
                             "past_seqlens values must be in [0, max_num_blocks_per_seq * block_size). Invalid value: ",

Comment thread onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h
Comment thread onnxruntime/contrib_ops/cuda/bert/paged_attention.cc Outdated

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we shall not introduce memory copy for performance reason.

tianleiwu added a commit that referenced this pull request Aug 5, 2026
### Description
The Web CI Pipeline failure on PR #31651 was caused by Terser choking on
a specific emitted JS pattern during webpack minification (reported as 4
webpack/Terser errors before `webpack 5.109.2 compiled with 4 errors`).
This change applies a minimal, targeted fix in the web build path to
avoid generating the problematic minifier input while keeping release
minification enabled.

- **Root-cause alignment (job `92166672347`)**
  - Preserved non-fatal ONNX/runtime warnings as-is.
- Addressed only the fatal webpack/Terser path tied to the four
end-of-job minification errors.

- **Minimal build/config correction**
- Updated the relevant web build/config source so webpack no longer
feeds Terser the failing construct.
- Kept production minification behavior intact (no blanket disablement).

- **Focused regression guard**
- Added/updated narrow coverage around the affected web build/codegen
path so this Terser failure mode is caught earlier.

```js
// Representative pattern: preserve minification while avoiding the emitted form that
// triggered the Terser compressor failure in CI.
optimization: {
  minimize: true,
  // targeted handling applied in build path/config, not a global minify-off workaround
}
```

### Motivation and Context
PR #31651’s Web CI Pipeline failed in the webpack production step with
`compiled with 4 errors` and Terser compressor stack frames.
The goal is to remove that exact failure mode with the smallest robust
change, without broad minification disablement or speculative toolchain
churn.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
apsonawane and others added 2 commits August 5, 2026 09:37
…ression

This commit addresses all open review comments from PR #31651:

SECURITY FIX (Copilot Comment #1):
- Enhanced CheckBlockTableAndPastSeqLensValues() with comprehensive validation:
  * Added check that cumulative_sequence_length[0] == 0
  * Added non-negative checks for all cumulative_seqlens values
  * Improved error messages with specific invalid indices
  * Prevents invalid memory access in CUDA kernels

PERFORMANCE FIX (tianleiwu CHANGES_REQUESTED - Blocking):
- Eliminated unconditional device-to-host sync on critical path:
  * Moved validation into existing FA/MEA code path (lines 240-296)
  * Removed separate D->H copies + sync that duplicated existing work
  * Validation now only runs when Flash Attention/MEA is enabled
  * Reuses already-synced cumulative_q buffer where possible
  * Result: Zero overhead for non-FA/MEA paths, minimal for FA/MEA

EDGE CASE FIX (Copilot Suppressed Comment):
- Improved handling of full cache with zero new tokens:
  * When q_len == 0: Allow past_length anywhere in [0, max_cache_sequence_length]
  * When q_len > 0: Require space for both past and current query
  * Proper separation of logic for zero-token vs has-token cases

TEST COVERAGE (Copilot Comment #3):
- Added 5 new comprehensive test cases:
  * CheckBlockTableAndPastSeqLensValuesRejectsNegativeCumulativeSeqLen
  * CheckBlockTableAndPastSeqLensValuesRejectsCumulativeNotStartingAtZero
  * CheckBlockTableAndPastSeqLensValuesRejectsNegativePastSeqlens
  * CheckBlockTableAndPastSeqLensValuesAllowsFullCacheWithZeroTokens
  * CheckBlockTableAndPastSeqLensValuesRejectsFullCacheWithNewTokens
- Comprehensive boundary condition testing

Files changed:
- paged_attention_helper.h: Enhanced validation logic with security checks
- paged_attention.cc: Refactored to integrate validation with existing D->H sync
- paged_attention_helper_test.cc: Added 5 new test cases for edge cases

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can commit the suggested changes from lintrunner.

Comment thread onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h Outdated
Comment thread onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h Outdated
Comment thread onnxruntime/contrib_ops/cuda/bert/paged_attention_helper.h Outdated
Comment thread onnxruntime/test/contrib_ops/cuda_kernels/paged_attention_helper_test.cc Outdated
Comment thread onnxruntime/test/contrib_ops/cuda_kernels/paged_attention_helper_test.cc Outdated
@apsonawane
apsonawane enabled auto-merge (squash) August 5, 2026 16:52
@apsonawane
apsonawane requested a review from tianleiwu August 5, 2026 18:02
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.

4 participants