Fix crash when active field-expiry leaves a single-entry HT vset buck…#3957
Conversation
…et (valkey-io#3950) When a hash has many fields whose expirations fall in the same volatile-set time-bucket (>127 entries), that bucket is encoded as a hashtable (HT). The active field-expiry path drains expired entries via vsetBucketRemoveExpired_HASHTABLE(), but it is bounded by the per-key expire quota (max_count, from dbReclaimExpiredFields). When the quota stops the drain one entry short, the function only collapsed the bucket to NONE at size 0 and left it as an HT bucket holding a single entry. That violates the invariant enforced by removeFromBucket_HASHTABLE(), which asserts hashtableSize(ht) > 0 after a delete and downgrades an HT bucket to SINGLE at size 1. A later normal removal of that lone field -- HDEL, or a cross-bucket HSETEX update routed through removeEntryFromRaxBucket() -- deletes the sole entry, drops the size 1 -> 0, and trips the assertion. Fix: Apply the same downgrade in the expiry path: when draining leaves exactly one entry, downgrade the HT bucket to SINGLE so the invariant holds. Behavior is unchanged when the bucket drains fully (-> NONE) or retains two or more entries (-> HT, as the normal removal path already tolerates). Add a regression test that fills one time-bucket past the HT threshold, expires all but one entry, then removes the survivor through the normal path; it crashed before this fix and passes after. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 9.1 #3957 +/- ##
==========================================
- Coverage 76.64% 76.36% -0.29%
==========================================
Files 163 163
Lines 81003 81027 +24
==========================================
- Hits 62086 61875 -211
- Misses 18917 19152 +235
🚀 New features to boost your workflow:
|
enjoy-binbin
left a comment
There was a problem hiding this comment.
OK, I thought we already had a backport bot.
you are probably right. I thought we need to remove the unitests, but actually in 9.1 we can still have them |
backport of (#3950)
When a hash has many fields whose expirations fall in the same volatile-set time-bucket (>127 entries), that bucket is encoded as a hashtable (HT). The active field-expiry path drains expired entries via vsetBucketRemoveExpired_HASHTABLE(), but it is bounded by the per-key expire quota (max_count, from dbReclaimExpiredFields). When the quota stops the drain one entry short, the function only collapsed the bucket to NONE at size 0 and left it as an HT bucket holding a single entry.
That violates the invariant enforced by removeFromBucket_HASHTABLE(), which asserts hashtableSize(ht) > 0 after a delete and downgrades an HT bucket to SINGLE at size 1. A later normal removal of that lone field -- HDEL, or a cross-bucket HSETEX update routed through removeEntryFromRaxBucket() -- deletes the sole entry, drops the size 1 -> 0, and trips the assertion.
Fix: Apply the same downgrade in the expiry path: when draining leaves exactly one entry, downgrade the HT bucket to SINGLE so the invariant holds. Behavior is unchanged when the bucket drains fully (-> NONE) or retains two or more entries (-> HT, as the normal removal path already tolerates).
Add a regression test that fills one time-bucket past the HT threshold, expires all but one entry, then removes the survivor through the normal path; it crashed before this fix and passes after.