mirror of
https://github.com/Hestia-Homes/Model.git
synced 2026-07-22 08:48:38 +00:00
A live test returned 'no documents could be packaged' for 91 correctly-resolved properties. Root cause: no upload source populates uploaded_files.landlord_property_id (pashub/magic-plan/audit set hubspot_deal_id; ECMK sets hubspot_listing_id), so matching on landlord_property_id found zero rows. Match fix: the worker now joins uploaded_files -> hubspot_deal_data (on deal_id) -> landlord_property_id, taking the property identity from the bridge. The repository returns a small PropertyDocument read-model instead of the infra ORM row, so the orchestrator no longer names infrastructure.postgres.* (resolves the leak dancafc flagged) and the s3_upload_timestamp cast is gone. Coverage is limited to deal-id-linked sources; listing_id/uprn-only files are a noted follow-up. Observability: the empty-selection failure now carries stage counts (selected/matched/planned/skipped) in both the message (-> the worker WARNING log) and details (-> sub_task.outputs), and the run logs those counts. The next failure says 'matched 0 documents' instead of failing opaquely. ADR-0060 + CONTEXT.md updated (matching decision + considered options). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
133 lines
8.2 KiB
Markdown
133 lines
8.2 KiB
Markdown
---
|
|
status: accepted (builds on ADR-0055, ADR-0059)
|
|
---
|
|
|
|
# Bulk Document Download builds one capped, best-effort Download Package per request
|
|
|
|
Users need to pull the documents held in `uploaded_files` for many properties
|
|
at once — per property, the latest file of each **Document Type** — as a single
|
|
archive, without clicking through the UI file by file. The request is initiated
|
|
in the front end, can span a hand-picked set of properties or one or more whole
|
|
HubSpot projects (by project code), and finishes with a link emailed to the
|
|
requester (ADR-0059).
|
|
|
|
`uploaded_files` links to a property by `uprn` **or** `landlord_property_id`
|
|
(both nullable) and has **no `property_id`** column; `file_type` (the Document
|
|
Type) is itself nullable; and files live across arbitrary buckets
|
|
(`s3_file_bucket` per row).
|
|
|
|
## Decision
|
|
|
|
**A request produces exactly one Download Package: a ZIP of one folder per
|
|
property, each holding the latest file of each Document Type — built
|
|
best-effort, size-capped at the trigger, on the app-owned-task + attach-mode
|
|
lane (ADR-0055).**
|
|
|
|
- **Trigger & lifecycle (ADR-0055).** The **front end** creates the app-owned
|
|
`tasks` row and writes the **selection config** —
|
|
`{project_codes?: str[], landlord_property_ids?: str[], portfolio_id?: int}` —
|
|
into `tasks.inputs` (TEXT/JSON, FE-owned), then calls the FastAPI route with
|
|
**only the `task_id`**. A large selection therefore never travels in an HTTP
|
|
body. The route **reads the selection from `tasks.inputs`** and resolves it to
|
|
the distinct `landlord_property_id` set — the **union** of every property in
|
|
the named HubSpot `project_codes` (read from `hubspot_deal_data`, where the
|
|
project↔property grain lives) and the hand-picked `landlord_property_ids`.
|
|
`landlord_property_id` is the selection key throughout because
|
|
`uploaded_files` is matched on it (it has no `property_id`). At least one of
|
|
the two must be provided; `portfolio_id` is optional and used only to name the
|
|
package. The route then caps the set, resolves the recipient email from the
|
|
authenticated user (ADR-0059), **pins the resolved recipe**
|
|
(`landlord_property_ids`, `recipient_email`, `package_name`) onto **one
|
|
pre-created `sub_task`'s `inputs`**, and drops one SQS message (`task_id`,
|
|
`sub_task_id`). The new `applications/bulk_document_download` Lambda runs in
|
|
**attach mode**, reading that recipe from the sub_task; `TaskOrchestrator`
|
|
owns status + roll-up. (`task.inputs` = the FE's selection; `sub_task.inputs`
|
|
= the backend's resolved recipe.)
|
|
- **One package = one sub_task, streamed.** No fan-out. A Download Package can
|
|
be **several GB**, so it is never held whole in memory: each document is read
|
|
from its own `s3_file_bucket`, written into an on-disk ZIP in `/tmp`, and
|
|
released; the finished archive is **multipart-uploaded from disk**
|
|
(`S3Client.upload_file`). The Lambda's ephemeral storage (`/tmp`) is raised
|
|
accordingly (up to 10 GB) — see Consequences. One artifact, one URL, one
|
|
email.
|
|
- **Selection cap at the route.** Reject `> N` properties synchronously with a
|
|
legible "narrow your selection" error (property count, not a document/byte
|
|
query — cheap, no S3 on the hot path). A `/tmp` size budget inside the Lambda
|
|
is a backstop that fails the sub_task with a clear reason if a pathological
|
|
selection still overflows. `N` starts at a conservative, tunable value.
|
|
- **Matching via `hubspot_deal_id`, bridged through `hubspot_deal_data`.** The
|
|
selection is a set of `landlord_property_id`s, but `uploaded_files` is matched
|
|
on **`hubspot_deal_id`**: in practice no upload source populates
|
|
`uploaded_files.landlord_property_id` (pashub, magic plan and the audit
|
|
generator set `hubspot_deal_id`; ECMK sets `hubspot_listing_id`), so the
|
|
worker joins `uploaded_files → hubspot_deal_data (on deal_id) → landlord_property_id`
|
|
and takes the property identity from the bridge, not the file row. **Coverage
|
|
limitation:** files linked *only* by `hubspot_listing_id` (ECMK) or `uprn` are
|
|
not reached; extending the bridge to those keys is a follow-up. Property
|
|
display info (the folder name) is enriched from the property record
|
|
(`repositories/property/`).
|
|
- **Layout.** One folder per property named by human-readable **address**
|
|
(hubspot-enriched), with `landlord_property_id` appended for uniqueness;
|
|
inside, one file per Document Type = the newest by `s3_upload_timestamp`.
|
|
Rows with a **null Document Type are skipped** and listed in the run output.
|
|
- **Best-effort failure contract.** Build from whatever resolves. Skipped
|
|
properties (no documents) and skipped null-type files are recorded in
|
|
`sub_task.outputs`. The run **fails only** on an infrastructure error
|
|
(S3/DB/zip/upload/email) or when the whole selection yields **zero
|
|
documents**. The email carries an "N properties, M documents, X skipped"
|
|
summary.
|
|
- **Delivery — both channels.** The presigned URL (60-minute expiry) and the
|
|
skip-summary are written to `sub_task.outputs` (the FE already polls task
|
|
status and can show the link) **and** emailed (ADR-0059).
|
|
- **A dedicated exports bucket.** Packages are written to a **separate
|
|
`DOCUMENT_EXPORTS_BUCKET`**, not `DATA_BUCKET` — the generated archives are a
|
|
distinct class of artifact (transient, user-facing, wide-read source but
|
|
single-writer) and keeping them out of the shared data bucket keeps IAM and
|
|
any future retention policy cleanly scoped to exports. No lifecycle rule is
|
|
imposed on `DATA_BUCKET`. Retention on the exports bucket is an open decision
|
|
(the packages are transient, so an expiry is likely warranted, but it is left
|
|
to a follow-up rather than baked in here).
|
|
|
|
## Considered options
|
|
|
|
- **Fan out into per-batch sub_tasks / multiple ZIPs** (like the modelling
|
|
run). Rejected: the user would receive N links / N emails and "the download"
|
|
would stop being one file; the capped-single-package model keeps the artifact
|
|
and the notification singular.
|
|
- **Cap on resolved document count / bytes.** Rejected for v1: it adds
|
|
synchronous DB + S3 HEAD work to the trigger and couples the route to the
|
|
packaging logic; a property-count cap is cheap and predictable.
|
|
- **Strict "all-or-nothing" packaging.** Rejected: one unreadable row or a
|
|
property with no documents would deny the entire package; best-effort +
|
|
reporting matches "include documents and properties where they exist".
|
|
- **Match on `landlord_property_id` directly.** Rejected once tested against
|
|
real data: `uploaded_files.landlord_property_id` is unpopulated, so a direct
|
|
match returns nothing. `hubspot_deal_id` (bridged via `hubspot_deal_data`) is
|
|
the key that upload sources actually set.
|
|
- **Match on `uprn` / `hubspot_listing_id` as well.** Deferred: `hubspot_deal_id`
|
|
covers the bulk of sources; add `uprn`/`listing_id` to the bridge if a source
|
|
keyed only by those needs including.
|
|
|
|
## Consequences
|
|
|
|
- New DDD pieces: `applications/bulk_document_download/` (thin handler +
|
|
trigger body), `orchestration/bulk_document_download_orchestrator.py`,
|
|
packaging rules in `domain/`, a `hubspot_deal_id`-bridged query on the
|
|
uploaded-file repository that returns a small `PropertyDocument` read-model
|
|
(so the orchestrator never names `infrastructure.postgres.*`), a
|
|
`generate_presigned_url` + multipart upload on `S3Client`, and the email
|
|
port/adapter from ADR-0059. The only `backend/` touch is the trigger route.
|
|
- The stored, pinned `property_id` set makes a run reproducible even if the
|
|
portfolio changes between trigger and execution.
|
|
- `N`, the Lambda's ephemeral-storage (`/tmp`) size, and its memory are tunable
|
|
knobs, not load-bearing invariants. `/tmp` must be raised beyond the 512 MB
|
|
default (up to 10 GB) to hold a multi-GB archive — a new terraform knob on the
|
|
shared Lambda modules, backward-compatible (every other Lambda keeps 512 MB).
|
|
- A **new S3 bucket** (`DOCUMENT_EXPORTS_BUCKET`) is provisioned for the
|
|
packages, with its own IAM (single-writer, presign-read). Its retention policy
|
|
is a deliberate follow-up, not set here.
|
|
- Because matching bridges through `hubspot_deal_data`, a file is only reachable
|
|
once its deal has a `hubspot_deal_data` row (and the file carries that
|
|
`hubspot_deal_id`). Populating `uploaded_files.landlord_property_id` at upload
|
|
time, or closing the `uploaded_files.property_id` gap, would let matching drop
|
|
the bridge without changing the package model.
|