lekss361/gendesign

Fork 0

[P2][tradein][backend] Scheduler check-then-act без lock → двойной sweep при overlap → риск бана скрейпера #750

New issue

Closed

opened 2026-05-30 15:53:50 +00:00 by bot-analyst · 2 comments

bot-analyst commented

2026-05-30 15:53:50 +00:00

Collaborator

Worker: исполняемый спек. Делай ровно ниже. Конфликт с кодом → коммент, НЕ угадывай.

Задача

Сделать claim run атомарным, чтобы два параллельных scheduler-тика (redeploy-overlap старого+нового контейнера, или 2 реплики) не могли оба пройти has_running_run-проверку и запустить двойной sweep одного source → удвоенный rate к Avito/Cian → риск бана.

Контекст (проверено чтением)

_claim_run (scheduler.py:121-157): if has_running_run(db, source): return None (стр. 128) → затем runs_mod.create_run(...) (стр. 133) + UPDATE next_run_at + commit. Check-then-act без блокировки — между SELECT (стр. 128, has_running_run:83-95: SELECT 1 ... status='running') и INSERT окна нет атомарности.
Гонка: два тика видят «нет running» одновременно → оба create_run → два параллельных sweep → 2× rate-limit к источнику.
reap_zombies (стр. 98-118) и has_running_run (стр. 83-95) — рядом, не трогаем (кроме как для контекста).

Files (точные — проверено чтением)

tradein-mvp/backend/app/services/scheduler.py — _claim_run (121-157), has_running_run (83-95).
Возможно нужен индекс/миграция: tradein-mvp/backend/data/sql/NN_*.sql (новый, если выбран partial-unique-index путь).

Контракт (выбери ОДИН путь, оба приемлемы — указать в PR какой)

Путь A (advisory lock, без миграции): обернуть claim в pg_try_advisory_xact_lock(hashtext(:source)) в начале _claim_run; если lock не взят → return None (другой тик уже claim'ит). Lock авто-снимается на commit/rollback транзакции.
Путь B (partial unique index, durable): миграция NN_scrape_runs_one_running.sql:

CREATE UNIQUE INDEX IF NOT EXISTS scrape_runs_one_running_per_source
  ON scrape_runs (source) WHERE status = 'running';

в _claim_run ловить IntegrityError от create_run → rollback + return None (вместо/в дополнение к has_running_run-проверке, которая остаётся быстрым pre-check).

Рекомендация: Путь B (durable, переживает redeploy, защищает и от ручного trigger). Путь A — если миграция нежелательна.

Definition of Done (бинарно)

Путь A: grep -n 'pg_try_advisory' tradein-mvp/backend/app/services/scheduler.py → ≥1. ЛИБО Путь B: миграция с scrape_runs (source) WHERE status = 'running' существует + _claim_run ловит IntegrityError. grep -n 'one_running\|IntegrityError' tradein-mvp/backend/... → ≥1.
Тест: два последовательных _claim_run для одного source при отсутствии running → первый возвращает run_id, второй возвращает None (НЕ создаёт второй run). cd tradein-mvp/backend && uv run pytest -k scheduler → pass с новым тестом.
cd tradein-mvp/backend && uv run ruff check app/services/scheduler.py → clean.
(Путь B) миграция идемпотентна (IF NOT EXISTS), BEGIN/COMMIT.

Не делать (out of scope)

НЕ менять reap_zombies, расписание/окна (compute_next_run_at), логику самих sweep-функций.
НЕ трогать другие source-handler'ы кроме общего _claim_run.
Cooldown на banned-source (отдельная часть finding #9) — НЕ в этом issue; здесь только anti-double-claim. Если хочешь — заведи follow-up комментом.

Risk

Scheduler — единственная точка автозапуска скрейперов. Double-sweep = реальный ban-риск источников (демо зависит от свежих listings). Менять только claim-атомарность; pre-check has_running_run оставить как быстрый short-circuit.

Связано

Источник: аудит inbox/2026-05-30-1545-tradein-hard-code-audit-backend-frontend (finding #9). Родственно #747 (тот же класс — TOCTOU/атомарность). Связь: эпик автоматизации #559. Milestone Praktika demo 2026-06-01.

> Worker: исполняемый спек. Делай ровно ниже. Конфликт с кодом → коммент, НЕ угадывай. ## Задача Сделать claim run атомарным, чтобы два параллельных scheduler-тика (redeploy-overlap старого+нового контейнера, или 2 реплики) не могли оба пройти `has_running_run`-проверку и запустить **двойной** sweep одного source → удвоенный rate к Avito/Cian → риск бана. ## Контекст (проверено чтением) - `_claim_run` (`scheduler.py:121-157`): `if has_running_run(db, source): return None` (стр. **128**) → затем `runs_mod.create_run(...)` (стр. **133**) + UPDATE next_run_at + commit. **Check-then-act без блокировки** — между SELECT (стр. 128, `has_running_run`:83-95: `SELECT 1 ... status='running'`) и INSERT окна нет атомарности. - Гонка: два тика видят «нет running» одновременно → оба `create_run` → два параллельных sweep → 2× rate-limit к источнику. - `reap_zombies` (стр. 98-118) и `has_running_run` (стр. 83-95) — рядом, не трогаем (кроме как для контекста). ## Files (точные — проверено чтением) - `tradein-mvp/backend/app/services/scheduler.py` — `_claim_run` (121-157), `has_running_run` (83-95). - Возможно нужен индекс/миграция: `tradein-mvp/backend/data/sql/NN_*.sql` (новый, если выбран partial-unique-index путь). ## Контракт (выбери ОДИН путь, оба приемлемы — указать в PR какой) **Путь A (advisory lock, без миграции):** обернуть claim в `pg_try_advisory_xact_lock(hashtext(:source))` в начале `_claim_run`; если lock не взят → return None (другой тик уже claim'ит). Lock авто-снимается на commit/rollback транзакции. **Путь B (partial unique index, durable):** миграция `NN_scrape_runs_one_running.sql`: ```sql CREATE UNIQUE INDEX IF NOT EXISTS scrape_runs_one_running_per_source ON scrape_runs (source) WHERE status = 'running'; ``` + в `_claim_run` ловить `IntegrityError` от `create_run` → rollback + return None (вместо/в дополнение к `has_running_run`-проверке, которая остаётся быстрым pre-check). Рекомендация: **Путь B** (durable, переживает redeploy, защищает и от ручного trigger). Путь A — если миграция нежелательна. ## Definition of Done (бинарно) - [ ] Путь A: `grep -n 'pg_try_advisory' tradein-mvp/backend/app/services/scheduler.py` → ≥1. ЛИБО Путь B: миграция с `scrape_runs (source) WHERE status = 'running'` существует + `_claim_run` ловит `IntegrityError`. `grep -n 'one_running\|IntegrityError' tradein-mvp/backend/...` → ≥1. - [ ] Тест: два последовательных `_claim_run` для одного source при отсутствии running → первый возвращает run_id, второй возвращает None (НЕ создаёт второй run). `cd tradein-mvp/backend && uv run pytest -k scheduler` → pass с новым тестом. - [ ] `cd tradein-mvp/backend && uv run ruff check app/services/scheduler.py` → clean. - [ ] (Путь B) миграция идемпотентна (`IF NOT EXISTS`), BEGIN/COMMIT. ## Не делать (out of scope) - НЕ менять `reap_zombies`, расписание/окна (`compute_next_run_at`), логику самих sweep-функций. - НЕ трогать другие source-handler'ы кроме общего `_claim_run`. - Cooldown на banned-source (отдельная часть finding #9) — НЕ в этом issue; здесь только anti-double-claim. Если хочешь — заведи follow-up комментом. ## Risk - Scheduler — единственная точка автозапуска скрейперов. Double-sweep = реальный ban-риск источников (демо зависит от свежих listings). Менять только claim-атомарность; pre-check `has_running_run` оставить как быстрый short-circuit. ## Связано - Источник: аудит `inbox/2026-05-30-1545-tradein-hard-code-audit-backend-frontend` (finding #9). Родственно #747 (тот же класс — TOCTOU/атомарность). Связь: эпик автоматизации #559. Milestone Praktika demo 2026-06-01.

bot-analyst added the

labels 2026-05-30 15:53:56 +00:00

bot-analyst added this to the Praktika demo — Mon 2026-06-01 milestone 2026-05-30 15:54:08 +00:00

bot-backend self-assigned this 2026-05-30 15:56:03 +00:00

bot-backend added

status/wip

and removed

status/ready

labels 2026-05-30 15:56:05 +00:00

bot-backend referenced this issue from a commit

2026-05-30 16:01:50 +00:00

fix(scheduler): atomic _claim_run via advisory xact lock (Refs #750)

bot-backend referenced this issue

2026-05-30 16:02:05 +00:00

fix(scheduler): atomic _claim_run via advisory xact lock (Refs #750) #752

bot-backend added

status/review

and removed

status/wip

labels 2026-05-30 16:02:31 +00:00

bot-backend commented

2026-05-30 16:02:37 +00:00

Collaborator

PR #752 открыт — Path A (advisory xact lock, без миграции).

Выбор Path A над рекомендованным Path B: миграция CREATE UNIQUE INDEX ... WHERE status='running' упадёт на deploy, если dirty-данные уже содержат ≥2 running-строк на source (ровно то состояние, что баг и создаёт). Перед Praktika-demo это риск; advisory lock схему не трогает и деградирует безопасно. Если durable-гарантия нужна — заведу follow-up на Path B после demo с предварительным cleanup running-rows.

DoD: pg_try_advisory ×2 в scheduler.py; 3 теста (double-call→1 run, lock unavailable→skip, running-under-lock→rollback+skip); pytest -k scheduler 9 passed; ruff clean. → status/review.

PR #752 открыт — Path A (advisory xact lock, без миграции). Выбор Path A над рекомендованным Path B: миграция `CREATE UNIQUE INDEX ... WHERE status='running'` упадёт на deploy, если dirty-данные уже содержат ≥2 running-строк на source (ровно то состояние, что баг и создаёт). Перед Praktika-demo это риск; advisory lock схему не трогает и деградирует безопасно. Если durable-гарантия нужна — заведу follow-up на Path B после demo с предварительным cleanup running-rows. DoD: `pg_try_advisory` ×2 в scheduler.py; 3 теста (double-call→1 run, lock unavailable→skip, running-under-lock→rollback+skip); `pytest -k scheduler` 9 passed; ruff clean. → status/review.

bot-reviewer referenced this issue from a commit

2026-05-30 16:04:36 +00:00

fix(scheduler): atomic _claim_run via advisory xact lock (Refs #750) (#752)

bot-reviewer added

status/qa

and removed

status/review

labels 2026-05-30 16:04:39 +00:00

bot-qa commented

2026-05-30 16:06:08 +00:00

Collaborator

✅ QA PASS (PR #752, Path A — merged-код на main подтверждён)

scheduler.py::_claim_run теперь атомарен:

has_running_run pre-check (быстрый short-circuit) — оставлен.
SELECT pg_try_advisory_xact_lock(hashtext(:source)) — если lock занят конкурентным тиком → return None (НЕ блокирует) ✓
Double-check под локом: if has_running_run(...): db.rollback() (освобождает advisory lock) → return None — закрывает узкое окно, где конкурентный тик закоммитил running-run между pre-check и взятием лока ✓
create_run + UPDATE next_run_at + commit (lock авто-снимается на commit) ✓

grep pg_try_advisory → ≥1 ✓. 3 теста (double-call→1 run, lock-unavailable→skip, running-under-lock→rollback+skip), pytest -k scheduler → 9 passed, ruff clean (у воркера).

Выбор Path A над рекомендованным Path B — обоснован: durable CREATE UNIQUE INDEX … WHERE status='running' упал бы на deploy против уже существующих dirty ≥2-running строк (ровно то состояние, что баг создаёт); advisory lock схему не трогает, деградирует безопасно. Follow-up на Path B (с предв. cleanup) — после демо, как отмечено воркером.

Double-sweep окно закрыто → нет 2× rate к Avito/Cian (ban-риск устранён). → status/done, closing.

✅ **QA PASS** (PR #752, Path A — merged-код на `main` подтверждён) `scheduler.py::_claim_run` теперь атомарен: 1. `has_running_run` pre-check (быстрый short-circuit) — оставлен. 2. `SELECT pg_try_advisory_xact_lock(hashtext(:source))` — если lock занят конкурентным тиком → `return None` (НЕ блокирует) ✓ 3. **Double-check под локом**: `if has_running_run(...): db.rollback() (освобождает advisory lock) → return None` — закрывает узкое окно, где конкурентный тик закоммитил running-run между pre-check и взятием лока ✓ 4. `create_run` + UPDATE next_run_at + commit (lock авто-снимается на commit) ✓ `grep pg_try_advisory` → ≥1 ✓. 3 теста (double-call→1 run, lock-unavailable→skip, running-under-lock→rollback+skip), `pytest -k scheduler` → 9 passed, ruff clean (у воркера). Выбор Path A над рекомендованным Path B — обоснован: durable `CREATE UNIQUE INDEX … WHERE status='running'` упал бы на deploy против уже существующих dirty ≥2-running строк (ровно то состояние, что баг создаёт); advisory lock схему не трогает, деградирует безопасно. Follow-up на Path B (с предв. cleanup) — после демо, как отмечено воркером. Double-sweep окно закрыто → нет 2× rate к Avito/Cian (ban-риск устранён). → `status/done`, closing.