Skip to content

[#16251][feat] KV cache manager v1: add disk (L3) tier with per-request retention TTL - #16252

Open
nafis271 wants to merge 55 commits into
NVIDIA:mainfrom
deepinfra:disk-kv-tier-pr
Open

[#16251][feat] KV cache manager v1: add disk (L3) tier with per-request retention TTL#16252
nafis271 wants to merge 55 commits into
NVIDIA:mainfrom
deepinfra:disk-kv-tier-pr

Conversation

@nafis271

@nafis271 nafis271 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Adds an optional disk-backed L3 KV cache tier to the v1 KV cache manager.
  • Adds asynchronous disk spill and GPU onboarding after prefix hits.
  • Adds per-request disk retention through kv_cache_ttl_seconds.
  • Adds disk configuration:
    • disk_cache_size
    • disk_cache_path
    • disk_cache_retained_only
    • disk_cache_protect_unexpired
  • Adds readiness gating and request parking for pending asynchronous disk reads.
  • Prevents speculative-decoding disk-slot collisions by reusing parked-request slots and namespacing draft and target cache directories.
  • Strengthens disk I/O checks by rejecting short reads and writes.
  • Adds disk-level eviction, retention, expiry, admission, and protection logic.
  • Extends C++ and Python serialization, bindings, and configuration APIs.

Dev Engineer Review

  • Disk-tier configuration is propagated through KVCacheManager, BlockManager, WindowBlockManager, nanobind, and Python resource management.
  • KVCacheBlock tracks disk slots, deadlines, retention state, and reuse counts.
  • Asynchronous reader and writer workers use bounded queues, completion tracking, per-slot ordering, and orderly shutdown.
  • areBlocksReady() prevents forwarding before detached disk reads complete and reports asynchronous read failures.
  • PyExecutor parks pending context requests and synchronizes tensor-parallel readiness where supported.
  • Disk admission supports retained-only and unexpired-protection policies.
  • Serialization and pickle state include the new disk configuration and retention fields.
  • Review should verify concurrency around detached reads, block recycling, disk residency swaps, and worker shutdown.
  • Review should verify validation for enabled disk caches, disk paths, unsupported parallelism modes, and environment-controlled queue behavior.

QA Engineer Review

Test changes

  • cpp/tests/unit_tests/batch_manager/evictionPolicyTest.cpp

    • Added LRUPolicyTest.IsEnqueuedTest.
    • Updated placeholder cache-level expectations.
  • cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp

    • Added disk spill and onboard correctness tests.
    • Added byte-exact, concurrent, and synchronous onboarding tests.
    • Added readiness-gating tests.
    • Added asynchronous read and write failure tests.
    • Added worker shutdown and queue-bound tests.
    • Added TTL expiry, retention restamping, retained-only admission, deadline ordering, unexpired-protection, and deadline-bookkeeping tests.
  • tests/unittest/llmapi/test_disk_tier_args.py

    • Added configuration default, round-trip, pickle, and pybind conversion tests.
    • Added request TTL translation tests.
    • Added validation for negative, zero, and positive kv_cache_ttl_seconds values.

Coverage

  • No test-list changes were identified.
  • Coverage in tests/integration/test_lists/, test-db/, and qa/ was not confirmed.
  • The added test functions were not confirmed in test-db/ or qa/.

Verdict: needs follow-up.

Description

Closes #16251.

The v1 KV cache block manager currently supports two tiers: GPU memory (primary) and
host memory (secondary). This PR adds a third, disk-backed tier. When a block is
evicted from the secondary (host) tier it is spilled to disk instead of being dropped,
and is onboarded back to GPU on a later prefix hit — extending KV reuse well beyond
DRAM (a disk read is far cheaper than recomputing a long prefix).

It also adds a user-provided retention TTL (kv_cache_ttl_seconds, per request): the
engine keeps that request's KV blocks reusable on disk for the requested duration and
tries to honor it unless the disk tier is out of space.

KV cache manager v2 already has a three-tier (disk) cache, but today it must run with
the overlap scheduler disabled, its performance with the disk tier is not
production-level
(see Benchmarks below), and it has no user-provided retention TTL.
This PR brings a production-oriented disk tier — with a retention TTL — to the default v1
manager.

The tier is off by default; nothing is added to the scheduling hot path when
disk_cache_size is unset.

How it works

request ─▶ radix reuse tree  (prefix match by token hash)
           a reusable block lives at exactly one level:

   GPU (primary) ──spill──▶ host (secondary) ──spill──▶ disk (L3, new)
        ▲                                                    │
        └──────────────────── onboard ◀──────────────────────┘
                              (on a prefix hit)
  • Levels & bookkeeping. Each reusable block is tracked at one cache level in the
    existing radix reuse tree. Disk blocks are poolless — a block id + a fixed per-rank
    disk slot, no KV memory pool — so disk capacity is independent of GPU/host memory. A new
    kDiskLevel is added to the eviction policy (evictionPolicy.cpp).
  • Spill (host → disk). When the host tier reclaims a block that still has content, its
    bytes are written to that block's disk slot instead of being dropped; the block stays in
    the reuse tree, now disk-resident (reclaimSecondaryBlockspillToFile in
    kvCacheManager.cpp / kvCacheTransferManager.cpp). A small pool of pre-reserved host
    blocks (RESERVED_BLOCKS) lets a spill hand back a free slot immediately — the
    evicted block drains to disk in the background — so the scheduler never waits on a copy;
    if that pool empties under a burst, the block is copied out instead.
  • Onboard (disk → GPU). On a prefix hit to a disk-resident block, its bytes are read
    back into a GPU block. Reads are asynchronous with per-block readiness: the scheduler
    parks the request until its blocks are ready and then resumes it, never blocking on disk
    I/O (onboardBlock / loadFromFile; park logic in py_executor.py).
  • Async I/O. Spills and onboards run on background writer/reader threads draining a
    bounded shared queue (diskWriterLoop / diskReaderLoop); the scheduler enqueues and
    continues, with backpressure when the queue is full.
  • Eviction. Disk blocks evict LRU by default, or earliest-expiry-first when a
    retention TTL is set.
  • Retention TTL. kv_cache_ttl_seconds stamps a request's committed blocks with an
    expiry (mRetentionExpiry); on disk they are protected from eviction (deadline heap
    mDiskDeadlines) until they expire. The stamp is orthogonal — it never changes
    GPU/host eviction, so retained traffic can't crowd normal traffic out of the fast tiers.
  • Tensor parallelism. Per-rank disk directory; onboard readiness is synchronized across
    ranks.

Configuration (kv_cache_config)

field type default meaning
disk_cache_size int (bytes) None Size of the disk (L3) tier; enables it when > 0.
disk_cache_path str None Local directory for disk KV files. Required when disk_cache_size > 0.
disk_cache_retained_only bool false When true, only blocks whose request carried a retention TTL may enter the disk tier.
disk_cache_protect_unexpired bool false When true, an unexpired retained disk block is never evicted to admit a new one — the new block is refused instead.
kv_cache_config:
  enable_block_reuse: true
  free_gpu_memory_fraction: 0.8
  host_cache_size: 107374182400        # 100 GiB host tier (L2)
  disk_cache_size: 322122547200        # 300 GiB disk tier (L3)
  disk_cache_path: /mnt/nvme/kv        # local NVMe directory
  disk_cache_retained_only: false
  disk_cache_protect_unexpired: false

Environment variables (tuning)

variable default meaning
TLLM_KV_DISK_ASYNC_STORE unset (off) Spill on background writer threads instead of synchronously on the scheduler thread.
TLLM_KV_DISK_WRITERS 1 Number of background writer threads draining the spill queue.
TLLM_KV_DISK_READERS 0 Number of background onboard (reader) threads; 0 = no dedicated readers.
TLLM_KV_DISK_WRITE_QUEUE 1024 Max queued spills before enqueue backpressure.
TLLM_KV_DISK_RESERVED_BLOCKS 0 (off) Reserve a pool of host blocks to lower the scheduler's per-spill overhead under heavy spill load; larger = more burst headroom. 0 uses the default spill path. Size it against how far the writers can fall behind.
TLLM_KV_DISK_MIN_REUSE 0 Spill only blocks reused at least k times (0 = spill all).
TLLM_KV_DISK_DROP_ON_PRESSURE 0 (off) Under writer saturation, drop best-effort spills instead of stalling; retained (TTL) spills always land.
TLLM_KV_DISK_GDS unset (off) Use GPUDirect Storage for onboard reads instead of POSIX (experimental).

Serve API (retention TTL)

kv_cache_ttl_seconds (optional int) on chat-completion and completion requests: "Keep
this request's KV cache reusable for this many seconds (disk-tier retention)."
Omitted =
today's best-effort LRU reuse.

curl http://localhost:8000/v1/chat/completions \
  -H 'Content-Type: application/json' \
  -d '{
    "model": "openai/gpt-oss-120b",
    "messages": [
      {"role": "system", "content": "<large shared system prompt>"},
      {"role": "user", "content": "..."}
    ],
    "kv_cache_ttl_seconds": 3600
  }'

Benchmarks

Setup. gpt-oss-120b, single B300, TP=1. Both arms share: 100 GiB host tier + 300 GiB
disk tier, free_gpu_memory_fraction=0.8, block reuse on / partial-reuse off, MoE-TRTLLM
backend, MAX_UTILIZATION scheduler, chunked prefill. v1 (this PR): overlap scheduler
on; asynchronous spill/onboard with 4 reader + 4 writer threads, a reserved-block pool,
spill-all — every evicted block written to disk (disk_cache_retained_only=false, reuse
gate off), no drop-on-pressure.
v2: use_kv_cache_manager_v2=true; overlap scheduler off (required — overlap-on crashes
under load); disk prefetch tested at both disk_prefetch_num_reqs=2 and =32, with
identical results (~250 s TTFT either way) — V2's collapse is not a prefetch-tuning artifact.
Load generator: LMCache LMBenchmark, the
synthetic-multi-round-qa/long_input_short_output_run.sh workload — 120 concurrent users,
12 rounds, ~50k-token chat history, 1000-token system prompt, 100-token answers, at qps
1.5 for 500 s
— a working set that overflows GPU + host into the disk tier. Identical
workload for both arms; figures are LMBenchmark's per-interval report windows.

Configuration Overlap sched Throughput (in-tok/s) TTFT Result
v1 + disk tier (this PR) on ~85k (sustains offered load) ~0.8–1.2 s steady converges to steady state; disk fully engaged (307 GB, ~1.27M spills / ~1.0M onboards); 0 crashes
v2 + disk tier (stock rc19) off (required) ~15–20k (collapses to ~1/5) diverges: ~5 s → 253 s never stabilizes — growing backlog once spilling starts; no crash
  • v1 (this PR) runs with the overlap scheduler enabled. After a one-time cold-fill
    warmup (TTFT transiently ~16 s while the 300 GB tier writes out), it converges to a
    steady ~0.8–1.2 s TTFT while sustaining the full ~85k in-tok/s offered load.
  • v2 requires the overlap scheduler disabled (overlap-on crashes under load). Even
    in that stable mode it cannot sustain the load: once the working set overflows into
    disk, throughput collapses to ~1/5 of offered and per-window TTFT climbs monotonically
    past 250 s — a backlog that never stabilizes.

Test Coverage

  • cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp — disk-tier suite: byte-exact
    spill/onboard round-trip, disk_cache_retained_only admission gate, TTL / deadline
    eviction order, disk_cache_protect_unexpired, the TLLM_KV_DISK_MIN_REUSE reuse gate,
    and disk-tier queue-integrity validation.
  • cpp/tests/unit_tests/batch_manager/evictionPolicyTest.cpp — three-level eviction policy.
  • tests/unittest/llmapi/test_disk_tier_args.py — disk config validation and the
    serve-layer kv_cache_ttl_seconds → retention-config translation.

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title. (This PR is api-compatible: new optional config fields + an optional request field; no behavior change when unset.)

  • Any new dependencies have been scanned for license and vulnerabilities (none added)

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

nafis271 added 29 commits July 10, 2026 13:26
…sk blocks, host->disk spill and disk->GPU onboard

Based on v1.3.0rc19. Adds a third KV-cache tier backed by POSIX slot files:
- KvCacheConfig gains diskCacheSize/diskCachePath (serialization + nanobind + llm_args passthrough)
- Poolless disk blocks at eviction level 2 (placeholders renumber to 3); block residency
  swaps between pool index and disk slot so the reuse-tree identity never moves
- Host-tier victims with reusable content spill to disk on reclaim (spill-all;
  filtering mode to follow); disk-resident matches onboard file->GPU in onboardBlock
- Synchronous POSIX I/O v1; pytorch path only (TRT-engine path passes 0)

Validated on gpt-oss-120b: 49824/49836 cached tokens served through a disk round
trip with temperature-0 byte-identical output; 300k+ spills and 11k+ onboards under
sustained production traffic with zero I/O errors.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…retention TTL, serve wiring

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… displacement (empties first, exact TTL order, suffix-first FIFO), retention wiring at all serve entry points

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…aking), unit tests (C++ disk-tier suite + isEnqueued + python plumbing), disk-count getters

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…ate/expiry/reuse/deadline + isEnqueued + placeholder level fix)

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…use new blocks when disk full of live TTLs; only expired evictable) + unit tests

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…empty slots before displacing cached blocks)

Empty disk slots initialized at default priority (bucket 35) while unmarked spilled
content was filed at kMin (bucket 0). getFreeBlock returns the lowest bucket first, so
claimDiskTarget displaced just-spilled cached blocks before using free empty slots ->
unmarked content never survived to re-request (spill-all onboard ~0% under churn).

Restore order empty(kMin) < unmarked(kDefault) < retained(kMax):
- evictionPolicy initialize(): disk-level blocks start at kMin (+ setPriority in lockstep
  so claim/release erase from the correct queue).
- reclaimSecondaryBlock: unmarked spilled content -> kDefault (above empties, below
  retained); evicted LRU only once disk is genuinely full.
retained path unchanged. Verified: spill-all onboard 0%->99%, disk-tier unit tests
pass, gate+expiry+retained-survival unaffected.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…served-pool unstaged)

Working per-slot async fire-and-forget store: background writer thread +
per-slot files + staged memcpy + load-wait. GDS path still present. Snapshot
taken before the unstaged reserved-block-pool rework so we can roll back cleanly.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…in source, hand out reserved slot)

Async spill hands the writer the victim host slot directly (no staging memcpy),
pins that slot until the write drains, and returns a pre-claimed reserved host
block instead. Reaped drained slots rejoin the pool: one out, one in. Gated by
TLLM_KV_DISK_RESERVED_BLOCKS (0=off=staged).

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… vs enqueue-backpressure vs queue depth)

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…ared queue (TLLM_KV_DISK_WRITERS)

Single writer saturated the queue under eviction bursts (qpeak pinned at 1024,
multi-second enqueue backpressure -> TTFT spikes). Spawn N workers draining the
shared queue; idle workers pull the next job (auto-balanced), per-file in-flight
gate keeps same-slot writes serialized. Default 1.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… scheduler thread

Disk onboards read SSD->GPU synchronously on the scheduler thread (or, with
a reader pool, in parallel but still barriered before the forward), stalling
every request in the step behind one slow read.

Engine (C++): a disk onboard hands each of a block's pool reads to the reader
pool and returns; completion is tracked per block, keyed by the matched-identity
block id so requests reusing a prefix gate on the same key. The reader makes its
copy device-complete before publishing, and GDS/POSIX both flow through the pool.
areBlocksReady(requestId) reports when a request's blocks have landed; the read
barrier (waitForAllReads/mReadInflight) is removed.

Scheduler (Python): a context request whose onboard reads are still in flight is
held out of the forward pass and re-checked each step -- the capacity scheduler
counts its already-allocated blocks and _context_seq_len skips re-adding it --
then admitted once areBlocksReady().

Tests: detached reads byte-exact incl. concurrent draining; block-manager reuse
matches the synchronous path and its readiness gate resolves.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…_KV_DISK_MIN_REUSE)

Add a per-block reuse counter (mReuseCount, bumped at the prefix-match reuse
point in onboardAndAllocateBlocks) and gate the host->disk spill on it: only
spill blocks reused >= k times (k = TLLM_KV_DISK_MIN_REUSE env, 0 = off).
Sits beside the retained_only gate as an independent 3rd disk mode
(spill-all / retention-only / reuse-gate). The counter is block metadata, so it
survives GPU/host/disk tier moves like mRetentionExpiry (swapDiskResidency
swaps only residency pointers).

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…k dir + cross-rank onboard readiness)

Two fixes to run the disk KV-cache tier under TP>1 (validated on gpt-oss-120b TP=2).

Per-rank disk cache directory: all ranks share one container filesystem and one
disk_cache_path, but the disk slot files (block_<id>_pool_<p>.bin) are named identically
across ranks while each rank holds a different KV shard -- so without namespacing the
ranks clobber each other's KV on load. Append /rank_<mapping.rank> (the global rank,
unique across TP and PP) and mkdir it. Note: disk_cache_size is per-rank, so total disk
= world_size * disk_cache_size.

Cross-rank onboard-readiness sync: the detached disk-onboard reads land at slightly
different times per rank, so a per-rank park decision in _park_requests_awaiting_onboard
would give ranks different batches and the TP all-reduce in the forward would mismatch
(hang/crash). AND the per-request readiness across the TP group -- a cheap tp_allreduce
"any pending?" fast path, then tp_allgather when a read is in flight -- so a request is
admitted only once its KV has landed on every rank. pp_size==1 => the TP group is the
whole world; TP=1 is a no-op (identical to the prior path).

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… read is in flight

areBlocksReady scans every block a request holds via isBlockReadPending, which takes
mReadMutex per block, and the scheduler-side park calls it for every context request
every step. With no disk read in flight (the common case) that is an O(context-length)
mutex-locked scan over blocks that are all trivially ready.

Add a lock-free atomic mirror of the in-flight read count (mReadInflightCount, updated
under mReadMutex on every pending-set insert/erase) and short-circuit areBlocksReady to
return true immediately when it is zero. Behaviour is identical when reads are pending.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…s CUDA device

Reader threads created their CUDA stream without setting the device, so on TP ranks whose device is not 0 the POSIX H2D onboard targeted the wrong GPU and crashed. Bind the thread to the manager's device at loop entry.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…validators

verifyQueueIntegrity had three validators/labels for four queue levels: the disk level reused the placeholder validator and the loop indexed one past the array. Give each level its own validator and label (disk = isOnDisk).

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…blocks

A disk-resident block cannot serve a partial reuse -- its bytes live in a reserved disk slot, not in pool memory, so the partial-copy path reads garbage. Exclude on-disk blocks from both partial-match routes; full matches stay reusable via the async onboard.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… read completes

onboardBlock enqueued the async read of a disk slot then immediately returned its card to the disk free queue, so the next spill could claim and overwrite the slot file before the read ran -- onboarding the wrong block. Hold the card until the read lands (reclaimed in claimDiskTarget once the read completes) and drop the now-unused completed-reads list.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
Short reads/writes and open failures on disk slot files were logged but the block was still published, serving or caching corrupt KV. Fail loud on any disk I/O error, matching the synchronous spill path.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…lain TP

The cross-rank onboard-readiness allreduce/allgather is only correct under plain tensor parallelism; under attention-DP or pipeline parallelism it deadlocks or drops requests. Gate the collective to plain TP, and refuse the disk tier under attention-DP/PP at KV-manager init.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…ard-pending requests

can_queue was computed before parking requests whose disk onboard had not landed; if parking emptied the batch we forwarded on nothing. Re-check can_queue after parking, broadening the existing post-batch-change re-check.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…scheduler into the reader

loadFromFile ran waitForDiskSlotWrites on the scheduler even on the detached path, so onboarding a just-spilled slot could park the whole engine behind the write queue. Do that wait on the reader instead -- the owning request stays parked via isBlockReadPending until the read lands, so the scheduler is never blocked. The synchronous fallback still waits inline.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… (opt-in TLLM_KV_DISK_DROP_ON_PRESSURE)

Under write-queue saturation the scheduler blocked in enqueueDiskWrite* waiting for room. Add an opt-in policy (TLLM_KV_DISK_DROP_ON_PRESSURE, default off = prior wait behavior): best-effort (non-retained) spills are shed at the eviction gate instead of stalling; retained spills never drop and bypass the queue cap (their TTL guarantee requires they land). Drop is decided per-spill at the gate, not per-pool in enqueue*, which would otherwise leave partial/corrupt blocks and leak the reserved-pool reap.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…host buffer

The POSIX reader staged each onboard through a pageable std::vector, so cudaMemcpyAsync fell back to the driver's hidden pinned-staging copy -- H2D ran at ~5-8 GB/s and the call was secretly synchronous. Stage through a per-reader pinned buffer instead (grown on demand to one block's pool bytes; pageable fallback if pinning is refused). Each pool reads into its own offset with a single stream sync after the block, so the async DMA never races the next read and pool reads overlap prior pools' DMA. Onboard latency is parked-request TTFT for disk-cache hits.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
… collective when the disk tier is off

_park_requests_awaiting_onboard ran on every context step and, on plain TP>1, issued a per-step object-pickling tp_allreduce -- even with no disk tier, because it only bailed on hasattr(are_blocks_ready), which is always true. That taxed every TP>1 model sharing the image. Gate the park on a flag captured at init (disk_cache_size>0 AND TLLM_KV_DISK_READERS>0): detached onboards are impossible otherwise, so are_blocks_ready() is always true and the park + its collective are pure overhead. No-op for disk deployments (flag true -> unchanged); the skip only affects non-disk / sync-onboard configs.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…n onboard/release

Two paths could hand back valid-looking KV that is actually the wrong tokens:

1. releaseBlocks freed a block whose detached disk-onboard read was still in
   flight. areBlocksReady() parks the forward path on read-readiness, but nothing
   gated release: a request completing/cancelling mid-onboard returned its
   DMA-destination block to the free queue, and getFreeBlock has no readiness
   check -- so a stranger's prefill and the late read collided in the same GPU
   memory. Mirror the park onto the release path: hold read-pending blocks in
   mReleaseReadPending until the DMA lands (reapReadPendingReleases frees them).
   Gated by mNumDiskBlocks>0 && anyReadPending(), so non-disk models are unaffected.

2. reclaimSecondaryBlock's sync (copy) spill returned diskTarget without detaching
   its displaced disk-cached identity, relying on the caller's cleanup. getFreeBlock
   detaches; offloadBlock (the other caller) does not -- leaving a radix-tree node
   pointing at the recycled host slot, so a later prefix match reused freed content.
   Detach the identity unconditionally in reclaimSecondaryBlock for both callers;
   getFreeBlock's later detach then becomes a harmless no-op.

Validated: kvCacheManagerTest disk-tier suite 11/11 green (spill/onboard/release +
byte-exact onboard); the 12 pre-existing non-disk failures are unchanged.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…disk-free check

reapReadPendingReleases() -- which returns landed onboard cards (and, since the
release-vs-onboard fix, release-deferred blocks) to their free queues -- ran only
inside claimDiskTarget(). But reclaimSecondaryBlock() reaches claimDiskTarget only
after passing a getNumFreeBlocks(kDiskLevel)==0 early-return, and parked cards sit
OUTSIDE the free queue. So an onboard-heavy burst that parks every free disk card
drives the free count to 0, fires the early-return, and never reaches claimDiskTarget
(the only reap site) -- the landed-but-parked cards are never reclaimed and spills
wedge permanently, exactly under the load the retained tier is for.

Reap right after claiming the victim, before the disk-free check, guarded by
mNumDiskBlocks>0. Placed in reclaimSecondaryBlock so it covers both callers
(getFreeBlock and offloadBlock); claimDiskTarget's reap stays as a harmless second pass.

Validated: kvCacheManagerTest disk-tier suite 11/11 green; the 12 pre-existing
non-disk failures are unchanged.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
nafis271 added 2 commits July 19, 2026 20:37
…ve the disk

A retained block spilled to disk pushes an entry onto mDiskDeadlines, but
nothing removed that entry when the block was onboarded back to GPU. The
only cleanup was inside the displacement walk, which runs only when the
disk is full of retained blocks -- so under normal reuse the stale entries
accumulated forever (memory leak, and ever-longer walks once displacement
does run).

Fix: keep the deadlines in a std::set keyed by (expiry, seq). Each block
remembers its key, so its entry is erased with one lookup whenever the
block leaves the disk (onboard or displacement). The walk also erases any
stale entry it finds.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
@mikeiovine
mikeiovine requested review from mikeiovine and removed request for schetlur-nv July 20, 2026 15:30
nafis271 added 4 commits July 20, 2026 21:14
…parallelism

The drop decision depends on each rank's write-queue depth, so ranks
could diverge on which blocks remain reusable. Refuse the combination
at init, like attention-DP and pipeline parallelism.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
…lock

Retention deadlines were anchored and checked against each rank's own
wall clock, which could diverge the radix trees across ranks. Now one
rank broadcasts its time each iteration and every rank uses it for
retention decisions.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
@nafis271

Copy link
Copy Markdown
Contributor Author

Hi @chienchunhung, thanks again for taking the time to review; those should be fixed now. Would appreciate it if you could take another look when you have time.
also if a maintainer could add the api-compatible label: the PR only adds an optional config field and an optional request field, so default behavior is unchanged.

@nvpohanh

Copy link
Copy Markdown
Collaborator

[by Codex] @thorjohnsen Could you please review this PR? Thank you!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h (2)

1517-1542: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

mDiskWritePressureDropped counter has no public getter.

getNumDiskSpills, getNumDiskGateDropped, getNumDiskAdmissionRefused, and getNumDiskOnboards are all exposed at both WindowBlockManager (1524-1542) and aggregated at BlockManager (2205-2243) level, but mDiskWritePressureDropped (declared at line 1519) has no corresponding getter at either level. Silent write-pressure drops are exactly the kind of disk-tier backpressure signal operators need to diagnose reliability issues; without a getter it's currently unobservable.

Also applies to: 2205-2253

🤖 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 `@cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h` around lines 1517 -
1542, Add public getters for mDiskWritePressureDropped in both
WindowBlockManager and the aggregated BlockManager interfaces, following the
existing getNumDiskSpills and related getter patterns, and ensure BlockManager
aggregates and exposes the corresponding counter.

1322-1330: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Doxygen comment appears merged/misattributed between reclaimSecondaryBlock and onboardBlock.

Lines 1322-1323 ("Bring offloaded block from secondary to primary memory... Does nothing if block is already in primary memory") describe onboardBlock's behavior, but the new lines 1324-1326 were inserted between that comment and reclaimSecondaryBlock()'s declaration, so the whole stacked block (two \brief tags) now documents reclaimSecondaryBlock, leaving onboardBlock (line 1329) with no doc comment at all.

📝 Proposed fix to restore correct doc-comment attribution
-    //! \brief Bring offloaded block from secondary to primary memory.
-    //! \details Does nothing if block is already in primary memory.
-    //! \brief Reclaim a free secondary (host) block for reuse. If the victim still holds
+    //! \brief Reclaim a free secondary (host) block for reuse. If the victim still holds
     //! reusable content and the disk tier has room, spill it to disk first (residency swap;
     //! the tree-resident identity moves to the disk level). Returns a CLAIMED block.
     [[nodiscard]] BlockPtr reclaimSecondaryBlock();

+    //! \brief Bring offloaded block from secondary to primary memory.
+    //! \details Does nothing if block is already in primary memory.
     void onboardBlock(GenerationRequest& sequence, BlockPtr const& offloadBlock,
         executor::KvCacheTransferMode mode = executor::KvCacheTransferMode::DRAM, std::string const& directory = "");
As per coding guidelines, "use Doxygen comments for new interfaces."
🤖 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 `@cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h` around lines 1322 -
1330, Separate the Doxygen comments so the reclaimSecondaryBlock documentation
describes only reclaiming a free secondary block and spilling reusable content
to disk, while the “Bring offloaded block…” documentation immediately precedes
onboardBlock. Ensure both interfaces retain accurate, distinct documentation and
avoid stacked \brief tags.

Source: Coding guidelines

🧹 Nitpick comments (1)
cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h (1)

459-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

New sequence-number fields use unsigned types without an approved exception.

mDiskDeadlineSeq/getDiskDeadlineSeq/setDiskDeadlineSeq (459-467), DiskDeadline::seq + mDiskSpillSeq (1560-1577), and mUnstagedSpillSeq/mPendingSpillBlocks key (1586-1601) are all new std::uint64_t fields that are monotonically-increasing counters, not bitmaps, external-library requirements, or size comparisons.

As per coding guidelines, "Prefer signed integers; use unsigned types only for bitmaps, external-library requirements, or unavoidable size comparisons."

Also applies to: 1560-1577, 1586-1601

🤖 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 `@cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h` around lines 459 -
467, Replace the unsigned types used for the sequence counters with approved
signed integer types throughout the affected declarations and APIs. Update
getDiskDeadlineSeq/setDiskDeadlineSeq, DiskDeadline::seq, mDiskSpillSeq,
mUnstagedSpillSeq, and the mPendingSpillBlocks key consistently, including any
related initializers or comparisons.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h`:
- Around line 1517-1542: Add public getters for mDiskWritePressureDropped in
both WindowBlockManager and the aggregated BlockManager interfaces, following
the existing getNumDiskSpills and related getter patterns, and ensure
BlockManager aggregates and exposes the corresponding counter.
- Around line 1322-1330: Separate the Doxygen comments so the
reclaimSecondaryBlock documentation describes only reclaiming a free secondary
block and spilling reusable content to disk, while the “Bring offloaded block…”
documentation immediately precedes onboardBlock. Ensure both interfaces retain
accurate, distinct documentation and avoid stacked \brief tags.

---

Nitpick comments:
In `@cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h`:
- Around line 459-467: Replace the unsigned types used for the sequence counters
with approved signed integer types throughout the affected declarations and
APIs. Update getDiskDeadlineSeq/setDiskDeadlineSeq, DiskDeadline::seq,
mDiskSpillSeq, mUnstagedSpillSeq, and the mPendingSpillBlocks key consistently,
including any related initializers or comparisons.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9bcdd530-40cb-4e20-8459-871e5d912725

📥 Commits

Reviewing files that changed from the base of the PR and between 6d61f66 and 3ea3cd8.

📒 Files selected for processing (8)
  • cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
  • cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h
  • cpp/include/tensorrt_llm/executor/executor.h
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
  • cpp/tensorrt_llm/executor/serialization.cpp
  • cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/nanobind/executor/executorConfig.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
  • cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h
  • cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/executor/serialization.cpp
  • cpp/tensorrt_llm/nanobind/executor/executorConfig.cpp
  • cpp/include/tensorrt_llm/executor/executor.h
  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp

…doc comment

Expose mDiskWritePressureDropped via getters at both WindowBlockManager and
BlockManager, matching the other disk counters. Split the stacked doc comment
so reclaimSecondaryBlock and onboardBlock each carry their own.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
@nvpohanh

Copy link
Copy Markdown
Collaborator

[by Codex] @thorjohnsen Friendly review reminder: this PR is awaiting your review. Thanks!

@nafis271

nafis271 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@thorjohnsen @nvpohanh @chienchunhung gentle ping - review when you get a chance would be much appreciated 🙏

…spec decode

A parked disk onboard re-enters as a first context chunk, so eagle3/mtp
reuse an existing hidden-state slot instead of re-allocating. And the
disk slot directory is namespaced by draft/target, not just rank, so a
draft KV manager cannot overwrite the target's slot files.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tensorrt_llm/_torch/pyexecutor/resource_manager.py (2)

1504-1510: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add the required public API type and documentation.

Annotate request as LlmRequest. Add Google-style documentation for set_retention_clock.

Proposed fix
-    def are_blocks_ready(self, request) -> bool:
+    def are_blocks_ready(self, request: LlmRequest) -> bool:
🤖 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 `@tensorrt_llm/_torch/pyexecutor/resource_manager.py` around lines 1504 - 1510,
Update ResourceManager.are_blocks_ready to annotate its request parameter as
LlmRequest, importing the type if needed. Add a Google-style docstring to
set_retention_clock describing the now_ns argument and the method’s
retention-clock behavior, while preserving the existing delegation.

Source: Coding guidelines


1509-1510: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Synchronize the retention clock before disaggregated generation initialization.

_prepare_disagg_gen_init() calls the KV resource manager directly before _sync_retention_clock(). This path can perform retention admission or eviction with stale mRetentionNow. Synchronize the clock before this call.

🤖 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 `@tensorrt_llm/_torch/pyexecutor/resource_manager.py` around lines 1509 - 1510,
Update the disaggregated generation initialization flow in
_prepare_disagg_gen_init() to invoke _sync_retention_clock() before calling the
KV resource manager, ensuring retention admission and eviction use the current
clock. Keep set_retention_clock() unchanged and preserve the existing
initialization sequence after synchronization.
🤖 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.

Outside diff comments:
In `@tensorrt_llm/_torch/pyexecutor/resource_manager.py`:
- Around line 1504-1510: Update ResourceManager.are_blocks_ready to annotate its
request parameter as LlmRequest, importing the type if needed. Add a
Google-style docstring to set_retention_clock describing the now_ns argument and
the method’s retention-clock behavior, while preserving the existing delegation.
- Around line 1509-1510: Update the disaggregated generation initialization flow
in _prepare_disagg_gen_init() to invoke _sync_retention_clock() before calling
the KV resource manager, ensuring retention admission and eviction use the
current clock. Keep set_retention_clock() unchanged and preserve the existing
initialization sequence after synchronization.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ff523bfb-7a2e-4a62-b40c-7adbdff7fd59

📥 Commits

Reviewing files that changed from the base of the PR and between 343512c and 4bcca7b.

📒 Files selected for processing (3)
  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
  • tensorrt_llm/_torch/speculative/eagle3.py
  • tensorrt_llm/_torch/speculative/mtp.py

# Conflicts:
#	cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
#	cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
#	cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
… are_blocks_ready

The disagg-gen-init path allocates and commits KV before the main-loop
retention-clock sync, so it would anchor deadlines against a stale clock;
sync there too. Also annotate are_blocks_ready's request parameter and
document set_retention_clock.

Signed-off-by: Md Nafis Ul Haque Shifat <nafis@deepinfra.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (7)
cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h (1)

199-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Declare mDiskUseGds as const.

mDiskUseGds is read once at construction and never reassigned, like mAsyncDiskStore on line 203. Mark it bool const for consistency and to prevent later mutation.

As per coding guidelines: "declare unmodified variables as const".

♻️ Proposed change
-    bool mDiskUseGds{std::getenv("TLLM_KV_DISK_GDS") != nullptr};
+    bool const mDiskUseGds{std::getenv("TLLM_KV_DISK_GDS") != nullptr};
🤖 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 `@cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h` at line 199,
Declare the KVCacheTransferManager member mDiskUseGds as const, matching the
existing const treatment of other immutable configuration members such as
mAsyncDiskStore while preserving its current environment-based initialization.

Source: Coding guidelines

tests/unittest/llmapi/test_disk_tier_args.py (1)

51-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use an isolated temporary directory for this test.

/tmp/kvdisk-test is shared across test runs and remains after the test completes. A pre-existing file at that path can make os.makedirs(..., exist_ok=True) fail. Use the tmp_path fixture and pass str(tmp_path) as disk_cache_path.

Proposed change
-def test_llm_args_kv_cache_config_disk_fields():
-    import os
-
-    os.makedirs("/tmp/kvdisk-test", exist_ok=True)  # pydantic validator requires an existing dir
+def test_llm_args_kv_cache_config_disk_fields(tmp_path):
     from tensorrt_llm.llmapi.llm_args import KvCacheConfig as PydanticKvCacheConfig
 
     py_cfg = PydanticKvCacheConfig(
         disk_cache_size=1 << 30,
-        disk_cache_path="/tmp/kvdisk-test",
+        disk_cache_path=str(tmp_path),
🤖 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 `@tests/unittest/llmapi/test_disk_tier_args.py` around lines 51 - 69, Update
test_llm_args_kv_cache_config_disk_fields to accept the tmp_path fixture and use
it as the disk cache directory instead of the shared /tmp/kvdisk-test path.
Remove the manual os.makedirs call, pass str(tmp_path) to disk_cache_path, and
update the corresponding assertion to compare against that value.
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp (4)

11122-11123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the new includes to the top of the file.

#include <filesystem> and #include <thread> appear at Line 11122, after roughly 11000 lines of code. Include directives belong in the file's include block so the translation unit's dependencies are visible in one place.

🤖 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 `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp` around lines 11122
- 11123, Move the <filesystem> and <thread> includes from their current location
near the end of the file into the existing top-of-file include block, keeping
the dependencies declared together and removing the duplicate late includes.

11655-11658: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Initialize the secondary pool before spilling from it.

pool.secondaryPtr is allocated as pinned host memory and is never written before spillToFileUnstaged reads it. The writer therefore copies indeterminate bytes to disk. The test passes either way, but sanitizers report the read. Add a std::memset or a fill loop, matching what runDiskOnboardByteRoundTrip does at Line 11568.

🤖 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 `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp` around lines 11655
- 11658, Initialize the pinned host buffer assigned to pool.secondaryPtr before
calling spillToFileUnstaged, using std::memset or the same fill approach as
runDiskOnboardByteRoundTrip. Ensure the entire allocated secondary pool contains
defined bytes before it is read and written to disk.

11445-11452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The control assertion depends on eviction victim selection.

EXPECT_EQ(blockManager.getNumDiskSpills(), spillsBefore) assumes the second churnOnce evicts only seqB's expired blocks. seqA2's blocks carry a 60 s TTL and were reused earlier in the same test, so if the churn selects any of them as a host victim, they pass the retained-only gate and the spill count increases. That makes this control assertion order-dependent.

Consider scoping the control to a fresh BlockManager, or asserting on a per-sequence signal instead of the global spill counter.

🤖 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 `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp` around lines 11445
- 11452, Make the control assertion independent of eviction victim selection by
isolating it in a fresh BlockManager, or replace the global getNumDiskSpills
comparison with a per-sequence signal that verifies seqB is not re-spilled.
Update the control setup around addDiskTierSequence, churnOnce, and the
EXPECT_EQ assertion while preserving the intended expired-sequence gating
behavior.

11221-11226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the doc comment to the function it describes.

The comment at Line 11221 describes addDiskTierSequence ("Adds a sequence; diskTtl set => ..."), but it sits directly above retentionTick. Move it to Line 11228, immediately above addDiskTierSequence.

🤖 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 `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp` around lines 11221
- 11226, Move the descriptive comment from above retentionTick to immediately
above addDiskTierSequence, leaving retentionTick without that unrelated
documentation and preserving the comment text.
tensorrt_llm/_torch/pyexecutor/py_executor.py (1)

2643-2645: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Remove the no-op park call from the PP loop.

resource_manager rejects disk KV-cache configurations when pp_size > 1, and _park_requests_awaiting_onboard() returns when disk onboarding is inactive. The PP loop therefore cannot shrink the batch through this call and does not need the non-PP re-check.

🤖 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 `@tensorrt_llm/_torch/pyexecutor/py_executor.py` around lines 2643 - 2645,
Remove the `_park_requests_awaiting_onboard(scheduled_batch)` call from the PP
loop after `prepare_resources`; leave `_sync_retention_clock()` and
`resource_manager.prepare_resources(scheduled_batch)` unchanged.
🤖 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 `@cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h`:
- Around line 225-236: Add a shared environment-size parsing helper for
TLLM_KV_DISK_WRITE_QUEUE, TLLM_KV_DISK_WRITERS, and mNumDiskReaders that rejects
malformed, negative, and overflowing values, reports the variable name
diagnostically, and clamps each result to its valid range; use a minimum of 1
for the queue capacity and writer count, while preserving 0 as the valid
synchronous-read value for mNumDiskReaders. Update all three member initializers
to use the helper and add the required <cstring>, <stdexcept>, and <algorithm>
includes.

In `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`:
- Around line 11837-11856: The bounded-queue test runs its burst synchronously
without a timeout, so a stalled writer can hang indefinitely. In
runWriteQueueBoundedTest at
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp lines 11837-11856,
move burst production to a separate thread, join it with a deadline, and call
KVCacheTransferManagerTestAccess::requestWriterStop on timeout so the producer
unblocks before assertions; retain requestWriterStop at lines 115-123 because
this change uses it.
- Around line 11744-11770: The seed spills in the setup around
KVCacheTransferManager and spillToFile are asynchronous because
TLLM_KV_DISK_ASYNC_STORE is enabled before manager construction. Make the setup
description accurate by disabling or scoping out asynchronous disk storage
during seeding so spillToFile completes inline, then restore the asynchronous
setting before exercising queued reads; alternatively update the comment to
describe the actual queued behavior if synchronous seeding is not required.

In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 592-601: Update the environment parsing used for disk-reader
activation and construction to strictly validate TLLM_KV_DISK_READERS as a
non-negative decimal value, rejecting malformed strings such as “4x” and
negative values such as “-1”. Reuse the validated value consistently for
_disk_onboard_active and both disk-reader components, while preserving the
existing unset-default behavior of zero.

In `@tests/unittest/llmapi/test_disk_tier_args.py`:
- Around line 1-5: Add the repository-standard NVIDIA copyright and SPDX header
at the beginning of test_disk_tier_args.py, before its module docstring, using
2026 as the latest meaningful modification year.

---

Nitpick comments:
In `@cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h`:
- Line 199: Declare the KVCacheTransferManager member mDiskUseGds as const,
matching the existing const treatment of other immutable configuration members
such as mAsyncDiskStore while preserving its current environment-based
initialization.

In `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`:
- Around line 11122-11123: Move the <filesystem> and <thread> includes from
their current location near the end of the file into the existing top-of-file
include block, keeping the dependencies declared together and removing the
duplicate late includes.
- Around line 11655-11658: Initialize the pinned host buffer assigned to
pool.secondaryPtr before calling spillToFileUnstaged, using std::memset or the
same fill approach as runDiskOnboardByteRoundTrip. Ensure the entire allocated
secondary pool contains defined bytes before it is read and written to disk.
- Around line 11445-11452: Make the control assertion independent of eviction
victim selection by isolating it in a fresh BlockManager, or replace the global
getNumDiskSpills comparison with a per-sequence signal that verifies seqB is not
re-spilled. Update the control setup around addDiskTierSequence, churnOnce, and
the EXPECT_EQ assertion while preserving the intended expired-sequence gating
behavior.
- Around line 11221-11226: Move the descriptive comment from above retentionTick
to immediately above addDiskTierSequence, leaving retentionTick without that
unrelated documentation and preserving the comment text.

In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 2643-2645: Remove the
`_park_requests_awaiting_onboard(scheduled_batch)` call from the PP loop after
`prepare_resources`; leave `_sync_retention_clock()` and
`resource_manager.prepare_resources(scheduled_batch)` unchanged.

In `@tests/unittest/llmapi/test_disk_tier_args.py`:
- Around line 51-69: Update test_llm_args_kv_cache_config_disk_fields to accept
the tmp_path fixture and use it as the disk cache directory instead of the
shared /tmp/kvdisk-test path. Remove the manual os.makedirs call, pass
str(tmp_path) to disk_cache_path, and update the corresponding assertion to
compare against that value.
🪄 Autofix

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: 82d6dff9-97ea-41a5-b346-456005f5c37a

📥 Commits

Reviewing files that changed from the base of the PR and between 5533f66 and cebfeb5.

📒 Files selected for processing (23)
  • cpp/include/tensorrt_llm/batch_manager/evictionPolicy.h
  • cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
  • cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h
  • cpp/include/tensorrt_llm/executor/executor.h
  • cpp/tensorrt_llm/batch_manager/evictionPolicy.cpp
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
  • cpp/tensorrt_llm/executor/kvCacheConfig.cpp
  • cpp/tensorrt_llm/executor/kvCacheRetentionConfig.cpp
  • cpp/tensorrt_llm/executor/serialization.cpp
  • cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/nanobind/executor/executorConfig.cpp
  • cpp/tensorrt_llm/nanobind/executor/request.cpp
  • cpp/tests/unit_tests/batch_manager/evictionPolicyTest.cpp
  • cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
  • tensorrt_llm/_torch/pyexecutor/py_executor.py
  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
  • tensorrt_llm/_torch/speculative/eagle3.py
  • tensorrt_llm/_torch/speculative/mtp.py
  • tensorrt_llm/llmapi/llm_args.py
  • tensorrt_llm/serve/openai_protocol.py
  • tensorrt_llm/serve/openai_server.py
  • tests/unittest/llmapi/test_disk_tier_args.py
🚧 Files skipped from review as they are similar to previous changes (19)
  • cpp/include/tensorrt_llm/batch_manager/evictionPolicy.h
  • tensorrt_llm/_torch/speculative/eagle3.py
  • cpp/tensorrt_llm/executor/kvCacheRetentionConfig.cpp
  • tensorrt_llm/_torch/speculative/mtp.py
  • tensorrt_llm/serve/openai_protocol.py
  • cpp/tests/unit_tests/batch_manager/evictionPolicyTest.cpp
  • tensorrt_llm/llmapi/llm_args.py
  • cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
  • cpp/tensorrt_llm/nanobind/executor/request.cpp
  • cpp/tensorrt_llm/executor/kvCacheConfig.cpp
  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
  • cpp/tensorrt_llm/nanobind/executor/executorConfig.cpp
  • cpp/include/tensorrt_llm/executor/executor.h
  • tensorrt_llm/serve/openai_server.py
  • cpp/tensorrt_llm/batch_manager/evictionPolicy.cpp
  • cpp/tensorrt_llm/executor/serialization.cpp
  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
  • cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp

Comment thread cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h Outdated
Comment thread cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
Comment thread cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
Comment thread tensorrt_llm/_torch/pyexecutor/py_executor.py
Comment thread tests/unittest/llmapi/test_disk_tier_args.py Outdated
@nvpohanh

nvpohanh commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

[by Codex] @thorjohnsen Could you please review PR #16252 for the KV-cache manager changes? Thanks!

@thorjohnsen thorjohnsen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume onboarding from disk takes a long time, during which the GPUs sit idle. Wouldn't it make more sense to onboard via host from disk, disk -> host -> gpu? Scheduler sees the next sequence to be launched has reusable blocks on disk and start prefetching those to host memory, sequence only launches after prefetching has finished.

@nafis271

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look @thorjohnsen ! The onboard is asynchronous, so the GPU does not actually sit idle during a disk read as the other requests keep being processed, so the GPU stays busy. For the transfer path, the async POSIX path stages disk - > pinned host - > GPU and the GDS path does a direct write to GPU skipping the host (though i have only verified GDS functionally, not benchmarked its throughput). I did think about the prefetching idea and I think it could be a followup to this pr.

@thorjohnsen

Copy link
Copy Markdown
Collaborator

Thanks, I see that the scheduler does not wait for onboarding from disk, the only real cost is that the GPU target blocks are held until the request can start. This will reduce amount of available KV cache slightly, but I agree it can be postponed to a follow-up PR.

/// @brief Get the amount of free blocks in the primary memory pool
virtual SizeType32 getNumFreeBlocks(SizeType32 cacheLevel) = 0;

/// @brief True when the block currently sits in a free queue (evictable right now).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nafis271 Could you check if you can switch to kvCacheManagerV2 which already has this support? If you want more information about kvCacheManagerV2, please let us know. Thanks!

We are planning to migrate from V1 to V2 very soon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know! The reason we implemented it on V1 is that when we wanted this explicit user-provided TTL cache at DeepInfra, most of our models ran on V1 and we could not migrate due to performance and compatibility issues with V2. But I understand it makes sense to move to V2 if that is the long term plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Extend the v1 KV cache manager with a disk (L3) tier and per-request retention

4 participants