Generalize "is idle?" check in idle device notification scheduler

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);
+  record JobDescriptor(UUID accountIdentifier, byte deviceId, long lastSeen) {}
-
-  @VisibleForTesting
-  record AccountAndDeviceIdentifier(UUID accountIdentifier, byte deviceId) {}
 
   public IdleDeviceNotificationScheduler(final AccountsManager accountsManager,
       final PushNotificationManager pushNotificationManager,
@@ -52,24 +49,24 @@ public class IdleDeviceNotificationScheduler extends JobScheduler {
 
   @Override
   protected CompletableFuture<String> 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 -> {
+                account.getDevice(jobDescriptor.deviceId()).map(device -> {
-                      if (!isIdle(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) {
+  public CompletableFuture<Void> scheduleNotification(final Account account, final Device device, final LocalTime preferredDeliveryTime) {
-    final Duration idleDuration = Duration.between(Instant.ofEpochMilli(device.getLastSeen()), clock.instant());
-
-    return idleDuration.compareTo(MIN_IDLE_DURATION) >= 0;
-  }
-
-  public CompletableFuture<Void> scheduleNotification(final Account account, final byte deviceId, 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);

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<Void> 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<Boolean> 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

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<Arguments> 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));
-    }
-  }
 }

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, messagesManager, Clock.fixed(CURRENT_TIME, ZoneId.systemDefault())).block());
-    assertEquals(expectEligible, NotifyIdleDevicesWithoutMessagesCommand.isDeviceEligible(account, device, idleDeviceNotificationScheduler, messagesManager).block());
   }
 
   private static List<Arguments> 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<Arguments> 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) {