lekss361/gendesign

Fork 0

backend/weather_cache: тест требует single-flight, которого код НЕ обещает — гейт красит PR-ы по таймингу #2783

New issue

Closed

opened 2026-08-07 09:22:10 +00:00 by bot-backend · 1 comment

bot-backend commented

2026-08-07 09:22:10 +00:00

Collaborator

Поймано на чужом PR (#2781, CI / backend-tests, run 6961): 1 failed, 4637 passed. Упавший тест к диффу того PR отношения не имеет — диффа там два файла, оба про парсер КРТ.

tests/services/test_weather_cache.py:283: AssertionError
E  ожидался 1 сетевой вызов, было 2
E  assert 2 == 1

Это НЕ флейк и НЕ регресс. Тест утверждает то, чего в коде нет

Тест (test_weather_cache.py:283) запускает 16 потоков через threading.Barrier и требует:

# Single-flight под lock'ом + check-then-fetch — РОВНО один реальный вызов.
assert get_call_count == 1, f"ожидался 1 сетевой вызов, было {get_call_count}"

А реализация (app/services/weather_cache.py:279-292) прямым текстом говорит обратное:

with _FORECAST_LOCK:
    entry = _FORECAST_CACHE.get(key)
    if entry is not None and entry[1] > now:
        return entry[0]
# MISS или истёк → сетевой вызов ВНЕ lock'а ... Ценой — cold-start на ОДИН ключ
# может породить НЕСКОЛЬКО ПАРАЛЛЕЛЬНЫХ ЗАПРОСОВ (last-write wins при store ниже),
# что приемлемо: вызовы идемпотентны, TTL длинный.
value = _fetch_weather_remote(lat, lon)

Лок держится только на чтении кэша и только на записи. Между ними — сеть без лока. Single-flight'а нет и он снят сознательно (#1370: иначе все analyze сериализуются на время httpx-вызова, даже для разных координат).

То есть тест зеленеет не потому, что защита работает, а потому что при GIL и мгновенном моке первый поток обычно успевает сложить результат в кэш до того, как остальные дойдут до проверки. На нагруженном раннере не успевает — и тест честно показывает ровно то поведение, которое описано в комментарии к коду.

Локально 5 прогонов подряд — 20 passed. Ровно поэтому глазами это и не видно.

Почему это стоит закрыть

Пока так, у гейта есть проверка, которая красит ЧУЖИЕ PR-ы по таймингу раннера. Дальше по накатанной: «ну это же тот флейкающий тест, перезапусти» — и рядом однажды проедет настоящая красота. Молча выключить (deselect/xfail без причины) нельзя — это тот же класс дефекта, что #2722.

Развилка — нужно решение, а не только код

Починить тест под фактический контракт. Утверждать то, что код правда гарантирует: все 16 результатов одинаковы, в кэше ровно одна запись, вызовов ≥1 и ≤16. Дёшево, честно, гейт перестаёт врать. Комментарий про «single-flight под lock'ом» убрать — он неверен.
Реализовать настоящий single-flight — per-key lock ({key: Lock}), тогда разные координаты по-прежнему не сериализуются, а один ключ фетчится один раз. Тест остаётся как есть и начинает проверять реальную гарантию. Дороже, но убирает и лишние обращения к Open-Meteo на cold-start.

По умолчанию разумнее (1): текущее поведение объявлено приемлемым в #1370 явно и с обоснованием, менять его без запроса не за чем.

То же место, тот же вопрос

get_seasonal_weather_cached (строки 302-314) устроен идентично. Если выбирается вариант 2 — чинить оба; если вариант 1 — проверить, нет ли у него теста с таким же утверждением.

Поймано на чужом PR (#2781, `CI / backend-tests`, run 6961): `1 failed, 4637 passed`. Упавший тест к диффу того PR отношения не имеет — диффа там два файла, оба про парсер КРТ. ``` tests/services/test_weather_cache.py:283: AssertionError E ожидался 1 сетевой вызов, было 2 E assert 2 == 1 ``` ## Это НЕ флейк и НЕ регресс. Тест утверждает то, чего в коде нет Тест (`test_weather_cache.py:283`) запускает 16 потоков через `threading.Barrier` и требует: ```python # Single-flight под lock'ом + check-then-fetch — РОВНО один реальный вызов. assert get_call_count == 1, f"ожидался 1 сетевой вызов, было {get_call_count}" ``` А реализация (`app/services/weather_cache.py:279-292`) прямым текстом говорит обратное: ```python with _FORECAST_LOCK: entry = _FORECAST_CACHE.get(key) if entry is not None and entry[1] > now: return entry[0] # MISS или истёк → сетевой вызов ВНЕ lock'а ... Ценой — cold-start на ОДИН ключ # может породить НЕСКОЛЬКО ПАРАЛЛЕЛЬНЫХ ЗАПРОСОВ (last-write wins при store ниже), # что приемлемо: вызовы идемпотентны, TTL длинный. value = _fetch_weather_remote(lat, lon) ``` Лок держится только на чтении кэша и только на записи. Между ними — сеть без лока. **Single-flight'а нет и он снят сознательно** (#1370: иначе все `analyze` сериализуются на время httpx-вызова, даже для разных координат). То есть тест зеленеет не потому, что защита работает, а потому что при GIL и мгновенном моке первый поток обычно успевает сложить результат в кэш до того, как остальные дойдут до проверки. На нагруженном раннере не успевает — и тест честно показывает ровно то поведение, которое описано в комментарии к коду. Локально 5 прогонов подряд — 20 passed. Ровно поэтому глазами это и не видно. ## Почему это стоит закрыть Пока так, у гейта есть проверка, которая красит ЧУЖИЕ PR-ы по таймингу раннера. Дальше по накатанной: «ну это же тот флейкающий тест, перезапусти» — и рядом однажды проедет настоящая красота. Молча выключить (`deselect`/`xfail` без причины) нельзя — это тот же класс дефекта, что #2722. ## Развилка — нужно решение, а не только код 1. **Починить тест под фактический контракт.** Утверждать то, что код правда гарантирует: все 16 результатов одинаковы, в кэше ровно одна запись, вызовов `≥1` и `≤16`. Дёшево, честно, гейт перестаёт врать. Комментарий про «single-flight под lock'ом» убрать — он неверен. 2. **Реализовать настоящий single-flight** — per-key lock (`{key: Lock}`), тогда разные координаты по-прежнему не сериализуются, а один ключ фетчится один раз. Тест остаётся как есть и начинает проверять реальную гарантию. Дороже, но убирает и лишние обращения к Open-Meteo на cold-start. По умолчанию разумнее (1): текущее поведение объявлено приемлемым в #1370 явно и с обоснованием, менять его без запроса не за чем. ## То же место, тот же вопрос `get_seasonal_weather_cached` (строки 302-314) устроен идентично. Если выбирается вариант 2 — чинить оба; если вариант 1 — проверить, нет ли у него теста с таким же утверждением.

bot-backend referenced this issue

2026-08-07 09:31:02 +00:00

test(scrapers/krt): вернуть в прогон проверку многоблочной страницы (#2778) #2781

Light1YT referenced this issue from a commit

2026-08-07 10:12:43 +00:00

test(ci): развести «нагрузку не создать» и «защита сломана» в двух гейтах (#2783)

bot-backend referenced this issue

2026-08-07 10:13:39 +00:00

test(ci): развести «нагрузку не создать» и «защита сломана» в двух флапающих гейтах (#2783) #2789

bot-backend referenced this issue from a commit

2026-08-07 10:35:18 +00:00

test(ci): развести «нагрузку не создать» и «защита сломана» в двух флапающих гейтах (#2783) (#2789)

bot-backend commented

2026-08-07 10:36:50 +00:00

Author

Collaborator

Закрыто. Диагноз подтверждён: подводило само утверждение, а не тайминг

Замер (200 «штормов» на каждое условие):

условие	распределение числа вызовов
вхолостую, обычный `switchinterval`	`1` × 199, `2` × 1 → ~0.5% прогонов красные
под нагрузкой load ~120	то же самое — нагрузка ни при чём
`setswitchinterval(1e-6)`	больше одного вызова в 197 из 200, все 16 — в 173 из 200

То есть утверждение «ровно один вызов» ложно почти всегда, когда гонка вообще случается. Зелёным тест бывал лишь потому, что при обычном интервале переключения первый поток успевает раньше.

Правится тест, не код. Поведение объявлено приемлемым в #1370 с обоснованием: лишние запросы бывают только на холодном старте одного ключа и идемпотентны. Замок на ключ — отдельная задача с отдельным обоснованием, а не побочный эффект починки теста.

Тест теперь утверждает настоящий контракт: одно значение у всех потоков, число вызовов в границах 1 ≤ n ≤ 16, ровно один округлённый ключ, и после шторма кэш отвечает без сети. Гонка сделана неслучайной (задержка в подставном источнике) — 30 штормов из 30 дают ровно 16 вызовов, то есть верхняя граница проверяется на самой границе, а не в среднем.

Доказано, что краснеет — три разные поломки, три разных сообщения: потерянная запись, чужой ключ, мёртвый TTL.

PR #2789 (e4680082), обе ветки CI зелёные.

Соседний гейт из того же PR — и остаток, который я оставляю с критерием

test_login_flood_capped_by_rate… (падал на assert 100 > 100) чинился иначе, потому что там подводило предусловие, а не утверждение:

нагрузка раннера	падений
свободная машина	0 / 20
load ~90	0 / 20
load ~185	12 / 15, из них 10 ровно на этом `assert`

Корень: срок проверяется перед запросом, и на занятом раннере первый круг из ста запросов съедает всю секунду — каждое соединение отвечает ровно раз.

Продлевать окно было бы бесполезно: предлагаемый темп — свойство машины, а не длительности. Предусловие сделано содержательным: флуд обязан предлагать больше того же порога, с которым сверяется вердикт. Пока предлагает меньше, следующее утверждение зелено даже на системе вовсе без потолка — то есть измерения нет, и красный честно говорит «раннер не потянул», а не «потолок сломан».

Остаток, названный вслух

Под load ~185 тест продолжает изредка падать на первом утверждении («худший сторонний запрос дольше 500 мс») — 5 из 15 после правки. Диагноз честный: при такой нагрузке «API встаёт» неотличимо от «машина встала».

На нагрузке, где ловилось предусловие (load ~90), эти утверждения не упали ни разу за 20 прогонов, поэтому в CI они пока не проявлялись.

Критерий приёмки, записанный до факта: если этот гейт снова покрасит чужой PR — то есть PR, чей дифф не касается ни auth, ни password, — считать остаток проявившимся и чинить первое утверждение отдельно. Срок пересмотра, если не проявится: 2026-09-07, чтобы «пока не воспроизводится» не жило бессрочно.

## Закрыто. Диагноз подтверждён: подводило само утверждение, а не тайминг Замер (200 «штормов» на каждое условие): | условие | распределение числа вызовов | |---|---| | вхолостую, обычный `switchinterval` | `1` × 199, `2` × 1 → **~0.5% прогонов красные** | | под нагрузкой load ~120 | то же самое — **нагрузка ни при чём** | | `setswitchinterval(1e-6)` | больше одного вызова в **197 из 200**, все 16 — в **173 из 200** | То есть утверждение «ровно один вызов» ложно почти всегда, когда гонка вообще случается. Зелёным тест бывал лишь потому, что при обычном интервале переключения первый поток успевает раньше. **Правится тест, не код.** Поведение объявлено приемлемым в #1370 с обоснованием: лишние запросы бывают только на холодном старте одного ключа и идемпотентны. Замок на ключ — отдельная задача с отдельным обоснованием, а не побочный эффект починки теста. Тест теперь утверждает настоящий контракт: одно значение у всех потоков, число вызовов в границах `1 ≤ n ≤ 16`, ровно один округлённый ключ, и **после шторма кэш отвечает без сети**. Гонка сделана неслучайной (задержка в подставном источнике) — 30 штормов из 30 дают ровно 16 вызовов, то есть верхняя граница проверяется на самой границе, а не в среднем. **Доказано, что краснеет** — три разные поломки, три разных сообщения: потерянная запись, чужой ключ, мёртвый TTL. PR #2789 (`e4680082`), обе ветки CI зелёные. --- ## Соседний гейт из того же PR — и остаток, который я оставляю с критерием `test_login_flood_capped_by_rate…` (падал на `assert 100 > 100`) чинился иначе, потому что там подводило **предусловие**, а не утверждение: | нагрузка раннера | падений | |---|---| | свободная машина | **0 / 20** | | load ~90 | **0 / 20** | | load ~185 | **12 / 15**, из них 10 ровно на этом `assert` | Корень: срок проверяется **перед** запросом, и на занятом раннере первый круг из ста запросов съедает всю секунду — каждое соединение отвечает ровно раз. Продлевать окно было бы бесполезно: предлагаемый темп — свойство машины, а не длительности. Предусловие сделано **содержательным**: флуд обязан предлагать больше того же порога, с которым сверяется вердикт. Пока предлагает меньше, следующее утверждение зелено даже на системе вовсе без потолка — то есть измерения нет, и красный честно говорит «раннер не потянул», а не «потолок сломан». ### Остаток, названный вслух Под load ~185 тест **продолжает изредка падать на первом утверждении** («худший сторонний запрос дольше 500 мс») — 5 из 15 после правки. Диагноз честный: при такой нагрузке «API встаёт» неотличимо от «машина встала». На нагрузке, где ловилось предусловие (load ~90), эти утверждения не упали ни разу за 20 прогонов, поэтому в CI они пока не проявлялись. **Критерий приёмки, записанный до факта:** если этот гейт снова покрасит чужой PR — то есть PR, чей дифф не касается ни `auth`, ни `password`, — считать остаток проявившимся и чинить первое утверждение отдельно. Срок пересмотра, если не проявится: **2026-09-07**, чтобы «пока не воспроизводится» не жило бессрочно.

bot-backend closed this issue

2026-08-07 10:36:51 +00:00