[https://nvbugs/6418912][test] Fix detached-block eviction tests

[chienchunhung]

Summary by CodeRabbit

• Bug Fixes

  • Updated cache manager test coverage to match current block reuse, recycling, and detachment behavior.
  • Refined partial-copy validation, including verification of copied content and warning/output expectations.
  • Adjusted block replace event-stream expectations and attention-window reuse assertions.

• Tests

  • Revised unit tests to validate updated cache block selection/reuse outcomes across multiple scenarios (including partial-reuse and ID-based isolation cases).

Description

PR #11685 changed the KV-cache free queue so detached blocks are recycled before reusable cached blocks. The production behavior is intentional, but 17 tests still encode the previous physical block-allocation order. Once PR #15994 allows the A30 suite to advance past the earlier BufferIndexHolder timeout, those stale block-ID and event-count expectations fail.

Update the affected expectations and comments to match detached-block-first eviction. The nine partial-copy variants also used the same test_copy0 GDS directory from parallel test processes; use a per-process, per-datatype directory to prevent NIXL/GDS registration collisions. Production behavior is unchanged.

This was exposed while validating NVBUG 6418912 / PR #15994, but is independent of the BufferIndexHolder fix.

Affected tests

• KVCacheManagerTest.BlockManagerTestPartialCopyINT64
• KVCacheManagerTest.BlockManagerTestPartialCopyINT32
• KVCacheManagerTest.BlockManagerTestPartialCopyFLOAT
• KVCacheManagerTest.BlockManagerTestPartialCopyBF16
• KVCacheManagerTest.BlockManagerTestPartialCopyHALF
• KVCacheManagerTest.BlockManagerTestPartialCopyBOOL
• KVCacheManagerTest.BlockManagerTestPartialCopyUINT8
• KVCacheManagerTest.BlockManagerTestPartialCopyINT8
• KVCacheManagerTest.BlockManagerTestPartialCopyFP8
• KVCacheManagerTest.BlockManagerReuseTest
• KVCacheManagerTest.BlockManagerReuseWithExtraIdTest
• KVCacheManagerTest.BlockManagerReuseWithMultimodalHashTest
• KVCacheManagerTest.BlockManagerReuseWithLoraTaskIdTest
• KVCacheManagerTest.BlockManagerReuseWithCacheSaltTest
• KVCacheManagerTest.KVCacheManagerLeafBlockTest
• KVCacheManagerTest.KVCacheManagerEventStream
• KVCacheManagerTest.KVCacheManagerMaxAttentionWindowWithReuseTest

Test Coverage

• pre-commit run --files cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
• Combined validation PR [https://nvbugs/6418912][fix] Fix KV-cache lifecycle test failures #16019 included this PR together with [https://nvbugs/6418912][fix] Prevent BufferIndexHolder test deadlock #15994, [https://nvbugs/6418912][test] Fix KV cache reuse accounting tests #16012, and the independent sequence-link fix in [https://nvbugs/6418912][fix] Clear detached KV cache sequence links #16037.
• Targeted A30-CPP-1 pipeline #46619 completed SUCCESS on integration head 1248a186.
• The batch-manager report completed 525 gtests with 0 failures, including all nine partial-copy variants.
• [https://nvbugs/6418912][fix] Clear detached KV cache sequence links #16037 documents the final end-to-end evidence and fixes the separate FillKvCacheWithRequestsAndCompleteOneByOne/5 crash that remained after these expectation fixes.

This combined result validates the changes together; it is not an independent CI run of this PR alone.

[thorjohnsen]

LGTM

[coderabbitai]

📝 Walkthrough

Walkthrough

Unit test expectations in kvCacheManagerTest.cpp were updated for revised KV-cache block reuse, partial-copy handling, event counting, and sliding-window reuse behavior.

Changes

KV-cache manager test expectation updates

Layer / File(s)	Summary
Partial-copy test setup and verification cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	GDS offload directory naming now uses a PID/dtype-based filename; partial-copy reuse targets a dedicated partialCopyBlockId (6) with updated assertions, logging, and DRAM-based onboard handling.
Basic block reuse expectations cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	BlockManagerReuseTest assertions and comments were updated for revised recycled, new, and freed block ids.
Extra-id and multimodal-hash reuse expectations cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	BlockManagerReuseWithExtraIdTest and BlockManagerReuseWithMultimodalHashTest were updated with new expected block-id sequences and comments.
LoraTaskId reuse expectations cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	BlockManagerReuseWithLoraTaskIdTest block-id assertions and comments were updated across recycled, no-reuse, and partial-reuse scenarios.
Cache-salt and leaf-block reuse expectations cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	BlockManagerReuseWithCacheSaltTest cache-block expectations were updated, and KVCacheManagerLeafBlockTest changed the detached-block recycling expectation.
Event-stream replace and max-attention-window reuse expectations cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp	KVCacheManagerEventStream now counts three replace-path events with updated categorization, and KVCacheManagerMaxAttentionWindowWithReuseTest updates block-id expectations as the sliding window advances.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: lancelly

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is specific and matches the main change: updating detached-block eviction tests for the NVBugs issue.
Description check	✅ Passed	The description follows the template with Description, Test Coverage, and PR Checklist sections and explains the issue, fix, and validation.
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)

440-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Partial-copy verification is tightly coupled to internal free-block selection order.

partialCopyBlockId = 6 is fetched via getBlockById and its primary-pool memory is memset to zero before addSequenceBatch runs for seq2, assuming the block manager will pick exactly block 6 (and that it's currently resident in the primary pool) as the partial-copy destination. This is inherent to white-box testing of the new detached-block-first eviction order and appears intentional (validated per PR description via integration testing), but it means this test will silently need re-tuning again if the free-queue/eviction policy changes further. A brief comment explaining why block 6 is guaranteed to be primary at this point would help future maintainers avoid re-deriving this from BlockManager internals.

🤖 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 440 -
505, Add a short clarifying comment in the partial-copy verification test around
the `partialCopyBlockId`, `getBlockById`, and `addSequenceBatch` flow to explain
why block 6 is expected to be the primary destination for the partial copy at
that point. Note that this test is intentionally white-box and depends on the
current detached-block-first selection behavior in `BlockManager`, so future
changes to free-block ordering or eviction policy may require updating the test
assumptions.

🤖 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 440-505: Add a short clarifying comment in the partial-copy
verification test around the `partialCopyBlockId`, `getBlockById`, and
`addSequenceBatch` flow to explain why block 6 is expected to be the primary
destination for the partial copy at that point. Note that this test is
intentionally white-box and depends on the current detached-block-first
selection behavior in `BlockManager`, so future changes to free-block ordering
or eviction policy may require updating the test assumptions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1d721ff5-a8ac-4302-ba92-ec33c5237ced

📥 Commits

Reviewing files that changed from the base of the PR and between 148bf8a and 11b7388.

📒 Files selected for processing (1)

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

[chienchunhung]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #58043 [ run ] triggered by Bot. Commit: 11b7388 Link to invocation

[chienchunhung]

Closing this PR as it is subsumed by #16019

[tensorrt-cicd]

PR_Github #58043 [ run ] completed with state SUCCESS. Commit: 11b7388
/LLM/main/L0_MergeRequest_PR pipeline #46713 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation