MirrorMirror
/
Signal-Server
mirror of https://github.com/signalapp/Signal-Server.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Expand the scope of the "notify idle devices" job to cover short-idle devices WITH messages

This commit is contained in:
Jon Chambers 2024-08-09 16:33:09 -04:00 committed by Jon Chambers
parent 0e267509da
commit df3caeb04a
3 changed files with 150 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
View File

@ -263,7 +263,7 @@ import org.whispersystems.textsecuregcm.workers.CheckDynamicConfigurationCommand
import org.whispersystems.textsecuregcm.workers.DeleteUserCommand;
import org.whispersystems.textsecuregcm.workers.IdleDeviceNotificationSchedulerFactory;
import org.whispersystems.textsecuregcm.workers.MessagePersisterServiceCommand;
import org.whispersystems.textsecuregcm.workers.NotifyIdleDevicesWithoutMessagesCommand;
import org.whispersystems.textsecuregcm.workers.NotifyIdleDevicesCommand;
import org.whispersystems.textsecuregcm.workers.ProcessScheduledJobsServiceCommand;
import org.whispersystems.textsecuregcm.workers.RemoveExpiredAccountsCommand;
import org.whispersystems.textsecuregcm.workers.RemoveExpiredBackupsCommand;
@ -326,7 +326,7 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
bootstrap.addCommand(new RemoveExpiredBackupsCommand(Clock.systemUTC()));
bootstrap.addCommand(new BackupMetricsCommand(Clock.systemUTC()));
bootstrap.addCommand(new RemoveExpiredLinkedDevicesCommand());
bootstrap.addCommand(new NotifyIdleDevicesWithoutMessagesCommand());
bootstrap.addCommand(new NotifyIdleDevicesCommand());
bootstrap.addCommand(new ProcessScheduledJobsServiceCommand("process-idle-device-notification-jobs",
"Processes scheduled jobs to send notifications to idle devices",
new IdleDeviceNotificationSchedulerFactory()));

53
service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommand.java → service/src/main/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesCommand.java
View File

@ -22,7 +22,7 @@ import java.time.Duration;
import java.time.Instant;
import java.time.LocalTime;
public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassCrawlAccountsCommand {
public class NotifyIdleDevicesCommand extends AbstractSinglePassCrawlAccountsCommand {
private static final int DEFAULT_MAX_CONCURRENCY = 16;
@ -36,23 +36,29 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC
static final LocalTime PREFERRED_NOTIFICATION_TIME = LocalTime.of(14, 0);
@VisibleForTesting
static final Duration MIN_IDLE_DURATION = Duration.ofDays(30);
static final Duration MIN_SHORT_IDLE_DURATION = Duration.ofDays(3);
@VisibleForTesting
static final Duration MAX_IDLE_DURATION = Duration.ofDays(45);
static final Duration MAX_SHORT_IDLE_DURATION = Duration.ofDays(30);
@VisibleForTesting
static final Duration MIN_LONG_IDLE_DURATION = Duration.ofDays(30);
@VisibleForTesting
static final Duration MAX_LONG_IDLE_DURATION = Duration.ofDays(45);
private static final Counter DEVICE_INSPECTED_COUNTER =
Metrics.counter(MetricsUtil.name(NotifyIdleDevicesWithoutMessagesCommand.class, "deviceInspected"));
Metrics.counter(MetricsUtil.name(NotifyIdleDevicesCommand.class, "deviceInspected"));
private static final String SCHEDULED_NOTIFICATION_COUNTER_NAME =
MetricsUtil.name(NotifyIdleDevicesWithoutMessagesCommand.class, "scheduleNotification");
MetricsUtil.name(NotifyIdleDevicesCommand.class, "scheduleNotification");
private static final String DRY_RUN_TAG_NAME = "dryRun";
private static final Logger log = LoggerFactory.getLogger(NotifyIdleDevicesWithoutMessagesCommand.class);
private static final Logger log = LoggerFactory.getLogger(NotifyIdleDevicesCommand.class);
public NotifyIdleDevicesWithoutMessagesCommand() {
super("notify-idle-devices-without-messages", "Schedules push notifications for devices that have been idle for a long time, but have no pending messages");
public NotifyIdleDevicesCommand() {
super("notify-idle-devices", "Schedules push notifications for idle devices");
}
@Override
@ -136,23 +142,42 @@ public class NotifyIdleDevicesWithoutMessagesCommand extends AbstractSinglePassC
final MessagesManager messagesManager,
final Clock clock) {
// There are two populations of interest for this crawler:
//
// 1. Devices that have only been idle for a little while, but have messages that they don't seem to be retrieving
// 2. Devices that have been idle for a long time, but don't have any messages
//
// We think the first group sometimes just needs a little nudge to wake up and get their messages, and the latter
// group generally WOULD get their messages if they had any. We want to notify the first group to prompt them to
// actually get their messages and the latter group to prevent them from getting deleted due to inactivity (since
// they are otherwise healthy installations that just aren't getting much traffic).
if (!hasPushToken(device)) {
return Mono.just(false);
}
if (!isIdle(device, clock)) {
if (isShortIdle(device, clock)) {
return Mono.fromFuture(messagesManager.mayHaveUrgentPersistedMessages(account.getIdentifier(IdentityType.ACI), device));
} else if (isLongIdle(device, clock)) {
return Mono.fromFuture(messagesManager.mayHavePersistedMessages(account.getIdentifier(IdentityType.ACI), device))
.map(mayHavePersistedMessages -> !mayHavePersistedMessages);
} else {
return Mono.just(false);
}
return Mono.fromFuture(messagesManager.mayHavePersistedMessages(account.getIdentifier(IdentityType.ACI), device))
.map(mayHavePersistedMessages -> !mayHavePersistedMessages);
}
@VisibleForTesting
static boolean isIdle(final Device device, final Clock clock) {
static boolean isShortIdle(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;
return idleDuration.compareTo(MIN_SHORT_IDLE_DURATION) >= 0 && idleDuration.compareTo(MAX_SHORT_IDLE_DURATION) < 0;
}
@VisibleForTesting
static boolean isLongIdle(final Device device, final Clock clock) {
final Duration idleDuration = Duration.between(Instant.ofEpochMilli(device.getLastSeen()), clock.instant());
return idleDuration.compareTo(MIN_LONG_IDLE_DURATION) >= 0 && idleDuration.compareTo(MAX_LONG_IDLE_DURATION) < 0;
}
@VisibleForTesting

148
service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesWithoutMessagesCommandTest.java → service/src/test/java/org/whispersystems/textsecuregcm/workers/NotifyIdleDevicesCommandTest.java
View File

@ -31,23 +31,23 @@ import org.whispersystems.textsecuregcm.storage.Device;
import org.whispersystems.textsecuregcm.storage.MessagesManager;
import reactor.core.publisher.Flux;
class NotifyIdleDevicesWithoutMessagesCommandTest {
class NotifyIdleDevicesCommandTest {
private MessagesManager messagesManager;
private IdleDeviceNotificationScheduler idleDeviceNotificationScheduler;
private TestNotifyIdleDevicesWithoutMessagesCommand notifyIdleDevicesWithoutMessagesCommand;
private TestNotifyIdleDevicesCommand notifyIdleDevicesWithoutMessagesCommand;
private static final Instant CURRENT_TIME = Instant.now();
private static class TestNotifyIdleDevicesWithoutMessagesCommand extends NotifyIdleDevicesWithoutMessagesCommand {
private static class TestNotifyIdleDevicesCommand extends NotifyIdleDevicesCommand {
private final CommandDependencies commandDependencies;
private final IdleDeviceNotificationScheduler idleDeviceNotificationScheduler;
private boolean dryRun = false;
private TestNotifyIdleDevicesWithoutMessagesCommand(final MessagesManager messagesManager,
private TestNotifyIdleDevicesCommand(final MessagesManager messagesManager,
final IdleDeviceNotificationScheduler idleDeviceNotificationScheduler) {
this.commandDependencies = new CommandDependencies(
@ -94,8 +94,8 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
@Override
protected Namespace getNamespace() {
return new Namespace(Map.of(
NotifyIdleDevicesWithoutMessagesCommand.MAX_CONCURRENCY_ARGUMENT, 1,
NotifyIdleDevicesWithoutMessagesCommand.DRY_RUN_ARGUMENT, dryRun));
NotifyIdleDevicesCommand.MAX_CONCURRENCY_ARGUMENT, 1,
NotifyIdleDevicesCommand.DRY_RUN_ARGUMENT, dryRun));
}
}
@ -108,7 +108,7 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
.thenReturn(CompletableFuture.completedFuture(null));
notifyIdleDevicesWithoutMessagesCommand =
new TestNotifyIdleDevicesWithoutMessagesCommand(messagesManager, idleDeviceNotificationScheduler);
new TestNotifyIdleDevicesCommand(messagesManager, idleDeviceNotificationScheduler);
}
@ParameterizedTest
@ -122,7 +122,7 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
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());
.thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION).toEpochMilli());
final Device ineligibleDevice = mock(Device.class);
when(ineligibleDevice.getId()).thenReturn((byte) (Device.PRIMARY_ID + 1));
@ -138,9 +138,9 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
notifyIdleDevicesWithoutMessagesCommand.crawlAccounts(Flux.just(account));
if (dryRun) {
verify(idleDeviceNotificationScheduler, never()).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME);
verify(idleDeviceNotificationScheduler, never()).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesCommand.PREFERRED_NOTIFICATION_TIME);
} else {
verify(idleDeviceNotificationScheduler).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesWithoutMessagesCommand.PREFERRED_NOTIFICATION_TIME);
verify(idleDeviceNotificationScheduler).scheduleNotification(account, eligibleDevice, NotifyIdleDevicesCommand.PREFERRED_NOTIFICATION_TIME);
}
verify(idleDeviceNotificationScheduler, never()).scheduleNotification(eq(account), eq(ineligibleDevice), any());
@ -151,13 +151,17 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
void isDeviceEligible(final Account account,
final Device device,
final boolean mayHaveMessages,
final boolean mayHaveUrgentMessages,
final boolean expectEligible) {
when(messagesManager.mayHavePersistedMessages(account.getIdentifier(IdentityType.ACI), device))
.thenReturn(CompletableFuture.completedFuture(mayHaveMessages));
when(messagesManager.mayHaveUrgentPersistedMessages(account.getIdentifier(IdentityType.ACI), device))
.thenReturn(CompletableFuture.completedFuture(mayHaveUrgentMessages));
assertEquals(expectEligible,
NotifyIdleDevicesWithoutMessagesCommand.isDeviceEligible(account, device, messagesManager, Clock.fixed(CURRENT_TIME, ZoneId.systemDefault())).block());
NotifyIdleDevicesCommand.isDeviceEligible(account, device, messagesManager, Clock.fixed(CURRENT_TIME, ZoneId.systemDefault())).block());
}
private static List<Arguments> isDeviceEligible() {
@ -169,54 +173,97 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164));
{
// Idle device with push token and messages
// Long-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());
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, true, false));
arguments.add(Arguments.of(account, device, true, true, false));
}
{
// Idle device missing push token, but with messages
// Long-idle device missing push token, but with messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli());
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, true, false));
arguments.add(Arguments.of(account, device, true, true, false));
}
{
// Idle device missing push token and messages
// Long-idle device missing push token and messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesWithoutMessagesCommand.MIN_IDLE_DURATION).toEpochMilli());
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, false, false));
arguments.add(Arguments.of(account, device, false, false, false));
}
{
// Idle device with push token, but no messages
// Long-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.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION).toEpochMilli());
when(device.getApnId()).thenReturn("apns-token");
arguments.add(Arguments.of(account, device, false, true));
arguments.add(Arguments.of(account, device, false, false, true));
}
{
// Active device with push token and messages
// Short-idle device with push token and urgent messages
final Device device = mock(Device.class);
when(device.getApnId()).thenReturn("apns-token");
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, true, true, true));
}
{
// Short-idle device with push token and only non-urgent messages
final Device device = mock(Device.class);
when(device.getApnId()).thenReturn("apns-token");
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, true, false, false));
}
{
// Short-idle device missing push token, but with urgent messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, true, true, false));
}
{
// Short-idle device missing push token and messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION).toEpochMilli());
arguments.add(Arguments.of(account, device, false, false, false));
}
{
// Short-idle device with push token, but no messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.minus(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION).toEpochMilli());
when(device.getApnId()).thenReturn("apns-token");
arguments.add(Arguments.of(account, device, false, false, false));
}
{
// Active device with push token and urgent 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, true, false));
arguments.add(Arguments.of(account, device, true, true, false));
}
{
// Active device missing push token, but with messages
// Active device missing push token, but with urgent messages
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli());
arguments.add(Arguments.of(account, device, true, false));
arguments.add(Arguments.of(account, device, true, true, false));
}
{
@ -224,7 +271,7 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
final Device device = mock(Device.class);
when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli());
arguments.add(Arguments.of(account, device, false, false));
arguments.add(Arguments.of(account, device, false, false, false));
}
{
@ -233,7 +280,7 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli());
when(device.getApnId()).thenReturn("apns-token");
arguments.add(Arguments.of(account, device, false, false));
arguments.add(Arguments.of(account, device, false, false, false));
}
return arguments;
@ -241,31 +288,54 @@ class NotifyIdleDevicesWithoutMessagesCommandTest {
@ParameterizedTest
@MethodSource
void isIdle(final Duration idleDuration, final boolean expectIdle) {
void isShortIdle(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));
assertEquals(expectIdle, NotifyIdleDevicesCommand.isShortIdle(device, clock));
}
private static List<Arguments> isIdle() {
private static List<Arguments> isShortIdle() {
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)
Arguments.of(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION, true),
Arguments.of(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION.plusMillis(1), true),
Arguments.of(NotifyIdleDevicesCommand.MIN_SHORT_IDLE_DURATION.minusMillis(1), false),
Arguments.of(NotifyIdleDevicesCommand.MAX_SHORT_IDLE_DURATION, false),
Arguments.of(NotifyIdleDevicesCommand.MAX_SHORT_IDLE_DURATION.plusMillis(1), false),
Arguments.of(NotifyIdleDevicesCommand.MAX_SHORT_IDLE_DURATION.minusMillis(1), true)
);
}
@ParameterizedTest
@MethodSource
void isLongIdle(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, NotifyIdleDevicesCommand.isLongIdle(device, clock));
}
private static List<Arguments> isLongIdle() {
return List.of(
Arguments.of(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION, true),
Arguments.of(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION.plusMillis(1), true),
Arguments.of(NotifyIdleDevicesCommand.MIN_LONG_IDLE_DURATION.minusMillis(1), false),
Arguments.of(NotifyIdleDevicesCommand.MAX_LONG_IDLE_DURATION, false),
Arguments.of(NotifyIdleDevicesCommand.MAX_LONG_IDLE_DURATION.plusMillis(1), false),
Arguments.of(NotifyIdleDevicesCommand.MAX_LONG_IDLE_DURATION.minusMillis(1), true)
);
}
@ParameterizedTest
@MethodSource
void hasPushToken(final Device device, final boolean expectHasPushToken) {
assertEquals(expectHasPushToken, NotifyIdleDevicesWithoutMessagesCommand.hasPushToken(device));
assertEquals(expectHasPushToken, NotifyIdleDevicesCommand.hasPushToken(device));
}
private static List<Arguments> hasPushToken() {