Move transient-error classification into the SDK; simplify retry loop

Address review feedback on the upload retry:

- The retry decision now delegates to APIFailure.is_transient_error()
  (socketdev>=3.3.0, SocketDev/socket-sdk-python#93), which classifies
  by the HTTP status code the SDK records when raising. The CLI no
  longer encodes the SDK's exception hierarchy or parses status codes
  out of message text, so SDK restructuring can't silently break the
  classification.
- The backoff schedule is now the single source of truth for the loop:
  FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS = (10.0, 30.0, None), where
  each entry is the wait before the next attempt and the final None
  re-raises instead of retrying. FULL_SCAN_UPLOAD_MAX_ATTEMPTS is
  computed from its length.

Note: uv.lock is intentionally not regenerated yet - socketdev 3.3.0
must be released to PyPI first (blocked on socket-sdk-python#93).

Author: mtorp

CHANGELOG.md

@@ -10,11 +10,14 @@
   jitter). Such failures are intermittent and a retried upload almost always succeeds.
   In these failure modes the server never finished reading the request body, so no scan
   was created and a retry does not duplicate one; in the rare case where a gateway
-  timeout races a request the server later
-  completes, the extra scan is benign and superseded by the retried one (as if the CLI had
-  run twice).
+  timeout races a request the server later completes, the extra scan is benign and
+  superseded by the retried one (as if the CLI had run twice).
   Non-transient errors (400/401/403/404/429 and error payloads) are never retried. Each
   retry logs a warning explaining what failed and when the next attempt happens.
+- Requires `socketdev>=3.3.0`: the SDK now records the HTTP status code on the exceptions
+  it raises and owns the transient-vs-deterministic classification
+  (`APIFailure.is_transient_error()`), so the CLI no longer parses status codes out of
+  exception message text.
 
 ## 2.4.7
 

pyproject.toml

@@ -16,7 +16,7 @@ dependencies = [
     'GitPython',
     'packaging',
     'python-dotenv',
-    "socketdev>=3.2.1,<4.0.0",
+    "socketdev>=3.3.0,<4.0.0",
     "bs4>=0.0.2",
     "markdown>=3.10",
     "brotli>=1.0.9; platform_python_implementation == 'CPython'",

socketsecurity/core/__init__.py

@@ -14,12 +14,7 @@
 if TYPE_CHECKING:
     from socketsecurity.config import CliConfig
 from socketdev import socketdev
-from socketdev.exceptions import (
-    APIBadGateway,
-    APIConnectionError,
-    APIFailure,
-    APITimeout,
-)
+from socketdev.exceptions import APIFailure
 from socketdev.fullscans import FullScanParams, SocketArtifact
 from socketdev.org import Organization
 from socketdev.repos import RepositoryInfo
@@ -84,44 +79,18 @@
 
 # Full scan upload retry policy. An upload can fail transiently at the gateway/connection
 # level (an HTTP 502/503/504/408, a dropped or reset connection, or a client-side timeout)
-# without the server having created the scan. In these failure modes no scan was created,
-# so a retry does not duplicate one. (A duplicate is possible only if a gateway timeout
-# races a request the server later completes; that is benign - the retried scan supersedes
-# the orphaned one, same as running the CLI twice.)
-FULL_SCAN_UPLOAD_MAX_ATTEMPTS = 3
-# Wait before retry attempt 2 and attempt 3 respectively (plus a little jitter so a fleet of
-# CI jobs hitting the same failure doesn't retry in lock-step).
-FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS = (10.0, 30.0)
+# without the server having created the scan; the SDK classifies those failures via
+# APIFailure.is_transient_error() (socketdev>=3.3.0). In these failure modes no scan was
+# created, so a retry does not duplicate one. (A duplicate is possible only if a gateway
+# timeout races a request the server later completes; that is benign - the retried scan
+# supersedes the orphaned one, same as running the CLI twice.)
+#
+# Each schedule entry is the wait before the next attempt once the current one fails (plus
+# a little jitter so a fleet of CI jobs hitting the same failure doesn't retry in
+# lock-step); the final None means the last attempt's failure is re-raised, not retried.
+FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS = (10.0, 30.0, None)
+FULL_SCAN_UPLOAD_MAX_ATTEMPTS = len(FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS)
 FULL_SCAN_UPLOAD_BACKOFF_JITTER_SECONDS = 2.0
-# Transient gateway/timeout HTTP statuses that the SDK does NOT raise as a dedicated
-# exception class (502 has APIBadGateway; 408/503/504 surface as the catch-all APIFailure
-# with the status only present in the message text - see _is_transient_full_scan_upload_error).
-FULL_SCAN_UPLOAD_RETRYABLE_STATUS_CODES = frozenset({408, 503, 504})
-# Matches the status code the SDK embeds in catch-all APIFailure messages
-# (socketdev/core/api.py: "Bad Request: HTTP original_status_code:<code> ...").
-_API_FAILURE_STATUS_CODE_RE = re.compile(r"original_status_code:(\d{3})")
-
-
-def _is_transient_full_scan_upload_error(error: Exception) -> bool:
-    """Whether a full-scan upload failure is transient and safe to retry.
-
-    Transient means the failure happened at the gateway/connection level, normally before the
-    server finished reading the request body (so no scan was created server-side): HTTP
-    502/503/504/408, client-side timeouts, and dropped/reset connections. 4xx client errors
-    (400/401/403/404/429) and success responses carrying an error payload are never retried.
-    """
-    if isinstance(error, (APIBadGateway, APIConnectionError, APITimeout)):
-        # 502 / connection reset-dropped / request timeout - the SDK raises dedicated classes.
-        return True
-    if type(error) is APIFailure:
-        # The SDK raises 408/503/504 (and every other status without a dedicated class,
-        # including 400) as the catch-all APIFailure, so match on the exact class plus the
-        # status code embedded in the message. Subclasses (APIAccessDenied, APIResourceNotFound,
-        # APIInsufficientQuota, ...) are deliberately excluded - those are never transient.
-        match = _API_FAILURE_STATUS_CODE_RE.search(str(error))
-        if match:
-            return int(match.group(1)) in FULL_SCAN_UPLOAD_RETRYABLE_STATUS_CODES
-    return False
 
 
 def _humanize_alert_type(alert_type: str) -> str:
@@ -837,19 +806,19 @@ def create_full_scan(self, files: List[str], params: FullScanParams, base_paths:
             # Retry transient gateway/timeout failures (502/503/504/408, dropped connections,
             # timeouts) with increasing waits. In these failure modes the server never finished
             # reading the request body, so no scan was created and a retry does not duplicate
-            # one (see the retry-policy comment above FULL_SCAN_UPLOAD_MAX_ATTEMPTS). fullscans.post()
-            # rebuilds its lazy file loaders from the plain paths in upload_files on every call,
-            # so simply calling it again per attempt is safe. The loop must stay inside this try
-            # so the temp .br files (cleaned up in the finally below) outlive every attempt.
-            for attempt in range(1, FULL_SCAN_UPLOAD_MAX_ATTEMPTS + 1):
+            # one (see the retry-policy comment above FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS).
+            # fullscans.post() rebuilds its lazy file loaders from the plain paths in
+            # upload_files on every call, so simply calling it again per attempt is safe. The
+            # loop must stay inside this try so the temp .br files (cleaned up in the finally
+            # below) outlive every attempt.
+            for attempt, backoff_seconds in enumerate(FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS, start=1):
                 try:
                     res = self.sdk.fullscans.post(upload_files, params, use_types=True, use_lazy_loading=True, max_open_files=50, base_paths=base_paths)
                     break
                 except APIFailure as error:
-                    if attempt >= FULL_SCAN_UPLOAD_MAX_ATTEMPTS or not _is_transient_full_scan_upload_error(error):
+                    if backoff_seconds is None or not error.is_transient_error():
                         raise
-                    backoff_index = min(attempt, len(FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS)) - 1
-                    wait_seconds = FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS[backoff_index] + random.uniform(
+                    wait_seconds = backoff_seconds + random.uniform(
                         0, FULL_SCAN_UPLOAD_BACKOFF_JITTER_SECONDS
                     )
                     # SDK error messages can span many lines (path + response headers); the

tests/unit/test_full_scan_retry.py

@@ -2,9 +2,10 @@
 
 A `POST /orgs/<org>/full-scans` upload can fail transiently (an HTTP 502/503/504/408, a
 dropped or reset connection, or a timeout) without the server having created the scan.
-`Core.create_full_scan` retries those transient failures; these tests cover the retry
-classification, the loop bounds, and that the temporary brotli-compressed facts files
-survive until every attempt has finished.
+`Core.create_full_scan` retries the failures the SDK classifies as transient
+(`APIFailure.is_transient_error()`, socketdev>=3.3.0); these tests cover the retry
+decision, the loop bounds, and that the temporary brotli-compressed facts files survive
+until every attempt has finished.
 """
 
 import logging
@@ -26,7 +27,6 @@
     SOCKET_FACTS_BROTLI_FILENAME,
     SOCKET_FACTS_FILENAME,
     Core,
-    _is_transient_full_scan_upload_error,
 )
 
 
@@ -46,13 +46,14 @@ def _success_response():
     return response
 
 
-# Catch-all APIFailure messages as the SDK formats them (socketdev/core/api.py); only the
-# embedded original_status_code distinguishes a transient 503/504/408 from e.g. a 400.
+# Catch-all APIFailure as the SDK raises it for statuses without a dedicated class
+# (socketdev/core/api.py); the recorded status_code drives is_transient_error().
 def _catch_all_failure(status_code: int) -> APIFailure:
     return APIFailure(
         f"Bad Request: HTTP original_status_code:{status_code}\n"
         f"Path: https://api.socket.dev/v0/orgs/org/full-scans\n\n"
-        f"Headers:\ncf-ray: abc123"
+        f"Headers:\ncf-ray: abc123",
+        status_code=status_code,
     )
 
 
@@ -121,11 +122,14 @@ def test_upload_raises_after_exhausting_attempts(
     )
 
 
-def test_upload_retries_on_catch_all_503(core_with_mock_sdk, tmp_path, no_sleep):
+@pytest.mark.parametrize("status_code", [408, 503, 504])
+def test_upload_retries_on_catch_all_transient_statuses(
+    core_with_mock_sdk, tmp_path, no_sleep, status_code
+):
     manifest = tmp_path / "package.json"
     manifest.write_text("{}")
     core_with_mock_sdk.sdk.fullscans.post.side_effect = [
-        _catch_all_failure(503),
+        _catch_all_failure(status_code),
         _success_response(),
     ]
 
@@ -164,13 +168,17 @@ def test_upload_does_not_retry_on_400(core_with_mock_sdk, tmp_path, no_sleep):
     no_sleep.assert_not_called()
 
 
-@pytest.mark.parametrize("error_class", [APIAccessDenied, APIResourceNotFound])
+@pytest.mark.parametrize(
+    "error_class,status_code", [(APIAccessDenied, 401), (APIResourceNotFound, 404)]
+)
 def test_upload_does_not_retry_on_dedicated_4xx_classes(
-    core_with_mock_sdk, tmp_path, no_sleep, error_class
+    core_with_mock_sdk, tmp_path, no_sleep, error_class, status_code
 ):
     manifest = tmp_path / "package.json"
     manifest.write_text("{}")
-    core_with_mock_sdk.sdk.fullscans.post.side_effect = error_class()
+    core_with_mock_sdk.sdk.fullscans.post.side_effect = error_class(
+        status_code=status_code
+    )
 
     with pytest.raises(error_class):
         core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock())
@@ -241,25 +249,37 @@ def test_temp_br_file_cleaned_after_exhausted_retries(
     assert not compressed.exists()
 
 
-def test_transient_classifier():
-    assert _is_transient_full_scan_upload_error(APIBadGateway())
-    assert _is_transient_full_scan_upload_error(APIConnectionError())
-    assert _is_transient_full_scan_upload_error(APITimeout())
-    assert _is_transient_full_scan_upload_error(_catch_all_failure(408))
-    assert _is_transient_full_scan_upload_error(_catch_all_failure(503))
-    assert _is_transient_full_scan_upload_error(_catch_all_failure(504))
-
-    assert not _is_transient_full_scan_upload_error(_catch_all_failure(400))
-    assert not _is_transient_full_scan_upload_error(_catch_all_failure(500))
-    assert not _is_transient_full_scan_upload_error(
-        APIFailure()
-    )  # wrapped unexpected error
-    assert not _is_transient_full_scan_upload_error(APIAccessDenied("denied"))
-    assert not _is_transient_full_scan_upload_error(APIResourceNotFound())
-    # Subclasses never match the catch-all branch, even with a retryable-looking message.
-    assert not _is_transient_full_scan_upload_error(
-        APIAccessDenied("original_status_code:503")
-    )
-    assert not _is_transient_full_scan_upload_error(
-        ValueError("original_status_code:503")
-    )
+class _StubFailure(APIFailure):
+    """An APIFailure whose transience is fixed, regardless of class or status code."""
+
+    def __init__(self, transient: bool):
+        super().__init__("stub failure")
+        self._transient = transient
+
+    def is_transient_error(self) -> bool:
+        return self._transient
+
+
+@pytest.mark.parametrize("transient,expected_calls", [(True, 2), (False, 1)])
+def test_retry_decision_delegates_to_sdk_classification(
+    core_with_mock_sdk, tmp_path, no_sleep, transient, expected_calls
+):
+    # The CLI encodes no knowledge of the SDK's exception hierarchy or status codes:
+    # the retry decision is exactly APIFailure.is_transient_error(). (The transient /
+    # non-transient truth table itself is tested in the SDK, next to the code that
+    # raises the exceptions.)
+    manifest = tmp_path / "package.json"
+    manifest.write_text("{}")
+    core_with_mock_sdk.sdk.fullscans.post.side_effect = [
+        _StubFailure(transient),
+        _success_response(),
+    ]
+
+    if transient:
+        full_scan = core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock())
+        assert full_scan.id == "scan-1"
+    else:
+        with pytest.raises(_StubFailure):
+            core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock())
+
+    assert core_with_mock_sdk.sdk.fullscans.post.call_count == expected_calls