fix: apply sizeChange delta before HasRemoveKey early return in ObjectStore IPU

ObjectStore InPlaceUpdaterWorker's HasRemoveKey path (e.g. SPOP/LPOP/HDEL/ZREM
removing the last element) returned false with ExpireAndStop without applying
the sizeChange from Operate. The mutation had already happened in-place
(HeapMemorySize updated on the object), but the tracker never saw the negative
delta. OnDispose(Deleted) then subtracted only the post-removal empty-collection
size, leaving the pre-removal payload permanently over-counted.

Fix: apply sizeChange to cacheSizeTracker before the early return so the
tracker reflects the in-place mutation before ExpireAndStop disposes the record.

Found by Opus 4.7 review.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: badrishc

libs/server/Storage/Functions/ObjectStore/RMWMethods.cs

@@ -130,8 +130,12 @@ bool InPlaceUpdaterWorker(ref LogRecord logRecord, ref ObjectInput input, ref Ob
                         functionsState.watchVersionMap.IncrementVersion(rmwInfo.KeyHash);
                     if (functionsState.appendOnlyFile != null)
                         rmwInfo.UserData |= NeedAofLog;
-                    // Heap disposal and cache size tracking are handled by
-                    // OnDispose(Deleted) in the ExpireAndStop path of InternalRMW.
+                    // Apply the mutation delta before the ExpireAndStop path disposes the record.
+                    // Operate already mutated the object (e.g. removed the last element), so
+                    // HeapMemorySize is now the post-removal size. Without this, the delta from
+                    // the removal is lost and the tracker permanently over-counts.
+                    if (sizeChange != 0)
+                        functionsState.cacheSizeTracker?.AddHeapSize(sizeChange);
                     rmwInfo.Action = RMWAction.ExpireAndStop;
                     return false;
                 }

libs/storage/Tsavorite/cs/src/core/Allocator/AllocatorBase.cs

@@ -1426,7 +1426,7 @@ internal void EvictPageForRecovery(long page)
             {
                 _wrapper.EvictRecordsInRange(start, end, source);
             }
-            if (onEvictionObserver is not null && !ReferenceEquals(onEvictionObserver, logSizeTracker))
+            if (onEvictionObserver is not null)
             {
                 MemoryPageScan(start, end, onEvictionObserver);
             }
@@ -1504,7 +1504,7 @@ private void OnPagesClosedWorker()
 
                     // Legacy observer path — skip if the observer IS the logSizeTracker, since
                     // EvictRecordsInRange below already handles heap accounting via logSizeTracker.
-                    if (onEvictionObserver is not null && !ReferenceEquals(onEvictionObserver, logSizeTracker))
+                    if (onEvictionObserver is not null)
                         MemoryPageScan(start, end, onEvictionObserver);
 
                     // Per-record eviction walk: handles internal heap-size accounting (key overflow

libs/storage/Tsavorite/cs/src/core/Index/Common/LogSizeTracker.cs

@@ -33,7 +33,7 @@ public class LogSizeTracker
     /// <summary>Tracks and controls size of log</summary>
     /// <typeparam name="TStoreFunctions"></typeparam>
     /// <typeparam name="TAllocator"></typeparam>
-    public sealed class LogSizeTracker<TStoreFunctions, TAllocator> : LogSizeTracker, IObserver<ITsavoriteScanIterator>
+    public sealed class LogSizeTracker<TStoreFunctions, TAllocator> : LogSizeTracker
         where TStoreFunctions : IStoreFunctions
         where TAllocator : IAllocator<TStoreFunctions>
     {
@@ -190,22 +190,6 @@ public void UpdateTargetSize(long newTargetSize, long highDelta, long lowDelta)
                 resizeTaskEvent.Set();
         }
 
-        /// <summary>Callback on allocator completion</summary>
-        public void OnCompleted() { }
-
-        /// <summary>Callback on allocator error</summary>
-        public void OnError(Exception error) { }
-
-        /// <summary>Callback on allocator evicting a page to disk</summary>
-        public void OnNext(ITsavoriteScanIterator recordIter)
-        {
-            long size = 0;
-            while (recordIter.GetNext())
-                size += MemoryUtils.CalculateHeapMemorySize(in recordIter);
-            if (size != 0)
-                heapSize.Increment(-size); // Reduce size as records are being evicted
-        }
-
         /// <summary>Adds size to the tracked total count</summary>
         public void IncrementSize(long size)
         {

libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/LogAccessor.cs

@@ -176,8 +176,6 @@ public IDisposable Subscribe(IObserver<ITsavoriteScanIterator> readOnlyObserver)
         public IDisposable SubscribeEvictions(IObserver<ITsavoriteScanIterator> evictionObserver)
         {
             allocatorBase.onEvictionObserver = evictionObserver;
-            if (evictionObserver is LogSizeTracker<TStoreFunctions, TAllocator> tracker)
-                allocatorBase.logSizeTracker = tracker;
             return new LogSubscribeDisposable(allocatorBase, isReadOnly: false);
         }
 

test/Garnet.test/CacheSizeTrackerTests.cs

@@ -259,5 +259,67 @@ public void ReadCacheIncreaseEmptyPageCountTest()
             // The first page of the read cache has been evicted => 11 records removed, 9 remain.
             ClassicAssert.AreEqual(9 * MemorySizePerEntry, cacheSizeTracker.readCacheTracker.LogHeapSizeBytes);
         }
+
+        /// <summary>
+        /// Verifies that removing the last element from an object collection (triggering HasRemoveKey →
+        /// ExpireAndStop) correctly returns the heap tracker to zero. This exercises the IPU path where
+        /// Operate mutates the object in-place, the sizeChange delta is applied before the ExpireAndStop
+        /// early return, and OnDispose(Deleted) subtracts the remaining empty-collection overhead.
+        /// </summary>
+        [Test]
+        public void RemoveLastElementReturnsTrackerToZero_IPU()
+        {
+            ClassicAssert.AreEqual(0, cacheSizeTracker.mainLogTracker.LogHeapSizeBytes);
+
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+
+            // Add a single element to a set — creates the object record with heap tracking.
+            db.SetAdd("myset", "value1");
+            var heapAfterAdd = cacheSizeTracker.mainLogTracker.LogHeapSizeBytes;
+            ClassicAssert.Greater(heapAfterAdd, 0, "Heap should be positive after SADD");
+
+            // Remove the only element — triggers HasRemoveKey → ExpireAndStop → tombstone.
+            var removed = db.SetRemove("myset", "value1");
+            ClassicAssert.IsTrue(removed, "SREM should return true");
+
+            // The tracker should return to zero: the sizeChange delta from SREM (negative) plus
+            // OnDispose(Deleted) subtracting the empty-collection overhead should exactly cancel
+            // the original SADD increment.
+            ClassicAssert.AreEqual(0, cacheSizeTracker.mainLogTracker.LogHeapSizeBytes,
+                "Heap tracker must return to zero after removing the last element (IPU path)");
+        }
+
+        /// <summary>
+        /// Same scenario but with the record in the readonly region, forcing the remove-last-element
+        /// through CopyUpdate → PostCopyUpdater → HasRemoveKey → ExpireAndStop. The PCU path's
+        /// tombstoned new record must not leak value heap because +value is only added on pcuSuccess=true.
+        /// </summary>
+        [Test]
+        public void RemoveLastElementReturnsTrackerToZero_PCU()
+        {
+            ClassicAssert.AreEqual(0, cacheSizeTracker.mainLogTracker.LogHeapSizeBytes);
+
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+
+            // Add a single element to a list.
+            db.ListRightPush("mylist", "value1");
+            var heapAfterAdd = cacheSizeTracker.mainLogTracker.LogHeapSizeBytes;
+            ClassicAssert.Greater(heapAfterAdd, 0, "Heap should be positive after RPUSH");
+
+            // Shift the record to the readonly region so the next mutation goes through CopyUpdate.
+            store.Log.ShiftReadOnlyAddress(store.Log.TailAddress, wait: true);
+
+            // Remove the only element — forces CopyUpdate → PCU → HasRemoveKey → ExpireAndStop.
+            var popped = db.ListLeftPop("mylist");
+            ClassicAssert.AreEqual("value1", (string)popped, "LPOP should return the value");
+
+            // Force eviction to flush any remaining sealed source records.
+            store.Log.FlushAndEvict(wait: true);
+
+            ClassicAssert.AreEqual(0, cacheSizeTracker.mainLogTracker.LogHeapSizeBytes,
+                "Heap tracker must return to zero after removing the last element (PCU path)");
+        }
     }
 }