fix: retry docker pull network errors instead of failing deployments

src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java

@@ -12,6 +12,7 @@
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerPullException;
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerServiceUnavailableException;
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.InvalidImageOrAccessDeniedException;
+import com.aws.greengrass.componentmanager.plugins.docker.exceptions.UnknownDockerPullException;
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.UserNotAuthorizedForDockerException;
 import com.aws.greengrass.logging.api.Logger;
 import com.aws.greengrass.logging.impl.LogManager;
@@ -26,6 +27,8 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
 
@@ -37,15 +40,70 @@ public class DefaultDockerClient {
     public static final Logger logger = LogManager.getLogger(DefaultDockerClient.class);
 
     /**
-     * connect error messages.
+     * Errors indicating that a docker pull failed at the network transport layer rather than being rejected by the
+     * registry. Such a failure is recoverable once connectivity is restored, so it is retried indefinitely.
+     *
+     * <p>Two shapes are matched. Go renders a {@code net.OpError} as
+     * {@code "<op> <net> <source>-><addr>: <cause>"}, so matching on the op prefix classifies a failure at the TCP
+     * layer regardless of which cause follows, including causes not seen before. Only {@code dial tcp} was matched
+     * previously, which covers a connection that fails to be established; a connection that dies mid-transfer
+     * instead reports {@code read tcp} or {@code write tcp}. The remaining entries are transport-level causes,
+     * matched on their own because docker and containerd also surface them without an op prefix, for example when
+     * wrapping an error raised while copying an image layer.
      */
-    private static final String READ_CONNECTION_TIME_OUT = "read: connection timed out";
-    private static final String NET_HTTP_TIMEOUT = "net/http";
-    private static final String TEMPORARY_FAILURE_IN_NAME_RESOLUTION = "Temporary failure in name resolution";
-    private static final String REQUEST_CANCELED = "request canceled";
-    private static final String DOCKER_PULL_TIMEOUT = "timeout";
-    private static final String NO_SUCH_HOST = "no such host";
-    private static final String DIAL_TCP = "dial tcp";
+    private static final List<String> CONNECTION_ERRORS = Collections.unmodifiableList(Arrays.asList(
+            // Go net.OpError op prefixes for the transport operations
+            "dial tcp",
+            "read tcp",
+            "write tcp",
+            // Transport-level causes
+            "connection reset by peer",
+            "connection refused",
+            "connection aborted",
+            "broken pipe",
+            "network is unreachable",
+            "network is down",
+            "host is unreachable",
+            "no route to host",
+            "read: connection timed out",
+            "i/o timeout",
+            "unexpected eof",
+            "\": eof",
+            // TLS transport failures. Only wire-level failures are listed. An error saying the peer rejected our
+            // certificate, or that we do not trust theirs, is a configuration problem a retry cannot fix, so
+            // "x509:", "bad certificate" and "handshake failure" are deliberately absent.
+            "tls: use of closed connection",
+            "bad record mac",
+            "tls: internal error",
+            // HTTP/2 transport failures, matched on the package prefix for the same reason "net/http" is. A stream
+            // error carries no such prefix, so it is matched separately.
+            "http2:",
+            "stream error: stream id",
+            // Transport failures that containerd surfaces while copying an image layer, having discarded the
+            // net.OpError that produced them
+            "file already closed",
+            // Timeouts and cancellations, which all mean the request did not complete rather than that the registry
+            // rejected it
+            "net/http",
+            "timeout",
+            "request canceled",
+            "context canceled",
+            "context deadline exceeded",
+            // Name resolution failures
+            "no such host",
+            "temporary failure in name resolution",
+            "server misbehaving"));
+
+    /**
+     * Errors that a retry cannot recover from, so a docker pull failing with one of these should fail fast.
+     */
+    private static final List<String> NON_RETRYABLE_ERRORS = Collections.unmodifiableList(Arrays.asList(
+            "manifest unknown",
+            "no matching manifest for",
+            "invalid reference format",
+            "no space left on device",
+            "authentication required",
+            "requested access to the resource is denied"));
 
     /**
      * Sanity check for installation.
@@ -110,7 +168,9 @@ public void login(Registry registry)
      *                                             the registry
      * @throws UserNotAuthorizedForDockerException when current user is not authorized to use docker
      * @throws ConnectionException                 network error
-     * @throws DockerPullException                 unexpected error
+     * @throws DockerPullException                 an error that a retry cannot recover from, or, as
+     *                                             {@link UnknownDockerPullException}, an unrecognized error that may
+     *                                             be transient
      */
     public void pullImage(Image image) throws DockerServiceUnavailableException, InvalidImageOrAccessDeniedException,
             UserNotAuthorizedForDockerException, DockerPullException, ConnectionException {
@@ -135,16 +195,18 @@ public void pullImage(Image image) throws DockerServiceUnavailableException, Inv
                     throw new InvalidImageOrAccessDeniedException(
                             String.format("Invalid image or login - %s", response.err));
                 }
-                if (response.err.contains(READ_CONNECTION_TIME_OUT)
-                        || response.err.contains(TEMPORARY_FAILURE_IN_NAME_RESOLUTION)
-                        || response.err.toLowerCase().contains(NET_HTTP_TIMEOUT)
-                        || response.err.toLowerCase().contains(REQUEST_CANCELED)
-                        || response.err.toLowerCase().contains(DOCKER_PULL_TIMEOUT)
-                        || response.err.toLowerCase().contains(NO_SUCH_HOST)
-                        || response.err.toLowerCase().contains(DIAL_TCP)) {
+                if (isConnectionError(response.err)) {
                     throw new ConnectionException(String.format("Network issue when docker pull - %s", response.err));
                 }
-                throw new DockerPullException(
+                if (isNonRetryableError(response.err)) {
+                    throw new DockerPullException(
+                            String.format("Unexpected error while trying to perform docker pull - %s", response.err),
+                            response.failureCause);
+                }
+                // The error is not recognized as either a network error or a non-retryable one. Assume it may be
+                // transient and let the caller apply a small, bounded number of retries rather than failing the
+                // deployment on the first attempt.
+                throw new UnknownDockerPullException(
                         String.format("Unexpected error while trying to perform docker pull - %s", response.err),
                         response.failureCause);
             }
@@ -154,6 +216,32 @@ public void pullImage(Image image) throws DockerServiceUnavailableException, Inv
         }
     }
 
+    /**
+     * Check if a docker CLI error indicates a network-level failure, which is recoverable once connectivity
+     * is restored and so is retried indefinitely.
+     *
+     * @param err stderr emitted by the docker CLI
+     * @return true if the error is a known network error
+     */
+    static boolean isConnectionError(String err) {
+        return containsAny(err, CONNECTION_ERRORS);
+    }
+
+    /**
+     * Check if a docker CLI error is one that a retry cannot recover from, such as a missing image or a full disk.
+     *
+     * @param err stderr emitted by the docker CLI
+     * @return true if the error is known to be non-retryable
+     */
+    static boolean isNonRetryableError(String err) {
+        return containsAny(err, NON_RETRYABLE_ERRORS);
+    }
+
+    private static boolean containsAny(String err, List<String> lowerCaseNeedles) {
+        String lowerCaseErr = err.toLowerCase(Locale.ROOT);
+        return lowerCaseNeedles.stream().anyMatch(lowerCaseErr::contains);
+    }
+
     /**
      * Check if an image exists locally.
      *

src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java

@@ -17,6 +17,7 @@
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerImageDeleteException;
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerLoginException;
 import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerServiceUnavailableException;
+import com.aws.greengrass.componentmanager.plugins.docker.exceptions.UnknownDockerPullException;
 import com.aws.greengrass.dependency.Context;
 import com.aws.greengrass.mqttclient.MqttClient;
 import com.aws.greengrass.util.CrashableSupplier;
@@ -35,6 +36,7 @@
 import java.security.MessageDigest;
 import java.time.Duration;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
@@ -56,7 +58,7 @@ public class DockerImageDownloader extends ArtifactDownloader {
     @Setter(AccessLevel.PACKAGE)
     private RetryUtils.RetryConfig infiniteAttemptsRetryConfig =
             RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMinutes(1L))
-                    .maxRetryInterval(Duration.ofMinutes(64L)).maxAttempt(Integer.MAX_VALUE).retryableExceptions(
+                    .maxRetryInterval(Duration.ofMinutes(5L)).maxAttempt(Integer.MAX_VALUE).retryableExceptions(
                         Arrays.asList(ConnectionException.class, SdkClientException.class, ServerException.class))
                     .build();
     @Setter(AccessLevel.PACKAGE)
@@ -66,6 +68,16 @@ public class DockerImageDownloader extends ArtifactDownloader {
                             Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
                             SdkClientException.class, ServerException.class)).build();
 
+    // A docker pull error that is recognized as neither a network error nor a non-retryable error may still be
+    // transient, so allow a small number of retries. Deliberately much shorter than finiteAttemptsRetryConfig: the
+    // error is not known to be recoverable, so this only needs to be long enough to ride out a brief blip before
+    // reporting the failure.
+    @Setter(AccessLevel.PACKAGE)
+    private RetryUtils.RetryConfig unknownErrorRetryConfig =
+            RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(10L))
+                    .maxRetryInterval(Duration.ofMinutes(2L)).maxAttempt(5).retryableExceptions(
+                            Collections.singletonList(UnknownDockerPullException.class)).build();
+
     @Setter(AccessLevel.PACKAGE)
     private RetryUtils.RetryConfig finiteDeleteAttemptsRetryConfig =
             RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(10L))
@@ -285,8 +297,11 @@ private <T> void run(CrashableSupplier<T, Exception> task, String description, S
             throws PackageDownloadException, InterruptedException {
         try {
             // Finite retry attempts for errors that are not due to connectivity issues and
-            // might need explicit intervention to recover from
-            RetryUtils.runWithRetry(finiteAttemptsRetryConfig, () -> RetryUtils
+            // might need explicit intervention to recover from, and a shorter finite retry for errors that could
+            // not be classified at all
+            RetryUtils.runWithRetry(RetryUtils.DifferentiatedRetryConfig.builder()
+                            .retryConfigList(Arrays.asList(finiteAttemptsRetryConfig, unknownErrorRetryConfig)).build(),
+                    () -> RetryUtils
                     // Indefinite retry for errors that are due to connectivity issues and can be
                     // resolved when connectivity comes back
                     .runWithRetry(infiniteAttemptsRetryConfig, () -> runWithConnectionErrorCheck(task), description,
@@ -306,7 +321,15 @@ private <T> T runWithConnectionErrorCheck(CrashableSupplier<T, Exception> task)
             // can be fixed with retries, explicitly check if an error could be due to connectivity problem
             // we infer that based on Mqtt connection, even though not perfect, it should accurately represent if
             // device is having connectivity issues most of the times.
-            if (e instanceof DockerServiceUnavailableException && !mqttClient.getMqttOnline().get()) {
+            // The same inference applies to a pull error that could not be classified at all. Classification depends
+            // on docker's stderr text, which is not a stable contract and cannot cover every transport failure, so
+            // the device's own connectivity is used as independent evidence: if it is offline, an unrecognized
+            // failure is far more likely to be that outage than anything the registry reported, and should be
+            // retried until connectivity returns rather than given up on after a bounded number of attempts. When
+            // the device is online the bounded retry still applies, so an error that is genuinely permanent
+            // continues to surface promptly.
+            if ((e instanceof DockerServiceUnavailableException || e instanceof UnknownDockerPullException)
+                    && !mqttClient.getMqttOnline().get()) {
                 throw new ConnectionException("Device appears to be offline, should retry the task", e);
             }
             throw e;

src/main/java/com/aws/greengrass/componentmanager/plugins/docker/exceptions/UnknownDockerPullException.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.aws.greengrass.componentmanager.plugins.docker.exceptions;
+
+/**
+ * A {@code docker pull} failure that could not be classified as either a known network error or a known
+ * non-retryable error.
+ *
+ * <p>Such a failure is treated as possibly transient and is given a small, bounded number of retries. Extends
+ * {@link DockerPullException} so that the deployment error code reported to the customer once those retries are
+ * exhausted is unchanged.
+ */
+public class UnknownDockerPullException extends DockerPullException {
+    static final long serialVersionUID = -3387516993124229948L;
+
+    public UnknownDockerPullException(String message) {
+        super(message);
+    }
+
+    public UnknownDockerPullException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

src/main/java/com/aws/greengrass/util/RetryUtils.java

@@ -61,12 +61,18 @@ public static <T> T runWithRetry(RetryConfig retryConfig, CrashableSupplier<T, E
     public static <T> T runWithRetry(DifferentiatedRetryConfig differentiatedRetryConfig,
                                      CrashableSupplier<T, Exception> task, String taskDescription, Logger logger)
             throws Exception {
-        long retryInterval = 0;
         long totalAttempts = 0;
         long totalMaxAttempts = calculateTotalMaxAttempts(differentiatedRetryConfig);
         Map<RetryConfig, Integer> attemptMap = new HashMap<>();
+        // Each config backs off independently. Sharing one interval across configs would let a config with a high
+        // ceiling ramp the interval and then have a config with a lower ceiling sleep past its own ceiling, because
+        // the sleep happens before the interval is clamped.
+        Map<RetryConfig, Long> retryIntervalMap = new HashMap<>();
         differentiatedRetryConfig.getRetryConfigList()
-                .forEach(retryConfig -> attemptMap.put(retryConfig, 1));
+                .forEach(retryConfig -> {
+                    attemptMap.put(retryConfig, 1);
+                    retryIntervalMap.put(retryConfig, retryConfig.getInitialRetryInterval().toMillis());
+                });
 
         while (totalAttempts < totalMaxAttempts) {
             if (Thread.currentThread().isInterrupted()) {
@@ -98,11 +104,10 @@ public static <T> T runWithRetry(DifferentiatedRetryConfig differentiatedRetryCo
                         logBuilder.kv("task-attempt", attempt).setCause(e).log("task failed and will be retried");
 
                         // sleep with back-off
-                        if (retryInterval == 0) {
-                            retryInterval = retryConfig.getInitialRetryInterval().toMillis();
-                        }
+                        long retryInterval = retryIntervalMap.get(retryConfig);
                         Thread.sleep(retryInterval / 2 + RANDOM.nextInt((int) (retryInterval / 2 + 1)));
-                        retryInterval = Math.min(retryInterval * 2, retryConfig.getMaxRetryInterval().toMillis());
+                        retryIntervalMap.put(retryConfig,
+                                Math.min(retryInterval * 2, retryConfig.getMaxRetryInterval().toMillis()));
 
                         // break since exception is found
                         break;