Skip to content

perf(sglang): isolate sidecar request shutdown waiters - #14727

Merged
jthomson04 merged 2 commits into
mainfrom
jthomson04/sglang-sidecar-child-cancellation
Sep 14, 2026
Merged

jthomson04 merged 2 commits into
mainfrom
jthomson04/sglang-sidecar-child-cancellation

Conversation

@jthomson04

@jthomson04 jthomson04 commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Give each SGLang gRPC generation request a child of the engine shutdown token. This separates cancellation waiters across concurrent streams while preserving engine-wide shutdown propagation. The native HTTP path is unchanged.

Add one regression test using the existing mock gRPC server to check that cancelling one request leaves other requests running, engine shutdown cancels the remaining streams, and requests created after shutdown finish as cancelled. Existing tests cover native HTTP cancellation and the gRPC Abort RPC, but not propagation through the gRPC generation shutdown token. The new test reuses the existing gRPC integration fixture.

Validation

  • cargo test --locked -p dynamo-sglang-mocker --test sidecar request_cancellation_is_isolated_and_shutdown_reaches_grpc_streams: passed.
  • Existing SGLang sidecar and mock package tests passed before adding the regression test.
  • cargo fmt -p dynamo-sglang-sidecar -p dynamo-sglang-mocker -- --check and git diff --check passed.
  • The Tyche campaign validated this change with streaming and shutdown checks. No isolated SGLang throughput gain is claimed because the original baseline failed validation.

Related Issues

  • Confirmed — no related issue.

Summary by CodeRabbit

  • Bug Fixes
    • Improved cancellation handling for concurrent generation requests so stopping one request no longer interrupts other active streams.
    • Engine cleanup now reliably closes remaining streams and reports cancelled completion status for affected requests.
    • Added coverage to verify request isolation, cleanup behavior, stream closure, and timely completion.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 requested review from a team as code owners September 11, 2026 15:50
@github-actions github-actions Bot added the perf label Sep 11, 2026
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 283d2b62-1c99-4559-b23c-f84fffd78d59

📥 Commits

Reviewing files that changed from the base of the PR and between fccaf24 and c97739a.

📒 Files selected for processing (2)
  • lib/mocker/servers/sglang/tests/sidecar.rs
  • lib/sidecar/sglang/src/engine.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Walkthrough

The SGLang engine now derives a child cancellation token for each request. An integration test verifies isolated stream cancellation, engine cleanup cancellation, post-cleanup request handling, and prompt completion.

Changes

SGLang cancellation behavior

Layer / File(s) Summary
Per-request cancellation token
lib/sidecar/sglang/src/engine.rs
Generation derives a child cancellation token from the engine-wide cancellation token.
Concurrent stream cancellation test
lib/mocker/servers/sglang/tests/sidecar.rs
The asynchronous test verifies isolated request cancellation, engine cleanup cancellation, post-cleanup cancellation, and prompt completion.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c9773

Request cancellation remains isolated while engine shutdown still cancels active and future streams. No merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: isolating SGLang sidecar request shutdown waiters.
Description check ✅ Passed The description explains the implementation, regression test, validation results, unchanged native HTTP path, and related-issue status. It does not include a dedicated "Where should the reviewer start…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 enabled auto-merge (squash) September 11, 2026 16:08
@jthomson04
jthomson04 merged commit 250213a into main Sep 14, 2026
121 checks passed
@jthomson04
jthomson04 deleted the jthomson04/sglang-sidecar-child-cancellation branch September 14, 2026 17:45
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.

2 participants