fix(tradein/proxy_pool): защита бана не считает узел с истёкшей арендой, условия резерва проверены по значению (#3299)
Ревью PR #3565: у нового предиката резерва две части не проверялись ни одним тестом. Снятие `other.expires_at` из подзапроса acquire и снятие `other.consecutive_fails` из подзапроса mark_banned оставляли прогон test_3299 + services/test_proxy_pool.py зелёным (72 passed). Проверено тем же способом, и нашлась третья такая часть: `other.expires_at` в mark_banned. Добавлено по live-тесту на каждое условие: просроченный резерв в acquire, резерв в карантине и просроченный резерв в mark_banned. Попутно найден дефект рядом. Внешний отбор mark_banned («есть ли у источника другой узел») срок аренды не проверял, хотя acquire такой узел не выдаёт, а докстринг обещает «тем же правилом, что acquire()». Если у источника остались только просроченные узлы, бан уходил последнему живому, и источник оставался без прокси, пока healthcheck не наберёт просроченным узлам отказов. Добавлена проверка `sp.expires_at` и тест на этот случай. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
47a91acf61
commit
29ac3043b4
2 changed files with 58 additions and 4 deletions
|
|
@ -996,6 +996,10 @@ def mark_banned(db: Session, proxy_id: int, *, source: str, reason: str | None =
|
||||||
WHERE sp.id <> CAST(:proxy_id AS bigint)
|
WHERE sp.id <> CAST(:proxy_id AS bigint)
|
||||||
AND sp.enabled
|
AND sp.enabled
|
||||||
AND sp.consecutive_fails < CAST(:max_fails AS integer)
|
AND sp.consecutive_fails < CAST(:max_fails AS integer)
|
||||||
|
-- Как в acquire(): узел с истёкшей арендой порта остаётся enabled,
|
||||||
|
-- но не выдаётся. Без этой строки он «спасал» источник, и бан уходил
|
||||||
|
-- последнему живому узлу (ревью #3565 к #3299).
|
||||||
|
AND (sp.expires_at IS NULL OR sp.expires_at > now())
|
||||||
AND NOT EXISTS (
|
AND NOT EXISTS (
|
||||||
SELECT 1
|
SELECT 1
|
||||||
FROM scrape_proxy_source_bans b
|
FROM scrape_proxy_source_bans b
|
||||||
|
|
|
||||||
|
|
@ -63,16 +63,17 @@ def db() -> Iterator[Session]:
|
||||||
conn.close()
|
conn.close()
|
||||||
|
|
||||||
|
|
||||||
def _node(db: Session, affinity: str, *, fails: int = 0) -> int:
|
def _node(db: Session, affinity: str, *, fails: int = 0, expired: bool = False) -> int:
|
||||||
proxy_id = db.execute(
|
proxy_id = db.execute(
|
||||||
text(
|
text(
|
||||||
"""
|
"""
|
||||||
INSERT INTO scrape_proxies (url, provider_affinity, consecutive_fails)
|
INSERT INTO scrape_proxies (url, provider_affinity, consecutive_fails, expires_at)
|
||||||
VALUES ('http://t3299-' || gen_random_uuid(), :aff, :fails)
|
VALUES ('http://t3299-' || gen_random_uuid(), :aff, :fails,
|
||||||
|
CASE WHEN :expired THEN now() - interval '1 minute' END)
|
||||||
RETURNING id
|
RETURNING id
|
||||||
"""
|
"""
|
||||||
),
|
),
|
||||||
{"aff": affinity, "fails": fails},
|
{"aff": affinity, "fails": fails, "expired": expired},
|
||||||
).scalar_one()
|
).scalar_one()
|
||||||
db.commit()
|
db.commit()
|
||||||
return int(proxy_id)
|
return int(proxy_id)
|
||||||
|
|
@ -148,6 +149,19 @@ def test_unhealthy_any_node_is_not_a_backup(db: Session) -> None:
|
||||||
assert _leased_by(db, dedicated) is None
|
assert _leased_by(db, dedicated) is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_expired_any_node_is_not_a_backup(db: Session) -> None:
|
||||||
|
"""Узел с истёкшей арендой порта acquire не выдаёт — резервом он тоже не считается.
|
||||||
|
|
||||||
|
Отдельно от карантина: у предиката резерва два независимых условия пригодности, и
|
||||||
|
каждое проверяется своим узлом (ревью PR #3565: без этого теста снятие проверки
|
||||||
|
`expires_at` из подзапроса оставляло сьют зелёным)."""
|
||||||
|
dedicated = _node(db, "avito")
|
||||||
|
_node(db, "any", expired=True)
|
||||||
|
|
||||||
|
assert acquire(db, "domclick", run_id=None) is None
|
||||||
|
assert _leased_by(db, dedicated) is None
|
||||||
|
|
||||||
|
|
||||||
# ── mark_banned: защита считает так же, как acquire ──────────────────────────
|
# ── mark_banned: защита считает так же, как acquire ──────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -179,3 +193,39 @@ def test_mark_banned_still_protects_when_dedicated_node_has_no_usable_backup(
|
||||||
assert mark_banned(db, banned, source="cian") == "protected"
|
assert mark_banned(db, banned, source="cian") == "protected"
|
||||||
lease = acquire(db, "cian", run_id=None)
|
lease = acquire(db, "cian", run_id=None)
|
||||||
assert lease is not None and lease.id == banned
|
assert lease is not None and lease.id == banned
|
||||||
|
|
||||||
|
|
||||||
|
def test_mark_banned_unhealthy_backup_does_not_count(db: Session) -> None:
|
||||||
|
"""Резерв выделенного узла в карантине по consecutive_fails — для cian узла нет,
|
||||||
|
защита держит. Сам резерв кандидатом cian не считается по той же причине (карантин
|
||||||
|
проверяется и во внешнем отборе), так что ответ решает именно предикат резерва."""
|
||||||
|
banned = _node(db, "any")
|
||||||
|
_node(db, "avito")
|
||||||
|
_ban(db, banned, "avito")
|
||||||
|
_node(db, "any", fails=MAX_CONSECUTIVE_FAILS)
|
||||||
|
|
||||||
|
assert mark_banned(db, banned, source="cian") == "protected"
|
||||||
|
|
||||||
|
|
||||||
|
def test_mark_banned_expired_backup_does_not_count(db: Session) -> None:
|
||||||
|
"""Резерв выделенного узла с истёкшей арендой — для cian узла нет, защита держит."""
|
||||||
|
banned = _node(db, "any")
|
||||||
|
_node(db, "avito")
|
||||||
|
_ban(db, banned, "avito")
|
||||||
|
_node(db, "any", expired=True)
|
||||||
|
|
||||||
|
assert mark_banned(db, banned, source="cian") == "protected"
|
||||||
|
|
||||||
|
|
||||||
|
def test_mark_banned_expired_node_does_not_save_the_source(db: Session) -> None:
|
||||||
|
"""Внешний отбор: у cian остался только 'any'-узел с истёкшей арендой. acquire его не
|
||||||
|
выдаёт, значит банимый узел последний — бан не пишется, и cian получает этот узел.
|
||||||
|
|
||||||
|
До правки защита засчитывала просроченный узел (пока healthcheck не наберёт ему
|
||||||
|
отказов), бан уходил, а cian оставался ни с чем."""
|
||||||
|
banned = _node(db, "any")
|
||||||
|
_node(db, "any", expired=True)
|
||||||
|
|
||||||
|
assert mark_banned(db, banned, source="cian") == "protected"
|
||||||
|
lease = acquire(db, "cian", run_id=None)
|
||||||
|
assert lease is not None and lease.id == banned
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue