Repository navigation
[Objects] HCOLLECT, ZCOLLECT: Do not delete keys that still hold unexpired fields or members - #2200
Merged
Badrish Chandramouli (badrishc) merged 2 commits intoOct 2, 2026
Conversation
Copilot started reviewing on behalf of
Federico Laurianti (Laurianti)
October 2, 2026 04:31
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation directly fixes the shared-state defect and includes focused regression coverage for all affected paths.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes cross-key state leakage in hash and sorted-set expiration collection.
Changes:
- Creates a fresh
ObjectOutputper collected key. - Prevents incorrect deletion and
WrongTypecarryover. - Adds regression coverage for explicit and wildcard collection.
| File | Description |
|---|---|
libs/server/Storage/Session/ObjectStore/Common.cs |
Isolates output state during wildcard/background collection. |
libs/server/Storage/Session/ObjectStore/HashOps.cs |
Isolates each HCOLLECT key operation. |
libs/server/Storage/Session/ObjectStore/SortedSetOps.cs |
Isolates each ZCOLLECT key operation. |
test/standalone/Garnet.test.collections/RespHashTests.cs |
Tests live-field preservation and wrong-type ordering. |
test/standalone/Garnet.test.collections/RespSortedSetTests.cs |
Tests live-member preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Badrish Chandramouli (badrishc)
approved these changes
Oct 2, 2026
Badrish Chandramouli (badrishc)
merged commit Oct 2, 2026
143adec
into
microsoft:main
5 checks passed
x@01 (x-at-01)
added a commit
to webc-fork/garnet
that referenced
this pull request
Oct 4, 2026
…n missing key (microsoft#2192), ZADD XX INCR null (microsoft#2197), no empty object from object RMW (microsoft#2194), fresh ObjectOutput per key HCOLLECT/ZCOLLECT (microsoft#2200), CompletionEvent disposal + LogSizeTracker coalesce (microsoft#2198), dynamic test ports (microsoft#2193)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause
HashCollectandSortedSetCollect(listed keys) andObjectCollect(*and the background collection task) created oneObjectOutputand passed it to the RMW of every key.OperatesetsRemoveKeywhen a key becomes empty, and nothing cleared it, so every following key was deleted byInPlaceUpdaterorPostCopyUpdatereven with unexpired fields or members left. AWrongTypeflag carried over the same way, and an emptied key after a key of another type was left in place.Description of Change
libs/server/Storage/Session/ObjectStore/HashOps.cs,SortedSetOps.csandCommon.cs: each key in the collection loops gets a newObjectOutput, as theadd-garnet-commandskill describes for each call to the storage API. A key is now deleted only when its own collection empties it.Tests
RespHashTests.HashCollectKeepsLiveFieldsOfEveryKey: hashes emptied by the collection are deleted, and hashes with an unexpired field keep it, with listed keys and with*.RespHashTests.HashCollectRemovesEmptiedKeyAfterWrongTypeKey:HCOLLECTwith a string key first repliesWRONGTYPEand still deletes the emptied hash.RespSortedSetTests.SortedSetCollectKeepsLiveMembersOfEveryKey: the same as the first test forZCOLLECT.mainwith this change, net10.0:Garnet.test.collections785 passed,Garnet.test1291 passed (9 skipped).Issues Fixed
Fixes #2199