Skip to content

fix(sglang): handle health signals from the tokenizer socket - #2879

Merged
slin1237 merged 1 commit into
smg-project:mainfrom
minghe-zty:fix/sglang-tokenizer-health-signals
Oct 8, 2026
Merged

slin1237 merged 1 commit into
smg-project:mainfrom
minghe-zty:fix/sglang-tokenizer-health-signals

Conversation

@minghe-zty

Copy link
Copy Markdown
Contributor

Description

Problem

SGLang 0.5.21 can send a HealthCheckOutput directly to the tokenizer socket
while the scheduler is busy. The dedicated tokenizer loop currently only
dispatches communicator responses, so it treats that health signal as unknown
and does not refresh last_receive_tstamp. The custom gRPC HealthCheck RPC can
therefore wait until its 20-second timeout even though the scheduler responded.

The busy-path signal is constructed with http_worker_ipc only, so its
inherited rid can be None:
https://github.com/sgl-project/sglang/blob/v0.5.21/python/sglang/srt/managers/scheduler.py#L4533-L4542

Solution

Recognize HealthCheckOutput on the dedicated tokenizer socket before
communicator dispatch. Every such signal refreshes scheduler liveness, but only
a signal with a non-empty request ID is forwarded to
_handle_health_check_output(). This avoids a spurious unknown request: None
warning for SGLang's request-less busy-path signal while preserving completion
for request-backed health outputs.

Changes

  • handle tokenizer-socket HealthCheckOutput messages
  • refresh last_receive_tstamp for both request-backed and request-less health
    signals
  • skip request handling when rid is missing or empty
  • add engine-free regression coverage for rid=None, an empty rid, and a
    normal health-check request ID

Test Plan

  • Before the fix, all three new cases failed and the tokenizer loop logged the
    health output as an unknown type
  • python -m pytest tests/test_sglang_tokenizer_health.py tests/test_health_probe_logging.py -q
    -- 11 passed
  • python -m ruff check ... -- passed
  • python -m ruff format --check ... -- passed
  • python -m codespell_lib ... -- passed
  • python -m py_compile ... -- passed
  • git diff --check -- passed

Rust-wide cargo checks were not run locally because this environment has no
Rust toolchain. The change is confined to the Python SGLang gRPC servicer and
its engine-free tests.

Checklist
  • cargo +nightly fmt passes
  • cargo clippy --all-targets --all-features -- -D warnings passes
  • (Optional) Documentation updated
  • (Optional) Please join us on Slack #sig-smg to discuss, review, and merge PRs

SGLang sends HealthCheckOutput directly to the tokenizer socket while the scheduler is busy. Refresh liveness for every signal and only dispatch signals with a request id, so request-less busy-path signals do not produce spurious unknown-request warnings.

Signed-off-by: minghe-zty <116366231+minghe-zty@users.noreply.github.com>
@github-actions github-actions Bot added grpc gRPC client and router changes tests Test changes labels Oct 8, 2026
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2e279aa5-abf4-4cff-a7ec-5b1f312af948
📥 Commits

Reviewing files that changed from the base of the PR and between 21d5e44 and f5bf0a7.

📒 Files selected for processing (2)
  • grpc_servicer/smg_grpc_servicer/sglang/request_manager.py
  • grpc_servicer/tests/test_sglang_tokenizer_health.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Tokenizer health checks now refresh service liveness when received. Checks without a request ID are not dispatched as requests, while checks with an ID are handled appropriately.

Walkthrough

The tokenizer loop now refreshes liveness when it receives a health signal and calls the health-check handler only when the signal has a truthy request ID. Tests cover missing, empty, and truthy request IDs.

Changes

Tokenizer health signal handling

Layer / File(s) Summary
Health signal handling and validation
grpc_servicer/smg_grpc_servicer/sglang/request_manager.py, grpc_servicer/tests/test_sglang_tokenizer_health.py
The tokenizer loop refreshes the receive timestamp for health signals and calls the health-check handler only when rid is truthy. Tests check these cases and verify that communicator output is not dispatched.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f5bf0

The change lets busy-path health signals refresh liveness, so the health check no longer waits for its timeout. No merge-blocking risk was found in the supplied changes.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f5bf0

This is a narrow health-check behavior change, with no identified expansion of access or privileges. Production signal freshness and duplicate-delivery behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated effect is scoped to health evaluation and request state owned by a GrpcRequestManager instance. The inspected change introduces no mechanism for extending authority into another service, tenant, or data store.

Trust Boundaries and Controls

  • observed — The new branch operates after the existing recv_pyobj receive operation on the same scheduler-output socket. It adds no network listener or credential authority and removes no authentication control. This does not establish that deployed socket access is restricted; endpoint permissions and upstream transport controls were not available.

Resilience and Maintainability Implications

  • observed — Request cleanup remains idempotent dictionary removal, invoked by the request generator's finally block and by HealthCheck on success or timeout. Late outputs whose IDs have been removed are rejected. Tests cover falsey and normal IDs, but do not establish delayed-delivery, duplicate, or cancellation-race behavior end to end.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely describes the main change: handling health signals received from the SGLang tokenizer socket.
Description check ✅ Passed The description is directly related to the changeset. It explains the tokenizer-socket health signal problem, the liveness update, request ID handling, tests, and validation results.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@slin1237
slin1237 merged commit 110ee08 into smg-project:main Oct 8, 2026
50 of 52 checks passed
@slin1237 slin1237 mentioned this pull request Oct 8, 2026
1 of 4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grpc gRPC client and router changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants