Simplify RateLimitExceeded with no retry-duration

- Avoid passing negative durations in error cases
- Drop unused message
- Return a duration for a bad forwarded-for

service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java

@@ -635,9 +635,13 @@ public class AccountController {
     }
 
     final String mostRecentProxy = ForwardedIpUtil.getMostRecentProxy(forwardedFor)
-        // Missing/malformed Forwarded-For, cannot calculate a reasonable backoff
+        .orElseThrow(() -> {
-        // duration
+          // Missing/malformed Forwarded-For, so we cannot check for a rate-limit.
-        .orElseThrow(() -> new RateLimitExceededException(Duration.ofHours(-1)));
+          // This shouldn't happen, so conservatively assume we're over the rate-limit
+          // and indicate that the client should retry
+          logger.error("Missing/bad Forwarded-For, cannot check account {}", uuid.toString());
+          return new RateLimitExceededException(Duration.ofHours(1));
+        });
 
     rateLimiters.getCheckAccountExistenceLimiter().validate(mostRecentProxy);
 

service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java

@@ -4,22 +4,26 @@
  */
 package org.whispersystems.textsecuregcm.controllers;
 
+import javax.annotation.Nullable;
 import java.time.Duration;
 import java.util.Optional;
 
 public class RateLimitExceededException extends Exception {
 
-  private final Optional<Duration> retryDuration;
+  private final @Nullable
+  Duration retryDuration;
 
-  public RateLimitExceededException(final Duration retryDuration) {
+  /**
-    this(null, retryDuration);
+   * Constructs a new exception indicating when it may become safe to retry
+   *
+   * @param retryDuration A duration to wait before retrying, null if no duration can be indicated
+   */
+  public RateLimitExceededException(final @Nullable Duration retryDuration) {
+    super(null, null, true, false);
+    this.retryDuration = retryDuration;
   }
 
-  public RateLimitExceededException(final String message, final Duration retryDuration) {
+  public Optional<Duration> getRetryDuration() {
-    super(message, null, true, false);
+    return Optional.ofNullable(retryDuration);
-    // we won't provide a backoff in the case the duration is negative
-    this.retryDuration = retryDuration.isNegative() ? Optional.empty() : Optional.of(retryDuration);
   }
-
-  public Optional<Duration> getRetryDuration() { return retryDuration; }
 }

service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java

@@ -54,7 +54,8 @@ public class CardinalityRateLimiter {
     });
 
     if (rateLimitExceeded) {
-      throw new RateLimitExceededException(Duration.ofSeconds(getRemainingTtl(key)));
+      long remainingTtl = getRemainingTtl(key);
+      throw new RateLimitExceededException(remainingTtl >= 0 ? Duration.ofSeconds(remainingTtl) : null);
     }
   }
 
@@ -66,7 +67,14 @@ public class CardinalityRateLimiter {
     return ttl;
   }
 
+  /**
+   * Get the remaining ttl for the specified key
+   *
+   * @param key with timeout to check
+   * @return the ttl, or negative in the case of error
+   */
   public long getRemainingTtl(final String key) {
+    // ttl() returns -2 if key does not exist, -1 if key has no expiration
     return cacheCluster.withCluster(connection -> connection.sync().ttl(getHllKey(key)));
   }
 

service/src/main/java/org/whispersystems/textsecuregcm/limits/LockingRateLimiter.java

@@ -31,7 +31,7 @@ public class LockingRateLimiter extends RateLimiter {
   public void validate(String key, int amount) throws RateLimitExceededException {
     if (!acquireLock(key)) {
       meter.mark();
-      throw new RateLimitExceededException("Locked", Duration.ZERO);
+      throw new RateLimitExceededException(Duration.ZERO);
     }
 
     try {

service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java

@@ -57,7 +57,7 @@ public class RateLimiter {
         setBucket(key, bucket);
       } else {
         meter.mark();
-        throw new RateLimitExceededException(key + " , " + amount, bucket.getTimeUntilSpaceAvailable(amount));
+        throw new RateLimitExceededException(bucket.getTimeUntilSpaceAvailable(amount));
       }
     }
   }

service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java

@@ -4,6 +4,8 @@
  */
 package org.whispersystems.textsecuregcm.mappers;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException;
 
 import javax.ws.rs.core.Response;
@@ -12,17 +14,25 @@ import javax.ws.rs.ext.Provider;
 
 @Provider
 public class RateLimitExceededExceptionMapper implements ExceptionMapper<RateLimitExceededException> {
+  private static final Logger logger = LoggerFactory.getLogger(RateLimitExceededExceptionMapper.class);
 
   /**
    * Convert a RateLimitExceededException to a 413 response with a
    * Retry-After header.
    *
-   * @param e A RateLimitExceededException potentially containing a reccomended retry duration
+   * @param e A RateLimitExceededException potentially containing a recommended retry duration
    * @return the response
    */
   @Override
   public Response toResponse(RateLimitExceededException e) {
     return e.getRetryDuration()
+        .filter(d -> {
+          if (d.isNegative()) {
+            logger.warn("Encountered a negative retry duration: {}, will not include a Retry-After header in response", d);
+          }
+          // only include non-negative durations in retry headers
+          return !d.isNegative();
+        })
         .map(d -> Response.status(413).header("Retry-After", d.toSeconds()))
         .orElseGet(() -> Response.status(413)).build();
   }

service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java

@@ -274,14 +274,14 @@ class AccountControllerTest {
     when(recaptchaClient.verify(eq(INVALID_CAPTCHA_TOKEN), anyString())).thenReturn(false);
     when(recaptchaClient.verify(eq(VALID_CAPTCHA_TOKEN), anyString())).thenReturn(true);
 
-    doThrow(new RateLimitExceededException(SENDER_OVER_PIN, Duration.ZERO)).when(pinLimiter).validate(eq(SENDER_OVER_PIN));
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(pinLimiter).validate(eq(SENDER_OVER_PIN));
 
-    doThrow(new RateLimitExceededException(RATE_LIMITED_PREFIX_HOST, Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_PREFIX_HOST));
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_PREFIX_HOST));
-    doThrow(new RateLimitExceededException(RATE_LIMITED_IP_HOST, Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_IP_HOST));
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_IP_HOST));
 
-    doThrow(new RateLimitExceededException(SENDER_OVER_PREFIX, Duration.ZERO)).when(smsVoicePrefixLimiter).validate(SENDER_OVER_PREFIX.substring(0, 4+2));
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoicePrefixLimiter).validate(SENDER_OVER_PREFIX.substring(0, 4+2));
-    doThrow(new RateLimitExceededException(RATE_LIMITED_IP_HOST, Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_IP_HOST);
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_IP_HOST);
-    doThrow(new RateLimitExceededException(RATE_LIMITED_HOST2, Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_HOST2);
+    doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_HOST2);
   }
 
   @AfterEach
@@ -1833,7 +1833,7 @@ class AccountControllerTest {
         .head();
 
     assertThat(response.getStatus()).isEqualTo(413);
-    assertThat(response.getHeaderString("Retry-After")).isNull();
+    assertThat(Long.parseLong(response.getHeaderString("Retry-After"))).isNotNegative();
   }
 
   @Test