A few things to note here.

1. OIDC access tokens have a limited lifetime and refresh. This means we'll end up writing a row once per refresh period per active device to this table. I'm not super keen on this. But stale entries automatically get cleaned up from this table, so it might not be the worst thing in the world.

2. We wondered if this might affect the user retention metrics. AFAICS these are based on the user_daily_visits table, which is updated with this query:

   synapse/synapse/storage/databases/main/metrics.py

   Lines 403 to 417 in 736199b

   	sql = """
   	INSERT INTO user_daily_visits (user_id, device_id, timestamp, user_agent)
   	SELECT u.user_id, u.device_id, ?, MAX(u.user_agent)
   	FROM user_ips AS u
   	LEFT JOIN (
   	SELECT user_id, device_id, timestamp FROM user_daily_visits
   	WHERE timestamp = ?
   	) udv
   	ON u.user_id = udv.user_id AND u.device_id=udv.device_id
   	INNER JOIN users ON users.name=u.user_id
   	WHERE ? <= last_seen AND last_seen < ?
   	AND udv.timestamp IS NULL AND users.is_guest=0
   	AND users.appservice_id IS NULL
   	GROUP BY u.user_id, u.device_id
   	"""

That query groups on (user_ips.user_id, user_ips.device_id), so can handle the same device appearing twice in the user_ips table. (As indeed it may, for e.g. a mobile client moving between wifi and mobile internet connections.) So we haven't written a bug here.

3. Both of these concerns would also affect people using non-OIDC refreshing access tokens. It's just that nobody really used them(?) after they got specced.

4 . A cleaner approach would be to change this table to be keyed off (user, device, ip) instead of today's (user, access_token, ip). But I'm punting that for the future.