ref(transport): Measure rate-limit backoff on a monotonic clock (JAVA-574)

CHANGELOG.md

@@ -6,9 +6,14 @@
 
 - Add `Session.State.Unhandled` for unhandled errors that do not terminate the process ([#5919](https://github.com/getsentry/sentry-java/pull/5919))
 
+### Behavioral Changes
+
+- Measure HTTP rate-limit backoff on a monotonic clock instead of the wall clock, so that a device time change no longer lifts or extends an active rate limit ([#6030](https://github.com/getsentry/sentry-java/pull/6030))
+
 ### Internal
 
 - Add internal `UptimeClock` and `ElapsedRealtimeClock` abstractions with `Deadline` and `Stopwatch` primitives ([#6028](https://github.com/getsentry/sentry-java/pull/6028))
+- Deprecate `RateLimiter(ICurrentDateProvider, SentryOptions)` in favour of `RateLimiter(ElapsedRealtimeClock, RateLimiterConfig)` ([#6030](https://github.com/getsentry/sentry-java/pull/6030))
 
 ### Fixes
 

sentry/api/sentry.api

@@ -3648,7 +3648,7 @@ public final class io/sentry/SentryOpenTelemetryMode : java/lang/Enum {
 	public static fun values ()[Lio/sentry/SentryOpenTelemetryMode;
 }
 
-public class io/sentry/SentryOptions {
+public class io/sentry/SentryOptions : io/sentry/transport/RateLimiterConfig {
 	public static final field DEFAULT_PROPAGATION_TARGETS Ljava/lang/String;
 	public static final field MAX_EVENT_SIZE_BYTES J
 	protected final field lock Lio/sentry/util/AutoClosableReentrantLock;
@@ -7675,6 +7675,7 @@ public final class io/sentry/transport/NoOpTransportGate : io/sentry/transport/I
 
 public final class io/sentry/transport/RateLimiter : java/io/Closeable {
 	public fun <init> (Lio/sentry/SentryOptions;)V
+	public fun <init> (Lio/sentry/time/ElapsedRealtimeClock;Lio/sentry/transport/RateLimiterConfig;)V
 	public fun <init> (Lio/sentry/transport/ICurrentDateProvider;Lio/sentry/SentryOptions;)V
 	public fun addRateLimitObserver (Lio/sentry/transport/RateLimiter$IRateLimitObserver;)V
 	public fun close ()V
@@ -7689,6 +7690,12 @@ public abstract interface class io/sentry/transport/RateLimiter$IRateLimitObserv
 	public abstract fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V
 }
 
+public abstract interface class io/sentry/transport/RateLimiterConfig {
+	public abstract fun getClientReportRecorder ()Lio/sentry/clientreport/IClientReportRecorder;
+	public abstract fun getLogger ()Lio/sentry/ILogger;
+	public abstract fun getTimerExecutorService ()Lio/sentry/ISentryExecutorService;
+}
+
 public final class io/sentry/transport/ReusableCountLatch {
 	public fun <init> ()V
 	public fun <init> (I)V

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -27,6 +27,7 @@
 import io.sentry.transport.ITransportGate;
 import io.sentry.transport.NoOpEnvelopeCache;
 import io.sentry.transport.NoOpTransportGate;
+import io.sentry.transport.RateLimiterConfig;
 import io.sentry.util.AutoClosableReentrantLock;
 import io.sentry.util.LazyEvaluator;
 import io.sentry.util.LoadClass;
@@ -56,7 +57,7 @@
 
 /** Sentry SDK options */
 @Open
-public class SentryOptions {
+public class SentryOptions implements RateLimiterConfig {
 
   @ApiStatus.Internal public static final @NotNull String DEFAULT_PROPAGATION_TARGETS = ".*";
 
@@ -845,6 +846,7 @@ public void setDebug(final boolean debug) {
    *
    * @return the logger
    */
+  @Override
   public @NotNull ILogger getLogger() {
     return logger;
   }
@@ -1608,6 +1610,7 @@ public void setExecutorService(final @NotNull ISentryExecutorService executorSer
    * @return the timer executor service
    */
   @ApiStatus.Internal
+  @Override
   @NotNull
   public ISentryExecutorService getTimerExecutorService() {
     return timerExecutorService;
@@ -2599,6 +2602,7 @@ public void setInstrumenter(final @NotNull Instrumenter instrumenter) {
    * @return a client report recorder or NoOp
    */
   @ApiStatus.Internal
+  @Override
   public @NotNull IClientReportRecorder getClientReportRecorder() {
     return clientReportRecorder;
   }

sentry/src/main/java/io/sentry/transport/RateLimiter.java

@@ -14,6 +14,8 @@
 import io.sentry.hints.DiskFlushNotification;
 import io.sentry.hints.Retryable;
 import io.sentry.hints.SubmissionResult;
+import io.sentry.time.Deadline;
+import io.sentry.time.ElapsedRealtimeClock;
 import io.sentry.util.AutoClosableReentrantLock;
 import io.sentry.util.HintUtils;
 import io.sentry.util.StringUtils;
@@ -22,40 +24,56 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.Date;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Future;
 import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 /** Controls retry limits on different category types sent to Sentry. */
 public final class RateLimiter implements Closeable {
 
-  private static final int HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS = 60000;
+  private static final long HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS = 60_000;
 
-  private final @NotNull ICurrentDateProvider currentDateProvider;
-  private final @NotNull SentryOptions options;
-  private final @NotNull Map<DataCategory, @NotNull Date> sentryRetryAfterLimit =
+  private final @NotNull ElapsedRealtimeClock clock;
+  private final @NotNull RateLimiterConfig config;
+  private final @NotNull Map<DataCategory, @NotNull Deadline> sentryRetryAfterLimit =
       new ConcurrentHashMap<>();
   private final @NotNull List<IRateLimitObserver> rateLimitObservers = new CopyOnWriteArrayList<>();
   private final @NotNull List<Future<?>> notifyObserversFutures = new ArrayList<>();
   private final @NotNull AutoClosableReentrantLock notifyFuturesLock =
       new AutoClosableReentrantLock();
 
   public RateLimiter(
-      final @NotNull ICurrentDateProvider currentDateProvider,
-      final @NotNull SentryOptions options) {
-    this.currentDateProvider = currentDateProvider;
-    this.options = options;
+      final @NotNull ElapsedRealtimeClock clock, final @NotNull RateLimiterConfig config) {
+    this.clock = clock;
+    this.config = config;
   }
 
   public RateLimiter(final @NotNull SentryOptions options) {
-    this(CurrentDateProvider.getInstance(), options);
+    this(options.getElapsedRealtimeClock(), options);
+  }
+
+  /**
+   * @deprecated backoff is measured on {@link SentryOptions#getElapsedRealtimeClock()}; use {@link
+   *     #RateLimiter(SentryOptions)}. An injected wall clock is adapted so that an existing custom
+   *     transport keeps the behaviour it has today, but it is not monotonic and can step.
+   */
+  @Deprecated
+  public RateLimiter(
+      final @NotNull ICurrentDateProvider currentDateProvider,
+      final @NotNull SentryOptions options) {
+    // ICurrentDateProvider is itself a `long ()` interface, so an unadorned lambda matches this
+    // constructor as readily as the intended one. The cast is what makes the call non-recursive.
+    this(
+        (ElapsedRealtimeClock)
+            () -> TimeUnit.MILLISECONDS.toNanos(currentDateProvider.getCurrentTimeMillis()),
+        options);
   }
 
   public @Nullable SentryEnvelope filter(
@@ -70,14 +88,14 @@ public RateLimiter(final @NotNull SentryOptions options) {
         }
 
         dropItems.add(item);
-        options
+        config
             .getClientReportRecorder()
             .recordLostEnvelopeItem(DiscardReason.RATELIMIT_BACKOFF, item);
       }
     }
 
     if (dropItems != null) {
-      options
+      config
           .getLogger()
           .log(
               SentryLevel.WARNING,
@@ -94,7 +112,7 @@ public RateLimiter(final @NotNull SentryOptions options) {
 
       // no reason to continue
       if (toSend.isEmpty()) {
-        options
+        config
             .getLogger()
             .log(SentryLevel.WARNING, "Envelope discarded due all items rate limited.");
 
@@ -107,16 +125,11 @@ public RateLimiter(final @NotNull SentryOptions options) {
     return envelope;
   }
 
-  @SuppressWarnings({"JdkObsolete", "JavaUtilDate"})
   public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) {
-    final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis());
-
     // check all categories
-    final Date dateAllCategories = sentryRetryAfterLimit.get(DataCategory.All);
-    if (dateAllCategories != null) {
-      if (!currentDate.after(dateAllCategories)) {
-        return true;
-      }
+    final @Nullable Deadline allCategories = sentryRetryAfterLimit.get(DataCategory.All);
+    if (allCategories != null && !allCategories.hasPassed()) {
+      return true;
     }
 
     // Unknown should not be rate limited
@@ -125,24 +138,15 @@ public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) {
     }
 
     // check for specific dataCategory
-    final Date dateCategory = sentryRetryAfterLimit.get(dataCategory);
-    if (dateCategory != null) {
-      return !currentDate.after(dateCategory);
-    }
-
-    return false;
+    final @Nullable Deadline categoryLimit = sentryRetryAfterLimit.get(dataCategory);
+    return categoryLimit != null && !categoryLimit.hasPassed();
   }
 
   @SuppressWarnings({"JdkObsolete", "JavaUtilDate"})
   public boolean isAnyRateLimitActive() {
-    final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis());
-
-    for (DataCategory dataCategory : sentryRetryAfterLimit.keySet()) {
-      final Date dateCategory = sentryRetryAfterLimit.get(dataCategory);
-      if (dateCategory != null) {
-        if (!currentDate.after(dateCategory)) {
-          return true;
-        }
+    for (final @NotNull Deadline limit : sentryRetryAfterLimit.values()) {
+      if (!limit.hasPassed()) {
+        return true;
       }
     }
 
@@ -163,7 +167,7 @@ private void markHintWhenSendingFailed(final @NotNull Hint hint, final boolean r
         DiskFlushNotification.class,
         (diskFlushNotification) -> {
           diskFlushNotification.markFlushed();
-          options.getLogger().log(SentryLevel.DEBUG, "Disk flush envelope fired due to rate limit");
+          config.getLogger().log(SentryLevel.DEBUG, "Disk flush envelope fired due to rate limit");
         });
   }
 
@@ -251,15 +255,11 @@ public void updateRetryAfterLimits(
 
         if (rateLimit.length > 0) {
           final String retryAfter = rateLimit[0];
-          long retryAfterMillis = parseRetryAfterOrDefault(retryAfter);
+          final @NotNull Deadline deadline = parseRetryAfterOrDefault(retryAfter);
 
           if (rateLimit.length > 1) {
             final String allCategories = rateLimit[1];
 
-            // we dont care if Date is UTC as we just add the relative seconds
-            final Date date =
-                new Date(currentDateProvider.getCurrentTimeMillis() + retryAfterMillis);
-
             if (allCategories != null && !allCategories.isEmpty()) {
               final String[] categories = allCategories.split(";", -1);
 
@@ -270,48 +270,43 @@ public void updateRetryAfterLimits(
                   if (catItemCapitalized != null) {
                     dataCategory = DataCategory.valueOf(catItemCapitalized);
                   } else {
-                    options.getLogger().log(ERROR, "Couldn't capitalize: %s", catItem);
+                    config.getLogger().log(ERROR, "Couldn't capitalize: %s", catItem);
                   }
                 } catch (IllegalArgumentException e) {
-                  options.getLogger().log(INFO, e, "Unknown category: %s", catItem);
+                  config.getLogger().log(INFO, e, "Unknown category: %s", catItem);
                 }
                 // we dont apply rate limiting for unknown categories
                 if (DataCategory.Unknown.equals(dataCategory)) {
                   continue;
                 }
 
-                applyRetryAfterOnlyIfLonger(dataCategory, date, retryAfterMillis);
+                applyRetryAfterOnlyIfLonger(dataCategory, deadline);
               }
             } else {
               // if categories are empty, we should apply to "all" categories.
-              applyRetryAfterOnlyIfLonger(DataCategory.All, date, retryAfterMillis);
+              applyRetryAfterOnlyIfLonger(DataCategory.All, deadline);
             }
           }
         }
       }
     } else if (errorCode == 429) {
-      final long retryAfterMillis = parseRetryAfterOrDefault(retryAfterHeader);
-      // we dont care if Date is UTC as we just add the relative seconds
-      final Date date = new Date(currentDateProvider.getCurrentTimeMillis() + retryAfterMillis);
-      applyRetryAfterOnlyIfLonger(DataCategory.All, date, retryAfterMillis);
+      applyRetryAfterOnlyIfLonger(DataCategory.All, parseRetryAfterOrDefault(retryAfterHeader));
     }
   }
 
   /**
-   * apply new timestamp for rate limiting only if its longer than the previous one
+   * apply the new deadline for rate limiting only if it is longer than the previous one
    *
    * @param dataCategory the DataCategory
-   * @param date the Date to be applied
-   * @param delayMillis the millis until the rate limit is lifted
+   * @param deadline when the rate limit is lifted
    */
-  @SuppressWarnings({"JdkObsolete", "JavaUtilDate"})
   private void applyRetryAfterOnlyIfLonger(
-      final @NotNull DataCategory dataCategory, final @NotNull Date date, final long delayMillis) {
-    final Date oldDate = sentryRetryAfterLimit.get(dataCategory);
+      final @NotNull DataCategory dataCategory, final @NotNull Deadline deadline) {
+    final @Nullable Deadline oldLimit = sentryRetryAfterLimit.get(dataCategory);
 
-    // only overwrite its previous date if the limit is even longer
-    if (oldDate == null || date.after(oldDate)) {
-      sentryRetryAfterLimit.put(dataCategory, date);
+    // only overwrite the previous deadline if the limit is even longer
+    if (oldLimit == null || deadline.isAfter(oldLimit)) {
+      sentryRetryAfterLimit.put(dataCategory, deadline);
 
       notifyRateLimitObservers();
 
@@ -326,11 +321,12 @@ private void applyRetryAfterOnlyIfLonger(
         }
         try {
           notifyObserversFutures.add(
-              options
+              config
                   .getTimerExecutorService()
-                  .schedule(this::notifyRateLimitObservers, delayMillis));
+                  .schedule(
+                      this::notifyRateLimitObservers, deadline.remaining(TimeUnit.MILLISECONDS)));
         } catch (RejectedExecutionException e) {
-          options
+          config
               .getLogger()
               .log(SentryLevel.WARNING, "Failed to schedule rate limit lifted notification.", e);
         }
@@ -344,7 +340,7 @@ private void applyRetryAfterOnlyIfLonger(
    * @param retryAfterHeader the header
    * @return the millis in seconds or the default seconds value
    */
-  private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) {
+  private @NotNull Deadline parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) {
     long retryAfterMillis = HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS;
     if (retryAfterHeader != null) {
       try {
@@ -354,7 +350,7 @@ private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) {
         // let's use the default then
       }
     }
-    return retryAfterMillis;
+    return Deadline.in(clock, retryAfterMillis, TimeUnit.MILLISECONDS);
   }
 
   private void notifyRateLimitObservers() {

sentry/src/main/java/io/sentry/transport/RateLimiterConfig.java

@@ -0,0 +1,29 @@
+package io.sentry.transport;
+
+import io.sentry.ILogger;
+import io.sentry.ISentryExecutorService;
+import io.sentry.clientreport.IClientReportRecorder;
+import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * The configuration {@link RateLimiter} reads. Declared next to its consumer rather than alongside
+ * the implementation, so that the collaborators a rate limiter actually touches are three lines to
+ * read instead of three hundred, and a test can supply them without building a {@link
+ * io.sentry.SentryOptions}.
+ *
+ * <p>Implementations are expected to delegate to live configuration rather than snapshot it, so
+ * that a logger or executor replaced after {@code Sentry.init} is still picked up.
+ */
+@ApiStatus.Internal
+public interface RateLimiterConfig {
+
+  @NotNull
+  ILogger getLogger();
+
+  @NotNull
+  IClientReportRecorder getClientReportRecorder();
+
+  @NotNull
+  ISentryExecutorService getTimerExecutorService();
+}