[frostzt]

Use listpacks for small hashes with field expiration.

The idea is that we'll introduce a new tagged metadata 0xF5 to encode expiry within the listpack encoded value itself.

No RDB changes are introduced in this PR. When storing a hash with volatile fields, it is stored as if it were a hashtable in the RDB format.

So far, field expiration couldn't be encoded in listpacks, so any hash with field expiration could only be stored as a hashtable, which consumes more memory.

Added vsetSize(): returns the number of entries in a vset by summing per-bucket counts (single/vector/hashtable/rax), so callers can get the element count without iterating the entries.

Closes #2618.

[zuiderkwast]

feat(listpack)

Where is this naming convention coming from? I've seen it in many projects but I haven't seen it described anywhere.

Anyway, we're not using it in Valkey. We just use plain language but we often include words _like "fix" (if it's a bug fix) or "add" or "support" if it's a feature. Here, "listpack" and "support" are already in the title, so no need for the extra prefix function call syntax.

Btw, I believe we'd prefer to have the full implementation for hashes in the same PR, if you plan to finish it all, but we can review it incrementally if you want. Just add multiple commits. We usually squash them when we merge, or we can keep them as separate commits depending on the situation.

[frostzt]

feat(listpack)

Where is this naming convention coming from? I've seen it in many projects but I haven't seen it described anywhere.

Anyway, we're not using it in Valkey. We just use plain language but we often include words _like "fix" (if it's a bug fix) or "add" or "support" if it's a feature. Here, "listpack" and "support" are already in the title, so no need for the extra prefix function call syntax.

Btw, I believe we'd prefer to have the full implementation for hashes in the same PR, if you plan to finish it all, but we can review it incrementally if you want. Just add multiple commits. We usually squash them when we merge, or we can keep them as separate commits depending on the situation.

For sure updated it I'll keep making changes and pushing them here, I'd love a scrutinized review this would not only be a great learning opportunity but rather would be me getting more familiar with the repository too 😃

[frostzt]

Did something related to tests went in recently? This seems to break I think tests were migrated from c -> cpp and something is probably missing from my end?

[sarthakaggarwal97]

@frostzt the project recently introduced GTEST, the CI failures look related to those.

[frostzt]

Ok getting somewhere now this particular issue is in hashTypeNext and I kinda realize why @ranshid was scrutinizing the validateEntry hook, what's going on is that the hashTypeNext for Listpack should also validate the expiry cause if not the generic HGETALL fails.

Same as @sarthakaggarwal97 mentioned I think hashTypeNext is actually one of the function that uses the lpGet and lpNext but we're encountering metadata values which is good just the expiry to be added :)

[cjx-zar]

I have two suggestions based on my review:

1. A new RDB type is needed for forward compatibility

Currently, hash listpacks with 0xF5-tagged metadata are saved using the existing RDB_TYPE_HASH_LISTPACK. This creates a silent data corruption risk when an older Valkey version loads the RDB.

This could be used conditionally: hashes without any HFE field continue using the old RDB_TYPE_HASH_LISTPACK (downgrade-safe), while hashes with at least one volatile field use the new type (blocks downgrade, as expected when HFE is in use). This is consistent with how RDB_TYPE_HASH_2 was introduced for hashtable-encoded hashes with volatile fields.

2. Consider adding a volatile field counter for O(1) volatile tracking

hashTypeHasVolatileFields currently scans the entire listpack to check for 0xF5 metadata entries, making it O(n). Since this function is called before and after every hash mutation to decide whether to update keys_with_volatile_items, the cost adds up.

A simple wrapper struct with a counter would make this O(1):

typedef struct {
    int64_t volatile_count;
    void *lp;
} listpackLp2;

Every metadata add/remove operation updates volatile_count. Then hashTypeHasVolatileFields becomes O(1) with no listpack traversal. This also enables fast LISTPACK2 → LISTPACK encoding transitions when the count drops to zero.

The already-defined but currently unused OBJ_ENCODING_LISTPACK2 could serve as the in-memory encoding type to distinguish listpacks wrapped in this struct.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Implements per-field TTLs for hash listpack entries with tagged metadata. Propagates expiry handling through listpack APIs, hash operations, RDB persistence, SCAN, AOF rewriting, volatile-set accounting, replication, and regression tests.

Changes

Hash Field Expiration with Listpack Metadata

Layer / File(s)	Summary
Listpack metadata encoding infrastructure src/listpack.c, src/listpack.h, src/unit/test_listpack.cpp	Adds tagged metadata entries, metadata access and mutation APIs, metadata-aware traversal, deletion, lookup, and integrity validation.
Hash expiry foundation and public API src/expire.h, src/server.h, src/t_hash.c	Adds listpack hash encoding and iterator contracts, volatile-count APIs, TTL visibility checks, and listpack expiry lookup.
Hash field expiry lifecycle src/t_hash.c	Updates listpack set, expire, persist, delete, and iterator paths to maintain expiry metadata and aggregate volatile counts.
Hash conversion and expiry-aware operations src/t_hash.c	Preserves expiries during conversion and updates random selection, sampling, and active expiration for visible fields.
RDB serialization and loading src/rdb.c	Serializes TTL-bearing listpack hashes as HASH_2, validates metadata, and loads fields with preserved expiries and volatile counts.
Runtime scans and AOF rewrite src/db.c, src/aof.c	Skips expired fields during listpack scans and emits visible volatile and persistent fields during AOF rewrite.
Validation, volatile-set sizing, and regression coverage src/vset.c, src/vset.h, src/unit/test_vset.cpp, src/t_stream.c, tests/unit/hashexpire.tcl, tests/integration/dual-channel-replication.tcl, .gitignore	Updates validation callers, adds volatile-set size accounting, and covers metadata behavior, expiry tracking, persistence, replication, and cleanup.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested reviewers: jjuleslasarte, sarthakaggarwal97

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant HashCommands
  participant Listpack
  participant RDB
  participant VolatileSet
  Client->>HashCommands: set hash field expiry
  HashCommands->>Listpack: attach tagged expiry metadata
  HashCommands->>VolatileSet: update volatile-field count
  RDB->>Listpack: load field/value/expiry records
  Listpack->>VolatileSet: restore volatile-field count
  Client->>HashCommands: read or scan hash
  HashCommands->>Listpack: filter expired fields

Loading

Merge Risk: 🟠 High · up to 65c03

The new expiry-aware listpack conversion can drop expired hash fields on a primary while replicas retain them, causing inconsistent data between nodes. This correctness issue should be fixed before merge.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding listpack encoding support for hash field expiration.
Description check	✅ Passed	The description is directly related to the changeset. It explains tagged metadata for expiring hash fields, memory benefits, the vsetSize API, and the linked issue.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/listpack.c (2)

368-392: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the lpCurrentEncodedSizeBytes() → lpCurrentEncodedSizeUnsafe() safety contract.

lpValidateNext() only bounds-checks p + lenbytes before calling the unsafe decoder. For tagged entries, lenbytes == 1 validates only the outer tag, but lpCurrentEncodedSizeUnsafe() immediately parses the inner header and can read past the buffer on truncated/corrupt tagged values. This also leaves nested tags unbounded in the recursive path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/listpack.c` around lines 368 - 392, lpCurrentEncodedSizeBytes currently
returns 1 for tagged encodings which lets callers (like lpValidateNext) validate
only the outer tag byte before lpCurrentEncodedSizeUnsafe reads the inner header
and may overrun; update lpCurrentEncodedSizeBytes (and callers' expectations) so
that for LP_ENCODING_IS_TAGGED it returns the total number of header bytes
required including the inner element's header(s) (i.e., 1 + the result of
recursively computing the inner header size using the safe bytes-only routine),
thereby preserving the safety contract that callers can bound-check p + lenbytes
before any unsafe parsing and preventing unbounded recursion on nested tags;
reference lpCurrentEncodedSizeBytes, lpCurrentEncodedSizeUnsafe, and
lpValidateNext when making this change.

---

767-848: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Tagged insertion currently changes the meaning of elestr.

The normal lpInsert() contract treats elestr as raw input that still needs listpack encoding. The tagged branch copies elestr verbatim, so lpInsertMetadata(..., elestr, ...) can serialize an invalid inner entry. That becomes especially dangerous because lpGetMetadataValue() later assumes an integer payload and will silently misread a tagged string.

Possible containment if metadata is intentionally integer-only

     if (tagged) {
+        serverAssert(elestr == NULL);
+        serverAssert(eleint != NULL);
         enctype = LP_ENCODING_TAGGED;
-        enclen = size + 1; /* size is the encoded value +1 for the prefixed tag */
+        enclen = size + 1; /* size is the encoded inner value */
     } else if (elestr) {

🧹 Nitpick comments (2)

src/expire.h (1)

4-7: ⚡ Quick win

Move fmacros.h ahead of dict.h.

The added comment says feature-test macros must be visible early, but the new include order does the opposite. Keeping fmacros.h first avoids reintroducing the same include-order problem if dict.h ever gains a system-header dependency.

Suggested reorder

-#include "dict.h"
 `#include` "fmacros.h"
+#include "dict.h"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/expire.h` around lines 4 - 7, The include order in expire.h is reversed:
feature-test macros from fmacros.h must be defined before any headers like
dict.h; move the fmacros.h include so it appears before dict.h (i.e., include
"fmacros.h" first, then "dict.h") to ensure feature-test macros are visible to
any system headers pulled in by dict.h or future changes.

src/t_hash.c (1)

77-84: 🏗️ Heavy lift

Track volatile listpack fields in O(1).

hashTypeHasVolatileFields() now walks the full listpack looking for metadata. This helper is called before/after many write paths in this file, so volatile listpack hashes pick up an extra O(n) scan on hot mutations. A wrapper encoding that stores a volatile-field count would avoid repeatedly rescanning the same payload.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/t_hash.c` around lines 77 - 84, hashTypeHasVolatileFields() currently
scans the entire listpack for metadata (O(n)); add a wrapper encoding that
stores a volatile-field count and use that count for O(1) checks. Concretely:
introduce a volatile_count field on the listpack-backed hash object (the
OBJ_ENCODING_LISTPACK wrapper), update hashTypeHasVolatileFields() to return
true when volatile_count>0 (fall back to the full scan only if volatile_count is
absent/zero for compatibility), and update every write/mutation path that
touches listpack entries (the functions that call lpFirst, lpNext, lpIsMetadata
and all hash mutation helpers in this file) to increment/decrement
volatile_count on insert/delete/update when lpIsMetadata would change; ensure
creation/decoding of existing listpacks initializes volatile_count by performing
a single scan once.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/aof.c`:
- Around line 2088-2206: The HMSET arity is miscomputed for listpack-encoded
hashes: non_volatile_items is derived from hashTypeLength(o) but the listpack
emission loop skips any field/value pair that has a metadata node (meta &&
lpIsMetadata(meta)), so the HMSET bulk count can be too large; in
rewriteHashObject adjust the listpack branch to compute non_volatile_items by
scanning the listpack (using lpFirst, lpNext, lpGet, lpIsMetadata) and counting
only the field/value pairs that will actually be emitted (i.e., where meta is
NULL or not lpIsMetadata(meta)), or alternatively compute cmd_items/arity
on-the-fly from that scan before writing each HMSET; ensure the same skip
predicate is used when counting so the computed 2 + cmd_items * 2 matches
emitted fields.

In `@src/t_hash.c`:
- Around line 673-688: The HPERSIST branch currently uses
lpFind()/lpNext()/lpDelete() directly and removes metadata without rechecking
expirations, which can resurrect an expired field; modify the branch (around
objectGetVal, lpFirst, lpFind, lpNext, lpIsMetadata, lpDelete, objectSetVal) to
first detect and handle expiry the same way
hashTypeGetFromListpack()/hashTypeGetExpiry() do (i.e., interpret the metadata,
call the same expiry-check logic and treat expired entries as non-existent), and
only proceed to delete metadata and update the object if the field is confirmed
non-expired.
- Around line 2402-2408: The loop in the count-reply path uses listpackEntry
structs (field, value) returned by hashTypeRandomElement but always calls
addWritePreparedReplyBulkCBuffer which will pass NULL for integer-encoded
entries; update the replies to branch on entry type like
hrandfieldReplyWithListpack does: when field.sval is non-NULL call
addWritePreparedReplyBulkCBuffer(field.sval, field.slen) else call
addWritePreparedReplyBulkLongLong(field.lval); do the same for value when
withvalues is true (use value.sval/value.slen vs value.lval). Keep existing
addWritePreparedReplyArrayLen and error/loop logic intact.
- Around line 2646-2658: The code uses lpGet(fptr, &flen, NULL) which doesn't
materialize integer-encoded listpack entries, so when building the object for
expired field names you must use the integer-safe accessor flow (lpGetValue() /
listpackEntry) to obtain a proper char* and length before calling
createStringObject; replace the lpGet/fptr->field usage in this expired-entry
path with listpackEntry/listpackGetValue-based retrieval (honoring
flen/encoding) and then build the string object from the materialized value and
set out_entries[expired_count] accordingly to avoid corrupt copies/crashes.
- Around line 753-761: hashTypeLength() currently iterates the listpack
(OBJ_ENCODING_LISTPACK via objectGetVal, lpFirst, lpNext and lpIsMetadata) and
counts non-metadata entries but does not skip field/value pairs whose metadata
has expired; update the loop in hashTypeLength() so that when you encounter a
metadata entry you check the metadata's expiration with the same listpack-hash
expiration helper used elsewhere (the helper that determines if a metadata entry
is expired) and if that metadata is expired skip both the field and value
entries (advance p twice via lpNext without incrementing count), otherwise count
the field/value pair as before; ensure you continue to use lpIsMetadata, lpFirst
and lpNext to traverse.

---

Outside diff comments:
In `@src/listpack.c`:
- Around line 368-392: lpCurrentEncodedSizeBytes currently returns 1 for tagged
encodings which lets callers (like lpValidateNext) validate only the outer tag
byte before lpCurrentEncodedSizeUnsafe reads the inner header and may overrun;
update lpCurrentEncodedSizeBytes (and callers' expectations) so that for
LP_ENCODING_IS_TAGGED it returns the total number of header bytes required
including the inner element's header(s) (i.e., 1 + the result of recursively
computing the inner header size using the safe bytes-only routine), thereby
preserving the safety contract that callers can bound-check p + lenbytes before
any unsafe parsing and preventing unbounded recursion on nested tags; reference
lpCurrentEncodedSizeBytes, lpCurrentEncodedSizeUnsafe, and lpValidateNext when
making this change.

---

Nitpick comments:
In `@src/expire.h`:
- Around line 4-7: The include order in expire.h is reversed: feature-test
macros from fmacros.h must be defined before any headers like dict.h; move the
fmacros.h include so it appears before dict.h (i.e., include "fmacros.h" first,
then "dict.h") to ensure feature-test macros are visible to any system headers
pulled in by dict.h or future changes.

In `@src/t_hash.c`:
- Around line 77-84: hashTypeHasVolatileFields() currently scans the entire
listpack for metadata (O(n)); add a wrapper encoding that stores a
volatile-field count and use that count for O(1) checks. Concretely: introduce a
volatile_count field on the listpack-backed hash object (the
OBJ_ENCODING_LISTPACK wrapper), update hashTypeHasVolatileFields() to return
true when volatile_count>0 (fall back to the full scan only if volatile_count is
absent/zero for compatibility), and update every write/mutation path that
touches listpack entries (the functions that call lpFirst, lpNext, lpIsMetadata
and all hash mutation helpers in this file) to increment/decrement
volatile_count on insert/delete/update when lpIsMetadata would change; ensure
creation/decoding of existing listpacks initializes volatile_count by performing
a single scan once.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 367f9150-d70a-43d7-8d61-a3114d6aef2a

📥 Commits

Reviewing files that changed from the base of the PR and between 7abe49a and 548a04e.

📒 Files selected for processing (10)

• src/aof.c
• src/db.c
• src/expire.h
• src/listpack.c
• src/listpack.h
• src/server.h
• src/t_hash.c
• src/unit/test_listpack.cpp
• src/valkey-unit-tests
• tests/unit/hashexpire.tcl

[frostzt]

Thank you @ranshid I'll go through - this would months of work finally 😭

[ranshid]

@frostzt I see the fuzzer located assertion related to the hash not being removed from the volatile mapping: https://github.com/valkey-io/valkey/actions/runs/34470551920/job/102849189811?pr=3212 maybe you have cycles to TAL?

[frostzt]

@ranshid I checked this and I am not able to repro this tried all the sites - my idea here is that what I can do instead is push a change where if the assertion fails (in CI) I can log this key name encoding hashtypelen and hashtpyevolatilecount and weather the leading meta data exists or not. That way at least we cna get an idea as to what caused this. What do you think?

[ranshid]

all tests which are failing seems unrelated.

Would like to thank @jjuleslasarte, @zuiderkwast and @sarthakaggarwal97 and @cjx-zar for helping with the review!

Special thank you to @frostzt. He took on this "non-trivial" task and it has been a long journey from an idea to implementation. While the work sometimes linger, @frostzt lost no hope and we are finally ready to merge it. Thank you @frostzt !

[ranshid]

the test-ubuntu-jemalloc probably stuck due to this missing fix: #4643

IMO all is green to merge