feat(l1): add durable writeback and prefetch eviction - #17
Conversation
L1 eviction discarded objects outright, so chunks that never went through the async store path were lost under memory pressure, and a restart cost a full cold prefill for every session. Three new opt-in EvictionConfig knobs (all default off, CLI args included): - write_back_on_evict: eviction actions target L2_CACHE; readable objects are synchronously persisted in bounded 128-key batches and deleted from L1 only once every key in the batch is durable (per-key results from the native adapter). Repeated flush failures open a circuit breaker with exponential backoff instead of retrying a dead L2 every second. - periodic_flush_interval: below-watermark backup flush that copies evictable L1 keys to L2 without deleting them, with a rotating cursor over the keyspace. Restart or session expiry then restores from L2 instead of re-prefilling. - emergency_evict_for_prefetch: the prefetch controller may synchronously evict LRU keys (write-back first) when a large L2->L1 restore cannot reserve L1 buffers, and retries OOM reservations once. Non-WARM requests only, capped at 60% of L1 per restore. New L1Manager helpers get_evictable_keys() and num_objects() keep the controllers on the public interface.
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe distributed storage stack adds opt-in L1-to-L2 write-back eviction, periodic backup flushing, emergency eviction for prefetch restores, new configuration flags and validation, runtime adapter synchronization, and comprehensive tests. ChangesL1 write-back and prefetch capacity management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PrefetchController
participant L1EvictionController
participant L1Manager
participant L2Adapter
PrefetchController->>L1EvictionController: emergency_evict_bytes(target_free_bytes)
L1EvictionController->>L1Manager: select and reserve evictable objects
L1EvictionController->>L2Adapter: store_objects_sync(keys, objects)
L1EvictionController->>L1Manager: delete durably persisted objects
PrefetchController->>L1Manager: retry failed reserve_write keys
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
lmcache/v1/distributed/storage_controllers/prefetch_controller.py (1)
127-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
strict=Falsehides a mismatch the type already guarantees.
MemoryLayoutDesc.__post_init__(lmcache/v1/distributed/api.py:257) already rejects unequalshapes/dtypes, sostrict=Truehere costs nothing and turns a future invariant break into a loud failure instead of a silently truncated byte count.♻️ Suggested tweak
- for shape, dtype in zip(layout_desc.shapes, layout_desc.dtypes, strict=False): + for shape, dtype in zip(layout_desc.shapes, layout_desc.dtypes, strict=True):🤖 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 `@lmcache/v1/distributed/storage_controllers/prefetch_controller.py` around lines 127 - 136, Update _layout_nbytes to use strict=True when zipping layout_desc.shapes and layout_desc.dtypes. Preserve the existing byte-count calculation while ensuring any future length mismatch raises immediately instead of truncating the iteration.tests/v1/distributed/test_l1_write_back_eviction.py (1)
237-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTiming-sensitive negative assertion.
time.sleep(0.05)thenassert not replaced.is_set()is the only guard thatset_l2_adaptersactually blocks. It is a negative assertion so it will not flake spuriously on slow CI, but it also cannot prove blocking beyond 50 ms. Acceptable as-is; noting for awareness.🤖 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/v1/distributed/test_l1_write_back_eviction.py` around lines 237 - 267, The review identifies the timing-based negative assertion in test_adapter_replacement_waits_for_active_flush as acceptable; no code change is required. Preserve the existing synchronization and assertions.lmcache/v1/distributed/config.py (1)
314-319: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider also validating write-back against a configured L2 tier.
write_back_on_evictfails closed at runtime: with no L2 adapter exposingstore_objects_sync,EvictionDestination.L2_CACHEis registered but every flush aborts, so L1 stops evicting entirely and fills up (only alogger.warninginStorageManager.__init__). Whenconfig.l2_adapter_config.adaptersis empty this is statically knowable and could be rejected here rather than degrading silently at runtime.♻️ Optional validation
if eviction.emergency_evict_for_prefetch and not eviction.write_back_on_evict: raise ValueError("emergency_evict_for_prefetch requires write_back_on_evict") + if eviction.write_back_on_evict and not config.l2_adapter_config.adapters: + raise ValueError("write_back_on_evict requires at least one L2 adapter")🤖 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 `@lmcache/v1/distributed/config.py` around lines 314 - 319, Extend the validation near the existing eviction checks to reject write_back_on_evict when no L2 adapter is configured: if eviction.write_back_on_evict is enabled and config.l2_adapter_config.adapters is empty, raise a clear ValueError. Preserve the current periodic_flush_interval and emergency_evict_for_prefetch validations.lmcache/v1/distributed/storage_manager.py (1)
174-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicates the injection already performed by
_sync_l1_writeback_adapters.Config validation guarantees
emergency_evict_for_prefetchimplieswrite_back_on_evict(config.py:317-318), sowants_l1_write_backis always True here and_sync_l1_writeback_adapters()(Line 168) has already calledset_l1_eviction_controllerunder the samehas_l2_flush_adapter()condition. Only the warning is new; theifbody is dead.♻️ Suggested simplification
- if eviction_config.emergency_evict_for_prefetch: - if self._eviction_controller.has_l2_flush_adapter(): - self._prefetch_controller.set_l1_eviction_controller( - self._eviction_controller - ) - else: - logger.warning( - "emergency_evict_for_prefetch has no synchronous L2 " - "adapter; inactive until one is added" - ) + if ( + eviction_config.emergency_evict_for_prefetch + and not self._eviction_controller.has_l2_flush_adapter() + ): + logger.warning( + "emergency_evict_for_prefetch has no synchronous L2 " + "adapter; inactive until one is added" + )🤖 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 `@lmcache/v1/distributed/storage_manager.py` around lines 174 - 183, Remove the redundant emergency_evict_for_prefetch conditional injection block after _sync_l1_writeback_adapters has run. Keep the existing warning only if needed for the no-synchronous-L2-adapter case, and rely on _sync_l1_writeback_adapters for set_l1_eviction_controller rather than calling it again.lmcache/v1/distributed/storage_controllers/eviction_controller.py (1)
510-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBackup log double-counts across adapters and trusts
okoverpersisted.
total_persisted/total_bytesaccumulate over every adapter, so with two sync-capable adapters the log reports 2× the key count. Alsoif ok:addspersistedwithout checking it equalslen(readable_keys), unlike the strict check in_flush_one_l2_batch_then_delete. Log-only impact, but the numbers are misleading for operators.🤖 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 `@lmcache/v1/distributed/storage_controllers/eviction_controller.py` around lines 510 - 533, Update the periodic L2 backup aggregation around _snapshot_l2_adapters so totals represent one backup operation rather than summing duplicate results from every adapter. Reuse the strict success criterion from _flush_one_l2_batch_then_delete: only count a sync result when it is successful and persisted equals len(readable_keys), and ensure the log reports the corresponding key and byte totals without adapter double-counting.
🤖 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 `@lmcache/v1/distributed/l1_manager.py`:
- Around line 796-805: Change get_evictable_keys to support bounded,
cursor-aware retrieval so it scans and returns only the requested batch while
holding the L1 lock, rather than materializing the full keyspace. Update
_backup_to_l2_no_delete_locked to use this API and preserve its rotating-cursor
behavior across calls, including correct wraparound when fewer than
_BACKUP_FLUSH_BATCH_SIZE keys remain.
In `@lmcache/v1/distributed/storage_controllers/eviction_controller.py`:
- Around line 320-330: Update get_eviction_actions to replace the fixed 64-key
padding with request-scaled padding so small tracked keyspaces do not force
ratio to 1.0 and evict all of L1. Before performing the eviction scan, check
_sync_flush_backoff_until and has_l2_flush_adapter(); while the backoff is
active or no adapter exists, return without generating actions because
_flush_to_l2_then_delete cannot free space.
In `@lmcache/v1/distributed/storage_controllers/prefetch_controller.py`:
- Around line 924-936: The emergency eviction path can block the single
_prefetch_loop thread on unbounded synchronous L2 synchronization. Update
_make_room_for_restore and _retry_oom_reservations, or the
emergency_evict_bytes/store_objects_sync path they invoke, to enforce a
deadline/timeout for synchronous L2 stores and associated reserve_read/delete
operations, or defer the eviction flush off the prefetch loop while preserving
eviction correctness.
- Around line 1126-1139: Prevent a short retentions result from raising inside
the request-processing path: either validate the output of
_policy.select_l1_retentions in _transition_to_load_phase and complete the
request cleanly on mismatch, or remove strict=True in the retention_by_key zip
to preserve the existing lenient behavior. Ensure failures still reach
_complete_request so in-flight state, locks, and reservations are released.
In `@lmcache/v1/distributed/storage_manager.py`:
- Around line 1156-1171: Update _sync_l1_writeback_adapters to clear the
prefetch controller’s L1 eviction controller when has_l2_flush_adapter() is
false, while preserving the existing wiring when a flush adapter exists. Modify
PrefetchController.set_l1_eviction_controller to accept None so the field can be
explicitly disconnected after the last sync-capable adapter is removed.
In `@tests/v1/distributed/test_l1_write_back_eviction.py`:
- Around line 338-344: Update the TestPrefetchMakeRoom setup to provide a
fixture that creates the PrefetchController via _make_prefetch_controller and
closes its _submission_efd and _adapter_ctrl_efd notifiers during teardown
without calling stop(). Change the five affected tests to accept the
prefetch_controller fixture instead of invoking the helper directly, ensuring
notifier resources are released after each test.
---
Nitpick comments:
In `@lmcache/v1/distributed/config.py`:
- Around line 314-319: Extend the validation near the existing eviction checks
to reject write_back_on_evict when no L2 adapter is configured: if
eviction.write_back_on_evict is enabled and config.l2_adapter_config.adapters is
empty, raise a clear ValueError. Preserve the current periodic_flush_interval
and emergency_evict_for_prefetch validations.
In `@lmcache/v1/distributed/storage_controllers/eviction_controller.py`:
- Around line 510-533: Update the periodic L2 backup aggregation around
_snapshot_l2_adapters so totals represent one backup operation rather than
summing duplicate results from every adapter. Reuse the strict success criterion
from _flush_one_l2_batch_then_delete: only count a sync result when it is
successful and persisted equals len(readable_keys), and ensure the log reports
the corresponding key and byte totals without adapter double-counting.
In `@lmcache/v1/distributed/storage_controllers/prefetch_controller.py`:
- Around line 127-136: Update _layout_nbytes to use strict=True when zipping
layout_desc.shapes and layout_desc.dtypes. Preserve the existing byte-count
calculation while ensuring any future length mismatch raises immediately instead
of truncating the iteration.
In `@lmcache/v1/distributed/storage_manager.py`:
- Around line 174-183: Remove the redundant emergency_evict_for_prefetch
conditional injection block after _sync_l1_writeback_adapters has run. Keep the
existing warning only if needed for the no-synchronous-L2-adapter case, and rely
on _sync_l1_writeback_adapters for set_l1_eviction_controller rather than
calling it again.
In `@tests/v1/distributed/test_l1_write_back_eviction.py`:
- Around line 237-267: The review identifies the timing-based negative assertion
in test_adapter_replacement_waits_for_active_flush as acceptable; no code change
is required. Preserve the existing synchronization and assertions.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 015c0d82-80d2-48bf-99c7-4d64477e659f
📒 Files selected for processing (6)
lmcache/v1/distributed/config.pylmcache/v1/distributed/l1_manager.pylmcache/v1/distributed/storage_controllers/eviction_controller.pylmcache/v1/distributed/storage_controllers/prefetch_controller.pylmcache/v1/distributed/storage_manager.pytests/v1/distributed/test_l1_write_back_eviction.py
|
Review follow-up in 01743ad also applies the valid nitpicks: strict layout zip, removal of duplicate controller injection, and first-success accounting for periodic backup. I intentionally did not reject writeback when the static config has no L2 adapter: runtime adapter addition is supported, fail-closed behavior is deliberate until a compatible adapter is attached, and adapter synchronization activates/deactivates emergency eviction dynamically. Focused result: 24 passed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Ports the useful L1 writeback work from Florian's closed PR #5 onto the GLM-5.2 release base, then tightens the durability and adapter-lifecycle contracts found during review.
The native durable-store contract consumed here is provided by #15. No writeback behavior is enabled by default.
Provenance
Validation
ruff checkandruff formaton changed filesSummary by CodeRabbit
New Features
Bug Fixes