Fix HINCRBYFLOAT removes field expiration on replica

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#9

[claude]

🔴 rewriteClientCommandVector(c, 6, ...) at line 2571 passes 7 robj* arguments but argc=6, so the function only consumes the first 6 via va_arg — silently dropping newobj (the field value). The replicated command sent to replicas and AOF becomes HSETEX key KEEPTTL FIELDS 1 field (missing the value), which will cause parse errors on replicas and corrupt data on AOF reload. Change 6 to 7.

Extended reasoning...

What the bug is

The rewriteClientCommandVector call in hincrbyfloatCommand (src/t_hash.c:2571-2572) specifies argc=6 but passes 7 robj* variadic arguments:

rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl,
                    shared.fields, shared.integers[1], c->argv[2], newobj);

How rewriteClientCommandVector works

Looking at rewriteClientCommandVector in networking.c:4158-4174, the function allocates an array of exactly argc entries (line 4163) and then loops exactly argc times (line 4165) consuming arguments from the va_list via va_arg. With argc=6, only the first 6 of the 7 provided arguments are consumed. The 7th argument (newobj, the computed float value) is never read.

Step-by-step proof

When a user runs HINCRBYFLOAT h1 f1 1.1, the command computes the new value and rewrites the client command for replication. The 7 arguments passed are:

1. shared.hsetex → HSETEX
2. c->argv[1] → h1 (key)
3. shared.keepttl → KEEPTTL
4. shared.fields → FIELDS
5. shared.integers[1] → 1 (field count)
6. c->argv[2] → f1 (field name)
7. newobj → 2.1 (computed value) ← DROPPED

Since argc=6, the loop stops after consuming argument #6 (c->argv[2]). The resulting replicated command is: HSETEX h1 KEEPTTL FIELDS 1 f1 — a malformed command missing the value.

Impact

The HSETEX command requires field-value pairs after the FIELDS <count> argument. A command with a field name but no value will be rejected by the HSETEX parser on replicas, causing replication errors. Similarly, AOF files containing this malformed command will fail on reload, potentially causing data loss. Notably, the PR's own test expects the value in the replication stream ({hsetex h1 KEEPTTL FIELDS 1 f1 *}), which would fail with the current argc=6.

Fix

Change argc from 6 to 7 on line 2571:

rewriteClientCommandVector(c, 7, shared.hsetex, c->argv[1], shared.keepttl,
                    shared.fields, shared.integers[1], c->argv[2], newobj);

For reference, the analogous HINCRBY replication path uses rewriteClientCommandArgument (which modifies individual args in-place) and does not have this problem.

[claude]

🔴 Bug: HASH_SET_KEEP_TTL was removed from the hashTypeSet call in hincrbyfloatCommand (line 2559), causing field TTL to be cleared on the primary. Since replication now uses HSETEX KEEPTTL which preserves TTL on replicas, this creates primary-replica divergence. The fix is to restore the flag: HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL.

Extended reasoning...

What the bug is

The PR changes the hashTypeSet call in hincrbyfloatCommand from:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);

to:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE);

Without HASH_SET_KEEP_TTL, hashTypeSet actively removes any existing field TTL. For listpack encoding (lines 959-964), it calls listpackExUpdateExpiry with HASH_LP_NO_TTL, removing the TTL. For hashtable encoding (lines 989-992), it calls hfieldPersist(o, oldField) which explicitly removes the TTL from the field.

How primary-replica divergence occurs

On the primary, HINCRBYFLOAT now clears the field TTL via hashTypeSet (because HASH_SET_KEEP_TTL is missing). However, the replication is rewritten as HSETEX h1 KEEPTTL FIELDS 1 f1 <value>, which preserves the TTL on replicas. This means after HINCRBYFLOAT on a field with TTL:

• Primary: field TTL is removed (field becomes persistent)
• Replica: field TTL is preserved

This is a data consistency violation between primary and replica.

Step-by-step proof

1. Set a hash field with TTL: HSETEX h1 EX 100 FIELDS 1 f1 10
2. Run HINCRBYFLOAT h1 f1 1.5 on the primary
3. On the primary, hashTypeSet is called without HASH_SET_KEEP_TTL → field TTL is cleared → HPTTL h1 FIELDS 1 f1 returns -1
4. The command is replicated as HSETEX h1 KEEPTTL FIELDS 1 f1 11.5 → on the replica, TTL is preserved → HPTTL h1 FIELDS 1 f1 returns a value in [1, 100000]
5. The primary and replica now disagree on the field's expiration state

Evidence from the codebase

The analogous hincrbyCommand at line 2506 correctly retains HASH_SET_KEEP_TTL:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);

The existing test {HINCRBYFLOAT - preserve expiration time of the field} would fail with this change, as it sets a 20ms HPEXPIRE, runs HINCRBYFLOAT, then asserts HPTTL is in range [1, 20] — but without HASH_SET_KEEP_TTL, the TTL is cleared and HPTTL returns -1.

Fix

Restore HASH_SET_KEEP_TTL in the hashTypeSet call while keeping the new HSETEX KEEPTTL replication:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);