Skip to content

Commit

Permalink
Attempt to deflake `Device list doesn't change if remote server is do…
Browse files Browse the repository at this point in the history 
…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>
  • Loading branch information
@erikjohnston
David Robertson and erikjohnston authored Sep 16, 2021
1 parent 644071a commit 5a00f01
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 20 deletions.
15 changes: 10 additions & 5 deletions tests/41end-to-end-keys/06-device-lists.pl
View file
Original file line number Diff line number Diff line change
@@ -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
{
Expand All @@ -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();

Expand Down
75 changes: 60 additions & 15 deletions tests/50federation/40devicelists.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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",
Expand All @@ -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.
Expand All @@ -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",
Expand All @@ -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)
})
Expand Down

0 comments on commit 5a00f01

Please sign in to comment.