Skip to content

Refactor some of ASM and slot-stats functions#4

Open
tomerqodo wants to merge 3 commits intoaugment_full_base_refactor_some_of_asm_and_slot-stats_functions_pr4from
augment_full_head_refactor_some_of_asm_and_slot-stats_functions_pr4
Open

Refactor some of ASM and slot-stats functions#4
tomerqodo wants to merge 3 commits intoaugment_full_base_refactor_some_of_asm_and_slot-stats_functions_pr4from
augment_full_head_refactor_some_of_asm_and_slot-stats_functions_pr4

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#4

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Jan 26, 2026

🤖 Augment PR Summary

Summary: Refactors cluster/ASM initialization and cluster slot-data verification by moving shared logic into common entry points.

Changes:

  • Routes CLUSTER MIGRATION and CLUSTER SYNCSLOTS handling through clusterCommand() instead of clusterCommandSpecial().
  • Introduces clusterCommonInit() to initialize shared cluster state (slot-stats + ASM init) before clusterInit().
  • Moves ASM lifecycle hooks (asmCron(), asmBeforeSleep()) out of the legacy cluster implementation into serverCron()/beforeSleep().
  • Refactors startup data/config validation by centralizing verifyClusterConfigWithData() in cluster.c.
  • Splits legacy-specific “claim unassigned slots” logic into clusterClaimUnassignedSlots().

Technical Notes: Startup slot verification now runs deletion/claim helpers, and slot-stats allocation moved from main() to the new common init path.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/cluster.c
}

/* Delete keys in unowned slots */
clusterDeleteKeysInUnownedSlots();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In verifyClusterConfigWithData(), clusterDeleteKeysInUnownedSlots() runs before clusterClaimUnassignedSlots(), but clusterDeleteKeysInUnownedSlots() doesn’t appear to exclude unassigned slots. This can cause keys in unassigned slots to be deleted on startup, preventing the later claim step and potentially causing data loss.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Comment thread 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));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clusterCommonInit() allocates server.cluster_slot_stats via plain malloc() (previously zmalloc() in main()), and then immediately dereferences it via resetClusterStats()/clusterSlotStatResetAll(). If allocation fails, this becomes a NULL deref, and it also bypasses Redis’ zmalloc accounting/oom behavior.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants