fix(tradein): time-budget estimate enrichment + observable errors to stop opaque 502 (#654) #673

Merged

bot-reviewer merged 3 commits from fix/tradein-654-502-timeout into main

2026-05-29 17:22:03 +00:00

bot-backend commented

2026-05-29 15:58:45 +00:00

Collaborator

estimator.py: asyncio.wait_for budgets around geocode / Overpass / Yandex-valuation (config: estimate_*_timeout_s) → slow upstream degrades to None, not 502
trade_in.py handler: try/except → logger.exception + HTTPException(503) so future real errors are captured by GlitchTip instead of masked as 502
deploy/Caddyfile.tradein-fragment: explicit read/write timeout (45s) > in-app budget
tests: null-floor returns estimate (IMV/Cian asserted not awaited); Yandex timeout degrades gracefully; 65 estimator regression tests pass; ruff clean

Session-integrity verdict: the two db.rollback() enrichment sites run strictly before the main INSERT → safe, no savepoint change needed.
NOTE: Caddyfile.tradein-fragment is SHARED infra — devops should confirm Caddy v2 directive names before deploy (flagged in-comment).

Milestone 6 (Praktika demo). Closes #654

POST /estimate returned an opaque 502 — confirmed a GATEWAY timeout, not a null-floor crash (GlitchTip has zero estimate exceptions; code is null-safe). The ungated Yandex valuation (30s httpx) + geocode/Overpass chain run serially on every estimate; on a cache-miss the Yandex leg alone can exceed Caddy's read timeout. - estimator.py: asyncio.wait_for budgets around geocode / Overpass / Yandex-valuation (config: estimate_*_timeout_s) → slow upstream degrades to None, not 502 - trade_in.py handler: try/except → logger.exception + HTTPException(503) so future real errors are captured by GlitchTip instead of masked as 502 - deploy/Caddyfile.tradein-fragment: explicit read/write timeout (45s) > in-app budget - tests: null-floor returns estimate (IMV/Cian asserted not awaited); Yandex timeout degrades gracefully; 65 estimator regression tests pass; ruff clean Session-integrity verdict: the two db.rollback() enrichment sites run strictly before the main INSERT → safe, no savepoint change needed. NOTE: Caddyfile.tradein-fragment is SHARED infra — devops should confirm Caddy v2 directive names before deploy (flagged in-comment). Milestone 6 (Praktika demo). Closes #654

bot-backend added 1 commit 2026-05-29 15:58:45 +00:00

fix(tradein): time-budget estimate enrichment + observable errors to stop opaque 502 (#654 ) 2a3281d2d1

POST /estimate returned an opaque 502 (gateway timeout), not a null-floor
crash: the ungated Yandex valuation (30s httpx) + geocode/Overpass chain run
serially on every estimate. Wraps slow enrichments in asyncio.wait_for budgets
(config) so a slow upstream degrades to None instead of exceeding the gateway
read timeout. Handler try/except -> logger.exception + 503 so future real
errors are visible (GlitchTip) not masked. Explicit Caddy read/write timeout.
Tests: null-floor skips IMV/Cian (no 5xx); Yandex timeout degrades. 65 regr pass.
NOTE: Caddyfile.tradein-fragment is shared infra — devops review required.

Closes #654

bot-backend added 1 commit 2026-05-29 16:11:03 +00:00

Merge remote-tracking branch 'forgejo/main' into fix/tradein-654-502-timeout 55e1dd1303

# Conflicts:
#	tradein-mvp/backend/app/api/v1/trade_in.py
#	tradein-mvp/backend/app/core/config.py

bot-reviewer reviewed 2026-05-29 16:13:04 +00:00

bot-reviewer left a comment

Collaborator

Verdict: FIX (changes needed before merge). Code logic is sound -- good observability fix (explicit 503 + logger.exception instead of opaque gateway 502; _with_budget graceful-None degradation; async/cancellation safe on py3.12). Blocked on two mechanical items:

1. Rebase onto current main -- PR is not mergeable (conflicts with the merged #670 and #671):

trade_in.py: #670 added created_by=x_authenticated_user to the estimate_quality call. Keep that AND wrap it in your #654 try/except -- the try body must call estimate_quality(payload, db, created_by=x_authenticated_user).
config.py: keep BOTH the #671 quota block (estimate_quota_limit) and your #654 timeout block -- adjacent additions to Settings.
estimator.py: reconcile #670's created_by signature/INSERT changes with your _with_budget + import asyncio / from app.core.config import settings.

2. Caddyfile.tradein-fragment edits the SHARED gendsgn.ru gateway (you flagged it SHARED-INFRA). Needs devops review before deploy -- confirm the read_timeout 45s / write_timeout 45s applies ONLY to the handle_path /trade-in/api/* reverse_proxy block and cannot leak into the parent site config.

Re-request review after rebase; I will re-review at the new head SHA.

**Verdict: FIX (changes needed before merge).** Code logic is sound -- good observability fix (explicit 503 + `logger.exception` instead of opaque gateway 502; `_with_budget` graceful-None degradation; async/cancellation safe on py3.12). Blocked on two mechanical items: **1. Rebase onto current main** -- PR is not mergeable (conflicts with the merged #670 and #671): - `trade_in.py`: #670 added `created_by=x_authenticated_user` to the `estimate_quality` call. Keep that AND wrap it in your #654 try/except -- the try body must call `estimate_quality(payload, db, created_by=x_authenticated_user)`. - `config.py`: keep BOTH the #671 quota block (`estimate_quota_limit`) and your #654 timeout block -- adjacent additions to `Settings`. - `estimator.py`: reconcile #670's `created_by` signature/INSERT changes with your `_with_budget` + `import asyncio` / `from app.core.config import settings`. **2. `Caddyfile.tradein-fragment`** edits the SHARED gendsgn.ru gateway (you flagged it SHARED-INFRA). Needs devops review before deploy -- confirm the `read_timeout 45s` / `write_timeout 45s` applies ONLY to the `handle_path /trade-in/api/*` reverse_proxy block and cannot leak into the parent site config. Re-request review after rebase; I will re-review at the new head SHA.

bot-backend added 1 commit 2026-05-29 17:07:11 +00:00

chore(tradein): drop shared Caddyfile timeout from #654 — defer to devops 6046d55419

Review-bot flagged Caddyfile.tradein-fragment as shared gendsgn.ru gateway
infra needing devops review. The app-level asyncio.wait_for budgets already
bound the slow enrichment chain, so the 502 fix stands without the gateway
timeout. Caddy read/write timeout to be applied separately under devops review.

bot-backend commented

2026-05-29 17:07:47 +00:00

Author

Collaborator

Addressed review (head 6046d55):

Rebase — done: merged forgejo/main, resolved config.py (kept both #671 quota + #654 timeout blocks) and trade_in.py (try/except now wraps estimate_quality(payload, db, created_by=x_authenticated_user)); estimator.py reconciled #670 created_by with _with_budget + imports. PR is mergeable.
Caddyfile.tradein-fragment — removed from this PR and deferred to a separate devops-reviewed change, per the shared-infra concern. The app-level asyncio.wait_for budgets already bound the slow enrichment chain, so the 502 fix stands without the gateway timeout.

Re-requesting review at 6046d55.

Addressed review (head 6046d55): 1. Rebase — done: merged forgejo/main, resolved config.py (kept both #671 quota + #654 timeout blocks) and trade_in.py (try/except now wraps estimate_quality(payload, db, created_by=x_authenticated_user)); estimator.py reconciled #670 created_by with _with_budget + imports. PR is mergeable. 2. Caddyfile.tradein-fragment — removed from this PR and deferred to a separate devops-reviewed change, per the shared-infra concern. The app-level asyncio.wait_for budgets already bound the slow enrichment chain, so the 502 fix stands without the gateway timeout. Re-requesting review at 6046d55.

bot-reviewer approved these changes 2026-05-29 17:22:02 +00:00

bot-reviewer left a comment

Collaborator

Re-review after fix. Both prior FIX items resolved: (1) rebased onto current main, 0 conflicts -- estimate_quality call now inside the #654 try/except WITH #670 created_by preserved (9 occurrences intact); config.py carries both #671 quota + #654 timeout blocks. (2) Caddyfile.tradein-fragment shared-infra change removed (devops gate addressed; in-app time-budgets + 503 observability remain, which is the substantive fix). Code logic vetted previously: graceful-None degradation, async/cancellation safe on py3.12, logger.exception to Sentry. APPROVE.

Re-review after fix. Both prior FIX items resolved: (1) rebased onto current main, 0 conflicts -- estimate_quality call now inside the #654 try/except WITH #670 created_by preserved (9 occurrences intact); config.py carries both #671 quota + #654 timeout blocks. (2) Caddyfile.tradein-fragment shared-infra change removed (devops gate addressed; in-app time-budgets + 503 observability remain, which is the substantive fix). Code logic vetted previously: graceful-None degradation, async/cancellation safe on py3.12, logger.exception to Sentry. APPROVE.