From 091befc9ffd88628847a3c3a1ce6f6e4c736a70f Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 12 Sep 2026 14:59:52 +0300 Subject: [PATCH 1/3] =?UTF-8?q?fix(tradein):=20=D0=B8=D0=B4=D0=B5=D0=BC?= =?UTF-8?q?=D0=BF=D0=BE=D1=82=D0=B5=D0=BD=D1=82=D0=BD=D0=B0=D1=8F=20=D0=BE?= =?UTF-8?q?=D1=82=D0=BF=D1=80=D0=B0=D0=B2=D0=BA=D0=B0=20=D1=81=D0=BE=D0=BE?= =?UTF-8?q?=D0=B1=D1=89=D0=B5=D0=BD=D0=B8=D1=8F=20=D0=B2=20=D0=BF=D0=BE?= =?UTF-8?q?=D0=B4=D0=B4=D0=B5=D1=80=D0=B6=D0=BA=D1=83=20(#3471)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Сеть Selectel -> api.telegram.org теряет заметную долю коротких запросов, поэтому браузерный ретрай/двойной клик/переотправка по таймауту при отправке в веб-чат поддержки создавали ВТОРУЮ строку в web_support_messages И второе зеркало в support-топике Telegram, а не только дубль в БД. Ключ идемпотентности (миграция 301, колонка idempotency_key + partial unique индекс (thread_id, idempotency_key) WHERE direction='in'): - явный заголовок Idempotency-Key от клиента, если он есть и валидной формы; - иначе детерминированный fallback-отпечаток sha256(identity|текст|минутное окно) — старые клиенты без заголовка продолжают работать без изменений. Pre-check резолвит тред по identity и ищет существующее inbound-сообщение с этим ключом ДО похода в Telegram (не только до записи в БД) — иначе повтор всё равно отправил бы второе зеркало, даже если бы вторая строка в БД не создавалась. Гонку двух одновременных запросов с одним ключом закрывает INSERT ... ON CONFLICT DO NOTHING на уникальном индексе в web_support_storage.record_inbound (не read-then-write), а не сам pre-check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JY6iWDnGDthdvsMWgK1BMG --- tradein-mvp/backend/app/api/v1/support.py | 91 ++++++++++ .../app/services/tgbot/web_support_storage.py | 66 ++++++- ...01_web_support_message_idempotency_key.sql | 53 ++++++ tradein-mvp/backend/tests/test_support.py | 171 +++++++++++++++++- 4 files changed, 375 insertions(+), 6 deletions(-) create mode 100644 tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql diff --git a/tradein-mvp/backend/app/api/v1/support.py b/tradein-mvp/backend/app/api/v1/support.py index bdaf5268..07406a23 100644 --- a/tradein-mvp/backend/app/api/v1/support.py +++ b/tradein-mvp/backend/app/api/v1/support.py @@ -62,6 +62,7 @@ import hashlib import logging import re import secrets +import time from datetime import UTC, datetime from typing import Annotated, Literal @@ -170,6 +171,14 @@ _SEND_UNAVAILABLE_DETAIL = "Telegram сейчас недоступен. Попр # треде отдавало бы ВЕСЬ лог переписки. См. `web_support_storage.list_messages`. _LIST_MESSAGES_LIMIT = 200 +# Идемпотентность отправки (#3471 retry-storm) — см. `_resolve_idempotency_key`. +_IDEMPOTENCY_HEADER = "idempotency-key" +# Форма клиентского ключа — как у anon-токена (`_ANON_TOKEN_RE`): произвольная +# opaque-строка клиента, без пробелов/спецсимволов, которые попали бы в SQL-параметр +# как есть. Не матчится — считаем заголовок отсутствующим и уходим на fallback, +# а не пытаемся его "починить" (тот же принцип, что у `_read_anon_token`). +_CLIENT_IDEMPOTENCY_KEY_RE = re.compile(r"^[A-Za-z0-9_.-]{8,128}\Z") + def _require_username(request: Request) -> str: """Достаёт X-Authenticated-User. rbac_guard (app/main.py) уже гарантирует его @@ -345,8 +354,41 @@ def _rollback_quietly(db: Session) -> None: logger.warning("web support: rollback после сбоя БД тоже не удался", exc_info=True) +def _resolve_idempotency_key(request: Request, *, identity_key: str, text: str) -> str: + """Ключ идемпотентности inbound-отправки (#3471, миграция 301). + + Почему заголовок + fallback, а не что-то одно. Явный `Idempotency-Key` — + предпочтительный путь: клиент генерирует ключ ОДИН раз на "намерение + отправить" и переиспользует его на любом ретрае (fetch retry / переотправка + после таймаута) независимо от того, что именно менялось в UI между + попытками. Без заголовка (старые клиенты, п.5 требования — они не должны + сломаться) используем детерминированный отпечаток sha256(identity, текст, + минутное окно): типичный повтор (двойной клик, ретрай браузера) укладывается + в секунды, минутное окно с запасом это покрывает, а любые два РАЗНЫХ по + смыслу сообщения с одинаковым текстом, отправленные намеренно с разницей в + пару минут, в одно не схлопнутся. + + Хранит и потенциально логирует (см. вызовы в этом файле) только САМ ключ — + он либо непрозрачный клиентский токен, либо хэш. Текст сообщения сюда + попадает ТОЛЬКО как вход в sha256, в открытом виде не сохраняется и не + возвращается — жёсткое правило проекта "текст не в логах" (ПДн) при этом + не нарушается, даже если бы этот ключ где-то залогировали. + + Префиксы `client:`/`auto:` разводят два namespace'а ключей (защита от + случайного совпадения клиентского токена с fallback-хэшем) — дёшево и не + требует отдельной колонки. + """ + header = (request.headers.get(_IDEMPOTENCY_HEADER) or "").strip() + if header and _CLIENT_IDEMPOTENCY_KEY_RE.match(header): + return f"client:{header}" + window = int(time.time() // 60) + fingerprint = f"{identity_key}|{text}|{window}" + return "auto:" + hashlib.sha256(fingerprint.encode("utf-8")).hexdigest() + + @router.post("/support/messages", response_model=SupportMessageOut) async def send_support_message( + request: Request, payload: SupportMessageInput, username: Annotated[str, Depends(_require_username)], db: Annotated[Session, Depends(get_db)], @@ -380,6 +422,28 @@ async def send_support_message( if cooldown is not None: raise _too_many_failures_error(cooldown) + # Идемпотентность (#3471): 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: + existing = storage.find_inbound_by_idempotency_key( + db, thread_id=existing_thread_id, idempotency_key=idempotency_key + ) + if existing is not None: + logger.info( + "web support: repeat send (idempotency key already recorded) " + "username=%s thread_id=%d message_id=%s", + username, + existing_thread_id, + existing["id"], + ) + return SupportMessageOut(**existing) + # Общий клиент приложения (#tg-connection-resilience): на каждый запрос # свой создавать нельзя — это ноль keep-alive и полный TCP+TLS-хендшейк # до api.telegram.org перед каждой отправкой. Живёт в lifespan. @@ -438,6 +502,7 @@ async def send_support_message( text_body=payload.text, topic_message_id=topic_message_id, support_chat_id=settings.telegram_support_chat_id, + idempotency_key=idempotency_key, ) db.commit() except SQLAlchemyError: @@ -608,6 +673,31 @@ async def send_anon_support_message( raise _too_many_failures_error(cooldown) display_id = _anon_display_id(token) + + # Идемпотентность (#3471) — см. развёрнутый комментарий в авторизованной + # ветке. Ограничение специфично для анонимной ветки: если предыдущая + # попытка сама провалилась ДО выдачи куки (`is_new_token=True` тогда и + # сейчас), identity_key каждый раз новый (случайный токен) и pre-check + # структурно не может найти прошлую попытку — это тот же класс проблемы, + # что и потеря куки в сети, вне scope этой правки. + idempotency_key = _resolve_idempotency_key(request, identity_key=thread_key, text=payload.text) + existing_thread_id = storage.find_thread_id(db, thread_key) + if existing_thread_id is not None: + existing = storage.find_inbound_by_idempotency_key( + db, thread_id=existing_thread_id, idempotency_key=idempotency_key + ) + if existing is not None: + logger.info( + "web support (anon): repeat send (idempotency key already recorded) " + "%s thread_id=%d message_id=%s", + display_id, + existing_thread_id, + existing["id"], + ) + if is_new_token: + _set_anon_cookie(response, token) + return SupportMessageOut(**existing) + # Общий клиент приложения (#tg-connection-resilience): на каждый запрос # свой создавать нельзя — это ноль keep-alive и полный TCP+TLS-хендшейк # до api.telegram.org перед каждой отправкой. Живёт в lifespan. @@ -657,6 +747,7 @@ async def send_anon_support_message( text_body=payload.text, topic_message_id=topic_message_id, support_chat_id=settings.telegram_support_chat_id, + idempotency_key=idempotency_key, ) db.commit() except SQLAlchemyError: 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 e1b87a23..feb9ec2b 100644 --- a/tradein-mvp/backend/app/services/tgbot/web_support_storage.py +++ b/tradein-mvp/backend/app/services/tgbot/web_support_storage.py @@ -61,6 +61,36 @@ def get_or_create_thread(db: Session, username: str) -> int: return int(row[0]) +def find_inbound_by_idempotency_key( + db: Session, *, thread_id: int, idempotency_key: str +) -> dict[str, Any] | None: + """Уже записанное inbound-сообщение с этим ключом идемпотентности в треде, + если есть (#3471). Вызывается ИЗ `app.api.v1.support` ДО похода в Telegram + (`send_support_message` / `send_anon_support_message`) — повтор с тем же + ключом не должен создавать второе зеркало в топике, а не только вторую + строку в БД. `thread_id`, а не username/anon-token: таблица не хранит + identity напрямую, а тред уже гарантированно существует к моменту, когда + этот ключ мог быть записан (тред создаётся ДО `record_inbound`, см. H1 в + докстринге `app.api.v1.support`).""" + row = ( + db.execute( + text( + """ + SELECT id, direction, text_body, operator_tg_id, created_at + FROM web_support_messages + WHERE thread_id = CAST(:thread_id AS bigint) + AND direction = 'in' + AND idempotency_key = :idempotency_key + """ + ), + {"thread_id": thread_id, "idempotency_key": idempotency_key}, + ) + .mappings() + .one_or_none() + ) + return dict(row) if row is not None else None + + def record_inbound( db: Session, *, @@ -68,6 +98,7 @@ def record_inbound( text_body: str, topic_message_id: int | None, support_chat_id: int | None, + idempotency_key: str | None = None, ) -> dict[str, Any]: """Записывает сообщение пользователя сайта (direction='in'). `topic_message_id` — id зеркала (sendMessage) в support-топике, ключ маршрутизации ответа оператора. @@ -75,18 +106,33 @@ def record_inbound( review M1): скоупит будущий резолв `find_thread_by_topic_message` к ТЕКУЩЕЙ support-группе — если группу когда-нибудь сменят/пересоздадут, Telegram message_id стартует заново с 1 в новом чате и может совпасть с числом из - старого — без этого поля коллизия была бы ТИХОЙ (см. миграцию 187/188).""" + старого — без этого поля коллизия была бы ТИХОЙ (см. миграцию 187/188). + + `idempotency_key` (#3471, миграция 301) — вызывающая сторона (`app.api.v1.support`) + делает SELECT-затем-действие pre-check ДО Telegram-похода (см. + `find_inbound_by_idempotency_key`), но этот pre-check САМ ПО СЕБЕ гонку не + закрывает (TOCTOU): два запроса с одним ключом могут пройти его одновременно + и оба уйти в Telegram. Последняя линия защиты — здесь: `INSERT ... ON + CONFLICT (thread_id, idempotency_key) DO NOTHING` на partial unique индексе + (мигр. 301, тот же predicate). Если конфликт всё же случился, проигравший + НЕ создаёт вторую строку — читает уже вставленную и возвращает её, так что + оба запроса-конкурента получают ОДИН и тот же id. `idempotency_key=None` + (дефолт) ведёт себя как раньше: NULL никогда не конфликтует сам с собой в + partial-индексе (WHERE idempotency_key IS NOT NULL), INSERT всегда проходит.""" row = ( db.execute( text( """ INSERT INTO web_support_messages (thread_id, direction, text_body, topic_message_id, - support_chat_id, operator_tg_id, created_at) + 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, NOW()) + CAST(:support_chat_id AS bigint), NULL, :idempotency_key, NOW()) + ON CONFLICT (thread_id, idempotency_key) + WHERE idempotency_key IS NOT NULL AND direction = 'in' + DO NOTHING RETURNING id, direction, text_body, operator_tg_id, created_at """ ), @@ -95,12 +141,22 @@ def record_inbound( "text_body": text_body, "topic_message_id": topic_message_id, "support_chat_id": support_chat_id, + "idempotency_key": idempotency_key, }, ) .mappings() - .one() + .one_or_none() ) - return dict(row) + 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 + ) + assert existing is not None # конфликт по индексу гарантирует наличие строки + return existing def find_thread_by_topic_message( diff --git a/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql b/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql new file mode 100644 index 00000000..40b6771b --- /dev/null +++ b/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql @@ -0,0 +1,53 @@ +-- 301_web_support_message_idempotency_key.sql +-- Идемпотентность отправки веб-сообщения в поддержку (#3471 retry-storm): +-- канал Selectel -> api.telegram.org теряет заметную долю коротких запросов +-- (см. app/api/v1/support.py, _INTERACTIVE_SEND_TIMEOUT_S), поэтому повтор +-- клиента (fetch-ретрай/двойной клик/переотправка по таймауту) — обычное +-- дело. Раньше повтор создавал ВТОРУЮ строку в web_support_messages и ВТОРОЕ +-- зеркало в support-топике. +-- +-- idempotency_key — client-provided (заголовок Idempotency-Key) ИЛИ +-- детерминированный fallback-отпечаток sha256(identity|текст|минутное окно) +-- для клиентов без заголовка (app/api/v1/support.py::_resolve_idempotency_key). +-- Всегда opaque-строка (клиентский токен или hex-хэш) — ТЕЛО СООБЩЕНИЯ сюда +-- никогда не попадает в открытом виде, только его отпечаток. +-- +-- Scoped к (thread_id, direction='in'): идемпотентность осмысленна только для +-- inbound (клиентских) сообщений — direction='out' (ответ оператора) её не +-- требует, там уже есть своя partial-уникальность на topic_message_id (187). +-- thread_id, а не username/anon-token: таблица не хранит identity напрямую, +-- а тред уже гарантированно создан (`get_or_create_thread`) к моменту INSERT +-- (см. H1 в app/api/v1/support.py — тред создаётся ДО record_inbound). +-- +-- Гонку двух одновременных запросов с одним ключом закрывает САМ уникальный +-- индекс + `INSERT ... ON CONFLICT DO NOTHING` в +-- web_support_storage.record_inbound (не read-then-write) — на конфликте +-- вызывающая сторона дочитывает уже вставленную строку и возвращает её тем +-- же ответом (тот же id), а не создаёт вторую строку. +-- +-- IDEMPOTENCY: ADD COLUMN/CREATE INDEX IF NOT EXISTS — безопасный re-run. +-- Зависимости: 187_web_support_chat.sql (таблица web_support_messages). + +BEGIN; + +ALTER TABLE web_support_messages + ADD COLUMN IF NOT EXISTS idempotency_key text; + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'web_support_messages_idempotency_key_len_chk' + ) THEN + ALTER TABLE web_support_messages + ADD CONSTRAINT web_support_messages_idempotency_key_len_chk + CHECK (idempotency_key IS NULL OR char_length(idempotency_key) BETWEEN 1 AND 160); + END IF; +END $$; + +COMMENT ON COLUMN web_support_messages.idempotency_key IS 'Ключ идемпотентности inbound-отправки (#3471) — client-provided заголовок Idempotency-Key ("client:...") ИЛИ sha256-отпечаток thread+текст+минутное окно ("auto:...") для клиентов без заголовка. NULL для direction=''out'' и для строк до этой миграции.'; + +CREATE UNIQUE INDEX IF NOT EXISTS web_support_messages_thread_idempotency_uq + ON web_support_messages (thread_id, idempotency_key) + WHERE idempotency_key IS NOT NULL AND direction = 'in'; + +COMMIT; diff --git a/tradein-mvp/backend/tests/test_support.py b/tradein-mvp/backend/tests/test_support.py index 604bcef5..3abb418b 100644 --- a/tradein-mvp/backend/tests/test_support.py +++ b/tradein-mvp/backend/tests/test_support.py @@ -76,6 +76,20 @@ class _FakeTelegramClient: return _FakeTelegramClient._response +@pytest.fixture(autouse=True) +def _no_existing_idempotent_message(monkeypatch: pytest.MonkeyPatch) -> None: + """Дефолт для тестов, которые не проверяют идемпотентность (#3471) напрямую: + без этого автостаба pre-check в `support.py` дёргал бы РЕАЛЬНУЮ + storage-функцию против MagicMock `db` в каждом тесте, где замокан + `find_thread_id` для своих целей (rate-limit / DB-failure / anon-сценарии + и т.д.) — падало бы на MagicMock-результате, никак не связанное с тем, что + тест на самом деле проверяет. Тесты идемпотентности переопределяют это + через `_install_fake_storage` ниже.""" + monkeypatch.setattr( + support_module.storage, "find_inbound_by_idempotency_key", lambda *a, **kw: None + ) + + @pytest.fixture(autouse=True) def _fake_telegram_client(monkeypatch: pytest.MonkeyPatch) -> Any: _FakeTelegramClient.calls = [] @@ -238,7 +252,9 @@ def test_send_message_happy_path_mirrors_with_website_marker( monkeypatch.setattr(support_module.storage, "get_or_create_thread", fake_get_or_create_thread) recorded = {} - def fake_record_inbound(db, *, thread_id, text_body, topic_message_id, support_chat_id): + def fake_record_inbound( + db, *, thread_id, text_body, topic_message_id, support_chat_id, idempotency_key=None + ): recorded.update( thread_id=thread_id, text_body=text_body, @@ -1052,3 +1068,156 @@ def test_anon_send_failures_trigger_cooldown_per_ip( blocked = client.post("/api/v1/trade-in/support/anon/messages", json={"text": "2"}) assert blocked.status_code == 429 assert len(_fake_telegram_client.calls) == calls_before # до Telegram не дошло +# ── idempotency (#3471) ────────────────────────────────────────────────────── + + +class _FakeIdempotentStorage: + """In-memory stand-in для `web_support_storage`, достаточный чтобы точно + воспроизвести идемпотентный контракт (thread_id, idempotency_key) БЕЗ + настоящей БД: unique-конфликт на повторном ключе внутри `record_inbound` + (см. миграцию 301 и storage.record_inbound docstring).""" + + def __init__(self) -> None: + self.threads: dict[str, int] = {} + self.messages: list[dict[str, Any]] = [] + self._next_id = 1 + + def find_thread_id(self, db: Any, username: str) -> int | None: + return self.threads.get(username) + + def get_or_create_thread(self, db: Any, username: str) -> int: + if username not in self.threads: + self.threads[username] = len(self.threads) + 1 + return self.threads[username] + + def find_inbound_by_idempotency_key( + self, db: Any, *, thread_id: int, idempotency_key: str + ) -> dict[str, Any] | None: + for m in self.messages: + if m["_thread_id"] == thread_id and m["_idempotency_key"] == idempotency_key: + return {k: v for k, v in m.items() if not k.startswith("_")} + return None + + def record_inbound( + self, + db: Any, + *, + thread_id: int, + text_body: str, + topic_message_id: int | None, + support_chat_id: int | None, + idempotency_key: str | None = None, + ) -> dict[str, Any]: + # Тот же контракт, что и настоящий `INSERT ... ON CONFLICT DO NOTHING`: + # конфликт по (thread_id, idempotency_key) отдаёт УЖЕ существующую строку. + if idempotency_key is not None: + existing = self.find_inbound_by_idempotency_key( + db, thread_id=thread_id, idempotency_key=idempotency_key + ) + if existing is not None: + return existing + row = { + "id": self._next_id, + "direction": "in", + "text_body": text_body, + "operator_tg_id": None, + "created_at": "2026-09-12T00:00:00+00:00", + "_thread_id": thread_id, + "_idempotency_key": idempotency_key, + } + self._next_id += 1 + self.messages.append(row) + return {k: v for k, v in row.items() if not k.startswith("_")} + + +def _install_fake_storage(monkeypatch: pytest.MonkeyPatch) -> _FakeIdempotentStorage: + fake = _FakeIdempotentStorage() + monkeypatch.setattr(support_module.storage, "find_thread_id", fake.find_thread_id) + monkeypatch.setattr(support_module.storage, "get_or_create_thread", fake.get_or_create_thread) + monkeypatch.setattr( + support_module.storage, + "find_inbound_by_idempotency_key", + fake.find_inbound_by_idempotency_key, + ) + monkeypatch.setattr(support_module.storage, "record_inbound", fake.record_inbound) + return fake + + +def test_repeat_send_with_same_idempotency_header_returns_same_id_single_mirror( + client: TestClient, monkeypatch: pytest.MonkeyPatch, _fake_telegram_client: Any +) -> None: + """Повтор с тем же `Idempotency-Key` — тот же id, ОДНО зеркало в Telegram + (не только одна строка в БД, см. #3471 требование п.3).""" + _install_fake_storage(monkeypatch) + headers = {**_auth("kopylov"), "Idempotency-Key": "retry-abc123"} + + r1 = client.post( + "/api/v1/trade-in/support/messages", json={"text": "первое сообщение"}, headers=headers + ) + r2 = client.post( + "/api/v1/trade-in/support/messages", json={"text": "первое сообщение"}, headers=headers + ) + + assert r1.status_code == 200, r1.text + assert r2.status_code == 200, r2.text + assert r1.json()["id"] == r2.json()["id"] + # Ключевая проверка: повтор НЕ ушёл в Telegram второй раз. + assert len(_fake_telegram_client.calls) == 1 + + +def test_send_with_different_idempotency_keys_creates_different_messages( + client: TestClient, monkeypatch: pytest.MonkeyPatch, _fake_telegram_client: Any +) -> None: + _install_fake_storage(monkeypatch) + r1 = client.post( + "/api/v1/trade-in/support/messages", + json={"text": "вопрос один"}, + headers={**_auth("kopylov"), "Idempotency-Key": "key-one-111"}, + ) + r2 = client.post( + "/api/v1/trade-in/support/messages", + json={"text": "вопрос два"}, + headers={**_auth("kopylov"), "Idempotency-Key": "key-two-222"}, + ) + + assert r1.status_code == 200, r1.text + assert r2.status_code == 200, r2.text + assert r1.json()["id"] != r2.json()["id"] + assert len(_fake_telegram_client.calls) == 2 + + +def test_repeat_send_without_idempotency_header_still_dedupes_via_fallback_window( + client: TestClient, monkeypatch: pytest.MonkeyPatch, _fake_telegram_client: Any +) -> None: + """Требование п.5: отсутствие явного ключа не должно ломать старых клиентов — + сервер сам считает детерминированный fallback-отпечаток (thread+текст+минутное + окно), так что тот же повтор ТЕМ ЖЕ клиентом без заголовка тоже не создаёт + дубль. Время фиксируем, чтобы не зависеть от границы минутного окна в CI.""" + _install_fake_storage(monkeypatch) + monkeypatch.setattr(support_module.time, "time", lambda: 1_800_000_000.0) + + r1 = client.post( + "/api/v1/trade-in/support/messages", json={"text": "не могу войти"}, headers=_auth("bob") + ) + r2 = client.post( + "/api/v1/trade-in/support/messages", json={"text": "не могу войти"}, headers=_auth("bob") + ) + + assert r1.status_code == 200, r1.text + assert r2.status_code == 200, r2.text + assert r1.json()["id"] == r2.json()["id"] + assert len(_fake_telegram_client.calls) == 1 + + +def test_send_without_idempotency_header_first_call_still_succeeds( + client: TestClient, monkeypatch: pytest.MonkeyPatch, _fake_telegram_client: Any +) -> None: + """Требование п.5 (не ломает старых клиентов): один запрос без заголовка + отрабатывает как раньше — 200, тред создан, зеркало отправлено ровно раз.""" + _install_fake_storage(monkeypatch) + r = client.post( + "/api/v1/trade-in/support/messages", json={"text": "просто вопрос"}, headers=_auth("carol") + ) + assert r.status_code == 200, r.text + assert r.json()["persisted"] is True + assert len(_fake_telegram_client.calls) == 1 From 1b595a00911747e6d755f9ff75cea9a77a81f6ca Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 12 Sep 2026 15:02:23 +0300 Subject: [PATCH 2/3] =?UTF-8?q?fix(sql):=20lock=5Ftimeout=20=D1=83=20?= =?UTF-8?q?=D0=BC=D0=B8=D0=B3=D1=80=D0=B0=D1=86=D0=B8=D0=B8=20=D0=BA=D0=BB?= =?UTF-8?q?=D1=8E=D1=87=D0=B0=20=D0=B8=D0=B4=D0=B5=D0=BC=D0=BF=D0=BE=D1=82?= =?UTF-8?q?=D0=B5=D0=BD=D1=82=D0=BD=D0=BE=D1=81=D1=82=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Проверка миграций в CI требует его для блокирующего DDL: без ограничения ALTER встаёт в очередь за чужой сессией и уводит за собой все последующие обращения к таблице (#2752). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JY6iWDnGDthdvsMWgK1BMG --- .../data/sql/301_web_support_message_idempotency_key.sql | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql b/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql index 40b6771b..774a5bd3 100644 --- a/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql +++ b/tradein-mvp/backend/data/sql/301_web_support_message_idempotency_key.sql @@ -30,6 +30,12 @@ BEGIN; +-- Блокирующий DDL не должен ждать чужую сессию бесконечно: без этого +-- ALTER встаёт в очередь за долгим запросом и уводит за собой ВСЕ +-- последующие обращения к таблице (#2752). Пять секунд — не успел взять +-- лок, деплой падает честно, а прод продолжает работать. +SET LOCAL lock_timeout = '5s'; + ALTER TABLE web_support_messages ADD COLUMN IF NOT EXISTS idempotency_key text; From d5c876e3d0b9c1ca87b9768eee92ce96ceaf90b9 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 12 Sep 2026 15:24:21 +0300 Subject: [PATCH 3/3] =?UTF-8?q?fix(tradein):=20=D0=B2=D0=BA=D0=BB=D1=8E?= =?UTF-8?q?=D1=87=D0=B0=D0=B5=D0=BC=20idempotency-key=20=D0=BD=D0=B0=20?= =?UTF-8?q?=D1=84=D1=80=D0=BE=D0=BD=D1=82=D0=B5,=20=D1=87=D0=B8=D0=BD?= =?UTF-8?q?=D0=B8=D0=BC=20assert-crash=20=D0=B8=20=D1=87=D0=B5=D1=81=D1=82?= =?UTF-8?q?=D0=BD=D0=BE=D1=81=D1=82=D1=8C=20=D0=B4=D0=BE=D0=BA=D1=81=D1=82?= =?UTF-8?q?=D1=80=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) }); }, });