Attempt to deflake `Device list doesn't change if remote server is do…

…wn` (#1142)

* Attempt to deflake `Device list doesn't change if remote server is down`

Hopefully closes #1136

Co-authored-by: Erik Johnston <erik@matrix.org>

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

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",
@@ -485,7 +483,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 +560,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 +602,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 +626,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_id($local_user, $outbound_client_user)
+      })->then( sub {
          do_request_json_for( $local_user,
             method  => "POST",
             uri     => "/r0/keys/query",
@@ -616,20 +647,34 @@
                   $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) {
                   delete $values->{ unsigned };
                }
             }
          };
-
-         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)
       })