Skip to content

fix: roll back partial KV allocation on page miss#365

Open
shipiyouniao wants to merge 1 commit into
ovg-project:mainfrom
shipiyouniao:fix/alloc-page-miss-rollback
Open

fix: roll back partial KV allocation on page miss#365
shipiyouniao wants to merge 1 commit into
ovg-project:mainfrom
shipiyouniao:fix/alloc-page-miss-rollback

Conversation

@shipiyouniao

@shipiyouniao shipiyouniao commented Jun 24, 2026

Copy link
Copy Markdown

Summary

Fixes #364.

Fixes a lower-level allocation failure path in KVCacheManager.alloc() where PageAllocator.alloc_page() can raise after some block ids have already been reserved for the current allocation request.

Root Cause

available_size() and physical page allocation are not atomic across multiple instances sharing the same physical pool. A caller may pass the capacity check, consume reserved blocks or blocks from an existing page, and then fail when requesting a new physical page.

Before this change, that RuntimeError escaped without rolling back the partially allocated block ids.

What Changed

  • Catch RuntimeError around page_allocator.alloc_page().
  • Return None as an allocation miss instead of crashing the allocation path.
  • Roll back any partially allocated block ids before returning None.
  • Use an internal _free(..., release_empty_pages=False) rollback path so a partial-allocation rollback does not unmap/release physical pages that were not intentionally returned to the physical pool.
  • Keep normal public free() behavior unchanged for real releases.
  • Add unit coverage for:
    • reserved-block partial allocation rollback;
    • existing-page partial allocation rollback;
    • failure before any partial allocation.

Tests

python -m pytest tests/test_kvcache_manager_alloc_miss.py -q
python -m py_compile kvcached/kv_cache_manager.py tests/test_kvcache_manager_alloc_miss.py

Copilot AI review requested due to automatic review settings June 24, 2026 14:14

Copilot AI 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.

Pull request overview

Fixes a failure mode in KVCacheManager._alloc() where a PageAllocator.alloc_page() RuntimeError could escape after partially allocating block IDs, leaking those IDs instead of returning an allocation miss (None).

Changes:

  • Catch RuntimeError from page_allocator.alloc_page() / page.init() and return None (allocation miss) instead of propagating the exception.
  • Attempt to roll back any partially allocated block IDs via free() before returning None, and log rollback failures without masking the miss.
  • Add regression tests covering partial allocation followed by a page-allocation failure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
kvcached/kv_cache_manager.py Adds exception handling + rollback for partial allocations when physical page allocation fails.
tests/test_kvcache_manager_alloc_miss.py Adds regression tests for allocation-miss rollback scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_kvcache_manager_alloc_miss.py Outdated
Comment thread tests/test_kvcache_manager_alloc_miss.py Outdated
@shipiyouniao shipiyouniao force-pushed the fix/alloc-page-miss-rollback branch from cf0d799 to 11738b1 Compare July 6, 2026 16:29
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.

KVCacheManager does not roll back partial allocation when alloc_page fails

2 participants