From 5a29d8174a41c810bd4a6e7752beeb8518667291 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 11:51:01 -0500 Subject: [PATCH 01/13] fix(kv_cache): eliminate dangling reference race in KVCacheManager::getSequence Store GenerationRequest as shared_ptr in mSequences so object lifetime is decoupled from map membership. getSequence() now returns shared_ptr instead of a raw reference, which was released from mSequencesMtx before being returned to callers -- leaving every caller exposed to use-after-free if a concurrent removeSequence() extracted the node. Key changes: - mSequences: unordered_map<..., GenerationRequest> -> unordered_map<..., shared_ptr> - getSequence(): two non-const/const overloads returning raw refs -> single const overload returning shared_ptr (nullptr on miss) - addSequence(): construct shared_ptr before emplace; use local ptr after lock release, eliminating iterator-invalidation window - removeSequence(): erase under lock, return ptr; object destroyed only after releaseBlocks completes - storeContextBlocks(): remove TOCTOU double-lock; single getSequence call with null check replaces check-then-act across two lock scopes - rewindKVCache(): hold shared_ptr for loop duration so concurrent removeSequence cannot destroy the object mid-loop - removeToken(): null-guard on getSequence result; no-op if already gone - All other callers (addToken, storeNewBlock, storeBlocksForReuse, pinBlocks, copyBlockOffsets, getNeededBlocksOneStep, getCacheBlockIds, getBatchCacheBlockIds, getLastBlockId): TLLM_CHECK_WITH_INFO on result - getRemainingBlocksToCompletion: direct mSequences access updated to dereference shared_ptr value - Tests: getSequence call sites updated to dereference returned ptr Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../batch_manager/kvCacheManager.h | 8 +- .../batch_manager/kvCacheManager.cpp | 114 +++++++++--------- .../batch_manager/kvCacheManagerTest.cpp | 28 ++--- 3 files changed, 76 insertions(+), 74 deletions(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 95b161c4993c..68a9288a9694 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -1610,8 +1610,7 @@ class BaseKVCacheManager // void removeToken(SizeType32 seqSlotIdx); virtual void rewindKVCache(LlmRequest::RequestIdType requestId, SizeType32 rewindLengths) = 0; - [[nodiscard]] virtual GenerationRequest const& getSequence(LlmRequest::RequestIdType requestId) const = 0; - [[nodiscard]] virtual GenerationRequest& getSequence(LlmRequest::RequestIdType requestId) = 0; + [[nodiscard]] virtual std::shared_ptr getSequence(LlmRequest::RequestIdType requestId) const = 0; [[nodiscard]] virtual bool isCrossKv() const = 0; @@ -1986,8 +1985,7 @@ class KVCacheManager : public BaseKVCacheManager void removeToken(LlmRequest::RequestIdType requestId); void rewindKVCache(LlmRequest::RequestIdType requestId, SizeType32 rewindLengths) override; - [[nodiscard]] GenerationRequest const& getSequence(LlmRequest::RequestIdType requestId) const override; - [[nodiscard]] GenerationRequest& getSequence(LlmRequest::RequestIdType requestId) override; + [[nodiscard]] std::shared_ptr getSequence(LlmRequest::RequestIdType requestId) const override; [[nodiscard]] bool isCrossKv() const override { @@ -2124,7 +2122,7 @@ class KVCacheManager : public BaseKVCacheManager // Block manager BlockManager mBlockManager; // Map of all sequences - std::unordered_map mSequences; + std::unordered_map> mSequences; // Whether to cache KV pages for reuse bool mEnableBlockReuse; // Mutex to protect access to mSequences diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index ff79e4eb3a50..ddce9f0ad60c 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2270,7 +2270,9 @@ SizeType32 KVCacheManager::getNeededBlocksOneStep( return 0; } - auto const numCurrTokens = getSequence(req.mRequestId).getNumTokens(); + auto const seqPtr_ = getSequence(req.mRequestId); + TLLM_CHECK_WITH_INFO(seqPtr_, "No sequence found for request %lu", req.mRequestId); + auto const numCurrTokens = seqPtr_->getNumTokens(); auto const generatedTokens = numCurrTokens - req.getPromptLen(); auto const maxTokensToAddToKVCache = req.mMaxNewTokens - generatedTokens; auto const tokensPerStep = req.getNumDraftTokens() + 1; @@ -2322,7 +2324,7 @@ SizeType32 KVCacheManager::getRemainingBlocksToCompletion(LlmRequest const& req, if (seqIt != mSequences.end()) { auto const& seq = seqIt->second; - numAllocBlocksPerBeam = seq.getCacheBlockIds(windowSize).at(0).size(); + numAllocBlocksPerBeam = seq->getCacheBlockIds(windowSize).at(0).size(); } } @@ -2426,7 +2428,9 @@ void BlockManager::updateCacheBlockOffsetsAtIdx(GenerationRequest& sequence, Siz void KVCacheManager::addToken(RequestIdType requestId) { // TODO: add streamLLM support - auto& sequence = getSequence(requestId); + auto seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + auto& sequence = *seqPtr; sequence.addNewTokens(1); mBlockManager.adjustBlocksIfNeeded(sequence); } @@ -2488,19 +2492,18 @@ bool KVCacheManager::addSequence( ? llmRequest->getKvCacheRetentionConfig().value_or(executor::KvCacheRetentionConfig()) : executor::KvCacheRetentionConfig(); - auto const [seqIt, emplaceDone] = [&] + auto seq = std::make_shared(requestId, inputLength, beamWidth, + mBlockManager.getWindowSizesMetadata(), std::move(kvCacheRetentionConfig)); { auto lck = std::scoped_lock(mSequencesMtx); - return mSequences.try_emplace(requestId, requestId, inputLength, beamWidth, - mBlockManager.getWindowSizesMetadata(), kvCacheRetentionConfig); - }(); - - if (!emplaceDone) - { - return false; + auto const [_, emplaceDone] = mSequences.emplace(requestId, seq); + if (!emplaceDone) + { + return false; + } } - auto& sequence = seqIt->second; + auto& sequence = *seq; // Get statistics for block allocations/reuse pre request. SizeType32 const numAllocTotalBlocksPreRequest = mBlockManager.getNumAllocTotalBlocks(); @@ -2581,18 +2584,12 @@ bool KVCacheManager::addSequence( void KVCacheManager::storeContextBlocks(LlmRequest const& llmRequest) { auto const requestId = llmRequest.mRequestId; - bool found = false; - { - // protect the mSequences - std::scoped_lock lock(mSequencesMtx); - found = mSequences.find(requestId) != mSequences.end(); - } - if (found) + auto seqPtr = getSequence(requestId); + if (seqPtr) { - auto& sequence = getSequence(requestId); if (mEnableBlockReuse && !llmRequest.isDummyRequest()) { - mBlockManager.storeContextBlocks(sequence, llmRequest); + mBlockManager.storeContextBlocks(*seqPtr, llmRequest); } } else @@ -2607,7 +2604,9 @@ void KVCacheManager::storeNewBlock(LlmRequest const& llmRequest) // - Beam search is NOT enabled // - Block reuse is enabled. auto const requestId = llmRequest.mRequestId; - auto& sequence = getSequence(requestId); + auto seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + auto& sequence = *seqPtr; if (sequence.getBeamWidth() > 1 || !mEnableBlockReuse) { return; @@ -2619,21 +2618,25 @@ std::optional KVCacheManager::removeSequence( RequestIdType requestId, OptionalRef llmRequest, bool pinBlocks) { TLLM_LOG_TRACE("[%s]::%s start", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); - auto sequenceNode = [this, requestId] + auto seqPtr = [this, requestId]() -> std::shared_ptr { std::scoped_lock lock(mSequencesMtx); - return mSequences.extract(requestId); + auto it = mSequences.find(requestId); + if (it == mSequences.end()) return nullptr; + auto ptr = it->second; + mSequences.erase(it); + return ptr; }(); std::optional lastStoredId = std::nullopt; - if (!sequenceNode.empty()) + if (seqPtr) { if (mEnableBlockReuse) { - lastStoredId = mBlockManager.releaseBlocks(sequenceNode.mapped(), llmRequest, pinBlocks); + lastStoredId = mBlockManager.releaseBlocks(*seqPtr, llmRequest, pinBlocks); } else { - lastStoredId = mBlockManager.releaseBlocks(sequenceNode.mapped(), std::nullopt, pinBlocks); + lastStoredId = mBlockManager.releaseBlocks(*seqPtr, std::nullopt, pinBlocks); } } if (mBlockManager.isSequenceHeld(requestId)) @@ -2650,7 +2653,9 @@ std::vector KVCacheManager::storeBlocksForReuse( RequestIdType requestId, OptionalRef llmRequest, bool pinBlocks) { TLLM_LOG_TRACE("[%s]::%s start", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); - auto& sequence = getSequence(requestId); + auto seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + auto& sequence = *seqPtr; auto pinnedBlockIds = mBlockManager.storeBlocksForReuse(sequence, llmRequest, pinBlocks); TLLM_LOG_TRACE("[%s]::%s stop", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); return pinnedBlockIds; @@ -2664,8 +2669,9 @@ void KVCacheManager::schedulingRemoveSequence(RequestIdType requestId) void KVCacheManager::pinBlocks(RequestIdType requestId) { - auto& sequence = getSequence(requestId); - mBlockManager.pinBlocks(sequence); + auto seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + mBlockManager.pinBlocks(*seqPtr); } void KVCacheManager::unpinBlocksById(std::vector const& blockIds) @@ -2695,7 +2701,9 @@ tle::RetentionPriority KVCacheManager::getPriorityByBlockId(KVCacheBlock::IdType SizeType32 KVCacheManager::copyBlockOffsets(ITensor& output, SizeType32 outputSlotOffset, RequestIdType requestId) const { - auto const& sequence = getSequence(requestId); + auto const seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + auto const& sequence = *seqPtr; auto const beamWidth = sequence.getBeamWidth(); auto* dstPtr = bufferCast(output); @@ -3008,7 +3016,9 @@ BlocksPerWindow BaseKVCacheManager::calculateMaxNumBlocks(executor::KvCacheConfi void KVCacheManager::removeToken(RequestIdType requestId) { // TODO: add streamLLM support - auto& sequence = getSequence(requestId); + auto seqPtr = getSequence(requestId); + if (!seqPtr) return; + auto& sequence = *seqPtr; if (sequence.getNumTokens() == 0) { return; @@ -3027,16 +3037,12 @@ void KVCacheManager::removeToken(RequestIdType requestId) void KVCacheManager::rewindKVCache(RequestIdType requestId, SizeType32 rewindLengths) { - // Check if the sequence still exists before rewinding - // In overlap mode with MTP, the request may have been terminated and removed - // from mSequences before rewindKVCache is called + // Hold shared_ptr so a concurrent removeSequence cannot destroy the object during rewind. + auto seqPtr = getSequence(requestId); + if (!seqPtr) { - std::scoped_lock lck(mSequencesMtx); - if (mSequences.find(requestId) == mSequences.end()) - { - TLLM_LOG_DEBUG("Request %lu has already been removed from KV cache manager, skipping rewind", requestId); - return; - } + TLLM_LOG_DEBUG("Request %lu has already been removed from KV cache manager, skipping rewind", requestId); + return; } for (SizeType32 si = 0; si < rewindLengths; ++si) @@ -3044,17 +3050,11 @@ void KVCacheManager::rewindKVCache(RequestIdType requestId, SizeType32 rewindLen removeToken(requestId); } } - -GenerationRequest const& KVCacheManager::getSequence(RequestIdType requestId) const -{ - auto lck = std::scoped_lock(mSequencesMtx); - return mSequences.at(requestId); -} - -GenerationRequest& KVCacheManager::getSequence(RequestIdType requestId) +std::shared_ptr KVCacheManager::getSequence(RequestIdType requestId) const { auto lck = std::scoped_lock(mSequencesMtx); - return mSequences.at(requestId); + auto it = mSequences.find(requestId); + return it != mSequences.end() ? it->second : nullptr; } SizeType32 BaseKVCacheManager::getSinkBubbleLength(SizeType32 sinkTokenLen, SizeType32 tokensPerBlock) @@ -3067,7 +3067,9 @@ SizeType32 BaseKVCacheManager::getSinkBubbleLength(SizeType32 sinkTokenLen, Size std::vector> const& KVCacheManager::getCacheBlockIds( RequestIdType requestId, SizeType32 windowSize) const { - return getSequence(requestId).getCacheBlockIds(windowSize); + auto seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + return seqPtr->getCacheBlockIds(windowSize); } std::vector>> KVCacheManager::getBatchCacheBlockIds( @@ -3077,22 +3079,24 @@ std::vector>> KVCacheManager::getBatchCacheB result.reserve(requestIds.size()); for (auto const& requestId : requestIds) { - auto const& sequence = getSequence(requestId); - result.emplace_back(sequence.getCacheBlockIds(windowSize)); + auto const seqPtr = getSequence(requestId); + TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + result.emplace_back(seqPtr->getCacheBlockIds(windowSize)); } return result; } std::optional KVCacheManager::getLastBlockId(LlmRequest::RequestIdType requestId) const { - auto const& seq = getSequence(requestId); + auto const seq = getSequence(requestId); + if (!seq) return std::nullopt; // Use the first window size auto firstWindowSize = mBlockManager.getFirstWindowSize(); if (firstWindowSize == 0) { return std::nullopt; } - auto const& perBeam = seq.getCacheBlockIds(firstWindowSize); + auto const& perBeam = seq->getCacheBlockIds(firstWindowSize); if (perBeam.empty() || perBeam[0].empty()) { return std::nullopt; diff --git a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp index 763cd922f277..b36cfb47b80b 100644 --- a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp +++ b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp @@ -657,7 +657,7 @@ TEST_F(KVCacheManagerTest, FindBlocksInReuseTreeByBlockKeysTest) auto llmRequest0 = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest0); EXPECT_EQ(llmRequest0->getContextCurrentPosition(), 0); - auto cacheBlockIds = kvCacheManager.getSequence(requestId).getCacheBlockIds(maxAttentionWindow).at(beamIdx); + auto cacheBlockIds = kvCacheManager.getSequence(requestId)->getCacheBlockIds(maxAttentionWindow).at(beamIdx); EXPECT_THAT(cacheBlockIds, ::testing::ElementsAreArray({0, 1, 2})); (void) kvCacheManager.removeSequence(requestId, llmRequest0); @@ -2795,7 +2795,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockTest) auto llmRequest1 = std::make_shared(1, maxNewTokens, inputTokens1, samplingConfig, isStreaming); kvCacheManager.addSequence(1, inputLength1, beamWidth, llmRequest1); - GenerationRequest const& seq1 = kvCacheManager.getSequence(1); + auto & seq1 = *kvCacheManager.getSequence(1); EXPECT_EQ(llmRequest1->getContextCurrentPosition(), 0); // Block 1 should NOT be reused. It was not freed even if partial. EXPECT_THAT(seq1.getCacheBlockIds(maxAttentionWindow).at(0), ::testing::ElementsAreArray({2})); @@ -2866,7 +2866,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockWithDependentTest) = std::make_shared(requestId0, maxNewTokens, inputTokens0, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId0, inputLength0, beamWidth, llmRequest0); kvCacheManager.storeContextBlocks(*llmRequest0); - GenerationRequest const& seq0 = kvCacheManager.getSequence(requestId0); + auto & seq0 = *kvCacheManager.getSequence(requestId0); // Add two more blocks of generated tokens for (int i = tokensPerBlock; i < 3 * tokensPerBlock; ++i) @@ -2893,7 +2893,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockWithDependentTest) = std::make_shared(requestId1, maxNewTokens, inputTokens1, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId1, inputLength1, beamWidth, llmRequest1); kvCacheManager.storeContextBlocks(*llmRequest1); - GenerationRequest const& seq1 = kvCacheManager.getSequence(requestId1); + auto & seq1 = *kvCacheManager.getSequence(requestId1); // Verify that all primary blocks are in use EXPECT_EQ(blockManager.getNumFreeBlocks(), 0); @@ -3418,7 +3418,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowSmallerThanBlockSizeT auto constexpr beamIdx = 0; kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq0 = kvCacheManager.getSequence(requestId); + auto & seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); EXPECT_THAT(seq0.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0})); @@ -3688,7 +3688,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) auto constexpr beamIdx = 0; kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq0 = kvCacheManager.getSequence(requestId); + auto & seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); EXPECT_THAT(seq0.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 2, 3})); @@ -3719,7 +3719,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) inputTokens->resize(inputLength); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq1 = kvCacheManager.getSequence(requestId); + auto & seq1 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 6); EXPECT_THAT(seq1.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray( @@ -3742,7 +3742,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq2 = kvCacheManager.getSequence(requestId); + auto & seq2 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 9); EXPECT_THAT(seq2.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 7})); EXPECT_NO_THROW(static_cast(kvCacheManager.removeSequence(requestId, llmRequest))); @@ -3757,7 +3757,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq3 = kvCacheManager.getSequence(requestId); + auto & seq3 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 14); EXPECT_THAT(seq3.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 2, 8})); @@ -3821,7 +3821,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerSWAInvalidateReuseTest) = std::make_shared(/*requestId=*/0, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(/*requestId=*/0, inputLength, beamWidth, llmRequest0); - GenerationRequest const& seq0 = kvCacheManager.getSequence(/*requestId=*/0); + auto & seq0 = *kvCacheManager.getSequence(/*requestId=*/0); // Block 0 goes out-of-window and is detached llmRequest0->addNewToken(1011, beamIdx); @@ -3839,7 +3839,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerSWAInvalidateReuseTest) auto llmRequest1 = std::make_shared(/*requestId=*/1, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(/*requestId=*/1, inputLength, beamWidth, llmRequest1); - GenerationRequest const& seq1 = kvCacheManager.getSequence(/*requestId=*/1); + auto & seq1 = *kvCacheManager.getSequence(/*requestId=*/1); auto const onlyWindowSize = theOnlyWindowSize(kvCacheManager); EXPECT_FALSE(blockManager.isSequenceValidForStoreForReuse(seq0.getRequestId(), onlyWindowSize)); @@ -3920,7 +3920,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerVariableWindowAttentionWithReuseTest) auto llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq0 = kvCacheManager.getSequence(requestId); + auto & seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); assertBlocks(seq0, {0, 1}, {0, 1}); @@ -3952,7 +3952,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerVariableWindowAttentionWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - GenerationRequest const& seq1 = kvCacheManager.getSequence(requestId); + auto & seq1 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 6); assertBlocks(seq1, {0, 3}, {0, 3}); @@ -4137,7 +4137,7 @@ TEST_F(KVCacheManagerTest, GetPriorityByBlockId) kvCacheManager.storeContextBlocks(*llmRequest); // Get block IDs for the sequence - auto const& seq = kvCacheManager.getSequence(0); + auto & seq = *kvCacheManager.getSequence(0); auto cacheBlockIds = seq.getCacheBlockIds(maxAttentionWindow).at(0); ASSERT_GE(cacheBlockIds.size(), 1); From c3a9589ba8142c8b3644a5f6e6a2528cb50d3162 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 15:50:19 -0500 Subject: [PATCH 02/13] fix(kv_cache): update kvCacheUtils.h getSequence dot-chain to arrow Missed call site from the shared_ptr getSequence refactor. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/include/tensorrt_llm/batch_manager/kvCacheUtils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheUtils.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheUtils.h index 90416d124c3a..2c94e556bce4 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheUtils.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheUtils.h @@ -200,7 +200,7 @@ class BlockRange auto windowSize = cacheManager.getBlockManager().getPoolWindowSize(poolIdx); mPoolsPerWindow[windowSize].push_back(cacheManager.getBlockManager().getPrimaryPool(poolIdx)); mBlockIdsPerWindow[windowSize] - = cacheManager.getSequence(mRequestId).getCacheBlockIds(windowSize).at(kFIRST_AND_ONLY_BEAM); + = cacheManager.getSequence(mRequestId)->getCacheBlockIds(windowSize).at(kFIRST_AND_ONLY_BEAM); } if (cacheManager.isEnableIndexerKCache()) { From 177bfc2ccf3add4b8819b8cf833fe65097563639 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:16:40 -0500 Subject: [PATCH 03/13] fix(kv-cache): add per-sequence mutex to eliminate removeToken/removeSequence race Add `mutable std::mutex mMutex` and `bool mRemoved{false}` to `GenerationRequest`. `removeSequence` acquires the per-sequence lock and sets `mRemoved = true` before calling `releaseBlocks`, ensuring any concurrent mutator (`addToken`, `removeToken`, `storeNewBlock`, `storeBlocksForReuse`, `pinBlocks`) that already holds the lock will finish cleanly before blocks are freed, and any mutator that acquires the lock afterward will see `mRemoved` and exit without touching freed state. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../tensorrt_llm/batch_manager/kvCacheManager.h | 7 +++++++ cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 68a9288a9694..5f48ac5599b1 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -520,6 +521,12 @@ class GenerationRequest std::set mUsedBlocks; // Current prepopulated prompt length SizeType32 mCurrentPrepopulatedPromptLen; + // Serialises per-sequence mutations (addToken, removeToken, storeNewBlock, etc.) + // against removeSequence to prevent use of freed blocks. + mutable std::mutex mMutex; + // Set by removeSequence under mMutex before releasing blocks; checked by all + // mutating callers so they can exit cleanly rather than touch freed state. + bool mRemoved{false}; }; // attach metadata to a pool pointer diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index ddce9f0ad60c..c6b478c960b1 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2429,7 +2429,9 @@ void KVCacheManager::addToken(RequestIdType requestId) { // TODO: add streamLLM support auto seqPtr = getSequence(requestId); - TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + if (!seqPtr) return; + std::scoped_lock seqLock(seqPtr->mMutex); + if (seqPtr->mRemoved) return; auto& sequence = *seqPtr; sequence.addNewTokens(1); mBlockManager.adjustBlocksIfNeeded(sequence); @@ -2606,6 +2608,8 @@ void KVCacheManager::storeNewBlock(LlmRequest const& llmRequest) auto const requestId = llmRequest.mRequestId; auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + std::scoped_lock seqLock(seqPtr->mMutex); + if (seqPtr->mRemoved) return; auto& sequence = *seqPtr; if (sequence.getBeamWidth() > 1 || !mEnableBlockReuse) { @@ -2630,6 +2634,8 @@ std::optional KVCacheManager::removeSequence( std::optional lastStoredId = std::nullopt; if (seqPtr) { + std::scoped_lock seqLock(seqPtr->mMutex); + seqPtr->mRemoved = true; if (mEnableBlockReuse) { lastStoredId = mBlockManager.releaseBlocks(*seqPtr, llmRequest, pinBlocks); @@ -2655,6 +2661,8 @@ std::vector KVCacheManager::storeBlocksForReuse( TLLM_LOG_TRACE("[%s]::%s start", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + std::scoped_lock seqLock(seqPtr->mMutex); + if (seqPtr->mRemoved) return {}; auto& sequence = *seqPtr; auto pinnedBlockIds = mBlockManager.storeBlocksForReuse(sequence, llmRequest, pinBlocks); TLLM_LOG_TRACE("[%s]::%s stop", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); @@ -2671,6 +2679,8 @@ void KVCacheManager::pinBlocks(RequestIdType requestId) { auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); + std::scoped_lock seqLock(seqPtr->mMutex); + if (seqPtr->mRemoved) return; mBlockManager.pinBlocks(*seqPtr); } @@ -3018,6 +3028,8 @@ void KVCacheManager::removeToken(RequestIdType requestId) // TODO: add streamLLM support auto seqPtr = getSequence(requestId); if (!seqPtr) return; + std::scoped_lock seqLock(seqPtr->mMutex); + if (seqPtr->mRemoved) return; auto& sequence = *seqPtr; if (sequence.getNumTokens() == 0) { From 13aa0e1a8b6ab7117bfc4057cc1b8297f917f233 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:26:43 -0500 Subject: [PATCH 04/13] style: add braces to all single-line if bodies in modified code Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../batch_manager/kvCacheManager.cpp | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index c6b478c960b1..5d602f824c62 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2429,9 +2429,15 @@ void KVCacheManager::addToken(RequestIdType requestId) { // TODO: add streamLLM support auto seqPtr = getSequence(requestId); - if (!seqPtr) return; + if (!seqPtr) + { + return; + } std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) return; + if (seqPtr->mRemoved) + { + return; + } auto& sequence = *seqPtr; sequence.addNewTokens(1); mBlockManager.adjustBlocksIfNeeded(sequence); @@ -2609,7 +2615,10 @@ void KVCacheManager::storeNewBlock(LlmRequest const& llmRequest) auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) return; + if (seqPtr->mRemoved) + { + return; + } auto& sequence = *seqPtr; if (sequence.getBeamWidth() > 1 || !mEnableBlockReuse) { @@ -2626,7 +2635,10 @@ std::optional KVCacheManager::removeSequence( { std::scoped_lock lock(mSequencesMtx); auto it = mSequences.find(requestId); - if (it == mSequences.end()) return nullptr; + if (it == mSequences.end()) + { + return nullptr; + } auto ptr = it->second; mSequences.erase(it); return ptr; @@ -2662,7 +2674,10 @@ std::vector KVCacheManager::storeBlocksForReuse( auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) return {}; + if (seqPtr->mRemoved) + { + return {}; + } auto& sequence = *seqPtr; auto pinnedBlockIds = mBlockManager.storeBlocksForReuse(sequence, llmRequest, pinBlocks); TLLM_LOG_TRACE("[%s]::%s stop", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); @@ -2680,7 +2695,10 @@ void KVCacheManager::pinBlocks(RequestIdType requestId) auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) return; + if (seqPtr->mRemoved) + { + return; + } mBlockManager.pinBlocks(*seqPtr); } @@ -3027,9 +3045,15 @@ void KVCacheManager::removeToken(RequestIdType requestId) { // TODO: add streamLLM support auto seqPtr = getSequence(requestId); - if (!seqPtr) return; + if (!seqPtr) + { + return; + } std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) return; + if (seqPtr->mRemoved) + { + return; + } auto& sequence = *seqPtr; if (sequence.getNumTokens() == 0) { @@ -3101,7 +3125,10 @@ std::vector>> KVCacheManager::getBatchCacheB std::optional KVCacheManager::getLastBlockId(LlmRequest::RequestIdType requestId) const { auto const seq = getSequence(requestId); - if (!seq) return std::nullopt; + if (!seq) + { + return std::nullopt; + } // Use the first window size auto firstWindowSize = mBlockManager.getFirstWindowSize(); if (firstWindowSize == 0) From 11acc93444680c03ff666632027c32e5092e787f Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 22:49:15 +0000 Subject: [PATCH 05/13] precommit run Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../batch_manager/kvCacheManager.cpp | 5 ++-- .../batch_manager/kvCacheManagerTest.cpp | 26 +++++++++---------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index 9e02790313bb..587062c73fe7 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2500,8 +2500,8 @@ void KVCacheManager::addSequence( ? llmRequest->getKvCacheRetentionConfig().value_or(executor::KvCacheRetentionConfig()) : executor::KvCacheRetentionConfig(); - auto seq = std::make_shared(requestId, inputLength, beamWidth, - mBlockManager.getWindowSizesMetadata(), std::move(kvCacheRetentionConfig)); + auto seq = std::make_shared( + requestId, inputLength, beamWidth, mBlockManager.getWindowSizesMetadata(), std::move(kvCacheRetentionConfig)); { auto lck = std::scoped_lock(mSequencesMtx); auto const [_, emplaceDone] = mSequences.emplace(requestId, seq); @@ -3084,6 +3084,7 @@ void KVCacheManager::rewindKVCache(RequestIdType requestId, SizeType32 rewindLen removeToken(requestId); } } + std::shared_ptr KVCacheManager::getSequence(RequestIdType requestId) const { auto lck = std::scoped_lock(mSequencesMtx); diff --git a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp index 94a4db8f37c7..81f8efc7e590 100644 --- a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp +++ b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp @@ -2795,7 +2795,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockTest) auto llmRequest1 = std::make_shared(1, maxNewTokens, inputTokens1, samplingConfig, isStreaming); kvCacheManager.addSequence(1, inputLength1, beamWidth, llmRequest1); - auto & seq1 = *kvCacheManager.getSequence(1); + auto& seq1 = *kvCacheManager.getSequence(1); EXPECT_EQ(llmRequest1->getContextCurrentPosition(), 0); // Block 1 should NOT be reused. It was not freed even if partial. EXPECT_THAT(seq1.getCacheBlockIds(maxAttentionWindow).at(0), ::testing::ElementsAreArray({2})); @@ -2866,7 +2866,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockWithDependentTest) = std::make_shared(requestId0, maxNewTokens, inputTokens0, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId0, inputLength0, beamWidth, llmRequest0); kvCacheManager.storeContextBlocks(*llmRequest0); - auto & seq0 = *kvCacheManager.getSequence(requestId0); + auto& seq0 = *kvCacheManager.getSequence(requestId0); // Add two more blocks of generated tokens for (int i = tokensPerBlock; i < 3 * tokensPerBlock; ++i) @@ -2893,7 +2893,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerLeafBlockWithDependentTest) = std::make_shared(requestId1, maxNewTokens, inputTokens1, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId1, inputLength1, beamWidth, llmRequest1); kvCacheManager.storeContextBlocks(*llmRequest1); - auto & seq1 = *kvCacheManager.getSequence(requestId1); + auto& seq1 = *kvCacheManager.getSequence(requestId1); // Verify that all primary blocks are in use EXPECT_EQ(blockManager.getNumFreeBlocks(), 0); @@ -3418,7 +3418,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowSmallerThanBlockSizeT auto constexpr beamIdx = 0; kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq0 = *kvCacheManager.getSequence(requestId); + auto& seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); EXPECT_THAT(seq0.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0})); @@ -3688,7 +3688,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) auto constexpr beamIdx = 0; kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq0 = *kvCacheManager.getSequence(requestId); + auto& seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); EXPECT_THAT(seq0.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 2, 3})); @@ -3719,7 +3719,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) inputTokens->resize(inputLength); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq1 = *kvCacheManager.getSequence(requestId); + auto& seq1 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 6); EXPECT_THAT(seq1.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray( @@ -3742,7 +3742,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq2 = *kvCacheManager.getSequence(requestId); + auto& seq2 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 9); EXPECT_THAT(seq2.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 7})); EXPECT_NO_THROW(static_cast(kvCacheManager.removeSequence(requestId, llmRequest))); @@ -3757,7 +3757,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerMaxAttentionWindowWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq3 = *kvCacheManager.getSequence(requestId); + auto& seq3 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 14); EXPECT_THAT(seq3.getCacheBlockIds(onlyWindowSize).at(beamIdx), ::testing::ElementsAreArray({0, 1, 2, 8})); @@ -3821,7 +3821,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerSWAInvalidateReuseTest) = std::make_shared(/*requestId=*/0, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(/*requestId=*/0, inputLength, beamWidth, llmRequest0); - auto & seq0 = *kvCacheManager.getSequence(/*requestId=*/0); + auto& seq0 = *kvCacheManager.getSequence(/*requestId=*/0); // Block 0 goes out-of-window and is detached llmRequest0->addNewToken(1011, beamIdx); @@ -3839,7 +3839,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerSWAInvalidateReuseTest) auto llmRequest1 = std::make_shared(/*requestId=*/1, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(/*requestId=*/1, inputLength, beamWidth, llmRequest1); - auto & seq1 = *kvCacheManager.getSequence(/*requestId=*/1); + auto& seq1 = *kvCacheManager.getSequence(/*requestId=*/1); auto const onlyWindowSize = theOnlyWindowSize(kvCacheManager); EXPECT_FALSE(blockManager.isSequenceValidForStoreForReuse(seq0.getRequestId(), onlyWindowSize)); @@ -3920,7 +3920,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerVariableWindowAttentionWithReuseTest) auto llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq0 = *kvCacheManager.getSequence(requestId); + auto& seq0 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 0); assertBlocks(seq0, {0, 1}, {0, 1}); @@ -3952,7 +3952,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerVariableWindowAttentionWithReuseTest) std::iota(inputTokens->begin(), inputTokens->end(), firstToken); llmRequest = std::make_shared(requestId, maxNewTokens, inputTokens, samplingConfig, isStreaming); kvCacheManager.addSequence(requestId, inputLength, beamWidth, llmRequest); - auto & seq1 = *kvCacheManager.getSequence(requestId); + auto& seq1 = *kvCacheManager.getSequence(requestId); EXPECT_EQ(llmRequest->getContextCurrentPosition(), 6); assertBlocks(seq1, {0, 3}, {0, 3}); @@ -4137,7 +4137,7 @@ TEST_F(KVCacheManagerTest, GetPriorityByBlockId) kvCacheManager.storeContextBlocks(*llmRequest); // Get block IDs for the sequence - auto & seq = *kvCacheManager.getSequence(0); + auto& seq = *kvCacheManager.getSequence(0); auto cacheBlockIds = seq.getCacheBlockIds(maxAttentionWindow).at(0); ASSERT_GE(cacheBlockIds.size(), 1); From e4773f4f36a3e479a0af9cb4eef45d6ed021b800 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 19:28:15 -0500 Subject: [PATCH 06/13] fix: expose GenerationRequest mutex/removed state via public API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mMutex and mRemoved were private but accessed directly from KVCacheManager, which does not compile. Replace with three narrow public methods: - getLock() – acquires mMutex, returns moveable unique_lock - isRemoved() – read accessor for mRemoved - markRemoved() – called by removeSequence under the lock Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../batch_manager/kvCacheManager.h | 12 ++++++++++ .../batch_manager/kvCacheManager.cpp | 24 +++++++++---------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 4485d92091d8..0a64faa604e8 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -527,6 +527,18 @@ class GenerationRequest // Set by removeSequence under mMutex before releasing blocks; checked by all // mutating callers so they can exit cleanly rather than touch freed state. bool mRemoved{false}; + +public: + // Acquires mMutex; the returned unique_lock is moveable so callers hold RAII ownership. + [[nodiscard]] std::unique_lock getLock() const + { + return std::unique_lock(mMutex); + } + + bool isRemoved() const noexcept { return mRemoved; } + + // Called by KVCacheManager::removeSequence while holding the lock. + void markRemoved() noexcept { mRemoved = true; } }; // attach metadata to a pool pointer diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index 587062c73fe7..ec00d31cc617 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2433,8 +2433,8 @@ void KVCacheManager::addToken(RequestIdType requestId) { return; } - std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) { return; } @@ -2612,8 +2612,8 @@ void KVCacheManager::storeNewBlock(LlmRequest const& llmRequest) auto const requestId = llmRequest.mRequestId; auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); - std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) { return; } @@ -2644,8 +2644,8 @@ std::optional KVCacheManager::removeSequence( std::optional lastStoredId = std::nullopt; if (seqPtr) { - std::scoped_lock seqLock(seqPtr->mMutex); - seqPtr->mRemoved = true; + auto seqLock = seqPtr->getLock(); + seqPtr->markRemoved(); if (mEnableBlockReuse) { lastStoredId = mBlockManager.releaseBlocks(*seqPtr, llmRequest, pinBlocks); @@ -2671,8 +2671,8 @@ std::vector KVCacheManager::storeBlocksForReuse( TLLM_LOG_TRACE("[%s]::%s start", isCrossKv() ? "CROSS" : "SELF", __PRETTY_FUNCTION__); auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); - std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) { return {}; } @@ -2692,8 +2692,8 @@ void KVCacheManager::pinBlocks(RequestIdType requestId) { auto seqPtr = getSequence(requestId); TLLM_CHECK_WITH_INFO(seqPtr, "No sequence found for request %lu", requestId); - std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) { return; } @@ -3047,8 +3047,8 @@ void KVCacheManager::removeToken(RequestIdType requestId) { return; } - std::scoped_lock seqLock(seqPtr->mMutex); - if (seqPtr->mRemoved) + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) { return; } From 11dfe67be6ba138ebea363970000f773ad540f7d Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 16 Mar 2026 19:45:44 -0500 Subject: [PATCH 07/13] fix: update nanobind trampoline getSequence return type to shared_ptr PyKvCacheManager::getSequence still declared GenerationRequest const&, which no longer matches the pure-virtual base after the shared_ptr refactor, causing a compile error. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp index ad70994cc21c..ae275f6ee871 100644 --- a/cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp @@ -130,7 +130,7 @@ class PyKvCacheManager : public tbk::BaseKVCacheManager NB_OVERRIDE_PURE(storeBlocksForReuse, requestId, llmRequest, pinBlocks); } - tbk::GenerationRequest const& getSequence(tb::LlmRequest::RequestIdType requestId) const override + std::shared_ptr getSequence(tb::LlmRequest::RequestIdType requestId) const override { NB_OVERRIDE_PURE(getSequence, requestId); } From 4b1f83d901909ef163da137b222de13bc415be3f Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Tue, 17 Mar 2026 03:04:37 +0000 Subject: [PATCH 08/13] Fix syntax error Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index ec00d31cc617..7aa65a8d5c87 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -2507,7 +2507,7 @@ void KVCacheManager::addSequence( auto const [_, emplaceDone] = mSequences.emplace(requestId, seq); if (!emplaceDone) { - return false; + return; } } From 350b6e1222eb35dab880164a8b60598d6a76668a Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Tue, 17 Mar 2026 10:55:00 -0500 Subject: [PATCH 09/13] fix(test): pass GenerationRequest by const ref in assertBlocks lambda Adding mutable std::mutex to GenerationRequest deletes the copy constructor. assertBlocks took it by value, causing a compile error. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp index 81f8efc7e590..f5fca576261e 100644 --- a/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp +++ b/cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp @@ -3902,7 +3902,7 @@ TEST_F(KVCacheManagerTest, KVCacheManagerVariableWindowAttentionWithReuseTest) auto constexpr beamIdx = 0; auto const assertBlocks - = [minAttentionWindow, maxAttentionWindow, beamIdx](GenerationRequest seq, + = [minAttentionWindow, maxAttentionWindow, beamIdx](GenerationRequest const& seq, std::initializer_list expectedBlocksMin, std::initializer_list expectedBlocksMax) { auto blocksMin = seq.getCacheBlockIds(minAttentionWindow).at(beamIdx); From d39d5950f574129f64f41b09e396c7c693e6b3c1 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Tue, 17 Mar 2026 18:35:07 +0000 Subject: [PATCH 10/13] precommit run Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../tensorrt_llm/batch_manager/kvCacheManager.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 0a64faa604e8..56c564227d9b 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -535,10 +535,16 @@ class GenerationRequest return std::unique_lock(mMutex); } - bool isRemoved() const noexcept { return mRemoved; } + bool isRemoved() const noexcept + { + return mRemoved; + } // Called by KVCacheManager::removeSequence while holding the lock. - void markRemoved() noexcept { mRemoved = true; } + void markRemoved() noexcept + { + mRemoved = true; + } }; // attach metadata to a pool pointer From 782ddc36199dd1ef700fed47c9c0c075dfb9089c Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Tue, 17 Mar 2026 15:08:50 -0500 Subject: [PATCH 11/13] fix(kv-cache): hold per-sequence lock for full rewindKVCache loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rewindKVCache called removeToken once per iteration, releasing and re-acquiring the per-sequence lock each time. removeSequence could slip in between iterations, set mRemoved=true, and cause all subsequent removeToken calls to exit early — leaving the cache partially rewound with no error reported. Fix: extract removeTokenLocked(GenerationRequest&) which assumes the caller already holds the per-sequence lock, then have rewindKVCache acquire the lock once and call removeTokenLocked in the loop. removeToken continues to acquire the lock itself via the existing path. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- .../batch_manager/kvCacheManager.h | 3 ++ .../batch_manager/kvCacheManager.cpp | 42 ++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 56c564227d9b..79951f83b820 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -2127,6 +2127,9 @@ class KVCacheManager : public BaseKVCacheManager SizeType32 sinkTokenLength, SizeType32 blockCapacity, SizeType32 beamWidth, SizeType32 tokensPerBlock); private: + // Removes one token from sequence; caller must hold seqPtr->getLock(). + void removeTokenLocked(GenerationRequest& sequence); + // Maximum number of sequences SizeType32 mMaxNumSequences; // Maximum beam width diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index 7aa65a8d5c87..797a168609a1 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -3039,20 +3039,8 @@ BlocksPerWindow BaseKVCacheManager::calculateMaxNumBlocks(executor::KvCacheConfi return windowSizeToBlocks; } -void KVCacheManager::removeToken(RequestIdType requestId) +void KVCacheManager::removeTokenLocked(GenerationRequest& sequence) { - // TODO: add streamLLM support - auto seqPtr = getSequence(requestId); - if (!seqPtr) - { - return; - } - auto seqLock = seqPtr->getLock(); - if (seqPtr->isRemoved()) - { - return; - } - auto& sequence = *seqPtr; if (sequence.getNumTokens() == 0) { return; @@ -3069,22 +3057,44 @@ void KVCacheManager::removeToken(RequestIdType requestId) } } +void KVCacheManager::removeToken(RequestIdType requestId) +{ + // TODO: add streamLLM support + auto seqPtr = getSequence(requestId); + if (!seqPtr) + { + return; + } + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) + { + return; + } + removeTokenLocked(*seqPtr); +} + void KVCacheManager::rewindKVCache(RequestIdType requestId, SizeType32 rewindLengths) { - // Hold shared_ptr so a concurrent removeSequence cannot destroy the object during rewind. + // Hold the per-sequence lock for the entire loop so removeSequence cannot + // slip in between iterations and cause a partial rewind. auto seqPtr = getSequence(requestId); if (!seqPtr) { TLLM_LOG_DEBUG("Request %lu has already been removed from KV cache manager, skipping rewind", requestId); return; } - + auto seqLock = seqPtr->getLock(); + if (seqPtr->isRemoved()) + { + return; + } for (SizeType32 si = 0; si < rewindLengths; ++si) { - removeToken(requestId); + removeTokenLocked(*seqPtr); } } + std::shared_ptr KVCacheManager::getSequence(RequestIdType requestId) const { auto lck = std::scoped_lock(mSequencesMtx); From 5b3ce023e69c45df6c35bc5b66cefa9059ec2ac9 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Tue, 17 Mar 2026 20:17:25 +0000 Subject: [PATCH 12/13] precommit run Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index 797a168609a1..7fab1320041b 100644 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -3094,7 +3094,6 @@ void KVCacheManager::rewindKVCache(RequestIdType requestId, SizeType32 rewindLen } } - std::shared_ptr KVCacheManager::getSequence(RequestIdType requestId) const { auto lck = std::scoped_lock(mSequencesMtx); From 5753193c8688c94a684de5535fdee09f04347dc0 Mon Sep 17 00:00:00 2001 From: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> Date: Mon, 23 Mar 2026 11:09:53 -0500 Subject: [PATCH 13/13] refactor: remove dead mFreeBlockIterator member from KVCacheBlock The member was initialised to nullopt and never read or written after construction. Iterator tracking moved to EvictionPolicy::mFreeBlockIterators (a vector indexed by block ID) at some earlier point, leaving this field as dead code. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: thorjohnsen <41591019+thorjohnsen@users.noreply.github.com> --- cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h | 2 -- cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp | 1 - 2 files changed, 3 deletions(-) diff --git a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h index 79951f83b820..a2a367dc3c42 100644 --- a/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h +++ b/cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h @@ -338,8 +338,6 @@ class KVCacheBlock : public std::enable_shared_from_this // Distinct from getPrevBlock() (which navigates the radix lookup tree) BlockPtr mPrevBlockInSeq; - // Iterator pointing to this block in mFreeBlocks. - std::optional mFreeBlockIterator; // Flag indicating if block is full bool mIsFull; diff --git a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp index 4e8c17b4273d..c7693fd418a7 100755 --- a/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp +++ b/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp @@ -99,7 +99,6 @@ KVCacheBlock::KVCacheBlock(IdType blockId, tk::KVCacheIndex blockIdx) , mWindowSize{std::numeric_limits::max()} // sentinel: unattached; valid sizes are >= 1 or kRecurrentStates (-1) , mIsPlaceholder{false} - , mFreeBlockIterator(std::nullopt) , mIsFull{false} , mPriority{executor::KvCacheRetentionConfig::kDefaultRetentionPriority} , mDurationMs{std::nullopt}