Fix race which caused deleted devices to reappear (#6514)

Stop the `update_client_ips` background job from recreating deleted devices.
This commit is contained in:
Richard van der Hoff 2019-12-10 16:22:29 +00:00 committed by GitHub
parent c3dda2874d
commit 40eda84933
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 35 additions and 23 deletions

1
changelog.d/6514.bugfix Normal file
View File

@ -0,0 +1 @@
Fix race which occasionally caused deleted devices to reappear.

View File

@ -451,16 +451,18 @@ class ClientIpStore(ClientIpBackgroundUpdateStore):
# Technically an access token might not be associated with # Technically an access token might not be associated with
# a device so we need to check. # a device so we need to check.
if device_id: if device_id:
self.db.simple_upsert_txn( # this is always an update rather than an upsert: the row should
# already exist, and if it doesn't, that may be because it has been
# deleted, and we don't want to re-create it.
self.db.simple_update_txn(
txn, txn,
table="devices", table="devices",
keyvalues={"user_id": user_id, "device_id": device_id}, keyvalues={"user_id": user_id, "device_id": device_id},
values={ updatevalues={
"user_agent": user_agent, "user_agent": user_agent,
"last_seen": last_seen, "last_seen": last_seen,
"ip": ip, "ip": ip,
}, },
lock=False,
) )
except Exception as e: except Exception as e:
# Failed to upsert, log and continue # Failed to upsert, log and continue

View File

@ -37,9 +37,13 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
self.reactor.advance(12345678) self.reactor.advance(12345678)
user_id = "@user:id" user_id = "@user:id"
device_id = "MY_DEVICE"
# Insert a user IP
self.get_success(self.store.store_device(user_id, device_id, "display name",))
self.get_success( self.get_success(
self.store.insert_client_ip( self.store.insert_client_ip(
user_id, "access_token", "ip", "user_agent", "device_id" user_id, "access_token", "ip", "user_agent", device_id
) )
) )
@ -47,14 +51,14 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
self.reactor.advance(10) self.reactor.advance(10)
result = self.get_success( result = self.get_success(
self.store.get_last_client_ip_by_device(user_id, "device_id") self.store.get_last_client_ip_by_device(user_id, device_id)
) )
r = result[(user_id, "device_id")] r = result[(user_id, device_id)]
self.assertDictContainsSubset( self.assertDictContainsSubset(
{ {
"user_id": user_id, "user_id": user_id,
"device_id": "device_id", "device_id": device_id,
"ip": "ip", "ip": "ip",
"user_agent": "user_agent", "user_agent": "user_agent",
"last_seen": 12345678000, "last_seen": 12345678000,
@ -209,14 +213,16 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
self.store.db.updates.do_next_background_update(100), by=0.1 self.store.db.updates.do_next_background_update(100), by=0.1
) )
# Insert a user IP
user_id = "@user:id" user_id = "@user:id"
device_id = "MY_DEVICE"
# Insert a user IP
self.get_success(self.store.store_device(user_id, device_id, "display name",))
self.get_success( self.get_success(
self.store.insert_client_ip( self.store.insert_client_ip(
user_id, "access_token", "ip", "user_agent", "device_id" user_id, "access_token", "ip", "user_agent", device_id
) )
) )
# Force persisting to disk # Force persisting to disk
self.reactor.advance(200) self.reactor.advance(200)
@ -224,7 +230,7 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
self.get_success( self.get_success(
self.store.db.simple_update( self.store.db.simple_update(
table="devices", table="devices",
keyvalues={"user_id": user_id, "device_id": "device_id"}, keyvalues={"user_id": user_id, "device_id": device_id},
updatevalues={"last_seen": None, "ip": None, "user_agent": None}, updatevalues={"last_seen": None, "ip": None, "user_agent": None},
desc="test_devices_last_seen_bg_update", desc="test_devices_last_seen_bg_update",
) )
@ -232,14 +238,14 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
# We should now get nulls when querying # We should now get nulls when querying
result = self.get_success( result = self.get_success(
self.store.get_last_client_ip_by_device(user_id, "device_id") self.store.get_last_client_ip_by_device(user_id, device_id)
) )
r = result[(user_id, "device_id")] r = result[(user_id, device_id)]
self.assertDictContainsSubset( self.assertDictContainsSubset(
{ {
"user_id": user_id, "user_id": user_id,
"device_id": "device_id", "device_id": device_id,
"ip": None, "ip": None,
"user_agent": None, "user_agent": None,
"last_seen": None, "last_seen": None,
@ -272,14 +278,14 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
# We should now get the correct result again # We should now get the correct result again
result = self.get_success( result = self.get_success(
self.store.get_last_client_ip_by_device(user_id, "device_id") self.store.get_last_client_ip_by_device(user_id, device_id)
) )
r = result[(user_id, "device_id")] r = result[(user_id, device_id)]
self.assertDictContainsSubset( self.assertDictContainsSubset(
{ {
"user_id": user_id, "user_id": user_id,
"device_id": "device_id", "device_id": device_id,
"ip": "ip", "ip": "ip",
"user_agent": "user_agent", "user_agent": "user_agent",
"last_seen": 0, "last_seen": 0,
@ -296,11 +302,14 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
self.store.db.updates.do_next_background_update(100), by=0.1 self.store.db.updates.do_next_background_update(100), by=0.1
) )
# Insert a user IP
user_id = "@user:id" user_id = "@user:id"
device_id = "MY_DEVICE"
# Insert a user IP
self.get_success(self.store.store_device(user_id, device_id, "display name",))
self.get_success( self.get_success(
self.store.insert_client_ip( self.store.insert_client_ip(
user_id, "access_token", "ip", "user_agent", "device_id" user_id, "access_token", "ip", "user_agent", device_id
) )
) )
@ -324,7 +333,7 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
"access_token": "access_token", "access_token": "access_token",
"ip": "ip", "ip": "ip",
"user_agent": "user_agent", "user_agent": "user_agent",
"device_id": "device_id", "device_id": device_id,
"last_seen": 0, "last_seen": 0,
} }
], ],
@ -347,14 +356,14 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
# But we should still get the correct values for the device # But we should still get the correct values for the device
result = self.get_success( result = self.get_success(
self.store.get_last_client_ip_by_device(user_id, "device_id") self.store.get_last_client_ip_by_device(user_id, device_id)
) )
r = result[(user_id, "device_id")] r = result[(user_id, device_id)]
self.assertDictContainsSubset( self.assertDictContainsSubset(
{ {
"user_id": user_id, "user_id": user_id,
"device_id": "device_id", "device_id": device_id,
"ip": "ip", "ip": "ip",
"user_agent": "user_agent", "user_agent": "user_agent",
"last_seen": 0, "last_seen": 0,