diff --git a/tradein-mvp/backend/app/tasks/geocode_missing.py b/tradein-mvp/backend/app/tasks/geocode_missing.py index 5bad11f3..bcb1d18d 100644 --- a/tradein-mvp/backend/app/tasks/geocode_missing.py +++ b/tradein-mvp/backend/app/tasks/geocode_missing.py @@ -12,6 +12,14 @@ Pattern: dedup по паре (address, city) — 1 уникальная пара не схлопываться в один geocode-вызов и один UPDATE по тексту адреса). Rate limit: Nominatim 1 req/sec (#2593: Yandex Geocoder tier удалён из geocoder). +SELECT фильтрует `is_active` (#2604 п.1): на проде очередь была на 98.5% забита +мёртвыми объявлениями чужих регионов (Новосибирск/Казань/Челябинск/…) без is_active — +`ORDER BY listings_count DESC` ставил их В НАЧАЛО (у мусорного адреса вида +«Новосибирская обл.,Новосибирск» — сотни listings, у реального адреса — 1-2), поэтому +весь batch-бюджет (Nominatim 1 req/sec) съедался мусором и до настоящих адресов дело +не доходило (8 ночных прогонов подряд: saved=0). UPDATE после успешного/неуспешного +geocode НЕ фильтрует is_active — см. комментарии у соответствующих UPDATE ниже. + Отличие от /admin/geocode-missing (per-ID): - Этот модуль группирует по (address, city) → меньше API calls (dedup), но не схлопывает разные города с одинаковым текстом адреса. @@ -59,11 +67,14 @@ async def geocode_missing_listings( """Geocode listings с NULL coords (любой source). Steps: - 1. SELECT address, city FROM listings WHERE lat IS NULL AND address IS NOT NULL - GROUP BY address, city ORDER BY COUNT(*) DESC LIMIT batch_size + 1. SELECT address, city FROM listings WHERE lat IS NULL AND is_active + AND address IS NOT NULL GROUP BY address, city ORDER BY COUNT(*) DESC + LIMIT batch_size (приоритет парам address+city с большим числом listings — больший ROI per geocode call; группировка по паре, НЕ только по address — #2594 шаг 2/3: - один и тот же текст адреса в разных городах — разные записи) + один и тот же текст адреса в разных городах — разные записи. `is_active` — + #2604 п.1: не тратим Nominatim-бюджет на мёртвые объявления, которые никогда + не попадут в выдачу пользователю) 2. Для каждой пары (address, city): - geocode(address, db, city_hint=city) — auto-cache (hit или miss) @@ -98,6 +109,14 @@ async def geocode_missing_listings( # переезд адреса в кэше или смена провайдера), либо tried_at IS NULL (ещё не пробовали). # Это делает функцию loop-safe: при вызове несколько раз в одном прогоне # failed-пары не переотбираются бесконечно. + # + # AND is_active (#2604 п.1) — очередь без этого фильтра на 98.5% состояла из + # is_active=false объявлений чужих регионов (Новосибирск/Казань/Челябинск/…), + # а ORDER BY listings_count DESC ставил самый мусорный адрес («Новосибирская + # обл.,Новосибирск», сотни listings) В НАЧАЛО — весь batch съедался мусором, + # который пользователь никогда не увидит (is_active=false), 8 ночных прогонов + # подряд saved=0. Активные объявления с валидным адресом почти всегда попадают + # в topN только теперь, когда мусор не конкурирует за место в LIMIT. rows = ( db.execute( text( @@ -105,6 +124,7 @@ async def geocode_missing_listings( SELECT address, city, COUNT(*) AS listings_count FROM listings WHERE lat IS NULL + AND is_active AND address IS NOT NULL AND length(trim(address)) >= 5 AND (geocode_tried_at IS NULL @@ -150,6 +170,14 @@ async def geocode_missing_listings( # IS NOT DISTINCT FROM — city=NULL это отдельная группа, обычное # `=` не поймает NULL-город и не должно задеть другой город с тем # же текстом адреса. + # Намеренно БЕЗ `AND is_active` (#2604 п.2): tried_at — backoff-метка + # для (address, city) КАК ТЕКСТА, а не для конкретного listing. + # is_active=false дубликат этой пары и так никогда не будет выбран + # SELECT'ом заново (is_active=false исключён там навсегда) — фильтр + # здесь был бы no-op для неактивных строк. Единственный случай когда + # это имеет значение — если строка позже реактивируется (is_active + # → true): тогда tried_at уже стоит и backoff корректно защищает от + # немедленного повторного запроса того же заведомо неудачного адреса. db.execute( text( "UPDATE listings SET geocode_tried_at = NOW()" @@ -171,6 +199,11 @@ async def geocode_missing_listings( ) if not dry_run: # Пометить tried_at — geocoder не нашёл адрес, backoff 7 дней. + # Намеренно БЕЗ `AND is_active` (#2604 п.2) — то же обоснование, что + # и в except-ветке выше: backoff привязан к тексту (address, city), + # не к конкретному listing, is_active=false строка и так не выбирается + # SELECT'ом заново; при реактивации backoff корректно защитит от + # немедленного повтора заведомо неудачного запроса. db.execute( text( "UPDATE listings SET geocode_tried_at = NOW()" @@ -211,6 +244,18 @@ async def geocode_missing_listings( # city IS NOT DISTINCT FROM :city — обновляем ТОЛЬКО пару (address, city), из # которой был geocode-запрос; иначе тот же текст адреса в другом городе # (city IS NULL или другой явный город) перезаписался бы чужими координатами. + # + # Намеренно БЕЗ `AND is_active` (#2604 п.1): координаты — свойство физического + # адреса, а не свойство конкретного объявления. Если у этой же пары + # (address, city) есть is_active=false дубликат с lat IS NULL, он получит те же + # координаты бесплатно — Nominatim-вызов уже оплачен геокодом активного + # листинга, доп. запроса не будет. SELECT выше и так навсегда исключает + # is_active=false строки из очереди — без этого UPDATE такой дубликат остался + # бы с NULL lat/lon НАВСЕГДА (переезд в EKB-only локальные реестры/analytics по + # координатам сломан для него), хотя ответ уже есть в руках. Единственный + # довод «за» фильтр — консистентность с SELECT — не перевешивает: это не + # ошибка данных (координаты адреса объективны и не зависят от активности), + # а чистый выигрыш (та же строка при реактивации уже готова, доп. cost = 0). update_result = db.execute( text( """ @@ -327,6 +372,13 @@ async def run_geocode_missing_listings( ) break if res.addresses_total < batch_size: + # #2604 п.3: с is_active-фильтром в SELECT очередь резко уже (была + # 14294 строк/98.5% мёртвых, стало ~220 активных → десятки уникальных + # пар address+city после GROUP BY) — этот дренаж почти всегда сработает + # уже на первой итерации (addresses_total < default batch_size=200), и + # это ПРАВИЛЬНОЕ поведение: разгребли всё что было, ждём следующего + # прогона. Никакого деления тут нет (только сравнение int), пустая + # очередь (addresses_total=0) ловится веткой выше, а не этой. logger.info( "run_geocode_missing_listings: run_id=%d — дренаж " "(addresses_total=%d < batch_size=%d), завершаем", diff --git a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py index ac3c6ede..c14cd8e1 100644 --- a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py +++ b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py @@ -342,6 +342,31 @@ async def test_geocode_missing_recent_tried_at_excluded_via_where() -> None: assert "7 days" in sql_text +@pytest.mark.asyncio +async def test_geocode_missing_select_filters_is_active() -> None: + """SELECT содержит `AND is_active` (#2604 п.1). + + На проде очередь без этого фильтра была на 98.5% забита is_active=false + объявлениями чужих регионов (Новосибирск/Казань/Челябинск/…) без улицы и дома; + `ORDER BY listings_count DESC` ставил самый мусорный адрес («Новосибирская + обл.,Новосибирск», 214 listings) В НАЧАЛО очереди — весь Nominatim-бюджет + (1 req/sec) съедался мусором, до реальных активных адресов дело не доходило + (8 ночных прогонов подряд: saved=0). Falsification-проба: на коде ДО фикса + `"AND is_active" in sql_text` ложно, тест падает; после фикса проходит. + """ + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = [] + db.execute.return_value = select_result + + with patch("app.tasks.geocode_missing.geocode", new_callable=AsyncMock): + await geocode_missing_listings(db, batch_size=10) + + first_call = db.execute.call_args_list[0] + sql_text = str(first_call[0][0]) + assert "AND is_active" in sql_text + + @pytest.mark.asyncio async def test_run_geocode_missing_listings_terminates_on_drained() -> None: """run_geocode_missing_listings завершается когда addresses_total == 0 (ничего pending).""" @@ -560,6 +585,91 @@ async def test_geocode_missing_failed_pair_tried_at_update_scoped_to_city() -> N assert params["city"] == "Нижний Тагил" +# ── #2604 п.1/п.2: UPDATE decisions — locked in by test, not just comment ──── + + +@pytest.mark.asyncio +async def test_geocode_missing_success_update_not_filtered_by_is_active() -> None: + """Decision #2604 п.1 (UPDATE lat/lon): намеренно БЕЗ `is_active` в WHERE. + + Координаты — свойство физического адреса (address, city), не свойство + конкретного listing. is_active=false дубликат ЭТОЙ ЖЕ пары никогда не будет + независимо отобран SELECT'ом (он навсегда исключён оттуда) — без unfiltered + UPDATE такой дубликат остался бы с NULL lat/lon навсегда, хотя ответ уже + получен и оплачен Nominatim-вызовом активного листинга. + """ + rows = [{"address": "ул. Тестовая, 1", "city": "Екатеринбург", "listings_count": 2}] + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + update_result = MagicMock() + update_result.rowcount = 2 + db.execute.side_effect = [select_result, update_result] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result("nominatim"), + ): + await geocode_missing_listings(db, batch_size=200) + + update_call = db.execute.call_args_list[1] + sql = str(update_call.args[0]) + assert "is_active" not in sql + + +@pytest.mark.asyncio +async def test_geocode_missing_notfound_tried_at_update_not_filtered_by_is_active() -> None: + """Decision #2604 п.2 (geo is None → tried_at UPDATE): намеренно БЕЗ `is_active`. + + tried_at — backoff-метка для (address, city) КАК ТЕКСТА, не для конкретного + listing; is_active=false дубликат и так никогда не переотбирается SELECT'ом. + Единственный сценарий где это важно — реактивация (is_active → true) той же + строки: backoff уже стоит и корректно защищает от немедленного повтора + заведомо неудачного адреса. + """ + rows = [{"address": "несуществующий адрес", "city": None, "listings_count": 1}] + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + tried_at_result = MagicMock() + db.execute.side_effect = [select_result, tried_at_result] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=None, + ): + await geocode_missing_listings(db, batch_size=200) + + update_call = db.execute.call_args_list[1] + sql = str(update_call.args[0]) + assert "is_active" not in sql + + +@pytest.mark.asyncio +async def test_geocode_missing_exception_tried_at_update_not_filtered_by_is_active() -> None: + """Decision #2604 п.2 (geocode() raises → tried_at UPDATE): та же логика, что и + в NOT-FOUND ветке выше — намеренно БЕЗ `is_active`, зафиксировано тестом.""" + rows = [{"address": "ул. Битая, 99", "city": None, "listings_count": 1}] + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + tried_at_result = MagicMock() + db.execute.side_effect = [select_result, tried_at_result] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + side_effect=RuntimeError("timeout"), + ): + await geocode_missing_listings(db, batch_size=200) + + update_call = db.execute.call_args_list[1] + sql = str(update_call.args[0]) + assert "is_active" not in sql + + # ── Integration-style: estimator Avito exclusion removed ─────────────────────