test(sidecar): add isolated SGLang unit coverage

[JulienDarve]

Summary

Extend SGLang's unit coverage using the backend-local Rust test layout from #15243. Cover discovery, worker configuration, typed request/response conversion, opaque HTTP envelopes and tracing without starting an inference engine. Preserve existing assertions and move the protocol suites into adjacent request and response child modules.

Fix the reproduced hidden stop-token leak: numeric stop reasons are exposed only when explicitly requested by the caller. Document the test layout and the boundary between isolated units, existing sidecar–Mocker integration tests and real-engine validation.

Stacked on #15243 at 05a8d2167e.

Validation

• Common/vLLM/SGLang all-targets tests: 232 passed, including 91 SGLang tests.
• Existing SGLang and vLLM sidecar–Mocker integration suites: 4 passed each, for 240 tests total.
• SGLang Clippy with warnings denied, formatting, pre-commit and CODEOWNERS checks passed.
• The stop-token regression failed before the fix and passed afterward.
• These local results cover the code at 22ca1e1993. The latest push includes [skip ci]; full CI was not rerun.
• Full-workspace and native-engine/GPU suites were not run locally.

Related Issues

• Stacked on fix(sidecar): preserve terminal metadata and add isolated unit tests #15243.
• Linear: https://linear.app/nvidia/issue/DIS-2942

Implemented with Codex assistance.

Summary by CodeRabbit

• Bug Fixes
  • Improved stop-reason reporting so numeric stop-token matches are recognized only when they correspond to a configured, user-visible stop token.
• Tests
  • Expanded coverage for SGLang request and response handling, routing, service discovery, metadata validation, and RPC outcomes.
• Documentation
  • Updated the testing guide with SGLang test setup and instructions for running its tests.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 124f2649-88c8-4d23-a269-275e4990602d

📥 Commits

Reviewing files that changed from the base of the PR and between 05a8d21 and f326d9a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• lib/sidecar/sglang/Cargo.toml
• lib/sidecar/sglang/src/client.rs
• lib/sidecar/sglang/src/engine.rs
• lib/sidecar/sglang/src/native_http.rs
• lib/sidecar/sglang/src/protocol.rs
• lib/sidecar/sglang/src/protocol/request_tests.rs
• lib/sidecar/sglang/src/protocol/response_tests.rs
• lib/sidecar/testkit/README.md

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

---

Walkthrough

The SGLang sidecar adds broad tests for metadata discovery, engine setup, request conversion, native HTTP, RPC behavior, and response parsing. Numeric terminal stop-token handling now checks the request’s visible stop-token IDs. The test guide and documented library-test command now include SGLang.

Changes

SGLang sidecar

Layer / File(s)	Summary
Client metadata and RPC behavior lib/sidecar/sglang/src/client.rs	Tests cover metadata parsing and validation, model and tokenizer path selection, model-card and context-length fallbacks, streaming requirements, gRPC error mapping, and RPC deadlines.
Engine discovery and terminal outputs lib/sidecar/sglang/src/engine.rs	from_parsed passes the server discovery result to from_discovered. Tests cover worker configuration, discovery roles and addresses, engine registration, and KV-event validation. Prefill and decode terminal outputs receive the request’s stop conditions.
Protocol request conversion and handoff lib/sidecar/sglang/src/protocol/request_tests.rs	Tests cover native request mapping, optional and unsupported controls, routing, bootstrap validation, and prefill-to-decode handoff.
Native HTTP request and transport behavior lib/sidecar/sglang/Cargo.toml, lib/sidecar/sglang/src/native_http.rs	Tests cover native request fields, prefill sampling and handoff values, trace-context forwarding for HTTP and gRPC, output conversion, and HTTP discovery. The crate adds tracing-subscriber as a dev-dependency.
Response parsing and terminal metadata lib/sidecar/sglang/src/protocol.rs, lib/sidecar/sglang/src/protocol/response_tests.rs, lib/sidecar/testkit/README.md	Numeric matched stop values now produce an integer stop reason only when they are nonnegative, fit in u32, and appear in the configured visible stop-token IDs. Tests cover response metadata, logprobs, errors, and stop tokens. The guide documents SGLang test locations and includes its library tests in the documented command.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to f326d

The change expands SGLang test coverage and prevents unrequested numeric stop-token metadata from being exposed. No concrete merge-blocking issue remains in the supplied evidence; merge after normal checks pass.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 77 functions across 6 files. (2 skipped: 2 …
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 identifies the main change: adding isolated SGLang unit-test coverage.
Description check	✅ Passed	The description explains the scope, validation results, regression fix, test boundaries, and related work. It does not use all template headings and does not select the required issue-link or no-issue…

---

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

[nv-anants]

toml/lock changes lgtm

[dmitry-tokarev-nv]

Approved at 857f465a19. I found no blocking defect. The two P3 notes below do not block the merge.

The stop-token fix works in the sidecar, the moved tests keep their checks, and the new dev-dependency is clean.

What I ran at 857f465a19 and at the base 05a8d2167e, with controls.

• SGLang crate, cargo test --locked --all-targets: the base passed 43 tests and the head passed 88, with 0 failures. The description reports 91 tests at 22ca1e1993, and 857f465a19 removed 3.
• The head lib tests passed in 20 of 20 runs.
• cargo clippy -p dynamo-sglang-sidecar --no-deps --all-targets -- -D warnings and cargo fmt -p dynamo-sglang-sidecar -- --check are clean at the head. A planted len() == 0 made clippy fail, so the check works.
• Stop reason through the real sidecar generate(): in my copy only, I made the Mocker SGLang server end each stream with {"type": "stop", "matched": 128001}.

stop lists in the request	base stop_reason	head stop_reason
none	Some(Int(128001))	None
hidden [128001]	Some(Int(128001))	None
user [576], hidden [128001]	Some(Int(128001))	None
user [128001], hidden [128001]	Some(Int(128001))	Some(Int(128001))

Without that change, every case ended with Length and no stop reason on both trees.

• If a request lists "stop_reason" in nvext.extra_fields, the HTTP response shows this value as nvext.stop_reason. I found no other HTTP field that shows it. In a probe of the chat delta generator, StopReason::Int(128001) became {"stop_reason":128001} in nvext with that field. Without the field, the chunk had no nvext.
• If its own decoder sees a hidden stop token or no stop, the frontend keeps the engine value (lib/llm/src/backend.rs:501-506). I read that step but did not run it. So before the fix, a client that asked for stop_reason got the EOS id after an EOS stop. After the fix, that client gets no stop_reason in this case. /inference/v1/generate returns no stop_reason, so its callers see no change.
• If I put back the base line value.as_i64().map(StopReason::Int), the regression test user_stop_tokens_are_preserved_and_system_stop_tokens_are_hidden fails. Five more mutants, one for each row of its table, also fail it.
• Mutants of the logprob rank, the 4xx status boundary, and the merge and dedupe of stop ids each fail a new request or response test.
• The 10 moved tests that keep their names have the same assertions. Only rustfmt line joins and the new &StopConditions::default() argument differ.
• 857f465a19 removed 3 moved tests with 8 assertions. I restored them and ran one mutant for each assertion. Each mutant also fails a test that remains. For the message check, that test is an older native_http test (note 1).
• tracing-subscriber is a dev-dependency at the workspace version. The lock on main already has 0.3.23, and this PR adds only the edge. cargo metadata --locked passes on the base and the head. It fails on the base with the new Cargo.toml line and the old lock, so the check works.

CI did not run the Rust jobs at this head. The last three commits carry [skip ci], so rust-tests (.), rust-clippy (.) and pre-commit last ran at 791a15a2c4. My local runs cover only the SGLang crate.

[P3] A lost engine message does not fail terminal_errors_preserve_backend_category_and_details. lib/sidecar/sglang/src/protocol/response_tests.rs:150. Its message "generation failed" is part of the fallback text "SGLang generation failed". Please use a message that the fallback does not contain, for example "prefill allocation failed" from the removed test.

Mutant .get("message") to .get("msg") in terminal_failure, before and after that change.

message in the test	protocol:: tests with the mutant
"generation failed" (head)	34 of 34 pass
"prefill allocation failed"	terminal_errors_preserve_backend_category_and_details fails

Without the mutant, both versions pass 34 of 34. The removed abort_terminal_preserves_failure_metadata_as_error caught this mutant. In the lib tests, only native_http::tests::prefill_stream_reports_failures_after_success_headers still catches it.

[P3] No test checks that generate() passes the request's stop conditions to terminal_from_meta. lib/sidecar/sglang/src/engine.rs:321. If that call gets default stop conditions, the sidecar drops the stop reason for a stop token that the caller asked for. All 86 lib tests still pass then. Please add an engine-level case, for example a Mocker option that sends a numeric matched.

Mutant: request.stop_conditions replaced by StopConditions::default() at engine.rs:321.

• All 86 lib tests pass with the mutant.
• In the Mocker probe above, the case with user [128001] and hidden [128001] gives None instead of Some(Int(128001)). The other cases do not change.
• The Mocker SGLang server always ends a stream with {"type": "length"} (lib/mocker/servers/sglang/src/server_request.rs:131). The sidecar tests that use the Mocker cannot reach this path today.

[dmitry-tokarev-nv]

Approved at 08798943e6. The merge with main keeps the change of this PR and takes #15243 as it landed. I found no new defect.

Two P3 notes from my review at 857f465a19 are still open. They do not block the merge:

• [P3] A lost engine message does not fail terminal_errors_preserve_backend_category_and_details. lib/sidecar/sglang/src/protocol/response_tests.rs:150.
• [P3] No test covers the stop conditions that generate() passes to terminal_from_meta. lib/sidecar/sglang/src/engine.rs:363.

The merge commit takes main for the files of #15243 and adds two small hand edits.

• I ran git merge-tree --write-tree 857f465a19 3aa1d3d1d5 again. It gives 11 conflicted files. The committed tree differs from this result only in these 11 files.
• 9 of the 11 files belong to #15243. The commit takes the main version of each one. It also deletes lib/sidecar/vllm/src/json.rs, as main does.
• lib/sidecar/sglang/src/engine.rs has one conflict. The only new lines are the bootstrap_discover(..., bootstrap).await? call in from_parsed_async. This call keeps the async discovery of #15051 and the from_discovered split of this PR.
• lib/sidecar/testkit/README.md has 8 conflicts. The only new lines rewrap one sentence from main. The file keeps the SGLang rows of this PR and the testkit rows of main. Each path in its file tree exists at the head.
• Outside the 9 files of this PR, the head tree is identical to 3aa1d3d1d5. So this PR reverts nothing that #15243 landed.
• The diff of this PR is the same as before in 7 of 9 files. In engine.rs, only the discovery call changed, so +473/-4 became +477/-4. In the README, only the rewrapped sentence changed, so +20/-6 became +22/-8.

The head passes 284 tests, the stop-token fix holds, and 20 of 22 mutants fail a test.

I ran the 4 sidecar crates with cargo test --locked --all-targets and Rust 1.96.1, on the head and on main at 50889c0e37:

test binary	main	head
dynamo_sglang_sidecar lib	41 passed	86 passed
dynamo_sidecar_common lib	19 passed	19 passed
dynamo_vllm_sidecar lib	120 passed	120 passed
dynamo_trtllm_sidecar lib	51 passed, 2 ignored	51 passed, 2 ignored
executable and proto_contract	8 passed	8 passed
total	239 passed, 2 ignored	284 passed, 2 ignored

Of the 41 SGLang tests on main, 28 keep their path at the head, and 10 moved into the new child modules. The other 3 are the tests that 857f465a19 removed.

In my copy only, I made the Mocker SGLang server end each stream with {"type": "stop", "matched": 128001}. Then I read the stop reason from the real sidecar generate():

stop lists in the request	main	head	head, mutant at engine.rs:363
none	Some(Int(128001))	None	None
hidden [128001]	Some(Int(128001))	None	None
user [576], hidden [128001]	Some(Int(128001))	None	None
user [128001], hidden [128001]	Some(Int(128001))	Some(Int(128001))	None

Without that change, every case ends with Length and no stop reason on both trees. The mutant passes default stop conditions to terminal_from_meta.

I made 22 mutants at the head and ran the 86 SGLang lib tests for each one. 20 mutants fail at least one test:

• The fix reverted to the base line, and one mutant for each row of its table: user_stop_tokens_are_preserved_and_system_stop_tokens_are_hidden fails each time.
• A wrong component or model name in from_discovered: worker_preserves_options_and_uses_discovered_identity_and_role fails.
• from_parsed without validate_args: encoder_routing_is_rejected_before_discovery fails with CannotConnect.
• 10 more mutants each fail a test. Four of them change the 4xx status range, the logprob rank, the stop ids sent to SGLang, and their dedupe. The other six change number parsing, the RPC status mapping and deadline, prefill sampling, and the trace context on both request paths.
• A renamed message key: only native_http::tests::prefill_stream_reports_failures_after_success_headers fails. This is the first P3 note.

2 mutants pass all 86 tests:

• Default stop conditions at engine.rs:363. This is the second P3 note.
• true for the bootstrap argument of the new discovery call. This flag only selects eprintln! or tracing for retry logs. The same mutant also passes all 41 tests on main.

In CI, rust-tests (.) and rust-clippy (.) of Pre Merge passed on the merge of this head with 50889c0e37. The PR workflow on pull-request/15360 did not finish before I posted. main is at 91184445c6 now. Its 2 new commits change no Rust or Cargo file, and the head merges with it without a conflict.

[devin-ai-integration]

✅ Dynamo PR CI passed — run 36813474685 (attempt 2) on 08798943e6

✅ 44 passed · ❌ 0 failed · ⏹️ 0 cancelled · ⏭️ 33 skipped (not needed for this change)

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Each cell counts that group's GitHub Actions jobs by result (✅ passed, ❌ failed, ⏰ timed out, ⏹️ cancelled); ⏭️ = all skipped, ➖ = no such job.

Framework	Build	1-GPU amd64	1-GPU arm64	Multi-GPU amd64	Deploy	Snapshot
vLLM	✅ 3	✅ 1	✅ 1	✅ 1	⏭️	⏭️
SGLang	✅ 3	✅ 1	✅ 1	✅ 1	⏭️	⏭️
TRT-LLM	✅ 3	✅ 1	✅ 1	✅ 1	⏭️	⏭️

Other	Jobs
changed-files	✅ 1
dynamo-runtime	✅ 8
dynamo-sidecar	✅ 1
frontend	✅ 2
frontend (amd64)	✅ 1
frontend (arm64)	✅ 1
planner	✅ 5
sidecar-runtime	✅ 1
sidecar-runtime (1)	✅ 2
sidecar-runtime (2)	✅ 2
triton-runtime	✅ 2

⏭️ 7 other components not run (skipped by change detection or an upstream result)

allure-report, deploy-operator, DGDR Deploy Test, Helm Chart Tests, Operator, Operator Integration, Power Agent

_{Posted automatically by Devin for run 36813474685. Updated on every full-CI run of this PR.}