Refactor some of ASM and slot-stats functions#4
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| * legacy slot migration approach either, since the importing state | ||
| * has already been persisted in node.conf. */ | ||
| clusterDelKeysInSlot(i, 0); | ||
| } |
There was a problem hiding this comment.
Data loss: keys deleted from unassigned slots before claiming
High Severity
The clusterDeleteKeysInUnownedSlots() function deletes keys from any slot where clusterIsMySlot(i) returns false. However, for unassigned slots (owner is NULL), clusterIsMySlot() also returns false because getNodeBySlot() returns NULL. This causes keys in unassigned slots to be deleted instead of preserved for claiming. When clusterClaimUnassignedSlots() runs afterward, the slots have no keys and won't be claimed, resulting in data loss. The original code explicitly checked server.cluster->slots[j] == NULL to distinguish unassigned slots from slots owned by others.
Additional Locations (1)
| /* 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)); |
There was a problem hiding this comment.
Using malloc instead of zmalloc breaks memory tracking
Medium Severity
The clusterCommonInit() function uses malloc() for allocating server.cluster_slot_stats, but the original code in server.c used zmalloc(). Redis uses zmalloc for memory tracking and accounting. Using plain malloc bypasses this system, causing inaccurate memory usage reporting and potential inconsistencies.
2 participants
Benchmark PR from agentic-review-benchmarks#4
Note
clusterCommonInit()sets upcluster_slot_statsand callsasmInit(); used at startup beforeclusterInit().asmCron()andasmBeforeSleep()now run fromserver.calongsideclusterCron()/clusterBeforeSleep()instead of legacy cluster paths.verifyClusterConfigWithData()now delegates toclusterDeleteKeysInUnownedSlots()andclusterClaimUnassignedSlots()(legacy impl) to drop keys in non-owned slots and take ownership of unassigned slots with data.CLUSTER migrationandCLUSTER syncslotshandled in commonclusterCommand(); removed from legacy handler.clusterCommonInit()andclusterClaimUnassignedSlots()declarations.Written by Cursor Bugbot for commit 865827b. Configure here.