Fix checkpoint failure when serializing a concurrently-mutated object (#2101)

[Ted Hart (TedHartMS)]

Fixes #2101.

The failure

A snapshot checkpoint throws InvalidOperationException: Collection was modified after the enumerator was instantiated out of ListObject.DoSerialize. The exception escapes the flush, so the checkpoint never completes and the pipeline wedges permanently: every later BGSAVE returns ERR checkpoint already in progress, a core stays pinned, and the AOF grows without bound.

Root cause

Clone() is a shallow copy, so a CopyUpdate leaves the (v) and (v+1) records sharing one LinkedList<byte[]>.

1. During checkpoint C1, an RMW on key K hits CPR and does an RCU: record A (v) → record B (v+1). A.CacheSerializedObjectData captures A's bytes and sets its phase to SERIALIZED; C1 serializes A from those cached bytes, which is correct. B is (v+1), so the snapshot excludes it.
2. C1 completes and post-checkpoint cleanup calls Log.ClearSerializedObjectData(...), which freed the cached bytes and reset the phase to REST.
3. A is still Sealed-but-Valid in the log, and still points at the list B keeps mutating.
4. In C2, A is below the fuzzy-region start, so the snapshot includes it. Its phase is REST, so Serialize() takes the direct path and enumerates the live shared list while a client mutates it.

Changes

Change 1 is the fix for #2101 itself. Auditing the surrounding code for the same class of problem — an object being serialized by the flush while something else mutates or frees it — turned up four further issues, each independently reachable, which are fixed here as well.

1. Keep a superseded object's serialization phase terminal (HeapObjectBase)

ClearSerializedObjectData now releases the cached bytes but leaves the phase SERIALIZED. SERIALIZED with null bytes is exactly the state Serialize() already documents as "superseded after checkpoint completion": it writes the null indicator. That is safe because bytes are only ever cached for a source a CopyUpdate has just superseded, so a higher-address record always carries the live data.

Serialization is also made exception-safe. A DoSerialize that throws used to strand the object in SERIALIZING, where every later Serialize() and CacheSerializedObjectData() spins on it forever — a second, independent way to wedge the checkpoint pipeline. The direct path now restores REST in a finally, and the caching path discards partial bytes and restores REST before rethrowing.

2. Stop a read-path command from mutating a live object (HashTimeToLive)

HashObjectImpl.HashTimeToLive called DeleteExpiredItems(), mutating hash, expirationTimes and expirationQueue. But HashOps.HashTimeToLive dispatches through ReadObjectStoreOperation, so it runs under a shared lock and could restructure the dictionary while the flush path enumerated it.

The purge is removed. Responses are unchanged: ContainsKey/GetExpiration already report expired fields as absent (-2). This also makes HTTL consistent with SortedSetTimeToLive, the sibling command on the same read dispatch that never purged, and with Hash's own read accessors. Reclamation is unchanged in kind and now symmetric with sorted sets — mutating ops, HCOLLECT/ZCOLLECT, or the background collector.

3. Do not dispose a source record a checkpoint has frozen

OnDispose(..., Deleted) clears the record's heap fields, which returns the value's ObjectIdMap slot to that page's free list for reuse. The snapshot flush reads object ids from its page copy but resolves them against the live map, so disposing a frozen record lets the flush serialize a freed — or recycled and therefore unrelated — object.

A new OnDisposeSupersededSource skips disposal while the record is frozen; the value is released at page eviction instead, which accounts for it correctly. Applied to the four RMW expiration paths and the Delete path.

4. Keep reused allocations above the checkpoint's fuzzy-region start

The IsInV1 clamp on minRevivAddress only covered the free-record pool. An allocation saved before the version shift and retried after CPR_SHIFT_DETECTED could place a (v+1) record below the fuzzy start, where the snapshot includes it regardless of its version bit, leaking (v+1) data into the checkpoint image. The clamp now covers both reuse paths.

5. Elide the source record on Delete

CreateNewRecordDelete already passed elideSourceRecord to the allocator, which relaxes minRevivAddress on the premise that the source leaves the tag chain — but it never actually elided it, so that relaxed bound was unsound. The tombstone now carries the source's PreviousAddress, and the source is invalidated and freelisted, as CreateNewRecordUpsert and CreateNewRecordRMW do.

Unlike those two, Delete fires OnDispose(Deleted) before eliding. Garnet keys range-index data-file cleanup and Vector Set deletion off that reason, so a literal copy of the Upsert path would have silently skipped both. The subsequent elide/freelist disposal only accounts for the key, so nothing is counted twice.

6. State machine documentation

Adds website/docs/dev/tsavorite/state-machine.md, describing the checkpoint state machine top-down: defining and launching a machine, each transition, the transition-in/transition-out handles, and the waiting list, including two current limitations. Wired into the sidebar and linked from the Tsavorite intro. The accompanying StateMachineDriver.cs changes are comments only — no behavior change.

Tests

#2101 reproduction — CheckpointSharedObjectMutationTests.SupersededObjectIsNotSerializedFromLiveStateAfterCleanup.
This is the test that demonstrates the reported failure. It drives the production path end to end: a Snapshot
checkpoint is paused on entry to WAIT_FLUSH via IStateMachineCallback, a real RMW issued in that window is
forced by CPR through InternalRMW → CreateNewRecordRMW → CacheSerializedObjectData, and the value's Clone()
is a shallow copy sharing the source's List, exactly as Garnet's collection objects do. Post-checkpoint
cleanup then runs, the surviving record mutates the shared list, and the superseded object is serialized again
with a second mutation landing mid-enumeration. Against the pre-fix allocator it fails with the issue's own
exception, raised from the same direct-serialize path:

InvalidOperationException: Collection was modified; enumeration operation may not execute.

Serialization-phase unit tests — ObjectLogScanTests.

• CleanupOfCachedDataLeavesObjectTerminal — asserts the phase invariant directly: after cleanup releases the
  cached bytes, the object must stay terminal and Serialize() must write the null indicator. It calls
  CacheSerializedObjectData directly and its test object holds no collection, so it pins the invariant cheaply
  but does not reproduce Checkpoint fails with InvalidOperationException in ListObject.DoSerialize under concurrent LIST mutation, then never completes ("checkpoint already in progress") #2101; that is what the test above is for.
• FailedSerializationRestoresRestPhase — a throwing DoSerialize on the direct path must leave the object in REST.
• FailedCachedSerializationRestoresRestPhase — the same guarantee for the CopyUpdate capture path, which has its
  own catch: the exception is rethrown, REST is restored, and no partial capture is published.

All four fail against the exact pre-fix code with their intended diagnostics.

RecordLifecycleTests.PageEvictionFiresOnEvictForEveryLiveRecord was updated: it assumed delete sources stay in
the tag chain and are visited by eviction, and now derives the expected eviction count from the number of sources
actually elided.

Status

Item	State
#2101 root cause fixed (HeapObjectBase phase lifecycle)	✅
#2101 reproduced end to end, fails without the fix	✅
Serialization exception-safety, both call paths	✅
HTTL read-path mutation (gap 1)	✅
Frozen-source disposal (gap 2)	✅
Fuzzy-start clamp for reused allocations (gap 3)	✅
Delete source elision	✅
State machine documentation	✅
Merged with main, conflict-free	✅
Review threads	none unresolved

Remaining work items

Two review comments are accurate and are not closed by this PR. Both paths are guarded by Ctx.IsInV1, so
they are inert outside a checkpoint, and neither is reached by existing lifecycle tests (which shift read-only
while in REST):

1. BlockAllocate.cs fuzzy-start clamp has no targeted test. Its failure mode is an allocation saved before
   the version shift, retried after CPR_SHIFT_DETECTED, and reused below startLogicalAddress. Reaching it
   requires forcing a retry across a version boundary; the PauseAtPhase harness added here makes that feasible.
2. Helpers.OnDisposeSupersededSource frozen branch is unexercised. Note that the new end-to-end test does
   not cover it: that helper is called only from the expiration paths, so a non-expiring RMW during a
   checkpoint never reaches it. A test needs an RMW returning ExpireAndStop/ExpireAndResume against a frozen
   source while a checkpoint is in the fuzzy region.

Neither is a correctness regression; both are coverage gaps on code this PR adds. Flagging rather than silently
assuming the new checkpoint test covers them.

A third review comment — that removing DeleteExpiredItems() from HTTL leaks memory without bound — was
investigated and rebutted; see the analysis comment.
In short, a field only becomes expirable through HEXPIRE/HPEXPIRE, which is an RMW that purges first, so the
unpurged set cannot exceed the fields carrying a TTL at the last write and a read-only workload cannot grow it.
The bounded residual matches SortedSetTimeToLive, which has never purged.

Validation

Tsavorite: recovery 240, root 342, recordops 220, hlog 593, session 155, session.context 133, epoch 15.
Garnet: collections 780, rangeindex 62, vectorset 392. dotnet format --verify-no-changes clean on both solutions.

End-to-end with the reporter's repro (revivification plus delete/recreate churn): 88/88 BGSAVEs completed, no
exceptions in the server log, no stall, over 2.5M mutations. The same workload reproduced the failure in roughly
55 seconds before the fix.

Checkpoint-only recovery with AOF disabled: 10 lists / 176,212 entries byte-identical before and after recovery.

[Copilot]

🟡 Changes recommended

Unresolved correctness concerns and missing regression coverage remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request fixes checkpoint failures caused by concurrent object mutation and hardens related Tsavorite record lifecycle behavior.

Changes:

• Makes serialization exception-safe and terminal.
• Protects frozen records, allocation bounds, and delete elision.
• Prevents Hash TTL reads from mutating shared state.
• Adds regression tests and state-machine documentation.

File summaries

File	Summary
website/sidebars.js	Registers the state-machine documentation.
website/docs/dev/tsavorite/state-machine.md	Adds state-machine documentation.
website/docs/dev/tsavorite/intro.md	Links to the new documentation.
libs/storage/Tsavorite/cs/test/test.recovery/ObjectLogScanTests.cs	Adds serialization regression coverage.
libs/storage/Tsavorite/cs/test/test.recordops/RecordLifecycleTests.cs	Updates eviction lifecycle expectations.
libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalRMW.cs	Protects frozen records during expiration RMWs.
libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalDelete.cs	Implements safe delete-source elision.
libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/Helpers.cs	Handles frozen-source disposal safely.
libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/BlockAllocate.cs	Enforces checkpoint-safe allocation bounds.
libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/StateMachineDriver.cs	Clarifies state-machine behavior in comments.
libs/storage/Tsavorite/cs/src/core/Allocator/HeapObjectBase.cs	Hardens serialization state transitions and cleanup.
libs/server/Objects/Hash/HashObjectImpl.cs	Prevents mutation during Hash TTL reads.

Review details

Suppressed comments (4)

libs/server/Objects/Hash/HashObjectImpl.cs:461

• Removing DeleteExpiredItems() fixes the shared-lock mutation, but it also removes the only automatic cleanup for a read-only hash. With ExpiredObjectCollectionFrequencySecs disabled by default and no later mutating operation or HCOLLECT, expired entries remain in hash, expirationTimes, and expirationQueue indefinitely; HTTL merely hides them in responses, so repeated field TTLs can grow the object's memory without bound. Please provide a write-safe way to schedule/perform equivalent reclamation rather than dropping it entirely.

            // This is a read operation, so it must not mutate the object: it runs under a shared lock and may execute
            // while the flush path is serializing this same instance. Expired fields are reported as absent by
            // ContainsKey (via GetExpiration) without being removed; the mutating paths purge them from the live object.

libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/BlockAllocate.cs:71

• The new fuzzy-start clamp has no regression test for the failure mode it addresses. Existing revivification tests cover ordinary free-record reuse and snapshots after deletes, but not an allocation saved before the version shift and retried after CPR_SHIFT_DETECTED; without that interleaving, a reused v+1 record can still land below startLogicalAddress and leak into the snapshot. Add a deterministic retry/free-list test that asserts the snapshot excludes the v+1 value.

            // A (v+1) record must never land below the checkpoint's fuzzy-region start: the snapshot includes every record
            // below that address regardless of its version bit, so reusing a lower address would leak (v+1) data into the
            // checkpoint image. This applies to both reuse paths below, including an allocation that was saved before the
            // version shift and is only now being retried as (v+1).
            if (sessionFunctions.Ctx.IsInV1)
            {
                var fuzzyStartAddress = _hybridLogCheckpoint.info.startLogicalAddress;
                if (fuzzyStartAddress > minRevivAddress)
                    minRevivAddress = fuzzyStartAddress;

libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/Helpers.cs:126

• The new frozen-source branch is not exercised by the current lifecycle tests: those tests only call ShiftReadOnlyAddress while the session remains in REST, so Ctx.IsInV1 is false; none overlaps an RMW/delete expiration with an active checkpoint. This is the safety property that prevents a snapshot from resolving a freed or recycled ObjectIdMap slot, so add a deterministic checkpoint-overlap regression test and verify the recovered object data.

            if (IsFrozen<TInput, TOutput, TContext, TSessionFunctionsWrapper>(sessionFunctions, ref stackCtx, logRecord.Info))
                return;

libs/storage/Tsavorite/cs/test/test.recovery/ObjectLogScanTests.cs:327

• This does not exercise the regression path described by the test: it manually marks a wrapper record as new and calls CacheSerializedObjectData directly, without running an RMW/CopyUpdate or a checkpoint flush while the cloned value is being mutated. It therefore cannot catch failures in the actual CAS/phase transitions or the concurrent serialization that caused #2101. Please add an end-to-end test that performs a real CopyUpdate during an active checkpoint and mutates the resulting shared value while the snapshot is flushed.

            var logRecord = store.hlogBase._wrapper.CreateLogRecord(beginAddress);
            logRecord.InfoRef.SetIsInNewVersion();
            RMWInfo rmwInfo = default;
            value.CacheSerializedObjectData(ref logRecord, ref rmwInfo, srcIsOnMemoryLog: true);

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Ted Hart (TedHartMS)]

Blocker analysis

Addressing the two CI failures and the four review comments. Summary: one review comment was correct and has been fixed, one rests on a premise that does not hold, and both CI failures are demonstrably not regressions from this diff.

---

1. ObjectLogScanTests.cs:327 does not reproduce #2101 — correct, and fixed

This was the most important comment and it is right.

CleanupOfCachedDataLeavesObjectTerminal calls CacheSerializedObjectData directly on a manually-constructed log record. It never enters InternalRMW, no checkpoint is active, and CountingSerializationHeapObject holds no collection — so it cannot produce "Collection was modified" under any scheduling. It is a valid unit test of the serialization-phase invariant (it does fail without the fix, with "Cleanup returned a superseded object to REST"), but it is not a reproduction of the reported bug.

The real call site is InternalRMW.cs:610, inside CreateNewRecordRMW after a successful CAS.

Added CheckpointSharedObjectMutationTests.SupersededObjectIsNotSerializedFromLiveStateAfterCleanup, which drives the actual path:

1. Upsert a key whose value holds a List<int>; Clone() is a shallow copy sharing that list, exactly as Garnet's collection objects do.
2. Start a Snapshot checkpoint and pause the state machine on entry to WAIT_FLUSH via IStateMachineCallback, so IN_PROGRESS is published and the fuzzy region is open.
3. Issue a real RMW in that window. CPR forces the copy-update, so this runs InternalRMW → CreateNewRecordRMW → CacheSerializedObjectData on the superseded (v) object, and the clone shares its list.
4. Release the state machine and complete the checkpoint.
5. Run Log.ClearSerializedObjectData(...), as DatabaseManagerBase.RunPostCheckpointCleanup does.
6. Mutate the shared list through the surviving record, then serialize the superseded object again, with a second mutation landing mid-enumeration.

Against the pre-fix allocator (HeapObjectBase.cs restored from 78f361812~1) this fails with both assertions:

#2101: the superseded object was re-serialized from its live, shared collection after cleanup
#2101: serializing the superseded object threw InvalidOperationException:
       Collection was modified; enumeration operation may not execute.

That is the exception from the issue, raised from the same direct-serialize path. With the fix, 1/1 passes.

The original unit test is kept — it pins the phase invariant directly and is much cheaper — but it is no longer the only evidence.

---

2. HashObjectImpl.cs:461 — retention is bounded, not unbounded

The comment states that repeated field TTLs can grow the object's memory without bound. That premise does not hold, because setting a TTL is itself a purging write.

A field can only become expirable through HEXPIRE/HPEXPIRE → HashObjectImpl.HashExpire (line 438), which is dispatched as an RMW and calls DeleteExpiredItems() before doing anything else. Every one of the six purge sites is on a write path:

Method	Line	Dispatch
HashSet	187	RMW
HashCollect	245	RMW
HashIncrement	303	RMW
HashIncrementFloat	378	RMW
HashExpire	438	RMW
HashPersist	506	RMW

So the expired-but-unpurged set can never exceed the fields that were carrying a TTL at the last mutating operation, and a read-only workload cannot grow it: no writes, no new TTLs, no new expirations beyond that fixed set. It is reclaimed by the next mutating operation, by HCOLLECT, or by the background collector.

The accurate residual statement is: a hash whose fields expire after its last write retains that bounded memory until one of those three events. That is precisely the behavior SortedSetTimeToLive (ZTTL) has always had — it filters with IsExpired and never purges — so this change makes HTTL consistent with its sibling rather than introducing a new class of retention.

Keeping the fix. Mutating hash, expirationTimes and expirationQueue under a shared lock while the flush path enumerates the same instance is a data-corruption bug; trading it for bounded, reclaimable retention that matches the sorted-set command is the right call. The analysis is now recorded as a comment at the call site so the tradeoff is explicit rather than implied.

---

3. CI failures on 598838c7 (run 35154807588) — neither is a regression

RespTests.MultipleClientsUnblockAndAddTest(20) (ubuntu, net8.0, Debug)

Fails at RespTests.cs:5502, ClassicAssert.IsTrue(blockingResult.StartsWith("-UNBLOCKED")).

The decisive detail is which assertion failed. Line 5498 — AreEqual(numberOfItems - numberOfItemsReturned, listLength) — passed. Data integrity was correct. The failure is on the following line, about which of three concurrent CLIENT UNBLOCK calls raced 20 concurrent ListLeftPush calls against one BLMPOP, i.e. purely the blocking-manager outcome.

Reachability against this diff: the test uses BLMPOP, LPUSH, LLEN, CLIENT UNBLOCK. No DELETE, no TTL, no checkpoint.

• HeapObjectBase — flush/checkpoint only; neither occurs.
• HashObjectImpl.HashTimeToLive — HTTL only; no hashes.
• InternalRMW — all four edits are inside RMWAction.ExpireAndStop/ExpireAndResume; no TTLs are set, so no expiration action is ever returned.
• BlockAllocate — the clamp is guarded by sessionFunctions.Ctx.IsInV1; no checkpoint.
• Helpers.OnDisposeSupersededSource — returns early only when IsFrozen, which requires Ctx.IsInV1; with no checkpoint it calls OnDispose(Deleted), identical to the previous code.
• InternalDelete — the only edit reachable in principle, and the passing length assertion shows the chain stayed correct.

Locally: 5/5 passes on this branch (net8.0 Debug). #2110 ("Fix root causes behind recurring Garnet .NET CI flakes"), merged after this branch's last merge, reworks GarnetServerTcp.cs and GarnetClientSession.cs, which is where this test's client/connection behavior lives.

ClusterVectorSetTests.MultipleReplicasWithVectorSetsAndDeletesAsync (windows, net8.0, Release)

Fails in WaitForReplicaAofSync: [127.0.0.1:7600]: 1,588088 != [127.0.0.1:7603]: 1,574228 — the replica was ~14 KB of AOF behind the primary when the backoff window expired. That is a sync-throughput timeout, not a correctness assertion; nothing in this diff touches AOF, replication or gossip.

Locally: 3/3 passes on this branch (net8.0 Release). #2110 rewrote exactly this path — VectorManager.Replication.cs (+171), VectorManager.cs (+143), AofSyncTask.cs, AofSyncDriver.cs, Gossip.cs — and adjusted the replication sync tests themselves.

I treated this one as a real suspect rather than assuming flakiness, because the test name contains "Deletes" and this PR changes the Delete path. The Delete change elides the source record from the tag chain; it does not alter what is written to the AOF, which records the DELETE command. The failure is offset lag, not divergence.

This branch is deliberately still BEHIND main and has not picked up #2110. It is also staying behind while main is red for the Tsavorite build (#2161), to avoid importing four inherited Build Tsavorite failures that would obscure this PR's own signal.

---

4. Remaining coverage gaps — acknowledged, not closed

Two review comments are accurate and I am not claiming otherwise:

• BlockAllocate.cs:71 — the fuzzy-start clamp has no test for its specific failure mode (an allocation saved before the version shift, retried after CPR_SHIFT_DETECTED, reused below startLogicalAddress). Reaching it requires forcing a retry across a version boundary, which the new state-machine pause harness now makes feasible.
• Helpers.cs:126 — the frozen-source branch is still unexercised. The new end-to-end test performs an RMW during an active checkpoint, but OnDisposeSupersededSource is only called from the expiration paths, so a non-expiring RMW does not reach it. A test needs an RMW returning ExpireAndStop/ExpireAndResume against a frozen source while a checkpoint is in the fuzzy region.

Both are guarded by Ctx.IsInV1 and are therefore inert outside a checkpoint; neither is exercised by existing lifecycle tests, which shift read-only while in REST.

---

Validation

Tsavorite.test.recovery 212 passed (incl. the new test), Tsavorite.test.recordops 220, Garnet.test.collections 780. dotnet format --verify-no-changes clean on both solutions. Every run checked for error CS first, since a compile failure makes dotnet test silently run a stale assembly.