Send duplicate multi meet packet only for node which supports it

[hpatro]

This prevents crashes on the older nodes in mixed clusters where some nodes are running 8.0 or older. Mixed clusters often exist temporarily during rolling upgrades.

Fixes: #2341

Would need to backport this to 9.0 and 8.1

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.41%. Comparing base (01a7657) to head (abf06e7).
⚠️ Report is 32 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/cluster_legacy.c	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2840      +/-   ##
============================================
- Coverage     72.46%   72.41%   -0.05%     
============================================
  Files           128      128              
  Lines         70364    70370       +6     
============================================
- Hits          50986    50961      -25     
- Misses        19378    19409      +31     

Files with missing lines	Coverage Δ
src/cluster_legacy.c	87.37% <80.00%> (-0.13%)	⬇️

... and 15 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.

[zuiderkwast]

Bump.

This one is a blocker for us for upgrading to 8.1. During upgrade we have a mixed cluster with 8.0 and 8.1.

[zuiderkwast]

LGTM. Why is this still a draft? Anything missing?

We should include this fix in the next patch release.

[zuiderkwast]

We don't really need this code, because CLUSTER_NODE_MULTI_MEET_SUPPORT == CLUSTER_NODE_LIGHT_HDR_MODULE_SUPPORTED and we already have the same code for CLUSTER_NODE_LIGHT_HDR_MODULE_SUPPORTED. We rely on them being equal for the compatibility logic (e.g. when running a mixed cluster with 8.1.0 and 8.1.5).

[hpatro]

This is dead code but added for folks not to get confused.

[madolson]

Can we not have dead code? I was also confuse while reviewing this code. It's better to just have a note that adds the extra explanation no?

Or better yet. Add a high level FEATURE_FLAGS mask and then do;

sender->flage &= ~FEATURE_FLAGS;
sender->flags |= flags & FEATURE_FLAGS;

[hpatro]

LGTM. Why is this still a draft? Anything missing?

I think the code is good. However, couldn't create a test to reproduce the issue.

[zuiderkwast]

LGTM. Why is this still a draft? Anything missing?

I think the code is good. However, couldn't create a test to reproduce the issue.

OK, then I'd like to merge this as is. I trust it because it modifies only the if added in https://github.com/valkey-io/valkey/pull/1307/files#diff-2515500619600c5a1e7a8d9aaa8761a6071314cd226c3b7ee4d0e054691f0a8eR4976 to resend the MEET.

[madolson]

I dislike this change still. Why are we not fixing the actual fix for the double meets? @hpatro @zuiderkwast We have to maintain this code forever no?

[zuiderkwast]

I dislike this change still.

@madolson I didn't know you dislike the fix.

I felt it was quite urgent. Now, I have already backported this fix to 8.1 and released it in 8.1.5. It was a blocker for us for upgrading to 8.1. We got this crash during upgrade when we have mixed clusters with 8.0 and 8.1.

Why are we not fixing the actual fix for the double meets?

Two reasons:

1. Users would have to upgrade to that 8.0.x patch version before continuing to 8.1 to avoid the crashes.
2. We would have had to backport a number of changes to 8.0. These fixes were depending on each other and other and it became quite large and non-trivial to backport. Hari's conclusion in [CRASH] Crash in Valkey 8.0.3 during downgrade from Valkey 8.1.0 #2341 (comment) was to do this fix instead.

We have to maintain this code forever no?

Yes, we maintain all compatibility checks forever, including if a node supports light-weight pubsub messages, and all things like that. This one follows the same style and it's not more complex than the other checks. Actually it's smaller than most other compatibility checks.