feat(frontend): add DYN_ENABLE_NVEXT and DYN_ENABLE_FRONTEND_ADMIN_API master switches - #10556
Conversation
…I master switches Addresses PSIRT findings 6067559/6067562 (unauthenticated busy_threshold) and the 6067564 by-design routing-header surface by giving operators two binary master switches that fully close off the nvext extension protocol and the frontend admin API. DYN_ENABLE_NVEXT (default true) When false at frontend startup, the chat/completions/responses handlers drop request.nvext at the boundary and ignore the routing-override headers (x-worker-instance-id, x-prefill-instance-id, x-dp-rank, x-prefill-dp-rank). Response-side extra_fields opt-in is silently ignored. This shuts off worker-targeting via header AND body in one switch. Compatibility: disables EPP/GAIE serving, Prime-RL-style training that uses nvext.cache_salt for KV isolation, multi-tenant agent platforms that forward agent_hints/agent_context, and clients that opt into response disclosure via nvext.extra_fields. DYN_ENABLE_FRONTEND_ADMIN_API (default true) When false, the GET /busy_threshold and POST /busy_threshold routes are not registered. Inference, metrics, models, health, and liveness routes are unaffected. (The historical POST /clear_kv_blocks route was already unwired in #1629; not re-exposed.) Design notes - Both flags are read once at HttpService::run() via env_is_falsey on DYN_ENABLE_NVEXT / DYN_ENABLE_FRONTEND_ADMIN_API, ANDed with the HttpServiceConfig builder field. Env-falsey only — unset env keeps the default-true behavior intact. - State carries the nvext_enabled flag as a plain bool field set at construction; handlers read it via state.nvext_enabled() — single struct-field load on the hot path, no atomic, no per-request env lookup, no OnceLock. Matches the existing pattern used for StateFlags + the enable_rl_router gate around line 698. - The admin-api gate is purely at the route-registration site, so disabled mode means the route literally doesn't exist (404) rather than running a handler that 503s. Tested with cargo check and cargo clippy --no-deps on dynamo-llm. Signed-off-by: nnshah1 <neelays@nvidia.com>
WalkthroughThis PR adds two master switches to the HTTP service layer. The ChangesFeature flags for nvext extension and admin API routes
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/runtime/src/config/environment_names.rs (1)
715-726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd new environment variable constants to the duplicate-check test.
The test array is missing the newly added constants
llm::DYN_ENABLE_NVEXTandllm::DYN_ENABLE_FRONTEND_ADMIN_API. Without them, the test cannot detect duplicate names for these variables.🔧 Suggested fix
llm::DYN_ENABLE_ANTHROPIC_API, + llm::DYN_ENABLE_NVEXT, + llm::DYN_ENABLE_FRONTEND_ADMIN_API, llm::DYN_STRIP_ANTHROPIC_PREAMBLE,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/runtime/src/config/environment_names.rs` around lines 715 - 726, The duplicate-check test's array of environment constants is missing the newly added llm::DYN_ENABLE_NVEXT and llm::DYN_ENABLE_FRONTEND_ADMIN_API symbols; update the test array in environment_names.rs (the list used for duplicate detection) to include llm::DYN_ENABLE_NVEXT and llm::DYN_ENABLE_FRONTEND_ADMIN_API so the test will detect duplicates for those new env vars.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/http/service/service_v2.rs`:
- Line 316: The doc comment in service_v2.rs currently references a removed
route (`POST /clear_kv_blocks`) alongside `busy_threshold`; remove the `POST
/clear_kv_blocks` mention from that doc comment so it only documents the
existing routes (e.g., leave `POST/GET /busy_threshold` and related text
intact). Locate the comment near the busy_threshold documentation in the
service_v2 module and update the comment text accordingly.
In `@lib/runtime/src/config/environment_names.rs`:
- Line 311: Remove the stale documentation bullet that references the removed
admin route `POST /clear_kv_blocks` in the doc comment inside
environment_names.rs (the comment block containing " - `POST /clear_kv_blocks`
(clear all KV cache blocks on workers)"). Edit that comment to delete the line
or update the list so it no longer lists `POST /clear_kv_blocks`, leaving other
documented routes unchanged.
---
Outside diff comments:
In `@lib/runtime/src/config/environment_names.rs`:
- Around line 715-726: The duplicate-check test's array of environment constants
is missing the newly added llm::DYN_ENABLE_NVEXT and
llm::DYN_ENABLE_FRONTEND_ADMIN_API symbols; update the test array in
environment_names.rs (the list used for duplicate detection) to include
llm::DYN_ENABLE_NVEXT and llm::DYN_ENABLE_FRONTEND_ADMIN_API so the test will
detect duplicates for those new env vars.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e0ea28f-48e9-4ae9-b9c1-f6759d631f26
📒 Files selected for processing (3)
lib/llm/src/http/service/openai.rslib/llm/src/http/service/service_v2.rslib/runtime/src/config/environment_names.rs
- Drop POST /clear_kv_blocks from env-var docs (it's not a registered route) and delete the orphaned lib/llm/src/http/service/clear_kv_blocks.rs source file (dead since PR #1629 removed the route registration; no mod declaration, so it wasn't even compiled). - Rename State::new_with_flags -> State::new_with_nvext_enabled (only one flag for now). - Collapse the 6-line copy-pasted comment block at the 3 openai.rs handler call sites to a one-liner; rewrite the if/else as an expression assignment. - Trim const docs in environment_names.rs to behavior + default; the EPP/GAIE/Prime-RL compatibility paragraph is design rationale and lives in the PR description, not the public env-var reference. - Trim the env-mirror comment, the field/accessor doc duplication, and switch the admin-API-disabled log to structured fields. - Add two tests: enable_nvext propagates through the builder onto State.nvext_enabled (default + on + off), and enable_admin_api=false causes GET /busy_threshold to 404 while /live still serves 200. Signed-off-by: nnshah1 <neelays@nvidia.com>
Graham-review followup: the one-liner 'nvext master switch — see State::nvext_enabled' was copy-pasted at three handler call sites. state.nvext_enabled() is already self-documenting and the accessor's own /// doc points at the env var definition; the call-site comment adds nothing. Signed-off-by: nnshah1 <neelays@nvidia.com>
…, add env-var test, consolidate docs
Addresses biswapanda's review feedback on the master-switch PR:
1. ("keep them consistent") Rename DYN_ENABLE_NVEXT to
DYN_ENABLE_FRONTEND_NVEXT to match the DYN_ENABLE_FRONTEND_ADMIN_API
sibling. Only env-var name changes; internal API (enable_nvext field,
nvext_enabled() accessor) stays.
2. ("embeddings has no nvext_enabled() gate") KISS — instead of gating
the embedding handler, remove nvext from the embedding protocol
entirely. The embedding-local NvExt only carried an annotations
field (SSE event triggers, useless for typically-non-streaming
embeddings) and never carried any routing fields. Delete
lib/llm/src/protocols/openai/embeddings/nvext.rs and drop the
nvext: Option<NvExt> field from NvCreateEmbeddingRequest plus the
AnnotationsProvider impl. preprocessor.rs's
preprocess_embedding_request no longer reads request annotations.
3. ("add a test case for unset/true/false env-var with temp_env::with_vars")
Add test_dyn_enable_frontend_nvext_env_var_mirror covering unset,
truthy, falsey, and the builder-overrides-env case.
4. ("we'd update user-facing docs") Add a Frontend feature switches
section under docs/components/frontend/configuration.md with one
consolidated table covering DYN_ENABLE_FRONTEND_NVEXT and
DYN_ENABLE_FRONTEND_ADMIN_API: default values, behavior when false,
and compatibility notes. The existing /busy_threshold rows in the
Infrastructure routes table cross-reference the new section.
cargo check / cargo fmt --check / cargo clippy --tests --no-deps all
clean. 4 unit tests pass:
- test_liveness_endpoint_reflects_cancellation
- test_enable_nvext_propagates_through_builder_to_state
- test_dyn_enable_frontend_nvext_env_var_mirror (new)
- test_admin_api_disabled_404s_busy_threshold
Signed-off-by: nnshah1 <neelays@nvidia.com>
Reversal of the deletion in fd3a977. Per offline discussion with @nnshah1: even though the embedding-local NvExt currently only exposes 'annotations', deleting it removes a contract clients may be relying on. Keep the surface, gate its consumption — same pattern as the three other handlers — so DYN_ENABLE_FRONTEND_NVEXT=false closes off the *behavior* without breaking the request shape. Embedding handler now does the same one-line clear at handler entry: if !state.nvext_enabled() { request.nvext = None; } Routing-override headers don't apply to embeddings, so no apply_header_routing_overrides call to gate. Restored: - lib/llm/src/protocols/openai/embeddings/nvext.rs - lib/llm/src/protocols/openai/embeddings.rs (nvext field + NvExtProvider + AnnotationsProvider impls) - lib/llm/src/preprocessor.rs (request.has_annotation() + request.annotations() calls) Doc table updated to list /v1/embeddings alongside the other three gated handlers. Signed-off-by: nnshah1 <neelays@nvidia.com>
Add an end-to-end integration test asserting that when the frontend nvext extension is disabled, a request asking for response-side extra_fields (plus a routing header and body routing field) produces no nvext field in the response — proving the request- and response-side strips are wired together, not just unit-tested in isolation. Also warn once at the handler when a request carries nvext data (body or routing headers) while the extension is disabled, so the otherwise silent strip is observable in logs. The check lives only in the disabled branch, so the default request path is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: nnshah1 <neelays@nvidia.com>
Move warn_nvext_disabled above the handler_completions doc block (it had been inserted mid-doc-comment, splitting it) and trim verbose docstrings on the helper and the new test per review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: nnshah1 <neelays@nvidia.com>
Replace the verbose master-switch preamble with a short deployment-focused note: extensions are on by default; disable the ones a deployment doesn't need to prevent accidental abuse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: nnshah1 <neelays@nvidia.com>
…I master switches (ai-dynamo#10556) Signed-off-by: nnshah1 <neelays@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-dynamo#6291) Operators need a runtime control to flush the reused KV prefix cache across every worker. The frontend had no such route, so the control returned HTTP 404. The earlier version of this PR added the route to clear_kv_blocks.rs. That module was removed upstream in ai-dynamo#10556, which reworked the frontend admin API behind the DYN_ENABLE_FRONTEND_ADMIN_API / DYN_DISABLE_FRONTEND_ADMIN_API master switch. This reimplements the route against the new admin API: - New module lib/llm/src/http/service/reset_prefix_cache.rs registers POST /reset_prefix_cache, following the busy_threshold admin-route pattern. The handler discovers every worker group exposing the clear_kv_blocks endpoint and fans the control out to each instance via PushRouter, reporting per-instance success/failure. - The route is registered only when the admin API is enabled, reusing the DistributedRuntime already plumbed through HttpServiceConfig.runtime. - Adds a #[tokio::test] in lib/llm/tests/http-service.rs asserting the route is mounted (returns 200 with a clear message rather than 404). Fixes ai-dynamo#6291 Signed-off-by: Asad Shahid <asad.shahid@berkeley.edu> Co-Authored-By: Claude <noreply@anthropic.com>
…-dynamo#6291) Operators need a runtime control to flush the reused KV prefix cache across every worker. The frontend had no such route, so the control returned HTTP 404. The earlier version of this PR added the route to clear_kv_blocks.rs. That module was removed upstream in ai-dynamo#10556, which reworked the frontend admin API behind the DYN_ENABLE_FRONTEND_ADMIN_API / DYN_DISABLE_FRONTEND_ADMIN_API master switch. This reimplements the route against the new admin API: - New module lib/llm/src/http/service/reset_prefix_cache.rs registers POST /reset_prefix_cache, following the busy_threshold admin-route pattern. The handler discovers every worker group exposing the clear_kv_blocks endpoint and fans the control out to each instance via PushRouter, reporting per-instance success/failure. - The route is registered only when the admin API is enabled, reusing the DistributedRuntime already plumbed through HttpServiceConfig.runtime. - Adds a #[tokio::test] in lib/llm/tests/http-service.rs asserting the route is mounted (returns 200 with a clear message rather than 404). Fixes ai-dynamo#6291 Signed-off-by: Asad Shahid <asad.shahid@berkeley.edu>
Summary
Adds two binary master switches that let operators close off non-OpenAI-spec surfaces on the frontend HTTP service without affecting inference, metrics, models, or health/liveness probes. Both default to true (no behavior change unless the operator explicitly opts out).
DYN_ENABLE_NVEXTtruerequest.nvextat the boundary; routing-override headers (x-worker-instance-id,x-prefill-instance-id,x-dp-rank,x-data-parallel-rank,x-prefill-dp-rank) are ignored; response-sideextra_fieldsopt-in returns the default empty selectionDYN_ENABLE_FRONTEND_ADMIN_APItrueGET /busy_thresholdandPOST /busy_thresholdroutes are not registered (404 instead of 503)Each switch is also exposed as a
HttpServiceConfigbuilder field (enable_nvext,enable_admin_api) so Python frontend code can flip it from CLI without an env var.Motivation
PSIRT findings 6067559 / 6067561 / 6067562 / 6067563 / 6067566 flagged the frontend admin / metadata surfaces as unauthenticated. 6067564 documented the EPP↔frontend GAIE routing-header protocol (
x-worker-instance-idetc.) as a by-design but operator-disable-able surface. The proposed deployment-guide remediation (DYN-2647) needed an in-process kill-switch so a default-secure deployment can lock these off without source patches.DYN_ENABLE_NVEXTshuts off worker-targeting via header and body in one switch (header parsing + nvext field interpretation both gated by the same flag).DYN_ENABLE_FRONTEND_ADMIN_APIshuts off the busy_threshold admin surface.Compatibility
Defaults preserve current behavior end-to-end. Operators who set
DYN_ENABLE_NVEXT=falseshould be aware:nvext.cache_saltfor KV cache isolation across checkpoint stepsnvext.agent_hints/nvext.agent_contextextra_fieldsresponse opt-in —nvext.extra_fieldsis silently ignoredDocumented in the const docstring on
DYN_ENABLE_NVEXTinenvironment_names.rs.Design notes
HttpService::run()(singleenv_is_falseycall), ANDed with the builder field, and stored as a plainboolonState. Handlers consume viastate.nvext_enabled()— single struct-field load + predicted-taken branch. No atomic, noOnceLock, no per-request env lookup. Matches the existing precedent atservice_v2.rs:698(enable_rl_router).enable_nvext=truefrom the builder. So a typo likeDYN_ENABLE_NVEXT=turefalls through totruerather than silently disabling a default-on feature.State::newAPI preserved — addedState::new_with_flags(...)for the wiring; the originalState::new(...)defaultsnvext_enabled=true.Test plan
cargo check -p dynamo-llm(clean)cargo clippy -p dynamo-llm --no-deps(clean)cargo fmt --checkDYN_ENABLE_NVEXT=false, send a chat request withx-worker-instance-idheader → verify it's ignoredDYN_ENABLE_FRONTEND_ADMIN_API=false→ verifyGET /busy_threshold404sOut of scope (follow-ups)
components/src/dynamo/frontend/frontend_args.py(--enable-nvext/--enable-frontend-admin-api)DYN_KV_INDEXER_TEST_ENDPOINTS→DYN_ENABLE_KV_INDEXER_TEST_APIfor naming parity (separate PR — touches a different component)Summary by CodeRabbit
New Features
Chores