fix(KV-MAMBA-ALIGN): prefix_cache_retention_interval is consumed but never produced, so retention is permanently dense
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 423
- Forks
- 53
- Avg merge
- 20h 26m
- Merged PRs (30d)
- 310
Description
Row: KV-MAMBA-ALIGN (.agents/engine-matrix.md:101); KV-HYBRID-COORD (:100) is the alternative owner if the SWA half is split out.
Found by wave PORTQ-5 (#2679) re-deriving PORT-NOW entry [182], upstream 017e9f4448 vllm#52216. Nothing was executed.
The shape of the gap: the consumer is written, the producer is not
The retention machinery is fully implemented and reached, and then fed nullopt unconditionally:
include/vllm/v1/core/kv_cache_coordinator.h:185—std::optional<int> retention_interval = std::nullopt;src/vllm/v1/core/kv_cache_coordinator.cpp:166—// retention_interval stays nullopt (dense caching); env read deferred.The constructor never assigns it.- The three-way semantics upstream defines already exist downstream:
src/vllm/v1/core/single_type_kv_cache_manager.cpp:503-508(!has_value()=> dense,== 0=> no periodic segment,> 0=> interval), with the latest-replay-boundary pin at:523-530and the Mamba arm at:704-715. - Both call sites pass it through (
kv_cache_coordinator.cpp:237,:461). KVCacheConfig(include/vllm/v1/kv_cache_interface.h:512-538) has no such field.
The deferral is deliberate and recorded at include/vllm/v1/core/kv_cache_coordinator.h:68-70, which is the only occurrence of the string prefix_cache_retention_interval in src/+include/+tests/ (positive control through the identical probe form: retention_interval => 26 hits).
What upstream did
Moved the value off envs.VLLM_PREFIX_CACHE_RETENTION_INTERVAL onto a resolved CacheConfig.prefix_cache_retention_interval (ge=0, deprecated-env fallback), threaded it through arg_utils.py into KVCacheConfig, and changed the default from None to 0, with _validate_prefix_cache_retention_interval gaining an early return for 0 so a model with no SWA/Mamba group no longer raises.
Porting this subsumes a pre-pin hole
At the pin, vllm/v1/core/kv_cache_coordinator.py:152 reads the value from the env — which this tree also does not do. So the tree is behind the pin on the env read and behind the target on the config promotion. Our current default (nullopt = dense) equals upstream's pre-commit default, so the flip to 0 is a real, observable behaviour change for SWA and Mamba groups.
Size
~80-140 lines plus tests:
prefix_cache_retention_intervalonKVCacheConfig. Note the tree's own comment atinclude/vllm/v1/kv_cache_interface.h:535: a default member initializer is required to keep positional aggregate init valid.- Resolution with default
0(notnullopt) plus the deprecated-env fallback. _validate_prefix_cache_retention_interval, including the== 0early return and the% scheduler_block_sizecheck.- Coordinator assignment replacing the hard-wired
nulloptatkv_cache_coordinator.cpp:166. - The CLI/ABI knob. There is no
CacheConfigclass here (include/vllm/config/cache.hholds only dtype helpers), so this lands onvllm_model_paramsbesideenable_prefix_caching(include/vllm.h:449) and is an ABI bump.
Not established
Which shipped gate models exercise a SlidingWindow or Mamba group with prefix caching on. The default flip from dense to 0 changes retained checkpoints for exactly those, and nothing was run — so the flip should not land without that being checked.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the retention_interval declaration and assignment in include/vllm/v1/core/kv_cache_coordinator.h and src/vllm/v1/core/kv_cache_coordinator.cpp, then trace KVCacheConfig in include/vllm/v1/kv_cache_interface.h and vllm_model_params in include/vllm.h. Compare the existing validation and cache-manager semantics with the upstream behavior described in the issue, and identify the affected tests before changing the default. Done means the value is resolved, threaded through configuration and the coordinator, validated, exposed through the CLI/ABI, and the SWA/Mamba default change is checked.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100