diff --git a/tradein-mvp/backend/app/api/v1/trade_in.py b/tradein-mvp/backend/app/api/v1/trade_in.py index 4a8fd865..42b75cd3 100644 --- a/tradein-mvp/backend/app/api/v1/trade_in.py +++ b/tradein-mvp/backend/app/api/v1/trade_in.py @@ -61,7 +61,23 @@ async def estimate( """ account_quota.check_and_raise(db, x_authenticated_user) from app.services.estimator import estimate_quality - result = await estimate_quality(payload, db, created_by=x_authenticated_user) + + # #654: ранее любое исключение estimate_quality всплывало необработанным и + # маскировалось апстрим-прокси (Caddy) как непрозрачный 502. Ловим, логируем + # через logger.exception (→ GlitchTip/Sentry получает stack trace) и отдаём + # явный 503 — так любая БУДУЩАЯ реальная ошибка становится видимой, а не + # «глотается» шлюзом. HTTPException пробрасываем как есть (это не сбой). + # created_by (#656) прокидываем в estimate_quality для скоупа /history. + try: + result = await estimate_quality(payload, db, created_by=x_authenticated_user) + except HTTPException: + raise + except Exception: + logger.exception("estimate failed for address=%r", payload.address) + raise HTTPException( + status_code=503, + detail="estimate temporarily unavailable — try again shortly", + ) from None account_quota.increment(db, x_authenticated_user) return result diff --git a/tradein-mvp/backend/app/core/config.py b/tradein-mvp/backend/app/core/config.py index f0d0792d..cd5fd820 100644 --- a/tradein-mvp/backend/app/core/config.py +++ b/tradein-mvp/backend/app/core/config.py @@ -54,6 +54,21 @@ class Settings(BaseSettings): dadata_api_token: str | None = None dadata_api_secret: str | None = None + # ── Estimate enrichment time-budgets (#654) ────────────────────────────── + # POST /estimate делает несколько ПОСЛЕДОВАТЕЛЬНЫХ блокирующих сетевых + # вызовов (geocode → Overpass → Yandex valuation → IMV → Cian). Yandex + # valuation (внутренний httpx timeout 30s) НЕ gated на наличие floor и + # выполняется на каждой оценке — главный подозреваемый на gateway-таймаут + # (Caddy 502/504). Эти budget'ы оборачивают самые медленные ungated-вызовы + # в asyncio.wait_for(): при превышении источник деградирует в None (тот же + # graceful-путь что и сетевая ошибка), а НЕ роняет весь /estimate в 5xx. + # Держать суммарный budget ниже Caddy read/write timeout (см. + # deploy/Caddyfile.tradein-fragment). ENV: ESTIMATE_YANDEX_VALUATION_TIMEOUT_S, + # ESTIMATE_GEOCODE_BUDGET_S, ESTIMATE_HOUSE_META_TIMEOUT_S. + estimate_yandex_valuation_timeout_s: float = 8.0 + estimate_geocode_budget_s: float = 12.0 + estimate_house_meta_timeout_s: float = 8.0 + # Лимит успешных оценок trade-in за календарный месяц на аккаунт (#658). # Конфигурируется через env ESTIMATE_QUOTA_LIMIT. Default 15. estimate_quota_limit: int = 15 diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 1f9fae3f..60ee2d4b 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -19,6 +19,7 @@ from __future__ import annotations +import asyncio import hashlib import json import logging @@ -32,6 +33,7 @@ from uuid import uuid4 from sqlalchemy import text from sqlalchemy.orm import Session +from app.core.config import settings from app.schemas.trade_in import ( AggregatedEstimate, AnalogLot, @@ -658,6 +660,27 @@ def _save_yandex_history_items( return 0 +# ── Time-budget guard (#654) ──────────────────────────────────────────────── +async def _with_budget(coro: Any, budget_s: float, *, label: str) -> Any: + """Await `coro` under an asyncio.wait_for() time budget. + + On timeout the coroutine is cancelled and we return None — mapping a slow + upstream onto the SAME graceful "None" path these enrichments already take + on network error, so a single slow source degrades the estimate instead of + blowing the gateway read timeout (#654: opaque Caddy 502/504). + + budget_s <= 0 disables the guard (await directly) — escape hatch via config. + """ + if budget_s is None or budget_s <= 0: + return await coro + try: + return await asyncio.wait_for(coro, timeout=budget_s) + except TimeoutError: + # asyncio.TimeoutError is an alias of builtin TimeoutError (py3.11+). + logger.warning("%s exceeded %.1fs budget — degrading to None (#654)", label, budget_s) + return None + + # ── Public ─────────────────────────────────────────────────────────────────── async def estimate_quality( payload: TradeInEstimateInput, db: Session, created_by: str | None = None @@ -674,10 +697,15 @@ async def estimate_quality( Returns: AggregatedEstimate с estimate_id, медианой, диапазоном, аналогами. """ - # 1. Geocode + # 1. Geocode (#654: time-budgeted — Yandex/Nominatim retry chain can stack + # multiple network round-trips + 1s Nominatim rate-limit sleeps). geo: GeocodeResult | None = None if payload.address: - geo = await geocode(payload.address, db) + geo = await _with_budget( + geocode(payload.address, db), + settings.estimate_geocode_budget_s, + label="geocode", + ) if geo is None: # Без координат не можем искать через PostGIS. Возвращаем low confidence. @@ -718,10 +746,16 @@ async def estimate_quality( # 2. #392: обогащаем год / тип дома из картографии (OSM Overpass), если # пользователь их не указал — это улучшает house-match аналогов (#6). # Best-effort: при недоступности OSM target_* остаются None. + # #654: time-budgeted — Overpass httpx timeout 15s сам по себе близок к + # gateway-таймауту; деградируем в None при превышении budget. target_year = payload.year_built target_house_type = payload.house_type if target_year is None or target_house_type is None: - house_meta = await get_house_metadata(geo.lat, geo.lon, db) + house_meta = await _with_budget( + get_house_metadata(geo.lat, geo.lon, db), + settings.estimate_house_meta_timeout_s, + label="house_metadata(overpass)", + ) if house_meta is not None: if target_year is None: target_year = house_meta.year_built @@ -934,11 +968,18 @@ async def estimate_quality( sources_used_pre = sorted(set(sources_used_pre) | {"avito_imv"}) # ── Stage 8: Yandex Valuation as on-demand source (anonymous, cached 24h) ── + # #654: главный латентность-подозреваемый. Этот источник UNGATED — бежит на + # КАЖДОЙ оценке (в т.ч. без floor/total_floors), а его внутренний httpx + # timeout 30s + curl_cffi impersonation + sleep_between_requests могут одни + # превысить gateway read timeout → opaque 502. Оборачиваем в budget: при + # превышении → None (cache-hit путь быстрый, timeout бьёт только по медленному + # cache-miss fetch). Деградация идентична сетевой ошибке внутри функции. yandex_val: YandexValuationResult | None = None if geo is not None and geo.full_address: - yandex_val = await _get_or_fetch_yandex_valuation_cached( - db, - address=geo.full_address, + yandex_val = await _with_budget( + _get_or_fetch_yandex_valuation_cached(db, address=geo.full_address), + settings.estimate_yandex_valuation_timeout_s, + label="yandex_valuation", ) if yandex_val is not None: sources_used_pre = sorted(set(sources_used_pre) | {"yandex_valuation"}) diff --git a/tradein-mvp/backend/tests/test_estimator_null_floor_timeout.py b/tradein-mvp/backend/tests/test_estimator_null_floor_timeout.py new file mode 100644 index 00000000..fecc2c63 --- /dev/null +++ b/tradein-mvp/backend/tests/test_estimator_null_floor_timeout.py @@ -0,0 +1,232 @@ +"""Regression tests for #654 — POST /estimate 502 on null floor / slow upstream. + +Two contracts locked here: + 1. estimate with floor=None & total_floors=None returns an AggregatedEstimate + (no exception) AND the floor-gated sources (Avito IMV + Cian valuation) are + NOT awaited — this is the gating contract that makes null-floor safe. + 2. A Yandex-valuation timeout (asyncio.TimeoutError / httpx.TimeoutException) + degrades gracefully: estimate WITHOUT 'yandex_valuation' in sources_used, + no 5xx. Locks the #654 time-budget → None mapping. + +Style mirrors test_estimator_cian_integration.py: MagicMock db, geocode / +_fetch_analogs / _fetch_deals patched, scrapers stubbed. +""" + +import os + +# Settings requires DATABASE_URL at init time. Set dummy DSN before any app import. +os.environ.setdefault("DATABASE_URL", "postgresql://test:test@localhost/test_db") + +from unittest.mock import AsyncMock, MagicMock, patch + +import anyio +import httpx + + +def _make_fake_geo(): + from app.services.geocoder import GeocodeResult + + return GeocodeResult( + lat=56.838, + lon=60.595, + full_address="Свердловская обл., Екатеринбург, ул. Учителей, 18", + provider="nominatim", + ) + + +def _make_payload_null_floor(): + """Simplified-form payload: floor + total_floors are None (the #654 trigger).""" + from app.schemas.trade_in import TradeInEstimateInput + + return TradeInEstimateInput( + address="ЕКБ, ул. Учителей, 18", + area_m2=38.8, + rooms=1, + floor=None, + total_floors=None, + ) + + +def _make_payload_full(): + from app.schemas.trade_in import TradeInEstimateInput + + return TradeInEstimateInput( + address="ЕКБ, ул. Учителей, 18", + area_m2=38.8, + rooms=1, + floor=4, + total_floors=16, + ) + + +def test_estimate_null_floor_skips_imv_and_cian() -> None: + """floor=None & total_floors=None → AggregatedEstimate, IMV+Cian NOT awaited. + + Locks the gating contract: the two floor-required sources must be skipped + (not called) when floor data is absent — confirming the null path is safe + and the 502 was never a null-floor crash (#654). + """ + from app.services.estimator import estimate_quality + + db = MagicMock() + payload = _make_payload_null_floor() + + imv_mock = AsyncMock(return_value=None) + cian_mock = AsyncMock(return_value=None) + yandex_mock = AsyncMock(return_value=None) + + async def _run() -> None: + with ( + patch("app.services.estimator.geocode", + new=AsyncMock(return_value=_make_fake_geo())), + patch("app.services.estimator.get_house_metadata", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._fetch_analogs", return_value=([], False, "W")), + patch("app.services.estimator._fetch_deals", return_value=[]), + patch("app.services.estimator._get_or_fetch_imv_cached", new=imv_mock), + patch("app.services.estimator._get_or_fetch_yandex_valuation_cached", + new=yandex_mock), + patch("app.services.estimator.estimate_via_cian_valuation", new=cian_mock), + patch("app.services.estimator._get_asking_sold_ratio", + return_value=(None, None)), + ): + result = await estimate_quality(payload, db) + + # No exception — got a real estimate back. + assert result.estimate_id is not None + # Gating contract: floor-required sources never awaited. + imv_mock.assert_not_awaited() + cian_mock.assert_not_awaited() + assert "avito_imv" not in result.sources_used + assert "cian_valuation" not in result.sources_used + + anyio.run(_run) + + +def test_estimate_full_floor_awaits_imv_and_cian() -> None: + """Sanity counter-test: with floor present, IMV+Cian ARE awaited. + + Guards against a regression where the gating predicate gets inverted / + broken — proves the null-floor skip above is the gate, not a no-op. + """ + from app.services.estimator import estimate_quality + + db = MagicMock() + payload = _make_payload_full() + + imv_mock = AsyncMock(return_value=None) + cian_mock = AsyncMock(return_value=None) + yandex_mock = AsyncMock(return_value=None) + + async def _run() -> None: + with ( + patch("app.services.estimator.geocode", + new=AsyncMock(return_value=_make_fake_geo())), + patch("app.services.estimator.get_house_metadata", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._fetch_analogs", return_value=([], False, "W")), + patch("app.services.estimator._fetch_deals", return_value=[]), + patch("app.services.estimator._get_or_fetch_imv_cached", new=imv_mock), + patch("app.services.estimator._get_or_fetch_yandex_valuation_cached", + new=yandex_mock), + patch("app.services.estimator.estimate_via_cian_valuation", new=cian_mock), + patch("app.services.estimator._get_asking_sold_ratio", + return_value=(None, None)), + ): + result = await estimate_quality(payload, db) + + assert result.estimate_id is not None + cian_mock.assert_awaited_once() + # IMV is additionally gated on house_type/renovation mapping; with + # house_type=None it is correctly skipped — so we assert on Cian only, + # which is gated purely on floor/total_floors presence. + + anyio.run(_run) + + +def test_estimate_yandex_timeout_degrades_no_5xx() -> None: + """Yandex valuation raising TimeoutError → estimate without it, no exception. + + The _with_budget() guard wraps the call; asyncio.wait_for surfaces a + TimeoutError which must map to the same graceful None path as a network + error. Estimator returns an AggregatedEstimate; 'yandex_valuation' absent. + """ + from app.services.estimator import estimate_quality + + db = MagicMock() + payload = _make_payload_null_floor() + + # Yandex call hangs/raises a timeout — simulate the slow-upstream case. + yandex_mock = AsyncMock(side_effect=TimeoutError("yandex slow")) + + async def _run() -> None: + with ( + patch("app.services.estimator.geocode", + new=AsyncMock(return_value=_make_fake_geo())), + patch("app.services.estimator.get_house_metadata", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._fetch_analogs", return_value=([], False, "W")), + patch("app.services.estimator._fetch_deals", return_value=[]), + patch("app.services.estimator._get_or_fetch_imv_cached", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._get_or_fetch_yandex_valuation_cached", + new=yandex_mock), + patch("app.services.estimator.estimate_via_cian_valuation", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._get_asking_sold_ratio", + return_value=(None, None)), + ): + result = await estimate_quality(payload, db) + + assert result.estimate_id is not None # no 5xx — degraded gracefully + assert "yandex_valuation" not in result.sources_used + + anyio.run(_run) + + +def test_estimate_yandex_httpx_timeout_at_scraper_degrades_no_5xx() -> None: + """Real network path: scraper raises httpx.TimeoutException on cache-miss fetch. + + Here we DON'T patch _get_or_fetch_yandex_valuation_cached — we let the real + cache helper run with a cache MISS, then make the underlying scraper raise + httpx.TimeoutException. The helper's internal `except Exception` maps it to + None (graceful), so the estimate completes without 'yandex_valuation' and + no 5xx. This is the production-realistic timeout path (#654). + """ + from app.services.estimator import estimate_quality + + db = MagicMock() + # Cache MISS so the helper proceeds to the (timing-out) fresh fetch. + db.execute.return_value.mappings.return_value.first.return_value = None + payload = _make_payload_null_floor() + + fake_scraper = MagicMock() + fake_scraper.__aenter__ = AsyncMock(return_value=fake_scraper) + fake_scraper.__aexit__ = AsyncMock(return_value=False) + fake_scraper.fetch_house_history = AsyncMock( + side_effect=httpx.TimeoutException("read timeout") + ) + + async def _run() -> None: + with ( + patch("app.services.estimator.geocode", + new=AsyncMock(return_value=_make_fake_geo())), + patch("app.services.estimator.get_house_metadata", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._fetch_analogs", return_value=([], False, "W")), + patch("app.services.estimator._fetch_deals", return_value=[]), + patch("app.services.estimator._get_or_fetch_imv_cached", + new=AsyncMock(return_value=None)), + patch("app.services.estimator.YandexValuationScraper", + return_value=fake_scraper), + patch("app.services.estimator.estimate_via_cian_valuation", + new=AsyncMock(return_value=None)), + patch("app.services.estimator._get_asking_sold_ratio", + return_value=(None, None)), + ): + result = await estimate_quality(payload, db) + + assert result.estimate_id is not None + assert "yandex_valuation" not in result.sources_used + + anyio.run(_run)