From d5c876e3d0b9c1ca87b9768eee92ce96ceaf90b9 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 12 Sep 2026 15:24:21 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein):=20=D0=B2=D0=BA=D0=BB=D1=8E=D1=87?= =?UTF-8?q?=D0=B0=D0=B5=D0=BC=20idempotency-key=20=D0=BD=D0=B0=20=D1=84?= =?UTF-8?q?=D1=80=D0=BE=D0=BD=D1=82=D0=B5,=20=D1=87=D0=B8=D0=BD=D0=B8?= =?UTF-8?q?=D0=BC=20assert-crash=20=D0=B8=20=D1=87=D0=B5=D1=81=D1=82=D0=BD?= =?UTF-8?q?=D0=BE=D1=81=D1=82=D1=8C=20=D0=B4=D0=BE=D0=BA=D1=81=D1=82=D1=80?= =?UTF-8?q?=D0=B8=D0=BD=D0=B3=D0=BE=D0=B2=20(#3471)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью PR #3495 нашло, что механизм был мёртвым кодом: фронт не отправлял Idempotency-Key ни в одном запросе, весь прод-трафик шёл по ненадёжному fallback-отпечатку. Плюс два "assert" в record_inbound после ON CONFLICT давали AssertionError (не ловится except SQLAlchemyError) уже ПОСЛЕ доставки в Telegram — под `python -O` assert и вовсе исчезает. - useSupportChat.ts: useSendSupportMessage генерирует Idempotency-Key (crypto.randomUUID()) на намерение отправить, переиспользует его при повторной отправке ТОГО ЖЕ текста, сбрасывает на успехе. - web_support_storage.record_inbound: assert -> явные ветки с логом; логируем отброшенный topic_message_id проигравшего гонку (не молча). - Докстринги/комментарии переписаны честно: что именно закрывает pre-check (ответ клиенту потерян / двойной клик после успеха), а что НЕ закрывает (сетевую потерю на плече Selectel -> Telegram — там сообщение просто не доставлено, повтор это законная первая попытка). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JY6iWDnGDthdvsMWgK1BMG --- tradein-mvp/backend/app/api/v1/support.py | 45 +++++++---- .../app/services/tgbot/web_support_storage.py | 74 +++++++++++++++++-- .../frontend/src/lib/useSupportChat.ts | 30 +++++++- 3 files changed, 126 insertions(+), 23 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/support.py b/tradein-mvp/backend/app/api/v1/support.py index 07406a23..5183efbe 100644 --- a/tradein-mvp/backend/app/api/v1/support.py +++ b/tradein-mvp/backend/app/api/v1/support.py @@ -358,15 +358,25 @@ def _resolve_idempotency_key(request: Request, *, identity_key: str, text: str) """Ключ идемпотентности inbound-отправки (#3471, миграция 301). Почему заголовок + fallback, а не что-то одно. Явный `Idempotency-Key` — - предпочтительный путь: клиент генерирует ключ ОДИН раз на "намерение + ЕДИНСТВЕННЫЙ надёжный путь: клиент генерирует ключ ОДИН раз на "намерение отправить" и переиспользует его на любом ретрае (fetch retry / переотправка после таймаута) независимо от того, что именно менялось в UI между - попытками. Без заголовка (старые клиенты, п.5 требования — они не должны - сломаться) используем детерминированный отпечаток sha256(identity, текст, - минутное окно): типичный повтор (двойной клик, ретрай браузера) укладывается - в секунды, минутное окно с запасом это покрывает, а любые два РАЗНЫХ по - смыслу сообщения с одинаковым текстом, отправленные намеренно с разницей в - пару минут, в одно не схлопнутся. + попытками (см. `useSendSupportMessage` во фронтенде — реальный трафик + обязан идти этим путём). Fallback без заголовка — детерминированный + отпечаток sha256(identity, текст, минутное окно) — существует ТОЛЬКО для + клиентов, которые заголовок не прислали (п.5 требования — старое поведение + не должно сломаться), и у него два честных изъяна, оба снимаются самим + фактом использования заголовка: + 1. два РАЗНЫХ по смыслу сообщения с одинаковым текстом от одного и того + же человека в течение одной минуты ("да" и ещё раз "да", "+", "ок") + СХЛОПНУТСЯ в одно — второе будет молча проглочено, клиент получит id + первого, оператор не увидит второе сообщение вообще; + 2. окно — не скользящие 60 секунд, а округление `time.time() // 60` вниз: + фактический срок дедупликации случаен от 0 до 60с в зависимости от + момента внутри минуты, и часть настоящих повторов (ретрай ровно на + границе окна) эту защиту не получит. + Оба пункта перестают быть важны, когда клиент реально шлёт заголовок + (см. `useSendSupportMessage`). Хранит и потенциально логирует (см. вызовы в этом файле) только САМ ключ — он либо непрозрачный клиентский токен, либо хэш. Текст сообщения сюда @@ -422,12 +432,21 @@ async def send_support_message( if cooldown is not None: raise _too_many_failures_error(cooldown) - # Идемпотентность (#3471): pre-check ДО похода в Telegram — повтор с тем же - # ключом не должен слать второе зеркало в топик, не только не писать вторую - # строку в БД. Тред может ещё не существовать (это первая отправка этого - # ключа) — тогда сравнивать не с чем, идём в Telegram как обычно; сам факт - # "нет треда" здесь безопасен, т.к. тред создаётся ТОЛЬКО в этой же ручке - # (см. H1) — если бы он уже был записан под этим ключом, тред уже был бы. + # Идемпотентность (#3471). Что именно закрывает этот pre-check — важно не + # переоценить: строка в БД (и её idempotency_key) появляется ТОЛЬКО ПОСЛЕ + # успешной доставки в Telegram (H1 ниже), поэтому потерю самого запроса на + # плече Selectel -> api.telegram.org этот механизм НЕ дедуплицирует — если + # `send_message` не удался, ключ нигде не записан, и повтор клиента после + # неудачи это законная первая попытка. Реально закрывается другой, тоже + # частый случай: доставка УЖЕ состоялась (Telegram принял, строка + # закоммичена), но ответ до клиента не дошёл (обрыв на обратном пути, + # клиентский таймаут) или пользователь кликнул "отправить" второй раз по + # той же ещё не отрисовавшейся отправке — тогда pre-check находит уже + # записанное сообщение по ключу и отдаёт его же, не уходя в Telegram снова. + # Тред может ещё не существовать (это первая попытка этого ключа) — тогда + # сравнивать не с чем, идём в Telegram как обычно; сам факт "нет треда" + # здесь безопасен, т.к. тред создаётся ТОЛЬКО в этой же ручке (см. H1) — + # если бы сообщение под этим ключом уже было записано, тред уже был бы. idempotency_key = _resolve_idempotency_key(request, identity_key=username, text=payload.text) existing_thread_id = storage.find_thread_id(db, username) if existing_thread_id is not None: diff --git a/tradein-mvp/backend/app/services/tgbot/web_support_storage.py b/tradein-mvp/backend/app/services/tgbot/web_support_storage.py index feb9ec2b..8d80e1f1 100644 --- a/tradein-mvp/backend/app/services/tgbot/web_support_storage.py +++ b/tradein-mvp/backend/app/services/tgbot/web_support_storage.py @@ -149,14 +149,74 @@ def record_inbound( ) if row is not None: return dict(row) - # Конфликт пойман индексом (см. докстринг выше) — idempotency_key точно не - # NULL здесь: при NULL partial-индекс в конфликт не участвует вообще. - assert idempotency_key is not None - existing = find_inbound_by_idempotency_key( - db, thread_id=thread_id, idempotency_key=idempotency_key + + # Конфликт пойман индексом — idempotency_key почти наверняка не NULL здесь + # (при NULL partial-индекс в конфликт не участвует), но НЕ `assert`: это + # прод-путь ПОСЛЕ уже доставленного в Telegram сообщения (см. H1 в + # app.api.v1.support), а `except SQLAlchemyError` в вызывающей стороне + # `AssertionError` не ловит — под `python -O` assert вдобавок исчезает + # молча и `existing` осталось бы `None`, что развалилось бы чуть ниже при + # сборке ответа. Вместо падения — явная ветка с логом и WORKING откатом. + existing = ( + find_inbound_by_idempotency_key(db, thread_id=thread_id, idempotency_key=idempotency_key) + if idempotency_key is not None + else None ) - assert existing is not None # конфликт по индексу гарантирует наличие строки - return existing + if existing is not None: + # Ожидаемый случай гонки (#3471): проигравший запрос уже отправил своё + # СОБСТВЕННОЕ зеркало в Telegram (свой topic_message_id) до того, как + # обнаружил конфликт здесь — эта копия зеркала теперь осиротела + # (реплай оператора на неё никуда не смаршрутизируется, т.к. строки + # для неё в БД нет). Осознанно не чиним это в этом PR (см. коммент + # выше по коду), но фиксируем в логе, а не молчим — ровно то же самое, + # для чего уже есть `_warn_operator_message_not_recorded` в другом месте. + logger.warning( + "web support: гонка по idempotency_key — зеркало topic_message_id=%s " + "(thread_id=%d) отправлено, но НЕ записано, победила строка id=%d", + topic_message_id, + thread_id, + existing["id"], + ) + return existing + + # Крайний случай (в норме недостижим при READ COMMITTED, которую использует + # этот сервис): индекс сообщил о конфликте, но повторное чтение строку не + # нашло. Логируем и пишем БЕЗ идемпотентности — NULL-ключ никогда не + # конфликтует сам с собой (partial-индекс его не видит), INSERT гарантированно + # пройдёт; для этого одного сообщения дедупликация отключается, но клиент + # получает корректный ответ вместо 500 после уже состоявшейся доставки. + logger.warning( + "web support: ON CONFLICT сообщил о конфликте (thread_id=%d, " + "idempotency_key=%s), но повторное чтение строки её не нашло — " + "записываем без идемпотентности", + thread_id, + "" if idempotency_key is not None else None, + ) + row = ( + db.execute( + text( + """ + INSERT INTO web_support_messages + (thread_id, direction, text_body, topic_message_id, + support_chat_id, operator_tg_id, idempotency_key, created_at) + VALUES + (CAST(:thread_id AS bigint), 'in', :text_body, + CAST(:topic_message_id AS bigint), + CAST(:support_chat_id AS bigint), NULL, NULL, NOW()) + RETURNING id, direction, text_body, operator_tg_id, created_at + """ + ), + { + "thread_id": thread_id, + "text_body": text_body, + "topic_message_id": topic_message_id, + "support_chat_id": support_chat_id, + }, + ) + .mappings() + .one() + ) + return dict(row) def find_thread_by_topic_message( diff --git a/tradein-mvp/frontend/src/lib/useSupportChat.ts b/tradein-mvp/frontend/src/lib/useSupportChat.ts index 209b2984..cdf712b5 100644 --- a/tradein-mvp/frontend/src/lib/useSupportChat.ts +++ b/tradein-mvp/frontend/src/lib/useSupportChat.ts @@ -42,6 +42,7 @@ * setQueryData-merge path that would be pure incidental complexity here. */ +import { useRef } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { apiFetch, HTTPError } from "@/lib/api"; @@ -129,13 +130,36 @@ export function useSupportUnread(enabled: boolean, scope: SupportScope = "auth") export function useSendSupportMessage(scope: SupportScope = "auth") { const queryClient = useQueryClient(); + // Идемпотентность (#3471): ОДИН `Idempotency-Key` на все повторы ОДНОГО + // намерения отправить. Бэкенд пишет строку/ключ ТОЛЬКО ПОСЛЕ успешной + // доставки в Telegram, поэтому это не про потери на плече Selectel -> + // api.telegram.org (неудачный send там — законная первая попытка) — это + // про то, что доставка УЖЕ состоялась, а ответ до клиента не дошёл (обрыв + // на обратном пути / клиентский таймаут) или пользователь дважды нажал + // "отправить" по одной и той же ещё не отрисовавшейся отправке. Без + // заголовка единственная защита — backend-fallback (минутный + // hash-отпечаток, `_resolve_idempotency_key` в app/api/v1/support.py), а + // он ненадёжен для осмысленно повторяющихся коротких сообщений ("+", "ок", + // два "да" подряд). Намерение отождествляем с текстом: тот же текст подряд — + // это ретрай ЭТОГО намерения, переиспользуем ключ; новый текст — новое + // намерение, новый UUID. + const intentRef = useRef<{ text: string; key: string } | null>(null); + return useMutation({ - mutationFn: (text) => - apiFetch(`${scopeBase(scope)}/messages`, { + mutationFn: (text) => { + if (!intentRef.current || intentRef.current.text !== text) { + intentRef.current = { text, key: crypto.randomUUID() }; + } + return apiFetch(`${scopeBase(scope)}/messages`, { method: "POST", + headers: { "Idempotency-Key": intentRef.current.key }, body: JSON.stringify({ text }), - }), + }); + }, onSuccess: () => { + // Успех закрывает намерение — следующая отправка (даже с тем же текстом) + // обязана получить НОВЫЙ ключ, иначе она молча схлопнется с этой. + intentRef.current = null; queryClient.invalidateQueries({ queryKey: messagesKey(scope) }); }, });