Refactor some of ASM and slot-stats functions

[tomerqodo]

Benchmark PR from qodo-benchmark#96

Summary by CodeRabbit

• Bug Fixes

  • Enhanced cluster data consistency verification mechanisms during node startup
  • Improved automatic claiming and management of unassigned cluster slots

• Refactor

  • Unified cluster state initialization and maintenance routines
  • Streamlined cluster-wide slot ownership management
  • Removed legacy cluster subcommands for migration and slot synchronization

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

[coderabbitai]

Walkthrough

The pull request refactors cluster initialization and slot management by introducing centralized initialization functions, replacing legacy auxiliary startup steps and cron hooks, and updating server startup and maintenance loops to use the new patterns.

Changes

Cohort / File(s)	Summary
Cluster API Extensions src/cluster.c, src/cluster.h	Adds three new public functions: clusterCommonInit() for shared cluster slot stats and initialization, clusterDeleteKeysInUnownedSlots() for proactive cleanup, and verifyClusterConfigWithData() for data/config consistency validation. Introduces new cluster subcommands "migration" and "syncslots" handling.
Cluster Legacy Refactoring src/cluster_legacy.c	Removes legacy initialization calls (resetClusterStats(), asmInit()), eliminates atomic slot migration cron hook (asmCron), and replaces old consistency checks with new clusterClaimUnassignedSlots() function for slot ownership. Removes "migration" and "syncslots" subcommand branches.
Server Integration src/server.c	Updates server cron to invoke asmCron() before clusterCron() when cluster enabled. Adds asmBeforeSleep() call in pre-sleep maintenance. Replaces direct cluster slot stats allocation with centralized clusterCommonInit() call during startup.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 New slots are claimed, old ghosts are gone,
Init flows centralized, the cluster shines on,
Data and config now perfectly aligned,
Legacy paths swept out, fresh code designed! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.82% 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 directly relates to the main changes in the pull request, which involve refactoring ASM (atomic slot migration) and slot-stats initialization functions across multiple files.

_{✏️ 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: 1

🤖 Fix all issues with AI agents

In `@src/cluster.c`:
- Around line 2135-2141: The allocation for server.cluster_slot_stats in
clusterCommonInit uses deprecated malloc; replace it with Redis's allocator
(e.g., zmalloc or zcalloc) so CI doesn't fail. Update the assignment of
server.cluster_slot_stats to use zmalloc/zcalloc with the size computed from
CLUSTER_SLOTS and sizeof(clusterSlotStat) (or use zcalloc to get zeroed memory
if resetClusterStats relies on zeroed state), and ensure any necessary include
for the Redis allocator is present; keep the rest of clusterCommonInit
(resetClusterStats, asmInit) unchanged.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95d21a5 and 7bbc514.

📒 Files selected for processing (4)

• src/cluster.c
• src/cluster.h
• src/cluster_legacy.c
• src/server.c

🧰 Additional context used

🧬 Code graph analysis (4)

src/cluster.c (3)

src/cluster_asm.c (2)

• clusterMigrationCommand (992-1007)
• clusterSyncSlotsCommand (1764-2091)

src/cluster_legacy.c (4)

• clusterNodeIsSlave (5923-5925)
• getMyClusterNode (5832-5834)
• getImportingSlotSource (6491-6493)
• clusterClaimUnassignedSlots (5262-5284)

src/kvstore.c (1)

• kvstoreSize (329-331)

src/cluster.h (1)

src/cluster.c (1)

• clusterCommonInit (2137-2141)

src/server.c (3)

src/cluster_asm.c (2)

• asmCron (2507-2576)
• asmBeforeSleep (2463-2505)

src/cluster_legacy.c (2)

• clusterCron (4731-4935)
• clusterBeforeSleep (4942-4976)

src/cluster.c (1)

• clusterCommonInit (2137-2141)

src/cluster_legacy.c (1)

src/cluster.c (1)

• countKeysInSlot (820-822)

🪛 GitHub Actions: CI

src/cluster.c

[error] 2138-2138: clusterCommonInit: 'malloc' is deprecated [-Werror=deprecated-declarations]. Warnings are treated as errors during build.

src/cluster.h

[error] 1-1: Command failed: make -C src all (cluster.o build error).

src/server.c

[error] 1-1: Command failed: make -C src all (cluster.o build error).

src/cluster_legacy.c

[error] 1-1: Command failed: make -C src all (cluster.o build error).

🪛 GitHub Actions: Codecov

src/cluster.c

[error] 2138-2138: malloc is deprecated and treated as an error by the compiler (-Werror=deprecated-declarations). This usage occurs in clusterCommonInit at cluster.c:2138.

🪛 GitHub Actions: External Server Tests

src/cluster.c

[error] 2138-2138: Command 'make REDIS_CFLAGS=-Werror' failed: 'malloc' is deprecated (deprecated-declarations) in clusterCommonInit. Remove deprecated usage or replace with a supported allocation pattern.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Analyze (cpp)
• GitHub Check: build-old-chain-jemalloc
• GitHub Check: build-old-chain-jemalloc

🔇 Additional comments (9)

src/cluster.h (1)

86-92: No action required. The declarations for clusterCommonInit() (line 89) and clusterClaimUnassignedSlots() (line 92) are both properly implemented. Both cluster.o and cluster_legacy.o are unconditionally linked in the standard build, so there is no risk of link errors from missing definitions.

src/server.c (3)

1657-1660: LGTM — ASM cron integrated ahead of cluster cron.
Line 1657-1660: Running asmCron() before clusterCron() keeps ASM task retries/timeouts progressing without changing cluster tick cadence.

---

1841-1844: LGTM — ASM before-sleep hook added alongside cluster maintenance.
Line 1841-1844: ASM pre-sleep work is still executed before blockedBeforeSleep(), so task state can advance without delaying blocked-client handling.

---

7739-7741: Code wiring is correct; no build issues detected.

Verification confirms:

• clusterCommonInit prototype (cluster.h:89) matches definition (cluster.c:2137)
• Slot-stats (server.cluster_slot_stats) allocated exactly once at cluster.c:2138 inside clusterCommonInit()
• Function call sequence at server.c:7740 correctly invokes clusterCommonInit() before clusterInit()

The dependency chain is properly established and no prototype/definition mismatches or multiple initialization sites exist.

src/cluster.c (4)

1092-1095: LGTM: ASM subcommands are now wired into clusterCommand.

The routing looks consistent with the existing dispatch pattern.

---

2126-2134: Reset path looks consistent.

The guard on server.cluster_enabled keeps this safe during non-cluster startup.

---

2143-2169: LGTM: unowned slot cleanup is safely gated to masters.

The checks for ownership/importing align with the intended cleanup behavior.

---

2172-2214: LGTM: startup verification flow is clear and centralized.

The sequence (DB check → delete unowned → claim unassigned) reads well.

src/cluster_legacy.c (1)

5259-5284: No action needed. clusterClaimUnassignedSlots() is called only once at server startup within verifyClusterConfigWithData(), which is invoked in src/server.c line 7768 during the server initialization sequence (after loading data from disk and before accepting connections). There is no risk of repeated invocations causing latency, and deferral is not necessary.

Likely an incorrect or invalid review comment.

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

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Replace deprecated malloc to fix CI build failure (Line 2138).

The pipeline is failing on -Werror=deprecated-declarations. Use Redis’s allocator instead.

🐛 Proposed fix

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

🧰 Tools

🪛 GitHub Actions: CI

[error] 2138-2138: clusterCommonInit: 'malloc' is deprecated [-Werror=deprecated-declarations]. Warnings are treated as errors during build.

🪛 GitHub Actions: Codecov

[error] 2138-2138: malloc is deprecated and treated as an error by the compiler (-Werror=deprecated-declarations). This usage occurs in clusterCommonInit at cluster.c:2138.

🪛 GitHub Actions: External Server Tests

[error] 2138-2138: Command 'make REDIS_CFLAGS=-Werror' failed: 'malloc' is deprecated (deprecated-declarations) in clusterCommonInit. Remove deprecated usage or replace with a supported allocation pattern.

🤖 Prompt for AI Agents

In `@src/cluster.c` around lines 2135 - 2141, The allocation for
server.cluster_slot_stats in clusterCommonInit uses deprecated malloc; replace
it with Redis's allocator (e.g., zmalloc or zcalloc) so CI doesn't fail. Update
the assignment of server.cluster_slot_stats to use zmalloc/zcalloc with the size
computed from CLUSTER_SLOTS and sizeof(clusterSlotStat) (or use zcalloc to get
zeroed memory if resetClusterStats relies on zeroed state), and ensure any
necessary include for the Redis allocator is present; keep the rest of
clusterCommonInit (resetClusterStats, asmInit) unchanged.