fix(tradein-estimator): atomic IMV link, batched yandex history, drop redundant CAST #509

Merged

lekss361 merged 1 commit from feat/tradein-estimator-tx-fixes into main

2026-05-24 11:18:13 +00:00

lekss361 commented

2026-05-24 11:02:14 +00:00

Owner

Summary

3 fixes inside services/estimator.py from 2026-05-24 audit:

#4 IMV link UPDATE moved inside main tx — closes race where two concurrent estimates sharing a cache_key could overwrite each other's estimate_id after the main commit. New flow: INSERT trade_in_estimates → conditional UPDATE avito_imv_evaluations → single commit → log. No inner try/except (let DB errors bubble — atomicity is the point).
#5 _save_yandex_history_items batched under single try/except — prior per-item db.rollback() destroyed the parent transaction; next iteration could run on a rolled-back session. New shape: build params first, single try/except around the loop, one outer rollback() if anything fails, commit() once on success.
#9 Drop redundant CAST inside IS NOT NULL predicates in _fetch_analogs — CAST(NULL AS T) IS NOT NULL is equivalent to NULL IS NOT NULL but evaluates per-row. THEN-branch CASTs preserved (typed arithmetic / <> comparison require explicit type).

Test impact

test_save_history_items_db_error_continues in tests/test_estimator_yandex_integration.py was asserting old per-item-continue semantics (saved == 1 after 1 of 2 items failed). Renamed to _rolls_back_batch and updated to assert new batch semantics (saved == 0, db.rollback.assert_called_once(), db.commit.assert_not_called()). Included in same commit.

pytest tradein-mvp/backend/tests/test_estimator_*.py → 22 passed in 0.94s locally.

Test plan

After deploy: run two concurrent POST /api/v1/trade-in/estimate with same address (same cache_key); verify SELECT estimate_id, fetched_at FROM avito_imv_evaluations WHERE cache_key = ... resolves to the winner's estimate_id (not flapping).
Force a Yandex history item with invalid raw payload to trigger batch failure; verify no partial inserts and commit() not called.
EXPLAIN ANALYZE on _fetch_analogs SQL before/after; CAST removal should drop ~6 function calls per row (300 rows max → tiny but free win).

## Summary 3 fixes inside `services/estimator.py` from 2026-05-24 audit: - **#4** IMV link UPDATE moved inside main tx — closes race where two concurrent estimates sharing a `cache_key` could overwrite each other's `estimate_id` after the main commit. New flow: `INSERT trade_in_estimates → conditional UPDATE avito_imv_evaluations → single commit → log`. No inner try/except (let DB errors bubble — atomicity is the point). - **#5** `_save_yandex_history_items` batched under single try/except — prior per-item `db.rollback()` destroyed the parent transaction; next iteration could run on a rolled-back session. New shape: build params first, single try/except around the loop, one outer `rollback()` if anything fails, `commit()` once on success. - **#9** Drop redundant CAST inside `IS NOT NULL` predicates in `_fetch_analogs` — `CAST(NULL AS T) IS NOT NULL` is equivalent to `NULL IS NOT NULL` but evaluates per-row. THEN-branch CASTs preserved (typed arithmetic / `<>` comparison require explicit type). ## Test impact `test_save_history_items_db_error_continues` in `tests/test_estimator_yandex_integration.py` was asserting old per-item-continue semantics (`saved == 1` after 1 of 2 items failed). Renamed to `_rolls_back_batch` and updated to assert new batch semantics (`saved == 0`, `db.rollback.assert_called_once()`, `db.commit.assert_not_called()`). Included in same commit. `pytest tradein-mvp/backend/tests/test_estimator_*.py` → **22 passed in 0.94s** locally. ## Test plan - [ ] After deploy: run two concurrent `POST /api/v1/trade-in/estimate` with same address (same `cache_key`); verify `SELECT estimate_id, fetched_at FROM avito_imv_evaluations WHERE cache_key = ...` resolves to the winner's estimate_id (not flapping). - [ ] Force a Yandex history item with invalid raw payload to trigger batch failure; verify no partial inserts and `commit()` not called. - [ ] EXPLAIN ANALYZE on `_fetch_analogs` SQL before/after; CAST removal should drop ~6 function calls per row (300 rows max → tiny but free win).

lekss361 added 1 commit 2026-05-24 11:02:15 +00:00

fix(tradein-estimator): atomic IMV link, batched yandex history, drop redundant CAST 872c83caf5

3 fixes inside services/estimator.py from 2026-05-24 audit:

* IMV link UPDATE moved inside main tx (finding #4) — closes race where two
  concurrent estimates sharing a cache_key could overwrite each other's
  estimate_id after the main commit.
* _save_yandex_history_items batched under single try/except (finding #5) —
  prior per-item db.rollback() destroyed the parent transaction; next
  iteration could run on a rolled-back session.
* Drop redundant CAST inside IS NOT NULL predicates in _fetch_analogs
  (finding #9) — CAST(NULL AS T) IS NOT NULL is equivalent to NULL IS NOT NULL
  but evaluates per-row. THEN-branch CASTs preserved (typed arithmetic).

test: update test_save_history_items_db_error_continues → batch rollback semantics

lekss361 commented

2026-05-24 11:18:06 +00:00

Author

Owner

Deep Code Review — PR #509 — verdict APPROVE

Summary

Status: APPROVE
Files: 2 (P1: services/estimator.py, P2: tests/test_estimator_yandex_integration.py)
Lines: +78 / -66
SHA: 872c83c · matches head

Atomicity — IMV link UPDATE (finding #4)

estimator.py:677-698 — INSERT trade_in_estimates + conditional UPDATE avito_imv_evaluations now share one tx with a single COMMIT. Correct.
WHERE clause cache_key = :ck AND (estimate_id IS NULL OR estimate_id = :estimate_id) was already idempotent — atomic move strengthens the invariant ("estimate saved ⇒ link attempted") but the race window was narrower than the description implies. Still net positive.
No deadlock with PR #501 pg_advisory_xact_lock(42, hashtext(fp)): estimator.py does NOT call match_or_create_house directly (grep confirmed); the lock fires only from scrapers writing into houses, on a disjoint key namespace. Lock-order graph stays acyclic.
Minor regression note (not blocking): old code wrapped the UPDATE in try/except so a row-lock conflict on avito_imv_evaluations would log a warning and still return the estimate. New code lets DB errors bubble — a transient lock contention on the IMV row now fails the whole POST /estimate. Acceptable trade-off since the UPDATE touches a single row keyed by UNIQUE cache_key with PK-style locking (very low contention in practice), but worth knowing for ops.

Batch tx — `_save_yandex_history_items` (finding #5)

estimator.py:336-408 — single try/except around the loop, single rollback() / single commit(). Fixes the prior bug where per-item db.rollback() destroyed the parent tx and corrupted subsequent iterations.
Early-return if not result.history_items: return 0 — correctly avoids issuing a no-op commit on an empty batch.
ON CONFLICT (source, ext_item_id) DO NOTHING preserves idempotency. Hash is stable across calls (existing test _ext_id_stable_across_calls).
Nit (not blocking): for row in rows: db.execute(sql, row) could become db.execute(sql, rows) (SQLAlchemy 2.0 executemany) — fewer round-trips. Skip for now, scope is correctness not perf.

CAST cleanup (finding #9)

estimator.py:832, 836, 851, 855 — removed CAST(:target_year AS integer) IS NOT NULL and same for target_house_type. Correct: CAST(NULL AS T) IS NOT NULL ≡ NULL IS NOT NULL ≡ false, the cast adds nothing in IS-NULL context. THEN-branch CASTs preserved where arithmetic/<> need typed values — comment in code captures the AmbiguousParameter reasoning.

Test coverage

test_estimator_yandex_integration.py:191-198 — renamed _db_error_continues → _rolls_back_batch, asserts saved == 0, db.rollback.assert_called_once(), db.commit.assert_not_called(). Matches new semantics. Other 21 tests preserved.
PR claims pytest tradein-mvp/backend/tests/test_estimator_*.py → 22 passed — plausible given file layout.

Cross-file impact

avito_imv.py:save_imv_evaluation still commits its own row before we re-enter estimator's atomic block — that's by design (cache write must survive even if final estimate INSERT fails downstream); the link UPDATE is a separate concern handled atomically in this PR.
data/sql/030_avito_imv_cache_key_unique.sql ensures UNIQUE (cache_key) — required for the race-protection WHERE clause to mean "first writer wins".
No frontend / API contract changes; response shape of POST /estimate unchanged.

Vault cross-ref

PR description references "2026-05-24 audit" findings #4/#5/#9 — these are NOT the items from decisions/Decision_TradeIn_DataQuality_8PR_Roadmap.md (which numbers 1-13) nor from audits/TradeIn_MVP_CodeReview_May23.md (C-/H-/M-/L- scheme). Suggest filing a short fix-note in vault post-merge so the audit trail isn't orphaned.

Pre-flight

Branch feat/tradein-estimator-tx-fixes — correct shape.
mergeable: true, base on commit 7f23a9b, merge_base 099b8e4. No conflicts.
No --no-verify / --force / --amend signals in PR.

Verdict

Clean, focused fix. All three findings addressed precisely with no scope creep. Test renamed correctly. No security / correctness / perf regressions. Merging.

## Deep Code Review — PR #509 — verdict APPROVE ### Summary - Status: APPROVE - Files: 2 (P1: services/estimator.py, P2: tests/test_estimator_yandex_integration.py) - Lines: +78 / -66 - SHA: 872c83c · matches head ### Atomicity — IMV link UPDATE (finding #4) - `estimator.py:677-698` — INSERT trade_in_estimates + conditional UPDATE avito_imv_evaluations now share one tx with a single COMMIT. Correct. - WHERE clause `cache_key = :ck AND (estimate_id IS NULL OR estimate_id = :estimate_id)` was already idempotent — atomic move strengthens the invariant ("estimate saved ⇒ link attempted") but the race window was narrower than the description implies. Still net positive. - No deadlock with PR #501 `pg_advisory_xact_lock(42, hashtext(fp))`: estimator.py does NOT call `match_or_create_house` directly (grep confirmed); the lock fires only from scrapers writing into `houses`, on a disjoint key namespace. Lock-order graph stays acyclic. - Minor regression note (not blocking): old code wrapped the UPDATE in try/except so a row-lock conflict on `avito_imv_evaluations` would log a warning and still return the estimate. New code lets DB errors bubble — a transient lock contention on the IMV row now fails the whole `POST /estimate`. Acceptable trade-off since the UPDATE touches a single row keyed by UNIQUE `cache_key` with PK-style locking (very low contention in practice), but worth knowing for ops. ### Batch tx — `_save_yandex_history_items` (finding #5) - `estimator.py:336-408` — single try/except around the loop, single rollback() / single commit(). Fixes the prior bug where per-item `db.rollback()` destroyed the parent tx and corrupted subsequent iterations. - Early-return `if not result.history_items: return 0` — correctly avoids issuing a no-op commit on an empty batch. - ON CONFLICT (source, ext_item_id) DO NOTHING preserves idempotency. Hash is stable across calls (existing test `_ext_id_stable_across_calls`). - Nit (not blocking): `for row in rows: db.execute(sql, row)` could become `db.execute(sql, rows)` (SQLAlchemy 2.0 executemany) — fewer round-trips. Skip for now, scope is correctness not perf. ### CAST cleanup (finding #9) - `estimator.py:832, 836, 851, 855` — removed `CAST(:target_year AS integer) IS NOT NULL` and same for `target_house_type`. Correct: `CAST(NULL AS T) IS NOT NULL ≡ NULL IS NOT NULL ≡ false`, the cast adds nothing in IS-NULL context. THEN-branch CASTs preserved where arithmetic/`<>` need typed values — comment in code captures the AmbiguousParameter reasoning. ### Test coverage - `test_estimator_yandex_integration.py:191-198` — renamed `_db_error_continues` → `_rolls_back_batch`, asserts `saved == 0`, `db.rollback.assert_called_once()`, `db.commit.assert_not_called()`. Matches new semantics. Other 21 tests preserved. - PR claims `pytest tradein-mvp/backend/tests/test_estimator_*.py → 22 passed` — plausible given file layout. ### Cross-file impact - `avito_imv.py:save_imv_evaluation` still commits its own row before we re-enter estimator's atomic block — that's by design (cache write must survive even if final estimate INSERT fails downstream); the link UPDATE is a separate concern handled atomically in this PR. - `data/sql/030_avito_imv_cache_key_unique.sql` ensures UNIQUE (cache_key) — required for the race-protection WHERE clause to mean "first writer wins". - No frontend / API contract changes; response shape of POST /estimate unchanged. ### Vault cross-ref - PR description references "2026-05-24 audit" findings #4/#5/#9 — these are NOT the items from `decisions/Decision_TradeIn_DataQuality_8PR_Roadmap.md` (which numbers 1-13) nor from `audits/TradeIn_MVP_CodeReview_May23.md` (C-/H-/M-/L- scheme). Suggest filing a short fix-note in vault post-merge so the audit trail isn't orphaned. ### Pre-flight - Branch `feat/tradein-estimator-tx-fixes` — correct shape. - `mergeable: true`, base on commit 7f23a9b, merge_base 099b8e4. No conflicts. - No `--no-verify` / `--force` / `--amend` signals in PR. ### Verdict Clean, focused fix. All three findings addressed precisely with no scope creep. Test renamed correctly. No security / correctness / perf regressions. Merging.