Fix live-object mutation and scan liveness defects (#2092) - #2095
Fix live-object mutation and scan liveness defects (#2092)#2095Badrish Chandramouli (badrishc) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes live-object mutation races in collection commands, preserves serialization safety, and prevents scans from returning deleted keys.
Changes:
- Routes list, set, and blocking collection mutations through RMW operations.
- Makes hash and sorted-set serialization non-mutating.
- Corrects scan liveness detection and adds regression coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
libs/server/Storage/Session/ObjectStore/ListOps.cs |
Uses RMW for list moves. |
libs/server/Storage/Session/ObjectStore/SetOps.cs |
Uses RMW for set moves. |
libs/server/Objects/ItemBroker/CollectionItemBroker.cs |
Uses RMW for blocking operations and adds destination notifications. |
libs/server/Objects/Hash/HashObject.cs |
Serializes expired fields without mutation. |
libs/server/Objects/SortedSet/SortedSetObject.cs |
Serializes expired members without mutation. |
libs/storage/Tsavorite/cs/src/core/Allocator/AllocatorScan.cs |
Filters unreachable records during scans. |
libs/storage/Tsavorite/cs/test/test.hlog/SpanByteIterationTests.cs |
Covers bounded-scan RCU behavior. |
test/standalone/Garnet.test/RespScanCommandsTests.cs |
Covers deleted-key scan regressions. |
test/standalone/Garnet.test.collections/RespListTests.cs |
Covers LMOVE persistence and TTL behavior. |
test/standalone/Garnet.test.collections/RespSetTest.cs |
Covers SMOVE persistence and wrong types. |
test/standalone/Garnet.test.collections/RespBlockingCollectionTests.cs |
Covers blocking-operation persistence and notifications. |
test/standalone/Garnet.test.collections/GarnetObjectTests.cs |
Verifies serialization does not mutate objects. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the review comment, plus a test-quality fix it surfaced. 1. Root-caused past the reported symptom.
The first row does not involve the flagged fast path at all, so notifying from that path would have fixed only half the defect. The observer queue is now drained until the collection can no longer serve the observer at the head; that observer is left queued rather than skipped, keeping the queue FIFO and removing a latent infinite loop in the old New tests, all three verified failing on the pre-fix build: 2. Flaky serialization tests (
Status: 226 checks passing, 0 failing. One unrelated job, |
Address review feedback on #2095. Comment the expiration state on HashObject and SortedSetObject -- the two object types that carry expirationTimes/expirationQueue -- describing what each structure holds and how they interact: they are allocated and torn down together, expirationTimes is the source of truth, and expirationQueue entries are ordering hints that may go stale because PriorityQueue cannot update a priority in place. Assert in ConditionalScanPush that a record whose key has an empty HashBucket is Invalid. Closed records only reach this path when the iterator was created with includeClosedRecords, which ScanCursor does when maxAddress is bounded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
CI status note for a7316af: the run shows red, but every The first attempt did have one genuine test failure,
Local Debug validation for this commit: |
Address review feedback on #2095. Comment the expiration state on HashObject and SortedSetObject -- the two object types that carry expirationTimes/expirationQueue -- describing what each structure holds and how they interact: they are allocated and torn down together, expirationTimes is the source of truth, and expirationQueue entries are ordering hints that may go stale because PriorityQueue cannot update a priority in place. Assert in ConditionalScanPush that a record whose key has an empty HashBucket is Invalid. Closed records only reach this path when the iterator was created with includeClosedRecords, which ScanCursor does when maxAddress is bounded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a7316af to
8bc6db7
Compare
Address review feedback on #2095. Comment the expiration state on HashObject and SortedSetObject -- the two object types that carry expirationTimes/expirationQueue -- describing what each structure holds and how they interact: they are allocated and torn down together, expirationTimes is the source of truth, and expirationQueue entries are ordering hints that may go stale because PriorityQueue cannot update a priority in place. Assert in ConditionalScanPush that a record whose key has an empty HashBucket is Invalid. Closed records only reach this path when the iterator was created with includeClosedRecords, which ScanCursor does when maxAddress is bounded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8bc6db7 to
6fdd4e6
Compare
|
Followed up on the
The test builds its
The fix sets Evidence: over 60 runs of the test's parameter set, 2 failures before the change and 0 after (control isolated to just this line), plus 5 consecutive clean full-suite runs (580 passed each). Separately, while chasing this I found my earlier post-rebase hlog numbers were run against a stale binary -- |
a12c501 to
113d7e3
Compare
Keys intermittently and permanently became Strings holding a 4-byte 0xFF FF FF FF payload, after which every write returned WRONGTYPE. Root cause: StorageSession.ListMove called GET to obtain the IGarnetObject and then mutated the returned heap object in place. That is only legal when the record is in the mutable region; otherwise it races the object log writer, which throws "Collection was modified" inside WriteAsync. The page write is then never issued, FlushedUntilAddress stalls, and the log wedges. On restart the newest persisted record is an older CopyUpdate source that was legitimately cleared to an inline ObjectIdMap.InvalidObjectId, which reads back as a valid String -- hence the permanent WRONGTYPE. Auditing every object operation found the same anti-pattern in exactly two other places, plus four further defects: - ListMove (LMOVE/RLMOVE), SetMove (SMOVE) and CollectionItemBroker (all blocking collection commands) mutated live heap objects. All three now route through RMW. - SetMove destroyed the member when the destination held a wrong type; it now returns WRONGTYPE. - Same-key singleton LMOVE/BLMOVE discarded the key TTL. The no-op fast path now also covers Count == 1. - BLMOVE did not wake a client blocked on the destination key. The broker event is now enqueued after txnManager.Commit, keeping it lock-free. - SortedSetObject and HashObject mutated their expirationTimes collection during DoSerialize, so a concurrent reader could observe a torn structure. DoSerialize is now a two-pass pure read. Both passes share one timestamp and one snapshot of the expirationTimes reference, since the element count is written before the entries and cannot be back-patched. Emitted bytes are unchanged. Separately, KEYS could list a deleted key. ScanLookup's dedup step (ConditionalScanPush) is a recency test -- "is a newer version of this key reachable from the tag chain below maxAddress?" -- not a liveness test. A key whose chain has been fully elided has no chain left to search, so the test correctly finds nothing newer and pushes a record that is still physically present in the log. Deletion normally hides a key via a tombstone sitting above the old record, but elision removes precisely that tombstone. CanElide requires the record to be the sole member of its tag chain (hei.Address == recSrc.LogicalAddress and PreviousAddress < BeginAddress), and TryElide then CASes the bucket slot to kInvalidAddress. So an elided chain is exactly an empty slot, and FindTag failing (OperationStatus. NOTFOUND, which is set only on that path) is an exact test for "this key is unreachable". ConditionalScanPush now skips those records. This keys off the reachability of the key rather than of the record, so it is safe for point-in-time consumers: if the key was re-inserted above maxAddress, as during slot migration, the chain is non-empty and the record is still pushed. Filtering on RecordInfo.Invalid instead would be a per-record test and silently drops keys during slot migration, since Invalid marks the elided record while the key remains live at a higher address. Tests added, each verified to fail without the corresponding fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A collection-update event served at most one blocked observer, even when the collection still held items for the waiters behind it. Pushing three elements in one LPUSH woke a single BLPOP client and stranded the other two along with two available items, and a no-op same-key BLMOVE woke one client while leaving the item in place for the rest. Continue draining the observer queue after a successful assignment, stopping when the collection can no longer serve the observer at the head. That observer is left queued rather than skipped, so the queue stays FIFO and every iteration either dequeues an observer or breaks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The expiring member was given a 200 ms TTL when its bytes were built, but the deserializing constructor drops members that have already expired. A stalled CI machine could take longer than that to reach the constructor, leaving an object with no expirable items and failing the test. It failed this way on windows-latest/net8.0/Debug. Build the object inside a retry that widens the window until the expiring member survives construction, then wait for the member's own expiration timestamp instead of sleeping a fixed amount. HashObject.HasExpirableItems becomes internal so the hash test can use the same signal as the sorted set one, which also lets it assert that serialization leaves the expiration structures intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback on #2095. Comment the expiration state on HashObject and SortedSetObject -- the two object types that carry expirationTimes/expirationQueue -- describing what each structure holds and how they interact: they are allocated and torn down together, expirationTimes is the source of truth, and expirationQueue entries are ordering hints that may go stale because PriorityQueue cannot update a priority in place. Assert in ConditionalScanPush that a record whose key has an empty HashBucket is Invalid. Closed records only reach this path when the iterator was created with includeClosedRecords, which ScanCursor does when maxAddress is bounded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CommitRecordBoundedGrowthTest built its TsavoriteLog without setting TryRecoverLatest, which defaults to true, so the constructor ran a synchronous recovery. The test always starts from an empty log and never intends to recover. MethodTestDir is keyed only on the test method name, so every parameterization of this test shares one directory. A commit written late by the previous parameterization can land there after BaseSetup has cleaned it, and recovering that commit onto this run's fresh, empty device fails with "Error reading page 0 from device". Setting TryRecoverLatest = false makes syncRecover false, so the constructor never calls RecoverAsync and the failing path is unreachable. This matches the other two tests in the fixture, which already disable recovery. Measured on 60 runs of the test's parameter set: 2 failures before, 0 after, plus 5 clean full-suite runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
113d7e3 to
6e83d6e
Compare
Fixes #2092
The reported symptom
List keys intermittently and permanently became Strings holding a 4-byte
0xFF FF FF FFpayload. Every subsequent write returnedWRONGTYPE, and the condition survived restart.Root cause
StorageSession.ListMovecalledGETto obtain theIGarnetObjectand then mutated the returned heap object in place:That is only legal when the record is in the mutable region (
>= ReadOnlyAddress). Otherwise it races the object log writer and throwsInvalidOperationException: Collection was modifiedinsideObjectAllocatorImpl.WriteAsync. The page write is never issued,FlushedUntilAddressstalls, and the log wedges.The
WRONGTYPEis the aftermath, not the bug. On restart with recovery, the newest persisted record for the key is an older CopyUpdate source that was legitimately cleared to an inlineObjectIdMap.InvalidObjectId— four0xFFbytes, which read back as a perfectly valid String.Repro (standalone server + concurrent
LMOVEstress):Audit for the same anti-pattern
Per review feedback the audit covered every object operation. The pattern —
GETan object, then mutate its collection directly — appeared in exactly three places:StorageSession.ListMoveLMOVE,RLMOVEListPop/ListPushRMW helpersStorageSession.SetMoveSMOVECollectionItemBrokerTryGetNextListResult/TryGetNextSortedSetResultFour further defects surfaced while fixing those:
SMOVEdestroyed the member when the destination held a wrong type. Now returnsWRONGTYPE.LMOVE/BLMOVEdiscarded the key TTL. The no-op fast path now also coversCount == 1.BLMOVE src dstnever woke a client blocked ondst. Reproduced on unmodifiedmain, so this is pre-existing. The broker event is now enqueued aftertxnManager.Commit(true), which also avoids re-enteringkeysToObserversLock.TryAssignItemFromKeyreturned immediately after the firstHandleSetResult, so one event woke one observer regardless of how many items were available. Pre-existing onmain. With three clients blocked,LPUSH k a b cwoke 1 of 3 and stranded two items, and a no-opBLMOVE k k LEFT LEFTwoke 1 of 3 with the item still present; Redis serves all three in both cases. The observer queue is now drained until the collection can no longer serve the observer at the head. That observer is left queued rather than skipped, which keeps the queue FIFO and removes a latent infinite loop in the oldif (currCount > 0) continue;path, which re-peeked the same undequeued observer forever.SortedSetObject/HashObjectmutatedexpirationTimesduringDoSerialize. Serialization must be a pure read: the CAS state machine inHeapObjectBase.Serializecoordinates serializer-vs-serializer and serializer-vs-CopyUpdate only — plain readers never touchSerializationPhase, andObjectLogWriter.DoSerializetakes no record lock. Confirmed empirically by instrumentingDoSerializeunderZEXPIREload on a tiered-storage server.DoSerializeis now two-pass and non-mutating. Both passes share onenowand one snapshot of theexpirationTimesreference, because the element count is written before the entries and the writer is forward-only, so it cannot be back-patched. Emitted bytes are byte-identical.Separate defect:
KEYSlisted deleted keysScanLookup's dedup step (ConditionalScanPush) is a recency test — "is a newer version of this key reachable from the tag chain belowmaxAddress?" — not a liveness test.Deletion normally hides a key via two mechanisms, and elision defeats both:
The iterator walks raw log bytes, not the chain, so both records are still visited.
Bis caught by the existing tombstone filter.Ais not a tombstone, so it reachesConditionalScanPush,FindTaghits an empty chain, nothing newer is found — and the stale record is pushed. The tombstone that was supposed to suppressAis exactly what elision removed.The fix, and why this specific predicate
CanEliderequires the record to be the sole member of its tag chain:and
TryElidethen CASes the bucket slot tokInvalidAddress. So an elided chain is exactly an empty slot, andFindTagfailing is an exact test for "this key is unreachable."OperationStatus.NOTFOUNDis set only on that path (TryFindRecordInMainLogForPendingOperationonly ever setsSUCCESSorRETRY_LATER), so it is an unambiguous discriminator.The whole change is 3 lines in one file:
This keys off reachability of the key, not of the record — which is what makes it safe for point-in-time consumers. If the key was re-inserted above
maxAddress, as during slot migration, the chain is non-empty and the record is still pushed.Tests
Every test below was verified to fail without its corresponding fix.
RespListTests/RespSetTest— object-mutation and TTL regressionsRespBlockingCollectionTests— broker correctness and theBLMOVEdestination-notification gapGarnetObjectTests— non-mutating serializationRespScanCommandsTests— 3KEYSregressions, assertingKEYS+SCAN+DBSIZE+EXISTS+GETunderAssert.Multipleso no single view masks anotherSpanByteIterationTests.SpanByteIterateLookupSnapshotEmitsKeyRcudAboveSnapshotTail— locks the guarantee that an RCU landing above the captured tail does not lose the keyFour test assertions that had encoded the old
EXPIRE(key, TimeSpan.Zero)-on-empty behavior (which left a phantom record) were updated.Validation
Garnet.testGarnet.test.collectionsGarnet.test.clusterGarnet.test.cluster.migratedotnet build Garnet.slnxdotnet format(Garnet + Tsavorite)Run on
net10.0Debug.Notes for reviewers
AllocatorScan.cs(9 of them comment) with no public API change.ConditionalCopyToTail(compaction) is deliberately untouched; onlyConditionalScanPushis affected.DELon an already-expired key returns1where Redis returns0;SMOVE key key memberreturns0where Redis returns1(currently encoded in our own tests);SortedSetIntersectionreturns the liveDictionaryviaoutfor the single-key/no-weights case; the public mutable collection properties (ListObject.LnkList,SetObject.Set,SortedSetObject.Dictionary) would be better as read-only views; and eight redundantEXPIRE(key, TimeSpan.Zero)delete-on-empty hacks remain.