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

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#4

Refactor some of ASM and slot-stats functions

---

---

Summary by Gitar

• Cluster initialization refactor: Consolidated ASM and slot-stats setup into new clusterCommonInit() function; moved verifyClusterConfigWithData() from cluster_legacy.c to shared cluster.c
• Slot verification decomposition: Split slot ownership verification into clusterClaimUnassignedSlots() and clusterDeleteKeysInUnownedSlots() for independent reuse across cluster implementations
• Lifecycle management: Moved ASM cron/before-sleep handlers to server.c to integrate with generic cluster lifecycle rather than legacy-specific code

_{This will update automatically on new commits.}

[gitar-bot-staging]

Note

Gitar has paused automatic reviews. Comment "Gitar review" to trigger a review manually.

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));

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]

🚨 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]

⚠️ 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

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}