crypto.keyring: TypeError '<' not supported between instances of 'NoneType' and 'int'

[evilham]

Description

From the logs:

2019-06-07_11:31:30.41626     return g.throw(self.type, self.value, self.tb)
2019-06-07_11:31:30.41626   File "/var/local/matrix/venv/lib/python3.7/site-packages/synapse/crypto/keyring.py", line 350, in do_iterations
2019-06-07_11:31:30.41626     yield self._attempt_key_fetches_with_fetcher(f, remaining_requests)
2019-06-07_11:31:30.41627   File "/var/local/matrix/venv/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
2019-06-07_11:31:30.41627     result = g.send(result)
2019-06-07_11:31:30.41627   File "/var/local/matrix/venv/lib/python3.7/site-packages/synapse/crypto/keyring.py", line 425, in _attempt_key_fetches_with_fetcher
2019-06-07_11:31:30.41627     < verify_request.minimum_valid_until_ts
2019-06-07_11:31:30.41628 TypeError: '<' not supported between instances of 'NoneType' and 'int'

Steps to reproduce

• Start matrix 1.0.0rc1
• Check the logs
• Sometimes this will show up

Version information

• Version: 1.0.0rc1

• Install method: pip

Patch

It looks like issue was introduced here:

synapse/synapse/crypto/keyring.py

Line 419 in 8dc1eb6

if not fetch_key_result:

Where the expectation is: "either fetch_key_result = result_keys.get(key_id) results in a None var, or in real values and it is not being the case, with fetch_key_result.valid_until_ts being None

Either Fetcher is to be fixed or L419 should check that both fetch_key_result and fetch_key_result.valid_until_ts have a valid value.

FWIW, the second option can be: replacing L419 with
if not (fetch_key_result and fetch_key_result.valid_until_ts):

Without knowing the details of Fetcher it's hard for me to know which way a fix should go.

[fredcy]

I was encountering many of the same TypeError exceptions after upgrading to 1.0.0rc1. I then applied the above patch and after restarting those exceptions are gone. So the patch seems to work for me.

[erikjohnston]

This seems most likely due to ts_valid_until_ms being null in the database. I think the correct thing here would be to treat it as 0, i.e. if we set min_valid_until_ms then we try and refetch, otherwise we just accept it.