Ревью pull request по diff

Ревью PR: ретраи и Idempotency-Key для create_charge

1. Вердикт

Мержить после правок. Подход верный: идемпотентный ключ плюс ретраи сетевых ошибок закрывают описанный сценарий двойного списания. Но в текущем виде ключ создаётся с гонкой и может быть вытеснен из Redis, то есть дубль charge всё ещё возможен. Кроме того, create_charge теперь молча возвращает None вместо исключения, а в service.py попала незаявленная смена источника суммы списания. Это три блокера, и все они правятся в рамках этого diff.

Сверка с описанием:

• Пункты 1–4 реализованы.
• Изменение в app/payments/service.py в описании не упомянуто. Оно противоречит фразе «не трогал обработку платежей в биллинге» и к цели PR не относится.
• У service.py в заголовке diff одинаковые хеши (index 7c2d9aa..7c2d9aa), чего у реального изменения быть не может. Проверь, что diff собран из ветки, а не вручную, и что это изменение действительно в ней есть.

Что не видно по diff и влияет на вердикт:

• Celery-таск, вызывающий process_order_payment: его autoretry_for, max_retries, soft_time_limit.
• Конфиг redis_client: decode_responses, maxmemory-policy, persistence.
• Контракт идемпотентности biller-gateway.

Без них часть выводов ниже условна, это отмечено в комментариях и в вопросах.

2. Комментарии к строкам

app/payments/client.py:49-55 — блокер: гонка при создании ключа

get, а затем set выполняются неатомарно. Два конкурентных вызова для одного order_id оба видят None, генерируют разные UUID и шлют разные ключи. Конкурентные вызовы — это, например, Celery-ретрай, запущенный, пока первый воркер ещё висит в ReadTimeout (15 с × 3), или дубль таска. Gateway создаёт два charge, то есть ровно тот баг, который чинит PR.

Есть ещё одна связанная проблема. Redis в проекте уже используется под кеш тарифов, и если там maxmemory-policy allkeys-lru или allkeys-random, ключ idemp:* может быть вытеснен. Тот же эффект дадут рестарт без AOF и failover. Следующая попытка получит новый ключ.

Предлагаю детерминированный ключ: он убирает гонку, вытеснение, TTL и зависимость от Redis в критическом пути одновременно.

_IDEMPOTENCY_NAMESPACE = uuid.UUID("<сгенерировать один раз и зафиксировать>")


def _idempotency_key(order_id: str) -> str:
    # Детерминированный ключ: одинаков для любых повторов по заказу,
    # не зависит от доступности/вытеснения Redis и не имеет гонки.
    return str(uuid.uuid5(_IDEMPOTENCY_NAMESPACE, f"charge:{order_id}"))

Если по заказу легитимно возможны повторные charge (например, после decline другой картой), в ключ нужно добавить номер попытки оплаты из БД, а не из Redis. Если Redis всё же оставляете, то как минимум сделайте атомарную запись:

IDEMPOTENCY_KEY_TTL = 60 * 60 * 24


def _get_or_create_idempotency_key(order_id: str) -> str:
    redis_key = f"idemp:{order_id}"
    candidate = str(uuid.uuid4())
    # SET NX атомарен: из конкурирующих вызовов выиграет ровно один
    if redis_client.set(redis_key, candidate, nx=True, ex=IDEMPOTENCY_KEY_TTL):
        return candidate
    existing = redis_client.get(redis_key)
    if existing is None:
        raise IdempotencyKeyUnavailable(order_id)  # не генерировать новый молча
    return existing.decode() if isinstance(existing, bytes) else existing

Плюс отдельный инстанс или DB Redis с noeviction для этих ключей.

app/payments/client.py:41-43 — блокер: except Exception: return None глотает все ошибки

Под этот except попадают:

• HTTPStatusError: 4xx (decline, 409 или 422 по идемпотентности, 400 валидации) и 5xx;
• ошибки парсинга ChargeResult;
• любые баги в коде.

Вызывающий код получает None без причины. Раньше исключение пробрасывалось, теперь отказ банка неотличим от падения сети.

Отдельно: сигнатура осталась -> ChargeResult, то есть аннотация врёт.

Предлагаю пробрасывать неретраибельные ошибки как есть:

        except RETRYABLE_EXCEPTIONS as exc:
            ...
        # никакого except Exception — 4xx/парсинг/баги летят наружу как раньше

app/payments/client.py:45-46 — блокер: исчерпание ретраев возвращает None

Раньше после сетевой ошибки летело исключение. Теперь возвращается None, а service.py:30 падает с AttributeError: 'NoneType' object has no attribute 'id'.

Как на это отреагирует Celery, по diff не видно. Если в таске autoretry_for=(httpx.HTTPError,) или аналог, ретрая больше не будет и платёж зависнет. Если autoretry_for=(Exception,), ретраить будет, но по AttributeError, что в логах выглядит как баг. В обоих случаях это изменение контракта для кода, который PR сознательно не трогает (BILL-482).

Предлагаю сохранить контракт «ошибка = исключение»:

class BillerGatewayUnavailable(Exception):
    def __init__(self, order_id: str):
        super().__init__(f"biller-gateway unavailable for order {order_id}")
        self.order_id = order_id

...
    logger.error("biller-gateway exhausted retries for order %s", order_id)
    raise BillerGatewayUnavailable(order_id) from last_exc

Если хочется сохранить совместимость с текущим autoretry_for, можно наследоваться от httpx.TransportError, но лучше явно проверить таск.

app/payments/service.py:28 — блокер: незаявленная смена источника суммы

Было order.amount и order.currency из БД, стало amount и currency из аргументов. Это меняет, сколько списывается: теперь сумму определяет вызывающий, а не заказ. Если аргументы где-то расходятся с заказом (устаревшее значение в payload таска, копейки против рублей), получим неверное списание.

Это не связано с ретраями и идемпотентностью. Нужно либо откатить, либо вынести в отдельный PR с обоснованием.

    result = create_charge(order_id, order.amount, order.currency)

Если цель была в том, чтобы аргументы amount и currency перестали быть мёртвыми, правильнее сверять их с заказом и падать при расхождении. Но это тоже отдельный PR.

app/payments/service.py:28-31 — важно: вызывающий код не адаптирован к новому контракту

Даже если create_charge останется с None, здесь нет проверки. Кроме того, order.status = "charged" выставляется до проверки результата. Если order — объект ORM-сессии с autoflush или commit в обработчике ошибок, статус charged может сохраниться без charge_id. Если create_charge бросает исключение (см. выше), проблема снимается. Для надёжности порядок стоит поменять:

    result = create_charge(order_id, order.amount, order.currency)
    order.charge_id = result.id
    order.status = "charged"
    save_order(order)

app/payments/client.py:34 — важно: неполный список ретраибельных ошибок

Сейчас за бортом:

• WriteTimeout и PoolTimeout;
• ReadError и RemoteProtocolError: обрыв соединения посреди ответа, типичный случай для «сеть отваливается на 3–5 секунд»;
• 502/503/504 от gateway или балансировщика.

С идемпотентным ключом всё это безопасно ретраить. Сейчас они уходят в except Exception и превращаются в None.

RETRYABLE_EXCEPTIONS = (
    httpx.TimeoutException,       # Connect/Read/Write/PoolTimeout
    httpx.ConnectError,
    httpx.ReadError,
    httpx.RemoteProtocolError,
)
RETRYABLE_STATUSES = {502, 503, 504}

...
        try:
            response = httpx.post(...)
            response.raise_for_status()
            return ChargeResult.parse_obj(response.json())
        except httpx.HTTPStatusError as exc:
            if exc.response.status_code not in RETRYABLE_STATUSES:
                raise
            last_exc = exc
        except RETRYABLE_EXCEPTIONS as exc:
            last_exc = exc
        logger.warning(
            "biller-gateway retry %s/%s for order %s: %r",
            attempt, MAX_RETRIES, order_id, last_exc,
        )
        if attempt < MAX_RETRIES:
            time.sleep(BACKOFF_BASE ** attempt)

app/payments/client.py:39 — важно: sleep после последней попытки и суммарное время

После третьей неудачи код спит ещё 8 секунд, прежде чем сдаться. Это лишнее.

Худший случай сейчас: 3 × 15 с таймаута + 2 + 4 + 8 = ~59 с блокировки воркера. Нужно сверить это с soft_time_limit и time_limit Celery-таска, а при Redis-брокере ещё и с visibility_timeout. Если лимит меньше, таск убьют посреди попытки и Celery перезапустит его. С детерминированным ключом это безопасно, с текущей реализацией нет.

Правка if attempt < MAX_RETRIES показана выше. Ещё стоит добавить jitter (+ random.uniform(0, 1)), чтобы после сетевого сбоя все воркеры не ретраили синхронно.

app/payments/client.py:20,30 — важно: timeout=15.0 применяется и к connect

В httpx скаляр задаёт одинаковый таймаут для connect, read, write и pool. Ответ p99 ~9 с — аргумент про read. Ждать TCP-коннект до внутреннего сервиса 15 секунд бессмысленно: при «отвалившейся сети» каждая попытка будет тратить по 15 с вместо быстрого отказа и ретрая.

DEFAULT_TIMEOUT = httpx.Timeout(15.0, connect=3.0)

def create_charge(..., timeout: httpx.Timeout | float = DEFAULT_TIMEOUT) -> ChargeResult:

app/payments/client.py:52 — важно: cached.decode() зависит от конфига клиента

Если redis_client создан с decode_responses=True (частый вариант для кеша), get вернёт str, и .decode() упадёт с AttributeError. Причём упадёт только на повторной попытке, то есть ровно в том сценарии, ради которого PR. Тест это не ловит, потому что redis замокан и get возвращает None.

Правку см. в варианте с SET NX выше (isinstance(existing, bytes)). С детерминированным ключом вопрос исчезает.

app/payments/client.py:22 — важно: недоступность Redis

Вызов идёт вне try, поэтому при недоступном Redis наружу летит redis.ConnectionError. Fail-closed здесь правильное поведение, генерировать ключ без Redis нельзя. Но это нужно зафиксировать явно, в docstring и тесте, и убедиться, что Celery ретраит такую ошибку. Детерминированный ключ убирает и эту зависимость.

app/payments/client.py:12-13,54 — мелочь: именованные константы

BACKOFF_BASE = 2 не говорит, что это секунды и основание степени. 60 * 60 * 24 стоит вынести в константу.

MAX_ATTEMPTS = 3                    # всего попыток, включая первую
BACKOFF_BASE_SECONDS = 2            # задержка = BASE ** attempt
IDEMPOTENCY_KEY_TTL = 60 * 60 * 24  # должен быть >= окна идемпотентности gateway и окна ретраев Celery

MAX_RETRIES = 3 при range(1, MAX_RETRIES + 1) означает 3 попытки, то есть 2 ретрая. Имя вводит в заблуждение.

app/payments/client.py:42 — мелочь: потеря traceback

Если except Exception останется в каком-то виде, logger.error(... %s, exc) теряет стек. Нужен logger.exception(...).

tests/test_payments.py:~14-23 (test_create_charge_success) — важно: тест теперь ходит в реальный Redis

create_charge теперь всегда вызывает redis_client, а в старом тесте он не замокан. В CI без Redis тест упадёт. С Redis тест оставит idemp:order-1 и станет зависеть от порядка запусков. Предлагаю общую фикстуру:

@pytest.fixture(autouse=True)
def fake_redis(monkeypatch):
    import fakeredis
    client = fakeredis.FakeRedis()
    monkeypatch.setattr("app.payments.client.redis_client", client)
    return client

tests/test_payments.py:26-38 — важно: тест спит по-настоящему и не проверяет главное

Без патча time.sleep тест реально ждёт 2 с. Кроме того, тест не проверяет, что обе попытки ушли с одинаковым Idempotency-Key, хотя это суть PR.

@patch("app.payments.client.time.sleep")
def test_create_charge_retries_on_timeout(mock_sleep, respx_mock):
    route = respx_mock.post("https://biller-gateway.internal.corp/v1/charges")
    route.side_effect = [
        httpx.ConnectTimeout("timeout"),
        httpx.Response(200, json={"id": "ch_456", "status": "succeeded"}),
    ]

    result = create_charge("order-2", 500, "RUB")

    assert result.id == "ch_456"
    assert route.call_count == 2
    keys = {call.request.headers["Idempotency-Key"] for call in route.calls}
    assert len(keys) == 1
    mock_sleep.assert_called_once()

Тесты, которые стоит добавить

3. Вопросы автору

1. Поддерживает ли biller-gateway Idempotency-Key и с какой семантикой? От ответов зависит, закрывает ли PR баг вообще:
   • Какое у gateway окно хранения ключа (если меньше 24 ч, наш TTL ничего не даёт)?
   • Что он отвечает на повтор с тем же ключом, пока первый запрос ещё в обработке (409, ожидание, дубль)? Это прямо наш сценарий с ReadTimeout.
   • Что будет при повторе с тем же ключом, но другой суммой?
   • Есть ли контрактный тест или документация?
2. Почему изменён service.py:28 (сумма из аргументов вместо заказа)? Есть ли вызовы, где amount и currency в аргументах отличаются от заказа?
3. Celery-таск: какие autoretry_for, max_retries, countdown, soft_time_limit? Переживёт ли он смену исключения на None или AttributeError и худший случай ~59 с внутри create_charge? Укладывается ли общее окно Celery-ретраев в TTL ключа?
4. Конфиг Redis: decode_responses у redis_client, maxmemory-policy и persistence инстанса, где лежит кеш тарифов? Если ключ остаётся в Redis, допустимо ли его вытеснение?
5. Бывают ли легитимные повторные charge по одному order_id (повторная оплата после decline, доплата)? От этого зависит выбор между детерминированным ключом и ключом с номером попытки из БД. С текущим ключом на 24 ч повторная оплата после decline, скорее всего, вернёт закешированный decline.
6. Вызывается ли create_charge из FastAPI-эндпоинтов (особенно из async def) или только из Celery? Если из async-кода, time.sleep и синхронный httpx.post на ~59 с заблокируют event loop.
7. Что с 7 уже случившимися двойными списаниями и с заказами, которые сейчас в процессе? После деплоя у них появится новый ключ. Нужен ли разовый скрипт сверки или возвратов? Это вне PR, но стоит убедиться, что задача заведена.