The invocation is architecturally one DB connection at a time (read up front,
sequential write Units of Work, overrides resolved on the unit's own session).
Keep that as the design intent, but back it with NullPool instead of a fixed
pool_size=1 pool: each checkout opens a fresh connection and closes it on return,
so there is no pool slot to exhaust.
The difference is the failure mode if a path ever regresses and holds two
Sessions at once. A pool_size=1/max_overflow=0 pool turns that into a hard
30s dead-lock that fails the whole invocation ("QueuePool limit of size 1
overflow 0 reached, connection timed out"). NullPool instead opens a transient
second connection for that instant and the Lambda keeps running. The design
target stays one connection; NullPool just keeps it alive if we slip.
The single-connection invariant itself is still enforced in the Unit of Work
(overrides read on the unit's own session) and pinned by the regression test,
which uses its own strict pool_size=1 engine so it asserts the architecture
regardless of the production NullPool choice.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
The modelling_e2e Lambda held up to ~4 concurrent Postgres connections per
invocation: the read Session stayed open across the write loop (the catalogue
was queried live and overrides were read per-Property), each per-Property Unit
of Work opened a second, and the TaskOrchestrator ran on its own NullPool
engine — so the pool needed pool_size=2 + max_overflow=1 just for the modelling
work. Under 32 concurrent containers that approached RDS max_connections.
Restructure the handler to read everything up front — overrides, Scenario, an
in-memory catalogue snapshot, and stored Solar — through one short-lived read
Session, close it, then write each Property in a sequential Unit of Work. The
read and write Sessions no longer overlap, so the engine drops to pool_size=1,
max_overflow=0. Fold the orchestrator onto the same pooled engine: its repos
commit on every save, releasing the connection between bookkeeping calls, so it
holds none during the work. One invocation now uses one connection at a time.
The catalogue becomes a per-invocation snapshot (MaterialSnapshotRepository),
mirroring ProductPostgresRepository.get exactly — same drift mapping, lowest-id
pick, and errors — but priced after the Session closes. Transaction isolation
is preserved: per-Property writes and orchestrator bookkeeping keep their own
independent transactions, just drawn sequentially from a single connection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ports the §6.1 conservatory split (declare the four glazed fields; exclude the
glazed BP from the fabric loop; carry it as sap_conservatory) to the remaining
gov mappers that lacked it. Fixes the modelling_e2e construction_age_band
NOT-NULL failures: 5 type-4 conservatory certs on RdSAP-Schema-20.0.0 (e.g.
property 725213) mapped the conservatory to a null-age fabric part that crashed
on persist. 17.0/17.1/18.0 ported preventively (no-op for non-conservatory
certs). 21.0.0 deferred — it never adopted the ADR-0027 all-Optional
SapBuildingPart, so a sparse conservatory part needs that broader change first.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per review (Khalim): lift the full-SAP detection out of the SAP-Schema-16.x
branch into a single top-level gate in from_api_response, and key it on the
gov-API's own assessment_type declaration rather than the structural
sap_opening_types proxy.
- _is_full_sap_assessment(data): assessment_type == "SAP" — the authoritative
SAP-vs-RdSAP classification. Verified to separate the entire fixture corpus:
every full-SAP schema (SAP-Schema-17.x/18.x + the broken LIG 16.x) is "SAP";
every reduced cert (RdSAP-Schema-* and reduced SAP-Schema-16.x, incl.
sap_16_0.json) is "RdSAP". data_type / sap_opening_types agree but are derived
shape artifacts; assessment_type is the meaning.
- A cert that is full-SAP by assessment_type but whose schema_type LABEL is not a
recognised full-SAP schema is a *broken schema type* (label disagrees with the
assessment). _record_broken_schema_type logs it — visible, not silently
rerouted — so unreliable labels surface and coverage grows as new mislabels
appear. Generalises beyond 16.x to any future mislabel.
- _from_full_sap maps it via the full-SAP 17.1 mapper (real measured openings,
no defaulting; only `tenure` defaulted). Correctly-labelled full-SAP certs keep
their dedicated branches (one-mapper-per-schema convention); reduced certs are
unchanged.
Tests: broken cert routed AND recorded; correctly-labelled full-SAP not recorded.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A full-SAP cert (assessment_type=SAP, the as-designed LIG-* new-builds) lodged
under a SAP-Schema-16.x version failed to map with:
ValueError: RdSapSchema17_1: missing required field 'door_count'
— crashing the property's modelling_e2e subtask. 89 distinct properties in the
2026-06-24 run hit this; ~22 of every 30 sampled are this full-SAP shape.
Root cause: from_api_response dispatches on the schema_type STRING only, and the
SAP-Schema-16.x branch assumed a single shape — "reduced-field (RdSAP-shaped)" —
sending every 16.x cert through _normalize_sap_schema_16_x -> RdSapSchema17_1.
But the SAP-Schema-16.x name covers two structurally different certs:
* reduced RdSAP (assessment_type=RdSAP): top-level door_count / glazed_area
band / construction-code building parts.
* full SAP (assessment_type=SAP): measured shape — sap_opening_types +
structured sap_building_parts carrying measured U-values and door/window
OPENINGS, with NO top-level door_count (the door is an opening). These omit
the reduced-only count fields, so the reduced normaliser failed loud on the
first one it checks (door_count) — and 14 others behind it.
Force-fitting a full-SAP cert to the reduced RdSAP schema was the bug: the data
was never missing, it was being validated against the wrong schema.
Fix: discriminate on shape (_is_full_sap_cert: assessment_type=SAP AND
sap_opening_types present) and route full-SAP 16.x certs to the existing
full-SAP 17.1 mapper, which reads the real measured openings (the lodged
0.9x2.1m door -> door_count 1) — no reduced-field defaulting. The only field the
16.x full-SAP shape omits that SapSchema17_1 needs is `tenure` (register
metadata, no SAP effect), defaulted. Reduced 16.x certs (assessment_type=RdSAP,
no opening types) are untouched — all pinned reduced 16.x fixtures stay on the
RdSAP path.
Regression test pins a real full-SAP-16.0 cert (0293-...-8795, lodged 83) mapping
via the full-SAP path + the reduced 16.2 cert staying on the RdSAP path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_predict_epc returned None for three unrelated causes — unresolved
property_type, an empty same-type cohort, and a degenerate (no MAIN part)
prediction — which the handler collapsed into one generic "not predictable"
string. The SubTask output could not say which cause fired or which data to
fix.
Raise a specific PropertyNotModellableError subclass per cause, each carrying
the property's identity (property_id, uprn, postcode, portfolio_id) and
cause-specific context. The unresolved-property-type message points at the
likely missing/contradictory Landlord Override. All subclass ValueError, so the
per-property failure boundary keeps catching them and records str(exc).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CONTEXT.md gains Non-Separated / Separated Conservatory glossary terms; ADR-0036
records the mapper-owned §6.1 split + 1:1 persistence + round-trip contract; the
migration note lists the five new epc_property columns for the FE schema repo.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fail if any EpcPropertyData field is neither reconstructed by _compose nor on a
documented allow-list, turning latent persistence gaps into explicit decisions
(would have caught the conservatory and roof-window drops). ADR-0036.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Persist SapConservatory as five nullable conservatory_* columns on epc_property
(1:1 with the dwelling) and rebuild it in _compose, so the §6.1 fold survives
save -> reload -> score. Without this the scored (re-hydrated) EPC silently
dropped the conservatory (persist != score) — a latent gap shared with the
21.0.1 path. Adds a deep-equality round-trip test. ADR-0036.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the merged 21.0.1 fix (d501535c): declare the four glazed conservatory
fields on the 19.0 SapBuildingPart so from_dict stops dropping them, exclude the
glazed BP from the fabric loop, and carry it as EpcPropertyData.sap_conservatory
(§6.1). Fixes cert 718138 (conservatory_type=4) mis-scoring as a fabric part and
failing to persist on the NOT-NULL construction_age_band.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A predicted EPC is seeded by deep-copying one representative neighbour's
structure. _template chose the member whose floor area was closest to the
cohort median, ignoring building-part labels. When that member's only part
was lodged with a null identifier (mapped to OTHER), the prediction had no
MAIN part and the modelling_e2e handler rejected it as "not predictable" —
discarding an otherwise-rich same-type cohort.
Restrict the template to MAIN-bearing members (median still over the whole
cohort); fall back to closest-on-size only when none are MAIN-bearing, so an
all-unlabelled cohort is left for the handler's MAIN-part guard to reject
rather than silently relabelling real data.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Note that code 39 "any tariff" currently collapses to the standard electricity
rate; future work should resolve it to the dwelling's actual tariff (off-peak vs
standard) so off-peak electric heating prices correctly. Comment-only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
modelling_e2e properties with main fuel 39 failed at the price boundary
(UnpricedFuelCode since #44fff767; previously mis-rated as non-electric →
the ~14-SAP over-rating flagged in earlier review).
Code 39 is SAP Table 12 "electricity, any tariff" (epc_codes.csv main_fuel 39 =
"electricity, unspecified tariff"; spec footnote (j): defines an electric system,
cost/CO2/PE = standard electricity). It was absent from API_FUEL_TO_TABLE_32, so
to_table_32_code(39) was None → is_electric_fuel_code(39) False and pricing
raised.
Fix: map API_FUEL_TO_TABLE_32[39] = 30 (standard electricity) — the canonical
place Khalim's fuel work added codes. One line makes classification, pricing,
CO2/PE and the billing carrier all agree (39 → 30 → ELECTRICITY).
Tests: to_table_32_code(39)==30, is_electric_fuel_code(39) True, price == standard
electricity, and the billing carrier resolves to ELECTRICITY. 0 corpus impact
(no lodged corpus cert uses 39); accuracy + mapper-corpus gates green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
