Fix expand ceiling bug in ObjectSizeController

Summary:
## Background

Object cache capacity is configured by setting `l1EntriesLimit` via `ObjectCacheConfig::setCacheCapacity()`. During initialization, the cache estimates how many slabs are needed to hold that many entries:

  allocsPerSlab  = Slab::kSize / l1AllocSize     (items that fit in one slab)
  slabsPerShard  = ceil(l1EntriesLimit / numShards / allocsPerSlab)

Because of the ceiling division, the total number of allocation slots (slabsPerShard × allocsPerSlab × numShards) is always >= l1EntriesLimit. The surplus slots are filled with "structural placeholders" — dummy items that prevent user data from exceeding the configured limit.

## Bug & Fix

In `FreeMemoryOnly` mode, `work()` calls `expandCacheByEntriesNum(expandCacheBy)` directly — it never flows through `adjustCacheEntriesLimit()` where the `std::min(newEntriesLimit, l1EntriesLimit)` cap lives. Without a ceiling check inside `expandCacheByEntriesNum()` itself, repeated expand calls could pop structural placeholders, pushing `currentEntriesLimit_` above `l1EntriesLimit`.

Example (l1EntriesLimit=100, 5 structural placeholders from slab rounding):
  After init:   currentEntriesLimit_=100, placeholders=5
  Shrink by 10: currentEntriesLimit_=90,  placeholders=15
  Expand by 15: pops all 15 → currentEntriesLimit_=105 > 100!

Fix: pre-calculate `maxExpand = l1EntriesLimit - currentEntriesLimit_` as the loop bound in `expandCacheByEntriesNum()`, so it caps itself regardless of the call path. Also snapshot the atomic `currentEntriesLimit_` in `adjustCacheEntriesLimit()` and fix `totalEntriesExpanded_` counter.

Reviewed By: rlyerly

Differential Revision: D97062845

fbshipit-source-id: e736a9325dcea88bc2ff4e4b44d9e0e985989145

Author: pbhandar2

cachelib/object_cache/ObjectCacheSizeController.h

@@ -57,7 +57,7 @@ class ObjectCacheSizeController : public PeriodicWorker {
 
  private:
   void work() override final;
-  void expandCacheByEntriesNum(size_t entries);
+  void expandCacheByEntriesNum(size_t entries, size_t currentLimit);
   int64_t getNewNumEntries();
 
   std::pair<size_t, size_t> trackJemallocMemStats() const {
@@ -123,7 +123,8 @@ void ObjectCacheSizeController<AllocatorT>::work() {
       ensureFreeMemoryAboveLowerLimit();
     } else if (freeMemBytes > objCache_.config_.upperLimitBytes) {
       // Only expand cache when free memory is above the upper limit
-      expandCacheByEntriesNum(objCache_.config_.expandCacheBy);
+      expandCacheByEntriesNum(objCache_.config_.expandCacheBy,
+                              getCurrentEntriesLimit());
     }
     // When free memory is between lower and upper limits, do nothing (stable
     // zone)
@@ -241,15 +242,17 @@ void ObjectCacheSizeController<AllocatorT>::work() {
 
   // entriesLimit should never exceed the configured entries limit
   newEntriesLimit = std::min(newEntriesLimit, objCache_.config_.l1EntriesLimit);
-  if (newEntriesLimit < currentEntriesLimit_ &&
+  auto currentEntriesLimit = getCurrentEntriesLimit();
+  if (newEntriesLimit < currentEntriesLimit &&
       currentNumEntries >= newEntriesLimit) {
     // shrink the cache
-    shrinkCacheByEntriesNum(currentEntriesLimit_ - newEntriesLimit);
-  } else if (newEntriesLimit > currentEntriesLimit_ &&
-             currentNumEntries == currentEntriesLimit_) {
+    shrinkCacheByEntriesNum(currentEntriesLimit - newEntriesLimit);
+  } else if (newEntriesLimit > currentEntriesLimit &&
+             currentNumEntries == currentEntriesLimit) {
     // expand cache when getting a higher new limit and current entries num
     // reaches the old limit
-    expandCacheByEntriesNum(newEntriesLimit - currentEntriesLimit_);
+    expandCacheByEntriesNum(newEntriesLimit - currentEntriesLimit,
+                            currentEntriesLimit);
   }
 }
 
@@ -275,31 +278,43 @@ void ObjectCacheSizeController<AllocatorT>::shrinkCacheByEntriesNum(
 
   XLOGF_EVERY_MS(
       INFO, 60'000,
-      "CacheLib size-controller: request to shrink cache by {} entries. "
+      "CacheLib size-controller: request to shrink cache by {} entries "
+      "(actual: {}). "
       "Placeholders num before: {}, after: {}. currentEntriesLimit: {}",
-      entries, before, objCache_.getNumPlaceholders(), currentEntriesLimit_);
+      entries, actualShrunk, before, objCache_.getNumPlaceholders(),
+      currentEntriesLimit_);
 }
 
 template <typename AllocatorT>
 void ObjectCacheSizeController<AllocatorT>::expandCacheByEntriesNum(
-    size_t entries) {
+    size_t entries, size_t currentLimit) {
   expandCacheCalls_.inc();
   util::Throttler t(throttlerConfig_);
   auto before = objCache_.getNumPlaceholders();
   entries = std::min<size_t>(entries, before);
+  // Not all callers cap entries against l1EntriesLimit (e.g. FreeMemoryOnly
+  // mode calls this directly), so enforce the ceiling here.
+  size_t maxExpand = (currentLimit < objCache_.config_.l1EntriesLimit)
+                         ? objCache_.config_.l1EntriesLimit - currentLimit
+                         : 0;
+  entries = std::min(entries, maxExpand);
+  size_t actualExpanded = 0;
   for (size_t i = 0; i < entries; i++) {
     objCache_.placeholders_.pop_back();
     currentEntriesLimit_++;
+    actualExpanded++;
     // throttle to slow down the release speed
     t.throttle();
   }
-  totalEntriesExpanded_.add(entries);
+  totalEntriesExpanded_.add(actualExpanded);
 
   XLOGF_EVERY_MS(
       INFO, 60'000,
-      "CacheLib size-controller: request to expand cache by {} entries. "
+      "CacheLib size-controller: request to expand cache by {} entries "
+      "(actual: {}). "
       "Placeholders num before: {}, after: {}. currentEntriesLimit: {}",
-      entries, before, objCache_.getNumPlaceholders(), currentEntriesLimit_);
+      entries, actualExpanded, before, objCache_.getNumPlaceholders(),
+      currentEntriesLimit_);
 }
 
 template <typename AllocatorT>

cachelib/object_cache/tests/ObjectCacheTest.cpp

@@ -2343,7 +2343,7 @@ TEST(ObjectCacheTest, DynamicFreeMemorySizeControlTest) {
   auto lowerLimitBytes = 9 * kMB;
   auto itemSize = 1024;
   uint64_t maxNumEntries = 1024 * 20;
-  uint64_t sizeControlIntervalMs = 50;
+  int sizeControlIntervalMs = 50;
 
   ObjectCache::Config config;
   config.setCacheName("dynamic_free_mem_test");
@@ -2448,5 +2448,71 @@ TEST(ObjectCacheTest, DynamicFreeMemorySizeControlTest) {
   auto entriesAfterExpand = objcache->getCurrentEntriesLimit();
 
   EXPECT_GT(entriesAfterExpand, entriesAfterShrink);
+  EXPECT_LE(entriesAfterExpand, maxNumEntries);
+}
+
+TEST(ObjectCacheTest, ExpandNeverExceedsL1EntriesLimit) {
+  const uint64_t kMB = 1024 * 1024;
+  auto upperLimitBytes = 15 * kMB;
+  auto lowerLimitBytes = 9 * kMB;
+  auto itemSize = 1024;
+  uint64_t maxNumEntries = 1024 * 20;
+  int sizeControlIntervalMs = 50;
+
+  ObjectCache::Config config;
+  config.setCacheName("expand_ceiling_test");
+  config.setItemDestructor([&](ObjectCacheDestructorData data) {
+    data.deleteObject<MemoryConsumer>();
+  });
+  config.setCacheCapacity(maxNumEntries, itemSize * maxNumEntries,
+                          sizeControlIntervalMs);
+  config.setObjectSizeControllerMode(ObjCacheSizeControlMode::FreeMemoryOnly,
+                                     upperLimitBytes, lowerLimitBytes);
+  config.memoryMode = FreeMemoryOnly;
+  // Large expand/shrink to trigger structural placeholder popping
+  config.expandCacheBy = 500;
+  config.shrinkCacheBy = 500;
+
+  static std::atomic<uint64_t> sExpandTestFreeMem{20 * kMB};
+  config.setFreeMemCb([]() { return sExpandTestFreeMem.load(); });
+
+  auto objcache = ObjectCache::create(config);
+
+  // Fill cache
+  for (size_t i = 0; i < maxNumEntries; i++) {
+    auto key = folly::sformat("key_{}", i);
+    objcache->insertOrReplace(key, std::make_unique<MemoryConsumer>(itemSize),
+                              itemSize);
+  }
+  EXPECT_EQ(objcache->getCurrentEntriesLimit(), maxNumEntries);
+
+  // Run 3 shrink->expand cycles
+  for (int cycle = 0; cycle < 3; cycle++) {
+    // Shrink: set free memory below lower limit
+    sExpandTestFreeMem.store(5 * kMB);
+    auto shrunk = test_util::eventuallyTrue(
+        [&]() { return objcache->getCurrentEntriesLimit() < maxNumEntries; },
+        3 /* timeoutSecs */);
+    EXPECT_TRUE(shrunk) << "Shrink failed on cycle " << cycle;
+
+    // Expand: set free memory above upper limit
+    sExpandTestFreeMem.store(20 * kMB);
+    // Wait for expand to settle
+    size_t lastLimit = objcache->getCurrentEntriesLimit();
+    auto settled = test_util::eventuallyTrue(
+        [&]() {
+          auto cur = objcache->getCurrentEntriesLimit();
+          if (cur != lastLimit) {
+            lastLimit = cur;
+            return false;
+          }
+          return true;
+        },
+        3 /* timeoutSecs */);
+    EXPECT_TRUE(settled) << "Expand didn't settle on cycle " << cycle;
+
+    EXPECT_LE(objcache->getCurrentEntriesLimit(), maxNumEntries)
+        << "currentEntriesLimit exceeded l1EntriesLimit on cycle " << cycle;
+  }
 }
 } // namespace facebook::cachelib::objcache2::test