Skip to content

Fix ping_sent getting stuck when peer traffic keeps link alive #4171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
enjoy-binbin wants to merge 5 commits into
base: unstable
Choose a base branch
from
Open

Fix ping_sent getting stuck when peer traffic keeps link alive #4171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
936e0d9
Fix ping_sent getting stuck when peer traffic keeps link alive
enjoy-binbin Jul 15, 2026
a1c57ee
More test code
enjoy-binbin Jul 16, 2026
cafb58a
Merge remote-tracking branch 'upstream/unstable' into fix_ping_stuck
enjoy-binbin Jul 16, 2026
b412c7d
Merge remote-tracking branch 'upstream/unstable' into fix_ping_stuck
enjoy-binbin Jul 17, 2026
44b401a
Try to fix the test race, allow ping_sent == 0
enjoy-binbin Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/cluster_legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6339,6 +6339,18 @@ void clusterCron(void) {
freeClusterLink(node->link);
}

/* In some situations the check above cannot disconnect the link,
* because data_received keeps being refreshed by the peer's own PINGs
* even though our PING was lost. If our PING stays outstanding for a
* full node timeout without a PONG, force a reconnect so a fresh PING
* 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) {

@valkey-review-bot valkey-review-bot Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@enjoy-binbin enjoy-binbin Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

@enjoy-binbin enjoy-binbin Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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);
}

@hpatro hpatro Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@enjoy-binbin enjoy-binbin Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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);
        }

@hpatro hpatro Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

/* Disconnect the link, it will be reconnected automatically. */
freeClusterLink(node->link);
}

/* If we have currently no active ping in this instance, and the
* received PONG is older than half the cluster timeout, send
* a new ping now, to ensure all the nodes are pinged without
Expand Down
2 changes: 2 additions & 0 deletions src/debug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,9 +634,11 @@ void debugCommand(client *c) {
long packet_type;
if (getLongFromObjectOrReply(c, c->argv[2], &packet_type, NULL) != C_OK) return;
server.cluster_drop_packet_filter = packet_type;
serverLog(LL_NOTICE, "Setting drop-cluster-packet-filter to %ld", packet_type);
addReply(c, shared.ok);
} else if (!strcasecmp(objectGetVal(c->argv[1]), "close-cluster-link-on-packet-drop") && c->argc == 3) {
server.debug_cluster_close_link_on_packet_drop = atoi(objectGetVal(c->argv[2]));
serverLog(LL_NOTICE, "Setting close-cluster-link-on-packet-drop to %d", atoi(objectGetVal(c->argv[2])));
Comment thread
enjoy-binbin marked this conversation as resolved.
addReply(c, shared.ok);
} else if (!strcasecmp(objectGetVal(c->argv[1]), "disable-cluster-random-ping") && c->argc == 3) {
server.debug_cluster_disable_random_ping = atoi(objectGetVal(c->argv[2]));
Expand Down
58 changes: 58 additions & 0 deletions tests/unit/cluster/misc.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,61 @@ start_cluster 3 0 {tags {external:skip cluster} overrides {cluster-require-full-
wait_for_cluster_state ok
}
}

start_cluster 2 0 {tags {cluster external:skip needs:debug}} {
test "A lost PING does not leave a node stuck in PFAIL when the peer keeps sending PINGs" {
set CLUSTER_PACKET_TYPE_MEET 2
set CLUSTER_PACKET_TYPE_NONE -1
set CLUSTER_PACKET_TYPE_ALL -2
set R1_nodeid [R 1 cluster myid]

# Configure different timeout values to better reproduce the issue.
R 0 config set cluster-node-timeout 15000
R 1 config set cluster-node-timeout 1500

# Drop all packets on R0 and wait for pfail.
R 0 debug drop-cluster-packet-filter $CLUSTER_PACKET_TYPE_ALL
wait_for_condition 1000 50 {
[cluster_has_flag [cluster_get_node_by_id 0 $R1_nodeid] "fail?"]
} else {
puts "R 0 cluster nodes:"
puts [R 0 cluster nodes]
fail "R0 did not mark R1 as PFAIL"
}

# Remember some information after the PFAIL.
set R0_ping_sent [dict get [cluster_get_node_by_id 0 $R1_nodeid] ping_sent]
set R0_ping_received [CI 0 cluster_stats_messages_ping_received]

# Restore the DEBUG setting on R0, but exclude MEET first to avoid
# multiple MEET reconnections. We will restore it after R0 receives
# the PING and refreshes data_received.
R 0 debug drop-cluster-packet-filter $CLUSTER_PACKET_TYPE_MEET
wait_for_condition 1000 50 {
[CI 0 cluster_stats_messages_ping_received] > $R0_ping_received
} else {
fail "R0 did not receive the PING"
}
R 0 debug drop-cluster-packet-filter $CLUSTER_PACKET_TYPE_NONE

# Ensure ping_sent does not get stuck and R0 can send PINGs.
wait_for_condition 1000 50 {
[dict get [cluster_get_node_by_id 0 $R1_nodeid] ping_sent] != $R0_ping_sent
} else {
puts "R 0 cluster nodes:"
puts [R 0 cluster nodes]
fail "R0 did not send the PING"
}

# All packets are being sent and received normally, and R0 should be
# able to remove the PFAIL flag.
wait_for_condition 1000 50 {
![cluster_has_flag [cluster_get_node_by_id 0 $R1_nodeid] "fail?"]
} else {
puts "R 0 cluster nodes:"
puts [R 0 cluster nodes]
fail "R0 did not remove the PFAIL flag"
}
wait_for_cluster_state ok
}
}
Loading