fix: ask the peers, not the node, whether a cluster member is failing

[melancholictheory]

This PR closes #441.

Summary

GetSyncedReplicas and countReadyShards both guard on

if slices.Contains(node.Flags, "fail") || slices.Contains(node.Flags, "pfail") {

and neither half can ever be true. pfail is not a token Valkey emits; CLUSTER_NODE_PFAIL renders as fail?. And node.Flags is the node's own myself line, which never carries a failure flag, because a node does not mark itself failing (clusterCron skips myself when setting PFAIL). A failure flag on a node is set by its peers, not by the node.

Both sites now ask the peers through ClusterState.IsNodeFailed, which already existed and already matches fail and fail?.

Features / Behaviour Changes

countReadyShards no longer counts a shard ready while a node in it, still reachable by the operator, is reported fail? or fail by its peers. Before, that shard passed, and if the flagged node was the primary nothing downstream stopped it either, since IsReplicationInSync returns true for any primary.

GetSyncedReplicas no longer offers such a replica as a failover target. In practice the neighbouring master_link_status != "up" check caught most of these already, so the visible change there is small.

An unreachable node is unaffected: it never enters shard.Nodes, and the node-count check handles it as before.

Implementation

GetSyncedReplicas takes *ClusterState. findFailoverShard, its only production caller, already holds it. countReadyShards already had state in scope.

Testing

make test and pre-commit run --all-files.

The old check looked covered. TestShardState_GetSyncedReplicas built replicas with Flags: {"slave", "fail"} and {"slave", "pfail"}, states that cannot occur, and asserted they were excluded. The test handed the check the exact strings it looked for. That fixture is replaced by a replica whose own scrape is clean and whose primary reports it as fail?, the same shape TestClusterState_IsNodeFailed uses.

countReadyShards had no test; TestCountReadyShards covers a healthy shard and a shard with a present node the primary reports fail?. TestFindFailoverShard gains a case where the only replica is peer-reported failing.

All three new cases fail with the old checks reinstated behind the new signatures and pass with the fix.

Checklist

• This Pull Request is related to one issue.
• Commit message explains what changed and why
• Tests are added or updated.
• Documentation files are updated.
• I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

Signed-off-by: melancholictheory selimvhorst@gmail.com

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9b19fea9-cce4-458c-a9bc-fd53b2a5b180

📥 Commits

Reviewing files that changed from the base of the PR and between 118c675 and ec545a1.

📒 Files selected for processing (6)

• internal/controller/failover.go
• internal/controller/failover_test.go
• internal/controller/ready_shards_test.go
• internal/controller/valkeycluster_controller.go
• internal/valkey/clusterstate.go
• internal/valkey/clusterstate_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The change replaces ineffective self-reported node flag checks with peer-reported failure checks. GetSyncedReplicas, failover selection, and ready-shard counting now use ClusterState.IsNodeFailed. Tests cover fail? reports from peers.

Changes

Peer-reported failure handling

Layer / File(s)	Summary
Cluster state failure detection internal/valkey/clusterstate.go, internal/valkey/clusterstate_test.go	GetSyncedReplicas now accepts ClusterState and excludes replicas that live peers report with fail or fail?. Tests cover this state and update the method calls.
Controller failure consumers internal/controller/failover.go, internal/controller/valkeycluster_controller.go, internal/controller/failover_test.go, internal/controller/ready_shards_test.go	Failover selection passes cluster state to GetSyncedReplicas. Ready-shard counting uses state.IsNodeFailed. Tests verify that peer-reported fail? replicas are excluded.

Suggested reviewers: jdheyburn

Priority: ➖ Normal

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to ec545

The change correctly excludes peer-reported failed replicas from readiness and failover decisions. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: determining cluster member failure from peer reports instead of the node itself.
Description check	✅ Passed	The description follows the repository template and includes the issue, behavior changes, implementation details, limitations, testing, and checklist status. It clearly explains the rationale and test…
Linked Issues check	✅ Passed	Issue #441 requires peer-reported failure checks for countReadyShards and GetSyncedReplicas, including Valkey's fail? flag. The PR uses state.IsNodeFailed(node.Id) in both paths. `IsNodeFailed…
Out of Scope Changes check	✅ Passed	The changes stay within Issue #441. The signature update, caller update, health-check changes, comments, and tests directly support the requested node-failure behavior. No unrelated production behavio…

• Fix all pre-merge checks with AI

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: build linters: plugin(logcheck): plugin "logcheck" not found
The command is terminated due to an error: build linters: plugin(logcheck): plugin "logcheck" not found

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Confidence Score: 5/5

Safe to merge: the changed controller paths correctly exclude peer-reported failed replicas without changing healthy replica eligibility or healthy shard readiness.

What we checked:

• Executed an isolated before-and-after controller contract test covering synced-replica selection and ready-shard counting.
• Observed that the parent revision selected a replica and counted its shard as ready when a reachable peer reported that replica as fail.
• Validated that the updated revision passed both the healthy-peer and peer-reported-fail cases, excluding the reported-failed replica and shard while preserving healthy behavior.
• Compared baseline exit 1 with PR exit 0 and linked the validation to the code regions internal/valkey/clusterstate.go:227-241, 342-360 and to internal/controller/failover.go:60-74 and internal/controller/valkeycluster_controller.go:1522-1548.

Summary

• Uses cluster peers’ CLUSTER NODES views to exclude replicas reported as fail or fail? from proactive failover selection.
• Applies the same peer-reported health check when counting ready shards.
• A before-and-after controller contract check confirmed that the earlier behavior selected a replica and counted its shard ready despite a peer reporting fail?; the updated implementation rejects that replica while retaining healthy behavior.

Merge safety: safe to merge.

_{Reviews (1) · Last reviewed commit: "fix: ask the peers, not the node, whethe..."}