From de71f9abb609e55a115ded38defd104c763ed0f0 Mon Sep 17 00:00:00 2001 From: Jun-te Kim Date: Wed, 24 Jun 2026 17:01:41 +0000 Subject: [PATCH] Resolve overrides on the unit's own session, not a second connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The modelling_e2e Lambda runs on a single-connection pool (pool_size=1, max_overflow=0) so one invocation uses one Postgres connection. But re-hydrating a Property through PostgresUnitOfWork resolved its Landlord Overrides through a PropertyOverridesPostgresReader built from the unit's session *factory* — which opens a brand-new Session per call. While the unit's own read transaction was still open (PropertyPostgresRepository.get_many had checked out the connection), that second Session asked the pool for a second connection, found none, and timed out after 30s: QueuePool limit of size 1 overflow 0 reached, connection timed out, timeout 30.00 The baseline stage (PropertyBaselineOrchestrator.run -> uow.property.get_many -> landlord overrides) hit this on every invocation. Read the overrides on the unit's OWN session instead. property_overrides is committed reference data, so reading it inside the unit's transaction sees the same rows and keeps the invocation on one connection. Extract the query/mapping into a shared helper and add OpenSessionPropertyOverridesReader (reads on a caller-owned, already-open session without closing it) for the unit; the standalone PropertyOverridesPostgresReader still opens its own short session for use outside a unit. Regression test pins the invariant with a real pool_size=1/max_overflow=0 engine: without the fix it reproduces the exact QueuePool timeout. Co-Authored-By: Claude Opus 4.8 (1M context) --- repositories/postgres_unit_of_work.py | 13 ++-- .../property_overrides_postgres_reader.py | 61 +++++++++++++------ tests/repositories/test_unit_of_work.py | 58 +++++++++++++++++- 3 files changed, 108 insertions(+), 24 deletions(-) diff --git a/repositories/postgres_unit_of_work.py b/repositories/postgres_unit_of_work.py index 8a66146c..25764397 100644 --- a/repositories/postgres_unit_of_work.py +++ b/repositories/postgres_unit_of_work.py @@ -15,7 +15,7 @@ from repositories.product.composite_product_repository import ( catalogue_with_off_catalogue_overrides, ) from repositories.property.property_overrides_postgres_reader import ( - PropertyOverridesPostgresReader, + OpenSessionPropertyOverridesReader, ) from repositories.property.property_postgres_repository import ( PropertyPostgresRepository, @@ -46,10 +46,13 @@ class PostgresUnitOfWork(UnitOfWork): spatial_repo = SpatialPostgresRepository(self._session) # Fold Landlord Overrides onto the Effective EPC on every re-hydration # (ADR-0032), so what the Baseline orchestrator scores off ``uow.property`` - # matches what the Plan was modelled from. The reader is uow-independent — - # ``property_overrides`` is committed reference data — so it opens its own - # short read session per call via the same session factory. - overrides_reader = PropertyOverridesPostgresReader(self._session_factory) + # matches what the Plan was modelled from. ``property_overrides`` is + # committed reference data, but the reader must read on THIS uow's session + # — a second session opened concurrently checks out a second connection + # and deadlocks the single-connection pool while this uow's transaction is + # open. Reading committed data inside the uow's transaction is correct and + # keeps the invocation on one connection. + overrides_reader = OpenSessionPropertyOverridesReader(self._session) self.property = PropertyPostgresRepository( self._session, epc_repo, spatial_repo, overrides_reader ) diff --git a/repositories/property/property_overrides_postgres_reader.py b/repositories/property/property_overrides_postgres_reader.py index 1ccb13d5..c30993b6 100644 --- a/repositories/property/property_overrides_postgres_reader.py +++ b/repositories/property/property_overrides_postgres_reader.py @@ -2,9 +2,16 @@ Read-only and uow-independent: ``property_overrides`` is committed reference data the ``bulk_upload_finaliser`` Lambda writes at Finalise, long before First -Run executes — there is no transactional coupling to the ingestion run, so this -opens its own short read session per call via the injected session factory -(mirroring the composition root's ``lambda: Session(engine)``). +Run executes — there is no transactional coupling to the ingestion run, so the +standalone reader opens its own short read session per call via the injected +session factory (mirroring the composition root's ``lambda: Session(engine)``). + +Inside a Unit of Work the overrides must instead be read on the UoW's *own* +session (``OpenSessionPropertyOverridesReader``): a second session opened +concurrently checks out a second connection, which deadlocks the modelling_e2e +Lambda's single-connection pool while the UoW's read transaction is still open. +Reading committed reference data inside the UoW's transaction is correct — it +sees the same committed rows — and keeps the invocation on one connection. """ from __future__ import annotations @@ -21,25 +28,43 @@ from repositories.property.property_overrides_reader import ( ) +def _resolve_overrides(session: Session, property_id: int) -> ResolvedPropertyOverrides: + rows = session.exec( + select(PropertyOverrideRow).where( + col(PropertyOverrideRow.property_id) == property_id + ) + ).all() + return ResolvedPropertyOverrides( + rows=tuple( + ResolvedPropertyOverride( + override_component=row.override_component, + building_part=row.building_part, + override_value=row.override_value, + ) + for row in rows + ) + ) + + class PropertyOverridesPostgresReader(PropertyOverridesReader): + """Opens its own short read session per call — for standalone use outside a + Unit of Work, where there is no shared session/connection to reuse.""" + def __init__(self, session_factory: Callable[[], Session]) -> None: self._session_factory = session_factory def overrides_for(self, property_id: int) -> ResolvedPropertyOverrides: with self._session_factory() as session: - rows = session.exec( - select(PropertyOverrideRow).where( - col(PropertyOverrideRow.property_id) == property_id - ) - ).all() + return _resolve_overrides(session, property_id) - return ResolvedPropertyOverrides( - rows=tuple( - ResolvedPropertyOverride( - override_component=row.override_component, - building_part=row.building_part, - override_value=row.override_value, - ) - for row in rows - ) - ) + +class OpenSessionPropertyOverridesReader(PropertyOverridesReader): + """Reads on a caller-owned, already-open session without closing it — for use + inside a Unit of Work so resolving overrides reuses the UoW's single + connection instead of checking out a second one.""" + + def __init__(self, session: Session) -> None: + self._session = session + + def overrides_for(self, property_id: int) -> ResolvedPropertyOverrides: + return _resolve_overrides(self._session, property_id) diff --git a/tests/repositories/test_unit_of_work.py b/tests/repositories/test_unit_of_work.py index 3cc13d6c..43f6f14e 100644 --- a/tests/repositories/test_unit_of_work.py +++ b/tests/repositories/test_unit_of_work.py @@ -7,7 +7,7 @@ from typing import Any import pytest from sqlalchemy import Engine -from sqlmodel import Session +from sqlmodel import Session, create_engine from datatypes.epc.domain.epc import Epc from datatypes.epc.domain.epc_property_data import ( @@ -152,6 +152,62 @@ def test_unit_hydrates_a_property_with_its_landlord_overrides_folded( assert main.wall_insulation_type == 3 +def test_hydrating_a_property_with_overrides_stays_on_one_connection( + db_engine: Engine, +) -> None: + """Resolving Landlord Overrides during re-hydration must read on the unit's + OWN session, not open a second one. The modelling_e2e Lambda runs on a + single-connection pool (``pool_size=1, max_overflow=0``); a second session + opened while the unit's read transaction is still open checks out a second + connection and deadlocks ("QueuePool limit of size 1 overflow 0 reached, + connection timed out"). This pins that the unit holds exactly one connection + while it hydrates a Property that has overrides. + """ + # Arrange — seed a property + EPC + an override (committed reference data). + with PostgresUnitOfWork(_session_factory(db_engine)) as uow: + row = PropertyRow(portfolio_id=1, postcode="A0 0AA", address="1 St", uprn=1) + uow._session.add(row) # pyright: ignore[reportPrivateUsage] + uow._session.flush() # pyright: ignore[reportPrivateUsage] + property_id = row.id + assert property_id is not None + EpcPostgresRepository(uow._session).save( # pyright: ignore[reportPrivateUsage] + _epc(), property_id=property_id + ) + uow._session.add( # pyright: ignore[reportPrivateUsage] + PropertyOverrideRow( + property_id=property_id, + portfolio_id=1, + building_part=0, + override_component="wall_type", + override_value="Solid brick, with internal insulation", + original_spreadsheet_description="solid brick, insulated", + ) + ) + uow.commit() + + # A pool that admits exactly one connection and fails fast (not after 30s) if + # a second is requested — the production modelling_e2e shape. + single_connection = create_engine( + db_engine.url, pool_size=1, max_overflow=0, pool_timeout=2 + ) + try: + # Act — hydrate through the unit; this resolves the override. + with PostgresUnitOfWork(_session_factory(single_connection)) as uow: + prop = uow.property.get(property_id) + finally: + single_connection.dispose() + + # Assert — reached here without a QueuePool timeout, and the override folded: + # cavity (4) → solid brick (3) / internal (3). + main = next( + part + for part in prop.effective_epc.sap_building_parts + if part.identifier is BuildingPartIdentifier.MAIN + ) + assert main.wall_construction == 3 + assert main.wall_insulation_type == 3 + + def test_leaving_the_block_without_commit_persists_nothing(db_engine: Engine) -> None: # Arrange new_unit = lambda: PostgresUnitOfWork(_session_factory(db_engine))