Avoid allocation when iterating over hashes, lists, sets and kvstores

[tomerqodo]

Benchmark PR from qodo-benchmark#28

Summary by CodeRabbit

• Refactor
  • Refactored iterator lifecycle management across list, set, hash, and key-value store implementations to use stack-allocated iterators instead of heap-allocated pointers, requiring initialization and reset patterns instead of dynamic allocation and deallocation.

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

[coderabbitai]

Walkthrough

This pull request refactors the iterator API across multiple data structures from heap-allocated pointer-based patterns to stack-allocated value-based patterns. Instead of functions allocating and returning iterator pointers, callers now declare iterators locally and pass their addresses to init/next/reset functions throughout the codebase.

Changes

Cohort / File(s)	Summary
Header API Definitions src/adlist.h, src/kvstore.h, src/server.h	Remove allocation-based iterator signatures (listGetIterator, kvstoreGetDictIterator, hashTypeInitIterator returning pointers); introduce init/reset variants accepting iterator pointers. Update public signatures for list, set, hash, and kvstore iterators to accept pre-allocated iterator objects.
List Iterator Implementation src/adlist.c, src/t_list.c	Refactor listTypeInitIterator and listGetIterator to initialize caller-provided iterators instead of allocating. Replace listReleaseIterator/listTypeReleaseIterator with listTypeResetIterator. Update all 20+ call sites in t_list.c to use stack-allocated iterators with addressable init/next/reset calls.
Set Iterator Implementation src/t_set.c	Convert setTypeInitIterator from allocation-returning to void initialization; replace setTypeReleaseIterator with setTypeResetIterator. Update 30+ usages across set operations to declare iterators locally and pass addresses to init/next/reset functions.
Hash Iterator Implementation src/t_hash.c	Refactor hashTypeInitIterator to take iterator pointer and return void; replace hashTypeReleaseIterator with hashTypeResetIterator. Update iterator struct to embed dictIterator di instead of pointer. Adjust 20+ call sites across hash operations.
KVStore Iterator Implementation src/kvstore.h, src/kvstore.c	Replace kvstoreIteratorInit (pointer-returning) with void initialization; introduce kvstoreInitDictIterator/kvstoreInitDictSafeIterator to replace kvstoreGetDictIterator/kvstoreGetDictSafeIterator. Replace release functions with reset variants. Remove allocation/return patterns from 30+ lines.
Call Sites – Bulk Operations src/aof.c	Update list, set, hash, and kvstore iterator usages in AOF rewrite paths. Replace heap allocations with stack declarations; update 10+ iteration loops to use init/next/reset pattern with addressable forms. Add error-handling labels (werr2) for proper cleanup.
Call Sites – Database/Cluster src/cluster.c, src/cluster_asm.c, src/db.c, src/pubsub.c, src/rdb.c	Convert kvstore and dict iterators to stack-allocated in clusterDelKeysInSlot, slot snapshot routines, keysCommand, scanGenericCommand, and channel list operations. Update error paths to use reset instead of release; introduce cleanup labels where needed.
Call Sites – Debugging/Utilities src/debug.c, src/module.c, src/sort.c	Refactor dataset digest computation and SORT command list/set/hash iterations to use stack-allocated iterators. Update module key iteration and cleanup. Replace all allocation patterns with local variable + init/reset calls.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Heap-bound pointers scattered free,
Now stack-alloc'd, serene and light,
Init calls spring, each iterator bright,
Reset unwound in harmony—
A hop toward cleaner memory! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 64.41% 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 PR title directly and accurately summarizes the main objective: eliminating memory allocations during iteration over multiple data structures.

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

✨ Finishing touches

• 📝 Generate docstrings

---

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

[coderabbitai]

Actionable comments posted: 9

Caution

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

⚠️ Outside diff range comments (2)

src/adlist.c (1)

195-231: Update the iterator usage example to match the new API.

The usage example around Line 227 still references listGetIterator, which was removed. Please update it to the new stack-allocated listIter + listInitIterator flow to avoid confusion.

📝 Suggested doc update

- * iter = listGetIterator(list,<direction>);
- * while ((node = listNext(iter)) != NULL) {
+ * listIter iter;
+ * listInitIterator(&iter, list, <direction>);
+ * while ((node = listNext(&iter)) != NULL) {

src/db.c (1)

1429-1465: Fix iterator reset selection (has_slot logic inverted).

The cleanup swaps the reset calls, so it resets an uninitialized iterator and skips cleanup for the active one, which can cause crashes/UB and leaks.

🐛 Proposed fix

-    if (!has_slot)
-        kvstoreResetDictIterator(&it.kvs_di);
-    else
-        kvstoreIteratorReset(&it.kvs_it);
+    if (has_slot)
+        kvstoreResetDictIterator(&it.kvs_di);
+    else
+        kvstoreIteratorReset(&it.kvs_it);

🤖 Fix all issues with AI agents

In `@src/debug.c`:
- Around line 216-236: The reset logic in hashTypeResetIterator is using the
wrong encoding check, causing dictResetIterator(&hi->di) to be called for
LISTPACKs instead of hash tables; update hashTypeResetIterator so it checks
hi->encoding == OBJ_ENCODING_HT (not OBJ_ENCODING_LISTPACK) before calling
dictResetIterator(&hi->di), ensuring consistency with hashTypeInitIterator and
the hashTypeIterator structure.
- Around line 144-152: The bug in listTypeResetIterator causes quicklist
iterators to not be released: change the conditional in listTypeResetIterator to
check for OBJ_ENCODING_QUICKLIST (not OBJ_ENCODING_LISTPACK) before calling
quicklistReleaseIterator(li->iter), and after releasing set li->iter = NULL;
ensure the function operates on the listTypeIterator structure used elsewhere
(listTypeIterator, li->iter) so quicklist-encoded lists are properly freed and
listpack paths remain untouched.

In `@src/kvstore.c`:
- Around line 628-642: kvstoreIteratorReset calls freeDictIfNeeded using
kvs_it->didx which can be -1 if iteration never started (set in
kvstoreIteratorInit), causing out-of-bounds access; modify kvstoreIteratorReset
to guard the call to freeDictIfNeeded by only invoking it when kvs_it->didx is a
valid index (e.g., check kvs_it->didx >= 0 and less than the number of dicts in
kvs, using kvs_it->kvs and its dict count) so freeDictIfNeeded is not called for
the sentinel -1 value.

In `@src/module.c`:
- Around line 739-742: The cleanup in listTypeResetIterator currently only
handles OBJ_ENCODING_LISTPACK and omits releasing QUICKLIST iterators, causing
leaks when listTypeInitIterator created a quicklist iterator via
quicklistGetIteratorAtIdx; update listTypeResetIterator to call
quicklistReleaseIterator(...) in the OBJ_ENCODING_QUICKLIST branch before
freeing key->iter so the quicklist iterator is properly released prior to
zfree(key->iter).

In `@src/server.h`:
- Around line 3629-3630: The reset logic in hashTypeResetIterator incorrectly
calls dictResetIterator when the object is LISTPACK-encoded; change the branch
so dictResetIterator is invoked only when the encoding is OBJ_ENCODING_HT, and
for OBJ_ENCODING_LISTPACK perform the appropriate listpack iterator
reset/initialization (the same listpack-reset path used by hashTypeInitIterator
or the listpack iterator helper), ensuring the embedded dictIterator is not
touched for listpacks.
- Around line 3106-3107: The reset logic in listTypeResetIterator incorrectly
checks encodings and can skip releasing quicklist iterators or call the wrong
release on a NULL listpack iterator; update listTypeResetIterator (working with
listTypeIterator and listTypeInitIterator) to check for OBJ_ENCODING_QUICKLIST
when calling quicklistReleaseIterator, and check for OBJ_ENCODING_LISTPACK when
calling listpackReleaseIterator, guard each release with a NULL check, and then
clear the iterator fields (e.g., li->subject, li->zi, li->iter pointers and
indexes) to NULL/0 to avoid leaks or double-frees.

In `@src/sort.c`:
- Around line 398-410: The listTypeResetIterator currently only frees the
iterator when li->encoding == OBJ_ENCODING_LISTPACK, which misses
quicklist-based iterators and causes a leak in callers like sort (see use of
listTypeInitIterator/listTypeResetIterator in src/sort.c); update
listTypeResetIterator in t_list.c to also free the quicklist iterator (handle
OBJ_ENCODING_QUICKLIST) and clear/zero the iterator fields (e.g., set
li->encoding to 0 and any quicklist iterator pointer to NULL) so both listpack
and quicklist iterators are properly released after use.

In `@src/t_hash.c`:
- Around line 1378-1381: The encoding check in hashTypeResetIterator is
inverted: currently it calls dictResetIterator for OBJ_ENCODING_LISTPACK which
is wrong and can dereference hi->di for listpack iterators; change the condition
to check for OBJ_ENCODING_HT so dictResetIterator(&hi->di) is only called when
hi->encoding == OBJ_ENCODING_HT, leaving listpack iterator state untouched (so
stack-allocated listpack iterators won't dereference uninitialized hi->di).

In `@src/t_list.c`:
- Around line 267-270: The function listTypeResetIterator has the encoding check
inverted: it currently checks li->encoding == OBJ_ENCODING_LISTPACK but calls
quicklistReleaseIterator, which should only run for OBJ_ENCODING_QUICKLIST;
update the condition in listTypeResetIterator (operating on the listTypeIterator
struct and its li->encoding) so that quicklistReleaseIterator(li->iter) is
invoked when li->encoding == OBJ_ENCODING_QUICKLIST and ensure any
listpack-specific cleanup (if present) is called when li->encoding ==
OBJ_ENCODING_LISTPACK.

♻️ Duplicate comments (1)

src/sort.c (1)

416-425: Same listTypeResetIterator cleanup concern as above.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf0ba1d and b68edd9.

📒 Files selected for processing (17)

• src/adlist.c
• src/adlist.h
• src/aof.c
• src/cluster.c
• src/cluster_asm.c
• src/db.c
• src/debug.c
• src/kvstore.c
• src/kvstore.h
• src/module.c
• src/pubsub.c
• src/rdb.c
• src/server.h
• src/sort.c
• src/t_hash.c
• src/t_list.c
• src/t_set.c

🧰 Additional context used

🧬 Code graph analysis (15)

src/pubsub.c (1)

src/kvstore.c (4)

• kvstoreInitDictSafeIterator (767-772)
• kvstoreDictIteratorNext (786-793)
• kvstoreResetDictIterator (775-783)
• kvstoreInitDictIterator (760-765)

src/adlist.h (1)

src/adlist.c (1)

• listInitIterator (199-206)

src/cluster_asm.c (1)

src/kvstore.c (4)

• kvstoreInitDictIterator (760-765)
• kvstoreDictIteratorNext (786-793)
• kvstoreResetDictIterator (775-783)
• kvstoreInitDictSafeIterator (767-772)

src/aof.c (4)

src/t_list.c (3)

• listTypeInitIterator (229-246)
• listTypeNext (275-293)
• listTypeResetIterator (267-270)

src/t_set.c (3)

• setTypeInitIterator (317-329)
• setTypeNext (357-385)
• setTypeResetIterator (331-334)

src/t_hash.c (3)

• hashTypeInitIterator (1360-1376)
• hashTypeNext (1385-1471)
• hashTypeResetIterator (1378-1381)

src/kvstore.c (4)

• kvstoreIteratorInit (629-634)
• kvstoreIteratorNext (672-687)
• kvstoreIteratorGetCurrentDictIndex (666-669)
• kvstoreIteratorReset (637-642)

src/sort.c (3)

src/t_list.c (4)

• listTypeInitIterator (229-246)
• listTypeLength (205-213)
• listTypeNext (275-293)
• listTypeResetIterator (267-270)

src/t_set.c (3)

• setTypeInitIterator (317-329)
• setTypeNextObject (394-402)
• setTypeResetIterator (331-334)

src/object.c (1)

• createObject (88-98)

src/kvstore.c (1)

src/dict.c (3)

• dictInitSafeIterator (1153-1157)
• dictInitIterator (1143-1151)
• dictGetKey (1038-1044)

src/cluster.c (5)

src/kvstore.c (4)

• kvstoreInitDictIterator (760-765)
• kvstoreDictIteratorNext (786-793)
• kvstoreResetDictIterator (775-783)
• kvstoreInitDictSafeIterator (767-772)

src/object.c (1)

• kvobjGetKey (234-246)

src/server.h (1)

• dictGetKV (3813-3813)

src/networking.c (1)

• addReplyBulkCBuffer (1141-1146)

src/sds.h (1)

• sdslen (73-86)

src/rdb.c (1)

src/kvstore.c (5)

• kvstoreIteratorInit (629-634)
• kvstoreIteratorNext (672-687)
• kvstoreIteratorGetCurrentDictIndex (666-669)
• kvstoreDictSize (745-751)
• kvstoreIteratorReset (637-642)

src/server.h (3)

src/t_list.c (2)

• listTypeInitIterator (229-246)
• listTypeResetIterator (267-270)

src/t_set.c (2)

• setTypeInitIterator (317-329)
• setTypeResetIterator (331-334)

src/t_hash.c (2)

• hashTypeInitIterator (1360-1376)
• hashTypeResetIterator (1378-1381)

src/db.c (2)

src/kvstore.c (8)

• kvstoreIteratorInit (629-634)
• kvstoreIteratorNext (672-687)
• kvstoreIteratorReset (637-642)
• kvstoreIteratorGetCurrentDictIndex (666-669)
• kvstoreDictSize (745-751)
• kvstoreInitDictSafeIterator (767-772)
• kvstoreDictIteratorNext (786-793)
• kvstoreResetDictIterator (775-783)

src/t_set.c (3)

• setTypeInitIterator (317-329)
• setTypeNext (357-385)
• setTypeResetIterator (331-334)

src/t_hash.c (1)

src/dict.c (3)

• dictInitIterator (1143-1151)
• dictResetIterator (1159-1167)
• dictNext (1183-1219)

src/kvstore.h (1)

src/kvstore.c (5)

• kvstoreIteratorInit (629-634)
• kvstoreIteratorReset (637-642)
• kvstoreInitDictIterator (760-765)
• kvstoreInitDictSafeIterator (767-772)
• kvstoreResetDictIterator (775-783)

src/t_set.c (1)

src/dict.c (3)

• dictInitIterator (1143-1151)
• dictResetIterator (1159-1167)
• dictNext (1183-1219)

src/module.c (4)

src/t_list.c (2)

• listTypeResetIterator (267-270)
• listTypeInitIterator (229-246)

src/zmalloc.c (2)

• zfree (468-485)
• zmalloc (167-171)

src/t_set.c (3)

• setTypeInitIterator (317-329)
• setTypeNextObject (394-402)
• setTypeResetIterator (331-334)

src/object.c (1)

• createObject (88-98)

src/debug.c (4)

src/t_list.c (3)

• listTypeInitIterator (229-246)
• listTypeNext (275-293)
• listTypeResetIterator (267-270)

src/t_set.c (3)

• setTypeInitIterator (317-329)
• setTypeNextObject (394-402)
• setTypeResetIterator (331-334)

src/t_hash.c (3)

• hashTypeInitIterator (1360-1376)
• hashTypeNext (1385-1471)
• hashTypeResetIterator (1378-1381)

src/kvstore.c (3)

• kvstoreIteratorInit (629-634)
• kvstoreIteratorNext (672-687)
• kvstoreIteratorReset (637-642)

🔇 Additional comments (45)

src/adlist.h (1)

61-61: LGTM: stack-iterator init API exposed.

src/sort.c (1)

429-438: LGTM: set iterator now stack-allocated with reset cleanup.

src/rdb.c (1)

1396-1479: LGTM: iterator reset is now consistent across success and error paths.

src/module.c (2)

4536-4538: LGTM: iterator init matches the new list iterator API.

---

11651-11659: LGTM: stack-based set iterator with reset is clean.

src/cluster.c (2)

1054-1066: Iterator migration looks good.

Stack-allocated kvstoreDictIterator with init/next/reset is consistent with the new API and avoids heap allocation overhead.

---

1673-1703: Safe iterator swap is correct here.

Using kvstoreInitDictSafeIterator with kvstoreResetDictIterator preserves delete-while-iterating semantics and removes the allocation.

src/pubsub.c (2)

323-348: Safe iterator usage fits delete-in-loop pattern.

The switch to kvstoreInitDictSafeIterator with reset keeps deletion behavior correct while removing allocation overhead.

---

699-713: Stack iterator migration is clean.

Init/next/reset with a stack kvstoreDictIterator is consistent with the new API and keeps the traversal straightforward.

src/cluster_asm.c (2)

2183-2277: Iterator refactor and error-path reset are consistent.

The stack kvstoreDictIterator plus the werr2 cleanup keeps failure paths safe while removing heap allocations.

---

3359-3380: Safe iterator change is appropriate for in-loop deletions.

Using a stack-based safe iterator with reset maintains correctness during key deletion and avoids allocation.

src/db.c (3)

147-156: LGTM — stack iterator init/next/reset is consistent here.

---

188-195: LGTM — iterator usage looks correct for per-slot size aggregation.

---

1804-1819: LGTM — stack-based set iteration with init/next/reset is clean.

src/aof.c (4)

1923-1958: LGTM — list iterator init/next/reset sequence is consistent.

---

1965-1992: LGTM — set iterator lifecycle looks correct.

---

2109-2159: LGTM — hash iterator is properly initialized and reset on all exits.

---

2433-2508: LGTM — kvstoreIterator usage and werr2 cleanup are correct.

src/kvstore.h (2)

50-63: LGTM — iterator structs are well-defined for stack allocation.

---

96-115: LGTM — iterator init/reset API declarations are consistent with new lifecycle.

src/t_list.c (4)

547-585: LGTM!

The iterator lifecycle is correctly implemented: stack-allocated iterator, initialized before the loop, advanced within the loop, and properly reset after iteration completes. This follows the new pattern consistently.

---

621-639: LGTM!

The iterator pattern for single-element index access is correctly implemented with proper initialization and cleanup.

---

1063-1088: LGTM!

The LPOS command correctly uses the stack-allocated iterator pattern with proper lifecycle management.

---

1115-1138: LGTM!

The LREM command correctly handles bidirectional iteration with conditional initialization based on the toremove parameter sign. The cleanup is properly performed after the loop.

src/t_set.c (10)

317-334: LGTM!

The setTypeInitIterator and setTypeResetIterator implementations correctly follow the new stack-allocated pattern. The initialization properly handles all three encodings (HT, INTSET, LISTPACK), and the reset function correctly calls dictResetIterator only for the HT encoding case. This aligns with the dictInitIterator/dictResetIterator pattern shown in the relevant code snippets.

---

79-95: LGTM!

The maybeConvertToIntset function correctly uses the stack-allocated iterator pattern with proper initialization, iteration, and cleanup.

---

508-565: LGTM!

Both encoding conversion paths (to HT and to LISTPACK) correctly implement the stack-allocated iterator pattern with proper initialization and cleanup.

---

596-604: LGTM!

The HT encoding branch of setTypeDup correctly uses the stack-allocated iterator pattern.

---

994-1013: LGTM!

The CASE 3 path in spopWithCountCommand correctly implements the stack-allocated iterator pattern for transferring the old set to the client.

---

1184-1198: LGTM!

The CASE 2 path in srandmemberWithCountCommand correctly implements the stack-allocated iterator pattern.

---

1241-1257: LGTM!

The CASE 3 path in srandmemberWithCountCommand correctly implements the stack-allocated iterator pattern for building the temporary dictionary.

---

1378-1506: LGTM!

The sinterGenericCommand function correctly uses the stack-allocated iterator to iterate through the smallest set for intersection computation.

---

1555-1586: LGTM!

The smembersCommand function correctly implements the stack-allocated iterator pattern for iterating through all set members.

---

1628-1801: LGTM!

The sunionDiffGenericCommand function correctly reuses the stack-allocated iterator across multiple operations (UNION, DIFF algorithms, and output). Each usage properly follows the init/next/reset pattern, and the iterator is safely reinitialized for each new iteration context.

src/server.h (3)

2769-2775: Embedded dictIterator removes heap allocation overhead.
Looks consistent with the new stack-based iterator lifecycle.

---

2781-2789: hashTypeIterator embedding aligns with the new iterator API.

---

3561-3562: Set iterator init/reset signatures look good.

src/t_hash.c (3)

1360-1376: Iterator init/next migration looks solid.

Switching to stack-based hashTypeIterator with dictInitIterator() and dictNext(&hi->di) is consistent and avoids heap churn.

Also applies to: 1459-1466

---

1621-1651: Stack iterator usage in conversions/dup is clean.

The updated loops use hashTypeIterator on the stack and reset after iteration, which aligns with the new API and avoids allocations.

Also applies to: 1666-1707, 1739-1812

---

3006-3043: Command iteration updates look good.

genericHgetallCommand and hrandfieldWithCountCommand now use stack iterators safely while preserving memory tracking.

Also applies to: 3210-3220

src/kvstore.c (3)

502-524: Stats iteration refactor looks good.

Stack-based kvstoreIterator usage keeps the same behavior while removing allocations.

---

760-783: Dict iterator init/reset updates are consistent.

In-place init/reset mirrors dict iterator semantics and keeps cleanup localized.

---

1061-1195: Tests updated appropriately for new iterator APIs.

The test cases now exercise the new init/reset pattern in both kvstore and dict iterators.

src/debug.c (2)

154-161: LGTM!

Correct usage of the new stack-allocated set iterator pattern. The setTypeResetIterator function properly handles cleanup for HT-encoded sets.

---

298-316: LGTM!

Correct usage of the new stack-allocated kvstore iterator pattern. The init/next/reset lifecycle is properly handled, and the reset function correctly cleans up the dict iterator and calls freeDictIfNeeded.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical: Memory leak due to bug in listTypeResetIterator.

The usage pattern in this file is correct. However, examining the listTypeResetIterator function in src/t_list.c:266-269 reveals a critical bug:

void listTypeResetIterator(listTypeIterator *li) {
    if (li->encoding == OBJ_ENCODING_LISTPACK)  // BUG: should be OBJ_ENCODING_QUICKLIST
        quicklistReleaseIterator(li->iter);
}

The condition is inverted:

• For OBJ_ENCODING_QUICKLIST: li->iter is allocated and must be released, but won't be
• For OBJ_ENCODING_LISTPACK: li->iter is NULL, so calling quicklistReleaseIterator is incorrect

This causes memory leaks for quicklist-encoded lists.

Suggested fix in src/t_list.c

 void listTypeResetIterator(listTypeIterator *li) {
-    if (li->encoding == OBJ_ENCODING_LISTPACK)
+    if (li->encoding == OBJ_ENCODING_QUICKLIST)
         quicklistReleaseIterator(li->iter);
 }

🤖 Prompt for AI Agents

In `@src/debug.c` around lines 144 - 152, The bug in listTypeResetIterator causes
quicklist iterators to not be released: change the conditional in
listTypeResetIterator to check for OBJ_ENCODING_QUICKLIST (not
OBJ_ENCODING_LISTPACK) before calling quicklistReleaseIterator(li->iter), and
after releasing set li->iter = NULL; ensure the function operates on the
listTypeIterator structure used elsewhere (listTypeIterator, li->iter) so
quicklist-encoded lists are properly freed and listpack paths remain untouched.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical: Bug in hashTypeResetIterator - wrong encoding check.

The usage in this file is correct (including proper use of hi.expire_time with . operator on line 232). However, examining hashTypeResetIterator in src/t_hash.c:1377-1380 reveals a bug:

void hashTypeResetIterator(hashTypeIterator *hi) {
    if (hi->encoding == OBJ_ENCODING_LISTPACK)  // BUG: should be OBJ_ENCODING_HT
        dictResetIterator(&hi->di);
}

According to hashTypeInitIterator, the dict iterator is only initialized for OBJ_ENCODING_HT, not LISTPACK. The condition is inverted:

• For OBJ_ENCODING_HT: dict iterator needs reset but won't be
• For OBJ_ENCODING_LISTPACK: no dict iterator exists, but tries to reset it

Suggested fix in src/t_hash.c

 void hashTypeResetIterator(hashTypeIterator *hi) {
-    if (hi->encoding == OBJ_ENCODING_LISTPACK)
+    if (hi->encoding == OBJ_ENCODING_HT)
         dictResetIterator(&hi->di);
 }

🤖 Prompt for AI Agents

In `@src/debug.c` around lines 216 - 236, The reset logic in hashTypeResetIterator
is using the wrong encoding check, causing dictResetIterator(&hi->di) to be
called for LISTPACKs instead of hash tables; update hashTypeResetIterator so it
checks hi->encoding == OBJ_ENCODING_HT (not OBJ_ENCODING_LISTPACK) before
calling dictResetIterator(&hi->di), ensuring consistency with
hashTypeInitIterator and the hashTypeIterator structure.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Guard against empty-iterator reset OOB.

kvstoreIteratorReset always calls freeDictIfNeeded with didx. When iteration never started (e.g., empty kvstore), didx remains -1, leading to out-of-bounds access on kvs->dicts.

🔧 Proposed fix

 void kvstoreIteratorReset(kvstoreIterator *kvs_it) {
     dictIterator *iter = &kvs_it->di;
     dictResetIterator(iter);
     /* In the safe iterator context, we may delete entries. */
-    freeDictIfNeeded(kvs_it->kvs, kvs_it->didx);
+    if (kvs_it->didx >= 0)
+        freeDictIfNeeded(kvs_it->kvs, kvs_it->didx);
 }

🤖 Prompt for AI Agents

In `@src/kvstore.c` around lines 628 - 642, kvstoreIteratorReset calls
freeDictIfNeeded using kvs_it->didx which can be -1 if iteration never started
(set in kvstoreIteratorInit), causing out-of-bounds access; modify
kvstoreIteratorReset to guard the call to freeDictIfNeeded by only invoking it
when kvs_it->didx is a valid index (e.g., check kvs_it->didx >= 0 and less than
the number of dicts in kvs, using kvs_it->kvs and its dict count) so
freeDictIfNeeded is not called for the sentinel -1 value.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for listTypeResetIterator implementation
rg -n "listTypeResetIterator" -C 10 -t c

Repository: qodo-benchmark/redis-combined-coderabbit

Length of output: 13351

---

🏁 Script executed:

#!/bin/bash
# Search for list encoding types and initialization
rg -n "OBJ_ENCODING_QUICKLIST|OBJ_ENCODING_LISTPACK" -t c | head -30
# Also check listTypeInitIterator to see what encodings it handles
rg -n "listTypeInitIterator" -A 15 src/t_list.c | head -50

Repository: qodo-benchmark/redis-combined-coderabbit

Length of output: 4239

---

Fix incomplete cleanup in listTypeResetIterator for QUICKLIST-encoded lists.

The current listTypeResetIterator() implementation (src/t_list.c:267-270) only handles OBJ_ENCODING_LISTPACK encoding and skips cleanup for OBJ_ENCODING_QUICKLIST. Since listTypeInitIterator() allocates quicklist iterators for QUICKLIST-encoded lists (via quicklistGetIteratorAtIdx), these iterators leak when listTypeResetIterator() is called without releasing them. The module code at lines 739-742 will then attempt to free an improperly cleaned iterator struct.

Add a corresponding quicklistReleaseIterator() call in the QUICKLIST case to properly clean up the iterator before zfree(key->iter).

🤖 Prompt for AI Agents

In `@src/module.c` around lines 739 - 742, The cleanup in listTypeResetIterator
currently only handles OBJ_ENCODING_LISTPACK and omits releasing QUICKLIST
iterators, causing leaks when listTypeInitIterator created a quicklist iterator
via quicklistGetIteratorAtIdx; update listTypeResetIterator to call
quicklistReleaseIterator(...) in the OBJ_ENCODING_QUICKLIST branch before
freeing key->iter so the quicklist iterator is properly released prior to
zfree(key->iter).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fix listTypeResetIterator encoding check to avoid leaks/NULL release.
Line 3107 introduces listTypeResetIterator, but in src/t_list.c (Line 266-269) it checks OBJ_ENCODING_LISTPACK while releasing a quicklist iterator, which skips cleanup for quicklist and may call release on NULL for listpack.

🐛 Suggested fix in src/t_list.c

 void listTypeResetIterator(listTypeIterator *li) {
-    if (li->encoding == OBJ_ENCODING_LISTPACK)
-        quicklistReleaseIterator(li->iter);
+    if (li->encoding == OBJ_ENCODING_QUICKLIST) {
+        quicklistReleaseIterator(li->iter);
+        li->iter = NULL;
+    }
 }

🤖 Prompt for AI Agents

In `@src/server.h` around lines 3106 - 3107, The reset logic in
listTypeResetIterator incorrectly checks encodings and can skip releasing
quicklist iterators or call the wrong release on a NULL listpack iterator;
update listTypeResetIterator (working with listTypeIterator and
listTypeInitIterator) to check for OBJ_ENCODING_QUICKLIST when calling
quicklistReleaseIterator, and check for OBJ_ENCODING_LISTPACK when calling
listpackReleaseIterator, guard each release with a NULL check, and then clear
the iterator fields (e.g., li->subject, li->zi, li->iter pointers and indexes)
to NULL/0 to avoid leaks or double-frees.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

hashTypeResetIterator should reset only for HT encoding.
In src/t_hash.c (Line 1377-1380) the reset currently checks OBJ_ENCODING_LISTPACK but calls dictResetIterator. With the embedded dictIterator, that can touch uninitialized state for listpack and skip reset for HT.

🐛 Suggested fix in src/t_hash.c

 void hashTypeResetIterator(hashTypeIterator *hi) {
-    if (hi->encoding == OBJ_ENCODING_LISTPACK)
-        dictResetIterator(&hi->di);
+    if (hi->encoding == OBJ_ENCODING_HT)
+        dictResetIterator(&hi->di);
 }

🤖 Prompt for AI Agents

In `@src/server.h` around lines 3629 - 3630, The reset logic in
hashTypeResetIterator incorrectly calls dictResetIterator when the object is
LISTPACK-encoded; change the branch so dictResetIterator is invoked only when
the encoding is OBJ_ENCODING_HT, and for OBJ_ENCODING_LISTPACK perform the
appropriate listpack iterator reset/initialization (the same listpack-reset path
used by hashTypeInitIterator or the listpack iterator helper), ensuring the
embedded dictIterator is not touched for listpacks.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Potential quicklist iterator leak via listTypeResetIterator.
src/t_list.c lines 266-269 show listTypeResetIterator releasing the quicklist iterator only when li->encoding == OBJ_ENCODING_LISTPACK. With this PR switching callers to listTypeResetIterator, quicklist-based lists may never release their iterators, causing leaks during SORT. Please verify and fix the condition.

🔧 Proposed fix (src/t_list.c)

-void listTypeResetIterator(listTypeIterator *li) {
-    if (li->encoding == OBJ_ENCODING_LISTPACK)
-        quicklistReleaseIterator(li->iter);
-}
+void listTypeResetIterator(listTypeIterator *li) {
+    if (li->encoding == OBJ_ENCODING_QUICKLIST)
+        quicklistReleaseIterator(li->iter);
+}

🤖 Prompt for AI Agents

In `@src/sort.c` around lines 398 - 410, The listTypeResetIterator currently only
frees the iterator when li->encoding == OBJ_ENCODING_LISTPACK, which misses
quicklist-based iterators and causes a leak in callers like sort (see use of
listTypeInitIterator/listTypeResetIterator in src/sort.c); update
listTypeResetIterator in t_list.c to also free the quicklist iterator (handle
OBJ_ENCODING_QUICKLIST) and clear/zero the iterator fields (e.g., set
li->encoding to 0 and any quicklist iterator pointer to NULL) so both listpack
and quicklist iterators are properly released after use.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix wrong encoding check in hashTypeResetIterator.

dictResetIterator is currently triggered for LISTPACK, which leaves HT iterators un-reset and may dereference uninitialized hi->di for listpacks. This can assert/crash when iterators are stack-allocated.

🔧 Proposed fix

 void hashTypeResetIterator(hashTypeIterator *hi) {
-    if (hi->encoding == OBJ_ENCODING_LISTPACK)
+    if (hi->encoding == OBJ_ENCODING_HT)
         dictResetIterator(&hi->di);
 }

📝 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

	void hashTypeResetIterator(hashTypeIterator *hi) {
	if (hi->encoding == OBJ_ENCODING_LISTPACK)
	dictResetIterator(&hi->di);
	}
	void hashTypeResetIterator(hashTypeIterator *hi) {
	if (hi->encoding == OBJ_ENCODING_HT)
	dictResetIterator(&hi->di);
	}

🤖 Prompt for AI Agents

In `@src/t_hash.c` around lines 1378 - 1381, The encoding check in
hashTypeResetIterator is inverted: currently it calls dictResetIterator for
OBJ_ENCODING_LISTPACK which is wrong and can dereference hi->di for listpack
iterators; change the condition to check for OBJ_ENCODING_HT so
dictResetIterator(&hi->di) is only called when hi->encoding == OBJ_ENCODING_HT,
leaving listpack iterator state untouched (so stack-allocated listpack iterators
won't dereference uninitialized hi->di).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical bug: Incorrect encoding check in listTypeResetIterator.

The condition checks for OBJ_ENCODING_LISTPACK but then calls quicklistReleaseIterator(). This appears to be inverted logic - the quicklist iterator should be released when the encoding is OBJ_ENCODING_QUICKLIST, not OBJ_ENCODING_LISTPACK.

🐛 Proposed fix

 void listTypeResetIterator(listTypeIterator *li) {
-    if (li->encoding == OBJ_ENCODING_LISTPACK)
+    if (li->encoding == OBJ_ENCODING_QUICKLIST)
         quicklistReleaseIterator(li->iter);
 }

🧰 Tools

🪛 Cppcheck (2.19.0)

[error] 269-269: #error "Undefined or invalid BYTE_ORDER"

(preprocessorErrorDirective)

🤖 Prompt for AI Agents

In `@src/t_list.c` around lines 267 - 270, The function listTypeResetIterator has
the encoding check inverted: it currently checks li->encoding ==
OBJ_ENCODING_LISTPACK but calls quicklistReleaseIterator, which should only run
for OBJ_ENCODING_QUICKLIST; update the condition in listTypeResetIterator
(operating on the listTypeIterator struct and its li->encoding) so that
quicklistReleaseIterator(li->iter) is invoked when li->encoding ==
OBJ_ENCODING_QUICKLIST and ensure any listpack-specific cleanup (if present) is
called when li->encoding == OBJ_ENCODING_LISTPACK.