refactor: internalize CopyUpdated value-object accounting into Tsavorite

The CopyUpdated path is a partial clear (value-object slot only, key stays on
the record). Expecting IRecordTriggers implementers to know this nuance — that
they should subtract only ValueObject.HeapMemorySize and not the full
CalculateHeapMemorySize — is error-prone.

Move the accounting entirely into Tsavorite's InternalRMW:
- Decrement logSizeTracker by ValueObject.HeapMemorySize before clearing
- Call IHeapObject.Dispose() on the freed value object
- Then ClearValueIfHeap() nulls the ObjectIdMap slot

Remove OnDispose(CopyUpdated) from the trigger call site — the trigger is no
longer involved in CopyUpdate accounting. GarnetRecordTriggers.OnDispose now
only handles DisposeReason.Deleted. The IRecordTriggers doc is updated to
reflect that CopyUpdated is handled internally.

The trigger contract is now simple:
- OnDispose(Deleted): subtract CalculateHeapMemorySize (full record heap)
- OnEvict: subtract CalculateHeapMemorySize (whatever remains at eviction)
- Creation sites (PostInitialUpdater/Writer/CopyUpdater, InPlaceUpdater):
  add the new/changed heap

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

Author: badrishc

libs/server/Storage/Functions/GarnetRecordTriggers.cs

@@ -57,26 +57,13 @@ public void OnDispose(ref LogRecord logRecord, DisposeReason reason)
             if (reason == DisposeReason.Deleted)
             {
                 // Decrement the full heap contribution of the dying record (overflow key/value bytes
-                // and/or value-object heap). CalculateHeapMemorySize returns 0 for tombstoned records,
-                // which naturally makes this a no-op on the mutable Delete() path where
-                // MainStore.InPlaceDeleter already subtracted before tombstone was set. On the RMW
-                // expire paths (ExpireAndResume, ExpireAndStop for objects) the tombstone is NOT yet
-                // set when OnDispose fires, so CalculateHeapMemorySize returns the correct non-zero value.
+                // and/or value-object heap). Tombstone is NOT yet set when this fires — Tsavorite sets
+                // it AFTER OnDispose returns — so CalculateHeapMemorySize returns the correct non-zero
+                // value. ClearHeapFields then physically frees the heap fields from the ObjectIdMap.
                 var size = MemoryUtils.CalculateHeapMemorySize(in logRecord);
                 if (size != 0)
                     cacheSizeTracker.AddHeapSize(-size);
             }
-            else if (reason == DisposeReason.CopyUpdated)
-            {
-                // Source record's value-object slot is being cleared in place after a successful
-                // CopyUpdate CAS. The record itself stays alive on the sealed page until eviction,
-                // but the (v) object is about to be released — this is the paired decrement for
-                // PostCopyUpdater's unconditional +value.HeapMemorySize on the (v+1) value.
-                // Checkpoint/disk paths that leave the source alive don't reach this site; their
-                // decrement comes from OnEvict at page eviction.
-                if (logRecord.Info.ValueIsObject)
-                    cacheSizeTracker.AddHeapSize(-logRecord.ValueObject.HeapMemorySize);
-            }
         }
 
         /// <inheritdoc/>

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

@@ -242,8 +242,8 @@ public bool PostCopyUpdater<TSourceLogRecord>(in TSourceLogRecord srcLogRecord,
             sizeInfo.AssertOptionalsIfSet(dstLogRecord.Info);
 
             // Track creation of the (v+1) value unconditionally; the matching decrement for the (v) value is
-            // emitted at the removal site: OnDisposeValueObject(DisposeReason.CopyUpdated) when the source is
-            // cleared eagerly, or OnEvict when the sealed source page later evicts (checkpoint/disk paths).
+            // emitted internally by Tsavorite via logSizeTracker when the source value-object slot is cleared
+            // eagerly (ClearSourceValueObject), or by OnEvict when the sealed source page later evicts.
             functionsState.cacheSizeTracker?.AddHeapSize(value.HeapMemorySize);
 
             if (functionsState.appendOnlyFile != null)

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

@@ -175,8 +175,8 @@ public bool PostCopyUpdater<TSourceLogRecord>(in TSourceLogRecord srcLogRecord,
                 sizeInfo.AssertOptionalsIfSet(dstLogRecord.Info);
 
                 // Track creation of the (v+1) value unconditionally; the matching decrement for the (v) value is
-                // emitted at the removal site: OnDispose(DisposeReason.CopyUpdated) when the source is cleared
-                // eagerly, or OnEvict when the sealed source page later evicts (checkpoint/disk paths).
+                // emitted internally by Tsavorite via logSizeTracker when the source value-object slot is cleared
+                // eagerly (ClearSourceValueObject), or by OnEvict when the sealed source page later evicts.
                 functionsState.cacheSizeTracker?.AddHeapSize(value.HeapMemorySize);
             }
 

libs/storage/Tsavorite/cs/src/core/Index/StoreFunctions/IRecordTriggers.cs

@@ -48,14 +48,19 @@ public interface IRecordTriggers
         bool CallOnDiskRead { get; }
 
         /// <summary>
-        /// Called when a record or one of its heap slots is disposed due to delete, CAS failure,
-        /// copy-update source-slot clearing, or other store-internal reasons. Use <paramref name="reason"/>
-        /// to distinguish record-level events (e.g. <see cref="DisposeReason.Deleted"/>) from
-        /// slot-level events (e.g. <see cref="DisposeReason.CopyUpdated"/>, which signals that only
-        /// the value-object slot is being cleared in place while the record itself remains on the
-        /// sealed page until eviction). If the value object implements <see cref="IDisposable"/> and
-        /// the application needs to release its resources, it should invoke <see cref="IDisposable.Dispose"/>
-        /// from this callback on the appropriate reasons.
+        /// Called when a record is disposed due to delete, expiration, CAS failure, elision,
+        /// revivification, or other store-internal reasons. Use <paramref name="reason"/> to
+        /// distinguish the event type (e.g. <see cref="DisposeReason.Deleted"/> for tombstoning).
+        /// <para>
+        /// For heap-size tracking, <see cref="DisposeReason.Deleted"/> is the primary reason to handle:
+        /// the record's full heap contribution (overflow key/value + value object) can be obtained via
+        /// <c>CalculateHeapMemorySize</c> and should be subtracted from the tracked total. The tombstone
+        /// bit is NOT yet set when this callback fires for Deleted, so the size is correct.
+        /// </para>
+        /// <para>
+        /// Copy-update source-slot clearing (<see cref="DisposeReason.CopyUpdated"/>) is handled
+        /// internally by the store's <c>logSizeTracker</c> and does NOT fire this callback.
+        /// </para>
         /// NOT called for page eviction (use <see cref="OnEvict"/> instead).
         /// NOT called for transient records materialized from disk (use <see cref="OnDisposeDiskRecord"/>).
         /// Default implementation is a no-op.

libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalRMW.cs

@@ -597,13 +597,26 @@ private OperationStatus CreateNewRecordRMW<TKey, TInput, TOutput, TContext, TSes
                         {
                             if (rmwInfo.ClearSourceValueObject && isMemoryLogRecord)
                             {
-                                // Signal the source's value-object slot is being cleared in place (the record
-                                // itself stays alive on the sealed page until eviction). We pass the full
-                                // srcLogRecord + CopyUpdated reason so the application can distinguish this
-                                // slot-level dispose from a record-level Deleted dispose. The actual object
-                                // disposal is handled inside ClearValueIfHeap via ObjectIdMap.Free → IHeapObject.Dispose.
                                 ref var srcMemLogRecord = ref srcLogRecord.AsMemoryLogRecordRef();
-                                storeFunctions.OnDispose(ref srcMemLogRecord, DisposeReason.CopyUpdated);
+
+                                // Decrement the value-object's heap contribution from the tracker and dispose
+                                // the object BEFORE clearing the slot. This is an internal accounting and
+                                // disposal step — the trigger is NOT involved because CopyUpdated is a
+                                // partial clear (value only, key stays) and expecting trigger implementers
+                                // to know that nuance is error-prone. The remaining key overflow (if any)
+                                // is decremented later by OnEvict when the sealed page is evicted.
+                                if (srcMemLogRecord.Info.ValueIsObject)
+                                {
+                                    var valueObject = srcMemLogRecord.ValueObject;
+                                    if (valueObject is not null)
+                                    {
+                                        var valueHeap = valueObject.HeapMemorySize;
+                                        if (valueHeap != 0)
+                                            hlogBase.logSizeTracker?.IncrementSize(-valueHeap);
+                                        valueObject.Dispose();
+                                    }
+                                }
+
                                 srcMemLogRecord.ClearValueIfHeap();
                             }
                         }

libs/storage/Tsavorite/cs/test/RecordLifecycleTests.cs

@@ -233,15 +233,17 @@ private void Upsert(int key, int value)
             _ = s.BasicContext.Upsert(new TestObjectKey { key = key }, new TrackedObjectValue { value = value }, 0);
         }
 
-        #region OnDispose(CopyUpdated)
+        #region CopyUpdate — value-object slot clearing
 
         /// <summary>
         /// RMW on a record in the immutable region forces CopyUpdate. After the CAS succeeds,
-        /// Tsavorite fires <see cref="IRecordTriggers.OnDispose"/> with <see cref="DisposeReason.CopyUpdated"/>
-        /// to let the handler clear the source value slot (paired with PostCopyUpdater's +size add on the destination).
+        /// Tsavorite internally clears the source value-object slot and decrements the logSizeTracker
+        /// for the value-object's heap. The trigger is NOT involved — <see cref="DisposeReason.CopyUpdated"/>
+        /// does not fire via <see cref="IRecordTriggers.OnDispose"/>. The source record stays alive
+        /// (sealed) until eviction, where OnEvict picks up any remaining key overflow.
         /// </summary>
         [Test, Category("TsavoriteKV")]
-        public void CopyUpdateFiresOnDisposeCopyUpdatedExactlyOnce()
+        public void CopyUpdateDoesNotFireOnDisposeCopyUpdated()
         {
             Upsert(1, 100);
             store.Log.ShiftReadOnlyAddress(store.Log.TailAddress, wait: true);
@@ -254,27 +256,25 @@ public void CopyUpdateFiresOnDisposeCopyUpdatedExactlyOnce()
                 _ = s.BasicContext.RMW(new TestObjectKey { key = 1 }, ref input, ref output, 0);
             }
 
-            ClassicAssert.AreEqual(1, tracker.DisposeCount(DisposeReason.CopyUpdated),
-                "OnDispose(CopyUpdated) should fire exactly once after a successful CopyUpdate CAS");
+            ClassicAssert.AreEqual(0, tracker.DisposeCount(DisposeReason.CopyUpdated),
+                "CopyUpdated is handled internally by logSizeTracker — OnDispose must not fire for it");
             ClassicAssert.AreEqual(0, tracker.DisposeCount(DisposeReason.Deleted),
                 "Deleted must not fire on a CopyUpdate path");
-            // The source record survives on the (sealed) page; no eviction yet, so no OnEvict.
             ClassicAssert.AreEqual(0, tracker.TotalEvict(),
                 "No page eviction should have happened in this test window");
         }
 
         /// <summary>
-        /// With <see cref="LifecycleTracker.DisposeValuesOnDispose"/> set, the handler disposes the source
-        /// value object once during CopyUpdated. This asserts that Tsavorite itself never invokes Dispose
-        /// on the on-log value; only the handler does — and it does so at most once per record.
+        /// After CopyUpdate, the source value-object is cleared from the ObjectIdMap (via ClearValueIfHeap).
+        /// Tsavorite calls IHeapObject.Dispose on the freed object internally — the trigger is not involved.
+        /// Verify that exactly one IHeapObject.Dispose fires for the source value object.
         /// </summary>
         [Test, Category("TsavoriteKV")]
-        public void CopyUpdateDisposesSourceValueExactlyOnceWhenHandlerOptsIn()
+        public void CopyUpdateDisposesSourceValueExactlyOnce()
         {
             Upsert(1, 100);
             store.Log.ShiftReadOnlyAddress(store.Log.TailAddress, wait: true);
             tracker.Reset();
-            tracker.DisposeValuesOnDispose = true;
 
             using (var s = NewSession())
             {
@@ -283,8 +283,8 @@ public void CopyUpdateDisposesSourceValueExactlyOnceWhenHandlerOptsIn()
                 _ = s.BasicContext.RMW(new TestObjectKey { key = 1 }, ref input, ref output, 0);
             }
 
-            ClassicAssert.AreEqual(1, tracker.DisposeCount(DisposeReason.CopyUpdated),
-                "OnDispose(CopyUpdated) should fire exactly once");
+            ClassicAssert.AreEqual(0, tracker.DisposeCount(DisposeReason.CopyUpdated),
+                "CopyUpdated must not fire — handled internally");
 
             var (total, max) = TrackedObjectValue.Snapshot();
             ClassicAssert.AreEqual(1, total, "Exactly one IHeapObject.Dispose call expected (the CU source object)");