Fix HINCRBYFLOAT removes field expiration on replica

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#9

Summary by CodeRabbit

Release Notes

• New Features

  • Introduced HSETEX command for hash field operations with TTL management capabilities
  • Added FIELDS argument support for field operations

• Bug Fixes

  • Fixed HINCRBYFLOAT to preserve field expiration times during replication, maintaining TTL consistency across replicas

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce support for preserving field expiration during HINCRBYFLOAT operations. Two new shared constants (HSETEX and FIELDS) are added to the shared objects infrastructure, the HINCRBYFLOAT replication logic is refactored to use HSETEX with KEEPTTL semantics instead of basic HSET, and a test is added to validate TTL preservation on replicas.

Changes

Cohort / File(s)	Summary
Shared Objects Infrastructure src/server.h, src/server.c	Added two new shared object pointers to sharedObjectsStruct: *hsetex and *fields, initialized as string constants "HSETEX" and "FIELDS" during shared object creation.
HINCRBYFLOAT TTL Semantics src/t_hash.c	Modified hincrbyfloatCommand replication to emit HSETEX-based commands with KEEPTTL flag instead of HSET, changing how field expiration is preserved during floating-point increments on replicas and AOF.
Test Coverage tests/unit/type/hash-field-expire.tcl	Added test "HINCRBYFLOAT command won't remove field expiration on replica" to verify that HINCRBYFLOAT is replicated as HSETEX with KEEPTTL and that field TTLs remain consistent across master-replica synchronization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Float with care, the hopper hops with grace,
HSETEX preserves the expiration's place,
KEEPTTL shines where increments would dance,
Replicas mirror every field's advance,
TTL eternal, no fleeting chance! 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing a bug where HINCRBYFLOAT removes field expiration on replicas, which aligns with the actual changes made to the hincrbyfloatCommand and its replication behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch coderabbit_full_head_fix_hincrbyfloat_removes_field_expiration_on_replica_pr9

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/t_hash.c`:
- Line 2559: The hashTypeSet call is missing the HASH_SET_KEEP_TTL flag causing
TTL loss on master; update the call to use both flags (bitwise OR) so it becomes
HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL, mirroring the usage in hincrbyCommand
(which uses HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL) to preserve field
expiration and keep master/replica consistent (modify the hashTypeSet(c->db, o,
c->argv[2]->ptr, new, ...) invocation accordingly).

🧹 Nitpick comments (1)

tests/unit/type/hash-field-expire.tcl (1)

1990-1996: Consider adding master TTL verification to ensure consistency.

The test verifies TTL on the replica but doesn't explicitly check that TTL is preserved on the master after HINCRBYFLOAT. Adding a master-side assertion would ensure no inconsistency between master and replica.

♻️ Proposed addition to verify master TTL

                 r -1 hincrbyfloat h1 f1 1.1
                 r -1 hincrbyfloat h1 f2 1.1

+                # Verify TTL is preserved on master
+                assert_range [r -1 httl h1 FIELDS 1 f1] 90 100
+
                 # Expiration time should not be removed on replica and the value
                 # should be equal to the master.
                 wait_for_ofs_sync  [srv -1 client]  [srv 0 client]
                 assert_range [r httl h1 FIELDS 1 f1] 90 100

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Missing HASH_SET_KEEP_TTL flag causes TTL loss on master.

The HASH_SET_KEEP_TTL flag is missing from hashTypeSet. Compare with hincrbyCommand at line 2506 which uses HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL.

Without this flag:

1. Master removes the field's TTL during the set operation
2. Replica preserves TTL via the replicated HSETEX ... KEEPTTL ... command
3. This creates an inconsistency between master and replica

The existing test at line 851-857 (HINCRBYFLOAT - preserve expiration time of the field) verifies TTL preservation locally and would fail with this code.

🐛 Proposed fix to preserve TTL on master

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

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

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

🤖 Prompt for AI Agents

In `@src/t_hash.c` at line 2559, The hashTypeSet call is missing the
HASH_SET_KEEP_TTL flag causing TTL loss on master; update the call to use both
flags (bitwise OR) so it becomes HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL,
mirroring the usage in hincrbyCommand (which uses HASH_SET_TAKE_VALUE |
HASH_SET_KEEP_TTL) to preserve field expiration and keep master/replica
consistent (modify the hashTypeSet(c->db, o, c->argv[2]->ptr, new, ...)
invocation accordingly).