Skip to content

Commit 0358eea

Browse files
badrishcCopilot
andcommitted
fix: visit sealed source records during eviction to prevent overflow heap leak
EvictRecordsInRange previously skipped all SkipOnScan records (Sealed OR Invalid). This leaked tracked heap for sealed-but-valid source records from mutable-region CopyUpdates and immutable-region deletes, whose overflow key/value bytes were never decremented from the tracker. Fix: skip only Invalid records (already disposed/elided) and Tombstoned records (already decremented at delete site). Sealed-but-Valid records are now visited by OnEvict, which correctly picks up their remaining heap contribution (overflow bytes, and value objects kept alive during checkpoint). For records where OnDispose(Deleted) already cleared the value (immutable delete), CalculateHeapMemorySize returns only key overflow (if any) — no double-decrement because OnDispose decremented the full amount when the record was not yet tombstoned, and ClearHeapFields(clearKey=false) zeroed the value slot afterward. OnEvict sees only what remains (key overflow if present, typically 0 for inline keys). Updated PageEvictionFiresOnEvictForEveryLiveRecord test to use bounds assertions reflecting that sealed source records from immutable-region deletes are now correctly visited. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2e89f39 commit 0358eea

2 files changed

Lines changed: 33 additions & 13 deletions

File tree

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -377,11 +377,19 @@ internal void EvictRecordsInRange(long startAddress, long endAddress, EvictionSo
377377
if (offset == 0 || offset + allocatedSize > PageSize)
378378
break;
379379

380-
// Skip null, closed/sealed (SkipOnScan = Invalid | Sealed), and tombstoned records. Tombstoned records
381-
// were already disposed with DisposeReason.Deleted at the delete site; SkipOnScan records either copied
382-
// their heap contribution forward (CopyUpdater) and had it netted at the copy site, or are otherwise
383-
// not responsible for the allocator's heap counter.
384-
if (logRecord.Info.IsNull || logRecord.Info.SkipOnScan || logRecord.Info.Tombstone)
380+
// Skip null, invalid, and tombstoned records:
381+
// - IsNull: empty record slot, nothing to account for.
382+
// - Invalid: record was elided from the hash chain and already disposed
383+
// (OnDispose(Deleted/Elided) or transferred to the revivification freelist).
384+
// - Tombstone: heap was already decremented at the delete site via
385+
// OnDispose(Deleted) or InPlaceDeleter.
386+
//
387+
// Sealed-but-Valid records are NOT skipped. A Sealed source record from a
388+
// mutable-region CopyUpdate still owns its overflow key/value bytes (and
389+
// possibly its value object if a checkpoint prevented eager clearing via
390+
// ClearSourceValueObject). Skipping them would leak their heap contribution
391+
// in the tracker.
392+
if (logRecord.Info.IsNull || logRecord.Info.Invalid || logRecord.Info.Tombstone)
385393
{
386394
address += allocatedSize;
387395
continue;

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,11 @@ public void PendingReadFromDiskFiresOnDisposeDiskRecordOnce()
458458

459459
/// <summary>
460460
/// Filling the log well beyond its mutable window forces page eviction. OnEvict must fire for
461-
/// every non-tombstoned record evicted past HeadAddress. Tombstoned records short-circuit to 0
462-
/// heap contribution and must NOT cause double accounting through both OnDispose(Deleted) and OnEvict.
461+
/// every non-tombstoned, non-invalid record evicted past HeadAddress — including sealed source
462+
/// records from immutable-region deletes or CopyUpdates. Tombstoned records are skipped (heap
463+
/// was decremented at the delete site). Invalid/elided records are skipped (heap was already
464+
/// cleaned up). Sealed records are visited because they may still own overflow key/value bytes
465+
/// or value objects (checkpoint path).
463466
/// </summary>
464467
[Test, Category("TsavoriteKV")]
465468
public void PageEvictionFiresOnEvictForEveryLiveRecord()
@@ -479,14 +482,23 @@ public void PageEvictionFiresOnEvictForEveryLiveRecord()
479482
"Each Delete should fire OnDispose(Deleted) exactly once");
480483
var deletedDisposeCountBeforeEvict = tracker.DisposeCount(DisposeReason.Deleted);
481484

482-
// Force all records out to disk. This triggers OnEvict for every non-tombstoned page record.
485+
// Force all records out to disk. This triggers OnEvict for every non-tombstoned,
486+
// non-invalid record on each evicted page.
483487
store.Log.FlushAndEvict(wait: true);
484488

485-
// Precise count: every live (non-tombstoned) record must fire OnEvict exactly once.
486-
// The 'deleted' tombstones were counted via OnDispose(Deleted) above and are explicitly
487-
// skipped by ObjectAllocatorImpl.EvictRecordsInRange, so the eviction-side count is n - deleted.
488-
ClassicAssert.AreEqual(n - deleted, tracker.EvictCount(EvictionSource.MainLog),
489-
$"OnEvict(MainLog) must fire exactly once per live record ({n - deleted}), not for tombstones");
489+
// OnEvict fires for: (1) the n live records that were never deleted, plus (2) sealed
490+
// source records from immutable-region deletes (these are not tombstoned — the tombstone
491+
// is on a NEW record at the tail). In-place (mutable) deletes set tombstone directly on
492+
// the source and are skipped. The exact split depends on the mutable-vs-readonly boundary
493+
// at delete time, so we assert bounds rather than an exact count:
494+
// - Lower bound: at least n - deleted (the live records)
495+
// - Upper bound: at most n (if all deletes hit the readonly region, producing sealed sources)
496+
var mainLogEvicts = tracker.EvictCount(EvictionSource.MainLog);
497+
ClassicAssert.GreaterOrEqual(mainLogEvicts, n - deleted,
498+
$"OnEvict(MainLog) must fire for at least the {n - deleted} non-deleted records");
499+
ClassicAssert.LessOrEqual(mainLogEvicts, n,
500+
$"OnEvict(MainLog) must not fire more than {n} times (n original + sealed sources, minus tombstones)");
501+
490502
ClassicAssert.AreEqual(0, tracker.EvictCount(EvictionSource.ReadCache),
491503
"No read cache is configured, OnEvict(ReadCache) must never fire");
492504
// Tombstoned records must NOT produce a paired OnDispose(Deleted) during eviction; the

0 commit comments

Comments
 (0)