Retry target setup reads without recreating targets

.github/workflows/test.yml

@@ -131,6 +131,7 @@ jobs:
           - tests/mock_vws/test_model_target_retries.py
           - tests/mock_vws/vumark_generation_api
           - tests/mock_vws/test_vumark_generation_failure.py
+          - tests/mock_vws/test_target_setup_retries.py
           - tests/mock_vws/test_target_validators.py
           - tests/mock_vws/requests_mock
           - tests/mock_vws/respx_mock

tests/mock_vws/fixtures/api_contract.py

@@ -2,6 +2,7 @@
 
 import base64
 import binascii
+import contextlib
 import io
 import uuid
 from dataclasses import dataclass
@@ -12,7 +13,7 @@
 from tenacity.retry import retry_if_not_result
 from tenacity.stop import stop_after_attempt
 from vws import VWS, CloudRecoService
-from vws.reports import TargetStatuses
+from vws.reports import TargetStatusAndRecord, TargetStatuses
 
 from mock_vws.database import CloudDatabase
 from tests.mock_vws.utils import Endpoint, ModelTargetEndpoint
@@ -106,16 +107,20 @@ def _add_target(*, vws_client: VWS, image: io.BytesIO) -> str:
 
 
 @beartype
+@RETRY_ON_TRANSIENT_VWS_FAILURE
 @RETRY_ON_TARGET_NOT_YET_VISIBLE
-def _wait_for_target_processed(*, vws_client: VWS, target_id_: str) -> None:
-    """Wait for a target to finish processing.
+def _get_processed_target(
+    *, vws_client: VWS, target_id_: str
+) -> TargetStatusAndRecord:
+    """Wait for and read a known target without repeating its creation.
 
     Real Vuforia sometimes reports a target which it has just created as
     unknown, because the target is not yet visible to the read path. We know
     the target exists, because Vuforia gave us its ID, so we retry rather
     than fail.
     """
     vws_client.wait_for_target_processed(target_id=target_id_)
+    return vws_client.get_target_record(target_id=target_id_)
 
 
 @dataclass(frozen=True, kw_only=True)
@@ -135,7 +140,6 @@ def _target_processed_successfully(result: _TargetProcessingResult) -> bool:
 
 
 @beartype
-@RETRY_ON_TRANSIENT_VWS_FAILURE
 @retry(
     retry=retry_if_not_result(predicate=_target_processed_successfully),
     stop=stop_after_attempt(max_attempt_number=_TARGET_SUCCESS_ATTEMPTS),
@@ -156,14 +160,21 @@ def _add_target_which_processed_successfully(
         The ID and processing status of the added target.
     """
     target_id_ = _add_target(vws_client=vws_client, image=image)
-    _wait_for_target_processed(
-        vws_client=vws_client,
-        target_id_=target_id_,
-    )
-    target_details = vws_client.get_target_record(target_id=target_id_)
+    try:
+        target_details = _get_processed_target(
+            vws_client=vws_client,
+            target_id_=target_id_,
+        )
+    except Exception:
+        # Setup owns the target until it can return it to the test. Cleanup
+        # is best effort: a processing target may still reject deletion.
+        # The real adapter also clears remaining targets before the next test.
+        with contextlib.suppress(Exception):
+            vws_client.delete_target(target_id=target_id_)
+        raise
     # Failed processing is nondeterministic against the live service.
     if target_details.status != TargetStatuses.SUCCESS:
-        vws_client.delete_target(target_id=target_id_)  # pragma: no cover
+        vws_client.delete_target(target_id=target_id_)
     return _TargetProcessingResult(
         target_id=target_id_,
         status=target_details.status,

tests/mock_vws/test_target_setup_retries.py

@@ -0,0 +1,201 @@
+"""Regression tests for retries while preparing a successful target."""
+
+import io
+import time
+from unittest.mock import call, patch
+
+import pytest
+from requests.exceptions import ReadTimeout
+from tenacity import RetryError
+from vws import VWS
+from vws.exceptions.vws_exceptions import (
+    TooManyRequestsError,
+    UnknownTargetError,
+)
+from vws.reports import TargetRecord, TargetStatusAndRecord, TargetStatuses
+from vws.response import Response
+
+from tests.mock_vws.fixtures.api_contract import (
+    _add_target_which_processed_successfully,  # pyright: ignore[reportPrivateUsage]
+)
+from tests.mock_vws.utils.retries import TRANSIENT_VWS_RETRY_ATTEMPTS
+
+
+def _record(*, status: TargetStatuses) -> TargetStatusAndRecord:
+    """Return a record for a target whose processing has finished."""
+    return TargetStatusAndRecord(
+        status=status,
+        target_record=TargetRecord(
+            target_id="target",
+            active_flag=True,
+            name="name",
+            width=1,
+            tracking_rating=5,
+            reco_rating="",
+        ),
+    )
+
+
+def _throttle() -> TooManyRequestsError:
+    """Return the empty 429 response observed during fixture setup."""
+    return TooManyRequestsError(
+        response=Response(
+            text="",
+            url="https://example.com/summary/target",
+            status_code=429,
+            headers={"x-envoy-ratelimited": "true"},
+            request_body=None,
+            tell_position=0,
+            content=b"",
+        )
+    )
+
+
+def _no_sleep(seconds: float) -> None:
+    """Keep retry tests independent of wall-clock delays."""
+    del seconds
+
+
+@pytest.fixture(autouse=True)
+def fixture_no_sleep(*, monkeypatch: pytest.MonkeyPatch) -> None:
+    """Exercise the actual retry decorators without sleeping."""
+    monkeypatch.setattr(target=time, name="sleep", value=_no_sleep)
+
+
+@pytest.fixture(name="client")
+def fixture_client() -> VWS:
+    """Use synthetic credentials; all client operations are controlled."""
+    return VWS(server_access_key="key", server_secret_key="secret")
+
+
+@pytest.mark.parametrize(argnames="operation", argvalues=["poll", "record"])
+@pytest.mark.parametrize(
+    argnames="failure", argvalues=["429", "timeout", "unknown"]
+)
+def test_read_recovery_keeps_target(
+    *,
+    client: VWS,
+    operation: str,
+    failure: str,
+) -> None:
+    """A transient read or visibility failure never repeats the upload."""
+    error = {
+        "429": _throttle(),
+        "timeout": ReadTimeout("timed out"),
+        "unknown": UnknownTargetError(response=_throttle().response),
+    }[failure]
+    record = _record(status=TargetStatuses.SUCCESS)
+    with (
+        patch.object(
+            target=VWS, attribute="add_target", return_value="target"
+        ) as add,
+        patch.object(
+            target=VWS,
+            attribute="wait_for_target_processed",
+            side_effect=[error, None] if operation == "poll" else None,
+        ) as poll,
+        patch.object(
+            target=VWS,
+            attribute="get_target_record",
+            side_effect=[error, record] if operation == "record" else None,
+            return_value=record,
+        ) as read,
+        patch.object(target=VWS, attribute="delete_target") as delete,
+    ):
+        result = _add_target_which_processed_successfully(
+            vws_client=client, image=io.BytesIO()
+        )
+    assert result.target_id == "target"
+    add.assert_called_once()
+    assert poll.call_args_list == [call(target_id="target")] * 2
+    assert read.call_args_list == [call(target_id="target")] * (
+        2 if operation == "record" else 1
+    )
+    delete.assert_not_called()
+
+
+@pytest.mark.parametrize(argnames="operation", argvalues=["poll", "record"])
+@pytest.mark.parametrize(argnames="cleanup_fails", argvalues=[False, True])
+def test_read_exhaustion_preserves_error(
+    *,
+    client: VWS,
+    operation: str,
+    cleanup_fails: bool,
+) -> None:
+    """Exhaustion creates no replacement and cleanup cannot hide the error."""
+    error = _throttle()
+    with (
+        patch.object(
+            target=VWS, attribute="add_target", return_value="target"
+        ) as add,
+        patch.object(
+            target=VWS,
+            attribute="wait_for_target_processed",
+            side_effect=error if operation == "poll" else None,
+        ) as poll,
+        patch.object(
+            target=VWS, attribute="get_target_record", side_effect=error
+        ) as read,
+        patch.object(
+            target=VWS,
+            attribute="delete_target",
+            side_effect=RuntimeError("cleanup failed")
+            if cleanup_fails
+            else None,
+        ) as delete,
+        pytest.raises(expected_exception=TooManyRequestsError) as exc,
+    ):
+        _ = _add_target_which_processed_successfully(
+            vws_client=client, image=io.BytesIO()
+        )
+    assert exc.value is error
+    add.assert_called_once()
+    assert poll.call_count == TRANSIENT_VWS_RETRY_ATTEMPTS
+    assert read.call_count == (
+        0 if operation == "poll" else TRANSIENT_VWS_RETRY_ATTEMPTS
+    )
+    delete.assert_called_once_with(target_id="target")
+
+
+@pytest.mark.parametrize(argnames="recovers", argvalues=[False, True])
+def test_failed_processing_uses_bounded_replacements(
+    *, client: VWS, recovers: bool
+) -> None:
+    """Only an actual failed processing result triggers a new upload."""
+    failed = _record(status=TargetStatuses.FAILED)
+    success = _record(status=TargetStatuses.SUCCESS)
+    with (
+        patch.object(
+            target=VWS,
+            attribute="add_target",
+            side_effect=["first", "second", "third"],
+        ) as add,
+        patch.object(target=VWS, attribute="wait_for_target_processed"),
+        patch.object(
+            target=VWS,
+            attribute="get_target_record",
+            side_effect=[failed, success] if recovers else None,
+            return_value=failed,
+        ),
+        patch.object(target=VWS, attribute="delete_target") as delete,
+    ):
+        if recovers:
+            result = _add_target_which_processed_successfully(
+                vws_client=client, image=io.BytesIO()
+            )
+            assert result.target_id == "second"
+        else:
+            with pytest.raises(expected_exception=RetryError):
+                _ = _add_target_which_processed_successfully(
+                    vws_client=client, image=io.BytesIO()
+                )
+    assert add.call_count == (2 if recovers else 3)
+    assert delete.call_args_list == (
+        [call(target_id="first")]
+        if recovers
+        else [
+            call(target_id="first"),
+            call(target_id="second"),
+            call(target_id="third"),
+        ]
+    )