Don't retry permanent failures when uploading ICANN monthly reports (#328)

* Don't retry permanent failures when uploading ICANN monthly reports

There are two kinds of permanent failures that this checks for that we know will
never succeed, so it makes no sense to continue retrying 11 more times before
moving onto the next file to upload. These errors are:

1.
com.google.api.client.http.HttpResponseException: 403
Your IP address xx.xx.xx.xx is not allowed to connect

2.
com.google.api.client.http.HttpResponseException: 400
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><response xmlns="urn:ietf:params:xml:ns:iirdea-1.0"><result code="2002"><msg>A report for that month already exists, the cut-off date already passed.</msg><description>Date: 2019-09</description></result></response>

In order to implement this new functionality, this commit also adds a new way to
call Retriable that allows specifying the isRetryable Predicate (which is quite
useful).

core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java

@@ -39,6 +39,7 @@ import google.registry.util.Retrier;
 import google.registry.util.SendEmailService;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.mail.internet.InternetAddress;
@@ -102,7 +103,7 @@ public final class IcannReportingUploadAction implements Runnable {
                   final byte[] payload = readBytesFromGcs(gcsFilename);
                   return icannReporter.send(payload, reportFilename);
                 },
-                IOException.class);
+                IcannReportingUploadAction::isUploadFailureRetryable);
       } catch (RuntimeException e) {
         logger.atWarning().withCause(e).log("Upload to %s failed.", gcsFilename);
       }
@@ -115,6 +116,21 @@ public final class IcannReportingUploadAction implements Runnable {
         String.format("OK, attempted uploading %d reports", manifestedFiles.size()));
   }
 
+  /** Don't retry when reports are already uploaded or can't be uploaded. */
+  private static final String ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE =
+      "A report for that month already exists, the cut-off date already passed.";
+
+  /** Don't retry when the IP address isn't whitelisted, as retries go through the same IP. */
+  private static final Pattern ICANN_UPLOAD_WHITELIST_ERROR =
+      Pattern.compile("Your IP address .+ is not allowed to connect");
+
+  /** Predicate to retry uploads on IOException, so long as they aren't non-retryable errors. */
+  private static boolean isUploadFailureRetryable(Throwable e) {
+    return (e instanceof IOException)
+        && !e.getMessage().contains(ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE)
+        && !ICANN_UPLOAD_WHITELIST_ERROR.matcher(e.getMessage()).matches();
+  }
+
   private void emailUploadResults(ImmutableMap<String, Boolean> reportSummary) {
     String subject = String.format(
         "ICANN Monthly report upload summary: %d/%d succeeded",

core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java

@@ -154,6 +154,45 @@ public class IcannReportingUploadActionTest {
                 new InternetAddress("recipient@example.com"),
                 new InternetAddress("sender@example.com")));
   }
+
+  @Test
+  public void testFailure_quicklySkipsOverNonRetryableUploadException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException(
+            "<msg>A report for that month already exists, the cut-off date already"
+                + " passed.</msg>"));
+  }
+
+  @Test
+  public void testFailure_quicklySkipsOverIpWhitelistException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException("Your IP address 25.147.130.158 is not allowed to connect"));
+  }
+
+  private void runTest_nonRetryableException(Exception nonRetryableException) throws Exception {
+    IcannReportingUploadAction action = createAction();
+    when(mockReporter.send(PAYLOAD_FAIL, "a-activity-201706.csv"))
+        .thenThrow(nonRetryableException)
+        .thenThrow(
+            new AssertionError(
+                "This should never be thrown because the previous exception isn't retryable"));
+    action.run();
+    verify(mockReporter, times(1)).send(PAYLOAD_FAIL, "a-activity-201706.csv");
+    verify(mockReporter).send(PAYLOAD_SUCCESS, "test-transactions-201706.csv");
+    verifyNoMoreInteractions(mockReporter);
+    assertThat(((FakeResponse) action.response).getPayload())
+        .isEqualTo("OK, attempted uploading 2 reports");
+    verify(emailService)
+        .sendEmail(
+            EmailMessage.create(
+                "ICANN Monthly report upload summary: 1/2 succeeded",
+                "Report Filename - Upload status:\n"
+                    + "test-transactions-201706.csv - SUCCESS\n"
+                    + "a-activity-201706.csv - FAILURE",
+                new InternetAddress("recipient@example.com"),
+                new InternetAddress("sender@example.com")));
+  }
+
   @Test
   public void testFail_FileNotFound() throws Exception {
     IcannReportingUploadAction action = createAction();

util/src/main/java/google/registry/util/Retrier.java

@@ -70,32 +70,8 @@ public class Retrier implements Serializable {
    *
    * @return <V> the value returned by the {@link Callable}.
    */
-  private <V> V callWithRetry(
-      Callable<V> callable,
-      FailureReporter failureReporter,
-      Predicate<Throwable> isRetryable) {
-    int failures = 0;
-    while (true) {
-      try {
-        return callable.call();
-      } catch (Throwable e) {
-        if (++failures == attempts || !isRetryable.test(e)) {
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-        failureReporter.beforeRetry(e, failures, attempts);
-        try {
-          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
-          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
-        } catch (InterruptedException e2) {
-          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
-          // so the next blocking operation will know to abort the thread.
-          Thread.currentThread().interrupt();
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-      }
-    }
+  public <V> V callWithRetry(Callable<V> callable, Predicate<Throwable> isRetryable) {
+    return callWithRetry(callable, LOGGING_FAILURE_REPORTER, isRetryable);
   }
 
   /**
@@ -169,6 +145,34 @@ public class Retrier implements Serializable {
     callWithRetry(callable.asCallable(), failureReporter, retryableError, moreRetryableErrors);
   }
 
+  private <V> V callWithRetry(
+      Callable<V> callable,
+      FailureReporter failureReporter,
+      Predicate<Throwable> isRetryable) {
+    int failures = 0;
+    while (true) {
+      try {
+        return callable.call();
+      } catch (Throwable e) {
+        if (++failures == attempts || !isRetryable.test(e)) {
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+        failureReporter.beforeRetry(e, failures, attempts);
+        try {
+          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
+          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
+        } catch (InterruptedException e2) {
+          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
+          // so the next blocking operation will know to abort the thread.
+          Thread.currentThread().interrupt();
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+      }
+    }
+  }
+
   private static final FailureReporter LOGGING_FAILURE_REPORTER =
       (thrown, failures, maxAttempts) ->
           logger.atInfo().withCause(thrown).log(

util/src/test/java/google/registry/util/RetrierTest.java

@@ -38,7 +38,7 @@ public class RetrierTest {
     }
   }
 
-  /** Test object that always throws an exception with the current count. */
+  /** Test object that throws CountingExceptions up to a given limit, then succeeds. */
   static class CountingThrower implements Callable<Integer> {
 
     int count = 0;
@@ -125,4 +125,21 @@ public class RetrierTest {
         .isEqualTo(0);
     assertThat(reporter.numBeforeRetry).isEqualTo(0);
   }
+
+  @Test
+  public void testRetryPredicate_succeedsWhenRetries() {
+    // Throws a retryable "1" exception is retryable, and then it succeeds on "1".
+    assertThat(retrier.callWithRetry(new CountingThrower(1), e -> e.getMessage().equals("1")))
+        .isEqualTo(1);
+  }
+
+  @Test
+  public void testRetryPredicate_failsWhenDoesntRetry() {
+    // Throws a retryable "1" exception, then a non-retryable "2" exception, resulting in failure.
+    CountingException ex =
+        assertThrows(
+            CountingException.class,
+            () -> retrier.callWithRetry(new CountingThrower(2), e -> e.getMessage().equals("1")));
+    assertThat(ex).hasMessageThat().isEqualTo("2");
+  }
 }