From 7341bfae2c0f157e50b7a91fe2864f361a8c78ff Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 17 Nov 2021 17:42:43 +0000 Subject: [PATCH 1/8] Attempt to deflake `uploading signed devices gets propagated over federation` --- tests/41end-to-end-keys/08-cross-signing.pl | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index 07b721a3a..8fdc93b8e 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -682,7 +682,17 @@ })->then( sub { sync_until_user_in_device_list( $user1, $user2 ); })->then( sub { - matrix_get_e2e_keys( $user1, $user2_id ); + repeat_until_true { + # Wait until user1 sees signatures uploaded by user2. + # We repeat_until_true because this is sensitive to replication delays. + matrix_get_e2e_keys( $user1, $user2_id ) + ->then(sub { + my ( $content ) = @_; + if ( exists $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"}) { + return $content; + } + }); + }; })->then( sub { my ( $content ) = @_; @@ -739,7 +749,7 @@ sub matrix_upload_signatures { do_request_json_for( $user, method => "POST", - uri => "/unstable/keys/signatures/upload", + uri => "/unstable/keys/signatures/upload", # available under /v3/ for matrix 1.1 content => $signatures, ); } From 841120e62e56a6be3c26a2c2ba4e0a8a54e59dad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 18 Nov 2021 14:12:06 +0000 Subject: [PATCH 2/8] Use Andrew as a linter Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- tests/41end-to-end-keys/08-cross-signing.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index 8fdc93b8e..b94c4d9ba 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -686,9 +686,9 @@ # Wait until user1 sees signatures uploaded by user2. # We repeat_until_true because this is sensitive to replication delays. matrix_get_e2e_keys( $user1, $user2_id ) - ->then(sub { + ->then( sub { my ( $content ) = @_; - if ( exists $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"}) { + if ( exists $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"} ) { return $content; } }); From c820c95ef4f968a20a2390e544b28842e91d188c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 18 Nov 2021 16:27:16 +0000 Subject: [PATCH 3/8] Retry until success --- tests/41end-to-end-keys/08-cross-signing.pl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index b94c4d9ba..49347a47d 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -682,21 +682,21 @@ })->then( sub { sync_until_user_in_device_list( $user1, $user2 ); })->then( sub { - repeat_until_true { + retry_until_success { # Wait until user1 sees signatures uploaded by user2. - # We repeat_until_true because this is sensitive to replication delays. - matrix_get_e2e_keys( $user1, $user2_id ) - ->then( sub { + # We retry_until_success because this is sensitive to replication delays. + matrix_get_e2e_keys( $user1, $user2_id )->then( sub { my ( $content ) = @_; - if ( exists $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"} ) { - return $content; - } + log_if_fail "key query content2", $content; + $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"} + or die "No 'signatures' key present"; + Future->done( $content ); }); }; })->then( sub { my ( $content ) = @_; - log_if_fail "key query content2", $content; + log_if_fail "key query content3", $content; # Check that fetching the devices again returns the new signature assert_json_keys( $content->{device_keys}->{$user2_id}->{$user2_device}, "signatures" ); From 20b4d266de42571c23384bc7bf2fe7b698098263 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 18 Nov 2021 16:27:28 +0000 Subject: [PATCH 4/8] Tell people not to use retry_until_true --- run-tests.pl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/run-tests.pl b/run-tests.pl index bf7f67d27..fde891b87 100755 --- a/run-tests.pl +++ b/run-tests.pl @@ -454,10 +454,16 @@ (&%) } until => sub { $iter > $max_iter || !$_[0]->failure }; } +# /!\ You probably DON'T want to use repeat_until true /!\ +# It means that genuine test failures get turned into test timeouts. +# Prefer retry_until_success, which limits the number of retries. +# # Another wrapper which repeats (with delay) until the block returns a true # value. If the block fails entirely then it aborts, does not retry. sub repeat_until_true(&) { + warnings::warnif("deprecated", + "repeat_until_true is deprecated, use retry_until_success instead"); my ( $code ) = @_; my $delay = 0.1; From 77b4ca197bdb237fc5afd5a0b40846d1b9cb4bc0 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 22 Nov 2021 11:42:26 +0000 Subject: [PATCH 5/8] Rich is my linter Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- run-tests.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/run-tests.pl b/run-tests.pl index fde891b87..baa2dc288 100755 --- a/run-tests.pl +++ b/run-tests.pl @@ -462,8 +462,8 @@ (&%) # value. If the block fails entirely then it aborts, does not retry. sub repeat_until_true(&) { - warnings::warnif("deprecated", - "repeat_until_true is deprecated, use retry_until_success instead"); + warnings::warnif( "deprecated", + "repeat_until_true is deprecated, use retry_until_success instead" ); my ( $code ) = @_; my $delay = 0.1; From eaf37bbdb0f231642b6ceeb462e15fe4f4590bd3 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 22 Nov 2021 11:42:38 +0000 Subject: [PATCH 6/8] Rich is my linter 2 Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- tests/41end-to-end-keys/08-cross-signing.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index 49347a47d..3f5ed31b0 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -688,7 +688,7 @@ matrix_get_e2e_keys( $user1, $user2_id )->then( sub { my ( $content ) = @_; log_if_fail "key query content2", $content; - $content->{device_keys}->{$user2_id}->{$user2_device}->{"signatures"} + $content->{device_keys}{$user2_id}{$user2_device}{"signatures"} or die "No 'signatures' key present"; Future->done( $content ); }); From 5de07ebfe3acb041e007eedbd786b2add1c38588 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 22 Nov 2021 11:50:25 +0000 Subject: [PATCH 7/8] Remove redundant `then` --- tests/41end-to-end-keys/08-cross-signing.pl | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index 3f5ed31b0..d733dc536 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -679,8 +679,6 @@ $user2_device => $device } } ); - })->then( sub { - sync_until_user_in_device_list( $user1, $user2 ); })->then( sub { retry_until_success { # Wait until user1 sees signatures uploaded by user2. From 558935e107340e5782e18a8864378eb3d2215623 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 24 Nov 2021 17:51:04 +0000 Subject: [PATCH 8/8] Expand comment to explain raciness. --- tests/41end-to-end-keys/08-cross-signing.pl | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/41end-to-end-keys/08-cross-signing.pl b/tests/41end-to-end-keys/08-cross-signing.pl index d733dc536..bdb78baa4 100644 --- a/tests/41end-to-end-keys/08-cross-signing.pl +++ b/tests/41end-to-end-keys/08-cross-signing.pl @@ -681,8 +681,17 @@ } ); })->then( sub { retry_until_success { - # Wait until user1 sees signatures uploaded by user2. - # We retry_until_success because this is sensitive to replication delays. + # Wait until user1 sees signatures uploaded by user2. It's _not_ sufficient + # to just wait for user2's device to become visible to user1. + # + # On server1 hosting user 2: + # + # user2 joins a room + # user2 uploads signatures + # + # On server0, user1 syncs until they see user2's device. This is racey: the + # sync may complete before the signatures have uploaded, propagated over + # federation to server 1 and then over replication to the sync worker. matrix_get_e2e_keys( $user1, $user2_id )->then( sub { my ( $content ) = @_; log_if_fail "key query content2", $content;