Code Review Bench PR #14587 - Refactor some of ASM and slot-stats functions by ketkarameya · Pull Request #4 · gitarcode/redis

ketkarameya · 2026-02-10T22:05:01Z

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#4

Refactor some of ASM and slot-stats functions

Summary by Gitar

Extracts common cluster initialization: New clusterCommonInit() function consolidates slot statistics and ASM initialization, called before clusterInit() to prepare shared data structures for multiple cluster implementations
Refactors slot verification into reusable utilities: Splits verifyClusterConfigWithData() into clusterDeleteKeysInUnownedSlots() and clusterClaimUnassignedSlots(), using abstraction APIs instead of direct struct access for portability
Moves ASM lifecycle to generic event loop: Relocates asmCron() and asmBeforeSleep() from cluster_legacy.c to server.c, enabling Atomic Slot Migration support across all cluster implementations
Reorganizes command routing: Migrates "migration" and "syncslots" handlers from legacy-specific clusterCommandSpecial() to generic clusterCommand() dispatcher

gitar-bot-staging · 2026-02-10T22:14:03Z

Code Review 🚫 Blocked 0 resolved / 2 findings

Critical data loss bug: the two-pass split of slot verification deletes keys in unassigned slots before claiming them, silently destroying data that the old single-pass code preserved. Also, malloc should be zmalloc to maintain Redis memory tracking.

🚨 Bug: Two-pass refactor causes data loss for keys in unassigned slots

📄 src/cluster.c:2209-2212

The original verifyClusterConfigWithData() used a single pass through all slots. For each slot with keys that wasn't owned by this node and wasn't being imported, it checked:\n- If unassigned (NULL owner) → claim the slot (no data loss)\n- If assigned to another node → delete the keys\n\nThe refactored code splits this into two sequential passes:\n1. clusterDeleteKeysInUnownedSlots() — deletes keys in slots we don't own\n2. clusterClaimUnassignedSlots() — claims unassigned slots that have keys\n\nThe problem: clusterIsMySlot(i) returns false for unassigned slots (since getNodeBySlot() returns NULL ≠ myself). So in step 1, keys in unassigned slots are deleted. Then in step 2, countKeysInSlot(i) returns 0 for those same slots (keys were just deleted), so they are skipped and never claimed.\n\nResult: keys in unassigned slots are silently destroyed instead of being preserved by claiming the slot. This is a data loss regression.\n\nFix: Reverse the call order — call clusterClaimUnassignedSlots() before clusterDeleteKeysInUnownedSlots(), so unassigned slots are claimed first (preserving keys), and only then are keys in slots owned by other nodes deleted.

Suggested fix

    /* Take over slots that we have keys for, but are assigned to no one. */
    clusterClaimUnassignedSlots();
    /* Delete keys in unowned slots */
    clusterDeleteKeysInUnownedSlots();

⚠️

Bug: Use of raw malloc instead of zmalloc breaks memory tracking

📄 src/cluster.c:2138

The original code in server.c:main() used zmalloc() to allocate cluster_slot_stats. The new clusterCommonInit() uses raw malloc() instead. This means:\n\n1. The allocation is invisible to Redis's INFO memory reporting and used_memory tracking\n2. Memory fragmentation metrics will be inaccurate\n3. If Redis is configured with maxmemory, this allocation won't count toward the limit\n4. It's inconsistent with every other allocation in cluster.c (all use zmalloc)\n\nThe PR context notes mention "Use malloc instead of zmalloc for allocation (consistent with pattern)" but this appears incorrect — no other allocation in cluster.c uses raw malloc.\n\nFix: Change malloc to zmalloc to match the original code and Redis conventions.

Suggested fix

    server.cluster_slot_stats = zmalloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended 'Summary by Gitar' section with 4 technical bullet points describing cluster initialization refactoring

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

gitar-bot-staging · 2026-02-10T22:16:05Z

src/cluster.c

+    /* Delete keys in unowned slots */
+    clusterDeleteKeysInUnownedSlots();
+    /* Take over slots that we have keys for, but are assigned to no one. */
+    clusterClaimUnassignedSlots();


🚨 Bug: Two-pass refactor causes data loss for keys in unassigned slots

The original verifyClusterConfigWithData() used a single pass through all slots. For each slot with keys that wasn't owned by this node and wasn't being imported, it checked:\n- If unassigned (NULL owner) → claim the slot (no data loss)\n- If assigned to another node → delete the keys\n\nThe refactored code splits this into two sequential passes:\n1. clusterDeleteKeysInUnownedSlots() — deletes keys in slots we don't own\n2. clusterClaimUnassignedSlots() — claims unassigned slots that have keys\n\nThe problem: clusterIsMySlot(i) returns false for unassigned slots (since getNodeBySlot() returns NULL ≠ myself). So in step 1, keys in unassigned slots are deleted. Then in step 2, countKeysInSlot(i) returns 0 for those same slots (keys were just deleted), so they are skipped and never claimed.\n\nResult: keys in unassigned slots are silently destroyed instead of being preserved by claiming the slot. This is a data loss regression.\n\nFix: Reverse the call order — call clusterClaimUnassignedSlots() before clusterDeleteKeysInUnownedSlots(), so unassigned slots are claimed first (preserving keys), and only then are keys in slots owned by other nodes deleted.

_{Was this helpful? React with 👍 / 👎}

Suggested change

/* Delete keys in unowned slots */

clusterDeleteKeysInUnownedSlots();

/* Take over slots that we have keys for, but are assigned to no one. */

clusterClaimUnassignedSlots();

/* Take over slots that we have keys for, but are assigned to no one. */

clusterClaimUnassignedSlots();

/* Delete keys in unowned slots */

clusterDeleteKeysInUnownedSlots();

Apply suggested fix

gitar-bot-staging · 2026-02-10T22:16:06Z

src/cluster.c

+/* This function is called at server startup in order to initialize cluster data
+ * structures that are shared between the different cluster implementations. */
+void clusterCommonInit(void) {
+    server.cluster_slot_stats = malloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));


⚠️ Bug: Use of raw malloc instead of zmalloc breaks memory tracking

The original code in server.c:main() used zmalloc() to allocate cluster_slot_stats. The new clusterCommonInit() uses raw malloc() instead. This means:\n\n1. The allocation is invisible to Redis's INFO memory reporting and used_memory tracking\n2. Memory fragmentation metrics will be inaccurate\n3. If Redis is configured with maxmemory, this allocation won't count toward the limit\n4. It's inconsistent with every other allocation in cluster.c (all use zmalloc)\n\nThe PR context notes mention "Use malloc instead of zmalloc for allocation (consistent with pattern)" but this appears incorrect — no other allocation in cluster.c uses raw malloc.\n\nFix: Change malloc to zmalloc to match the original code and Redis conventions.

_{Was this helpful? React with 👍 / 👎}

Suggested change

server.cluster_slot_stats = malloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));

server.cluster_slot_stats = zmalloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));

Apply suggested fix

tezc and others added 3 commits January 25, 2026 12:12

Refactor some of ASM and slot-stats functions

074e913

better diff

56470f2

update pr

865827b

gitar-bot-staging bot mentioned this pull request Feb 10, 2026

Code Review Bench PR #14227 - Fix HINCRBYFLOAT removes field expiration on replica #8

Open

gitar-bot-staging bot reviewed Feb 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review Bench PR #14587 - Refactor some of ASM and slot-stats functions#4

Code Review Bench PR #14587 - Refactor some of ASM and slot-stats functions#4
ketkarameya wants to merge 3 commits intobase_pr_14587_20260125_4187from
corrupted_pr_14587_20260125_4187

ketkarameya commented Feb 10, 2026 •

edited by gitar-bot-staging bot

Loading

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading

Gitar Rules

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	server.cluster_slot_stats = malloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));
	server.cluster_slot_stats = zmalloc(CLUSTER_SLOTS*sizeof(clusterSlotStat));

Conversation

ketkarameya commented Feb 10, 2026 • edited by gitar-bot-staging bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Gitar

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Gitar Rules

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ketkarameya commented Feb 10, 2026 •

edited by gitar-bot-staging bot

Loading

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading