Skip to content

Fix some flaky tests #3430

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

Merged
zuiderkwast merged 7 commits into from
Apr 2, 2026
Merged

Fix some flaky tests #3430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f54a6cc
Use CLIENT REPLY OFF in slave buffer test to avoid output buffer buildup
zuiderkwast Apr 1, 2026
ebea47f
Use CLIENT REPLY OFF in memefficiency test to avoid output buffer bui…
zuiderkwast Apr 1, 2026
b3e6983
Prevent unhandled MOVED exception in replica-migration test
zuiderkwast Apr 1, 2026
5f5e3d7
Prevent unhandled MOVED exception in test_nonempty_replica
zuiderkwast Apr 2, 2026
587566f
Increase wait timeout for failover expiry in failover2 test
zuiderkwast Apr 2, 2026
ed1aa8e
Fix race in dual-channel-replication lazyfree test
zuiderkwast Apr 2, 2026
0eecef6
Merge remote-tracking branch 'valkey/unstable' into fix-maxmemory-tes…
zuiderkwast Apr 2, 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
16 changes: 10 additions & 6 deletions tests/integration/dual-channel-replication.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,15 +1404,17 @@ test "Test dual-channel-replication replica can lazyfree the local buffer" {
fail "replica didn't start sync session in time"
}

# Get the main channel connection id while sync is still in progress.
set replica_main_conn_id [get_client_id_by_last_cmd $primary "psync"]
assert_not_equal $replica_main_conn_id ""

# Adding more data to replica local buffer
set bigstr [string repeat x 1000000]
for {set j 0} {$j < 50} {incr j} {
for {set j 0} {$j < 10} {incr j} {
$primary set key $bigstr
}

# Kill the main channel so that the replica will abort the sync
set replica_main_conn_id [get_client_id_by_last_cmd $primary "psync"]
assert_not_equal $replica_main_conn_id ""
$primary client kill id $replica_main_conn_id

# Wait for replica to abort the sync and lazyfree the local buffer.
Expand Down Expand Up @@ -1453,15 +1455,17 @@ test "Test dual-channel-replication replica can lazyfree the local buffer" {
fail "replica didn't start sync session in time"
}

# Get the main channel connection id while sync is still in progress.
set replica_main_conn_id [get_client_id_by_last_cmd $primary "psync"]
assert_not_equal $replica_main_conn_id ""

# Adding more data to replica local buffer
set bigstr [string repeat x 1000000]
for {set j 0} {$j < 50} {incr j} {
for {set j 0} {$j < 10} {incr j} {
$primary set key $bigstr
}

# Kill the main channel so that the replica will abort the sync
set replica_main_conn_id [get_client_id_by_last_cmd $primary "psync"]
assert_not_equal $replica_main_conn_id ""
$primary client kill id $replica_main_conn_id

# Wait for replica to abort the sync and lazyfree the local buffer.
Expand Down
12 changes: 6 additions & 6 deletions tests/unit/cluster/failover2.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,17 @@ proc test_replica_config_epoch_failover {type} {

# Make sure both the automatic and the manual failover will fail in the first time.
if {$type == "automatic"} {
wait_for_log_messages -3 {"*Failover attempt expired*"} 0 1000 10
wait_for_log_messages -3 {"*Failover attempt expired*"} 0 1200 50
} elseif {$type == "manual"} {
R 3 cluster failover force
wait_for_log_messages -3 {"*Manual failover timed out*"} 0 1000 10
wait_for_log_messages -3 {"*Manual failover timed out*"} 0 1200 50
}

# Make sure the primaries prints the relevant logs.
wait_for_log_messages -1 {"*Failover auth denied to* epoch * > reqConfigEpoch*"} 0 1000 10
wait_for_log_messages -1 {"*has old slots configuration, sending an UPDATE message about*"} 0 1000 10
wait_for_log_messages -2 {"*Failover auth denied to* epoch * > reqConfigEpoch*"} 0 1000 10
wait_for_log_messages -2 {"*has old slots configuration, sending an UPDATE message about*"} 0 1000 10
wait_for_log_messages -1 {"*Failover auth denied to* epoch * > reqConfigEpoch*"} 0 1200 50
wait_for_log_messages -1 {"*has old slots configuration, sending an UPDATE message about*"} 0 1200 50
wait_for_log_messages -2 {"*Failover auth denied to* epoch * > reqConfigEpoch*"} 0 1200 50
wait_for_log_messages -2 {"*has old slots configuration, sending an UPDATE message about*"} 0 1200 50

# Make sure the replica has updated the config epoch.
wait_for_condition 1000 10 {
Expand Down
38 changes: 22 additions & 16 deletions tests/unit/cluster/replica-migration.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,15 @@ proc test_migrated_replica {type} {
R 3 readonly
R 7 readonly
wait_for_condition 1000 50 {
[R 3 get key_991803] == 1024 && [R 3 get key_977613] == 10240 &&
[R 4 get key_991803] == 1024 && [R 4 get key_977613] == 10240 &&
[R 7 get key_991803] == 1024 && [R 7 get key_977613] == 10240
[catch {expr {
[R 3 get key_991803] == 1024 && [R 3 get key_977613] == 10240 &&
[R 4 get key_991803] == 1024 && [R 4 get key_977613] == 10240 &&
[R 7 get key_991803] == 1024 && [R 7 get key_977613] == 10240
}} result] == 0 && $result
} else {
puts "R 3: [R 3 keys *]"
puts "R 4: [R 4 keys *]"
puts "R 7: [R 7 keys *]"
catch {puts "R 3: [R 3 keys *]"}
catch {puts "R 4: [R 4 keys *]"}
catch {puts "R 7: [R 7 keys *]"}
fail "Key not consistent"
}

Expand Down Expand Up @@ -203,11 +205,13 @@ proc test_nonempty_replica {type} {
# Make sure the key exists and is consistent.
R 7 readonly
wait_for_condition 1000 50 {
[R 4 get key_991803] == 1024 &&
[R 7 get key_991803] == 1024
[catch {expr {
[R 4 get key_991803] == 1024 &&
[R 7 get key_991803] == 1024
}} result] == 0 && $result
} else {
puts "R 4: [R 4 get key_991803]"
puts "R 7: [R 7 get key_991803]"
catch {puts "R 4: [R 4 get key_991803]"}
catch {puts "R 7: [R 7 get key_991803]"}
fail "Key not consistent"
}

Expand Down Expand Up @@ -327,13 +331,15 @@ proc test_sub_replica {type} {
R 3 readonly
R 7 readonly
wait_for_condition 1000 50 {
[R 3 get key_991803] == 1024 && [R 3 get key_977613] == 10240 &&
[R 4 get key_991803] == 1024 && [R 4 get key_977613] == 10240 &&
[R 7 get key_991803] == 1024 && [R 7 get key_977613] == 10240
[catch {expr {
[R 3 get key_991803] == 1024 && [R 3 get key_977613] == 10240 &&
[R 4 get key_991803] == 1024 && [R 4 get key_977613] == 10240 &&
[R 7 get key_991803] == 1024 && [R 7 get key_977613] == 10240
}} result] == 0 && $result
} else {
puts "R 3: [R 3 keys *]"
puts "R 4: [R 4 keys *]"
puts "R 7: [R 7 keys *]"
catch {puts "R 3: [R 3 keys *]"}
catch {puts "R 4: [R 4 keys *]"}
catch {puts "R 7: [R 7 keys *]"}
fail "Key not consistent"
}

Expand Down
9 changes: 6 additions & 3 deletions tests/unit/maxmemory.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,15 @@ proc test_slave_buffers {test_name cmd_count payload_len limit_memory pipeline}
# send some 10mb worth of commands that don't increase the memory usage
if {$pipeline == 1} {
set rd_master [valkey_deferring_client -1]
$rd_master client reply off
$rd_master flush
for {set k 0} {$k < $cmd_count} {incr k} {
$rd_master setrange key:0 0 [string repeat A $payload_len]
if {$k % 10000 == 0} {$rd_master flush}
}
for {set k 0} {$k < $cmd_count} {incr k} {
$rd_master read
}
$rd_master client reply on
$rd_master flush
$rd_master read ;# read the +OK from CLIENT REPLY ON
Comment on lines +343 to +347
Copy link
Copy Markdown
Member

@dvkashapov dvkashapov Apr 2, 2026

Choose a reason for hiding this comment

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

Is flush correct here, shouldn't we do client_reply_off_wait_for_server $rd_master like in 87d2330?

Copy link
Copy Markdown
Contributor Author

@zuiderkwast zuiderkwast Apr 2, 2026

Choose a reason for hiding this comment

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

client_reply_off_wait_for_server waits for the server to consume and execute the commands.

I'm not sure it's necessary here. If the replies build up in the $rd_master's incoming TCP buffer, $rd_master will not be able to handle TCP-layer metadata traffic, but now I guess the kernel can handle those TCP sequence numbers and ACK messages better. I'm not 100% sure though.

} else {
for {set k 0} {$k < $cmd_count} {incr k} {
$master setrange key:0 0 [string repeat A $payload_len]
Expand Down
8 changes: 5 additions & 3 deletions tests/unit/memefficiency.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
proc test_memory_efficiency {range} {
r flushall
set rd [valkey_deferring_client]
$rd client reply off
$rd flush
set base_mem [s used_memory]
set written 0
for {set j 0} {$j < 10000} {incr j} {
Expand All @@ -11,9 +13,9 @@ proc test_memory_efficiency {range} {
incr written [string length $val]
incr written 2 ;# A separator is the minimum to store key-value data.
}
for {set j 0} {$j < 10000} {incr j} {
$rd read ; # Discard replies
}
$rd client reply on
$rd flush
$rd read ;# read the +OK from CLIENT REPLY ON

set current_mem [s used_memory]
set used [expr {$current_mem-$base_mem}]
Expand Down
Loading