Track field TTLs in the volatile set when a HASH_2 load converts mid-listpack

[madolson]

Loading a HASH_2 payload can build a hash whose field-expiry metadata is inconsistent with its volatile set. The loader appends each field's expiry to the listpack as it goes but installs the aggregate volatile-count header only after the loop, and hashTypeConvertListpack() decides whether to register the converted entries in the volatile set by peeking at exactly that header. A payload whose later field crosses hash-max-listpack-value therefore converts a header-less listpack, and the resulting hashtable entries carry an expiry while belonging to no volatile set: the fields never expire, their TTLs are dropped by the next RDB save, and HDEL of one of them dereferences a NULL set. This derives the decision from the expiries of the entries the conversion actually carried over, so it no longer depends on the header being installed.

Details

Problem

rdbLoadObject() builds a RDB_TYPE_HASH_2 hash incrementally. Each field's expiry goes in as a trailing tagged metadata entry inside the loop (src/rdb.c:2428-2436), but the leading aggregate volatile-count header is installed once, after the loop (src/rdb.c:2449-2454), to avoid rewriting it per field.

Inside the loop, a field or value over hash-max-listpack-value converts the partially built object (src/rdb.c:2404-2409). That call site assumes conversion carries the already-loaded TTLs across on its own:

/* hashTypeConvert carries the TTLs of the pairs already in the
 * listpack into the volatile set; no header is needed for that. */
hashTypeConvert(o, OBJ_ENCODING_HASHTABLE);

It did not. hashTypeConvertListpack() gated its volatile-set registration loop on hashTypeHasVolatileFields(o) (src/t_hash.c:998), which for a listpack is an O(1) peek at the leading header (src/t_hash.c:128). Mid-load, the header does not exist yet, so the gate was false. The conversion loop still copied each field's expiry into the new entry (src/t_hash.c:1010-1011), so the hashtable ended up holding entries with an expiry that no vset knew about.

Three consequences, all confirmed on a build of da91ccd12:

keys_with_volatile_items stays 0 and the fields are never reaped or lazily hidden. dbTrackKeyWithVolatileItems() asks the same hashTypeHasVolatileFields() (src/db.c:549), which for a hashtable is set && !vsetIsEmpty(set), so the key is not tracked; and because hashTypeTrackEntry() never ran, hashTypeIgnoreTTL(o, false) never swapped in hashWithVolatileItemsHashtableType, so reads have no validate callback either.

The field TTLs are silently dropped by the next RDB save. rdbObjectType() selects RDB_TYPE_HASH_2 only when hashTypeHasVolatileFields(o) (src/rdb.c:775-783), so a desynced hash is written as plain RDB_TYPE_HASH:

httl before reload: 1000
OK
httl after reload:  -1

HDEL of a volatile field crashes. hashTypeUntrackEntry() takes hashTypeGetVolatileSet(), which returns NULL for uninitialized metadata (src/t_hash.c:67-71); the debugServerAssert(set) at src/t_hash.c:201 is compiled out of a release build and vsetRemoveEntryWithExpiry() dereferences it:

EIP:
0   valkey-server                       0x0000000104c7f5f4 vsetRemoveEntryWithExpiry + 40

Backtrace:
0   libsystem_platform.dylib            0x0000000181c81744 _sigtramp + 56
1   valkey-server                       0x0000000104c29a1c hashTypeUntrackEntry + 188
2   valkey-server                       0x0000000104c29a1c hashTypeUntrackEntry + 188
3   valkey-server                       0x0000000104c296d8 hashTypeDelete + 168
4   valkey-server                       0x0000000104c2bf28 hdelCommand + 376
5   valkey-server                       0x0000000104c10230 call + 1048

Reproduction, with no crafted payload; the source hash is one the server itself produced, and the threshold change stands in for any load whose hash-max-listpack-value is smaller than the one in effect when the payload was written:

config set hash-max-listpack-value 64
hset myhash a b cc dd
hexpire myhash 1000 FIELDS 1 a          # listpack, 'a' first and volatile
dump myhash
config set hash-max-listpack-value 1    # 'a'/'b' still fit, field 'cc' does not
restore myhash 0 <payload>              # converts after 'a' landed in the listpack
hdel myhash a                           # SIGSEGV

Both halves of the mismatch, the deferred header install and the header-based gate, came from c80da01 (#3212). No tag contains that commit.

Fix

Set has_volatile from the expiry of each entry the conversion carries over, in the same statement that hands that expiry to entryCreate(). The flag can no longer disagree with what landed in the hashtable, and it is unreachable from the header, so the loader's deferred install stops being load-bearing. The common case, converting a hash with no field TTLs, still skips the second pass.

Behavior of RDB_TYPE_HASH_2 loads, per input:

payload	before	after
len > hash-max-listpack-entries	correct: converts an empty listpack, every field tracked by the hashtable loop at rdb.c:2526-2528	unchanged
fits the listpack, no field over hash-max-listpack-value	correct: header installed after the loop	unchanged
first field over the value threshold	correct: converts before any expiry is appended, nothing to register	unchanged
a volatile field appended, then a field over the value threshold	entry keeps its expiry, vset empty	entry registered in the vset

Two commits: the behavior change, then a test-only follow-up that loosens the new HTTL assertion to a range so it does not depend on the DUMP/CONFIG/RESTORE sequence finishing inside the same second as the HEXPIRE.

Alternative considered

Install the aggregate header in rdb.c before the mid-load hashTypeConvert() call. It is the same line count and it fixes this caller, but it keeps hashTypeConvertListpack() depending on a header that every caller has to remember to maintain, which is the thing that broke. It also spends a lpInsertMetadata() realloc writing a header into a listpack the very next call frees. The conversion is already reading every field's expiry; asking it to consult a summary of data it holds in its hand buys nothing.

The report this came from also suggested making hashTypeUntrackEntry() fail gracefully on a NULL set. Not doing that: with the invariant repaired, a NULL set there means a new desync, and turning that into a returned error would hide it. The existing debugServerAssert() is the right shape.

Testing

The new test fails on da91ccd12's t_hash.c with the test in place:

[err]: RESTORE tracks field TTLs when the load converts mid-listpack in tests/unit/hashexpire.tcl
Expected '1' to be equal to '0' (context: type eval line 18 cmd {assert_equal 1 [get_keys_with_volatile_items r]} proc ::test)

hashexpire.tcl already covered DUMP/RESTORE of a listpack hash with field TTLs (tests/unit/hashexpire.tcl:5044) and loads that go straight to a hashtable on the entry count. Neither reaches a mid-load conversion: the first keeps the listpack encoding end to end, and the second converts an empty listpack before reading a single field. Nothing exercised a load that converts after a volatile field had already landed in the listpack.

The new guard's false branch, skipping the second pass when nothing carries an expiry, is a performance property only. Replacing if (expiry != EXPIRY_NONE) has_volatile = true; with an unconditional assignment leaves hashexpire.tcl and type/hash.tcl green, as it should.

This was generated by AI but verified, with love, by a human.