Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces startup seeding and periodic LRU-index sidecar checkpointing/restoring features to the S3L2Adapter, along with comprehensive unit tests. The review feedback highlights opportunities to optimize startup performance by batching eviction policy notifications in _seed_policy_in_order instead of notifying one key at a time, and to prevent potential shutdown hangs by adding a configurable timeout to _checkpoint_lru_index and using a shorter timeout during adapter closure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
The current implementation of _seed_policy_in_order notifies the eviction policy listener one key at a time in a loop. For a large bucket (e.g., 10k to 100k+ keys), this results in
Since policy.on_keys_created reverses the input list of keys internally, we can achieve the exact same ordering in a single batch notification by reversing the list of keys and sizes first. This reduces lock acquisitions and listener notifications from
Additionally, to ensure that the access ticks are assigned in the correct oldest-to-newest order when _track_access_order is enabled, we can temporarily disable tracking during the batch notification and manually populate _access_ticks in the correct order.
def _seed_policy_in_order(
self, ordered: list[tuple[ObjectKey, int]]
) -> int:
"""Seed usage + the eviction policy from keys in LRU order.
``ordered`` is ``[(ObjectKey, size), ...]`` with the **least
recently used key first**. The eviction policy is populated via
``_notify_keys_stored`` (base class -> ``L2EvictionPolicy`` ->
``policy.on_keys_created``); ``on_keys_created`` reverses keys
within a single call, so we notify one key at a time to make the
final OrderedDict ordering match ``ordered`` exactly (oldest at
the front == first eviction victim). ``_key_sizes`` is also
seeded so a later ``delete`` of a seeded key balances the
base-class byte accounting.
Returns the number of keys seeded.
"""
to_seed_keys = []
to_seed_sizes = []
with self._lock:
for key, size in ordered:
if key in self._key_sizes:
# Already known from a store in this process — don't
# double-count or reorder it.
continue
self._key_sizes[key] = size
self._object_size_cache[_object_key_to_string(key)] = size
to_seed_keys.append(key)
to_seed_sizes.append(size)
if not to_seed_keys:
return 0
# Temporarily disable tracking to avoid incorrect tick assignment during batch notification
was_tracking = self._track_access_order
self._track_access_order = False
try:
# Reverse the lists so that policy.on_keys_created (which processes keys in reverse)
# reconstructs the original oldest-to-newest order in its internal OrderedDict.
self._notify_keys_stored(list(reversed(to_seed_keys)), list(reversed(to_seed_sizes)))
finally:
self._track_access_order = was_tracking
# If tracking is enabled, manually initialize the ticks in the correct oldest-to-newest order
if was_tracking:
with self._lock:
for key, size in zip(to_seed_keys, to_seed_sizes, strict=True):
self._access_tick_counter += 1
self._access_ticks[_object_key_to_string(key)] = (size, self._access_tick_counter)
return len(to_seed_keys)References
- LMCache does not support Python 3.9 (it requires Python 3.10+). Therefore, Python 3.10 features such as
zip(..., strict=True)are fully supported and should not be avoided or removed for Python 3.9 compatibility.
| def _checkpoint_lru_index(self) -> bool: | ||
| """Serialize the access-order index and PUT it to the sidecar. | ||
|
|
||
| Best-effort: returns ``False`` (logged) on any failure so the | ||
| checkpoint loop keeps running. | ||
| """ | ||
| snapshot = self._snapshot_lru_index() | ||
| if not snapshot: | ||
| return False | ||
| try: | ||
| body = _serialize_lru_index(snapshot) | ||
| except Exception: | ||
| logger.exception("S3L2Adapter failed to serialize LRU index") | ||
| return False | ||
| fut = asyncio.run_coroutine_threadsafe( | ||
| self._execute_put_raw(LRU_INDEX_OBJECT_NAME, body), | ||
| self._loop, | ||
| ) | ||
| try: | ||
| ok = fut.result(timeout=60.0) | ||
| except Exception as exc: | ||
| logger.warning("S3L2Adapter LRU checkpoint PUT failed: %s", exc) | ||
| return False | ||
| if ok: | ||
| logger.debug( | ||
| "S3L2Adapter checkpointed LRU index (%d entries, %d bytes)", | ||
| len(snapshot), | ||
| len(body), | ||
| ) | ||
| return ok |
There was a problem hiding this comment.
The _checkpoint_lru_index method currently has a hardcoded timeout of 60.0 seconds when waiting for the S3 PUT request to complete. During process shutdown (in close()), blocking the calling thread for up to 60 seconds if S3 is slow or unresponsive can exceed the container's termination grace period (typically 30 seconds), leading to a forceful SIGKILL before other resources can clean up gracefully.
Adding a timeout parameter to _checkpoint_lru_index allows close() to use a much shorter timeout (e.g., 5.0 seconds) for the final checkpoint.
def _checkpoint_lru_index(self, timeout: float = 60.0) -> bool:
"""Serialize the access-order index and PUT it to the sidecar.
Best-effort: returns ``False`` (logged) on any failure so the
checkpoint loop keeps running.
"""
snapshot = self._snapshot_lru_index()
if not snapshot:
return False
try:
body = _serialize_lru_index(snapshot)
except Exception:
logger.exception("S3L2Adapter failed to serialize LRU index")
return False
fut = asyncio.run_coroutine_threadsafe(
self._execute_put_raw(LRU_INDEX_OBJECT_NAME, body),
self._loop,
)
try:
ok = fut.result(timeout=timeout)
except Exception as exc:
logger.warning("S3L2Adapter LRU checkpoint PUT failed: %s", exc)
return False
if ok:
logger.debug(
"S3L2Adapter checkpointed LRU index (%d entries, %d bytes)",
len(snapshot),
len(body),
)
return ok| if self._checkpoint_thread is not None: | ||
| self._checkpoint_stop.set() | ||
| try: | ||
| self._checkpoint_lru_index() | ||
| except Exception: | ||
| logger.exception("S3L2Adapter final LRU checkpoint failed") | ||
| self._checkpoint_thread.join(timeout=5) |
There was a problem hiding this comment.
Use a shorter timeout (e.g., 5.0 seconds) for the final checkpoint in close() to prevent blocking the shutdown process for up to 60 seconds if S3 is slow or unresponsive.
| if self._checkpoint_thread is not None: | |
| self._checkpoint_stop.set() | |
| try: | |
| self._checkpoint_lru_index() | |
| except Exception: | |
| logger.exception("S3L2Adapter final LRU checkpoint failed") | |
| self._checkpoint_thread.join(timeout=5) | |
| if self._checkpoint_thread is not None: | |
| self._checkpoint_stop.set() | |
| try: | |
| self._checkpoint_lru_index(timeout=5.0) | |
| except Exception: | |
| logger.exception("S3L2Adapter final LRU checkpoint failed") | |
| self._checkpoint_thread.join(timeout=5) |
|
Thanks — addressed both in the latest push:
Also folded in the earlier fix where a missing sidecar (404) raised |
|
Note on the red The failure is in an unrelated ROCm GPU benchmark: With It's repo-wide, not PR-specific:
This PR only touches |
|
@ApostaC @maobaolong @chunxiaozheng (and @YaoJiayi @hlin99 for the eviction-policy interaction) — review would be appreciated when convenient. Opt-in change so the S3 L2 LRU index survives process restarts: a startup bucket-scan seeds usage, and a periodic sidecar checkpoint persists the access order (default off → unchanged behavior otherwise). Gemini's notes are addressed (batched O(1) policy seed, bounded checkpoint timeout, Independent sibling of #3892. |
|
@divyvasal Thanks for contributing to LMCache @aeon-x can you help take a look? Thanks! |
|
Hey @divyvasal , we have a start up resync feature in LMCache coordinator. It does exactly what you have in this PR in the coordinator level. Can you check it out? |
|
Thanks @aeon-x — I looked at the coordinator startup-resync. IIUC it backfills the coordinator's usage/eviction trackers from an MP server's The case this PR targets is when that view is itself gone. On spot-reclaim / pod-replacement the whole L2 server process is replaced, so a freshly-started server reports an empty It also pairs with #3892 (opt-in per- So I see these as complementary to the coordinator resync rather than duplicating it — different source of truth (S3 bucket vs. server view). Happy to add a docs note cross-referencing the two so users pick the right mechanism for their topology. Does that distinction line up with your read? |
|
This pull request has been automatically marked as stale because it has not had activity within 60 days. It will be automatically closed if no further activity occurs within 30 days. |
The S3 L2 adapter's byte counter and the eviction policy's LRU key index are in-memory and reset to 0 on every process restart / box replacement. After a restart the adapter can't 'see' objects already in the bucket, so it never evicts them and the bucket grows across process generations until the 30-day lifecycle TTL. This makes the LRU cap bind across restarts via two opt-in, default-OFF features. (a) s3_seed_usage_on_start: on startup, paginate the bucket and seed both the aggregate usage counter and the eviction policy's key index (via _notify_keys_stored -> L2EvictionPolicy.on_l2_keys_stored -> policy.on_keys_created), ordered oldest-LastModified-first so the oldest object is the first eviction victim. Seeding is per-key to preserve global LRU order. Scan failures are logged and non-fatal. (b) s3_persist_lru_index: periodically checkpoint a compact JSON sidecar (_lmcache_lru_index.json) capturing true access-order ticks, and restore it on startup, reconciled against the live listing (drop vanished keys, add new keys by LastModified). Corrupt / partial / unknown-version sidecars are treated as absent (fall back to the scan). A permissive LastModified parser treats missing timestamps as oldest. Tests under tests/v1/distributed/test_s3_l2_adapter_seed.py cover the pure parts (LastModified parse, listing parse with mtime, oldest-first ordering, sidecar roundtrip + robustness, policy population in LRU order) with awscrt/torch stubbed; no live AWS required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Divy <divy@coralbricks.ai>
The raw GET for the sidecar raised RuntimeError inside the CRT on_done callback on any non-200 — including the expected 404 when no sidecar exists yet. That logged a noisy traceback even though finished_future already surfaces the error and the caller falls back to a scan. Don't raise in the callback; record status + error and let the awaiter return None quietly on 404/non-200. Signed-off-by: Divy <divy@coralbricks.ai>
- _seed_policy_in_order: replace the per-key notify loop with a SINGLE batch _notify_keys_stored of the reversed key list. on_keys_created reverses internally, so this reconstructs the same oldest-first policy order with O(1) lock/listener traffic instead of O(N) (a 100k-object bucket no longer thrashes on startup). Tick tracking is suppressed during the batch and seeded explicitly afterwards in oldest->newest order so access ticks stay correct. - _checkpoint_lru_index(timeout=60.0): make the PUT-wait bound configurable; close() now passes timeout=5.0 so a slow/unresponsive S3 can't block shutdown past the container termination grace period (-> SIGKILL). Signed-off-by: Divy <divy@coralbricks.ai>
66858cf to
7e19316
Compare
Signed-off-by: Divy <divy@coralbricks.ai>
fe09b05 to
570d69a
Compare
Summary
Make the S3 L2 cache's LRU eviction survive process restarts. Today the adapter's byte counter and the eviction policy's key index are in-memory — they reset to 0 on every restart / pod replacement / spot reclaim. So a fresh process doesn't "see" the objects already in the bucket, can't evict them, and the bucket grows across process generations until the lifecycle TTL ages it out. The configured LRU cap stops bounding the bucket across restarts. This adds two opt-in mechanisms to fix that. Both default off → no behavior change for existing deployments.
Independent of (and complementary to) the per-
cache_saltbucket PR (#3892) — different branch, different files-of-concern.(a)
s3_seed_usage_on_start(defaultFalse)On startup, scan the bucket (paginated
ListObjectsV2, progress-logged, non-fatal on error) and seed both:_total_bytes_used), andusing S3
LastModifiedas the access-order proxy (oldest-modified = first victim). After seeding,_check_and_evict_globalsees the true footprint and evicts down to the cap immediately — across any restart.The load-bearing detail: seeding usage alone is useless; the policy must get the keys too. The path is
_notify_keys_stored→L2EvictionPolicy.on_l2_keys_stored→policy.on_keys_created→LRUEvictionPolicy._order.on_keys_createdreverses keys within a call, so seeding is done one key at a time, oldest-LastModified-first, which lands the oldest object at the front of_order(first eviction victim) — verified against the realLRUEvictionPolicy.(b)
s3_persist_lru_index(defaultFalse,s3_lru_index_checkpoint_interval_sdefault 300)(a) uses
LastModifiedas the order proxy, which mis-ranks a hot-but-old prefix (created first, reused thousands of times, never rewritten) → it could be evicted despite being hot. To preserve true access-order across restarts:_lmcache_lru_index.json); access ticks are only recorded when the flag is on (hot path untouched otherwise).LastModifiedas coldest), and rebuild the tick counter. A missing/partial/corrupt/unknown-version sidecar is treated as absent → falls back to (a)'s scan.close()writes a final checkpoint.Tests
tests/v1/distributed/test_s3_l2_adapter_seed.py— 18 unit tests (stub awscrt/torch; pure logic runs for real, including against the realLRUEvictionPolicy):LastModifiedparsing + ordering, size-summing, policy_orderpopulation in eviction order, skip-already-known, sidecar serialize/deserialize roundtrip, corrupt/partial/unknown-version → treated as absent, tick recording gated by the flag.Notes / open questions (flagged, not guessed)
_order. If you'd prefer the checkpoint to live with the policy, that's a design call._notify_keys_storedonce per key to preserve exact order (one-time startup cost); a batched order-preserving insert on the policy would speed huge-bucket seeds but doesn't exist today.Draft for now — feedback welcome on the config surface and the two ordering questions above.