retry hCaptcha errors

Co-authored-by: Jon Chambers <63609320+jon-signal@users.noreply.github.com>

service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java

@@ -406,6 +406,8 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
         .scheduledExecutorService(name(getClass(), "secureValueRecoveryServiceRetry-%d")).threads(1).build();
     ScheduledExecutorService storageServiceRetryExecutor = environment.lifecycle()
         .scheduledExecutorService(name(getClass(), "storageServiceRetry-%d")).threads(1).build();
+    ScheduledExecutorService hcaptchaRetryExecutor = environment.lifecycle()
+        .scheduledExecutorService(name(getClass(), "hCaptchaRetry-%d")).threads(1).build();
 
     Scheduler messageDeliveryScheduler = Schedulers.fromExecutorService(
         ExecutorServiceMetrics.monitor(Metrics.globalRegistry,
@@ -569,9 +571,11 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
         config.getRecaptchaConfiguration().projectPath(),
         config.getRecaptchaConfiguration().credentialConfigurationJson(),
         dynamicConfigurationManager);
-    HttpClient hcaptchaHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2)
-        .connectTimeout(Duration.ofSeconds(10)).build();
-    HCaptchaClient hCaptchaClient = new HCaptchaClient(config.getHCaptchaConfiguration().apiKey().value(), hcaptchaHttpClient,
+    HCaptchaClient hCaptchaClient = new HCaptchaClient(
+        config.getHCaptchaConfiguration().getApiKey().value(),
+        hcaptchaRetryExecutor,
+        config.getHCaptchaConfiguration().getCircuitBreaker(),
+        config.getHCaptchaConfiguration().getRetry(),
         dynamicConfigurationManager);
     HttpClient shortCodeRetrieverHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2)
         .connectTimeout(Duration.ofSeconds(10)).build();

service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java

@@ -7,6 +7,7 @@ package org.whispersystems.textsecuregcm.captcha;
 
 import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;
 
+import com.google.common.annotations.VisibleForTesting;
 import io.micrometer.core.instrument.Metrics;
 import java.io.IOException;
 import java.math.BigDecimal;
@@ -16,15 +17,25 @@ import java.net.http.HttpClient;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.util.Collections;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
 import javax.ws.rs.core.Response;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration;
+import org.whispersystems.textsecuregcm.configuration.RetryConfiguration;
 import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration;
 import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
+import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient;
 import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;
+import org.whispersystems.textsecuregcm.util.ExceptionUtils;
 import org.whispersystems.textsecuregcm.util.SystemMapper;
 
 public class HCaptchaClient implements CaptchaClient {
@@ -34,16 +45,36 @@ public class HCaptchaClient implements CaptchaClient {
   private static final String ASSESSMENT_REASON_COUNTER_NAME = name(HCaptchaClient.class, "assessmentReason");
   private static final String INVALID_REASON_COUNTER_NAME = name(HCaptchaClient.class, "invalidReason");
   private final String apiKey;
-  private final HttpClient client;
+  private final FaultTolerantHttpClient client;
   private final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager;
 
+  @VisibleForTesting
+  HCaptchaClient(final String apiKey,
+      final FaultTolerantHttpClient faultTolerantHttpClient,
+      final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
+    this.apiKey = apiKey;
+    this.client = faultTolerantHttpClient;
+    this.dynamicConfigurationManager = dynamicConfigurationManager;
+  }
+
   public HCaptchaClient(
       final String apiKey,
-      final HttpClient client,
+      final ScheduledExecutorService retryExecutor,
+      final CircuitBreakerConfiguration circuitBreakerConfiguration,
+      final RetryConfiguration retryConfiguration,
       final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
-    this.apiKey = apiKey;
-    this.client = client;
-    this.dynamicConfigurationManager = dynamicConfigurationManager;
+    this(apiKey,
+        FaultTolerantHttpClient.newBuilder()
+            .withName("hcaptcha")
+            .withCircuitBreaker(circuitBreakerConfiguration)
+            .withExecutor(Executors.newCachedThreadPool())
+            .withRetryExecutor(retryExecutor)
+            .withRetry(retryConfiguration)
+            .withRetryOnException(ex -> ex instanceof IOException)
+            .withConnectTimeout(Duration.ofSeconds(10))
+            .withVersion(HttpClient.Version.HTTP_2)
+            .build(),
+        dynamicConfigurationManager);
   }
 
   @Override
@@ -82,11 +113,12 @@ public class HCaptchaClient implements CaptchaClient {
         .POST(HttpRequest.BodyPublishers.ofString(body))
         .build();
 
-    HttpResponse<String> response;
+    final HttpResponse<String> response;
     try {
-      response = this.client.send(request, HttpResponse.BodyHandlers.ofString());
-    } catch (InterruptedException e) {
-      throw new IOException(e);
+      response = this.client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join();
+    } catch (CompletionException e) {
+      logger.warn("failed to make http request to hCaptcha: {}", e.getMessage());
+      throw new IOException(ExceptionUtils.unwrap(e));
     }
 
     if (response.statusCode() != Response.Status.OK.getStatusCode()) {

service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java

@@ -5,8 +5,35 @@
 
 package org.whispersystems.textsecuregcm.configuration;
 
+import com.fasterxml.jackson.annotation.JsonProperty;
 import javax.validation.constraints.NotNull;
 import org.whispersystems.textsecuregcm.configuration.secrets.SecretString;
 
-public record HCaptchaConfiguration(@NotNull SecretString apiKey) {
+public class HCaptchaConfiguration {
+
+  @JsonProperty
+  @NotNull
+  SecretString apiKey;
+
+  @JsonProperty
+  @NotNull
+  CircuitBreakerConfiguration circuitBreaker = new CircuitBreakerConfiguration();
+
+  @JsonProperty
+  @NotNull
+  RetryConfiguration retry = new RetryConfiguration();
+
+
+  public SecretString getApiKey() {
+    return apiKey;
+  }
+
+  public CircuitBreakerConfiguration getCircuitBreaker() {
+    return circuitBreaker;
+  }
+
+  public RetryConfiguration getRetry() {
+    return retry;
+  }
+
 }

service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java

@@ -5,6 +5,7 @@
 
 package org.whispersystems.textsecuregcm.http;
 
+import com.google.common.annotations.VisibleForTesting;
 import io.github.resilience4j.circuitbreaker.CircuitBreaker;
 import io.github.resilience4j.retry.Retry;
 import io.github.resilience4j.retry.RetryConfig;
@@ -18,12 +19,14 @@ import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionStage;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Predicate;
 import java.util.function.Supplier;
 import org.glassfish.jersey.SslConfigurator;
 import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration;
 import org.whispersystems.textsecuregcm.configuration.RetryConfiguration;
 import org.whispersystems.textsecuregcm.util.CertificateUtil;
 import org.whispersystems.textsecuregcm.util.CircuitBreakerUtil;
+import org.whispersystems.textsecuregcm.util.ExceptionUtils;
 
 public class FaultTolerantHttpClient {
 
@@ -40,9 +43,10 @@ public class FaultTolerantHttpClient {
     return new Builder();
   }
 
-  private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor,
+  @VisibleForTesting
+  FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor,
       Duration defaultRequestTimeout, RetryConfiguration retryConfiguration,
-      CircuitBreakerConfiguration circuitBreakerConfiguration) {
+      final Predicate<Throwable> retryOnException, CircuitBreakerConfiguration circuitBreakerConfiguration) {
 
     this.httpClient = httpClient;
     this.retryExecutor = retryExecutor;
@@ -55,9 +59,12 @@ public class FaultTolerantHttpClient {
       if (this.retryExecutor == null) {
         throw new IllegalArgumentException("retryExecutor must be specified with retryConfiguration");
       }
-      RetryConfig retryConfig = retryConfiguration.<HttpResponse>toRetryConfigBuilder()
-          .retryOnResult(o -> o.statusCode() >= 500).build();
-      this.retry = Retry.of(name + "-retry", retryConfig);
+      final RetryConfig.Builder<HttpResponse> retryConfig = retryConfiguration.<HttpResponse>toRetryConfigBuilder()
+          .retryOnResult(o -> o.statusCode() >= 500);
+      if (retryOnException != null) {
+        retryConfig.retryOnException(retryOnException);
+      }
+      this.retry = Retry.of(name + "-retry", retryConfig.build());
       CircuitBreakerUtil.registerMetrics(retry, FaultTolerantHttpClient.class);
     } else {
       this.retry = null;
@@ -101,6 +108,7 @@ public class FaultTolerantHttpClient {
     private KeyStore trustStore;
     private String securityProtocol = SECURITY_PROTOCOL_TLS_1_2;
     private RetryConfiguration retryConfiguration;
+    private Predicate<Throwable> retryOnException;
     private CircuitBreakerConfiguration circuitBreakerConfiguration;
 
     private Builder() {
@@ -161,6 +169,11 @@ public class FaultTolerantHttpClient {
       return this;
     }
 
+    public Builder withRetryOnException(final Predicate<Throwable> predicate) {
+      this.retryOnException = throwable -> predicate.test(ExceptionUtils.unwrap(throwable));
+      return this;
+    }
+
     public FaultTolerantHttpClient build() {
       if (this.circuitBreakerConfiguration == null || this.name == null || this.executor == null) {
         throw new IllegalArgumentException("Must specify circuit breaker config, name, and executor");
@@ -181,7 +194,7 @@ public class FaultTolerantHttpClient {
       builder.sslContext(sslConfigurator.createSSLContext());
 
       return new FaultTolerantHttpClient(name, builder.build(), retryExecutor, requestTimeout, retryConfiguration,
-          circuitBreakerConfiguration);
+          retryOnException, circuitBreakerConfiguration);
     }
   }
 

service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java

@@ -15,6 +15,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CompletableFuture;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -22,6 +23,7 @@ import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration;
 import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
+import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient;
 import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;
 
 public class HCaptchaClientTest {
@@ -44,7 +46,7 @@ public class HCaptchaClientTest {
   public void captchaProcessed(final boolean success, final float score, final boolean expectedResult)
       throws IOException, InterruptedException {
 
-    final HttpClient client = mockResponder(200, String.format("""
+    final FaultTolerantHttpClient client = mockResponder(200, String.format("""
             {
               "success": %b,
               "score": %f,
@@ -64,14 +66,14 @@ public class HCaptchaClientTest {
 
   @Test
   public void errorResponse() throws IOException, InterruptedException {
-    final HttpClient httpClient = mockResponder(503, "");
+    final FaultTolerantHttpClient httpClient = mockResponder(503, "");
     final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
     assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null));
   }
 
   @Test
   public void invalidScore() throws IOException, InterruptedException {
-    final HttpClient httpClient = mockResponder(200, """
+    final FaultTolerantHttpClient httpClient = mockResponder(200, """
         {"success" : true, "score": 1.1}
         """);
     final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
@@ -80,7 +82,7 @@ public class HCaptchaClientTest {
 
   @Test
   public void badBody() throws IOException, InterruptedException {
-    final HttpClient httpClient = mockResponder(200, """
+    final FaultTolerantHttpClient httpClient = mockResponder(200, """
         {"success" : true,
         """);
     final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
@@ -102,15 +104,14 @@ public class HCaptchaClientTest {
     }
   }
 
-  private static HttpClient mockResponder(final int statusCode, final String jsonBody)
-      throws IOException, InterruptedException {
-    HttpClient httpClient = mock(HttpClient.class);
+  private static FaultTolerantHttpClient mockResponder(final int statusCode, final String jsonBody) {
+    FaultTolerantHttpClient httpClient = mock(FaultTolerantHttpClient.class);
     @SuppressWarnings("unchecked") final HttpResponse<Object> httpResponse = mock(HttpResponse.class);
 
     when(httpResponse.body()).thenReturn(jsonBody);
     when(httpResponse.statusCode()).thenReturn(statusCode);
 
-    when(httpClient.send(any(), any())).thenReturn(httpResponse);
+    when(httpClient.sendAsync(any(), any())).thenReturn(CompletableFuture.completedFuture(httpResponse));
     return httpClient;
   }
 

service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java

@@ -11,6 +11,11 @@ import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
 import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
 import io.github.resilience4j.circuitbreaker.CallNotPermittedException;
@@ -19,6 +24,8 @@ import java.net.URI;
 import java.net.http.HttpClient;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
+import java.time.Duration;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -114,6 +121,35 @@ class FaultTolerantHttpClientTest {
     wireMock.verify(3, getRequestedFor(urlEqualTo("/failure")));
   }
 
+  @Test
+  void testRetryGetOnException() {
+    final HttpClient mockHttpClient = mock(HttpClient.class);
+    final FaultTolerantHttpClient client = new FaultTolerantHttpClient(
+        "test",
+        mockHttpClient,
+        retryExecutor,
+        Duration.ofSeconds(1),
+        new RetryConfiguration(),
+        throwable -> throwable instanceof IOException,
+        new CircuitBreakerConfiguration());
+
+    when(mockHttpClient.sendAsync(any(), any()))
+        .thenReturn(CompletableFuture.failedFuture(new IOException("test exception")));
+
+    HttpRequest request = HttpRequest.newBuilder()
+        .uri(URI.create("http://localhost:1234/failure"))
+        .GET()
+        .build();
+
+    try {
+      client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join();
+      throw new AssertionError("Should have failed!");
+    } catch (CompletionException e) {
+      assertThat(e.getCause()).isInstanceOf(IOException.class);
+    }
+    verify(mockHttpClient, times(3)).sendAsync(any(), any());
+  }
+
   @Test
   void testNetworkFailureCircuitBreaker() throws InterruptedException {
     CircuitBreakerConfiguration circuitBreakerConfiguration = new CircuitBreakerConfiguration();