[https://nvbugs/6418912][fix] Clear detached KV cache sequence links

[chienchunhung]

Summary

• clear stale mPrevBlockInSeq ownership links when detached KV-cache blocks are released
• retain predecessor links for blocks that remain attached to the reuse tree
• add a focused assertion that released detached blocks no longer retain their prior sequence chain

Root cause

PR #11685 began returning detached blocks to the front of the free queue. Those blocks still retained strong mPrevBlockInSeq links. Reusing a subset of each completed request's blocks could therefore join otherwise completed request chains; the large FillKvCacheWithRequestsAndCompleteOneByOne/5 fixture eventually tears down an unbounded shared-ownership chain and segfaults.

Detached blocks have no reusable hash chain, so their predecessor link is obsolete once their reference count reaches zero. Reusable radix-tree blocks are unchanged.

Validation

Local:

• pre-commit on both modified C++ files
• git diff --check
• focused invariant assertion that released detached blocks no longer retain their old predecessor

Targeted CI through combined integration PR #16019:

• exact integration head: 1248a1862d69a3ef8c2e9bca0a2207f383ba31bd
• CI invocation
• PR_Github #57931 → L0_MergeRequest_PR #46619: SUCCESS
• only selected stage: A30-CPP-1
• only selected pytest: cpp/test_unit_tests.py::test_unit_tests[batch_manager-80]
• pytest result: 1 passed, 16,431 deselected, 9 warnings in 644.08s
• batch-manager gtest report: 525 tests, 0 failures (6 disabled, 2 skipped)
• formerly crashing FillKvCacheAndCompleteRequestsTest.FillKvCacheWithRequestsAndCompleteOneByOne/5: OK in 1,739 ms
• no retry, segfault, timeout, or test waiver

Regression evidence

The preceding integration run, pipeline #46583 on head 656e9e6, reproduced the exact /5 segfault twice (initial run and CTest rerun). All other concrete gtests in batch_manager-80, including the nine partial-copy variants, passed. The only code delta in the successful integration rerun is this fix.

Summary by CodeRabbit

• Bug Fixes
  • Fixed block recycling so detached blocks no longer keep stale sequence links when returned to reuse.
  • Improved block cleanup to better prevent incorrect ownership/back-pointer relationships.
  • Added test coverage to verify occupied blocks do not retain a previous-block link in the non-shared case.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modifies WindowBlockManager::releaseBlocks to clear prevBlockInSeq when a block is detached during release, preventing stale hash/back-pointer linkage on reused blocks. Adds a test assertion verifying getPrevBlockInSeq() is nullptr for occupied blocks in a non-shared scenario.

Changes

Detached Block Pointer Fix

Layer / File(s)	Summary
Clear stale prevBlockInSeq on detached block release cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp	releaseBlocks now clears prevBlockInSeq when block->isDetached() is true, using the computed isDetached value to guard front-queue placement and ownership-chain cleanup.
Test validation for detached block pointer cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	Adds a loop asserting getPrevBlockInSeq() returns nullptr for all occupied blocks in the last-block-not-shared test case.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Related PRs: None specified in the provided context.

Suggested reviewers: None specified in the provided context.

🐰 A block let go, its old link cleared,
No stale pointer trailing, no phantom feared,
The test now checks with careful eye,
Each freed block's past — a clean goodbye.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the fix and follows the required ticket/type format.
Description check	✅ Passed	The description covers the issue, fix, validation, and regression evidence in sufficient detail.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp (1)

222-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the attached-block path too.
This only checks the detached case (isEnableBlockReuse=false). Add one reuse-enabled case that keeps a block attached to the reuse tree and asserts getPrevBlockInSeq() stays non-null after releaseBlocks; WindowBlockManager::releaseBlocks only clears the link for detached blocks.

🤖 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 222 -
226, The current test only covers the detached block-reuse path and misses the
attached-block behavior in WindowBlockManager::releaseBlocks. Add a
reuse-enabled case in kvCacheManagerTest.cpp that leaves a block attached to the
reuse tree, calls releaseBlocks, and asserts getPrevBlockInSeq() remains
non-null; keep the existing detached-case assertion for the no-reuse path and
use the same blockManager/getBlockById setup to locate the block under test.

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

Nitpick comments:
In `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`:
- Around line 222-226: The current test only covers the detached block-reuse
path and misses the attached-block behavior in
WindowBlockManager::releaseBlocks. Add a reuse-enabled case in
kvCacheManagerTest.cpp that leaves a block attached to the reuse tree, calls
releaseBlocks, and asserts getPrevBlockInSeq() remains non-null; keep the
existing detached-case assertion for the no-reuse path and use the same
blockManager/getBlockById setup to locate the block under test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cf40e400-ce5e-4823-b27d-f2ba900d6c00

📥 Commits

Reviewing files that changed from the base of the PR and between bd7daec and b26308b.

📒 Files selected for processing (2)

• cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
• cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp

[chienchunhung]

Closing this PR as it is subsumed by #16019