From 295cedc0750cb69650ce66f1a8fbc94413161c83 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Fri, 13 Jun 2025 14:33:53 -0500 Subject: [PATCH] remove experiment configuration for low urgency pushes --- .../textsecuregcm/WhisperServerService.java | 2 +- .../push/PushNotificationManager.java | 20 +------ .../storage/MessagePersister.java | 5 +- .../websocket/WebSocketConnection.java | 7 +-- .../workers/CommandDependencies.java | 6 +-- .../push/PushNotificationManagerTest.java | 52 +++++++++++-------- 6 files changed, 36 insertions(+), 56 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index da403d9d9..9adda66e8 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -641,7 +641,7 @@ public class WhisperServerService extends Application> sendNewMessageNotification(final Account destination, final byte destinationDeviceId, final boolean urgent) throws NotPushRegisteredException { @@ -107,7 +101,7 @@ public class PushNotificationManager { @VisibleForTesting CompletableFuture> sendNotification(final PushNotification pushNotification) { - if (shouldScheduleNotification(pushNotification)) { + if (!pushNotification.urgent()) { // Schedule a notification for some time in the future (possibly even now!) rather than sending a notification // directly return pushNotificationScheduler @@ -156,16 +150,6 @@ public class PushNotificationManager { .thenApply(Optional::of); } - private boolean shouldScheduleNotification(final PushNotification pushNotification) { - return !pushNotification.urgent() && switch (pushNotification.tokenType()) { - // APNs imposes a per-device limit on background push notifications - case APN -> true; - case FCM -> experimentEnrollmentManager.isEnrolled( - pushNotification.destination().getUuid(), - SCHEDULE_LOW_URGENCY_FCM_PUSH_EXPERIMENT); - }; - } - private static BiConsumer logErrors() { return (ignored, throwable) -> { if (throwable != null) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java index 8bc94fdca..6baca7635 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java @@ -244,11 +244,8 @@ public class MessagePersister implements Managed { } while (!messages.isEmpty()); - final boolean inSkipExperiment = device.getGcmId() != null && experimentEnrollmentManager.isEnrolled( - accountUuid, - PushNotificationManager.SCHEDULE_LOW_URGENCY_FCM_PUSH_EXPERIMENT); DistributionSummary.builder(QUEUE_SIZE_DISTRIBUTION_SUMMARY_NAME) - .tags(Tags.of(platformTag).and("lowUrgencySkip", Boolean.toString(inSkipExperiment))) + .tags(Tags.of(platformTag)) .publishPercentileHistogram(true) .register(Metrics.globalRegistry) .record(messageCount); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnection.java b/service/src/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnection.java index 252a482c0..a3d9083f6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnection.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnection.java @@ -338,13 +338,8 @@ public class WebSocketConnection implements WebSocketConnectionEventListener { // Cleared the queue! Send a queue empty message if we need to consecutiveRetries.set(0); if (sentInitialQueueEmptyMessage.compareAndSet(false, true)) { - final boolean inSkipExperiment = auth.getAuthenticatedDevice().getGcmId() != null && experimentEnrollmentManager.isEnrolled( - auth.getAccount().getUuid(), - PushNotificationManager.SCHEDULE_LOW_URGENCY_FCM_PUSH_EXPERIMENT); - final Tags tags = Tags - .of(UserAgentTagUtil.getPlatformTag(client.getUserAgent())) - .and("lowUrgencySkip", Boolean.toString(inSkipExperiment)); + final Tags tags = Tags.of(UserAgentTagUtil.getPlatformTag(client.getUserAgent())); final long drainDuration = System.currentTimeMillis() - queueDrainStartTime.get(); Metrics.summary(INITIAL_QUEUE_LENGTH_DISTRIBUTION_NAME, tags).record(sentMessageCounter.sum()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java index 76ab8eee6..9893910f3 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java @@ -33,7 +33,6 @@ import org.whispersystems.textsecuregcm.backup.Cdn3RemoteStorageManager; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.controllers.SecureStorageController; import org.whispersystems.textsecuregcm.controllers.SecureValueRecovery2Controller; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.experiment.PushNotificationExperimentSamples; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.metrics.MicrometerAwsSdkMetricPublisher; @@ -118,9 +117,6 @@ record CommandDependencies( new DynamicConfigurationManager<>( configuration.getDynamicConfig().build(awsCredentialsProvider, dynamicConfigurationExecutor), DynamicConfiguration.class); dynamicConfigurationManager.start(); - final ExperimentEnrollmentManager experimentEnrollmentManager = - new ExperimentEnrollmentManager(dynamicConfigurationManager); - final ClientResources.Builder redisClientResourcesBuilder = ClientResources.builder(); FaultTolerantRedisClusterClient cacheCluster = configuration.getCacheClusterConfiguration() @@ -286,7 +282,7 @@ record CommandDependencies( PushNotificationScheduler pushNotificationScheduler = new PushNotificationScheduler(pushSchedulerCluster, apnSender, fcmSender, accountsManager, 0, 0); PushNotificationManager pushNotificationManager = new PushNotificationManager(accountsManager, - apnSender, fcmSender, pushNotificationScheduler, experimentEnrollmentManager); + apnSender, fcmSender, pushNotificationScheduler); PushNotificationExperimentSamples pushNotificationExperimentSamples = new PushNotificationExperimentSamples(dynamoDbAsyncClient, configuration.getDynamoDbTables().getPushNotificationExperimentSamples().getTableName(), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java index 1aed9f3bc..d37ce0291 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java @@ -24,7 +24,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.junitpioneer.jupiter.cartesian.CartesianTest; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; @@ -36,7 +35,6 @@ class PushNotificationManagerTest { private APNSender apnSender; private FcmSender fcmSender; private PushNotificationScheduler pushNotificationScheduler; - private ExperimentEnrollmentManager experimentEnrollmentManager; private PushNotificationManager pushNotificationManager; @@ -46,17 +44,15 @@ class PushNotificationManagerTest { apnSender = mock(APNSender.class); fcmSender = mock(FcmSender.class); pushNotificationScheduler = mock(PushNotificationScheduler.class); - experimentEnrollmentManager = mock(ExperimentEnrollmentManager.class); AccountsHelper.setupMockUpdate(accountsManager); pushNotificationManager = new PushNotificationManager(accountsManager, apnSender, fcmSender, - pushNotificationScheduler, experimentEnrollmentManager); + pushNotificationScheduler); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void sendNewMessageNotification(final boolean urgent) throws NotPushRegisteredException { + @Test + void sendNewUrgentMessageNotification() throws NotPushRegisteredException { final Account account = mock(Account.class); final Device device = mock(Device.class); @@ -66,13 +62,30 @@ class PushNotificationManagerTest { when(device.getGcmId()).thenReturn(deviceToken); when(account.getDevice(Device.PRIMARY_ID)).thenReturn(Optional.of(device)); - when(fcmSender.sendNotification(any())) - .thenReturn(CompletableFuture.completedFuture(new SendPushNotificationResult(true, Optional.empty(), false, Optional.empty()))); - - pushNotificationManager.sendNewMessageNotification(account, Device.PRIMARY_ID, urgent); - verify(fcmSender).sendNotification(new PushNotification(deviceToken, PushNotification.TokenType.FCM, PushNotification.NotificationType.NOTIFICATION, null, account, device, urgent)); + when(fcmSender.sendNotification(any())) + .thenReturn(CompletableFuture.completedFuture(new SendPushNotificationResult(true, Optional.empty(), false, Optional.empty()))); + pushNotificationManager.sendNewMessageNotification(account, Device.PRIMARY_ID, true); + verify(fcmSender).sendNotification(new PushNotification(deviceToken, PushNotification.TokenType.FCM, PushNotification.NotificationType.NOTIFICATION, null, account, device, true)); } + @Test + void sendNewNonUrgentMessageNotification() throws NotPushRegisteredException { + final Account account = mock(Account.class); + final Device device = mock(Device.class); + + final String deviceToken = "token"; + + when(device.getId()).thenReturn(Device.PRIMARY_ID); + when(device.getGcmId()).thenReturn(deviceToken); + when(account.getDevice(Device.PRIMARY_ID)).thenReturn(Optional.of(device)); + + when(pushNotificationScheduler.scheduleBackgroundNotification(any(), any(), any())) + .thenReturn(CompletableFuture.completedFuture(null)); + pushNotificationManager.sendNewMessageNotification(account, Device.PRIMARY_ID, false); + verify(pushNotificationScheduler).scheduleBackgroundNotification(PushNotification.TokenType.FCM, account, device); + } + + @Test void sendRegistrationChallengeNotification() { final String deviceToken = "token"; @@ -135,9 +148,8 @@ class PushNotificationManagerTest { } } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void testSendNotificationFcm(final boolean urgent) { + @Test + void testSendNotificationFcm() { final Account account = mock(Account.class); final Device device = mock(Device.class); @@ -145,7 +157,7 @@ class PushNotificationManagerTest { when(account.getDevice(Device.PRIMARY_ID)).thenReturn(Optional.of(device)); final PushNotification pushNotification = new PushNotification( - "token", PushNotification.TokenType.FCM, PushNotification.NotificationType.NOTIFICATION, null, account, device, urgent); + "token", PushNotification.TokenType.FCM, PushNotification.NotificationType.NOTIFICATION, null, account, device, true); when(fcmSender.sendNotification(pushNotification)) .thenReturn(CompletableFuture.completedFuture(new SendPushNotificationResult(true, Optional.empty(), false, Optional.empty()))); @@ -162,10 +174,9 @@ class PushNotificationManagerTest { @CartesianTest void testSendOrScheduleNotification( @CartesianTest.Enum(PushNotification.TokenType.class) PushNotification.TokenType tokenType, - @CartesianTest.Values(booleans = {false, true}) final boolean urgent, - @CartesianTest.Values(booleans = {false, true}) final boolean inExperiment) { + @CartesianTest.Values(booleans = {false, true}) final boolean urgent) { - final boolean expectSchedule = !urgent && (tokenType == PushNotification.TokenType.APN || inExperiment); + final boolean expectSchedule = !urgent; final Account account = mock(Account.class); final Device device = mock(Device.class); @@ -175,9 +186,6 @@ class PushNotificationManagerTest { when(account.getDevice(Device.PRIMARY_ID)).thenReturn(Optional.of(device)); when(account.getUuid()).thenReturn(aci); - when(experimentEnrollmentManager.isEnrolled(aci, PushNotificationManager.SCHEDULE_LOW_URGENCY_FCM_PUSH_EXPERIMENT)) - .thenReturn(inExperiment); - final PushNotification pushNotification = new PushNotification( "token", tokenType, PushNotification.NotificationType.NOTIFICATION, null, account, device, urgent);