From 8438d5243a08ec117a9d72230afb865598d8730b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 16 Sep 2021 11:43:36 +0100 Subject: [PATCH 1/3] Attempt to deflake `Device list doesn't change if remote server is down` Closes #1136 --- tests/50federation/40devicelists.pl | 69 ++++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/tests/50federation/40devicelists.pl b/tests/50federation/40devicelists.pl index 2f11cca98..8a753a512 100644 --- a/tests/50federation/40devicelists.pl +++ b/tests/50federation/40devicelists.pl @@ -485,7 +485,6 @@ }); }; -use Data::Dumper; test "Device list doesn't change if remote server is down", # see https://github.com/matrix-org/synapse/issues/5441 @@ -563,19 +562,36 @@ Future->done(1) }) ); - - # First we succesfully request the remote user's keys while the remote server is up. + my $room_id; + # First we successfully request the remote user's keys while the remote server is up. # We do this once they share a room. - matrix_create_room( + matrix_create_room_synced( $local_user, preset => "public_chat", )->then( sub { - my ( $room_id ) = @_; + ( $room_id ) = @_; $outbound_client->join_room( server_name => $local_user->server_name, room_id => $room_id, user_id => $outbound_client_user, ) + # Before making the key request, we need to ensure the server under test + # recognises that $local_user and $outbound_client_user now share a room. + })->then( sub { + await_sync_timeline_contains( + $local_user, + $room_id, + check => sub { + my ($event) = @_; + return $event->{type} eq "m.room.member" && + $event->{state_key} eq $outbound_client_user && + $event->{content}{membership} eq "join"; + } + ); + })->then( sub { + # Don't expect this sync to do anything, but it sets a next_batch_token + # on the local_user object, which is required by sync_until_user_in_device_list + matrix_sync($local_user) })->then( sub { do_request_json_for( $local_user, method => "POST", @@ -588,9 +604,13 @@ ) })->then( sub { ( $first_keys_query_body ) = @_; + log_if_fail("first_keys_query_body", $first_keys_query_body); + assert_eq( + $first_keys_query_body->{device_keys}->{$outbound_client_user}->{CURIOSITY_ROVER}->{user_id}, + $outbound_client_user, + "outbound client user mxid" + ); map {$_->cancel} @respond_with_keys; - log_if_fail (Dumper $first_keys_query_body); - # We take the remote server 'offline' and then make the same request # for the users keys. We expect no change in the keys. We respond with # 400 instead of 503 to avoid the server being marked as down. @@ -608,6 +628,19 @@ Future->done(1) }) ); + })->then( sub { + # Synapse with workers would often fail this test, because + # - the device list info retrieved over federation was written to DB + # by the master worker + # - the master worker notifies the others over replication, so the + # workers can invalidate their caches + # - the second request (that we're about to send out) would sometimes + # arrive at the stream_writer worker before it'd been notified over + # replication + # + # Sync so that we have a chance for the replication to propagate. + sync_until_user_in_device_list($local_user, new_User(user_id=>$outbound_client_user, http=>"", server_name=>"")); + })->then( sub { do_request_json_for( $local_user, method => "POST", uri => "/r0/keys/query", @@ -616,11 +649,26 @@ $outbound_client_user => [] } } - ) + )->then( sub { + my ($body) = @_; + log_if_fail("Attempted second request", $body); + # Failed requests have an empty device_list and a nonempty failures list. + assert_json_keys($body, "device_keys"); + assert_json_object($body->{device_keys}); + assert_json_keys($body->{device_keys}, $outbound_client_user); + my $size = scalar keys %{$body->{device_keys}}; + $size eq 1 or die "Expected device_keys to contain exactly one entry, not " . $size; + + # Failed requests have an empty device_list and a nonempty failures list. + assert_json_keys($body, "failures"); + $size = scalar keys %{$body->{failures}}; + $size eq 0 or die "Expected failures to be empty, not of size " . $size; + Future->done($body); + }) })->then( sub { ( $second_keys_query_body ) = @_; map {$_->cancel} @respond_400; - # The unsiged field is optional in the spec so we remove it from any response. + # The unsigned field is optional in the spec so we remove it from any response. foreach ($first_keys_query_body, $second_keys_query_body) { while (my ($user_key, $devices) = each %{$_->{ device_keys }} ) { while (my ($device_key, $values) = each %$devices) { @@ -628,8 +676,7 @@ } } }; - - log_if_fail (Dumper $second_keys_query_body); + log_if_fail("second_keys_query_body",$second_keys_query_body); assert_deeply_eq( $second_keys_query_body->{ device_keys }, $first_keys_query_body->{ device_keys }, "Query matches while federation server is down." ); Future->done(1) }) From 23f4aae4da8e965b96b51eafc9f224187ff6465d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 16 Sep 2021 12:56:33 +0100 Subject: [PATCH 2/3] Erik is my linter Co-authored-by: Erik Johnston --- tests/50federation/40devicelists.pl | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/50federation/40devicelists.pl b/tests/50federation/40devicelists.pl index 8a753a512..dab0a78da 100644 --- a/tests/50federation/40devicelists.pl +++ b/tests/50federation/40devicelists.pl @@ -591,7 +591,7 @@ })->then( sub { # Don't expect this sync to do anything, but it sets a next_batch_token # on the local_user object, which is required by sync_until_user_in_device_list - matrix_sync($local_user) + matrix_sync( $local_user ) })->then( sub { do_request_json_for( $local_user, method => "POST", @@ -604,7 +604,7 @@ ) })->then( sub { ( $first_keys_query_body ) = @_; - log_if_fail("first_keys_query_body", $first_keys_query_body); + log_if_fail "first_keys_query_body", $first_keys_query_body; assert_eq( $first_keys_query_body->{device_keys}->{$outbound_client_user}->{CURIOSITY_ROVER}->{user_id}, $outbound_client_user, @@ -650,20 +650,20 @@ } } )->then( sub { - my ($body) = @_; - log_if_fail("Attempted second request", $body); + my ( $body ) = @_; + log_if_fail "Attempted second request", $body; # Failed requests have an empty device_list and a nonempty failures list. - assert_json_keys($body, "device_keys"); - assert_json_object($body->{device_keys}); - assert_json_keys($body->{device_keys}, $outbound_client_user); - my $size = scalar keys %{$body->{device_keys}}; + assert_json_keys( $body, "device_keys" ); + assert_json_object( $body->{device_keys} ); + assert_json_keys( $body->{device_keys}, $outbound_client_user ); + my $size = scalar keys %{ $body->{device_keys} }; $size eq 1 or die "Expected device_keys to contain exactly one entry, not " . $size; # Failed requests have an empty device_list and a nonempty failures list. - assert_json_keys($body, "failures"); - $size = scalar keys %{$body->{failures}}; + assert_json_keys( $body, "failures" ); + $size = scalar keys %{ $body->{failures} }; $size eq 0 or die "Expected failures to be empty, not of size " . $size; - Future->done($body); + Future->done( $body ); }) })->then( sub { ( $second_keys_query_body ) = @_; @@ -676,7 +676,7 @@ } } }; - log_if_fail("second_keys_query_body",$second_keys_query_body); + log_if_fail "second_keys_query_body",$second_keys_query_body; assert_deeply_eq( $second_keys_query_body->{ device_keys }, $first_keys_query_body->{ device_keys }, "Query matches while federation server is down." ); Future->done(1) }) From 197cf6381690ae787b46d806f35e968b8f59292b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 16 Sep 2021 15:51:12 +0100 Subject: [PATCH 3/3] Introduce `sync_until_user_in_device_list_id` Sorry, I didn't want to go around updating every other call site --- tests/41end-to-end-keys/06-device-lists.pl | 15 ++++++++++----- tests/50federation/40devicelists.pl | 8 +++----- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/41end-to-end-keys/06-device-lists.pl b/tests/41end-to-end-keys/06-device-lists.pl index 8968dcc14..b128379ee 100644 --- a/tests/41end-to-end-keys/06-device-lists.pl +++ b/tests/41end-to-end-keys/06-device-lists.pl @@ -1,7 +1,8 @@ use Future::Utils qw( try_repeat_until_success ); #use Devel::StackTrace; -push our @EXPORT, qw( is_user_in_changed_list sync_until_user_in_device_list ); +push our @EXPORT, qw( is_user_in_changed_list sync_until_user_in_device_list + sync_until_user_in_device_list_id ); sub is_user_in_changed_list { @@ -13,17 +14,21 @@ sub is_user_in_changed_list } -# returns a Future which resolves to the body of the sync result which contains -# the change notification sub sync_until_user_in_device_list { my ( $syncing_user, $user_to_wait_for, %params ) = @_; + sync_until_user_in_device_list_id($syncing_user, $user_to_wait_for->user_id, %params) +} + +# returns a Future which resolves to the body of the sync result which contains +# the change notification +sub sync_until_user_in_device_list_id +{ + my ( $syncing_user, $wait_for_id, %params ) = @_; my $device_list = $params{device_list} // 'changed'; my $msg = $params{msg} // 'sync_until_user_in_device_list'; - my $wait_for_id = $user_to_wait_for->user_id; - # my $trace = Devel::StackTrace->new(no_args => 1); # log_if_fail $trace->frame(1)->as_string(); diff --git a/tests/50federation/40devicelists.pl b/tests/50federation/40devicelists.pl index dab0a78da..627434273 100644 --- a/tests/50federation/40devicelists.pl +++ b/tests/50federation/40devicelists.pl @@ -122,8 +122,7 @@ ) ) })->then( sub { - sync_until_user_in_device_list($user, new_User(server_name => "", http => "", user_id => - $creator_id)) + sync_until_user_in_device_list_id($user, $creator_id) })->then( sub { do_request_json_for( $user, method => "POST", @@ -163,8 +162,7 @@ } ) })->then( sub { - sync_until_user_in_device_list($user, new_User(server_name => "", http => "", user_id => - $creator_id)) + sync_until_user_in_device_list_id($user, $creator_id) })->then( sub { do_request_json_for( $user, method => "POST", @@ -639,7 +637,7 @@ # replication # # Sync so that we have a chance for the replication to propagate. - sync_until_user_in_device_list($local_user, new_User(user_id=>$outbound_client_user, http=>"", server_name=>"")); + sync_until_user_in_device_list_id($local_user, $outbound_client_user) })->then( sub { do_request_json_for( $local_user, method => "POST",