diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 2d5926db3..92c306491 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -18,6 +18,7 @@ import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Tags; import java.io.IOException; import java.time.Clock; +import java.time.Duration; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -79,6 +80,12 @@ public class AccountsManager { private final ObjectMapper mapper; private final Clock clock; + // An account that's used at least daily will get reset in the cache at least once per day when its "last seen" + // timestamp updates; expiring entries after two days will help clear out "zombie" cache entries that are read + // frequently (e.g. the account is in an active group and receives messages frequently), but aren't actively used by + // the owner. + private static final long CACHE_TTL_SECONDS = Duration.ofDays(2).toSeconds(); + public enum DeletionReason { ADMIN_DELETED("admin"), EXPIRED ("expired"), @@ -476,10 +483,9 @@ public class AccountsManager { cacheCluster.useCluster(connection -> { final RedisAdvancedClusterCommands commands = connection.sync(); - - commands.set(getAccountMapKey(account.getPhoneNumberIdentifier().toString()), account.getUuid().toString()); - commands.set(getAccountMapKey(account.getNumber()), account.getUuid().toString()); - commands.set(getAccountEntityKey(account.getUuid()), accountJson); + commands.setex(getAccountMapKey(account.getPhoneNumberIdentifier().toString()), CACHE_TTL_SECONDS, account.getUuid().toString()); + commands.setex(getAccountMapKey(account.getNumber()), CACHE_TTL_SECONDS, account.getUuid().toString()); + commands.setex(getAccountEntityKey(account.getUuid()), CACHE_TTL_SECONDS, accountJson); }); } catch (JsonProcessingException e) { throw new IllegalStateException(e); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index 366bbe852..db6ca8897 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -10,6 +10,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; @@ -229,7 +230,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private Account getLastAccountFromRedisMock(RedisAdvancedClusterCommands commands) throws IOException { ArgumentCaptor redisSetArgumentCapture = ArgumentCaptor.forClass(String.class); - verify(commands, atLeast(20)).set(anyString(), redisSetArgumentCapture.capture()); + verify(commands, atLeast(20)).setex(anyString(), anyLong(), redisSetArgumentCapture.capture()); return JsonHelpers.fromJson(redisSetArgumentCapture.getValue(), Account.class); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java index 5df767be7..99d932d2a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java @@ -10,6 +10,7 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.anyString; @@ -221,9 +222,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); - verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); + verify(commands, times(1)).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts, times(1)).getByE164(eq("+14152222222")); @@ -245,9 +246,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("Account3::" + uuid)); - verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); + verify(commands, times(1)).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts, times(1)).getByAccountIdentifier(eq(uuid)); @@ -270,9 +271,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands).get(eq("AccountMap::" + pni)); - verify(commands).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands).set(eq("Account3::" + uuid), anyString()); + verify(commands).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts).getByPhoneNumberIdentifier(pni); @@ -294,9 +295,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); - verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); + verify(commands, times(1)).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts, times(1)).getByE164(eq("+14152222222")); @@ -318,9 +319,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("Account3::" + uuid)); - verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); + verify(commands, times(1)).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands, times(1)).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts, times(1)).getByAccountIdentifier(eq(uuid)); @@ -343,9 +344,9 @@ class AccountsManagerTest { assertSame(retrieved.get(), account); verify(commands).get(eq("AccountMap::" + pni)); - verify(commands).set(eq("AccountMap::" + pni), eq(uuid.toString())); - verify(commands).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(commands).set(eq("Account3::" + uuid), anyString()); + verify(commands).setex(eq("AccountMap::" + pni), anyLong(), eq(uuid.toString())); + verify(commands).setex(eq("AccountMap::+14152222222"), anyLong(), eq(uuid.toString())); + verify(commands).setex(eq("Account3::" + uuid), anyLong(), anyString()); verifyNoMoreInteractions(commands); verify(accounts).getByPhoneNumberIdentifier(pni);