Replenish cross-request retry allowance on successful response (#1197)

* Replenish cross-request retry allowance on successful response

* Update changelog

* Rename `NotRetryable` to `UnretryableFailure` and fix credential retry classifiers

* Incorporate feedback

CHANGELOG.next.toml

@@ -11,6 +11,24 @@
 # meta = { "breaking" = false, "tada" = false, "bug" = false }
 # author = "rcoh"
 
+[[aws-sdk-rust]]
+message = "Fixed a bug that caused clients to eventually stop retrying. The cross-request retry allowance wasn't being reimbursed upon receiving a successful response, so once this allowance reached zero, no further retries would ever be attempted."
+references = ["smithy-rs#1197"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "Fixed a bug that caused clients to eventually stop retrying. The cross-request retry allowance wasn't being reimbursed upon receiving a successful response, so once this allowance reached zero, no further retries would ever be attempted."
+references = ["smithy-rs#1197"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "`aws_smithy_types::retry::RetryKind` had its `NotRetryable` variant split into `UnretryableFailure` and `Unnecessary`. If you implement the `ClassifyResponse`, then successful responses need to return `Unnecessary`, and failures that shouldn't be retried need to return `UnretryableFailure`."
+references = ["smithy-rs#1197"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
 [[smithy-rs]]
 message = "`aws_smithy_types::primitive::Encoder` is now a struct rather than an enum, but its usage remains the same."
 references = ["smithy-rs#1209"]

aws/rust-runtime/aws-config/src/http_credential_provider.rs

@@ -174,7 +174,7 @@ impl ClassifyResponse<SdkSuccess<Credentials>, SdkError<CredentialsError>>
          *   - Non-parseable 200 responses.
          *  */
         match response {
-            Ok(_) => RetryKind::NotRetryable,
+            Ok(_) => RetryKind::Unnecessary,
             // socket errors, networking timeouts
             Err(SdkError::DispatchFailure(client_err))
                 if client_err.is_timeout() || client_err.is_io() =>
@@ -192,7 +192,7 @@ impl ClassifyResponse<SdkSuccess<Credentials>, SdkError<CredentialsError>>
             {
                 RetryKind::Error(ErrorKind::ServerError)
             }
-            Err(_) => RetryKind::NotRetryable,
+            Err(_) => RetryKind::UnretryableFailure,
         }
     }
 }
@@ -260,7 +260,7 @@ mod test {
 
         assert_eq!(
             HttpCredentialRetryPolicy.classify(sdk_result.as_ref()),
-            RetryKind::NotRetryable
+            RetryKind::Unnecessary
         );
 
         assert!(sdk_result.is_ok(), "should be ok: {:?}", sdk_result)
@@ -275,7 +275,7 @@ mod test {
         let sdk_result = sdk_resp(error_response);
         assert_eq!(
             HttpCredentialRetryPolicy.classify(sdk_result.as_ref()),
-            RetryKind::NotRetryable
+            RetryKind::UnretryableFailure
         );
         let sdk_error = sdk_result.expect_err("should be error");
 

aws/rust-runtime/aws-config/src/imds/client.rs

@@ -693,7 +693,8 @@ impl ImdsErrorPolicy {
             _ if status.is_server_error() => RetryKind::Error(ErrorKind::ServerError),
             // 401 indicates that the token has expired, this is retryable
             _ if status.as_u16() == 401 => RetryKind::Error(ErrorKind::ServerError),
-            _ => RetryKind::NotRetryable,
+            // This catch-all includes successful responses that fail to parse. These should not be retried.
+            _ => RetryKind::UnretryableFailure,
         }
     }
 }
@@ -710,11 +711,11 @@ impl ImdsErrorPolicy {
 impl<T, E> ClassifyResponse<SdkSuccess<T>, SdkError<E>> for ImdsErrorPolicy {
     fn classify(&self, response: Result<&SdkSuccess<T>, &SdkError<E>>) -> RetryKind {
         match response {
-            Ok(_) => RetryKind::NotRetryable,
+            Ok(_) => RetryKind::Unnecessary,
             Err(SdkError::ResponseError { raw, .. }) | Err(SdkError::ServiceError { raw, .. }) => {
                 ImdsErrorPolicy::classify(raw)
             }
-            _ => RetryKind::NotRetryable,
+            _ => RetryKind::UnretryableFailure,
         }
     }
 }
@@ -723,20 +724,24 @@ impl<T, E> ClassifyResponse<SdkSuccess<T>, SdkError<E>> for ImdsErrorPolicy {
 pub(crate) mod test {
     use std::collections::HashMap;
     use std::error::Error;
+    use std::io;
     use std::time::{Duration, UNIX_EPOCH};
 
     use aws_smithy_async::rt::sleep::TokioSleep;
     use aws_smithy_client::erase::DynConnector;
     use aws_smithy_client::test_connection::{capture_request, TestConnection};
+    use aws_smithy_client::{SdkError, SdkSuccess};
     use aws_smithy_http::body::SdkBody;
+    use aws_smithy_http::operation;
+    use aws_smithy_types::retry::RetryKind;
     use aws_types::os_shim_internal::{Env, Fs, ManualTimeSource, TimeSource};
+    use http::header::USER_AGENT;
     use http::Uri;
     use serde::Deserialize;
     use tracing_test::traced_test;
 
-    use crate::imds::client::{Client, EndpointMode};
+    use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy};
     use crate::provider_config::ProviderConfig;
-    use http::header::USER_AGENT;
 
     const TOKEN_A: &str = "AQAEAFTNrA4eEGx0AQgJ1arIq_Cc-t4tWt3fB0Hd8RKhXlKc5ccvhg==";
     const TOKEN_B: &str = "alternatetoken==";
@@ -977,6 +982,35 @@ pub(crate) mod test {
         connection.assert_requests_match(&[]);
     }
 
+    /// Successful responses should classify as `RetryKind::Unnecessary`
+    #[test]
+    fn successful_response_properly_classified() {
+        use aws_smithy_http::retry::ClassifyResponse;
+
+        let policy = ImdsErrorPolicy;
+        fn response_200() -> operation::Response {
+            operation::Response::new(imds_response("").map(|_| SdkBody::empty()))
+        }
+        let success = SdkSuccess {
+            raw: response_200(),
+            parsed: (),
+        };
+        assert_eq!(
+            RetryKind::Unnecessary,
+            policy.classify(Ok::<_, &SdkError<()>>(&success))
+        );
+
+        // Emulate a failure to parse the response body (using an io error since it's easy to construct in a test)
+        let failure = SdkError::<()>::ResponseError {
+            err: Box::new(io::Error::new(io::ErrorKind::BrokenPipe, "fail to parse")),
+            raw: response_200(),
+        };
+        assert_eq!(
+            RetryKind::UnretryableFailure,
+            policy.classify(Err::<&SdkSuccess<()>, _>(&failure))
+        );
+    }
+
     // since tokens are sent as headers, the tokens need to be valid header values
     #[tokio::test]
     async fn invalid_token() {

aws/rust-runtime/aws-http/src/retry.rs

@@ -58,18 +58,18 @@ where
 {
     fn classify(&self, err: Result<&T, &SdkError<E>>) -> RetryKind {
         let (err, response) = match err {
-            Ok(_) => return RetryKind::NotRetryable,
+            Ok(_) => return RetryKind::Unnecessary,
             Err(SdkError::ServiceError { err, raw }) => (err, raw),
             Err(SdkError::DispatchFailure(err)) => {
                 return if err.is_timeout() || err.is_io() {
                     RetryKind::Error(ErrorKind::TransientError)
                 } else if let Some(ek) = err.is_other() {
                     RetryKind::Error(ek)
                 } else {
-                    RetryKind::NotRetryable
+                    RetryKind::UnretryableFailure
                 }
             }
-            Err(_) => return RetryKind::NotRetryable,
+            Err(_) => return RetryKind::UnretryableFailure,
         };
         if let Some(retry_after_delay) = response
             .http()
@@ -95,7 +95,7 @@ where
             return RetryKind::Error(ErrorKind::TransientError);
         };
         // TODO(https://github.com/awslabs/smithy-rs/issues/966): IDPCommuncation error needs to be retried
-        RetryKind::NotRetryable
+        RetryKind::UnretryableFailure
     }
 }
 
@@ -151,7 +151,7 @@ mod test {
         let test_response = http::Response::new("OK");
         assert_eq!(
             policy.classify(make_err(UnmodeledError, test_response).as_ref()),
-            RetryKind::NotRetryable
+            RetryKind::UnretryableFailure
         );
     }
 
@@ -177,7 +177,7 @@ mod test {
             .unwrap();
         assert_eq!(
             policy.classify(make_err(UnmodeledError, test_resp).as_ref()),
-            RetryKind::NotRetryable
+            RetryKind::UnretryableFailure
         );
     }
 

aws/sdk/integration-tests/dynamodb/tests/movies.rs

@@ -125,8 +125,8 @@ where
         &self,
         response: Result<&SdkSuccess<DescribeTableOutput>, &SdkError<DescribeTableError>>,
     ) -> RetryKind {
-        match self.inner.classify(response.clone()) {
-            RetryKind::NotRetryable => (),
+        match self.inner.classify(response) {
+            RetryKind::UnretryableFailure | RetryKind::Unnecessary => (),
             other => return other,
         };
         match response {
@@ -142,10 +142,10 @@ where
                 {
                     RetryKind::Explicit(Duration::from_secs(1))
                 } else {
-                    RetryKind::NotRetryable
+                    RetryKind::Unnecessary
                 }
             }
-            _ => RetryKind::NotRetryable,
+            _ => RetryKind::UnretryableFailure,
         }
     }
 }