Fix ping_sent getting stuck when peer traffic keeps link alive#4171
Fix ping_sent getting stuck when peer traffic keeps link alive#4171enjoy-binbin wants to merge 5 commits into
Conversation
The broken-link detection in clusterCron only reconnects a link when no traffic is seen for more than half the node timeout (data_delay > cluster_node_timeout/2). This leaves ping_sent unable to advance in this case: while ping_sent is set we never send another PING (clusterCron skips nodes with an outstanding ping), and ping_sent is only cleared by a PONG. If the peer keeps the link alive with its own PINGs (refreshing the data_received) while our outgoing PING was lost, the link is never reconnected, so ping_sent stays set forever and we can not send PING. Force a reconnect when our PING has been outstanding for a full node timeout without a PONG, independent of data_received. The reconnect itself sends a fresh PING, letting us receive a PONG and clear the stale state. A practical example is a two-node cluster: with no third node to confirm or correct the failure, a lost PING combined with the peer's own PINGs leaves the node permanently stuck in PFAIL state. Signed-off-by: Binbin <binloveplay1314@qq.com>
📝 WalkthroughWalkthroughCluster link timeout handling now forces reconnection when an outstanding PING exceeds the full node timeout. Debug packet-drop commands log configuration changes, and a two-node regression test verifies PFAIL recovery after selective packet loss. ChangesCluster link recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| * is sent and the stale state clears. */ | ||
| if (node->link && | ||
| now - node->link->ctime > server.cluster_node_timeout && | ||
| node->ping_sent && ping_delay > server.cluster_node_timeout) { |
There was a problem hiding this comment.
ping_sent here is the time we queued a PING, not the time it actually reached the wire: clusterSendPing() sets it before appending the packet to link->send_msg_queue (src/cluster_legacy.c:4765-4768, src/cluster_legacy.c:4967-4969). On a backpressured cluster-bus link, the peer can still keep data_received fresh with its own traffic while that queued PING sits behind older messages, which is why the code below still treats incoming data as proof of liveness under load (src/cluster_legacy.c:6381-6384). Disconnecting here will tear down a healthy link and freeClusterLink() drops the unsent queue (src/cluster_legacy.c:1809-1816), so this can lose queued cluster-bus traffic.
This reconnect needs to be gated on the PING having actually been written, or on some other condition that excludes queued-but-unsent PINGs, before forcing the link down.
There was a problem hiding this comment.
That seems to make some sense, we could check it using listLength, but i'm not sure if it's really necessary. @hpatro Do you want to review this PR?
There was a problem hiding this comment.
Adding this can also handle the issue i guess, perhaps both have its own value.
if (node->link && node->ping_sent && ping_delay > server.cluster_node_timeout) {
clusterSendPing(node->link, CLUSTERMSG_TYPE_PING);
}
There was a problem hiding this comment.
Yeah, I can help review this change.
I feel the logic is brittle here, we would perform frequent disconnection and on reconnection we might have the same behaviour. So, we would go on a retry loop.
And for your suggestion on the comment, I think it's more reasonable. I would rather wait for server.cluster_node_timeout/2 than the complete timeout. WDYT?
There was a problem hiding this comment.
About the disconnection and reconnection, we already had this logic, so i think it's actually okay in some ways. My only concern is that it bypasses (or skip) data_received, it was originally added to prevent pub/sub like issue like delaying PONG responses. But i think using the full node-timeout here should also help mitigate this issue. And disconnecting can also handle the connection issue like the comment said. In short, i might lean towards the current code actually.
/* If we are not receiving any data for more than half the cluster
* timeout, reconnect the link: maybe there is a connection
* issue even if the node is alive. */
mstime_t ping_delay = now - node->ping_sent;
mstime_t data_delay = now - node->data_received;
if (node->link && /* is connected */
now - node->link->ctime > server.cluster_node_timeout && /* was not already reconnected */
node->ping_sent && /* we already sent a ping */
/* and we are waiting for the pong more than timeout/2 */
ping_delay > server.cluster_node_timeout / 2 &&
/* and in such interval we are not seeing any traffic at all. */
data_delay > server.cluster_node_timeout / 2) {
/* Disconnect the link, it will be reconnected automatically. */
freeClusterLink(node->link);
}
There was a problem hiding this comment.
Yeah, doesn't seem like a major problem of not receiving a response quick enough from one node. The node information should ideally get refreshed by receiving gossip information from others.
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4171 +/- ##
============================================
+ Coverage 76.83% 76.85% +0.01%
============================================
Files 162 162
Lines 81455 81461 +6
============================================
+ Hits 62585 62605 +20
+ Misses 18870 18856 -14
🚀 New features to boost your workflow:
|
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin
added
the
run-extra-tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/debug.c`:
- Around line 640-641: Update the close-link flag handling in the command logic
around server.debug_cluster_close_link_on_packet_drop to parse the argument with
getLongFromObjectOrReply(), reject any value other than 0 or 1, and return the
parser error or an appropriate command error for invalid input. Assign and log
the validated parsed value instead of calling atoi() multiple times.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bb2a960-fafd-4f6e-a746-4da5780347a7
📒 Files selected for processing (3)
src/cluster_legacy.csrc/debug.ctests/unit/cluster/misc.tcl
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/cluster/misc.tcl
- src/cluster_legacy.c
2 participants
The broken-link detection in clusterCron only reconnects a link when
no traffic is seen for more than half the node timeout (data_delay >
cluster_node_timeout/2).
This leaves ping_sent unable to advance in this case: while ping_sent
is set we never send another PING (clusterCron skips nodes with an
outstanding ping), and ping_sent is only cleared by a PONG. If the peer
keeps the link alive with its own PINGs (refreshing the data_received)
while our outgoing PING was lost, the link is never reconnected, so
ping_sent stays set forever and we can not send PING.
Force a reconnect when our PING has been outstanding for a full node
timeout without a PONG, independent of data_received. The reconnect itself
sends a fresh PING, letting us receive a PONG and clear the stale state.
A practical example is a two-node cluster: with no third node to confirm
or correct the failure, a lost PING combined with the peer's own PINGs
leaves the node permanently stuck in PFAIL state.