fix(tradein/deactivate): validate cap_mult, calibrate avito, document scope gap
Round-2 review (MAJOR) left three items open: 1. cap_mult was threaded through as a jsonb default_params parameter but never validated, reproducing the exact ttl_days<=0 hole the earlier guard closed. Verified live: cap_mult=0 -> effective_ttl=0 -> whole active pool of the source would deactivate; cap_mult=0.5 pushes the ceiling BELOW the operator- configured ttl_days. Added `if cap_mult < 1: raise ValueError` next to the ttl_days guard (same fail-fast contract, before any SQL). Non-numeric values (e.g. a stringly-typed "6" from a typo in default_params) already fail safe via TypeError on the comparison, caught by the same except-block -> mark_failed. Covered with 5 new tests (zero/negative/<1/non-numeric/mark_failed routing). 2. The mechanical part of cap_mult (parameter + wiring) was merged but never calibrated for avito on prod -- no migration shipped, so prod default_params for deactivate_stale_avito still lacked "cap_mult" and ran with the module default (CAP_MULT=2, ceiling=20d), which is BELOW avito's own p99 revisit gap (42.1d) and below the observed prod peak (floor=52, three runs 08-10..08-12). Added data/sql/264_deactivate_stale_avito_cap_mult.sql (idempotent, same pattern as 219) setting cap_mult=6 for deactivate_stale_avito only (ceiling 60d, matching the order of magnitude already used for cian/yandex). cian/ yandex/domklik keep the CAP_MULT=2 default -- their p99 gaps (26.6/43.0/3.1) sit comfortably under their default ceilings (60/60/28), no override needed. Pinned the calibration with a dedicated test (test_avito_prod_floor_is_capped_by_calibrated_cap_mult) instead of leaving the avito slice skipped in the false-kill coverage test. 3. Confirmed (SSH read-only, prod counts): active rows aged >60d that this PR cannot touch regardless of cap_mult -- cian/novostroyki 9483, cian/NULL 211, yandex/NULL 523 (0 inside the jobs' actual scope: cian/vtorichka, yandex/vtorichka). deactivate_stale_cian/_yandex are scoped to segments=['vtorichka'] by a deliberate, documented DECISION (blanket TTL on novostroyki risks killing live inventory cian/yandex don't fully sweep). Widening that scope is a separate, riskier investigation and is out of scope here -- documented the gap directly in the module docstring next to the existing DECISION so it isn't lost. Verification (SSH read-only against prod, 2026-08-15): recomputed the exact per-source formula the next scheduled run will use. In-scope next-run deactivation is currently 0 for all four sources -- the active pool has already self-corrected to be consistent with each source's own recent effective TTL (yesterday's yandex run used effective=54, so no active row is older than that yet). This matches the round-2 reviewer's own conclusion: the cap is a preventative guardrail, not a retroactive cleanup, and isn't expected to fire on the exact day it's calibrated. It is not idle, though -- live recompute of yandex/vtorichka's raw (uncapped) floor right now is 78.2d, already above its 60d ceiling; the trailing 6-day counters show the identical loop (floor=75, deactivated=0, three days straight) already recurred twice without this cap in place. The mechanism will bind the moment the pool ages past the ceiling, which is exactly the recurrence it exists to stop. Tests: 106 passed (test_deactivate_stale_ttl_cap.py, test_deactivate_stale_revisit_floor.py, test_deactivate_stale_health_gate.py, test_deactivate_stale_listings.py, test_migrations_manifest.py). ruff clean. scripts/check-migration-lock-timeout.py: pass (UPDATE-only migration, no blocking DDL, no SET LOCAL needed).
This commit is contained in:
bot-backend
parent
bef2a05f60
commit
772ae116b5
5 changed files with 188 additions and 8 deletions
|
|
@ -7,6 +7,16 @@
|
|||
- Cian/Yandex не поддерживают full-coverage sweep -> паушальный TTL сломает живой
|
||||
инвентарь. DECISION: для yandex/cian деактивировать ТОЛЬКО listing_segment='vtorichka',
|
||||
TTL=30. novostroyki (9659 активных первичных строк) и NULL-сегмент не трогаем.
|
||||
ИЗВЕСТНЫЙ ПРОБЕЛ (ревью TTL-CAP круг 2, 2026-08-15): этот скоуп уже, чем множество
|
||||
реально протухших строк -- живой замер на проде даёт cian/novostroyki 9 483,
|
||||
cian/NULL-сегмент 211, yandex/NULL-сегмент 523 активных строки старше 60 суток, ни
|
||||
одна из них не деактивируется НИ ОДНОЙ джобой (внутри скоупа cian/vtorichka и
|
||||
yandex/vtorichka таких строк 0). Потолок cap_mult (см. CAP_MULT ниже) этот пробел
|
||||
не закрывает и закрыть не может -- он сжимает пул ВНУТРИ скоупа джобы, а не
|
||||
расширяет сам скоуп. Расширение скоупа -- отдельная задача (нужно сперва выяснить,
|
||||
поддерживают ли cian/yandex full-coverage sweep для novostroyki/NULL-сегмента
|
||||
СЕЙЧАС, иначе паушальный TTL повторит инцидент, ради которого этот DECISION и
|
||||
принят) и намеренно НЕ входит в TTL-CAP.
|
||||
- avito: все сегменты (segments=None), TTL=10 дней -- поведение без изменений.
|
||||
- Строки НЕ удаляются -- история нужна для бэктеста (#667).
|
||||
- #2674: деактивация в той же транзакции пишет снимок listings_snapshots со статусом
|
||||
|
|
@ -408,13 +418,31 @@ def deactivate_stale_listings(
|
|||
ttl_days<=0 в WHERE-условии last_seen_at < NOW() - INTERVAL 'N days'
|
||||
матчит практически весь активный пул -- без явного guard'а потолок
|
||||
(ttl_days * cap_mult <= 0) к тому же перебивал бы пол в формуле min(),
|
||||
снимая защиту, которую max(ttl_days, floor) давал раньше).
|
||||
снимая защиту, которую max(ttl_days, floor) давал раньше), ИЛИ cap_mult < 1
|
||||
(тот же класс дыры, но со стороны потолка, а не пола: cap_mult приходит из
|
||||
jsonb default_params расписания -- ЕДИНСТВЕННЫЙ запланированный способ его
|
||||
задать, т.е. именно там опечатка 0 / 0.5 вместо 6 доходит до прода. cap_mult=0
|
||||
даёт capped=0 -> effective_ttl_days=0 -> UPDATE снимает практически весь
|
||||
активный пул источника; cap_mult<1 (например 0.5) опускает потолок НИЖЕ
|
||||
заданного оператором ttl_days -- прямое нарушение инварианта «потолок не
|
||||
может понизить TTL ниже настроенного», который проверяет
|
||||
test_cap_never_lowers_ttl_below_configured_value).
|
||||
"""
|
||||
counters: dict[str, int] = {"deactivated": 0}
|
||||
try:
|
||||
if ttl_days <= 0:
|
||||
raise ValueError(f"ttl_days must be positive, got {ttl_days!r}")
|
||||
|
||||
# Тот же класс дыры, что и ttl_days<=0 выше, только со стороны потолка:
|
||||
# cap_mult < 1 может опустить потолок (ttl_days * cap_mult) НИЖЕ заданного
|
||||
# ttl_days, а cap_mult <= 0 -- сделать капнутый потолок <= 0 и победить пол
|
||||
# в min() молча (ровно та дыра, ради которой заведён guard выше). Единственный
|
||||
# запланированный способ задать cap_mult -- вписать его руками в jsonb
|
||||
# default_params расписания (см. миграцию для avito), т.е. именно там опечатка
|
||||
# 0 / 0.5 вместо 6 -- реальный риск, а не гипотетика.
|
||||
if cap_mult < 1:
|
||||
raise ValueError(f"cap_mult must be >= 1, got {cap_mult!r}")
|
||||
|
||||
# Whitelist-проверка ДО построения/выполнения SQL: только после неё имя колонки
|
||||
# интерполируется f-string'ом. Значения по-прежнему идут через param-binding.
|
||||
# Внутри try -> невалидная колонка финализирует run как failed (mark_failed),
|
||||
|
|
|
|||
|
|
@ -0,0 +1,52 @@
|
|||
-- 264_deactivate_stale_avito_cap_mult.sql
|
||||
-- Калибрует потолок эффективного TTL (cap_mult) для avito (#TTL-CAP, 2026-08-15).
|
||||
--
|
||||
-- ЗАЧЕМ. Пол TTL по измеренному циклу переобхода (#2659, deactivate_stale_avito.py)
|
||||
-- поднимает эффективный TTL через max(ttl_days, пол) без верхней границы -- на проде
|
||||
-- это оказалось петлёй с положительной обратной связью: медленный обход поднимает
|
||||
-- пол, высокий пол продлевает жизнь снятым лотам дольше, чем к ним успевает
|
||||
-- вернуться свежий обход, пул «активных» раздувается протухшими строками -- 23 687
|
||||
-- из 44 744 avito-строк не подтверждались >7 суток, самая старая «активная» запись
|
||||
-- не видена 86 суток. Потолок cap_mult ограничивает пол сверху: эффективный TTL не
|
||||
-- может превысить ttl_days * cap_mult (код -- app/tasks/deactivate_stale_avito.py,
|
||||
-- CAP_MULT).
|
||||
--
|
||||
-- ПОЧЕМУ ИМЕННО AVITO. Дефолт CAP_MULT=2 даёт разный АБСОЛЮТНЫЙ потолок на разных
|
||||
-- источниках (множитель от ttl_days), и ломается там, где хвост переобхода
|
||||
-- источника НЕ пропорционален его ttl_days:
|
||||
-- источник/сегмент p99 хвоста ttl_days потолок при cap_mult=2
|
||||
-- domklik vtorichka 3.1 14 28 (запас есть)
|
||||
-- cian vtorichka 26.6 30 60 (запас есть)
|
||||
-- yandex vtorichka 43.0 30 60 (запас есть)
|
||||
-- avito все сегменты 42.1 10 20 (ХВОСТ ВЫШЕ ПОТОЛКА)
|
||||
-- У avito p99=42.1 суток ВЫШЕ его же дефолтного потолка 20 -- дефолтный cap_mult=2
|
||||
-- может резать пол ниже собственного хвоста обхода, то есть ровно тот false-kill,
|
||||
-- ради которого пол вообще заведён. cian/yandex/domklik разрыва не имеют, дефолт
|
||||
-- cap_mult=2 для них калиброван верно, миграция их не трогает.
|
||||
--
|
||||
-- ПОЧЕМУ 6. Потолок 60 = 10 * 6 -- тот же порядок, что у cian/yandex (60), с запасом
|
||||
-- выше и статического p99=42.1 (_REVISIT_TAIL, tests/test_deactivate_stale_revisit_floor.py),
|
||||
-- и живого прод-пика: floor=52 три прогона подряд 2026-08-10..08-12
|
||||
-- (scrape_runs.counters, status=done, confirmations 6934..7138, гейт здоровья
|
||||
-- пропустил). Без этой калибровки в проде остаётся дефолт cap_mult=2 (потолок 20)
|
||||
-- -- именно тот случай, для которого потолок и его собственная калибровочная ручка
|
||||
-- заведены, но не применены к единственному источнику, ради которого ручка сделана.
|
||||
--
|
||||
-- ЗАВИСИМОСТИ: 052_scrape_schedules.sql (таблица + UNIQUE(source)), 219 (тот же
|
||||
-- приём -- UPDATE default_params через jsonb ?, min_confirmations).
|
||||
-- ТОЛЬКО данные (UPDATE default_params), DDL нет.
|
||||
-- Идемпотентность + уважение к ручной настройке: ключ проставляется лишь там, где
|
||||
-- его ещё нет, поэтому повторный прогон файла не затирает подкрученное оператором
|
||||
-- значение. Снять/поднять потолок вручную: cap_mult в default_params
|
||||
-- (deactivate_stale_avito), 1 -> потолок = сам ttl_days (см. guard cap_mult < 1
|
||||
-- в deactivate_stale_listings -- ниже 1 отклоняется до любого SQL).
|
||||
|
||||
BEGIN;
|
||||
|
||||
UPDATE scrape_schedules
|
||||
SET default_params = default_params || jsonb_build_object('cap_mult', 6),
|
||||
updated_at = NOW()
|
||||
WHERE source = 'deactivate_stale_avito'
|
||||
AND NOT default_params ? 'cap_mult';
|
||||
|
||||
COMMIT;
|
||||
|
|
@ -252,3 +252,4 @@
|
|||
261_listings_search_mv_drop_placeholder_columns.sql
|
||||
262_scrape_schedules_seed_oblast_city_sweeps_wave2.sql
|
||||
263_scrape_schedules_wave2_cian_newbuilding_only_false.sql
|
||||
264_deactivate_stale_avito_cap_mult.sql
|
||||
|
|
|
|||
|
|
@ -130,13 +130,13 @@ def test_effective_ttl_covers_every_proven_false_kill(monkeypatch: pytest.Monkey
|
|||
|
||||
Все они произошли на возрасте 29.9..30.3 суток. Эффективный TTL обязан быть
|
||||
строго выше этого возраста на cian/yandex-срезах — иначе следующий прогон
|
||||
снимет ту же строку снова. avito пропущен намеренно: 127 доказанных ложных
|
||||
снятий (_FALSE_KILLS_BY_CITY) измерены только по cian/yandex, а гипотетический
|
||||
замер пола avito=69.7 при ttl=10 -- ровно тот случай, для которого заведён
|
||||
потолок CAP_MULT (#TTL-CAP, 2026-08-15): без потолка пол растёт без
|
||||
ограничения (петля с положительной обратной связью, найдена на проде),
|
||||
покрытие такого выброса потолком намеренно НЕ гарантируется -- см.
|
||||
test_deactivate_stale_ttl_cap.py.
|
||||
снимет ту же строку снова. avito из этого цикла исключён намеренно: 127
|
||||
доказанных ложных снятий (_FALSE_KILLS_BY_CITY) измерены только по cian/yandex,
|
||||
у avito другой сценарий и своя проверка ниже
|
||||
(test_avito_prod_floor_is_capped_by_calibrated_cap_mult) -- калибровка cap_mult=6
|
||||
для avito (миграция 264_deactivate_stale_avito_cap_mult.sql) пиннится ТАМ, а не
|
||||
здесь, чтобы не смешивать два разных замера под одним порогом
|
||||
_FALSE_KILL_AGE_MAX, который к avito не относится.
|
||||
"""
|
||||
for slice_name, (source, segments, ttl_days, floor) in _PROD_FLOORS.items():
|
||||
if source == "avito":
|
||||
|
|
@ -162,6 +162,34 @@ def test_effective_ttl_covers_every_proven_false_kill(monkeypatch: pytest.Monkey
|
|||
), f"{slice_name}: UPDATE получил не поднятый TTL — пол посчитан и выброшен"
|
||||
|
||||
|
||||
def test_avito_prod_floor_is_capped_by_calibrated_cap_mult(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Пиннит калибровку cap_mult=6 для avito (миграция
|
||||
264_deactivate_stale_avito_cap_mult.sql) на измеренном прод-поле _PROD_FLOORS
|
||||
("avito/все сегменты" = 69.7, замер 2026-08-09).
|
||||
|
||||
С дефолтным cap_mult=2 потолок avito (20 сут) РЕЖЕТ ниже собственного хвоста
|
||||
переобхода p99=42.1 (_REVISIT_TAIL) -- ровно тот false-kill, ради которого пол
|
||||
заведён. С калиброванным cap_mult=6 потолок 60 сут -- выше и p99=42.1, и живого
|
||||
прод-пика 52 (замер 08-10..08-12), и этого гипотетического замера 69.7 (капается
|
||||
ровно на 60, не пропускается как есть). Без этого теста калибровка cap_mult=6
|
||||
нигде не пиннится числом -- только упоминается в комментарии/миграции.
|
||||
"""
|
||||
source, segments, ttl_days, floor = _PROD_FLOORS["avito/все сегменты"]
|
||||
db = _FakeDB(floor_days=floor)
|
||||
out = _run(
|
||||
db,
|
||||
monkeypatch,
|
||||
listing_source=source,
|
||||
ttl_days=ttl_days,
|
||||
segments=segments,
|
||||
revisit_floor_quantile=task_mod.DEFAULT_REVISIT_FLOOR_QUANTILE,
|
||||
cap_mult=6,
|
||||
)
|
||||
assert out["ttl_days_effective"] == 60, "cap_mult=6 * ttl_days=10 обязан дать потолок 60"
|
||||
assert out["ttl_floor_capped"] == 1
|
||||
assert out["ttl_days_floor_raw"] == 70, "ceil(69.7) == 70 -- пол считается по real-числу"
|
||||
|
||||
|
||||
def test_false_kill_ages_sit_inside_the_old_ttl(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Замер согласован сам с собой: снимали ровно на границе TTL=30, не раньше."""
|
||||
assert _FALSE_KILL_AGE_MIN < 30.0 <= _FALSE_KILL_AGE_MAX
|
||||
|
|
|
|||
|
|
@ -310,6 +310,77 @@ def test_ttl_days_zero_fails_the_run_via_mark_failed(monkeypatch: pytest.MonkeyP
|
|||
assert marked_failed[0][0] == 7
|
||||
|
||||
|
||||
# ── cap_mult < 1 (HIGH из ревью круга 2, 2026-08-15) ─────────────────────────────
|
||||
# Тот же класс дыры, что и ttl_days<=0 выше, но со стороны потолка: cap_mult -- ЕДИНСТВЕННЫЙ
|
||||
# запланированный способ его задать -- руками вписать в jsonb default_params расписания
|
||||
# (см. миграцию для avito), т.е. именно там опечатка 0 / 0.5 вместо 6 доходит до прода.
|
||||
# cap_mult=0 -> capped=0 -> effective_ttl_days=0 -> UPDATE снимает весь активный пул
|
||||
# источника молча. cap_mult<1 (например 0.5) опускает потолок НИЖЕ заданного оператором
|
||||
# ttl_days -- прямое нарушение инварианта, который проверяет
|
||||
# test_cap_never_lowers_ttl_below_configured_value для пола, но не было проверено для
|
||||
# потолка при некорректном cap_mult.
|
||||
|
||||
|
||||
def test_cap_mult_zero_is_rejected_before_any_sql(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
db = _FakeDB(floor_days=52.0)
|
||||
with pytest.raises(ValueError):
|
||||
_run(db, monkeypatch, ttl_days=10, revisit_floor_quantile=0.99, cap_mult=0)
|
||||
assert db.executed == []
|
||||
|
||||
|
||||
def test_cap_mult_negative_is_rejected_before_any_sql(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
db = _FakeDB(floor_days=52.0)
|
||||
with pytest.raises(ValueError):
|
||||
_run(db, monkeypatch, ttl_days=10, revisit_floor_quantile=0.99, cap_mult=-2)
|
||||
assert db.executed == []
|
||||
|
||||
|
||||
def test_cap_mult_below_one_is_rejected_before_any_sql(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""cap_mult=0.5 опустил бы потолок НИЖЕ заданного ttl_days -- та самая инверсия,
|
||||
которую тест test_cap_never_lowers_ttl_below_configured_value гарантирует для пола."""
|
||||
db = _FakeDB(floor_days=52.0)
|
||||
with pytest.raises(ValueError):
|
||||
_run(db, monkeypatch, ttl_days=10, revisit_floor_quantile=0.99, cap_mult=0.5)
|
||||
assert db.executed == []
|
||||
|
||||
|
||||
def test_cap_mult_non_numeric_fails_safe_before_any_sql(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Опечатка в jsonb default_params (строка вместо числа) не должна молча пройти
|
||||
в SQL -- TypeError из сравнения `cap_mult < 1` ловится тем же except Exception,
|
||||
что и ValueError-гварды, и маршрутизируется через mark_failed. Никакого SQL не
|
||||
исполняется, ни один active-лот не тронут."""
|
||||
db = _FakeDB(floor_days=52.0)
|
||||
with pytest.raises(TypeError):
|
||||
_run(db, monkeypatch, ttl_days=10, revisit_floor_quantile=0.99, cap_mult="6")
|
||||
assert db.executed == []
|
||||
|
||||
|
||||
def test_cap_mult_zero_fails_the_run_via_mark_failed(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Тот же контракт, что и ttl_days<=0: run помечается failed, а не остаётся 'running',
|
||||
и НИ ОДНА строка не деактивируется (в отличие от воспроизведённого на HEAD дефекта, где
|
||||
cap_mult=0 давало effective_ttl_days=0 и снимало весь активный пул источника)."""
|
||||
marked_failed: list[Any] = []
|
||||
monkeypatch.setattr(task_mod.runs_mod, "mark_done", lambda *a, **k: None)
|
||||
monkeypatch.setattr(
|
||||
task_mod.runs_mod,
|
||||
"mark_failed",
|
||||
lambda db, run_id, err, counters: marked_failed.append((run_id, err, counters)),
|
||||
)
|
||||
db = _FakeDB(floor_days=52.0)
|
||||
with pytest.raises(ValueError):
|
||||
task_mod.deactivate_stale_listings(
|
||||
db, # type: ignore[arg-type]
|
||||
9,
|
||||
listing_source="avito",
|
||||
ttl_days=10,
|
||||
revisit_floor_quantile=0.99,
|
||||
cap_mult=0,
|
||||
)
|
||||
assert len(marked_failed) == 1
|
||||
assert marked_failed[0][0] == 9
|
||||
assert db.executed == []
|
||||
|
||||
|
||||
# ── проводка cap_mult в product_handlers ─────────────────────────────────────────
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue