Deflake 'uploading signed devices gets propagated over federation' #1409
Conversation
|
Looks like this is failing on Synapse workers - cache invalidation not propagating properly? I need to look through the logs. Edit: Solved. It was indeed cache invalidation replication not working. |
This test started to fail after implementing element-hq/synapse#18899. The failure appeared to be due to the test not waiting for signatures to propagate over federation. The loop would exit as soon as it saw a 'signatures' key. But the value was an empty dict. A few moments later, the dict would be populated with the key we were waiting for. But while that was being sent over federation, the test would fail and exit abruptly. This commit changes the loop to actually check for the signature we're waiting for, instead of exiting upon seeing the 'signatures' key. This seems like it would be less flaky in general, and prevents the test from failing in this case.
anoadragon453
force-pushed
the
anoa/cache_device_key_signatures
branch
from
9c7bec9 to
46be345
Compare
| my $user2_device_key_id_hash = "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ"; | ||
| my $user2_device_key_id = "ed25519:$user2_device_key_id_hash"; |
There was a problem hiding this comment.
Looks like prior art but how/why are these static?
I would've thought auto-generated device names, etc would make these change.
There was a problem hiding this comment.
The key IDs are determined by the client (typically derived from a public key, as shown here). We could generate keys to use in these tests, but it's not necessary (and would add a smidge more time to running the tests).
I found it actually helps with debugging (being able to reuse grep commands) to have a static key ID.
| exists $sigs->{$user2_id} | ||
| && exists $sigs->{$user2_id}{$user2_device_key_id} | ||
| && $sigs->{$user2_id}{$user2_device_key_id} eq $cross_signature | ||
| or die "Expected cross-signature ($user2_device_key_id}->$cross_signature not visible"; |
There was a problem hiding this comment.
The braces/parentheses are unbalanced and seem wrong here. I don't know which way they should go.
| or die "Expected cross-signature ($user2_device_key_id}->$cross_signature not visible"; | |
| or die "Expected cross-signature {$user2_device_key_id}->$cross_signature not visible"; |
There was a problem hiding this comment.
Ah, this is just string formatting (literally printing (...) in the error). But I see now that using -> made it seem like we're pulling data out of a structure.
Co-authored-by: Eric Eastwood <[email protected]>
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Thanks for the review @MadLittleMods! 🐛
| exists $sigs->{$user2_id} | ||
| && exists $sigs->{$user2_id}{$user2_device_key_id} | ||
| && $sigs->{$user2_id}{$user2_device_key_id} eq $cross_signature | ||
| or die "Expected cross-signature ($user2_device_key_id}->$cross_signature not visible"; |
There was a problem hiding this comment.
Ah, this is just string formatting (literally printing (...) in the error). But I see now that using -> made it seem like we're pulling data out of a structure.
| my $user2_device_key_id_hash = "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ"; | ||
| my $user2_device_key_id = "ed25519:$user2_device_key_id_hash"; |
There was a problem hiding this comment.
The key IDs are determined by the client (typically derived from a public key, as shown here). We could generate keys to use in these tests, but it's not necessary (and would add a smidge more time to running the tests).
I found it actually helps with debugging (being able to reuse grep commands) to have a static key ID.
This test started to fail when ran against element-hq/synapse#18899. The failure appeared to be due to the test not waiting for signatures to propagate over federation. The loop would exit as soon as it saw a 'signatures' key. But the value was an empty dict.
A few moments later, the dict would be populated with the key we were waiting for. But while that was being sent over federation, the test would fail and exit abruptly.
This commit changes the loop to actually check for the signature we're waiting for, instead of exiting upon seeing the 'signatures' key. This seems like it would be less flaky in general, and prevents the test from failing in this case.