Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix race which caused deleted devices to reappear #6514

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Fix race which caused deleted devices to reappear #6514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca67b78
Fix race which caused deleted devices to reappear
richvdh Dec 10, 2019
9216466
fix test fails
richvdh Dec 10, 2019
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
1 change: 1 addition & 0 deletions changelog.d/6514.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix race which occasionally caused deleted devices to reappear.
8 changes: 5 additions & 3 deletions synapse/storage/data_stores/main/client_ips.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,16 +451,18 @@ def _update_client_ips_batch_txn(self, txn, to_update):
# Technically an access token might not be associated with
# a device so we need to check.
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,
table="devices",
keyvalues={"user_id": user_id, "device_id": device_id},
values={
updatevalues={
"user_agent": user_agent,
"last_seen": last_seen,
"ip": ip,
},
lock=False,
)
except Exception as e:
# Failed to upsert, log and continue
Expand Down
49 changes: 29 additions & 20 deletions tests/storage/test_client_ips.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,24 +37,28 @@ def test_insert_new_client_ip(self):
self.reactor.advance(12345678)

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.store.insert_client_ip(
user_id, "access_token", "ip", "user_agent", "device_id"
user_id, "access_token", "ip", "user_agent", device_id
)
)

# Trigger the storage loop
self.reactor.advance(10)

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(
{
"user_id": user_id,
"device_id": "device_id",
"device_id": device_id,
"ip": "ip",
"user_agent": "user_agent",
"last_seen": 12345678000,
Expand Down Expand Up @@ -209,37 +213,39 @@ def test_devices_last_seen_bg_update(self):
self.store.db.updates.do_next_background_update(100), by=0.1
)

# Insert a user IP
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.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
self.reactor.advance(200)

# But clear the associated entry in devices table
self.get_success(
self.store.db.simple_update(
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},
desc="test_devices_last_seen_bg_update",
)
)

# We should now get nulls when querying
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(
{
"user_id": user_id,
"device_id": "device_id",
"device_id": device_id,
"ip": None,
"user_agent": None,
"last_seen": None,
Expand Down Expand Up @@ -272,14 +278,14 @@ def test_devices_last_seen_bg_update(self):

# We should now get the correct result again
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(
{
"user_id": user_id,
"device_id": "device_id",
"device_id": device_id,
"ip": "ip",
"user_agent": "user_agent",
"last_seen": 0,
Expand All @@ -296,11 +302,14 @@ def test_old_user_ips_pruned(self):
self.store.db.updates.do_next_background_update(100), by=0.1
)

# Insert a user IP
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.store.insert_client_ip(
user_id, "access_token", "ip", "user_agent", "device_id"
user_id, "access_token", "ip", "user_agent", device_id
)
)

Expand All @@ -324,7 +333,7 @@ def test_old_user_ips_pruned(self):
"access_token": "access_token",
"ip": "ip",
"user_agent": "user_agent",
"device_id": "device_id",
"device_id": device_id,
"last_seen": 0,
}
],
Expand All @@ -347,14 +356,14 @@ def test_old_user_ips_pruned(self):

# But we should still get the correct values for the device
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(
{
"user_id": user_id,
"device_id": "device_id",
"device_id": device_id,
"ip": "ip",
"user_agent": "user_agent",
"last_seen": 0,
Expand Down