Classify 4xx (except 429) as terminal in RestSend retry policy

plugins/module_utils/rest/response_handler_nd.py

@@ -170,6 +170,25 @@ def _handle_response(self) -> None:
         else:
             self._handle_post_put_delete_response()
 
+    def _is_terminal_client_error(self, return_code) -> bool:
+        """
+        # Summary
+
+        Return True when `return_code` is a 4xx client error other than 429 (Too Many Requests).
+
+        ## Description
+
+        A 4xx response proves the request reached the application and was rejected: replaying the identical request
+        cannot succeed, so the failure is terminal for every verb. 429 is the one transient 4xx (rate limiting) and
+        stays retryable. No retryable 4xx is documented for any ND 4.2.1 endpoint (the dcnm-era retry-on-400 cases
+        do not carry over). See issue #457.
+
+        ## Raises
+
+        None
+        """
+        return isinstance(return_code, int) and 400 <= return_code <= 499 and return_code != 429
+
     def _handle_get_response(self) -> None:
         """
         # Summary
@@ -183,6 +202,11 @@ def _handle_get_response(self) -> None:
             -   success:
                     -   True if RETURN_CODE in (200, 201, 202, 204, 207, 404)
                     -   False otherwise (error status codes)
+            -   retryable:
+                    -   False when the request succeeded, or when it failed with a 4xx code other than 429 (the
+                        request reached the application and was rejected; an identical replay cannot succeed)
+                    -   True when it failed with any other code (e.g. 5xx — potentially transient, and GET retries
+                        also serve eventual-consistency polling)
         """
         result = {}
         return_code = self.response.get("RETURN_CODE")
@@ -191,14 +215,17 @@ def _handle_get_response(self) -> None:
         if self._strategy.is_not_found(return_code):
             result["found"] = False
             result["success"] = True
+            result["retryable"] = False
         # Success codes with no embedded error - resource found
         elif self._strategy.is_success(self.response):
             result["found"] = True
             result["success"] = True
+            result["retryable"] = False
         # Error codes - request failed
         else:
             result["found"] = False
             result["success"] = False
+            result["retryable"] = not self._is_terminal_client_error(return_code)
 
         self.result = copy.copy(result)
 
@@ -219,8 +246,10 @@ def _handle_post_put_delete_response(self) -> None:
             -   `retryable`:
                 -   False when the request succeeded, or when it failed with a success-class RETURN_CODE (the application
                     definitively rejected the request, e.g. a Multi-Status per-item failure — replaying the identical
-                    payload cannot succeed, so `RestSend` must not retry)
-                -   True when the request failed with a non-success RETURN_CODE (e.g. 5xx — potentially transient)
+                    payload cannot succeed, so `RestSend` must not retry), or when it failed with a 4xx code other than
+                    429 (the request reached the application and was rejected; same reasoning — see issue #457)
+                -   True when the request failed with any other non-success RETURN_CODE (e.g. 5xx — potentially
+                    transient) or with 429 (rate limiting)
 
         ## Raises
 
@@ -237,12 +266,13 @@ def _handle_post_put_delete_response(self) -> None:
             result["retryable"] = False
         else:
             # A failure on a success-class RETURN_CODE is an application-level rejection
-            # (embedded error or per-item failure): deterministic, so not retryable.
-            # A failure on a non-success RETURN_CODE keeps the historical retry behavior.
+            # (embedded error or per-item failure): deterministic, so not retryable. A 4xx
+            # other than 429 is equally deterministic — the application rejected the request
+            # (issue #457). Any other non-success RETURN_CODE keeps the historical retry behavior.
             return_code = self.response.get("RETURN_CODE", -1)
             result["success"] = False
             result["changed"] = self._strategy.is_changed_on_failure(self.response)
-            result["retryable"] = return_code not in self._strategy.success_codes
+            result["retryable"] = return_code not in self._strategy.success_codes and not self._is_terminal_client_error(return_code)
 
         self.result = copy.copy(result)
 

tests/unit/module_utils/test_response_handler_nd.py

@@ -2310,12 +2310,13 @@ def test_response_handler_nd_01440():
     """
     # Summary
 
-    Verify GET results carry no retryable key (GET retry semantics are unchanged).
+    Verify a GET failing with a 5xx code remains retryable (GET retries serve eventual-consistency polling and 5xx is
+    potentially transient; only 4xx-except-429 is terminal — see issue #457).
 
     ## Test
 
     - GET returns 500
-    - result has no "retryable" key, so RestSend's .get("retryable", True) default preserves today's GET retry behavior
+    - success is False and retryable is True
 
     ## Classes and Methods
 
@@ -2332,7 +2333,7 @@ def test_response_handler_nd_01440():
     with does_not_raise():
         instance.commit()
     assert instance.result["success"] is False
-    assert "retryable" not in instance.result
+    assert instance.result["retryable"] is True
 
 
 def test_response_handler_nd_01450():
@@ -2495,3 +2496,163 @@ def test_response_handler_nd_01490():
         instance.commit()
     assert instance.result["success"] is False
     assert instance.result["changed"] is False
+
+
+def test_response_handler_nd_01500():
+    """
+    # Summary
+
+    Verify a POST failing with a 4xx code is terminal (retryable=False): the request reached the application and was
+    rejected, so an identical replay cannot succeed (issue #457 — deterministic 400s previously burned the full retry
+    budget).
+
+    ## Test
+
+    - POST returns 400
+    - success is False, retryable is False, changed is False
+
+    ## Classes and Methods
+
+    - ResponseHandler._is_terminal_client_error()
+    - ResponseHandler._handle_post_put_delete_response()
+    - ResponseHandler.commit()
+    """
+    instance = ResponseHandler()
+    instance.response = {
+        "RETURN_CODE": 400,
+        "MESSAGE": "Bad Request",
+        "DATA": {"error": "Invalid fabric settings"},
+    }
+    instance.verb = HttpVerbEnum.POST
+    with does_not_raise():
+        instance.commit()
+    assert instance.result["success"] is False
+    assert instance.result["retryable"] is False
+    assert instance.result["changed"] is False
+
+
+def test_response_handler_nd_01510():
+    """
+    # Summary
+
+    Verify a POST failing with 429 (Too Many Requests) remains retryable: rate limiting is the one transient 4xx, so
+    it is excluded from the 4xx-terminal rule (issue #457).
+
+    ## Test
+
+    - POST returns 429
+    - success is False and retryable is True
+
+    ## Classes and Methods
+
+    - ResponseHandler._is_terminal_client_error()
+    - ResponseHandler._handle_post_put_delete_response()
+    - ResponseHandler.commit()
+    """
+    instance = ResponseHandler()
+    instance.response = {
+        "RETURN_CODE": 429,
+        "MESSAGE": "Too Many Requests",
+        "DATA": {},
+    }
+    instance.verb = HttpVerbEnum.POST
+    with does_not_raise():
+        instance.commit()
+    assert instance.result["success"] is False
+    assert instance.result["retryable"] is True
+
+
+def test_response_handler_nd_01520():
+    """
+    # Summary
+
+    Verify a GET failing with a 4xx code is terminal (retryable=False): a deterministic client error on a GET (bad
+    query parameter, malformed path segment) previously burned the full retry budget just like a mutation (issue
+    #457 scope note — the 4xx-terminal rule applies to all verbs).
+
+    ## Test
+
+    - GET returns 400
+    - success is False, found is False, retryable is False
+
+    ## Classes and Methods
+
+    - ResponseHandler._is_terminal_client_error()
+    - ResponseHandler._handle_get_response()
+    - ResponseHandler.commit()
+    """
+    instance = ResponseHandler()
+    instance.response = {
+        "RETURN_CODE": 400,
+        "MESSAGE": "Bad Request",
+        "DATA": {"error": "invalid query parameter"},
+    }
+    instance.verb = HttpVerbEnum.GET
+    with does_not_raise():
+        instance.commit()
+    assert instance.result["success"] is False
+    assert instance.result["found"] is False
+    assert instance.result["retryable"] is False
+
+
+def test_response_handler_nd_01530():
+    """
+    # Summary
+
+    Verify a GET returning 404 keeps its not-found-is-success contract with retryable=False (the key is now present
+    on every GET result for a consistent shape; a successful result never re-enters the retry loop).
+
+    ## Test
+
+    - GET returns 404
+    - success is True, found is False, retryable is False
+
+    ## Classes and Methods
+
+    - ResponseHandler._handle_get_response()
+    - ResponseHandler.commit()
+    """
+    instance = ResponseHandler()
+    instance.response = {
+        "RETURN_CODE": 404,
+        "MESSAGE": "Not Found",
+        "DATA": {},
+    }
+    instance.verb = HttpVerbEnum.GET
+    with does_not_raise():
+        instance.commit()
+    assert instance.result["success"] is True
+    assert instance.result["found"] is False
+    assert instance.result["retryable"] is False
+
+
+def test_response_handler_nd_01540():
+    """
+    # Summary
+
+    Verify a DELETE failing with 404 is terminal (retryable=False): only GET treats 404 as not-found-success; for a
+    mutation it is a client error, and replaying an identical DELETE against a missing resource cannot succeed
+    (issue #457 — orchestrators with bounded domain-level retries, e.g. L3Out attach, own that pacing themselves).
+
+    ## Test
+
+    - DELETE returns 404
+    - success is False and retryable is False
+
+    ## Classes and Methods
+
+    - ResponseHandler._is_terminal_client_error()
+    - ResponseHandler._handle_post_put_delete_response()
+    - ResponseHandler.commit()
+    """
+    instance = ResponseHandler()
+    instance.response = {
+        "RETURN_CODE": 404,
+        "MESSAGE": "Not Found",
+        "DATA": {"error": "resource does not exist"},
+    }
+    instance.verb = HttpVerbEnum.DELETE
+    with does_not_raise():
+        instance.commit()
+    assert instance.result["success"] is False
+    assert instance.result["retryable"] is False