Address review: stream workbook to /tmp, repo the scenario-name SQL

Dan's review on #1546:

- Stream the workbook to a /tmp temp file and S3Client.upload_file it, instead
  of render_workbook -> bytes -> put_object. render_workbook now saves straight
  to a path; the orchestrator renders to a temp file, multipart-uploads it, and
  always cleans it up. Restores ADR-0065's "never an in-memory BytesIO" decision
  (the OOM path at the 100k-row cap).

- Move the raw `SELECT ... FROM scenario` out of the Lambda handler into
  ScenarioNamesPostgresRepository, so the handler stays composition-only and all
  SQL lives in repositories/.

- current_sap_points goes through _float, matching its Optional[float] read-model
  type and the sibling numeric facts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

applications/ara_export/handler.py

@@ -19,8 +19,6 @@ import os
 from typing import Any
 
 import boto3
-from sqlalchemy import text
-from sqlmodel import Session
 
 from applications.ara_export.ara_export_trigger_body import AraExportTriggerBody
 from domain.tasks.tasks import Source
@@ -32,6 +30,9 @@ from orchestration.scenario_export_orchestrator import ScenarioExportOrchestrato
 from repositories.scenario_export.scenario_export_postgres_repository import (
     ScenarioExportPostgresRepository,
 )
+from repositories.scenario_export.scenario_names_postgres_repository import (
+    ScenarioNamesPostgresRepository,
+)
 from repositories.tasks.subtask_postgres_repository import SubTaskPostgresRepository
 from utilities.aws_lambda.task_handler import task_handler
 
@@ -42,27 +43,6 @@ logger = logging.getLogger(__name__)
 _URL_TTL_SECONDS = 3600
 
 
-class _ScenarioNames:
-    """Scenario id → display name, read from the ``scenario`` table (the domain
-    Scenario aggregate does not carry the name)."""
-
-    def __init__(self, session: Session, scenario_ids: list[int]) -> None:
-        rows = (
-            session.connection()
-            .execute(
-                text("SELECT id, name FROM scenario WHERE id = ANY(:ids)"),
-                {"ids": scenario_ids},
-            )
-            .all()
-        )
-        self._names: dict[int, str] = {
-            row[0]: (row[1] or f"scenario_{row[0]}") for row in rows
-        }
-
-    def name_for(self, scenario_id: int) -> str:
-        return self._names.get(scenario_id, f"scenario_{scenario_id}")
-
-
 def _email_sender() -> SesSmtpEmailSender:
     return SesSmtpEmailSender(
         host=os.environ["SES_SMTP_HOST"],
@@ -98,7 +78,7 @@ def handler(body: dict[str, Any], context: Any) -> dict[str, Any]:
         boto_s3: Any = boto3.client("s3")  # pyright: ignore[reportUnknownMemberType]
         orchestrator = ScenarioExportOrchestrator(
             rows=ScenarioExportPostgresRepository(session),
-            scenarios=_ScenarioNames(session, scenario_ids),
+            scenarios=ScenarioNamesPostgresRepository(session),
             exports=S3Client(boto_s3, os.environ["DOCUMENT_EXPORTS_BUCKET"]),
             email=_email_sender(),
             url_ttl_seconds=_URL_TTL_SECONDS,

infrastructure/xlsx/scenario_export_workbook.py

@@ -9,7 +9,6 @@ dynamic number of scenario sheets is handled natively.
 from __future__ import annotations
 
 import re
-from io import BytesIO
 from typing import Sequence
 
 from openpyxl import Workbook
@@ -36,10 +35,19 @@ def _safe_sheet_name(name: str, used: set[str]) -> str:
     return clean
 
 
-def render_workbook(sheets: Sequence[tuple[str, ExportSheet]]) -> bytes:
-    """Render the scenario sheets into a single branded ``.xlsx`` as bytes. Each
-    tuple is a Scenario name and its shaped sheet; sheet order follows the input
-    (the requested ``scenario_ids`` order)."""
+def render_workbook(
+    sheets: Sequence[tuple[str, ExportSheet]], destination_path: str
+) -> None:
+    """Render the scenario sheets into a single branded ``.xlsx`` at
+    ``destination_path``. Each tuple is a Scenario name and its shaped sheet;
+    sheet order follows the input (the requested ``scenario_ids`` order).
+
+    Saves straight to disk (``Workbook.save(path)``) so the caller can
+    ``S3Client.upload_file`` it with multipart — the workbook is never also held
+    as an in-memory ``bytes`` copy (ADR-0065; the OOM path the 4 GB sizing was
+    against at the 100k-row cap). The further memory win is openpyxl
+    ``write_only`` mode; that switch would live at this seam.
+    """
     workbook = Workbook()
     default = workbook.active
     if default is not None:
@@ -53,6 +61,4 @@ def render_workbook(sheets: Sequence[tuple[str, ExportSheet]]) -> bytes:
             worksheet.append([row.get(column, "") for column in sheet.columns])
         style_sheet(worksheet)
 
-    buffer = BytesIO()
-    workbook.save(buffer)
-    return buffer.getvalue()
+    workbook.save(destination_path)

orchestration/scenario_export_orchestrator.py

@@ -12,6 +12,8 @@ from __future__ import annotations
 
 import html
 import logging
+import os
+import tempfile
 from dataclasses import dataclass
 from typing import Protocol, Sequence
 
@@ -103,9 +105,20 @@ class ScenarioExportOrchestrator:
                 },
             )
 
-        data = render_workbook(named_sheets)
         key = f"{self._key_prefix}/{export_name}.xlsx"
-        self._exports.put_object(key, data)
+        # Render to a /tmp file and multipart-upload it, so the workbook is never
+        # also held in memory as a bytes copy (ADR-0065 — the OOM path at the
+        # 100k-row cap). The temp file is always cleaned up.
+        handle, workbook_path = tempfile.mkstemp(suffix=".xlsx")
+        os.close(handle)
+        try:
+            render_workbook(named_sheets, workbook_path)
+            self._exports.upload_file(workbook_path, key)
+        finally:
+            try:
+                os.remove(workbook_path)
+            except OSError:
+                pass
         url = self._exports.generate_presigned_url(key, self._url_ttl_seconds)
 
         subject, body, html_body = self._compose_email(

repositories/scenario_export/scenario_export_postgres_repository.py

@@ -345,7 +345,9 @@ class ScenarioExportPostgresRepository(ScenarioExportRepository):
         for row in connection.execute(_EPC_PERF_SQL, source_params).all():
             facts.setdefault(row[0], {}).update(
                 {
-                    "current_sap_points": row[1],
+                    # _float to match the read-model's Optional[float] typing and
+                    # the sibling numeric facts (the driver hands back an int).
+                    "current_sap_points": _float(row[1]),
                     "current_epc_rating": _str(row[2]),
                 }
             )

repositories/scenario_export/scenario_names_postgres_repository.py

@@ -0,0 +1,31 @@
+from __future__ import annotations
+
+from sqlalchemy import text
+from sqlmodel import Session
+
+# Scenario id → display name. The domain Scenario aggregate does not carry the
+# name, so it is read from the ``scenario`` table; kept in the repository layer
+# with the export's other reads (ADR-0065), so the Lambda handler stays
+# composition-only.
+_SCENARIO_NAME_SQL = text("SELECT name FROM scenario WHERE id = :scenario_id")
+
+
+class ScenarioNamesPostgresRepository:
+    """Reads a Scenario's display name from Postgres, satisfying the
+    orchestrator's ``ScenarioNames`` port. Looked up lazily and cached (an export
+    names only its handful of requested Scenarios)."""
+
+    def __init__(self, session: Session) -> None:
+        self._session = session
+        self._cache: dict[int, str] = {}
+
+    def name_for(self, scenario_id: int) -> str:
+        if scenario_id not in self._cache:
+            row = (
+                self._session.connection()
+                .execute(_SCENARIO_NAME_SQL, {"scenario_id": scenario_id})
+                .first()
+            )
+            name = row[0] if row is not None else None
+            self._cache[scenario_id] = name or f"scenario_{scenario_id}"
+        return self._cache[scenario_id]

tests/infrastructure/xlsx/test_scenario_export_workbook.py

@@ -3,7 +3,7 @@ sheet per Scenario, headers then rows, verified by re-opening the bytes."""
 
 from __future__ import annotations
 
-from io import BytesIO
+from pathlib import Path
 
 from openpyxl import load_workbook
 
@@ -12,7 +12,9 @@ from infrastructure.xlsx.branding import HEADER_FILL
 from infrastructure.xlsx.scenario_export_workbook import render_workbook
 
 
-def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows() -> None:
+def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows(
+    tmp_path: Path,
+) -> None:
     # arrange — one scenario's shaped sheet.
     sheet = ExportSheet(
         columns=("property_id", "loft_insulation"),
@@ -20,18 +22,19 @@ def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows() -> None
     )
 
     # act
-    data = render_workbook([("Fabric first", sheet)])
+    path = str(tmp_path / "export.xlsx")
+    render_workbook([("Fabric first", sheet)], path)
 
     # assert — a real workbook with a sheet named for the scenario, the column
     # contract as the header row, and the data beneath it.
-    workbook = load_workbook(BytesIO(data))
+    workbook = load_workbook(path)
     assert workbook.sheetnames == ["Fabric first"]
     worksheet = workbook["Fabric first"]
     assert [cell.value for cell in worksheet[1]] == ["property_id", "loft_insulation"]
     assert [cell.value for cell in worksheet[2]] == [1, 1200.0]
 
 
-def test_sanitises_and_deduplicates_scenario_sheet_names() -> None:
+def test_sanitises_and_deduplicates_scenario_sheet_names(tmp_path: Path) -> None:
     # arrange — a name over Excel's 31-char limit (used twice, so it must be
     # deduplicated) and one with characters Excel forbids in a sheet title.
     empty = ExportSheet(columns=("property_id",), rows=())
@@ -39,19 +42,20 @@ def test_sanitises_and_deduplicates_scenario_sheet_names() -> None:
     illegal_name = "Bad:/\\?*[]name"
 
     # act
-    data = render_workbook(
-        [(long_name, empty), (long_name, empty), (illegal_name, empty)]
+    path = str(tmp_path / "export.xlsx")
+    render_workbook(
+        [(long_name, empty), (long_name, empty), (illegal_name, empty)], path
     )
 
     # assert — three distinct, Excel-legal sheet names.
-    names = load_workbook(BytesIO(data)).sheetnames
+    names = load_workbook(path).sheetnames
     assert len(names) == 3
     assert len(set(names)) == 3
     assert all(len(name) <= 31 for name in names)
     assert not any(ch in name for name in names for ch in r":/\?*[]")
 
 
-def test_brands_the_header_row_and_freezes_it() -> None:
+def test_brands_the_header_row_and_freezes_it(tmp_path: Path) -> None:
     # arrange
     sheet = ExportSheet(
         columns=("property_id", "loft_insulation"),
@@ -59,11 +63,12 @@ def test_brands_the_header_row_and_freezes_it() -> None:
     )
 
     # act
-    data = render_workbook([("Fabric first", sheet)])
+    path = str(tmp_path / "export.xlsx")
+    render_workbook([("Fabric first", sheet)], path)
 
     # assert — the header is a bold, brand-filled band and the header row is
     # frozen so it stays visible while scrolling.
-    worksheet = load_workbook(BytesIO(data))["Fabric first"]
+    worksheet = load_workbook(path)["Fabric first"]
     header_cell = worksheet["A1"]
     assert header_cell.font.bold is True
     assert header_cell.fill.fgColor.rgb == HEADER_FILL