fix(sidecar): preserve terminal metadata and add isolated unit tests

[JulienDarve]

Summary

Add isolated common and vLLM sidecar unit tests beside the production code they exercise. Cover request and response conversion, discovery metadata, worker configuration and lifecycle, JSON conversion, LoRA state, and transport policy.

• Use ordinary #[test] and #[tokio::test] modules, with request and response tests in adjacent files and reusable native inputs in vllm/src/test_fixtures.rs.
• Run through the existing Cargo test commands, without lane markers, custom runners, or a separate CPU container.
• Preserve broader local-server tests and shared integration scenarios. Expanded integration coverage is handled separately in test(sidecar): add vLLM CPU integration coverage #15091.
• Correct hidden stop-token reporting and preserve cancelled prefill responses without requiring a cache handoff.
• Document the file layout, test commands, and how to add tests in lib/sidecar/testkit/README.md.

The unit-test support matrix and legacy Python coverage mapping are in the Google Doc’s Unit Test Matrix Actual tab. It records covered behavior, justified exclusions, and remaining coverage gaps.

Validation

At e8e7af8b47:

• cargo test --locked -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-sidecar-testkit --all-targets: 148 tests passed.
• Workspace formatting, targeted Clippy with warnings denied, ownership checks, and commit hooks passed.

No full Dynamo workspace or native-engine/GPU run was performed.

Linear: https://linear.app/nvidia/issue/DIS-2942

Summary by CodeRabbit

• Bug Fixes

  • Corrected terminal response metadata so hidden stop tokens are not reported unless they are also user-visible, and EOS tokens are reported only when user-visible.
  • Aborted prefill responses now include prompt data without processing KV-transfer parameters.
  • Connection pool startup now applies the remaining startup deadline to each connection attempt, including retries.

• Tests

  • Expanded coverage for transport errors, request and response conversion, model configuration, and related validation.

[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: d0e8b679-ed8e-486f-a905-8a355cabf201

📥 Commits

Reviewing files that changed from the base of the PR and between f6f9063 and 6370289.

📒 Files selected for processing (16)

• lib/sidecar/common/Cargo.toml
• lib/sidecar/common/src/error.rs
• lib/sidecar/common/src/lib.rs
• lib/sidecar/common/src/transport.rs
• lib/sidecar/common/src/transport/tests.rs
• lib/sidecar/testkit/README.md
• lib/sidecar/vllm/src/convert.rs
• lib/sidecar/vllm/src/convert/request_tests.rs
• lib/sidecar/vllm/src/convert/response_tests.rs
• lib/sidecar/vllm/src/engine.rs
• lib/sidecar/vllm/src/json.rs
• lib/sidecar/vllm/src/lib.rs
• lib/sidecar/vllm/src/lora.rs
• lib/sidecar/vllm/src/model.rs
• lib/sidecar/vllm/src/test_fixtures.rs
• lib/sidecar/vllm/src/tests.rs

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 common sidecar transport now uses a generic pool-connection path with a shared startup deadline. The vLLM sidecar adjusts terminal response conversion and adds tests and shared fixtures across conversion, engine, model, JSON, and LoRA code.

Changes

Common gRPC transport

Layer / File(s)	Summary
Pool startup and status validation lib/sidecar/common/Cargo.toml, lib/sidecar/common/src/error.rs, lib/sidecar/common/src/lib.rs, lib/sidecar/common/src/transport.rs, lib/sidecar/common/src/transport/tests.rs	Pool connection setup now uses a generic connection closure and a shared startup deadline. Paused-time tests cover retries, concurrent later slots, and deadline errors. Status-mapping tests check error types and text.

vLLM adapter tests and response conversion

Layer / File(s)	Summary
Shared fixtures and test layout lib/sidecar/testkit/README.md, lib/sidecar/vllm/src/lib.rs, lib/sidecar/vllm/src/test_fixtures.rs, lib/sidecar/vllm/src/convert.rs, lib/sidecar/vllm/src/tests.rs	Adds test-only vLLM fixtures and wires test modules to use them. The README describes the current test layout, commands, and integration coverage.
Request conversion coverage lib/sidecar/vllm/src/convert/request_tests.rs, lib/sidecar/vllm/src/tests.rs	Adds request-conversion tests for controls, validation, handoffs, routing, and multimodal features. Related tests are removed from the aggregated test module.
Terminal response conversion lib/sidecar/vllm/src/convert.rs, lib/sidecar/vllm/src/convert/response_tests.rs	Response conversion now filters hidden stop-token reasons and returns aborted prefill responses with prompt data before KV-transfer handling. Tests cover terminal behavior, metadata, logprobs, and malformed responses.
Engine discovery and model configuration lib/sidecar/vllm/src/engine.rs, lib/sidecar/vllm/src/model.rs, lib/sidecar/vllm/src/tests.rs	Engine startup discovers model metadata before worker configuration. Tests cover startup arguments, model compatibility, parallelism, and KV capacity.
JSON numeric conversion boundaries lib/sidecar/vllm/src/json.rs	Tests cover exact integer representation boundaries and errors for non-finite protobuf numbers.
LoRA validation and lock lifecycle lib/sidecar/vllm/src/lora.rs, lib/sidecar/vllm/src/tests.rs	Tests cover LoRA load URI and inventory validation, plus lock retention and reclamation.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 63702

This change mainly adds isolated sidecar unit tests. It also makes small runtime adjustments: stop-token reporting, aborted prefill handling, and a shared pool startup deadline. These adjustments match their tests, and no current risk that blocks merging was identified.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 73.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 131 functions across 14 files. (2 skipped…	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description gives detailed scope, validation results, limitations, and implementation details. However, it omits the required "Related Issues" section from the repository template, even though it …	Add the required "## Related Issues" section and select the linked-issue path, for example "- Closes #15243" or the correct repository issue reference. Remove or retain the no-related-issue option as required by the template.

✅ Passed checks (3 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 summarizes the main implementation fixes and the addition of isolated unit tests.

Full details: Docstring Coverage

Explanation

Docstring coverage is 73.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 131 functions across 14 files. (2 skipped: 2 unsupported.)

Full details: Description check

Explanation

The description gives detailed scope, validation results, limitations, and implementation details. However, it omits the required "Related Issues" section from the repository template, even though it includes a Linear issue link.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch jdarve/sidecar-local-unit-tests

---

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

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[dmitry-tokarev-nv]

Approve. I read 05a8d2167e and found no blocking defect.

[P3] The title says test, but two changes alter what the vLLM sidecar returns. At lib/sidecar/vllm/src/convert.rs:1407, a stop on a hidden stop token or on EOS no longer sets stop_reason. A token that stop_token_ids lists still sets it. At lib/sidecar/vllm/src/convert.rs:1446, an aborted prefill without kv_transfer_params ends as cancelled, not as a protocol error. Please retitle as fix(sidecar): ..., or move the two fixes to their own PR.

Open item: Pre Merge and the PR workflow did not run at 05a8d2167e, and the Rust CodeQL analysis is still running. The Pre Merge jobs rust-tests (.) and rust-clippy (.) and the vLLM sidecar-runtime E2E job are still to come. This approval rests on the runs below.

What I ran, with controls.

The tests ran on an x86_64 host with the pinned Rust 1.96.1. Each tree had its own target directory.

Tree	Command	Result
Base f6f90637cb	cargo test --locked -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-sidecar-testkit --all-targets	102 passed
Earlier head 6370289a8d	Same command	148 passed
6370289a8d merged with main 4dc45f7823, with the two new main tests moved to request_tests.rs	Same command	150 passed
05a8d2167e	cargo test --locked -p dynamo-sidecar-common -p dynamo-vllm-sidecar --all-targets	141 passed
05a8d2167e	cargo clippy --locked --no-deps -p dynamo-sidecar-common -p dynamo-vllm-sidecar --all-targets -- -D warnings	Passed
05a8d2167e	cargo fmt -p dynamo-sidecar-common -p dynamo-vllm-sidecar -- --check	Passed

At 05a8d2167e, all 34 files under lib/sidecar/common and lib/sidecar/vllm are identical to the merged tree in the table.

The move lost no test. The 102 base tests map to 64 tests at the same path, 33 moved tests and 5 renamed tests. Each renamed test covers the same case, and three of them assert more. The two new main tests from #15081 are in request_tests.rs.

At 05a8d2167e, each mutation below made a test fail. The unmutated tree passed.

Mutation	Test that failed
Old stop_reason mapping at convert.rs:1407	user_stops_are_preserved_and_system_stops_are_hidden
No aborted-prefill return at convert.rs:1446	prefill_handoff_is_required_only_for_successful_native_terminals and prefill_terminal_has_zero_completion_usage
No .min(remaining) at transport.rs:153	later_pool_slots_share_the_original_deadline
No .min(deadline) at transport.rs:229	retry_wait_is_capped_and_last_failure_is_preserved
wrapping_neg instead of saturating_neg at convert.rs:172	canonical_priority_preserves_native_ordering

The transport.rs and engine.rs changes keep the old behavior. In engine.rs, the second parse of --vllm-http-endpoint now runs after discovery. Of 40 test values, clap accepted 29, and the second parse accepted all 29. The base and 6370289a8d gave the same results, and engine.rs is the same at 05a8d2167e.

The two fixes match the frontend. lib/llm/src/backend.rs also leaves stop_reason empty for a system stop token that the user did not request. The prefill router returns a prefill that ends with a reason other than Length as a terminal response.

CodeQL alert 793 is fixed. It flagged a test input in an assertion message. No alert is open on this PR.

[alec-flowers]

A few small test cleanups and missing assertions; details inline.

[dmitry-tokarev-nv]

Approve. I read 912cfa237e and found no blocking defect.

912cfa237e changes only three #[cfg(test)] files, and it answers all 7 test asks from alec-flowers. If I break the code that a new assertion covers, that assertion fails.

My P3 about the title from my last review is resolved. The title now starts with fix(sidecar):.

Open item: Pre Merge and the PR workflow did not run at 912cfa237e, because its commit message has [skip ci]. The Pre Merge jobs rust-tests (.) and rust-clippy (.) and the vLLM sidecar-runtime job are still to come. This approval rests on the runs below.

How each ask is answered, measured by mutation.

For each mutation, I changed the production code and ran the four sidecar crates at 3f68ab1de7 and at 912cfa237e. The unmutated run of each tree passed.

These asks added assertions. Before the commit, no test failed. After it, the new assertion fails.

Ask	Mutation	Failed at 3f68ab1de7	Failed at 912cfa237e
Legacy-only bypass case	Remove the hydration of the legacy skip_reading_prefix_cache in normalize_response_options	None	compatibility_envelope_allows_projected_prefix_cache_bypass
Compare the kwargs bytes	Flip the low bit of each forwarded kwargs byte, so the length stays 64	None	preprocessed_multimodal_features_are_forwarded_to_vllm_grpc
Prompt logprobs on an aborted prefill	Remove attach_prompt_data from the aborted-prefill branch	None	prefill_handoff_is_required_only_for_successful_native_terminals
Two distinct connection failures	Keep the first connection error instead of the last	None	retry_wait_is_capped_and_last_failure_is_preserved
Control	Remove .min(deadline) from the retry sleep	retry_wait_is_capped_and_last_failure_is_preserved	retry_wait_is_capped_and_last_failure_is_preserved

These asks deleted or folded tests. At 3f68ab1de7, each mutation below failed the deleted test. At 912cfa237e, each mutation fails the test in the last column.

Deleted test	Mutation	Failed at 912cfa237e
special_token_policy_is_forwarded	Send no value for an explicit skip_special_tokens=false	absent_and_explicit_zero_controls_preserve_native_sentinels
absent_and_explicit_zero_options_remain_distinct	Treat an explicit temperature 0.0 as absent. Two more mutations do the same for logprobs=0 and prompt_logprobs=0.	absent_and_explicit_zero_controls_preserve_native_sentinels, for each of the 3 mutations
canonical_cache_controls_are_preserved	Drop the dynamo-cache-salt: prefix, ignore a canonical bypass_prefix_cache=true, or read a canonical false as true	canonical_cache_identity_and_bypass_alias_precedence_are_preserved, for each of the 3 mutations

What I ran, with controls.

The tests ran on an x86_64 host with the pinned Rust 1.96.1. Each tree had its own target directory.

Tree	Command	Result
3f68ab1de7	cargo test --locked -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-sglang-sidecar -p dynamo-trtllm-sidecar --all-targets	207 passed
912cfa237e	Same command	204 passed
912cfa237e merged with main e6bdf6f193	Same command	204 passed
912cfa237e	cargo clippy --locked --no-deps on the same crates, with --all-targets -- -D warnings	Passed
912cfa237e	cargo fmt -p dynamo-sidecar-common -p dynamo-vllm-sidecar -- --check	Passed

The only difference between the two test lists is the 3 deleted tests. The commit adds no test, because each new case is a row or a loop in an existing test.

The merge with main at e6bdf6f193 had no conflict. In the merged tree, the regenerated CODEOWNERS matches the committed file.

[dmitry-tokarev-nv]

Approve. I read b974cb9910 and found no defect in its merge of main.

b974cb9910 merges main at c7241c2f15 into 912cfa237e, which I approved before. The merge keeps the code changes of both sides. Outside the conflicts, its only hand edits change the PR's tests to use the new JSON codec API from #14349. Each of the 16 mutations from my last two reviews still makes the same tests fail.

Open item: the sidecar-runtime jobs of the PR workflow did not run on this head yet. They wait for dynamo-sidecar / Build multi-arch cpu. That build did not finish before this review.

How the merge resolves each conflict.

I did the merge again with git merge-tree. Six files had a conflict:

File	Resolution
lib/sidecar/common/Cargo.toml	Keeps the PR's [dev-dependencies] and the [features] table from main.
lib/sidecar/common/src/lib.rs	Keeps the PR's test module and the new pub use transport::{...} line from main.
lib/sidecar/vllm/src/convert.rs	Keeps the PR's early return for an aborted prefill before the struct_to_json_v14 call from main.
lib/sidecar/vllm/src/tests.rs	Keeps both imports. Drops the code that the PR moved to other files. In that code, main changed only the codec calls.
lib/sidecar/vllm/src/json.rs	Deleted, as on main.
lib/sidecar/testkit/README.md	Both sides added this file. The result has the PR's guide first and the text from main about shared integration tests second, with small edits. It drops the note from main about the PR stack. It also drops the PR's short section about the integration boundary, because the text from main covers it.

Three files had no conflict but have hand edits:

• lib/sidecar/common/src/json.rs gets the 2 tests that the PR added to the deleted lib/sidecar/vllm/src/json.rs. The tests use the new signatures, which take a payload name and a peer name.
• lib/sidecar/vllm/src/convert/request_tests.rs and response_tests.rs change 6 codec calls to struct_to_json_v14 and json_to_struct_v14. They use the same labels as convert.rs.

I compared the test names in the four trees. No test from main is missing. .github/codeowners/areas.yaml and CODEOWNERS left the PR's diff, because #14879 added the same lib/sidecar/testkit/ line to main.

What I ran, with controls.

The tests ran on an x86_64 host with the pinned Rust 1.96.1. The command is cargo test --locked -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-sglang-sidecar -p dynamo-trtllm-sidecar --all-targets.

Tree	Result
b974cb9910	234 passed, 2 ignored
main at c7241c2f15	191 passed, 2 ignored
b974cb9910 merged with main at 1a26bf9cb7	234 passed, 2 ignored
912cfa237e	204 passed

The PR adds 48 test names. Five of them replace old names, so the head has 43 more tests than main. The 30 tests more than at 912cfa237e come from main: 29 are in the TensorRT-LLM sidecar, and 1 is a codec test.

Clippy with -D warnings passed on the same crates at b974cb9910. cargo fmt --check passed on the common and vLLM crates.

For the 2 codec tests that moved, I broke the codec at the old head and at the new head:

Mutation	Test that fails at 912cfa237e and at b974cb9910
Accept an integer below -2^53	integer_boundaries_and_negative_inexact_values_are_checked
Let infinity through the finite check	incoming_nonfinite_numbers_fail_inside_nested_payloads

Pre Merge rust-tests (.) ran on the merge of b974cb9910 into c7241c2f15. All 138 common and vLLM tests of this head passed there.

[devin-ai-integration]

✅ Dynamo PR CI passed — run 36789283201 (attempt 2) on b974cb9910

✅ 7 passed · ❌ 0 failed · ⏹️ 0 cancelled · ⏭️ 57 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	Deploy	Snapshot
vLLM	⏭️	⏭️	⏭️
SGLang	⏭️	⏭️	⏭️
TRT-LLM	⏭️	⏭️	⏭️

Other	Jobs
changed-files	✅ 1
dynamo-sidecar	✅ 1
sidecar-runtime	✅ 1
sidecar-runtime (1)	✅ 2
sidecar-runtime (2)	✅ 2

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

allure-report, deploy-operator, DGDR Deploy Test, dynamo-runtime, frontend, Helm Chart Tests, Operator, Operator Integration, planner, Power Agent, triton-runtime

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