From ad461f892b4cf46953db172d2d6811ece5d5056d Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Mon, 17 Aug 2026 20:52:14 +0000 Subject: [PATCH 1/8] fix: retry unrecognized docker pull errors instead of failing immediately A failed docker pull is classified by substring-matching the docker CLI's stderr against a list of seven known network-error strings. Anything that matches becomes ConnectionException and is retried; anything that does not becomes DockerPullException, which appears in no retry config's retryableExceptions list and so fails the deployment on the first attempt. A device whose network fails over mid-pull reports: read tcp 172.28.1.230:52044->34.204.60.241:443: read: connection reset by peer That matches none of the seven. It is read tcp rather than dial tcp, and read: connection reset by peer rather than read: connection timed out. The same failover happening a moment earlier, before the connection is established, produces dial tcp and does retry, which is why the failure looks intermittent and why redeploying succeeds. Invert the default for this classification. Known network errors keep mapping to ConnectionException and its indefinite retry, and a new explicit list of errors a retry cannot recover from (manifest unknown, no matching manifest, invalid reference format, no space left on device, authentication required, access denied) keeps mapping to DockerPullException and failing fast. Every one of those already failed fast before this change, so their behavior is unchanged. What changes is the remainder: an error matching neither list is now UnknownDockerPullException, retried up to 5 times at 10s to 2m before the deployment fails. Bounding those retries rather than routing them to the indefinite tier keeps the failure mode for a genuinely permanent error that the non-retryable list does not yet name to a bounded delay before the same failure surfaces, rather than a deployment that never completes. UnknownDockerPullException extends DockerPullException so the deployment error code reported once retries are exhausted is still DOCKER_PULL_ERROR. --- .../plugins/docker/DefaultDockerClient.java | 67 ++++++++++++++++--- .../plugins/docker/DockerImageDownloader.java | 19 +++++- .../UnknownDockerPullException.java | 26 +++++++ .../docker/DefaultDockerClientTest.java | 67 +++++++++++++++++++ .../docker/DockerImageDownloaderTest.java | 57 ++++++++++++++++ 5 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 src/main/java/com/aws/greengrass/componentmanager/plugins/docker/exceptions/UnknownDockerPullException.java create mode 100644 src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java index 8e109c50ce..7521d62d14 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java +++ b/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; @@ -47,6 +48,16 @@ public class DefaultDockerClient { private static final String NO_SUCH_HOST = "no such host"; private static final String DIAL_TCP = "dial tcp"; + /** + * Errors that a retry cannot recover from, so a docker pull failing with one of these should fail fast. + */ + private static final String MANIFEST_UNKNOWN = "manifest unknown"; + private static final String NO_MATCHING_MANIFEST = "no matching manifest for"; + private static final String INVALID_REFERENCE_FORMAT = "invalid reference format"; + private static final String NO_SPACE_LEFT_ON_DEVICE = "no space left on device"; + private static final String AUTHENTICATION_REQUIRED = "authentication required"; + private static final String ACCESS_DENIED = "requested access to the resource is denied"; + /** * Sanity check for installation. * @@ -110,7 +121,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 +148,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 +169,40 @@ 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) { + String lowerCaseErr = err.toLowerCase(); + return err.contains(READ_CONNECTION_TIME_OUT) + || err.contains(TEMPORARY_FAILURE_IN_NAME_RESOLUTION) + || lowerCaseErr.contains(NET_HTTP_TIMEOUT) + || lowerCaseErr.contains(REQUEST_CANCELED) + || lowerCaseErr.contains(DOCKER_PULL_TIMEOUT) + || lowerCaseErr.contains(NO_SUCH_HOST) + || lowerCaseErr.contains(DIAL_TCP); + } + + /** + * 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) { + String lowerCaseErr = err.toLowerCase(); + return lowerCaseErr.contains(MANIFEST_UNKNOWN) + || lowerCaseErr.contains(NO_MATCHING_MANIFEST) + || lowerCaseErr.contains(INVALID_REFERENCE_FORMAT) + || lowerCaseErr.contains(NO_SPACE_LEFT_ON_DEVICE) + || lowerCaseErr.contains(AUTHENTICATION_REQUIRED) + || lowerCaseErr.contains(ACCESS_DENIED); + } + /** * Check if an image exists locally. * diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java index a2a308f1b2..2854f6c109 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java +++ b/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; @@ -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 void run(CrashableSupplier 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, diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/exceptions/UnknownDockerPullException.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/exceptions/UnknownDockerPullException.java new file mode 100644 index 0000000000..fe670487c9 --- /dev/null +++ b/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. + * + *

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); + } +} diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java new file mode 100644 index 0000000000..c3936b1191 --- /dev/null +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java @@ -0,0 +1,67 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.aws.greengrass.componentmanager.plugins.docker; + +import com.aws.greengrass.testcommons.testutilities.GGExtension; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@ExtendWith({GGExtension.class}) +class DefaultDockerClientTest { + + @ParameterizedTest + @ValueSource(strings = { + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": dial tcp: lookup registry-1" + + ".docker.io: no such host", + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" + + ".4:443: connect: connection refused", + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": read tcp 10.0.0" + + ".1:52044->1.2.3.4:443: read: connection timed out", + "Get \"https://registry-1.docker.io/v2/\": net/http: TLS handshake timeout", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": request canceled while waiting " + + "for connection", + "dial tcp: lookup 1234.dkr.ecr.us-east-1.amazonaws.com: Temporary failure in name resolution"}) + void GIVEN_known_network_error_WHEN_classified_THEN_treated_as_connection_error(String err) { + assertTrue(DefaultDockerClient.isConnectionError(err)); + // A network error must never also be claimed as non-retryable, since isConnectionError is evaluated first + // and the two classifications must not disagree + assertFalse(DefaultDockerClient.isNonRetryableError(err)); + } + + @ParameterizedTest + @ValueSource(strings = { + "Error response from daemon: manifest for alpine:doesnotexist not found: manifest unknown: manifest " + + "unknown", + "Error response from daemon: no matching manifest for linux/arm64/v8 in the manifest list entries", + "invalid reference format", + "failed to register layer: write /usr/lib/foo: no space left on device", + "Error response from daemon: Head \"https://registry-1.docker.io/v2/library/alpine/manifests/latest\": " + + "unauthorized: authentication required", + "Error response from daemon: denied: requested access to the resource is denied"}) + void GIVEN_known_non_retryable_error_WHEN_classified_THEN_treated_as_non_retryable(String err) { + assertTrue(DefaultDockerClient.isNonRetryableError(err)); + assertFalse(DefaultDockerClient.isConnectionError(err)); + } + + @ParameterizedTest + @ValueSource(strings = { + // The error reported by a device whose LTE connection failed over mid-pull. Previously this matched + // no known network error string and so failed the deployment without any retry. + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/vapr/manifests/sha256" + + ":d35a4457caa9e9bb60dc03a45b3fd9c0d7d242b06c1b0e36410cb5ed3b594050\": read tcp 172.28.1.230" + + ":52044->34.204.60.241:443: read: connection reset by peer", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": EOF", + "failed to copy: httpReadSeeker: failed open: unexpected status code 503", + "some error string docker has not emitted before"}) + void GIVEN_unrecognized_error_WHEN_classified_THEN_neither_connection_nor_non_retryable(String err) { + assertFalse(DefaultDockerClient.isConnectionError(err)); + assertFalse(DefaultDockerClient.isNonRetryableError(err)); + } +} diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java index 0ca4b01d86..71e27bf320 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java @@ -18,6 +18,7 @@ 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.RegistryAuthException; +import com.aws.greengrass.componentmanager.plugins.docker.exceptions.UnknownDockerPullException; import com.aws.greengrass.componentmanager.plugins.docker.exceptions.UserNotAuthorizedForDockerException; import com.aws.greengrass.mqttclient.MqttClient; import com.aws.greengrass.testcommons.testutilities.GGExtension; @@ -86,6 +87,11 @@ public class DockerImageDownloaderTest { Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class, SdkClientException.class, ServerException.class)).build(); + private final RetryUtils.RetryConfig unknownErrorRetryConfig = + RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(50L)) + .maxRetryInterval(Duration.ofMillis(50L)).maxAttempt(3).retryableExceptions( + Collections.singletonList(UnknownDockerPullException.class)).build(); + private final RetryUtils.RetryConfig finiteDeleteAttemptsRetryConfig = RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(50L)) .maxRetryInterval(Duration.ofMillis(50L)).maxAttempt(3).retryableExceptions( @@ -666,12 +672,63 @@ void GIVEN_a_container_component_with_an_ecr_image_latest_tag_WHEN_already_deplo verify(dockerClient, times(1)).pullImage(image); } + @Test + void GIVEN_unrecognized_pull_error_WHEN_download_docker_image_THEN_retry_a_bounded_number_of_times( + ExtensionContext extensionContext) throws Exception { + ignoreExceptionOfType(extensionContext, UnknownDockerPullException.class); + URI artifactUri = new URI("docker:alpine"); + Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); + when(dockerClient.dockerInstalled()).thenReturn(true); + doThrow(new UnknownDockerPullException("connection reset by peer")).when(dockerClient).pullImage(image); + + DockerImageDownloader downloader = getDownloader(artifactUri); + + assertThrows(PackageDownloadException.class, () -> downloader.download()); + + // Bounded by unknownErrorRetryConfig's maxAttempt rather than failing on the first attempt + verify(dockerClient, times(3)).pullImage(image); + } + + @Test + void GIVEN_unrecognized_pull_error_WHEN_it_recovers_THEN_download_succeeds(ExtensionContext extensionContext) + throws Exception { + ignoreExceptionOfType(extensionContext, UnknownDockerPullException.class); + URI artifactUri = new URI("docker:alpine"); + Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); + when(dockerClient.dockerInstalled()).thenReturn(true); + doThrow(new UnknownDockerPullException("connection reset by peer")).doNothing().when(dockerClient) + .pullImage(image); + + DockerImageDownloader downloader = getDownloader(artifactUri); + + downloader.download(); + + verify(dockerClient, times(2)).pullImage(image); + } + + @Test + void GIVEN_non_retryable_pull_error_WHEN_download_docker_image_THEN_fail_without_retrying( + ExtensionContext extensionContext) throws Exception { + ignoreExceptionOfType(extensionContext, DockerPullException.class); + URI artifactUri = new URI("docker:alpine"); + Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); + when(dockerClient.dockerInstalled()).thenReturn(true); + doThrow(new DockerPullException("manifest unknown")).when(dockerClient).pullImage(image); + + DockerImageDownloader downloader = getDownloader(artifactUri); + + assertThrows(PackageDownloadException.class, () -> downloader.download()); + + verify(dockerClient, times(1)).pullImage(image); + } + private DockerImageDownloader getDownloader(URI artifactUri) { DockerImageDownloader downloader = new DockerImageDownloader(TEST_COMPONENT_ID, ComponentArtifact.builder().artifactUri(artifactUri).build(), artifactDir, dockerClient, ecrAccessor, mqttClient, componentStore); downloader.setInfiniteAttemptsRetryConfig(infiniteAttemptsRetryConfig); downloader.setFiniteAttemptsRetryConfig(finiteAttemptsRetryConfig); + downloader.setUnknownErrorRetryConfig(unknownErrorRetryConfig); downloader.setFiniteDeleteAttemptsRetryConfig(finiteDeleteAttemptsRetryConfig); return downloader; } From f55f3dd657b4b102ede33ed4d2d6c70c6e47cac4 Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Mon, 17 Aug 2026 20:53:20 +0000 Subject: [PATCH 2/8] fix: lower docker download retry backoff ceiling from 64 to 5 minutes The indefinite retry tier for docker registry network errors backs off exponentially from 1 minute to a 64 minute ceiling, so after roughly seven consecutive failures a device waits an hour between attempts. On a device whose connectivity recovers in between, recovery is gated on that interval rather than on the network, and the deployment looks stalled. The other artifact downloaders retry network errors indefinitely at a flat 1 minute (S3Downloader, GreengrassRepositoryDownloader), so this tier is the outlier. Cap it at 5 minutes: exponential backoff is retained, so a sustained outage still costs roughly a twelfth of the requests a flat 1 minute retry would make, while worst-case recovery latency drops from an hour to 5 minutes. --- .../componentmanager/plugins/docker/DockerImageDownloader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java index 2854f6c109..fa47cfdda3 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java +++ b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java @@ -58,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) From b6161b16c3704bedfd781265ba9e325be5e35858 Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Mon, 17 Aug 2026 22:41:08 +0000 Subject: [PATCH 3/8] fix: classify transport-level docker pull errors as network errors A docker pull that fails at the network transport layer is recoverable once connectivity is restored, so it belongs in the indefinite retry tier alongside the errors already classified as ConnectionException. The classification only recognized transport failures that happened before the connection was established, matching "dial tcp" but nothing for a connection that dies mid-transfer, so the error a device reports when its network fails over during a pull was not recognized as a network error at all: read tcp 172.28.1.230:52044->34.204.60.241:443: read: connection reset by peer Match on the Go net.OpError op prefix. Go renders that error as " ->: ", so "read tcp" and "write tcp" classify a failure at the TCP layer regardless of which cause follows, including causes docker has not been observed emitting. This is what makes the classification robust rather than exhaustive: previously each new cause string needed to be enumerated, and the customer-reported error is precisely a cause that was not. Also match transport-level causes on their own, since docker and containerd surface some of them without an op prefix when wrapping an error raised while copying an image layer: connection reset by peer, connection refused, connection aborted, broken pipe, network is unreachable, network is down, host is unreachable, no route to host, i/o timeout, EOF and unexpected EOF, context deadline exceeded, and server misbehaving. The previously matched strings are all retained, so no error that was already retried indefinitely stops being. Two of them were matched case sensitively and are now matched case insensitively like the rest, which only widens what they match. connection refused and no route to host are listed explicitly but change nothing on their own, as Go always prefixes them with dial tcp. Collapse both classifications into lists rather than boolean chains, which keeps the matching in one place as the number of entries grows. Unrecognized errors continue to route to UnknownDockerPullException and its bounded retry, which remains the safety net for a genuinely novel error. This change narrows what reaches it to errors that are not identifiable as transport failures at all. --- .../plugins/docker/DefaultDockerClient.java | 83 ++++++++++++------- .../docker/DefaultDockerClientTest.java | 30 +++++-- 2 files changed, 76 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java index 7521d62d14..5948d12fa6 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java +++ b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java @@ -27,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; @@ -38,25 +40,56 @@ 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. + * + *

Two shapes are matched. Go renders a {@code net.OpError} as + * {@code " ->: "}, 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 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", + // Timeouts and cancellations, which all mean the request did not complete rather than that the registry + // rejected it + "net/http", + "timeout", + "request 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 String MANIFEST_UNKNOWN = "manifest unknown"; - private static final String NO_MATCHING_MANIFEST = "no matching manifest for"; - private static final String INVALID_REFERENCE_FORMAT = "invalid reference format"; - private static final String NO_SPACE_LEFT_ON_DEVICE = "no space left on device"; - private static final String AUTHENTICATION_REQUIRED = "authentication required"; - private static final String ACCESS_DENIED = "requested access to the resource is denied"; + private static final List 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. @@ -177,14 +210,7 @@ public void pullImage(Image image) throws DockerServiceUnavailableException, Inv * @return true if the error is a known network error */ static boolean isConnectionError(String err) { - String lowerCaseErr = err.toLowerCase(); - return err.contains(READ_CONNECTION_TIME_OUT) - || err.contains(TEMPORARY_FAILURE_IN_NAME_RESOLUTION) - || lowerCaseErr.contains(NET_HTTP_TIMEOUT) - || lowerCaseErr.contains(REQUEST_CANCELED) - || lowerCaseErr.contains(DOCKER_PULL_TIMEOUT) - || lowerCaseErr.contains(NO_SUCH_HOST) - || lowerCaseErr.contains(DIAL_TCP); + return containsAny(err, CONNECTION_ERRORS); } /** @@ -194,13 +220,12 @@ static boolean isConnectionError(String err) { * @return true if the error is known to be non-retryable */ static boolean isNonRetryableError(String err) { - String lowerCaseErr = err.toLowerCase(); - return lowerCaseErr.contains(MANIFEST_UNKNOWN) - || lowerCaseErr.contains(NO_MATCHING_MANIFEST) - || lowerCaseErr.contains(INVALID_REFERENCE_FORMAT) - || lowerCaseErr.contains(NO_SPACE_LEFT_ON_DEVICE) - || lowerCaseErr.contains(AUTHENTICATION_REQUIRED) - || lowerCaseErr.contains(ACCESS_DENIED); + return containsAny(err, NON_RETRYABLE_ERRORS); + } + + private static boolean containsAny(String err, List lowerCaseNeedles) { + String lowerCaseErr = err.toLowerCase(Locale.ROOT); + return lowerCaseNeedles.stream().anyMatch(lowerCaseErr::contains); } /** diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java index c3936b1191..d978a3b5b8 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java @@ -18,6 +18,7 @@ class DefaultDockerClientTest { @ParameterizedTest @ValueSource(strings = { + // Connection never established. These matched before this change. "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": dial tcp: lookup registry-1" + ".docker.io: no such host", "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" @@ -27,8 +28,26 @@ class DefaultDockerClientTest { "Get \"https://registry-1.docker.io/v2/\": net/http: TLS handshake timeout", "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": request canceled while waiting " + "for connection", - "dial tcp: lookup 1234.dkr.ecr.us-east-1.amazonaws.com: Temporary failure in name resolution"}) - void GIVEN_known_network_error_WHEN_classified_THEN_treated_as_connection_error(String err) { + "dial tcp: lookup 1234.dkr.ecr.us-east-1.amazonaws.com: Temporary failure in name resolution", + // Established connection dying mid-transfer. The first of these is the error reported by a device whose + // LTE connection failed over during a pull; before this change it matched nothing and so failed the + // deployment on the first attempt. + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/vapr/manifests/sha256" + + ":d35a4457caa9e9bb60dc03a45b3fd9c0d7d242b06c1b0e36410cb5ed3b594050\": read tcp 172.28.1.230" + + ":52044->34.204.60.241:443: read: connection reset by peer", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": write tcp 10.0.0.1:52044->1.2.3" + + ".4:443: write: broken pipe", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": EOF", + "failed to copy: httpReadSeeker: failed open: unexpected EOF", + // Routing lost, typically while an interface is being torn down or brought up + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" + + ".4:443: connect: network is unreachable", + "read tcp 10.0.0.1:52044->1.2.3.4:443: read: no route to host", + // A cause at the TCP layer that docker has not been observed emitting before is still classified by its + // op prefix + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": read tcp 10.0.0.1:52044->1.2.3" + + ".4:443: read: some errno not seen before"}) + void GIVEN_network_error_WHEN_classified_THEN_treated_as_connection_error(String err) { assertTrue(DefaultDockerClient.isConnectionError(err)); // A network error must never also be claimed as non-retryable, since isConnectionError is evaluated first // and the two classifications must not disagree @@ -52,13 +71,8 @@ void GIVEN_known_non_retryable_error_WHEN_classified_THEN_treated_as_non_retryab @ParameterizedTest @ValueSource(strings = { - // The error reported by a device whose LTE connection failed over mid-pull. Previously this matched - // no known network error string and so failed the deployment without any retry. - "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/vapr/manifests/sha256" - + ":d35a4457caa9e9bb60dc03a45b3fd9c0d7d242b06c1b0e36410cb5ed3b594050\": read tcp 172.28.1.230" - + ":52044->34.204.60.241:443: read: connection reset by peer", - "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": EOF", "failed to copy: httpReadSeeker: failed open: unexpected status code 503", + "Error response from daemon: failed to resolve reference: unexpected commit digest", "some error string docker has not emitted before"}) void GIVEN_unrecognized_error_WHEN_classified_THEN_neither_connection_nor_non_retryable(String err) { assertFalse(DefaultDockerClient.isConnectionError(err)); From 2909b887138d649e2f1d18cd400a162692f25350 Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Mon, 17 Aug 2026 23:05:36 +0000 Subject: [PATCH 4/8] fix: match tls, http2 and containerd docker pull transport errors Matching the Go net.OpError op prefix covers any transport failure that reaches us as a net.OpError, whatever cause follows it. It does not cover transport failures raised at a layer that does not produce one, which arrive with no op prefix to match: - crypto/tls errors are not OpErrors, so a connection corrupted or torn down mid-stream reports "remote error: tls: bad record MAC", "tls: use of closed connection", or "remote error: tls: internal error" - HTTP/2 connection and stream failures report "http2: server sent GOAWAY", "http2: client connection lost", or "stream error: stream ID N; INTERNAL_ERROR" - containerd discards the OpError when wrapping a failure while copying an image layer, leaving "read |0: file already closed" Add these, plus "context canceled" alongside the "request canceled" already matched. http2 is matched on its package prefix for the same reason net/http already is; a stream error carries no such prefix and is matched separately. Only wire-level TLS failures are listed. An error reporting that 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, and are noted as such so they are not added later by pattern-matching on the others. Registry 5xx responses are also deliberately absent. A gateway error is retryable but is not a transport failure, and a registry returning it persistently is a real fault that should surface rather than retry forever. Those already reach the bounded retry tier, and reach the indefinite tier when the device is offline. This does not make the classification exhaustive and cannot: the strings are an undocumented contract spanning the docker CLI, dockerd, containerd and the Go standard library, and Go itself deprecated net.Error.Temporary() on the grounds that transient versus permanent is not well defined. These are the transport failures known to be missed today. --- .../plugins/docker/DefaultDockerClient.java | 14 ++++++++++++++ .../plugins/docker/DefaultDockerClientTest.java | 12 ++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java index 5948d12fa6..0779c3f1a8 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java +++ b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClient.java @@ -69,11 +69,25 @@ public class DefaultDockerClient { "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", diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java index d978a3b5b8..8868f40a46 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java @@ -43,6 +43,18 @@ class DefaultDockerClientTest { "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" + ".4:443: connect: network is unreachable", "read tcp 10.0.0.1:52044->1.2.3.4:443: read: no route to host", + // Wire-level TLS failures, which do not arrive as a net.OpError + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": remote error: tls: bad record MAC", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": tls: use of closed connection", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": remote error: tls: internal error", + // HTTP/2 transport failures + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": http2: server sent GOAWAY and " + + "closed the connection", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": http2: client connection lost", + "failed to copy: stream error: stream ID 5; INTERNAL_ERROR; received from peer", + // Transport failures containerd surfaces after discarding the net.OpError + "failed to copy: read |0: file already closed", + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": context canceled", // A cause at the TCP layer that docker has not been observed emitting before is still classified by its // op prefix "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": read tcp 10.0.0.1:52044->1.2.3" From 3f963f0fd08f51f53acb399884b6d85f940e1f5e Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Mon, 17 Aug 2026 23:06:19 +0000 Subject: [PATCH 5/8] fix: retry unrecognized docker pull errors indefinitely when device is offline Classifying a docker pull failure depends on substring-matching docker's stderr, which is an undocumented contract spanning the docker CLI, dockerd, containerd and the Go standard library. No list of strings can be exhaustive, so a transport failure worded in a way not matched still lands in the unrecognized bucket and gives up after a bounded number of attempts, even though it would have recovered once connectivity returned. Use the device's own connectivity as a signal that does not depend on the error text at all. runWithConnectionErrorCheck already infers this for DockerServiceUnavailableException, which is likewise ambiguous about whether a retry can help; extend the same inference to UnknownDockerPullException. An unrecognized pull failure on a device whose MQTT connection is down is far more likely to be that outage than anything the registry reported, so it becomes a ConnectionException and is retried until connectivity returns. The bound still applies when the device is online, which is what keeps this from reintroducing the risk the bounded tier exists to avoid: an error that is genuinely permanent surfaces after a bounded delay on a healthy device rather than hanging a deployment that has no timeout. Two limits on how much this can be relied on, neither of which makes it worse than the status quo: - MQTT reachability does not prove registry reachability. They are different endpoints and can differ under a proxy or split routing, so an online device can still fail a pull for connectivity reasons and get only the bounded retry. - Detection lags the network. Keep-alive defaults to 60s with a 30s ping timeout, so getMqttOnline can report online for up to about 90s after connectivity is lost. A pull failing inside that window is not attributed to the outage. This is therefore a second line of defence behind the string matching rather than a replacement for it. --- .../plugins/docker/DockerImageDownloader.java | 10 ++++++- .../docker/DockerImageDownloaderTest.java | 29 ++++++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java index fa47cfdda3..2ac5117ceb 100644 --- a/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java +++ b/src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java @@ -321,7 +321,15 @@ private T runWithConnectionErrorCheck(CrashableSupplier 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; diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java index 71e27bf320..e397597f28 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java @@ -685,10 +685,37 @@ void GIVEN_unrecognized_pull_error_WHEN_download_docker_image_THEN_retry_a_bound assertThrows(PackageDownloadException.class, () -> downloader.download()); - // Bounded by unknownErrorRetryConfig's maxAttempt rather than failing on the first attempt + // The device is online per the default setup, so the error is not attributable to connectivity and the + // bounded unknownErrorRetryConfig applies rather than failing on the first attempt verify(dockerClient, times(3)).pullImage(image); } + @Test + void GIVEN_unrecognized_pull_error_and_device_offline_WHEN_connectivity_is_back_THEN_retry_and_succeed( + ExtensionContext extensionContext) throws Exception { + ignoreExceptionOfType(extensionContext, UnknownDockerPullException.class); + ignoreExceptionOfType(extensionContext, ConnectionException.class); + URI artifactUri = new URI("docker:alpine"); + Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); + when(mqttClient.getMqttOnline()).thenReturn(new AtomicBoolean(false)); + when(dockerClient.dockerInstalled()).thenReturn(true); + // Four failures exceeds unknownErrorRetryConfig's bound of 3 attempts, so succeeding on the fifth is only + // reachable if the offline device caused the error to be treated as a connection error and retried by + // infiniteAttemptsRetryConfig instead + doThrow(new UnknownDockerPullException("some error docker has not emitted before")) + .doThrow(new UnknownDockerPullException("some error docker has not emitted before")) + .doThrow(new UnknownDockerPullException("some error docker has not emitted before")) + .doThrow(new UnknownDockerPullException("some error docker has not emitted before")).doNothing() + .when(dockerClient).pullImage(image); + + DockerImageDownloader downloader = getDownloader(artifactUri); + + downloader.download(); + + verify(dockerClient, times(5)).pullImage(image); + verify(mqttClient, times(4)).getMqttOnline(); + } + @Test void GIVEN_unrecognized_pull_error_WHEN_it_recovers_THEN_download_succeeds(ExtensionContext extensionContext) throws Exception { From 5659a7a92a015bc8ccaa20f398ec3a3b866fd245 Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Tue, 18 Aug 2026 00:12:21 +0000 Subject: [PATCH 6/8] fix: back off each differentiated retry config independently runWithRetry(DifferentiatedRetryConfig) tracked one retryInterval shared by every config in the list, and slept on it before clamping to the matched config's ceiling. A config with a high ceiling could therefore ramp the interval and leave a config with a much lower ceiling sleeping far past its own. Both existing callers hid this. DeploymentDocumentDownloader pairs two configs that are both flat at 1 minute, and the single-config runWithRetry overload delegates through fromRetryConfig, where one config cannot interfere with another. Pairing configs whose ceilings differ, as the docker downloader now does with 32 minutes and 2 minutes, makes the behaviour reachable: several DockerServiceUnavailableException failures ramp the interval toward 32 minutes, and an UnknownDockerPullException arriving afterwards sleeps for up to that long despite its own 2 minute ceiling. Track the interval per config, initialized from each config's own initialRetryInterval. This also removes the lazy zero-initialization, which previously meant a config's initialRetryInterval only applied when its exception happened to be the first failure. Behaviour is unchanged for a single config and for two configs with equal intervals, so neither existing caller is affected. The new test pairs a config ramping 200ms to 30s with one fixed at 10ms and asserts total elapsed time. It fails at 6178ms against the shared interval and passes at 2275ms with the fix. --- .../com/aws/greengrass/util/RetryUtils.java | 17 ++++++--- .../aws/greengrass/util/RetryUtilsTest.java | 38 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/aws/greengrass/util/RetryUtils.java b/src/main/java/com/aws/greengrass/util/RetryUtils.java index 63ae1fe30d..434995b7e2 100644 --- a/src/main/java/com/aws/greengrass/util/RetryUtils.java +++ b/src/main/java/com/aws/greengrass/util/RetryUtils.java @@ -61,12 +61,18 @@ public static T runWithRetry(RetryConfig retryConfig, CrashableSupplier T runWithRetry(DifferentiatedRetryConfig differentiatedRetryConfig, CrashableSupplier task, String taskDescription, Logger logger) throws Exception { - long retryInterval = 0; long totalAttempts = 0; long totalMaxAttempts = calculateTotalMaxAttempts(differentiatedRetryConfig); Map 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 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 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; diff --git a/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java b/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java index eb9ca893f6..91543f48cf 100644 --- a/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java +++ b/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java @@ -18,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; class RetryUtilsTest { @@ -79,4 +80,41 @@ void GIVEN_differentiatedRetryConfig_WHEN_runWithRetry_THEN_retryDifferently() { }, "", logger)); assertEquals(4, invoked.get()); } + + @Test + void GIVEN_configs_with_different_ceilings_WHEN_run_with_retry_THEN_each_backs_off_independently() + throws Exception { + AtomicInteger invoked = new AtomicInteger(0); + List configList = new ArrayList<>(); + + // Ramps towards a high ceiling + configList.add(RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(200)) + .maxRetryInterval(Duration.ofSeconds(30)).maxAttempt(5).retryableExceptions( + Collections.singletonList(IOException.class)).build()); + + // Must stay at its own low ceiling regardless of how far the other config has ramped + configList.add(RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(10)) + .maxRetryInterval(Duration.ofMillis(10)).maxAttempt(2).retryableExceptions( + Collections.singletonList(RuntimeException.class)).build()); + + RetryUtils.DifferentiatedRetryConfig config = RetryUtils.DifferentiatedRetryConfig.builder() + .retryConfigList(configList).build(); + + long start = System.currentTimeMillis(); + assertThrows(RuntimeException.class, () -> RetryUtils.runWithRetry(config, () -> { + // Let the IOException config ramp its interval to 200/400/800/1600ms, then throw the exception whose + // config caps at 10ms. Sharing one interval across configs would make that final sleep ~3200ms. + if (invoked.getAndIncrement() < 4) { + throw new IOException(); + } + throw new RuntimeException(); + }, "", logger)); + long elapsed = System.currentTimeMillis() - start; + + assertEquals(6, invoked.get()); + // Four IOException sleeps are jittered at 50-100% of 200/400/800/1600ms, so at most 3000ms. The + // RuntimeException config allows one retry, whose sleep must come from its own 10ms interval rather than the + // 3200ms the other config ramped to. + assertTrue(elapsed < 3100, "elapsed " + elapsed + "ms indicates the ramped interval leaked across configs"); + } } From 845174d13a800a16532d20f618b72be845c99319 Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Tue, 18 Aug 2026 00:12:33 +0000 Subject: [PATCH 7/8] test: cover remaining connection error strings and fix misleading fixtures Two DockerImageDownloaderTest cases built an UnknownDockerPullException whose message was "connection reset by peer". That string is in CONNECTION_ERRORS, so DefaultDockerClient can never produce an UnknownDockerPullException carrying it, and the fixture implied a combination that cannot occur. Use the same unrecognized message the third case already used. Add classification cases for the connection error strings that had none: network is down, host is unreachable, connection aborted, context deadline exceeded, and server misbehaving. --- .../plugins/docker/DefaultDockerClientTest.java | 7 +++++++ .../plugins/docker/DockerImageDownloaderTest.java | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java index 8868f40a46..10b4c2c71c 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DefaultDockerClientTest.java @@ -43,6 +43,13 @@ class DefaultDockerClientTest { "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" + ".4:443: connect: network is unreachable", "read tcp 10.0.0.1:52044->1.2.3.4:443: read: no route to host", + "read tcp 10.0.0.1:52044->1.2.3.4:443: read: network is down", + "Error response from daemon: Get \"https://1234.dkr.ecr.us-east-1.amazonaws.com/v2/\": dial tcp 1.2.3" + + ".4:443: connect: host is unreachable", + "read tcp 10.0.0.1:52044->1.2.3.4:443: read: software caused connection aborted", + // Cancellations and DNS server faults + "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": context deadline exceeded", + "dial tcp: lookup 1234.dkr.ecr.us-east-1.amazonaws.com on 10.0.0.53:53: server misbehaving", // Wire-level TLS failures, which do not arrive as a net.OpError "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": remote error: tls: bad record MAC", "Error response from daemon: Get \"https://registry-1.docker.io/v2/\": tls: use of closed connection", diff --git a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java index e397597f28..e7aec997be 100644 --- a/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java +++ b/src/test/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java @@ -679,7 +679,7 @@ void GIVEN_unrecognized_pull_error_WHEN_download_docker_image_THEN_retry_a_bound URI artifactUri = new URI("docker:alpine"); Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); when(dockerClient.dockerInstalled()).thenReturn(true); - doThrow(new UnknownDockerPullException("connection reset by peer")).when(dockerClient).pullImage(image); + doThrow(new UnknownDockerPullException("some error docker has not emitted before")).when(dockerClient).pullImage(image); DockerImageDownloader downloader = getDownloader(artifactUri); @@ -723,7 +723,7 @@ void GIVEN_unrecognized_pull_error_WHEN_it_recovers_THEN_download_succeeds(Exten URI artifactUri = new URI("docker:alpine"); Image image = Image.fromArtifactUri(ComponentArtifact.builder().artifactUri(artifactUri).build()); when(dockerClient.dockerInstalled()).thenReturn(true); - doThrow(new UnknownDockerPullException("connection reset by peer")).doNothing().when(dockerClient) + doThrow(new UnknownDockerPullException("some error docker has not emitted before")).doNothing().when(dockerClient) .pullImage(image); DockerImageDownloader downloader = getDownloader(artifactUri); From db427a5a74a65b247bfb3ef595afb70b88cde21d Mon Sep 17 00:00:00 2001 From: Kevin Rickard Date: Tue, 18 Aug 2026 00:25:19 +0000 Subject: [PATCH 8/8] test: widen the separation margin in the differentiated backoff test The test asserted elapsed time under 3100ms, against a maximum of 3010ms for the fixed code, leaving 90ms of slack across five Thread.sleep calls. Thread.sleep guarantees only a minimum, so overshoot on a loaded machine could fail the assertion spuriously. The gap between the two cases works out to half the initial interval, because the leaked sleep is at least the ramped interval's half while the noise is the jitter spread of the sleeps preceding it. Sleeps late in a ramp dominate that spread, so one sleep on a large interval separates the cases far better than several on small ones. Ramp with a single 2s interval instead of four from 200ms. The fixed code now takes at most 2010ms and the shared interval at least 3000ms, so a 2500ms threshold leaves roughly 490ms either side. The test also runs faster, measuring 1932ms rather than 2275ms, and still fails against the shared interval, measuring 5641ms. --- .../aws/greengrass/util/RetryUtilsTest.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java b/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java index 91543f48cf..cab9514b72 100644 --- a/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java +++ b/src/test/java/com/aws/greengrass/util/RetryUtilsTest.java @@ -87,12 +87,12 @@ void GIVEN_configs_with_different_ceilings_WHEN_run_with_retry_THEN_each_backs_o AtomicInteger invoked = new AtomicInteger(0); List configList = new ArrayList<>(); - // Ramps towards a high ceiling - configList.add(RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(200)) - .maxRetryInterval(Duration.ofSeconds(30)).maxAttempt(5).retryableExceptions( + // Ramps its interval to 4s after one failure + configList.add(RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(2)) + .maxRetryInterval(Duration.ofSeconds(30)).maxAttempt(2).retryableExceptions( Collections.singletonList(IOException.class)).build()); - // Must stay at its own low ceiling regardless of how far the other config has ramped + // Must stay at its own ceiling regardless of how far the other config has ramped configList.add(RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(10)) .maxRetryInterval(Duration.ofMillis(10)).maxAttempt(2).retryableExceptions( Collections.singletonList(RuntimeException.class)).build()); @@ -102,19 +102,20 @@ void GIVEN_configs_with_different_ceilings_WHEN_run_with_retry_THEN_each_backs_o long start = System.currentTimeMillis(); assertThrows(RuntimeException.class, () -> RetryUtils.runWithRetry(config, () -> { - // Let the IOException config ramp its interval to 200/400/800/1600ms, then throw the exception whose - // config caps at 10ms. Sharing one interval across configs would make that final sleep ~3200ms. - if (invoked.getAndIncrement() < 4) { + // Let the IOException config ramp its interval to 4s, then throw the exception whose config caps at 10ms + if (invoked.getAndIncrement() < 1) { throw new IOException(); } throw new RuntimeException(); }, "", logger)); long elapsed = System.currentTimeMillis() - start; - assertEquals(6, invoked.get()); - // Four IOException sleeps are jittered at 50-100% of 200/400/800/1600ms, so at most 3000ms. The - // RuntimeException config allows one retry, whose sleep must come from its own 10ms interval rather than the - // 3200ms the other config ramped to. - assertTrue(elapsed < 3100, "elapsed " + elapsed + "ms indicates the ramped interval leaked across configs"); + assertEquals(3, invoked.get()); + // Sleeps are jittered at 50-100% of the interval. One IOException sleep on a 2s interval takes 1000-2000ms, + // then the single RuntimeException retry sleeps 5-10ms from its own interval, so at most 2010ms. Sharing one + // interval across configs would instead sleep 2000-4000ms from the ramped 4s interval, so at least 3000ms. + // A single 2s interval rather than a ramp over several separates the two cases by ~490ms either side of the + // threshold, well beyond any plausible Thread.sleep overshoot. + assertTrue(elapsed < 2500, "elapsed " + elapsed + "ms indicates the ramped interval leaked across configs"); } }