diff --git a/src/mud_server/services/character_provisioning.py b/src/mud_server/services/character_provisioning.py index 7ffb28f..a7c0d9a 100644 --- a/src/mud_server/services/character_provisioning.py +++ b/src/mud_server/services/character_provisioning.py @@ -29,6 +29,11 @@ logger = logging.getLogger(__name__) +# Keep retries intentionally small to avoid extending admin provisioning latency +# too much during degraded upstream conditions. +_NAMEGEN_MAX_ATTEMPTS = 3 +_NAMEGEN_RETRYABLE_HTTP_STATUS_CODES = frozenset({408, 429, 500, 502, 503, 504}) + @dataclass(slots=True) class CharacterProvisioningResult: @@ -76,6 +81,14 @@ def _fetch_generated_name( """ Request one name token from the external name-generation API. + Retry policy: + We retry only transient upstream failures: + 1. Request-layer exceptions (timeout/connection/reset). + 2. Retryable HTTP statuses (408/429/5xx). + + We intentionally do not retry schema/validation errors because those + indicate contract drift rather than transient transport issues. + Returns: Tuple ``(name, error_message)``. On success, ``error_message`` is None. """ @@ -98,15 +111,54 @@ def _fetch_generated_name( "seed": seed, } - try: - response = requests.post( - endpoint, - json=payload, - timeout=config.integrations.namegen_timeout_seconds, - ) + last_status_code: int | None = None + last_request_error: Exception | None = None + + for attempt in range(1, _NAMEGEN_MAX_ATTEMPTS + 1): + try: + response = requests.post( + endpoint, + json=payload, + timeout=config.integrations.namegen_timeout_seconds, + ) + except requests.exceptions.RequestException as exc: + last_request_error = exc + if attempt < _NAMEGEN_MAX_ATTEMPTS: + logger.warning( + "Name generation API retry attempt=%s/%s reason=request_exception error=%s", + attempt, + _NAMEGEN_MAX_ATTEMPTS, + exc, + ) + continue + logger.warning( + "Name generation API request failed after %s attempts: %s", + attempt, + exc, + ) + return None, "Name generation API unavailable." + if response.status_code != 200: + last_status_code = response.status_code + if ( + response.status_code in _NAMEGEN_RETRYABLE_HTTP_STATUS_CODES + and attempt < _NAMEGEN_MAX_ATTEMPTS + ): + logger.warning( + "Name generation API retry attempt=%s/%s reason=http_status status_code=%s", + attempt, + _NAMEGEN_MAX_ATTEMPTS, + response.status_code, + ) + continue return None, f"Name generation API returned HTTP {response.status_code}." - body = response.json() + + try: + body = response.json() + except ValueError: + logger.warning("Name generation API returned invalid JSON.") + return None, "Name generation API returned invalid JSON." + if not isinstance(body, dict): return None, "Name generation API returned a non-object payload." names = body.get("names") @@ -116,12 +168,13 @@ def _fetch_generated_name( if not generated_name: return None, "Name generation API returned an empty name." return generated_name, None - except requests.exceptions.RequestException as exc: - logger.warning("Name generation API request failed: %s", exc) + + if last_status_code is not None: + return None, f"Name generation API returned HTTP {last_status_code}." + if last_request_error is not None: + logger.warning("Name generation API request failed: %s", last_request_error) return None, "Name generation API unavailable." - except ValueError: - logger.warning("Name generation API returned invalid JSON.") - return None, "Name generation API returned invalid JSON." + return None, "Name generation API unavailable." def fetch_generated_full_name(seed: int) -> tuple[str | None, str | None]: diff --git a/tests/test_api/test_admin_create_character.py b/tests/test_api/test_admin_create_character.py index c34f1c6..9305690 100644 --- a/tests/test_api/test_admin_create_character.py +++ b/tests/test_api/test_admin_create_character.py @@ -77,7 +77,7 @@ def test_fetch_generated_name_handles_empty_base_url(monkeypatch): @pytest.mark.unit def test_fetch_generated_name_handles_non_200(monkeypatch): - """Name helper should surface upstream HTTP status failures.""" + """Retryable HTTP statuses should be retried before surfacing failure.""" monkeypatch.setattr(character_provisioning.config.integrations, "namegen_enabled", True) monkeypatch.setattr( character_provisioning.config.integrations, "namegen_base_url", "https://name.example.org" @@ -85,12 +85,34 @@ def test_fetch_generated_name_handles_non_200(monkeypatch): fake_response = Mock(status_code=503) fake_response.json.return_value = {} - monkeypatch.setattr(character_provisioning.requests, "post", Mock(return_value=fake_response)) + post_mock = Mock(return_value=fake_response) + monkeypatch.setattr(character_provisioning.requests, "post", post_mock) name, error = character_provisioning._fetch_generated_name(seed=12) assert name is None assert error == "Name generation API returned HTTP 503." + assert post_mock.call_count == 3 + + +@pytest.mark.unit +def test_fetch_generated_name_does_not_retry_on_non_retryable_http_status(monkeypatch): + """Non-retryable HTTP statuses should fail immediately without extra calls.""" + monkeypatch.setattr(character_provisioning.config.integrations, "namegen_enabled", True) + monkeypatch.setattr( + character_provisioning.config.integrations, "namegen_base_url", "https://name.example.org" + ) + + fake_response = Mock(status_code=400) + fake_response.json.return_value = {} + post_mock = Mock(return_value=fake_response) + monkeypatch.setattr(character_provisioning.requests, "post", post_mock) + + name, error = character_provisioning._fetch_generated_name(seed=19) + + assert name is None + assert error == "Name generation API returned HTTP 400." + post_mock.assert_called_once() @pytest.mark.unit @@ -149,13 +171,15 @@ def test_fetch_generated_name_handles_blank_name(monkeypatch): @pytest.mark.unit def test_fetch_generated_name_handles_request_exception(monkeypatch): - """Network-layer errors should become an availability message.""" + """Request exceptions should retry and then map to availability errors.""" monkeypatch.setattr(character_provisioning.config.integrations, "namegen_enabled", True) monkeypatch.setattr( character_provisioning.config.integrations, "namegen_base_url", "https://name.example.org" ) + attempts = {"count": 0} def _boom(*_args, **_kwargs): # noqa: ANN002,ANN003 - test double + attempts["count"] += 1 raise character_provisioning.requests.exceptions.RequestException("boom") monkeypatch.setattr(character_provisioning.requests, "post", _boom) @@ -164,6 +188,58 @@ def _boom(*_args, **_kwargs): # noqa: ANN002,ANN003 - test double assert name is None assert error == "Name generation API unavailable." + assert attempts["count"] == 3 + + +@pytest.mark.unit +def test_fetch_generated_name_retries_request_exception_then_succeeds(monkeypatch): + """Transient request exceptions should succeed when a later attempt succeeds.""" + monkeypatch.setattr(character_provisioning.config.integrations, "namegen_enabled", True) + monkeypatch.setattr( + character_provisioning.config.integrations, "namegen_base_url", "https://name.example.org" + ) + attempts = {"count": 0} + + def _post_mock(*_args, **_kwargs): # noqa: ANN002,ANN003 - test double + attempts["count"] += 1 + if attempts["count"] < 3: + raise character_provisioning.requests.exceptions.RequestException("temporary") + response = Mock(status_code=200) + response.json.return_value = {"names": ["Kira"]} + return response + + monkeypatch.setattr(character_provisioning.requests, "post", _post_mock) + + name, error = character_provisioning._fetch_generated_name(seed=72) + + assert error is None + assert name == "Kira" + assert attempts["count"] == 3 + + +@pytest.mark.unit +def test_fetch_generated_name_retries_retryable_http_status_then_succeeds(monkeypatch): + """Retryable status failures should be retried before succeeding.""" + monkeypatch.setattr(character_provisioning.config.integrations, "namegen_enabled", True) + monkeypatch.setattr( + character_provisioning.config.integrations, "namegen_base_url", "https://name.example.org" + ) + responses = [ + Mock(status_code=503), + Mock(status_code=504), + Mock(status_code=200), + ] + responses[0].json.return_value = {} + responses[1].json.return_value = {} + responses[2].json.return_value = {"names": ["Riven"]} + post_mock = Mock(side_effect=responses) + monkeypatch.setattr(character_provisioning.requests, "post", post_mock) + + name, error = character_provisioning._fetch_generated_name(seed=917) + + assert error is None + assert name == "Riven" + assert post_mock.call_count == 3 @pytest.mark.unit