Skip to content

Add logging for rejected MFSTART messages in cluster primary#4058

Merged
hpatro merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:mf_log
Jun 26, 2026
Merged

Add logging for rejected MFSTART messages in cluster primary#4058
hpatro merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:mf_log

Conversation

@enjoy-binbin

Copy link
Copy Markdown
Member

When a primary receives an MFSTART (manual failover start) message,
it was silently ignored if the sender was unknown or not a replica of
this primary. This made it impossible to diagnose why a manual failover
was timing out on the replica side — the primary had no indication that
it ever received the request.

In particular, cluster gossip may not have propagated the new replica
relationship to the primary yet, so the primary may legitimately not
recognize the sender as its replica. Without logging, this scenario
leaves no trace on the primary side.

When a primary receives an MFSTART (manual failover start) message,
it was silently ignored if the sender was unknown or not a replica of
this primary. This made it impossible to diagnose why a manual failover
was timing out on the replica side — the primary had no indication that
it ever received the request.

In particular, cluster gossip may not have propagated the new replica
relationship to the primary yet, so the primary may legitimately not
recognize the sender as its replica. Without logging, this scenario
leaves no trace on the primary side.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a28282e0-8083-4cbd-8f06-0c66208c8e83

📥 Commits

Reviewing files that changed from the base of the PR and between d38ff73 and 528b961.

📒 Files selected for processing (1)
  • src/cluster_legacy.c

📝 Walkthrough

Walkthrough

clusterProcessPacket() now handles CLUSTERMSG_TYPE_MFSTART with separate rejection paths for unknown senders and senders that are not this node’s replica, while preserving the existing manual failover setup flow.

Changes

Manual failover packet handling

Layer / File(s) Summary
MFSTART sender checks
src/cluster_legacy.c
clusterProcessPacket() splits the MFSTART sender validation into two explicit branches, logs a notice for each rejected case, and only initializes manual failover state after both checks pass.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding logging for rejected MFSTART messages in the cluster primary.
Description check ✅ Passed The description accurately explains the logging change and the manual failover diagnosis problem it addresses.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/cluster_legacy.c
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.75%. Comparing base (c4cfa0f) to head (528b961).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 33.33% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #4058      +/-   ##
============================================
- Coverage     76.77%   76.75%   -0.02%     
============================================
  Files           162      162              
  Lines         81023    81031       +8     
============================================
- Hits          62206    62199       -7     
- Misses        18817    18832      +15     
Files with missing lines Coverage Δ
src/cluster_legacy.c 88.33% <33.33%> (-0.12%) ⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hpatro hpatro merged commit 9cd063e into valkey-io:unstable Jun 26, 2026
65 checks passed
@enjoy-binbin enjoy-binbin deleted the mf_log branch June 26, 2026 11:29
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