diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationScheduler.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationScheduler.java index 06a7f0667..59625d6d1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationScheduler.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationScheduler.java @@ -26,10 +26,7 @@ public class IdleDeviceNotificationScheduler extends JobScheduler { private final Clock clock; @VisibleForTesting - static final Duration MIN_IDLE_DURATION = Duration.ofDays(14); - - @VisibleForTesting - record AccountAndDeviceIdentifier(UUID accountIdentifier, byte deviceId) {} + record JobDescriptor(UUID accountIdentifier, byte deviceId, long lastSeen) {} public IdleDeviceNotificationScheduler(final AccountsManager accountsManager, final PushNotificationManager pushNotificationManager, @@ -52,24 +49,24 @@ public class IdleDeviceNotificationScheduler extends JobScheduler { @Override protected CompletableFuture processJob(@Nullable final byte[] jobData) { - final AccountAndDeviceIdentifier accountAndDeviceIdentifier; + final JobDescriptor jobDescriptor; try { - accountAndDeviceIdentifier = SystemMapper.jsonMapper().readValue(jobData, AccountAndDeviceIdentifier.class); + jobDescriptor = SystemMapper.jsonMapper().readValue(jobData, JobDescriptor.class); } catch (final IOException e) { return CompletableFuture.failedFuture(e); } - return accountsManager.getByAccountIdentifierAsync(accountAndDeviceIdentifier.accountIdentifier()) + return accountsManager.getByAccountIdentifierAsync(jobDescriptor.accountIdentifier()) .thenCompose(maybeAccount -> maybeAccount.map(account -> - account.getDevice(accountAndDeviceIdentifier.deviceId()).map(device -> { - if (!isIdle(device)) { + account.getDevice(jobDescriptor.deviceId()).map(device -> { + if (jobDescriptor.lastSeen() != device.getLastSeen()) { return CompletableFuture.completedFuture("deviceSeenRecently"); } try { return pushNotificationManager - .sendNewMessageNotification(account, accountAndDeviceIdentifier.deviceId(), true) + .sendNewMessageNotification(account, jobDescriptor.deviceId(), true) .thenApply(ignored -> "sent"); } catch (final NotPushRegisteredException e) { return CompletableFuture.completedFuture("deviceTokenDeleted"); @@ -79,18 +76,12 @@ public class IdleDeviceNotificationScheduler extends JobScheduler { .orElse(CompletableFuture.completedFuture("accountDeleted"))); } - public boolean isIdle(final Device device) { - final Duration idleDuration = Duration.between(Instant.ofEpochMilli(device.getLastSeen()), clock.instant()); - - return idleDuration.compareTo(MIN_IDLE_DURATION) >= 0; - } - - public CompletableFuture scheduleNotification(final Account account, final byte deviceId, final LocalTime preferredDeliveryTime) { + public CompletableFuture scheduleNotification(final Account account, final Device device, final LocalTime preferredDeliveryTime) { final Instant runAt = SchedulingUtil.getNextRecommendedNotificationTime(account, preferredDeliveryTime, clock); try { return scheduleJob(runAt, SystemMapper.jsonMapper().writeValueAsBytes( - new AccountAndDeviceIdentifier(account.getIdentifier(IdentityType.ACI), deviceId))); + new JobDescriptor(account.getIdentifier(IdentityType.ACI), device.getId(), device.getLastSeen()))); } catch (final JsonProcessingException e) { // This should never happen when serializing an `AccountAndDeviceIdentifier` throw new AssertionError(e); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommand.java index c8c893f00..f079c8aae 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommand.java @@ -18,6 +18,8 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuples; import java.time.Clock; +import java.time.Duration; +import java.time.Instant; import java.time.LocalTime; public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassCrawlAccountsCommand { @@ -33,6 +35,12 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC @VisibleForTesting static final LocalTime PREFERRED_NOTIFICATION_TIME = LocalTime.of(14, 0); + @VisibleForTesting + static final Duration MIN_IDLE_DURATION = Duration.ofDays(15); + + @VisibleForTesting + static final Duration MAX_IDLE_DURATION = Duration.ofDays(30); + private static final Counter DEVICE_INSPECTED_COUNTER = Metrics.counter(MetricsUtil.name(StartPushNotificationExperimentCommand.class, "deviceInspected")); @@ -72,11 +80,12 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC final MessagesManager messagesManager = getCommandDependencies().messagesManager(); final IdleDeviceNotificationScheduler idleDeviceNotificationScheduler = buildIdleDeviceNotificationScheduler(); + final Clock clock = getClock(); accounts .flatMap(account -> Flux.fromIterable(account.getDevices()).map(device -> Tuples.of(account, device))) .doOnNext(ignored -> DEVICE_INSPECTED_COUNTER.increment()) - .flatMap(accountAndDevice -> isDeviceEligible(accountAndDevice.getT1(), accountAndDevice.getT2(), idleDeviceNotificationScheduler, messagesManager) + .flatMap(accountAndDevice -> isDeviceEligible(accountAndDevice.getT1(), accountAndDevice.getT2(), messagesManager, clock) .mapNotNull(eligible -> eligible ? accountAndDevice : null), maxConcurrency) .flatMap(accountAndDevice -> { final Account account = accountAndDevice.getT1(); @@ -84,7 +93,7 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC final Mono scheduleNotificationMono = dryRun ? Mono.empty() - : Mono.fromFuture(() -> idleDeviceNotificationScheduler.scheduleNotification(account, device.getId(), PREFERRED_NOTIFICATION_TIME)) + : Mono.fromFuture(() -> idleDeviceNotificationScheduler.scheduleNotification(account, device, PREFERRED_NOTIFICATION_TIME)) .onErrorResume(throwable -> { log.warn("Failed to schedule notification for {}:{}", account.getIdentifier(IdentityType.ACI), @@ -103,6 +112,11 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC .block(); } + @VisibleForTesting + protected Clock getClock() { + return Clock.systemUTC(); + } + @VisibleForTesting protected IdleDeviceNotificationScheduler buildIdleDeviceNotificationScheduler() { final DynamoDbTables.TableWithExpiration tableConfiguration = getConfiguration().getDynamoDbTables().getScheduledJobs(); @@ -119,14 +133,14 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC @VisibleForTesting static Mono isDeviceEligible(final Account account, final Device device, - final IdleDeviceNotificationScheduler idleDeviceNotificationScheduler, - final MessagesManager messagesManager) { + final MessagesManager messagesManager, + final Clock clock) { if (!hasPushToken(device)) { return Mono.just(false); } - if (!idleDeviceNotificationScheduler.isIdle(device)) { + if (!isIdle(device, clock)) { return Mono.just(false); } @@ -134,6 +148,13 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC .map(mayHavePersistedMessages -> !mayHavePersistedMessages); } + @VisibleForTesting + static boolean isIdle(final Device device, final Clock clock) { + final Duration idleDuration = Duration.between(Instant.ofEpochMilli(device.getLastSeen()), clock.instant()); + + return idleDuration.compareTo(MIN_IDLE_DURATION) >= 0 && idleDuration.compareTo(MAX_IDLE_DURATION) < 0; + } + @VisibleForTesting static boolean hasPushToken(final Device device) { // Exclude VOIP tokens since they have their own, distinct delivery mechanism diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationSchedulerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationSchedulerTest.java index bbf8558c4..021db2ef5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationSchedulerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/IdleDeviceNotificationSchedulerTest.java @@ -1,8 +1,6 @@ package org.whispersystems.textsecuregcm.push; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyByte; @@ -19,7 +17,6 @@ import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -57,14 +54,14 @@ class IdleDeviceNotificationSchedulerTest { void processJob(final boolean accountPresent, final boolean devicePresent, final boolean tokenPresent, - final Instant deviceLastSeen, + final boolean lastSeenChanged, final String expectedOutcome) throws JsonProcessingException, NotPushRegisteredException { final UUID accountIdentifier = UUID.randomUUID(); final byte deviceId = Device.PRIMARY_ID; final Device device = mock(Device.class); - when(device.getLastSeen()).thenReturn(deviceLastSeen.toEpochMilli()); + when(device.getLastSeen()).thenReturn(0L); final Account account = mock(Account.class); when(account.getDevice(deviceId)).thenReturn(devicePresent ? Optional.of(device) : Optional.empty()); @@ -82,50 +79,27 @@ class IdleDeviceNotificationSchedulerTest { } final byte[] jobData = SystemMapper.jsonMapper().writeValueAsBytes( - new IdleDeviceNotificationScheduler.AccountAndDeviceIdentifier(accountIdentifier, deviceId)); + new IdleDeviceNotificationScheduler.JobDescriptor(accountIdentifier, deviceId, lastSeenChanged ? 1 : 0)); assertEquals(expectedOutcome, idleDeviceNotificationScheduler.processJob(jobData).join()); } private static List processJob() { - final Instant idleDeviceLastSeenTimestamp = CURRENT_TIME - .minus(IdleDeviceNotificationScheduler.MIN_IDLE_DURATION) - .minus(Duration.ofDays(1)); - return List.of( // Account present, device present, device has tokens, device is idle - Arguments.of(true, true, true, idleDeviceLastSeenTimestamp, "sent"), + Arguments.of(true, true, true, false, "sent"), // Account present, device present, device has tokens, but device is active - Arguments.of(true, true, true, CURRENT_TIME, "deviceSeenRecently"), + Arguments.of(true, true, true, true, "deviceSeenRecently"), // Account present, device present, device is idle, but missing tokens - Arguments.of(true, true, false, idleDeviceLastSeenTimestamp, "deviceTokenDeleted"), + Arguments.of(true, true, false, false, "deviceTokenDeleted"), // Account present, but device missing - Arguments.of(true, false, true, idleDeviceLastSeenTimestamp, "deviceDeleted"), + Arguments.of(true, false, true, false, "deviceDeleted"), // Account missing - Arguments.of(false, true, true, idleDeviceLastSeenTimestamp, "accountDeleted") + Arguments.of(false, true, true, false, "accountDeleted") ); } - - @Test - void isIdle() { - { - final Device idleDevice = mock(Device.class); - when(idleDevice.getLastSeen()) - .thenReturn(CURRENT_TIME.minus(IdleDeviceNotificationScheduler.MIN_IDLE_DURATION).minus(Duration.ofDays(1)) - .toEpochMilli()); - - assertTrue(idleDeviceNotificationScheduler.isIdle(idleDevice)); - } - - { - final Device activeDevice = mock(Device.class); - when(activeDevice.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); - - assertFalse(idleDeviceNotificationScheduler.isIdle(activeDevice)); - } - } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommandTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommandTest.java index 67b45d806..59aa5b4ad 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommandTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommandTest.java @@ -2,7 +2,6 @@ package org.whispersystems.textsecuregcm.workers; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyByte; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -10,6 +9,10 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.i18n.phonenumbers.PhoneNumberUtil; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneId; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -35,10 +38,13 @@ class NotifyIdleDevicesWithoutMessagesCommandTest { private TestNotifyIdleDevicesWithoutMessagesCommand notifyIdleDevicesWithoutMessagesCommand; + private static final Instant CURRENT_TIME = Instant.now(); + private static class TestNotifyIdleDevicesWithoutMessagesCommand extends NotifyIdleDevicesWithoutMessagesCommand { private final CommandDependencies commandDependencies; private final IdleDeviceNotificationScheduler idleDeviceNotificationScheduler; + private boolean dryRun = false; private TestNotifyIdleDevicesWithoutMessagesCommand(final MessagesManager messagesManager, @@ -73,6 +79,11 @@ class NotifyIdleDevicesWithoutMessagesCommandTest { return commandDependencies; } + @Override + protected Clock getClock() { + return Clock.fixed(CURRENT_TIME, ZoneId.systemDefault()); + } + @Override protected IdleDeviceNotificationScheduler buildIdleDeviceNotificationScheduler() { return idleDeviceNotificationScheduler; @@ -91,7 +102,7 @@ class NotifyIdleDevicesWithoutMessagesCommandTest { messagesManager = mock(MessagesManager.class); idleDeviceNotificationScheduler = mock(IdleDeviceNotificationScheduler.class); - when(idleDeviceNotificationScheduler.scheduleNotification(any(), anyByte(), any())) + when(idleDeviceNotificationScheduler.scheduleNotification(any(), any(), any())) .thenReturn(CompletableFuture.completedFuture(null)); notifyIdleDevicesWithoutMessagesCommand = @@ -104,50 +115,47 @@ class NotifyIdleDevicesWithoutMessagesCommandTest { notifyIdleDevicesWithoutMessagesCommand.setDryRun(dryRun); final UUID accountIdentifier = UUID.randomUUID(); - final byte eligibleDeviceId = Device.PRIMARY_ID; - final byte ineligibleDeviceId = eligibleDeviceId + 1; final Device eligibleDevice = mock(Device.class); - when(eligibleDevice.getId()).thenReturn(eligibleDeviceId); + when(eligibleDevice.getId()).thenReturn(Device.PRIMARY_ID); when(eligibleDevice.getApnId()).thenReturn("apns-token"); + when(eligibleDevice.getLastSeen()) + .thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli()); final Device ineligibleDevice = mock(Device.class); - when(ineligibleDevice.getId()).thenReturn(ineligibleDeviceId); + when(ineligibleDevice.getId()).thenReturn((byte) (Device.PRIMARY_ID + 1)); final Account account = mock(Account.class); when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); when(account.getDevices()).thenReturn(List.of(eligibleDevice, ineligibleDevice)); - when(idleDeviceNotificationScheduler.isIdle(eligibleDevice)).thenReturn(true); when(messagesManager.mayHavePersistedMessages(accountIdentifier, eligibleDevice)) .thenReturn(CompletableFuture.completedFuture(false)); notifyIdleDevicesWithoutMessagesCommand.crawlAccounts(Flux.just(account)); if (dryRun) { - verify(idleDeviceNotificationScheduler, never()).scheduleNotification(account, eligibleDeviceId, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME); + verify(idleDeviceNotificationScheduler, never()).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME); } else { - verify(idleDeviceNotificationScheduler).scheduleNotification(account, eligibleDeviceId, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME); + verify(idleDeviceNotificationScheduler).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME); } - verify(idleDeviceNotificationScheduler, never()).scheduleNotification(eq(account), eq(ineligibleDeviceId), any()); + verify(idleDeviceNotificationScheduler, never()).scheduleNotification(eq(account), eq(ineligibleDevice), any()); } @ParameterizedTest @MethodSource void isDeviceEligible(final Account account, final Device device, - final boolean isDeviceIdle, final boolean mayHaveMessages, final boolean expectEligible) { when(messagesManager.mayHavePersistedMessages(account.getIdentifier(IdentityType.ACI), device)) .thenReturn(CompletableFuture.completedFuture(mayHaveMessages)); - when(idleDeviceNotificationScheduler.isIdle(device)).thenReturn(isDeviceIdle); - - assertEquals(expectEligible, NotifyIdleDevicesWithoutMessagesCommand.isDeviceEligible(account, device, idleDeviceNotificationScheduler, messagesManager).block()); + assertEquals(expectEligible, + NotifyIdleDevicesWithoutMessagesCommand.isDeviceEligible(account, device, messagesManager, Clock.fixed(CURRENT_TIME, ZoneId.systemDefault())).block()); } private static List isDeviceEligible() { @@ -162,57 +170,96 @@ class NotifyIdleDevicesWithoutMessagesCommandTest { // Idle device with push token and messages final Device device = mock(Device.class); when(device.getApnId()).thenReturn("apns-token"); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli()); - arguments.add(Arguments.of(account, device, true, true, false)); + arguments.add(Arguments.of(account, device, true, false)); } { // Idle device missing push token, but with messages - arguments.add(Arguments.of(account, mock(Device.class), true, true, false)); + final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli()); + + arguments.add(Arguments.of(account, device, true, false)); } { // Idle device missing push token and messages - arguments.add(Arguments.of(account, mock(Device.class), true, false, false)); + final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli()); + + arguments.add(Arguments.of(account, device, false, false)); } { // Idle device with push token, but no messages final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli()); when(device.getApnId()).thenReturn("apns-token"); - arguments.add(Arguments.of(account, device, true, false, true)); + arguments.add(Arguments.of(account, device, false, true)); } { // Active device with push token and messages final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); when(device.getApnId()).thenReturn("apns-token"); - arguments.add(Arguments.of(account, device, false, true, false)); + arguments.add(Arguments.of(account, device, true, false)); } { // Active device missing push token, but with messages - arguments.add(Arguments.of(account, mock(Device.class), false, true, false)); + final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(account, device, true, false)); } { // Active device missing push token and messages - arguments.add(Arguments.of(account, mock(Device.class), false, false, false)); + final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(account, device, false, false)); } { // Active device with push token, but no messages final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); when(device.getApnId()).thenReturn("apns-token"); - arguments.add(Arguments.of(account, device, false, false, false)); + arguments.add(Arguments.of(account, device, false, false)); } return arguments; } + @ParameterizedTest + @MethodSource + void isIdle(final Duration idleDuration, final boolean expectIdle) { + final Instant currentTime = Instant.now(); + final Clock clock = Clock.fixed(currentTime, ZoneId.systemDefault()); + + final Device device = mock(Device.class); + when(device.getLastSeen()).thenReturn(currentTime.minus(idleDuration).toEpochMilli()); + + assertEquals(expectIdle, NotifyIdleDevicesWithoutMessagesCommand.isIdle(device, clock)); + } + + private static List isIdle() { + return List.of( + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION, true), + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION.plusMillis(1), true), + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION.minusMillis(1), false), + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MAX_IDLE_DURATION, false), + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MAX_IDLE_DURATION.plusMillis(1), false), + Arguments.of(NotifyIdleDevicesWithoutMessagesCommand.MAX_IDLE_DURATION.minusMillis(1), true) + ); + } + @ParameterizedTest @MethodSource void hasPushToken(final Device device, final boolean expectHasPushToken) {