[Router][Feat] Add /reset_prefix_cache router passthrough - #1106
AyushKashyapII wants to merge 3 commits into
Conversation
Signed-off-by: AyushKashyapII <kashyap11ayush02@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new /reset_prefix_cache endpoint that relays the actual response body from vLLM, alongside unit tests to verify this behavior and prevent regressions on existing endpoints. The review feedback identifies a critical bug where the response content is not populated when a request body is present because the response-parsing logic is only implemented in the else branch of the request handler. To address this, the reviewer suggests refactoring the HTTP POST logic to unify the branches and parameterizing the unit tests to cover both empty and non-empty request body scenarios.
I am having trouble creating individual review comments. Click here to see my feedback.
src/vllm_router/services/request_service/request.py (1131-1132)
Bug: response_content is not populated when a request body is present
In route_sleep_wakeup_request, the request handling is split into two branches depending on whether request_body is present:
if request_body:(lines 1118–1124)else:(lines 1125–1132)
Currently, the logic to fetch the response JSON for /reset_prefix_cache is only implemented in the else: branch (lines 1131–1132). If a client sends a /reset_prefix_cache request with any body (even an empty JSON object {}), the if request_body: branch is executed, and response_content remains None. Consequently, the router will discard the actual response from vLLM and return the default {"status": "success"}.
Suggested Refactoring
To fix this and avoid duplicating the response parsing logic, you can refactor the HTTP POST request to use a single async with client.post block by dynamically building the request arguments:
post_kwargs = {"headers": headers, "params": upstream_params}
if request_body:
post_kwargs["json"] = json.loads(request_body)
async with client.post(url, **post_kwargs) as response:
response.raise_for_status()
response_status = response.status
if endpoint == "/reset_prefix_cache":
response_content = await response.json()src/tests/test_reset_prefix_cache.py (65-80)
Improvement: Parameterize test to cover both request body scenarios
The current test only verifies the scenario where the request has no body (body=b""). Because of this, it only exercises the else: branch in route_sleep_wakeup_request and misses the bug where response_content is not populated when a request body is present.
Parameterizing the test with both b"" and b"{}" ensures both code paths are fully covered and correctly relay the response.
@pytest.mark.asyncio
@pytest.mark.parametrize("request_body", [b"", b"{}"])
async def test_reset_prefix_cache_relays_real_response_body(request_body):
"""/reset_prefix_cache's real vLLM response body (`{"success": bool}`)
must be relayed to the caller, since `success: false` is a legitimate,
meaningful outcome (blocks still held) -- not swallowed into the old
canned `{"status": "success"}`.
"""
endpoint_info = EndpointInfo("X", "http://engine-x:8000", "pod-x")
service_discovery = MagicMock()
service_discovery.get_endpoint_info.return_value = [endpoint_info]
mock_session_cm, mock_client, mock_response = _mock_aiohttp_session(
response_status=200, response_json={"success": False}
)
request = _build_request({"id": "X", "reset_external": "true"}, body=request_body)
…an observe at runtime Signed-off-by: AyushKashyapII <kashyap11ayush02@gmail.com>
Bug
The router proxies /sleep, /wake_up, and /is_sleeping (all vLLM dev-mode endpoints) via route_sleep_wakeup_request, but not /reset_prefix_cache — calling it through the router 404s, forcing users to bypass the router entirely (e.g. port-forward directly to the pod).
Fix
Out of scope (follow-up)
/collective_rpc is not included — it accepts an arbitrary method name/args, a meaningfully larger surface than a fixed action like /sleep or /reset_prefix_cache, and needs its own discussion about access scoping before proxying it. Tracked separately.
Tests
Added src/tests/test_reset_prefix_cache.py (new file, since route_sleep_wakeup_request had no direct unit test coverage before this PR):
All 10 tests across test_reset_prefix_cache.py, test_k8s_service_name_sleep_mode.py, test_request_auth_headers.py, and test_request_validation.py pass.
Fixes #1105