[jperezdealgaba]

Summary

The P2P secondary tier manager assumes that the nested role sub-dicts
(remote_prefiller, remote_decoder, remote_kv_source) inside
kv_transfer_params are always dicts. When a client sends a non-dict value
(e.g. a string), the subsequent .get() call on the result raises
AttributeError, which escapes request handling and terminates EngineCore.

This adds isinstance(val, dict) checks in the three role accessor
functions so non-dict values are treated as absent (return None),
making the malformed input a request-local no-op instead of a fatal crash.

Changes

• vllm/v1/kv_offload/tiering/p2p/manager.py: Added type validation in
  _remote_prefiller_params, _remote_decoder_params, and
  _remote_kv_source_params — return None when the extracted value is
  not a dict.
• tests/v1/kv_offload/tiering/p2p/test_manager.py: Added
  TestNonDictRoleParamsRejected with regression and completeness tests
  for string, int, list, and bool values across all three role keys, plus
  full manager-flow tests for on_new_request, lookup, and
  submit_store.

Codepath coverage

All entry-point-to-sink paths verified as fixed:

• /v1/completions (and all protected endpoints) → EngineCore → Scheduler
  → P2P on_new_request → _annotate_req_context → _parse_source /
  _parse_dest → accessor functions (now guarded)
• /invocations (unauthenticated SageMaker) → same chain → same guard

Tests

• test_string_remote_prefiller_treated_as_absent (×4 bad types)
• test_string_remote_decoder_treated_as_absent (×4 bad types)
• test_string_remote_kv_source_treated_as_absent (×4 bad types)
• test_manager_survives_malformed_prefiller_role
• test_manager_survives_malformed_decoder_role
• test_manager_survives_malformed_kv_source_role

Run: pytest tests/v1/kv_offload/tiering/p2p/test_manager.py -v

Lint: ruff check and ruff format --check pass on both files.

AI assistance

This PR was developed with AI assistance.

Made with Cursor

Rebase validation — 2026-10-27

Rebased onto upstream main, preserving the original fix and current surrounding behavior. The duplicate audit found no complete merged or open replacement for this PR’s remaining control.

Updated existing test expectations from BLOCK_LEVEL to the renamed CHUNK_LEVEL policy.

Validation on macOS CPU: 116 passed. All applicable changed-file pre-commit checks and git diff --check passed.

VLLM_TARGET_DEVICE=cpu VLLM_PLUGINS='' PYTHONPATH=. HF_HUB_OFFLINE=1 .venv/bin/python -m pytest tests/v1/kv_offload/tiering/p2p/test_manager.py -q --tb=short -p no:cacheprovider

Native GPU/RDMA execution and model evaluations were not run during this rebase; required CI and human review remain pending. AI assistance (OpenAI Codex) was used for the conflict resolution and validation.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 47d86358-b133-44b0-b273-250417035dc0

📥 Commits

Reviewing files that changed from the base of the PR and between 31a8a26 and ab5853e.

📒 Files selected for processing (2)

• tests/v1/kv_offload/tiering/p2p/test_manager.py
• vllm/v1/kv_offload/tiering/p2p/manager.py

🚧 Files skipped from review as they are similar to previous changes (2)

• vllm/v1/kv_offload/tiering/p2p/manager.py
• tests/v1/kv_offload/tiering/p2p/test_manager.py

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

Walkthrough

The P2P tiering manager validates remote role parameters as dictionaries. Tests cover malformed values during parsing, lookup, and store flows.

Changes

P2P role parameter validation

Layer / File(s)	Summary
Validate remote role parameter types vllm/v1/kv_offload/tiering/p2p/manager.py	The remote prefiller, decoder, and KV source helpers accept only dictionary parameters. A malformed remote_decoder produces destination state without a request ID.
Test malformed role parameter flows tests/v1/kv_offload/tiering/p2p/test_manager.py	Parametrized tests verify absent routing state, BLOCK_LEVEL policies, MISS lookups, and failed stores for malformed role parameters.

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

Merge Risk: Minimal · up to 00700

This change makes malformed P2P remote-role parameters safe to handle and adds coverage for affected request, lookup, and store flows. No current merge-blocking risk is identified.

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title names the main change: the role sub-dict accessors in the P2P tier manager now validate that values are dicts. It uses a [Security] prefix, which is a tag rather than a summary. The title is…
Description check		The description explains the problem (non-dict role sub-dicts raise AttributeError and terminate EngineCore), the isinstance guards added to the three accessors, the regression and manager-flow tests,…

Full details: Title check

Explanation

The title names the main change: the role sub-dict accessors in the P2P tier manager now validate that values are dicts. It uses a [Security] prefix, which is a tag rather than a summary. The title is short, specific, and matches the changeset.

Full details: Description check

Explanation

The description explains the problem (non-dict role sub-dicts raise AttributeError and terminate EngineCore), the isinstance guards added to the three accessors, the regression and manager-flow tests, and the validation run. It is closely related to the changeset.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[russellb]

Thanks for hardening this path — the AttributeError crash on a non-dict role value is real and the guard is the right shape for the prefiller/kv-source (producer→consumer) roles.

My one substantive concern is the decoder role: treating a non-dict remote_decoder as absent silently changes a remote-decode request into a successful local store, which contradicts the documented fail-safe invariant (and the PR's own test now codifies that regression). Details inline. There's also a stale comment and an incompletely-guarded top-level container noted inline.

One finding can't be anchored inline because it's outside the diff: the comment in submit_store (~lines 434-436) still says "An empty/malformed dict is still a remote-decode signal and must fail the missing-id check below", and the P2PDestInfo docstring (~lines 117-123) states the same invariant. After this change a non-dict remote_decoder no longer produces a P2PDestInfo, so it takes the dest is None → success=True local path instead of failing. Please either preserve the fail-safe for the decoder role or update those docs/comments to match the new behavior.

(Disclosure: this review was AI-assisted.)

[russellb]

Re-reviewed at ce1fdc9 — all four findings from the previous pass are addressed:

• Decoder fail-safe (the substantive one): a present-but-non-dict remote_decoder now maps to {} and fails closed via the missing-id check, instead of silently storing locally with success=True. Behavior is now consistent between {} and non-dict malformed values, and the docstring reflects it.
• Top-level container: all three accessors now isinstance-check kv_params itself, closing the AttributeError path for a truthy non-dict kv_transfer_params.
• Test: now asserts success=False for the malformed-decoder store, correctly codifying fail-closed.
• Imports: moved to the module top-level block.

I ran the new tests locally: pytest tests/v1/kv_offload/tiering/p2p/test_manager.py::TestNonDictRoleParamsRejected → 19 passed.

Nit (non-blocking): the CodeRabbit pre-merge check flags docstring coverage at 58% vs the 80% threshold for touched functions — worth a quick look but not a blocker for this security fix.

LGTM. (This re-review was AI-assisted.)

[Manny7717]

Independent APPROVE — fully locally verified on head ce1fdc90 (CPU env, repo's own venv); no rubber stamp.

Vulnerability confirmed real: kv_transfer_params arrives from the client as free-form JSON (dict[str, Any] in the token-in-token-out protocol — no recursive shape validation). On base, a truthy non-dict value for any role, or a non-dict kv_transfer_params itself, reached _peer_id_from_params/role.get and crashed EngineCore with AttributeError: 'str' object has no attribute 'get' (reproduced: kv_params={"remote_prefiller": "not-a-dict"} → AttributeError at manager.py:100 on base; clean local-degradation on head). That is a remote availability hole on the serving path — the hardening is justified.

Fix semantics verified coherent across the whole surface (executed 11-case matrix on head):

• consumer roles (remote_prefiller, remote_kv_source) with non-dict values → treated as absent → request handled locally (OffloadPolicy.BLOCK_LEVEL, lookup → MISS), never a crash;
• producer role (remote_decoder) with a non-dict value → coerced to {} (present-but-empty) so submit_store fails closed (JobResult(success=False)) instead of silently storing locally a block a remote decoder is expecting — the correct asymmetry, since storing locally would be silent data loss for the decode side;
• empty-dict role values and empty kv_params behave exactly as on base (no behavior change); only the previously-crashing non-dict inputs change.
• isinstance(kv_params, dict) guards also cover the outer-params-non-dict case (string/int/None) that base mishandled for truthy values.

Regression-proven: all 19 new parametrized tests FAIL on base manager.py (crash) and PASS on head; full tests/v1/kv_offload/tiering/p2p/test_manager.py = 91/91 passed on head (72 pre-existing + 19 new). No call-site drift: _parse_source/_parse_dest/submit_store consumers all handle the new None/{} outputs correctly per the tests.

Nit (non-blocking): the fail-closed {} coercion for remote_decoder relies on the docstring to explain the asymmetry vs. the consumer roles returning None — a one-line comment at each call site would make the intent self-documenting, but the class docstrings and tests already carry it.

Matches maintainer russellb's approved direction; no remaining issues found.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[liranschour]

Thanks for this — the underlying bug is real and worth fixing. I confirmed against current main that {"remote_prefiller": "x"} reaches _peer_id_from_params("x") and raises AttributeError: 'str' object has no attribute 'get' out of on_new_request on the scheduler thread. Since kv_transfer_params is typed dict[str, Any] | None in the pydantic models, the nested values are genuinely unvalidated client JSON, so the entry point you describe is correct.

Two things need to change before this lands.

1. Three of the six new tests fail on this branch

OffloadPolicy.BLOCK_LEVEL does not exist. It was renamed to CHUNK_LEVEL in #52615 ([Refactor][kv_offload]: rename block→chunk), and the only three references to BLOCK_LEVEL anywhere in the tree are the ones this PR adds. Running the file on this PR's head:

3 failed, 88 passed

TestNonDictRoleParamsRejected::test_manager_survives_malformed_prefiller_role
TestNonDictRoleParamsRejected::test_malformed_decoder_role_fails_store
TestNonDictRoleParamsRejected::test_manager_survives_malformed_kv_source_role

AttributeError: type object 'OffloadPolicy' has no attribute 'BLOCK_LEVEL'

It's a one-word fix — CHUNK_LEVEL is also what on_new_request actually returns for a role-less request (bare RequestOffloadingContext()). With s/BLOCK_LEVEL/CHUNK_LEVEL/ applied, all 19 tests in the new class pass. ruff can't catch this because it's a runtime attribute lookup, and the unit-test job didn't run on this PR, so it slipped through green CI.

2. The guard doesn't close the crash class it targets

kv_request_id is equally Any, and it is used as a set/dict key. With this PR applied, a perfectly dict-shaped role still terminates EngineCore:

kv_transfer_params	sink	result
{"remote_prefiller": {"remote_host": "h", "remote_port": 1, "kv_request_id": ["x"]}}	lookup() → _failed_req_ids	TypeError: unhashable type: 'list'
same, kv_request_id: {"a": 1}	lookup() → _failed_req_ids	TypeError: unhashable type: 'dict'
{"remote_decoder": {"kv_request_id": ["x"]}}	submit_store() → _reaped_stores	TypeError: unhashable type: 'list'
same, kv_request_id: {"a": 1}	submit_store() → _reaped_stores	TypeError: unhashable type: 'dict'

Same endpoint, same fatal outcome, both reachable from plain JSON — so the PR's goal of making malformed input "a request-local no-op instead of a fatal crash" isn't met yet.

A per-key isinstance in the three accessors treats the symptom. The funnel is _parse_source / _parse_dest, which already validate shape:

if peer_id is None or not kv_request_id:
    return None

Tightening that to require isinstance(kv_request_id, str) (and the same for remote_host / remote_port in _peer_id_from_params) covers both classes in one place, and lets the three accessors stay as they are. Could you move the validation there instead?

Related, lower severity: remote_host / remote_port are unvalidated too, so peer_id becomes arbitrary attacker-controlled text — e.g. "{'a': 1}:8000" — which is then used as a session-map key and a ZMQ endpoint. Not a parse-time crash, but it belongs in the same validation pass.

3. Behavior change on {"remote_decoder": null}

Commit 2 swaps .get() for an in check plus an {} fallback, so an explicit JSON null now takes the fail-closed path: JobResult(success=False), where before it was success=True. A JSON null most naturally reads as "absent", and an external router templating "remote_decoder": null would start failing stores. Suggest an explicit if val is None: return None before the {} fallback.

What's right

The fail-closed asymmetry is the correct call and I'd keep it: consumer roles (remote_prefiller / remote_kv_source) degrade to local compute, while a present-but-malformed remote_decoder fails the store. That matches the existing P2PDestInfo docstring and the behavior already in tree for {"remote_decoder": {}}, so commit 2 makes the non-dict case consistent with the empty-dict case rather than introducing a new rule. The top-level isinstance(kv_params, dict) guard is unreachable via the HTTP endpoints (pydantic validates the container) but it's harmless defense-in-depth and doesn't regress the {} case.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jperezdealgaba.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jperezdealgaba.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[liranschour]

Re-checked at cf5aa86: #1 fixed, thanks.
#2 still reproduces (kv_request_id: ["x"] → TypeError: unhashable type: 'list' from both lookup() and submit_store()), and #3 (remote_decoder: null → success=False) is unchanged.

[mergify]

Hi @jperezdealgaba, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.