fix(dflash2): prefix-cache draft KV를 fail closed한다 - #183
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_07dbfc21-73da-4075-aed4-6ee7b0ae03bd) |
There was a problem hiding this comment.
🟡 Changes recommended
The new profile comment for the PREFIX_CACHE=1 rollback is misleading/internally inconsistent and should be corrected to avoid operator error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Disables vLLM automatic prefix caching by default for GLM53 DFlash2 serving to prevent the drafter from reading potentially unwritten context KV on cache-hit tokens (fail-closed behavior), while allowing an explicit rollback via PREFIX_CACHE=1.
Changes:
- Set
PREFIX_CACHE=0as the default in the GLM53 profile and in profile-less launcher runs. - Add early validation/wiring to pass
--no-enable-prefix-caching/--enable-prefix-cachingintovllm serve, including caller passthrough andDRY_RUNvisibility. - Add a regression test that pins defaults, validation, passthrough wiring, and serve-arg propagation.
File summaries
| File | Description |
|---|---|
| tests/test_logic.py | Adds a regression test that enforces fail-closed prefix-cache defaults and launcher wiring. |
| profiles/glm53.env | Sets PREFIX_CACHE=0 default for DFlash2 and documents the rationale/tradeoff. |
| launchers/start-glm53-nvfp4-tp4.sh | Introduces PREFIX_CACHE knob, validates 0/1, wires corresponding vLLM flags into SERVE_ARGS, and exposes it in passthrough + DRY_RUN. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # KV slots (upstream draft PR #47926). Prefer acceptance correctness over TTFT | ||
| # reuse until that multi-file runtime fix lands. Set 1 only as an explicit | ||
| # throughput-first rollback for a workload known not to reuse prefixes. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eb39d878a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| PREFIX_CACHE="${PREFIX_CACHE:-0}" | ||
| case "$PREFIX_CACHE" in | ||
| 0) PREFIX_CACHE_FLAG="--no-enable-prefix-caching" ;; |
There was a problem hiding this comment.
When the launcher is invoked with SPEC=0, its documented target-only mode cannot encounter the missing draft-KV problem, yet this unconditional default still emits --no-enable-prefix-caching; the same happens for the DFLASH2=0 MTP path. Before this change those modes inherited vLLM's enabled default, so shared-prefix target-only/MTP workloads now lose prefix reuse and its TTFT benefit unless callers discover and set the new override. Default the flag based on whether the DFlash2 path is actually active while still honoring an explicit PREFIX_CACHE value.
Useful? React with 👍 / 👎.
문제
glm53:v13-b12x의CacheConfig.enable_prefix_caching기본값은True이고 현재 런처는 별도 플래그를 전달하지 않습니다. DFlash2 context KV는 target forward에서 나온 hidden state로 채우는데, prefix-cache hit 토큰은 target forward를 건너뜁니다. 따라서 cache-hit 영역의 draft KV가 보장되지 않은 채 draft attention이 읽을 수 있습니다.이는 upstream draft PR vllm#47926이 다루는 문제로, shared-prefix workload에서 position-0-only acceptance까지 떨어질 수 있습니다. 해당 PR은 아직 draft이고 4개 runtime 파일을 수정하며 cached prefix를 draft 문맥에서 제거하는 큰 변경이라 현재 이미지에 그대로 포팅하지 않습니다.
변경
PREFIX_CACHE=0으로 설정0은--no-enable-prefix-caching,1은 명시적 throughput-first rollback인--enable-prefix-caching으로 전달이 변경은 cache-hit 토큰도 target forward를 통과시켜 DFlash context KV가 실제로 채워지도록 합니다. target 출력 및 rejection 규칙은 바꾸지 않습니다. 대가로 prefix-cache TTFT 이득을 포기합니다.
검증
bash -n launchers/start-glm53-nvfp4-tp4.shDRY_RUN=1 ... start-glm53-nvfp4-tp4.sh→PREFIX_CACHE 0ABORT ..., exit 2python3 tests/test_logic.py→ all OK (2051 checks)python3 -m compileall -q tests/test_logic.pygit diff --check요청대로 모델 실행 실험과 GPU 벤치마크는 수행하지 않았습니다. 따라서 개선 범위는 prefix를 재사용하는 요청이며 실제 수용률 증분은 측정하지 않았습니다.
Note
Medium Risk
Changes default serving behavior for all GLM53 DFlash2 boots (prefix caching off), which affects TTFT on shared-prefix workloads but avoids speculative acceptance collapse; rollback is explicit via PREFIX_CACHE=1.
Overview
DFlash2 can read uninitialized draft KV when vLLM’s automatic prefix cache restores tokens without running the target forward (upstream vLLM #47926). This PR fails closed by default:
PREFIX_CACHE=0in the launcher andprofiles/glm53.env, wired to--no-enable-prefix-cachingonvllm serve.Callers can set
PREFIX_CACHE=1for an explicit throughput-first rollback (--enable-prefix-caching); invalid values abort before serve args are built. The knob is included in profile override passthrough andDRY_RUNoutput. A regression test intests/test_logic.pypins the default, flags, validation order, and serve wiring.Trade-off: shared-prefix TTFT reuse is sacrificed until the upstream multi-file draft-KV repair lands; target outputs and rejection logic are unchanged.
Reviewed by Cursor Bugbot for commit 3eb39d8. Bugbot is set up for automated code reviews on this repo. Configure here.