Skip to content
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

Attempt to deflake Device list doesn't change if remote server is down #1142

Merged
merged 3 commits into from
Sep 16, 2021
Merged

Attempt to deflake Device list doesn't change if remote server is down #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8438d52
Attempt to deflake `Device list doesn't change if remote server is down`
Sep 16, 2021
23f4aae
Erik is my linter
Sep 16, 2021
197cf63
Introduce `sync_until_user_in_device_list_id`
Sep 16, 2021
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
15 changes: 10 additions & 5 deletions tests/41end-to-end-keys/06-device-lists.pl
View file
Open in desktop
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
Open in desktop
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