lekss361/gendesign

Fork 0

fix(tradein/deactivate): потолок эффективного TTL — защита от разгона пола и опечатки в расписании #2907

Merged

lekss361 merged 7 commits from fix/tradein-ttl-effective-cap into main

2026-08-15 18:06:55 +00:00

lekss361 commented

2026-08-15 18:01:16 +00:00

Owner

Что это и чем НЕ является

Изначально задача была сформулирована как «сжать раздутый пул активных объявлений». Три круга ревью показали, что этим она не является, и заголовок переписан честно.

Замер на проде: дельта деактиваций следующим прогоном — ноль строк на всех четырёх джобах, и до правки, и после. Раздутый пул лежит вне их скоупа. Подробности — в поправке к аудиту; настоящий дефект (протухшие строки с пустым сегментом, которые оценщик берёт) чинится отдельной веткой.

Ценность этого PR в другом.

Проблема, которая реальна

effective_ttl_days = max(ttl_days, revisit_floor_days) без верхней границы. Пол — это квантиль разрывов переобхода, он растёт сам по себе при медленном обходе. Живой факт: у yandex ttl_days_effective держался 75/75/75/39/52/54 шесть прогонов подряд при deactivated=0, сейчас пол 79.2 суток. Это положительная обратная связь — медленный обход поднимает пол, высокий пол продлевает жизнь снятым лотам.

Что сделано

Потолок min(max(ttl_days, floor), ttl_days * cap_mult) с калибровкой per-source, посчитанной по живым counters, а не по статической константе. avito получает cap_mult=6, yandex — cap_mult=3 (потолок 90 при живом поле 79).

Guard от опечатки в jsonb. Это то, ради чего PR стоит мержить. default_params расписания не валидируется, и "cap_mult": 0 дал бы effective_ttl = 0 — деактивацию всего активного пула источника одним прогоном.

Первый вариант guard'а сам имел дыру: bool — подкласс int в Python, поэтому "cap_mult": true проходило как 1 и молча отключало защиту. Ровно тот класс опечатки, ради которого guard написан. Теперь isinstance(x, bool) стоит до числового сравнения, для обоих параметров.

Что поймали три круга ревью

Круг	Находка
1	потолок мультипликативный, а `ttl_days` различается втрое — у источника с худшим обходом самый жёсткий потолок
2	механизм есть, миграции со значениями нет — на проде правка была бы no-op
3	дыра с `bool`; тест «пиннит калибровку», но значение захардкожено в самом тесте — мутация миграции 6→2 оставляла тест зелёным

Тест теперь читает значение из 264_*.sql регуляркой; мутация проверена — краснеет.

Отдельным коммитом — исправление вранья в комментарии

В шапке модуля и в тексте миграции стояло «23 687 из 44 744 avito-объявлений не подтверждались >7 суток» под заголовком «ЗАМЕР НА ПРОДЕ». Число реальное, но приписано не тому источнику: это все источники вместе, две трети — новостройки вне выборки оценщика, а у самого avito просроченных ноль.

Убрал. Оставлять такое в коде хуже, чем не писать вовсе: следующий человек проверит, не найдёт — и перестанет доверять остальным числам в том же абзаце, которые верны и сверены с scrape_runs.counters.

Известный остаток

Зеркальный пин-тест для yandex односторонний: ослабление потолка (3 → 100) он не ловит, только ужесточение. У avito пин двусторонний. Не блокер, вынесу отдельно.

Test plan

полный backend-сьют: 4508 passed, 0 failed
мутационные прогоны: удаление bool-guard и подмена значения в миграции — тесты краснеют
численный эффект замерен на проде: 0 деактиваций до и после (ожидаемо)
после деплоя: ttl_days_effective у yandex не должен превышать 90

## Что это и чем НЕ является Изначально задача была сформулирована как «сжать раздутый пул активных объявлений». **Три круга ревью показали, что этим она не является**, и заголовок переписан честно. Замер на проде: дельта деактиваций следующим прогоном — **ноль строк** на всех четырёх джобах, и до правки, и после. Раздутый пул лежит вне их скоупа. Подробности — в поправке к аудиту; настоящий дефект (протухшие строки с пустым сегментом, которые оценщик берёт) чинится отдельной веткой. Ценность этого PR в другом. ## Проблема, которая реальна `effective_ttl_days = max(ttl_days, revisit_floor_days)` без верхней границы. Пол — это квантиль разрывов переобхода, он растёт сам по себе при медленном обходе. Живой факт: у yandex `ttl_days_effective` держался **75/75/75/39/52/54** шесть прогонов подряд при `deactivated=0`, сейчас пол 79.2 суток. Это положительная обратная связь — медленный обход поднимает пол, высокий пол продлевает жизнь снятым лотам. ## Что сделано **Потолок** `min(max(ttl_days, floor), ttl_days * cap_mult)` с калибровкой per-source, посчитанной по живым `counters`, а не по статической константе. avito получает `cap_mult=6`, yandex — `cap_mult=3` (потолок 90 при живом поле 79). **Guard от опечатки в jsonb.** Это то, ради чего PR стоит мержить. `default_params` расписания не валидируется, и `"cap_mult": 0` дал бы `effective_ttl = 0` — **деактивацию всего активного пула источника одним прогоном**. Первый вариант guard'а сам имел дыру: `bool` — подкласс `int` в Python, поэтому `"cap_mult": true` проходило как `1` и молча отключало защиту. Ровно тот класс опечатки, ради которого guard написан. Теперь `isinstance(x, bool)` стоит до числового сравнения, для обоих параметров. ## Что поймали три круга ревью | Круг | Находка | |---|---| | 1 | потолок мультипликативный, а `ttl_days` различается втрое — у источника с худшим обходом самый жёсткий потолок | | 2 | механизм есть, **миграции со значениями нет** — на проде правка была бы no-op | | 3 | дыра с `bool`; тест «пиннит калибровку», но значение захардкожено в самом тесте — мутация миграции 6→2 оставляла тест зелёным | Тест теперь читает значение из `264_*.sql` регуляркой; мутация проверена — краснеет. ## Отдельным коммитом — исправление вранья в комментарии В шапке модуля и в тексте миграции стояло «23 687 из 44 744 **avito**-объявлений не подтверждались >7 суток» под заголовком «ЗАМЕР НА ПРОДЕ». Число реальное, но приписано не тому источнику: это все источники вместе, две трети — новостройки вне выборки оценщика, а у самого avito просроченных **ноль**. Убрал. Оставлять такое в коде хуже, чем не писать вовсе: следующий человек проверит, не найдёт — и перестанет доверять остальным числам в том же абзаце, которые верны и сверены с `scrape_runs.counters`. ## Известный остаток Зеркальный пин-тест для yandex односторонний: ослабление потолка (`3 → 100`) он не ловит, только ужесточение. У avito пин двусторонний. Не блокер, вынесу отдельно. ## Test plan - [x] полный backend-сьют: 4508 passed, 0 failed - [x] мутационные прогоны: удаление bool-guard и подмена значения в миграции — тесты краснеют - [x] численный эффект замерен на проде: 0 деактиваций до и после (ожидаемо) - [ ] после деплоя: `ttl_days_effective` у yandex не должен превышать 90

lekss361 added 7 commits 2026-08-15 18:01:17 +00:00

fix(tradein/deactivate): cap effective TTL floor at 2x configured value 3a1e29a7da

Revisit-floor (#2659) raises effective TTL via max(ttl_days, floor) with no
upper bound -- a positive feedback loop confirmed on prod: slow crawl raises
the floor, a high floor keeps stale listings marked active longer than a
fresh sweep needs to return, the "active" pool bloats with rot, and the next
floor measurement on that bloated pool comes out even higher. Yandex counters
sat at ttl_days_effective=75/75/75/39/52/54 for six runs straight with
deactivated=0; 23,687/44,744 "active" avito listings hadn't been confirmed in
>7 days, cian 10,572/19,514 and yandex 7,178/15,790 were >30 days stale, the
oldest "active" row hadn't been seen in 86 days.

CAP_MULT=2 caps the floor's upward push without disabling it -- the floor
still protects against premature deactivation during genuinely slow (but
alive) crawl cycles, it just can no longer grow unbounded. Beyond 2x, a
persistently low crawl rate is better handled by the existing health gate
(min_confirmations), which disables deactivation outright instead of
stretching TTL forever.

When the cap binds, counters gain ttl_floor_capped=1 + ttl_days_floor_raw
(the uncapped value) so it's visible in the run-history dashboard, not just
logs -- counters are stored as-is in scrape_runs.counters.

Single fix point: all four sources (avito/yandex/cian/domklik) route through
this one deactivate_stale_listings() via the product_handlers wildcard
"deactivate_stale_*" handler, so no other task file needed the change.

fix(tradein/deactivate): make TTL-cap multiplier configurable per source cb79c67bfc

Review of 3a1e29a7 found CAP_MULT=2 is uniform across sources with wildly
different ttl_days, so it produces a different ABSOLUTE ceiling per source:
cian/yandex (ttl=30) -> 60d, avito (ttl=10) -> 20d, domklik (ttl=14) -> 28d.
That breaks exactly where the crawl's revisit tail doesn't scale with
ttl_days: avito's measured p99 revisit gap is 42.1d (_REVISIT_TAIL) --
above its own default cap of 20d -- so a legitimately slow-but-alive avito
crawl cycle would get its floor cut below the very tail the floor exists
to protect (the false-kill scenario #2659 was filed for). cian/yandex/
domklik aren't affected: their default ceilings (60/60/28) already sit
comfortably above their own measured tails (26.6/43.0/3.1).

Fix: cap_mult is now a function parameter (same pattern as
revisit_floor_quantile/min_confirmations) with the module constant CAP_MULT
as its default, wired through product_handlers via default_params["cap_mult"]
so a schedule can override it without touching the shared default. Also
closes the ttl_days<=0 edge case flagged in the same review: before the cap,
max(ttl_days, floor) tolerated a misconfigured ttl_days<=0 as long as the
floor was positive; with the cap, min(floor, ttl_days*cap_mult<=0) would
silently defeat that protection and match nearly the whole active pool.
ttl_days<=0 now raises ValueError before any SQL, same contract as the
existing staleness_column whitelist check.

Also verified (read-only, postgres-tradein) the review's core "no-op"
claim: false. scrape_runs.counters for the 6 days since the revisit-floor
went live (08-10..08-15) show the cap DID bind on 3 of 6 runs for avito
(floor 52 vs cap 20) and 3 of 6 for yandex (floor 75 vs cap 60) -- the
reviewer's "no source hits the cap" read a single-day trough right after
a natural recovery, not the whole observation window. See PR discussion
for the full counter history and refutation detail.

Refs #2659

Merge remote-tracking branch 'forgejo/main' into local/fix-ttl-effective-cap-review2 bef2a05f60

fix(tradein/deactivate): validate cap_mult, calibrate avito, document scope gap 772ae116b5

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).

Merge remote-tracking branch 'forgejo/main' into fix/tradein-ttl-effective-cap 8fff174337

fix(tradein/deactivate): bool guard hole + unpinned test + yandex cap_mult gap (TTL-CAP round 3) 19c9da8119

Три остатка ревью TTL-CAP:

1. cap_mult < 1 пропускал bool: jsonb true -> True < 1 ложно -> потолок =
   ttl_days*True = ttl_days -> пол молча отключается без ValueError. Тот же
   класс дыры возможен и через ttl_days=true (TTL молча = 1). Оба параметра
   теперь явно отклоняют bool ДО числового сравнения; воспроизведено на HEAD
   и закрыто тестами (True/False на обоих параметрах).

2. test_avito_prod_floor_is_capped_by_calibrated_cap_mult хардкодил cap_mult=6
   как вход -- мутация миграции 264 (6 -> 2) оставляла набор зелёным. Тест
   теперь читает cap_mult ИЗ ФАЙЛА миграции regex'ом, ожидаемый результат
   (потолок 60) остаётся зафиксированным числом -- дрейф калибровки в SQL
   теперь ломает тест.

3. Текст миграции 264 утверждал "yandex 43.0 -> потолок 60, запас есть" по
   статическому p99. Живые полы из scrape_runs.counters (08-10..08-15:
   75/75/75/39/52/54) и live-замер сегодня (79.2, n=1961) выше потолка 60 --
   тот же false-kill класс, что у avito. Откалибровал yandex отдельной
   миграцией 265 (cap_mult=3 -> потолок 90, тот же запас ~14%, что у avito),
   поправил таблицу в 264 на живые числа и пиннящий тест по образцу avito.

Численный эффект (live-замер 2026-08-15, до и после): next-run deactivated=0
на всех четырёх джобах что до, что после -- ветка по-прежнему НЕ сжимает пул
(avito/cian: живой пол уже ниже потолка, cap не участвует; yandex: 0 активных
строк старше 39 суток вообще, калибровка убирает будущий риск, не текущее
число; domklik: блокирован гейтом здоровья, confirmations 94 < 200). Ветка
остаётся тем, чем и была: защита от опечатки в расписании + калибровка, не
сжатие пула.

4508 backend-тестов зелёные (uv run pytest tests/), ruff чист на изменённых
файлах.

docs(tradein/deactivate): убрать неверное число из обоснования потолка

CI Trade-In / changes (pull_request) Successful in 8s

Details

CI / changes (pull_request) Successful in 8s

Details

CI Trade-In / browser-tests (pull_request) Has been skipped

Details

CI Trade-In / frontend-checks (pull_request) Has been skipped

Details

CI / backend-tests (pull_request) Has been skipped

Details

CI / frontend-tests (pull_request) Has been skipped

Details

CI / openapi-codegen-check (pull_request) Has been skipped

Details

CI Trade-In / backend-tests (pull_request) Successful in 4m33s

Details

08cff706ec

В шапке модуля, в комментарии миграции 264 и в докстринге теста стояло «23 687 из
44 744 avito-объявлений не подтверждались >7 суток» под заголовком «ЗАМЕР НА ПРОДЕ».

Число реальное, но приписано не тому. Перепроверено запросом 2026-08-15:
это ВСЕ источники вместе, и две трети — новостройки, которых оценщик не берёт
(он фильтрует listing_segment IS NULL OR = 'vtorichka'). У самого avito
просроченных строк ноль: 8 663 активных, максимальный возраст 10 суток.

Оставлять это в коде нельзя: следующий человек прочитает «avito раздут вдвое»,
проверит и не найдёт — а заодно потеряет доверие к остальным числам в том же
абзаце, которые верны и сверены с scrape_runs.counters.

Заодно явно записано, чего потолок НЕ делает: он не сжимает пул (0 деактиваций
замерено на всех четырёх джобах), а защищает от разгона пола и от опечатки в
расписании. Настоящий раздутый срез — строки с пустым сегментом, они чинятся
отдельной джобой.