lekss361/gendesign

Fork 0

fix(tradein-matching): pg_advisory_xact_lock to prevent duplicate house INSERTs #501

Merged

lekss361 merged 2 commits from feat/tradein-houses-advisory-lock into main

2026-05-24 10:52:11 +00:00

lekss361 commented

2026-05-24 10:16:36 +00:00

Owner

Summary

Close finding #1 from 2026-05-24 trade-in audit.

Two scrapers calling match_or_create_house() concurrently for the same address could both miss Tier 0-3 (cadastr / source / fingerprint / geo) and each INSERT a new canonical house — splitting listings across duplicate house rows and breaking aggregations downstream.

Fix: serialize via pg_advisory_xact_lock(42, hashtext(fingerprint)) acquired right after fp computation, before any tier reads. Lock is transaction-scoped (released on caller's COMMIT/ROLLBACK). After unblock, the losing scraper's Tier 2 fingerprint lookup hits the winner's alias and returns the same house_id.

Namespace 42 reserves a private advisory-lock slot for trade-in house matching (no collision with other locks in the system).
address_fingerprint(None, lat, lon) returns a valid 32-char hex even for None address (normalize_address(address or '') guard in normalize.py:72) — lock call is safe for all callers.
Function docstring updated: old KNOWN LIMITATION block replaced with concurrency-safe description.

Test plan

After deploy, simulate two concurrent scraper runs with overlapping fresh addresses; confirm only one new houses row per unique fingerprint (compare counts before/after).
EXPLAIN (ANALYZE) SELECT pg_advisory_xact_lock(42, hashtext('test')) runs in < 1ms (sanity check, no blocking).
No long-running tx holding the lock: monitor pg_locks WHERE locktype = 'advisory' during a sweep.

## Summary Close finding #1 from 2026-05-24 trade-in audit. Two scrapers calling `match_or_create_house()` concurrently for the same address could both miss Tier 0-3 (cadastr / source / fingerprint / geo) and each INSERT a new canonical house — splitting listings across duplicate house rows and breaking aggregations downstream. Fix: serialize via `pg_advisory_xact_lock(42, hashtext(fingerprint))` acquired right after fp computation, before any tier reads. Lock is transaction-scoped (released on caller's COMMIT/ROLLBACK). After unblock, the losing scraper's Tier 2 fingerprint lookup hits the winner's alias and returns the same `house_id`. - Namespace `42` reserves a private advisory-lock slot for trade-in house matching (no collision with other locks in the system). - `address_fingerprint(None, lat, lon)` returns a valid 32-char hex even for None address (`normalize_address(address or '')` guard in `normalize.py:72`) — lock call is safe for all callers. - Function docstring updated: old `KNOWN LIMITATION` block replaced with concurrency-safe description. ## Test plan - [ ] After deploy, simulate two concurrent scraper runs with overlapping fresh addresses; confirm only one new `houses` row per unique fingerprint (compare counts before/after). - [ ] `EXPLAIN (ANALYZE) SELECT pg_advisory_xact_lock(42, hashtext('test'))` runs in < 1ms (sanity check, no blocking). - [ ] No long-running tx holding the lock: monitor `pg_locks WHERE locktype = 'advisory'` during a sweep.

lekss361 added 1 commit 2026-05-24 10:16:37 +00:00

fix(tradein-matching): pg_advisory_xact_lock to prevent duplicate house INSERTs 9a6890de6a

Two scrapers calling match_or_create_house() concurrently for the same address
could both miss Tier 0-3 and each INSERT a new canonical house — splitting
listings across duplicate house rows and breaking aggregations.

Serialize via pg_advisory_xact_lock(42, hashtext(fingerprint)). Lock is
transaction-scoped (released on caller's COMMIT/ROLLBACK). After unblock, the
losing scraper's Tier 2 fingerprint lookup hits the winner's alias and returns
the same house_id.

Closes finding #1 from 2026-05-24 trade-in audit.

lekss361 commented

2026-05-24 10:32:07 +00:00

Author

Owner

Deep Code Review — PR #501

Verdict: CHANGES REQUESTED

Fix is correct in design — pg_advisory_xact_lock(42, hashtext(fp)) after fp computation, before any tier reads, with two-arg form using a private namespace (cannot collide with single-arg locks like cadastre_fetch.py:225). Lock release tied to caller's COMMIT/ROLLBACK is the right tx-scope. Parametrized bind, no SQL injection surface, line length OK, psycopg v3-compatible.

🔴 Blocker — tests will fail on CI

tradein-mvp/backend/tests/test_matching.py has 5 tests (test_match_house_tier0_cadastr, _tier1_source_exact, _tier2_fingerprint, _tier3_geo, _new) that build db.execute.side_effect lists assuming the first execute call hits the relevant tier query. The new advisory-lock call is now the first db.execute(...) — every test will consume its first mock row for the lock and read off-by-one for the subsequent tier query (Tier 0 cadastr lookup gets None, falls through, etc.).

Fixup needed in each test's _make_db([...]) list — prepend a None row for the lock call. Example for test_match_house_tier0_cadastr:

db = _make_db([
    None,         # NEW: pg_advisory_xact_lock result
    {'id': 42},   # cadastral match
    None,         # _upsert_house_source
])

Applies to all 5 test_match_house_* cases. test_match_listing_* tests are unaffected (different function).

🟡 Recommended additions

Add a regression test asserting the lock call happens first and uses the fingerprint. Even a simple assert db.execute.call_args_list[0][0][0].text.startswith('SELECT pg_advisory_xact_lock') would prevent future re-ordering bugs.
PR test_plan checkboxes are unchecked — none of the three planned validations (concurrent-scraper sim, EXPLAIN sanity, pg_locks monitoring) appear to have run. At least the EXPLAIN sanity check is trivial to run pre-merge.

🟢 Observations (non-blocking)

No current callers in tradein-mvp/backend/app/ invoke match_or_create_house() yet — only the __init__.py re-export and tests. The race described in the PR body is preemptive hardening before scrapers are wired in (per old/decisions/Cross_Source_Matching_Strategy.md plan). This is fine, but worth noting that the fix can't be empirically validated under load until scrapers are connected.
Lock acquired even on Tier 0/1 fast paths (cadastr-only calls still serialize on hashtext(fp)). This is the right trade-off for safety — moving the lock past Tier 1 would re-introduce a TOCTOU window — but adds ~0.3-1ms per call. Bulk scrapers @ 100k calls ≈ +30-100s aggregate. Acceptable.
hashtext collision: 32-bit space → ~50% collision at ~65k concurrent unique fps. Two unrelated addresses with hash-colliding fps will spuriously block each other (correctness still preserved, just over-serialization). Fine at current scale.
Namespace 42 documented inline but not in a project-wide registry. If future code adds two-arg advisory locks, consider a constants module to avoid silent collisions.
Vault cross-ref: archived old/decisions/Cross_Source_Matching_Strategy.md documents the planned matching layer but does not yet describe this concurrency fix. Worth a /vault-write note after merge so future readers find the rationale.

Blast radius

File: tradein-mvp/backend/app/services/matching/houses.py (P1 — service layer, currently un-called)
Tests: 5 existing tests will break (must update mocks)
Migration / DDL: none
Reversibility: trivial (single function, one SQL call)

Next steps

Update _make_db([...]) rows in 5 test_match_house_* tests to include the lock result slot.
Optionally add regression assertion on lock call ordering.
Run pytest tradein-mvp/backend/tests/test_matching.py -v locally; confirm green.
Then re-push for re-review.

⚠️ PR touches blocked scope (backend/app/services/matching/houses.py — actually tradein-mvp/backend/...) — leaving merge decision to human reviewer regardless of verdict.

## Deep Code Review — PR #501 **Verdict: CHANGES REQUESTED** Fix is correct in design — `pg_advisory_xact_lock(42, hashtext(fp))` after fp computation, before any tier reads, with two-arg form using a private namespace (cannot collide with single-arg locks like `cadastre_fetch.py:225`). Lock release tied to caller's COMMIT/ROLLBACK is the right tx-scope. Parametrized bind, no SQL injection surface, line length OK, psycopg v3-compatible. ### 🔴 Blocker — tests will fail on CI `tradein-mvp/backend/tests/test_matching.py` has 5 tests (`test_match_house_tier0_cadastr`, `_tier1_source_exact`, `_tier2_fingerprint`, `_tier3_geo`, `_new`) that build `db.execute.side_effect` lists assuming the first execute call hits the relevant tier query. The new advisory-lock call is now the **first** `db.execute(...)` — every test will consume its first mock row for the lock and read off-by-one for the subsequent tier query (Tier 0 cadastr lookup gets `None`, falls through, etc.). Fixup needed in each test's `_make_db([...])` list — prepend a `None` row for the lock call. Example for `test_match_house_tier0_cadastr`: ```python db = _make_db([ None, # NEW: pg_advisory_xact_lock result {'id': 42}, # cadastral match None, # _upsert_house_source ]) ``` Applies to all 5 `test_match_house_*` cases. `test_match_listing_*` tests are unaffected (different function). ### 🟡 Recommended additions 1. **Add a regression test** asserting the lock call happens first and uses the fingerprint. Even a simple `assert db.execute.call_args_list[0][0][0].text.startswith('SELECT pg_advisory_xact_lock')` would prevent future re-ordering bugs. 2. **PR test_plan checkboxes are unchecked** — none of the three planned validations (concurrent-scraper sim, EXPLAIN sanity, `pg_locks` monitoring) appear to have run. At least the EXPLAIN sanity check is trivial to run pre-merge. ### 🟢 Observations (non-blocking) - **No current callers** in `tradein-mvp/backend/app/` invoke `match_or_create_house()` yet — only the `__init__.py` re-export and tests. The race described in the PR body is preemptive hardening before scrapers are wired in (per `old/decisions/Cross_Source_Matching_Strategy.md` plan). This is fine, but worth noting that the fix can't be empirically validated under load until scrapers are connected. - **Lock acquired even on Tier 0/1 fast paths** (cadastr-only calls still serialize on `hashtext(fp)`). This is the right trade-off for safety — moving the lock past Tier 1 would re-introduce a TOCTOU window — but adds ~0.3-1ms per call. Bulk scrapers @ 100k calls ≈ +30-100s aggregate. Acceptable. - **hashtext collision**: 32-bit space → ~50% collision at ~65k concurrent unique fps. Two unrelated addresses with hash-colliding fps will spuriously block each other (correctness still preserved, just over-serialization). Fine at current scale. - **Namespace 42 documented inline** but not in a project-wide registry. If future code adds two-arg advisory locks, consider a constants module to avoid silent collisions. - Vault cross-ref: archived `old/decisions/Cross_Source_Matching_Strategy.md` documents the planned matching layer but does not yet describe this concurrency fix. Worth a `/vault-write` note after merge so future readers find the rationale. ### Blast radius - File: `tradein-mvp/backend/app/services/matching/houses.py` (P1 — service layer, currently un-called) - Tests: 5 existing tests will break (must update mocks) - Migration / DDL: none - Reversibility: trivial (single function, one SQL call) ### Next steps 1. Update `_make_db([...])` rows in 5 `test_match_house_*` tests to include the lock result slot. 2. Optionally add regression assertion on lock call ordering. 3. Run `pytest tradein-mvp/backend/tests/test_matching.py -v` locally; confirm green. 4. Then re-push for re-review. ⚠️ PR touches blocked scope (`backend/app/services/matching/houses.py` — actually `tradein-mvp/backend/...`) — leaving merge decision to human reviewer regardless of verdict.

lekss361 referenced this pull request from a commit

2026-05-24 10:37:56 +00:00

test(tradein-matching): fix off-by-one in 5 match_house tests + add lock regression

lekss361 added 1 commit 2026-05-24 10:37:57 +00:00

test(tradein-matching): fix off-by-one in 5 match_house tests + add lock regression cea2af8d3d

Advisory lock in match_or_create_house() is now the first db.execute() call;
existing tests built side_effect lists assuming the tier query was first,
causing each to read off-by-one (Tier 0 cadastr lookup gets None, falls
through, etc.).

Prepend a None row to all 5 test_match_house_* _make_db lists. Add
test_match_house_advisory_lock_called_first as a regression guard against
future re-ordering of the lock call.

Per PR #501 reviewer feedback.

lekss361 commented

2026-05-24 10:44:45 +00:00

Author

Owner

Deep Code Review — PR #501 (re-review)

Verdict: APPROVE ✅ — fixup correctly addresses the test mock execute-order issue from prior review (sha=9a6890d).

Mock execute-order verification (all 5 tests + 1 new)

Walked through each test against actual match_or_create_house call chain at sha=cea2af8:

Test	Expected execute chain	Mock list	Match
`tier0_cadastr`	lock → cadastr SELECT → _upsert	`[None, {id:42}, None]`	✅
`tier1_source_exact`	lock → house_sources SELECT (returns early, no _upsert per source code)	`[None, {house_id:7}]`	✅
`tier2_fingerprint`	lock → house_sources(miss) → fp SELECT → _upsert	`[None, None, {house_id:15}, None]`	✅
`tier3_geo`	lock → house_sources(miss) → fp(miss) → geo SELECT → _upsert → _insert_alias	`[None, None, None, {id:22,dist:15.5}, None, None]`	✅
`new`	lock → house_sources(miss) → fp(miss) → geo(miss) → INSERT RETURNING → _upsert → _insert_alias	`[None, None, None, None, {id:99}, None, None]`	✅
`advisory_lock_called_first` (new regression test)	asserts `first_call[0][0]` contains `pg_advisory_xact_lock`, bind has `fp` of 32-char hex	—	✅ correct introspection of `call_args_list[0]`

Advisory-lock logic regression check

fp = address_fingerprint(...) is computed once at function entry (line 58), used by lock + Tier 2 + Tier 3 alias + new-house alias — no shadowing, no recompute.
Old fp = address_fingerprint(...) line inside Tier 2 region correctly removed (was line 86 in pre-fixup).
text('SELECT pg_advisory_xact_lock(42, hashtext(:fp))') is parametrized bind, no SQL injection surface, psycopg v3-compatible.
Two-arg form with namespace 42 cannot collide with single-arg locks elsewhere in repo (e.g. cadastre_fetch.py:225).
Lock is xact-scoped → released on caller's COMMIT/ROLLBACK.

Deadlock risk

Lock acquisition is the first DB operation in the function — no prior reads/writes that could create ordering inversions with other tables. No nested calls to match_or_create_house. Concurrent calls for same fp serialize on the same lock key; different fps are fully independent. hashtext collision case = over-serialization (correctness preserved), not deadlock. Risk: low.

🟢 Observations (non-blocking)

test_match_house_advisory_lock_called_first is a well-placed regression guard — exactly what was recommended in prior review.
Bind-payload assertion (len == 32, str) is tight enough to catch silent breakage if someone changes address_fingerprint() return shape.
PR description test_plan checkboxes still unchecked (concurrent-scraper sim, EXPLAIN sanity, pg_locks monitoring) — empirical validation deferred until scrapers are wired. Acceptable per prior review (no current callers in tradein-mvp/backend/app/).

Vault cross-check

No new vault entry yet — recommend /vault-write post-merge documenting the namespace 42 registry convention for future two-arg advisory locks (prevents silent collision).

Blast radius

tradein-mvp/backend/app/services/matching/houses.py (P1 — service layer, currently un-called by app code)
tradein-mvp/backend/tests/test_matching.py (5 updated + 1 new regression test)
Migration / DDL: none
Reversibility: trivial

Pre-flight

Tests will now pass (verified mock count == execute call count for every branch)
Pre-commit hooks: assumed clean (no ruff-fixable in diff)
Branch not main: ✅

✅ ready for human merge (tests fix verified) — blocked scope (backend/app/services/...), leaving merge to human reviewer per policy.

## Deep Code Review — PR #501 (re-review) **Verdict: APPROVE** ✅ — fixup correctly addresses the test mock execute-order issue from prior review (sha=9a6890d). ### Mock execute-order verification (all 5 tests + 1 new) Walked through each test against actual `match_or_create_house` call chain at sha=cea2af8: | Test | Expected execute chain | Mock list | Match | |---|---|---|---| | `tier0_cadastr` | lock → cadastr SELECT → _upsert | `[None, {id:42}, None]` | ✅ | | `tier1_source_exact` | lock → house_sources SELECT (returns early, no _upsert per source code) | `[None, {house_id:7}]` | ✅ | | `tier2_fingerprint` | lock → house_sources(miss) → fp SELECT → _upsert | `[None, None, {house_id:15}, None]` | ✅ | | `tier3_geo` | lock → house_sources(miss) → fp(miss) → geo SELECT → _upsert → _insert_alias | `[None, None, None, {id:22,dist:15.5}, None, None]` | ✅ | | `new` | lock → house_sources(miss) → fp(miss) → geo(miss) → INSERT RETURNING → _upsert → _insert_alias | `[None, None, None, None, {id:99}, None, None]` | ✅ | | `advisory_lock_called_first` (new regression test) | asserts `first_call[0][0]` contains `pg_advisory_xact_lock`, bind has `fp` of 32-char hex | — | ✅ correct introspection of `call_args_list[0]` | ### Advisory-lock logic regression check - `fp = address_fingerprint(...)` is computed once at function entry (line 58), used by lock + Tier 2 + Tier 3 alias + new-house alias — no shadowing, no recompute. - Old `fp = address_fingerprint(...)` line inside Tier 2 region correctly removed (was line 86 in pre-fixup). - `text('SELECT pg_advisory_xact_lock(42, hashtext(:fp))')` is parametrized bind, no SQL injection surface, psycopg v3-compatible. - Two-arg form with namespace `42` cannot collide with single-arg locks elsewhere in repo (e.g. `cadastre_fetch.py:225`). - Lock is xact-scoped → released on caller's COMMIT/ROLLBACK. ### Deadlock risk Lock acquisition is the **first** DB operation in the function — no prior reads/writes that could create ordering inversions with other tables. No nested calls to `match_or_create_house`. Concurrent calls for same fp serialize on the same lock key; different fps are fully independent. hashtext collision case = over-serialization (correctness preserved), not deadlock. Risk: **low**. ### 🟢 Observations (non-blocking) - `test_match_house_advisory_lock_called_first` is a well-placed regression guard — exactly what was recommended in prior review. - Bind-payload assertion (`len == 32`, str) is tight enough to catch silent breakage if someone changes `address_fingerprint()` return shape. - PR description test_plan checkboxes still unchecked (concurrent-scraper sim, EXPLAIN sanity, pg_locks monitoring) — empirical validation deferred until scrapers are wired. Acceptable per prior review (no current callers in `tradein-mvp/backend/app/`). ### Vault cross-check No new vault entry yet — recommend `/vault-write` post-merge documenting the namespace `42` registry convention for future two-arg advisory locks (prevents silent collision). ### Blast radius - `tradein-mvp/backend/app/services/matching/houses.py` (P1 — service layer, currently un-called by app code) - `tradein-mvp/backend/tests/test_matching.py` (5 updated + 1 new regression test) - Migration / DDL: none - Reversibility: trivial ### Pre-flight - Tests will now pass (verified mock count == execute call count for every branch) - Pre-commit hooks: assumed clean (no ruff-fixable in diff) - Branch not main: ✅ ✅ **ready for human merge (tests fix verified)** — blocked scope (`backend/app/services/...`), leaving merge to human reviewer per policy.

lekss361 merged commit 1026165abb into main

2026-05-24 10:52:11 +00:00

lekss361 deleted branch feat/tradein-houses-advisory-lock

2026-05-24 10:52:11 +00:00

lekss361 referenced this pull request from a commit

2026-05-24 10:52:11 +00:00

fix(tradein-matching): pg_advisory_xact_lock to prevent duplicate house INSERTs (#501)

lekss361 referenced this pull request

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

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

lekss361 referenced this pull request

2026-05-24 14:17:25 +00:00