diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd103d65..01ddb556 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -621,7 +621,7 @@ jobs: # the ADR 0013/0016 RTE capture/re-ingress case); those suites are MEFOR_TEST_SQLSERVER-gated # and run ONLY on the server-DB legs, so a file not matched here edits with NO real SS/PG # coverage (it merely skips the gated leg). Keep this in sync with those steps - # (docs/throughput-build-plan.md). + # (docs/archive/throughput/throughput-build-plan.md). # The transports/ + config/wiring arms are the CAPTURE SURFACE: capture_response / reingress_to # are declared there (ADR 0013), and SQL Server / Postgres both declare supports_response_capture # + supports_pt_reingress True — so a regression in that surface is a SERVER-DB regression and must @@ -871,7 +871,14 @@ jobs: # reliability-core invariant tests — crash-replay, poison-bound, and the per-lane-FIFO locked-head- # BLOCKS (#285) gate — that are MEFOR_TEST_SQLSERVER-gated but live in their OWN files, so the named # suites above never picked them up and they ran only on SQLite. Run them here against the real - # backend. APPEND each new lever's test file per docs/throughput-build-plan.md. + # backend. APPEND each new lever's test file per docs/archive/throughput/throughput-build-plan.md. + # + # NOT only throughput levers: this step is the catch-all for ANY MEFOR_TEST_SQLSERVER-gated file + # the named suites above do not collect. CI runs an explicit file LIST here, not the whole suite, + # so a new gated file that nobody appends never runs in CI at all — it just reports as skipped on + # the plain legs, which reads identical to passing. `tests/test_sqlserver_legacy_outbox_migration.py` + # (ASVS 14.2.7) is here for exactly that reason: it verifies a one-way schema migration that can + # only be observed against a real server. # # RETRY WRAPPER: pyodbc 5.3.0 intermittently segfaults in its C parameter-binding path under # py3.14 against the SQL Server 2025 container (upstream mkleehammer/pyodbc#1459, unfixed; 5.3.0 @@ -893,6 +900,7 @@ jobs: tests/test_per_lane_wake.py tests/test_stage_dispatcher.py tests/test_pooled_rider.py + tests/test_sqlserver_legacy_outbox_migration.py - name: Run the RTE capture / re-ingress loop on real SQL Server (ADR 0013/0016) env: @@ -1067,7 +1075,7 @@ jobs: MEFOR_ALLOW_INSECURE_TLS: "1" # The throughput levers' MEFOR_TEST_POSTGRES-gated invariants (esp. the FOR-UPDATE no-SKIP-LOCKED # locked-head-BLOCKS twin of #285, and the batch FIFO/crash tests) live in their own files. APPEND - # each new lever's test file per docs/throughput-build-plan.md. + # each new lever's test file per docs/archive/throughput/throughput-build-plan.md. run: >- pytest -v tests/test_inline_fast_path.py diff --git a/docs/PHI.md b/docs/PHI.md index f8691e76..92eb910b 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -86,56 +86,88 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h | **PL-4 · Operational metadata (non-PHI)** | Ids, hashes, counts, config labels — deliberately **not** ciphered. | | **PL-5 · Engine-unreachable substrate** | Journals, logs, version stores, indexes. The app-level AEAD **cannot** reach these; whole-DB / volume encryption is the only cover. | -| Location | Backends | Holds PHI? | Encrypted at rest? (cipher · cell-AAD · key path) | Protection level | Notes | -|---|---|---|---|---|---| -| `messages.raw` | all three | **Yes** — full inbound body | **Yes, when a key is set** — store cipher; AAD `("messages","raw",id)`; store DEK | **PL-1** | Preserved verbatim by design (operators must see what arrived) | -| `queue.payload` (stage=`ingress`/`routed`) | all three | **Yes** — the raw body, **transient** | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | A second copy of the raw, held only across the route→transform window: the `ingress` row is consumed at `route_handoff`, each `routed` row at `transform_handoff` (deleted, never kept). A stalled router/transform stage can hold several briefly — surfaced by the `queue_buildup` alert | -| `queue.payload` (stage=`outbound`) | all three | **Yes** — transformed outbound body | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | One row per destination; the persistent footprint | -| `shared_body.body` (store-once-deliver-many) | schema on all three; **rows written on SQLite only** | **Yes** — one transformed outbound body shared by N destinations | **Yes, when a key is set** — store cipher; AAD `("shared_body","body",hash)`; store DEK | **PL-1** | `hash` is the SHA-256 of the **plaintext** body (the content address). Refcounted: GC'd the moment the last referencing outbound row's body is purged. On SQL Server and Postgres the table is schema-parity only — `queue.body_ref` stays `NULL`, so no row is ever written. **Both** server backends nonetheless keep a read-side `LEFT JOIN shared_body` deref in `resend_to` (with its own `cell_aad("shared_body","body",…)` decrypt branch), which is why the column stays **cipher-covered** on all three. It is **rotation-swept on SQLite only** — `shared_body` has a pass in `MessageStore.reencrypt_to_active` but appears in neither server backend's rotation. Harmless while `body_ref` stays `NULL` there, and recorded as a deliberate decision in `store/postgres.py`'s `_CIPHER_COLUMNS` note ("schema-only this increment … it needs no rotation pass here until the dedup insert is wired") | -| `attachment_chunk.ciphertext` (ADR 0105 / #149) | all three | **Yes** — one slice of a detached very-large document (e.g. a base64 PDF from OBX-5.5) | **Yes, when a key is set** — store cipher, **sealed per chunk on write**; AAD `("attachment_chunk","ciphertext",attachment_id,seq)`; store DEK | **PL-1** | The document is detached at ingress, content-addressed (`sha256` of the verbatim plaintext) and chunked; the message keeps only an `mfdoc:v1:ref:` handle. Read back via `GET /messages/{id}/attachments/{id}` (§3) | -| `attachment` header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + `message_attachment` linkage | all three | **No** — size/type/linkage only | No (metadata, deliberately not ciphered) | **PL-4** | The linkage row is the security crux of the download route: it scopes a content address to a message the caller may already read | -| `response.body` (ADR 0013 captured replies; ADR 0021 `kind='ack_sent'`) | all three | **Yes** — the partner's reply body, or the ACK/NAK the engine returned | **Yes, when a key is set** — store cipher; AAD `("response","body",message_id,destination_name,response_seq)`; store DEK | **PL-1** | Composite PK, so it rides its own migration/rotation pass. An **ACK body is stored only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext (fail-safe), and a NAK never stores a body at all | -| `outbox.payload` (**legacy**, pre-staged-pipeline schema) | **SQL Server only** | **Yes** — a full outbound body on a store upgraded from the flat-`outbox` schema | **Yes, when a key is set** — store cipher (on-open migration only); AAD `("outbox","payload",id)`; store DEK | **PL-1** | SQL Server's schema pass re-runs a **create-if-absent** `IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox` on every open, so the table always exists and an existing one is never re-created or emptied — and it is never migrated away or dropped (SQLite migrates its rows into `queue` then `DROP`s it; Postgres has no such table). Empty on a greenfield install. **Three honest gaps:** it is absent from `reencrypt_to_active`, so a key rotation followed by dropping the retired key leaves those bodies undecryptable; `outbox.last_error` is in neither the migration nor the rotation pass; and **no purge path on any backend touches it** ([§8](#8-retention--purge)) | -| `[store].uploads_dir/*.blob` + `*.meta` — the cipher cells `uploaded_file.body` / `uploaded_file.meta` (offline uploaded logs, ADR 0134) | all (filesystem, not the DB) | **Yes** — an operator-uploaded diagnostic message file, held for offline browsing decoupled from any connection | **Yes, when a key is set** — the **same store cipher** (`build_store_cipher`); AAD `("uploaded_file","body")` / `("uploaded_file","meta")` + `file_id`; store DEK. **identity/plaintext-on-disk otherwise** (the File-connector-spill tier below) | **PL-1** | A PHI-at-rest location **outside** the message store, opt-in (unset ⇒ the subsystem is disabled — no surface). On-disk identity is a random 32-hex `file_id` (path-traversal guard); the operator filename is display-only. Every access is `files:*`-gated, browse is step-up + PHI-hop-guarded, all audited (metadata only). **Not** re-encrypted by `rotate-key` (outside the store) — stays readable via the decrypt keyring. The dir is created `0o700` and the sidecar written `0o600` **best-effort, and both are no-ops on Windows** — the engine applies no ACL here (it does not call the `icacls` enforcer). **Retention + quotas (ASVS 5.2.4):** uploaded files auto-prune after `[store].uploads_retention_days` (default **30**) — swept opportunistically at save time and by a periodic task; every prune is audited (`upload.prune`, file_id + uploader only, never content). Per-uploader caps `[store].max_upload_files_per_user` (default **100**) / `[store].max_upload_total_bytes_per_user` (default **250 MiB**) bound the at-rest volume; a would-be over-quota upload is refused **HTTP 409** with an `upload.reject_quota` audit before anything is written (defaults-ON, `ge=1` floors; quota is per-process per-`uploads_dir`). Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | -| `[backup].destination/mefor-backup-*.mfbak` (ADR 0049 DR backup) | **SQLite only** carries bodies | **SQLite: Yes** — a consistent store snapshot (full inbound + outbound bodies) + the config bundle. **SQL Server / Postgres: No** — config bundle only | **Yes** — `.mfbak` chunked-AEAD codec under the **store DEK** (`resolve_active_key`); an identity-cipher (no-key) box is **refused** unless `[backup].allow_unencrypted` writes a `.mfbak.plain` | **PL-1** (SQLite) / **PL-4** (server backends) | On a **server-DB store `snapshot_to` raises `DbaDelegatedError`**, so the BackupRunner writes a **config-only** archive — or skips entirely when `[backup].config_only_on_server_db = false`. There is therefore **no `.mfbak` containing message bodies on SQL Server or Postgres**; the DB-tier backup there is `BACKUP DATABASE` / Always On / `pg_dump` / PITR, infra-owned. Where bodies *are* present it is a second at-rest PHI copy, bounded by keep-N retention; like `uploads_dir` it is **not** re-encrypted by `rotate-key`. The share's own ACLs are infra-owned | -| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir, ADR 0049) | SQLite carries bodies; server backends config-only | **Yes** — a full store snapshot, and on verify a **decrypted** archive | **No** — the snapshot keeps the store's own column cipher, but the staging tar and the verify extraction are **plaintext on disk**; no engine ACL (`_secure_file` is never called on these paths) | **PL-1** | `run_backup` snapshots the store to `/store.db` and tars it **plaintext** before sealing it into the `.mfbak` (`pipeline/dr_backup.py`), and `[backup].verify_after_backup` (**default `true`**) decrypts the archive straight back out to a second temp dir on **every** run — independent of `full_restore_verify`. Transient (the `TemporaryDirectory` unlinks on exit) but **not** on a crash or `SIGKILL`. Lives under `%TEMP%` / `TMPDIR`, **not** the ACL'd data dir: cover the temp volume with FDE and point `TMP`/`TMPDIR` at an owner-only path ([§10](#10-secure-deployment--operations-checklist)) | -| File-connector output / spill dirs (`.hl7`, `.processed`, `.error`) | all | **Yes** — plaintext on disk | **No** — no cipher at all on this path | **PL-1** | Written by the File transport; treat the directory as PHI and cover it with volume/share encryption + an ACL | -| Application log files (`[logging].log_dir`; under NSSM, `\logs\service.out.log` and `service.err.log`) | all (filesystem, not the DB) | **Possibly** — redaction is best-effort; a single-token identifier can survive it | **No** — plaintext on disk, no app-level cipher | **PL-1** | The engine installs no file handler; NSSM captures stdout/stderr. The defence is the three handler filters + `safe_exc()`/`safe_text()` + the never-log-bodies rule ([§7](#7-logging--phi-redaction) row 1), and the residual is stated there. The directory ACL is the NSSM installer's **best-effort** `icacls /inheritance:r`; age deletion is `[retention].app_log_days` (files by **mtime** — content is never read, so nothing selective happens here) and optional in-place gzip is `[retention].app_log_compress_days` (the compressor **does** read a file's bytes to archive + integrity-verify them, but only in-process — nothing is logged, and the archive stays inside the same ACL'd directory at the source's mtime). A support bundle copies a 500-line tail of this file out of the ACL'd directory entirely ([§7](#7-logging--phi-redaction)). Cover the volume with FDE ([§10](#10-secure-deployment--operations-checklist)) | -| `messages.summary` | all three | **Yes** — MRN / patient name / order | **Yes, when a key is set** — store cipher; AAD `("messages","summary",id)`; store DEK (EF-3) | **PL-2** | Ingest-derived; no SQL search or index exists on it, so encrypting it costs nothing. NULL/blank stay as-is | -| `messages.metadata` | all three | **Yes** — operator/handler-attached values | **Yes, when a key is set** — store cipher; AAD `("messages","metadata",id)`; store DEK (EF-3) | **PL-2** | **Nulled by `purge_message_bodies` on the `[retention].messages_days` window, in the same statement as the body** (ASVS 14.2.7) — see [§8](#8-retention--purge) | -| `messages.error` | all three | **Possibly** — may embed raw fragments from exceptions | **Yes, when a key is set** — store cipher; AAD `("messages","error",id)`; store DEK (WP-5) | **PL-2** | Also `safe_exc()`-redacted **before** write. NULL/blank values stay as-is | -| `queue.last_error` | all three | **Possibly** — same | **Yes, when a key is set** — store cipher; AAD `("queue","last_error",id)`; store DEK (WP-5) | **PL-2** | Same double defence (`safe_exc()` then cipher) | -| `message_events.detail` | all three | **Possibly** — per-message disposition detail | **Yes, when a key is set** — store cipher; AAD `("message_events","detail",message_id,ts,event)`; store DEK | **PL-2** | `id` is AUTOINCREMENT/IDENTITY and unknown at INSERT, so the AAD binds the natural tuple and the column rides its own composite migration/rotation pass. `safe_text()`-scrubbed before write | -| `response.detail`, `response.resp_headers` | all three | **Possibly** — reply diagnostics / partner response headers | **Yes, when a key is set** — store cipher; AAD `("response","detail", …)` / `("response","resp_headers",message_id,destination_name,response_seq)`; store DEK | **PL-2** | `detail` is `safe_text()`-scrubbed and 200-char bounded before the cipher | -| `state.value` (ADR 0005 transform state) | all three | **Possibly** — a correlation map (e.g. MRN→surrogate) written by a Handler | **Yes, when a key is set** — JSON-encoded then store cipher; AAD `("state","value",namespace,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** — reachable only from a Handler via `state_get`/`state_set` | -| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** and **no retention purge** — a snapshot is replaced only by the next sync's build-new-then-flip | -| `search_presets.criteria` (ADR 0136 saved Log-Search filters) | all three | **Yes** — the operator's saved `content` / `field_value` needle is PHI-shaped by construction | **Yes, when a key is set** — store cipher; AAD `("search_presets","criteria",id)`; store DEK | **PL-2** | Never returned by the API: `GET /search/presets` lists names + timestamps only; the needle is loaded server-side by `GET /search/layered`. **`[retention].search_preset_days`** — `purge_search_presets` DELETEs the whole row past the window on every backend, keyed on last-**used** — the later of `updated_at` and `last_used_at`, #306 (`0` = keep forever, the default) ([§8](#8-retention--purge)) | -| `connection_event.reason` (#46 transport/lifecycle log, **default on**) | all three | **Possibly** — a free-text diagnostic fragment | **Yes, when a key is set** — store cipher; AAD `("connection_event","reason",connection,ts,kind)`; store DEK | **PL-2** | Defended twice: the emit site passes a `safe_exc()`-scrubbed string and the store re-applies `safe_text(reason)[:200]`. IDENTITY `id`, so its own composite pass. Every other column is bounded engine/config metadata **except `peer_host`** (its own PL-4 row below) — the table carries no frame, body or HL7 field value. Read under `monitoring:read`, **not** a PHI permission ([§7](#7-logging--phi-redaction)) | -| `connection_event.peer_host` | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The connecting peer's IP, taken from the socket; `NULL` for outbound/unknown. Personal data, not PHI — the **same class and the same decision** as `audit_log.client` / `sessions.client`: plaintext so it stays greppable for incident response. Returned to operators by `GET /events` under `monitoring:read`. Purged with its row by `[retention].connection_event_retention_hours` | -| `alert_instance.reason` (ADR 0044 operator alerts) | all three | **Possibly** — the alert's `detail`/`reason`/`label` free text | **Yes, when a key is set** — store cipher; AAD `("alert_instance","reason",event_type,connection)`; store DEK | **PL-2** | `safe_text(reason)[:200]` before the cipher. The AAD binds the de-dup grain, so the same AAD covers both the INSERT and the re-fire upsert UPDATE that never sees the `id`. Read under `monitoring:diagnose` ([§7](#7-logging--phi-redaction)) | -| `users.totp_secret` | all three | **No** — not PHI | **Yes, when a key is set** — store cipher; AAD `("users","totp_secret",id)`; store DEK | **PL-3** | The base32 TOTP MFA seed. Never returned by any API response model. Its siblings `users.password_hash` and `users.totp_recovery_codes` are **argon2id one-way hashes** and are deliberately **not** ciphered | -| `queue.handler_name` / `destination_name` / `channel_id` | all three | No — names, not bodies | No (metadata, deliberately not ciphered) | **PL-4** | The handler the transform worker runs; the destination the delivery worker drains | -| `messages.control_id`, `messages.message_type` | all three | Low (MSH-10/MSH-9) | **No** — plaintext by design | **PL-4** | Needed plaintext for dedup/routing/indexes (`ix_messages_control`). Covered only by the whole-DB / volume layer | -| `audit_log.detail` | all three | Low — exposed IDs/counts, not bodies | **No** — plaintext by design | **PL-4** | JSON metadata about PHI *access*, not the PHI itself. Its writers only ever store filter shapes, counts and ids | -| `audit_log.client` (ADR 0150) | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The caller's client address — the "from where" of an audited action; `NULL` for engine-internal/`system` writes. **Personal data, but not PHI**, and exactly what HIPAA §164.312(b) audit controls exist to capture. Plaintext by decision: it must stay greppable/indexable for incident response, it already appears in the clear in `sessions.client`, and it is folded **inside** the tamper-evident hash chain — so it carries **integrity** protection even without confidentiality. Widens a store-file compromise from *who did what* to *who did what from where*; volume encryption + owner-only ACLs on whichever host owns the files — the engine's own `_secure_file` covers the **SQLite** store only ([§10](#10-secure-deployment--operations-checklist)) — are the control | -| `delivered_keys` (H2 idempotency ledger) | all three | **No** — hashes + ids only | No (deliberately not ciphered — nothing to protect) | **PL-4** | One row per completed outbound delivery: a SHA-256 `delivery_key` over non-PHI ids + a replay-stable seq, plus `outbox_id`/`message_id`/`destination_name`/`delivery_seq`. **Never a body or any PHI** — `control_id` is only *folded into the hash input*, never stored in the clear here. Lets the FIFO claim skip-and-complete a re-claimed already-delivered head without re-sending | -| `state.namespace` / `state.key` | all three | **Possibly** — a Handler that keys correlation state on a raw MRN stores that identifier here in the clear | **No** — plaintext by construction: the pair is the composite primary key **and** the AAD input for `state.value`, so it cannot be ciphered without losing the lookup | **PL-4** | Authors must key state on a **surrogate, never a raw identifier**. Covered only by the whole-DB / volume layer. Rides `[retention].state_max_age_days` with its value | -| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. **No purge path at all** — a snapshot is replaced only by the next sync's build-new-then-flip | -| `sessions.token_hash` / `client`, `resend_log`, `processed_files`, `pending_approvals.params`, `webauthn_credentials.public_key` | all three | **No** | No (deliberately not ciphered) | **PL-4** | Session tokens are stored as SHA-256 only; `processed_files` holds a hashed derived file key, never a path; approval params carry connection names / channel ids / a config dir by construction; COSE public keys (ADR 0068) are **verification material, not secrets** and are explicitly excluded from the cipher and from rekey | -| `secret_rotation_meta` (`secret_key` / `fingerprint` / `tracked_since` / `last_rotated`) | **All three backends** (#1186 — SQLite `MessageStore`, `SqlServerStore` and `PostgresStore` each create the table at open and implement the `SecretRotationMetaStore` protocol) | **No** — non-secret rotation state: a **keyed MAC** (DEK-derived, one-way — never the secret value) + ISO dates only | No (deliberately not ciphered — it is neither PHI nor a secret; the MAC is one-way and un-guessable without the DEK) | **PL-4** | ASVS 13.3.4 rotation watcher (BACKLOG #282). `fingerprint` is a keyed MAC so obtaining the rows leaks no secret. Written on a keyed store; absent on a keyless one | -| SQLite DB file + `-wal` / `-shm` / temp files, and every index | SQLite | **Yes** (mirror the above) | **No** — the app cipher cannot reach them | **PL-5** | WAL/shm hold recently-written PHI outside any app-level encryption. Cover: SQLCipher (whole-DB) and/or FDE on the engine host's data volume | -| SQL Server `.ldf` **transaction log** + the **tempdb version store**, and every index | SQL Server | **Yes** (row images of `messages`/`queue`, ciphertext columns **plus** the always-plaintext `control_id`/`message_type`) | **No** — outside the app-level AEAD entirely | **PL-5** | The engine itself makes this load-bearing: it **force-enables `READ_COMMITTED_SNAPSHOT` and `ALLOW_SNAPSHOT_ISOLATION`** on the store database at open, so tempdb's version store holds row images for the lifetime of every open snapshot. The `.ldf` holds every row image for the same reason. Additional tempdb objects: the `#eligible` temp table used by `purge_message_bodies` and the FIFO-claim table variables (ids only, non-PHI). Cover: **SQL Server TDE at the database + FDE on the *SQL Server host's* volumes** — **not** BitLocker on the engine host | -| PostgreSQL WAL (`pg_wal`), base files and every index | Postgres | **Yes** (mirror the above) | **No** — outside the app-level AEAD | **PL-5** | Cover: cluster-level / filesystem encryption on the database host, infra-owned | +| Location | Backends | Holds PHI? | Encrypted at rest? (cipher · cell-AAD · key path) | Protection level | Notes | Retention | +|---|---|---|---|---|---|---| +| `messages.raw` | all three | **Yes** — full inbound body | **Yes, when a key is set** — store cipher; AAD `("messages","raw",id)`; store DEK | **PL-1** | Preserved verbatim by design (operators must see what arrived) | `` `[security].delete_message_bodies_after_days` `` | +| `queue.payload` (stage=`ingress`/`routed`) | all three | **Yes** — the raw body, **transient** | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | A second copy of the raw, held only across the route→transform window: the `ingress` row is consumed at `route_handoff`, each `routed` row at `transform_handoff` (deleted, never kept). A stalled router/transform stage can hold several briefly — surfaced by the `queue_buildup` alert | `UNBOUNDED — honest gap` | +| `queue.payload` (stage=`outbound`) | all three | **Yes** — transformed outbound body | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | One row per destination; the persistent footprint | `` `[retention].dead_letter_days` `` | +| `shared_body.body` (store-once-deliver-many) | schema on all three; **rows written on SQLite only** | **Yes** — one transformed outbound body shared by N destinations | **Yes, when a key is set** — store cipher; AAD `("shared_body","body",hash)`; store DEK | **PL-1** | `hash` is the SHA-256 of the **plaintext** body (the content address). Refcounted: GC'd the moment the last referencing outbound row's body is purged. On SQL Server and Postgres the table is schema-parity only — `queue.body_ref` stays `NULL`, so no row is ever written. **Both** server backends nonetheless keep a read-side `LEFT JOIN shared_body` deref in `resend_to` (with its own `cell_aad("shared_body","body",…)` decrypt branch), which is why the column stays **cipher-covered** on all three. It is **rotation-swept on SQLite only** — `shared_body` has a pass in `MessageStore.reencrypt_to_active` but appears in neither server backend's rotation. Harmless while `body_ref` stays `NULL` there, and recorded as a deliberate decision in `store/postgres.py`'s `_CIPHER_COLUMNS` note ("schema-only this increment … it needs no rotation pass here until the dedup insert is wired") | ``rides `[security].delete_message_bodies_after_days` `` | +| `attachment_chunk.ciphertext` (ADR 0105 / #149) | all three | **Yes** — one slice of a detached very-large document (e.g. a base64 PDF from OBX-5.5) | **Yes, when a key is set** — store cipher, **sealed per chunk on write**; AAD `("attachment_chunk","ciphertext",attachment_id,seq)`; store DEK | **PL-1** | The document is detached at ingress, content-addressed (`sha256` of the verbatim plaintext) and chunked; the message keeps only an `mfdoc:v1:ref:` handle. Read back via `GET /messages/{id}/attachments/{id}` (§3) | ``rides `[security].delete_message_bodies_after_days` `` | +| `attachment` header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + `message_attachment` linkage | all three | **No** — size/type/linkage only | No (metadata, deliberately not ciphered) | **PL-4** | The linkage row is the security crux of the download route: it scopes a content address to a message the caller may already read | ``rides `[security].delete_message_bodies_after_days` `` | +| `response.body` (ADR 0013 captured replies; ADR 0021 `kind='ack_sent'`) | all three | **Yes** — the partner's reply body, or the ACK/NAK the engine returned | **Yes, when a key is set** — store cipher; AAD `("response","body",message_id,destination_name,response_seq)`; store DEK | **PL-1** | Composite PK, so it rides its own migration/rotation pass. An **ACK body is stored only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext (fail-safe), and a NAK never stores a body at all | ``rides `[security].delete_message_bodies_after_days` `` | +| `[store].uploads_dir/*.blob` + `*.meta` — the cipher cells `uploaded_file.body` / `uploaded_file.meta` (offline uploaded logs, ADR 0134) | all (filesystem, not the DB) | **Yes** — an operator-uploaded diagnostic message file, held for offline browsing decoupled from any connection | **Yes, when a key is set** — the **same store cipher** (`build_store_cipher`); AAD `("uploaded_file","body")` / `("uploaded_file","meta")` + `file_id`; store DEK. **identity/plaintext-on-disk otherwise** (the File-connector-spill tier below) | **PL-1** | A PHI-at-rest location **outside** the message store, opt-in (unset ⇒ the subsystem is disabled — no surface). On-disk identity is a random 32-hex `file_id` (path-traversal guard); the operator filename is display-only. Every access is `files:*`-gated, browse is step-up + PHI-hop-guarded, all audited (metadata only). **Not** re-encrypted by `rotate-key` (outside the store) — stays readable via the decrypt keyring. The dir is created `0o700` and the sidecar written `0o600` **best-effort, and both are no-ops on Windows** — the engine applies no ACL here (it does not call the `icacls` enforcer). **Retention + quotas (ASVS 5.2.4):** uploaded files auto-prune after `[store].uploads_retention_days` (default **30**) — swept opportunistically at save time and by a periodic task; every prune is audited (`upload.prune`, file_id + uploader only, never content). Per-uploader caps `[store].max_upload_files_per_user` (default **100**) / `[store].max_upload_total_bytes_per_user` (default **250 MiB**) bound the at-rest volume; a would-be over-quota upload is refused **HTTP 409** with an `upload.reject_quota` audit before anything is written (defaults-ON, `ge=1` floors; quota is per-process per-`uploads_dir`). Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | `` `[store].uploads_retention_days` `` | +| `[backup].destination/mefor-backup-*.mfbak` (ADR 0049 DR backup) | **SQLite only** carries bodies | **SQLite: Yes** — a consistent store snapshot (full inbound + outbound bodies) + the config bundle. **SQL Server / Postgres: No** — config bundle only | **Yes** — `.mfbak` chunked-AEAD codec under the **store DEK** (`resolve_active_key`); an identity-cipher (no-key) box is **refused** unless `[backup].allow_unencrypted` writes a `.mfbak.plain` | **PL-1** (SQLite) / **PL-4** (server backends) | On a **server-DB store `snapshot_to` raises `DbaDelegatedError`**, so the BackupRunner writes a **config-only** archive — or skips entirely when `[backup].config_only_on_server_db = false`. There is therefore **no `.mfbak` containing message bodies on SQL Server or Postgres**; the DB-tier backup there is `BACKUP DATABASE` / Always On / `pg_dump` / PITR, infra-owned. Where bodies *are* present it is a second at-rest PHI copy, bounded by keep-N retention; like `uploads_dir` it is **not** re-encrypted by `rotate-key`. The share's own ACLs are infra-owned | ``keep-N `[backup].retention_keep` `` | +| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir, ADR 0049) | SQLite carries bodies; server backends config-only | **Yes** — a full store snapshot, and on verify a **decrypted** archive | **No** — the snapshot keeps the store's own column cipher, but the staging tar and the verify extraction are **plaintext on disk**; no engine ACL (`_secure_file` is never called on these paths) | **PL-1** | `run_backup` snapshots the store to `/store.db` and tars it **plaintext** before sealing it into the `.mfbak` (`pipeline/dr_backup.py`), and `[backup].verify_after_backup` (**default `true`**) decrypts the archive straight back out to a second temp dir on **every** run — independent of `full_restore_verify`. Transient (the `TemporaryDirectory` unlinks on exit) but **not** on a crash or `SIGKILL`. Lives under `%TEMP%` / `TMPDIR`, **not** the ACL'd data dir: cover the temp volume with FDE and point `TMP`/`TMPDIR` at an owner-only path ([§10](#10-secure-deployment--operations-checklist)) | `UNBOUNDED — honest gap` | +| File-connector output / spill dirs (`.hl7`, `.processed`, `.error`) | all | **Yes** — plaintext on disk | **No** — no cipher at all on this path | **PL-1** | Written by the File transport; treat the directory as PHI and cover it with volume/share encryption + an ACL | `UNBOUNDED — honest gap` | +| Application log files (`[logging].log_dir`; under NSSM, `\logs\service.out.log` and `service.err.log`) | all (filesystem, not the DB) | **Possibly** — redaction is best-effort; a single-token identifier can survive it | **No** — plaintext on disk, no app-level cipher | **PL-1** | The engine installs no file handler; NSSM captures stdout/stderr. The defence is the three handler filters + `safe_exc()`/`safe_text()` + the never-log-bodies rule ([§7](#7-logging--phi-redaction) row 1), and the residual is stated there. The directory ACL is the NSSM installer's **best-effort** `icacls /inheritance:r`; age deletion is `[retention].app_log_days` (files by **mtime** — content is never read, so nothing selective happens here) and optional in-place gzip is `[retention].app_log_compress_days` (the compressor **does** read a file's bytes to archive + integrity-verify them, but only in-process — nothing is logged, and the archive stays inside the same ACL'd directory at the source's mtime). A support bundle copies a 500-line tail of this file out of the ACL'd directory entirely ([§7](#7-logging--phi-redaction)). Cover the volume with FDE ([§10](#10-secure-deployment--operations-checklist)) | `` `[retention].app_log_days` `` | +| `messages.summary` | all three | **Yes** — MRN / patient name / order | **Yes, when a key is set** — store cipher; AAD `("messages","summary",id)`; store DEK (EF-3) | **PL-2** | Ingest-derived; no SQL search or index exists on it, so encrypting it costs nothing. NULL/blank stay as-is | ``rides `[security].delete_message_bodies_after_days` `` | +| `messages.metadata` | all three | **Yes** — operator/handler-attached values | **Yes, when a key is set** — store cipher; AAD `("messages","metadata",id)`; store DEK (EF-3) | **PL-2** | **Nulled by `purge_message_bodies` on the `[retention].messages_days` window, in the same statement as the body** (ASVS 14.2.7) — see [§8](#8-retention--purge) | ``rides `[security].delete_message_bodies_after_days` `` | +| `messages.error` | all three | **Possibly** — may embed raw fragments from exceptions | **Yes, when a key is set** — store cipher; AAD `("messages","error",id)`; store DEK (WP-5) | **PL-2** | Also `safe_exc()`-redacted **before** write. NULL/blank values stay as-is | ``rides `[security].delete_message_bodies_after_days` `` | +| `queue.last_error` | all three | **Possibly** — same | **Yes, when a key is set** — store cipher; AAD `("queue","last_error",id)`; store DEK (WP-5) | **PL-2** | Same double defence (`safe_exc()` then cipher) | ``rides `[security].delete_message_bodies_after_days` `` | +| `message_events.detail` | all three | **Possibly** — per-message disposition detail | **Yes, when a key is set** — store cipher; AAD `("message_events","detail",message_id,ts,event)`; store DEK | **PL-2** | `id` is AUTOINCREMENT/IDENTITY and unknown at INSERT, so the AAD binds the natural tuple and the column rides its own composite migration/rotation pass. `safe_text()`-scrubbed before write | ``rides `[security].delete_message_bodies_after_days` `` | +| `response.detail`, `response.resp_headers` | all three | **Possibly** — reply diagnostics / partner response headers | **Yes, when a key is set** — store cipher; AAD `("response","detail", …)` / `("response","resp_headers",message_id,destination_name,response_seq)`; store DEK | **PL-2** | `detail` is `safe_text()`-scrubbed and 200-char bounded before the cipher | ``rides `[security].delete_message_bodies_after_days` `` | +| `state.value` (ADR 0005 transform state) | all three | **Possibly** — a correlation map (e.g. MRN→surrogate) written by a Handler | **Yes, when a key is set** — JSON-encoded then store cipher; AAD `("state","value",namespace,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** — reachable only from a Handler via `state_get`/`state_set` | `` `[retention].state_max_age_days` `` | +| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API.** **`[retention].reference_snapshot_days`** — `purge_reference_snapshots` DELETEs the rows of a set config **no longer declares** whose active version was synced before the cutoff (`0` = keep forever, the default) ([§8](#8-retention--purge)). **Orphan-only, and the limit is the point:** a set that IS still declared is never touched however old its `synced_at`, because its snapshot is live data the engine serves — so the normal case, a wired set holding live PHI, is still bounded by nothing. Do not restate this as a plain window over `reference.value` | ``orphan-only `[retention].reference_snapshot_days` `` | +| `search_presets.criteria` (ADR 0136 saved Log-Search filters) | all three | **Yes** — the operator's saved `content` / `field_value` needle is PHI-shaped by construction | **Yes, when a key is set** — store cipher; AAD `("search_presets","criteria",id)`; store DEK | **PL-2** | Never returned by the API: `GET /search/presets` lists names + timestamps only; the needle is loaded server-side by `GET /search/layered`. **`[retention].search_preset_days`** — `purge_search_presets` DELETEs the whole row past the window on every backend, keyed on last-**used** — the later of `updated_at` and `last_used_at`, #306 (`0` = keep forever, the default) ([§8](#8-retention--purge)) | `` `[retention].search_preset_days` `` | +| `connection_event.reason` (#46 transport/lifecycle log, **default on**) | all three | **Possibly** — a free-text diagnostic fragment | **Yes, when a key is set** — store cipher; AAD `("connection_event","reason",connection,ts,kind)`; store DEK | **PL-2** | Defended twice: the emit site passes a `safe_exc()`-scrubbed string and the store re-applies `safe_text(reason)[:200]`. IDENTITY `id`, so its own composite pass. Every other column is bounded engine/config metadata **except `peer_host`** (its own PL-4 row below) — the table carries no frame, body or HL7 field value. Read under `monitoring:read`, **not** a PHI permission ([§7](#7-logging--phi-redaction)) | `` `[retention].connection_event_retention_hours` `` | +| `connection_event.peer_host` | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The connecting peer's IP, taken from the socket; `NULL` for outbound/unknown. Personal data, not PHI — the **same class and the same decision** as `audit_log.client` / `sessions.client`: plaintext so it stays greppable for incident response. Returned to operators by `GET /events` under `monitoring:read`. Purged with its row by `[retention].connection_event_retention_hours` | ``rides `[retention].connection_event_retention_hours` `` | +| `alert_instance.reason` (ADR 0044 operator alerts) | all three | **Possibly** — the alert's `detail`/`reason`/`label` free text | **Yes, when a key is set** — store cipher; AAD `("alert_instance","reason",event_type,connection)`; store DEK | **PL-2** | `safe_text(reason)[:200]` before the cipher. The AAD binds the de-dup grain, so the same AAD covers both the INSERT and the re-fire upsert UPDATE that never sees the `id`. Read under `monitoring:diagnose` ([§7](#7-logging--phi-redaction)) | ``rides `[retention].connection_event_retention_hours` `` | +| `users.totp_secret` | all three | **No** — not PHI | **Yes, when a key is set** — store cipher; AAD `("users","totp_secret",id)`; store DEK | **PL-3** | The base32 TOTP MFA seed. Never returned by any API response model. Its siblings `users.password_hash` and `users.totp_recovery_codes` are **argon2id one-way hashes** and are deliberately **not** ciphered | `keep-forever by design` — it lives and dies with the user row | +| `queue.handler_name` / `destination_name` / `channel_id` | all three | No — names, not bodies | No (metadata, deliberately not ciphered) | **PL-4** | The handler the transform worker runs; the destination the delivery worker drains | `n/a — not PHI` | +| `messages.control_id`, `messages.message_type` | all three | Low (MSH-10/MSH-9) | **No** — plaintext by design | **PL-4** | Needed plaintext for dedup/routing/indexes (`ix_messages_control`). Covered only by the whole-DB / volume layer | `keep-forever by design` — dedup/routing keys that live and die with the message row | +| `audit_log.detail` | all three | Low — exposed IDs/counts, not bodies | **No** — plaintext by design | **PL-4** | JSON metadata about PHI *access*, not the PHI itself. Its writers only ever store filter shapes, counts and ids | `keep-forever by design` — 45 CFR 164.316(b)(2)(i) six-year documentation retention; deleting rows would break the tamper-evident hash chain | +| `audit_log.client` (ADR 0150) | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The caller's client address — the "from where" of an audited action; `NULL` for engine-internal/`system` writes. **Personal data, but not PHI**, and exactly what HIPAA §164.312(b) audit controls exist to capture. Plaintext by decision: it must stay greppable/indexable for incident response, it already appears in the clear in `sessions.client`, and it is folded **inside** the tamper-evident hash chain — so it carries **integrity** protection even without confidentiality. Widens a store-file compromise from *who did what* to *who did what from where*; volume encryption + owner-only ACLs on whichever host owns the files — the engine's own `_secure_file` covers the **SQLite** store only ([§10](#10-secure-deployment--operations-checklist)) — are the control | `keep-forever by design` — same `audit_log` row lifetime as `detail`; the value is folded **inside** the hash chain | +| `delivered_keys` (H2 idempotency ledger) | all three | **No** — hashes + ids only | No (deliberately not ciphered — nothing to protect) | **PL-4** | One row per completed outbound delivery: a SHA-256 `delivery_key` over non-PHI ids + a replay-stable seq, plus `outbox_id`/`message_id`/`destination_name`/`delivery_seq`. **Never a body or any PHI** — `control_id` is only *folded into the hash input*, never stored in the clear here. Lets the FIFO claim skip-and-complete a re-claimed already-delivered head without re-sending | `keep-forever by design` — the idempotency ledger a re-claimed already-delivered row checks instead of re-sending | +| `state.namespace` / `state.key` | all three | **Possibly** — a Handler that keys correlation state on a raw MRN stores that identifier here in the clear | **No** — plaintext by construction: the pair is the composite primary key **and** the AAD input for `state.value`, so it cannot be ciphered without losing the lookup | **PL-4** | Authors must key state on a **surrogate, never a raw identifier**. Covered only by the whole-DB / volume layer. Rides `[retention].state_max_age_days` with its value | ``rides `[retention].state_max_age_days` `` | +| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. The key columns ride the same delete: `purge_reference_snapshots` removes whole rows of an **undeclared** set, so these go with the `value` they key. A **declared** set is never touched — same orphan-only limit as `reference.value` above | ``orphan-only `[retention].reference_snapshot_days` `` | +| `sessions.token_hash` / `client`, `resend_log`, `processed_files`, `pending_approvals.params`, `webauthn_credentials.public_key` | all three | **No** | No (deliberately not ciphered) | **PL-4** | Session tokens are stored as SHA-256 only; `processed_files` holds a hashed derived file key, never a path; approval params carry connection names / channel ids / a config dir by construction; COSE public keys (ADR 0068) are **verification material, not secrets** and are explicitly excluded from the cipher and from rekey | `n/a — not PHI` | +| `secret_rotation_meta` (`secret_key` / `fingerprint` / `tracked_since` / `last_rotated`) | **All three backends** (#1186 — SQLite `MessageStore`, `SqlServerStore` and `PostgresStore` each create the table at open and implement the `SecretRotationMetaStore` protocol) | **No** — non-secret rotation state: a **keyed MAC** (DEK-derived, one-way — never the secret value) + ISO dates only | No (deliberately not ciphered — it is neither PHI nor a secret; the MAC is one-way and un-guessable without the DEK) | **PL-4** | ASVS 13.3.4 rotation watcher (BACKLOG #282). `fingerprint` is a keyed MAC so obtaining the rows leaks no secret. Written on a keyed store; absent on a keyless one | `n/a — not PHI` | +| SQLite DB file + `-wal` / `-shm` / temp files, and every index | SQLite | **Yes** (mirror the above) | **No** — the app cipher cannot reach them | **PL-5** | WAL/shm hold recently-written PHI outside any app-level encryption. Cover: SQLCipher (whole-DB) and/or FDE on the engine host's data volume | `n/a — not PHI` | +| SQL Server `.ldf` **transaction log** + the **tempdb version store**, and every index | SQL Server | **Yes** (row images of `messages`/`queue`, ciphertext columns **plus** the always-plaintext `control_id`/`message_type`) | **No** — outside the app-level AEAD entirely | **PL-5** | The engine itself makes this load-bearing: it **force-enables `READ_COMMITTED_SNAPSHOT` and `ALLOW_SNAPSHOT_ISOLATION`** on the store database at open, so tempdb's version store holds row images for the lifetime of every open snapshot. The `.ldf` holds every row image for the same reason. Additional tempdb objects: the `#eligible` temp table used by `purge_message_bodies` and the FIFO-claim table variables (ids only, non-PHI). Cover: **SQL Server TDE at the database + FDE on the *SQL Server host's* volumes** — **not** BitLocker on the engine host | `n/a — not PHI` | +| PostgreSQL WAL (`pg_wal`), base files and every index | Postgres | **Yes** (mirror the above) | **No** — outside the app-level AEAD | **PL-5** | Cover: cluster-level / filesystem encryption on the database host, infra-owned | `n/a — not PHI` | + +> **Retention column — vocabulary (ASVS 14.2.7).** Every cell is exactly one of seven forms. The serve +> gate's tier list and the §2↔§8 drift test are GENERATED from these, so there are no prose variants — +> a hand-typed tuple with a longer literal is the same defect this column exists to remove. +> +> - **`[section].window`** — bounded by its **own** named window, cited exactly as an operator types it. +> - **rides `[section].window`** — no window of its own; deleted or nulled by another tier's purge. +> Valid **only** when §8's row for that window names this table/column in its Mechanism cell. This is +> the form most likely to be wrong, because it asserts coverage that lives somewhere else. +> - **orphan-only `[retention].reference_snapshot_days`** — `reference.*` only: purged **only** when +> config no longer declares the set. A **declared** set is never touched, whatever its age. +> - **keep-forever by design** — a deliberate decision, not a gap; the clause after the dash is the reason. +> - **n/a — not PHI** — PL-4 operational metadata or PL-5 engine-unreachable substrate. +> - **keep-N `[backup].retention_keep`** — bounded by a **count** of retained artifacts, not an age window. +> - **UNBOUNDED — honest gap** — no purge covers this tier. Stated plainly on purpose: an honest gap is +> worth more than a coverage claim that cannot be evidenced, and every one of these is also listed in +> [§8](#8-retention--purge). +> +> Machine-readable by construction: the form keyword is **un-backticked** and the setting is +> **backticked**, so one pattern extracts the window from every bounded form, and the three prose forms +> contain no backticked setting at all. **Per-backend cipher coverage, stated exactly.** The store cipher covers **18** `(table, column)` -pairs on SQLite. **SQL Server** covers 18 = the SQLite set **minus** `shared_body.body` (never written -there) **plus** the legacy `outbox.payload`. **Postgres** covers 17 = the SQLite set **minus** -`shared_body.body`. Two further asymmetries are real and worth knowing. **(1)** Neither SQL Server nor -Postgres sweeps `attachment_chunk` in its *on-open* plaintext→cipher migration (only in `rotate-key`), -which is harmless today because `put_attachment` always seals on write, but means a legacy -no-key→key transition would not sweep chunks the way SQLite's does. **(2)** SQL Server's legacy -`outbox.payload` is swept by the on-open migration but is **absent from `reencrypt_to_active`**, so a -rotation that retires the old key leaves those bodies undecryptable (the row above says the same). +pairs on SQLite. **SQL Server** covers 17 = the SQLite set **minus** `shared_body.body` (never written +there). **Postgres** covers 17 = the SQLite set **minus** `shared_body.body`. SQL Server's count was +18 until the legacy `outbox.payload` was retired (below); the two server backends now carry the same +set. One asymmetry remains and is worth knowing: neither SQL Server nor Postgres sweeps +`attachment_chunk` in its *on-open* plaintext→cipher migration (only in `rotate-key`), which is +harmless today because `put_attachment` always seals on write, but means a legacy no-key→key +transition would not sweep chunks the way SQLite's does. + +**The legacy SQL Server `outbox` table is gone (ASVS 14.2.7), and that closed two real gaps.** It was +recreated by the schema pass on every open and read by nothing, so a store upgraded from the +pre-staged-pipeline layout kept full outbound PHI bodies there that no purge on any backend reached — +while `messages.raw` blanked on its own window, so the message read as purged. It also sat outside +`reencrypt_to_active`, so a key rotation that retired the old key left those bodies undecryptable. +Both close the same way: a guarded statement in the SQL Server schema batch folds surviving rows into +`queue` as `stage='outbound'` — payload carried over verbatim, so encryption at rest is preserved — +and then `DROP`s the table. Migrated rows are ordinary outbound queue rows, bounded by +`[security].delete_message_bodies_after_days` / `[retention].dead_letter_days`, swept by the on-open +cipher migration and rotated by `reencrypt_to_active`, so the tier no longer needs its own row here. +SQLite already performed the equivalent migration (`_migrate_outbox_to_queue`); Postgres never had the +table. **The body cipher `[BUILT]`.** Each backend routes the columns above through the store's `_cipher` ([store/crypto.py](../messagefoundry/store/crypto.py)) on write/read — **AES-256-GCM when a store key @@ -225,7 +257,7 @@ for defense-in-depth without swapping the `aiosqlite` connector. 1. **Application-level AES-256-GCM `[BUILT]`.** The store's `_cipher` ([store/crypto.py](../messagefoundry/store/crypto.py)) encrypts the cipher-covered columns. **[§2](#2-where-phi-lives--data-at-rest-inventory) is the normative list** — 18 `(table, column)` - pairs on SQLite, 18 on SQL Server, 17 on Postgres, plus the two `uploaded_file` sidecar cells. The + pairs on SQLite, 17 on SQL Server, 17 on Postgres, plus the two `uploaded_file` sidecar cells. The per-level blocks below group that same set; they do not redefine it, and CI pins the counts **and** the membership of both sections to the store's cipher registry, so they cannot diverge. Stored format `mfenc:v1 ‖ key_id ‖ base64(nonce ‖ ciphertext ‖ GCM tag)` — the GCM tag @@ -259,9 +291,16 @@ for defense-in-depth without swapping the `aiosqlite` connector. `associated_data`, so **`v3` is cell-bound regardless of `aad_bind`**; and the audit-chain MAC is computed inside Transit. Missing config or an unreachable/unknown Transit key **fails closed** at `open_store` (`serve` refuses to start) — never an in-process fallback. Caveat worth knowing: the - Transit-backed audit MAC is threaded into the **SQLite** store only, so a `vault_transit` + SQL Server - or Postgres deployment runs an **unkeyed SHA-256** audit chain (tamper-evident, not - forgery-resistant). + Transit-backed audit MAC reaches **all three** backends. This bullet previously said SQLite-only — that + was the PRE-#301 state stated as current, and it was wrong in the direction that flatters nothing: + `TransitCipher.audit_mac_key()` returns `None` **by design**, so a server backend given only + `audit_mac_key` had no keying secret at all. #301 threads `audit_mac_fn` alongside it + (`store/base.py:1817`, forwarded at `:1826`/`:1836`), which is what closed it. `docs/ASVS-L2-PHASE0-CHANGES.md` + §"Audit chain" carries the accurate wording — the digest primitive is *"shared verbatim by all three + backends"*. **What IS unkeyed is the KEYLESS posture, not a backend:** with no store key, + `audit_mac_key()` returns `None` (`store/crypto.py:428`) and the chain stays keyless SHA-256 — + tamper-evident against a careless edit, not forgery-resistant against anyone who can write the + table. At-rest encryption is off by default, so that is the DEFAULT posture. 2. **Key management + rotation `[BUILT]`.** The key is a base64 32-byte secret from the **environment** (`MEFOR_STORE_ENCRYPTION_KEY`), never the TOML file — reusing the existing secrets convention (cf. `MEFOR_STORE_PASSWORD`). Mint one with `messagefoundry gen-key`. On Windows it may instead live @@ -350,7 +389,7 @@ a statement about *what is built today*; where a control does not exist, it says #### PL-1 · PHI body **Applies to:** `messages.raw` · `queue.payload` · `shared_body.body` · `attachment_chunk.ciphertext` · -`response.body` · `outbox.payload` (SQL Server legacy) · `[store].uploads_dir` blobs +`response.body` · `[store].uploads_dir` blobs (`uploaded_file.body` / `uploaded_file.meta`) · `.mfbak` archives (SQLite) · `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir) · File-connector spill dirs · application log files (`[logging].log_dir`). @@ -398,7 +437,9 @@ application log files (`[logging].log_dir`). by `purge_dead_letters`; `response.body` is set to `NULL` in place by the same pass; `shared_body.body` is refcount-decremented and GC'd at 0 when its **last** referrer is purged; streaming attachments are decref'd + GC'd at 0 (plus a startup `sweep_orphan_attachments`). - **`outbox.payload` is purged by nothing on any backend**, while `[store].uploads_dir` blobs auto-prune + The legacy SQL Server `outbox.payload` no longer exists to be purged — its rows were folded into + `queue` and the table DROPped ([§2](#2-where-phi-lives--data-at-rest-inventory)), so they now ride the + same window as any other outbound row. `[store].uploads_dir` blobs auto-prune after `[store].uploads_retention_days` (default **30**) via an age-based sweep — a periodic `UploadRetentionRunner` plus an opportunistic pass at save time, each pruned pair audited `upload.prune` (ASVS 5.2.4, #291); an operator `DELETE` is an additional removal path. **`.mfbak` archives** are bounded @@ -476,7 +517,7 @@ header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + the `mess linkage · `secret_rotation_meta` (all three backends) · `.mfbak` on the server backends. Deliberately **not** ciphered, so that ids stay indexable and the audit trail stays greppable for -incident response. Integrity for `audit_log` comes from the **tamper-evident hash chain** (the `client` +incident response. Integrity for `audit_log` comes from the **tamper-evident hash chain** — *keyless SHA-256 in the default keyless posture, upgraded to HMAC-SHA256 on an HKDF-derived subkey of the store DEK only when a store key is set (#190), or an isolated-module Transit MAC under `cipher_provider=vault_transit`* — so its strength is **key-custody-dependent**, and the unqualified reading describes the non-default case (the `client` address is folded *inside* it), not from a cipher. **Access, per tier — several of these ARE returned by an API, under RBAC:** @@ -716,7 +757,7 @@ control unchanged (`messages:view_raw`/`view_summary` RBAC, field-level redactio - **No PHI in browser storage.** The only stored item is the session token, in an **HttpOnly + SameSite= Strict** cookie JS cannot read (`mf_session`). Nothing is written to `localStorage`/`sessionStorage`/ - `IndexedDB`, and no PHI is placed in a URL (so it cannot leak into history, bookmarks, or Referer). + `IndexedDB`. **PHI in URLs — corrected 2026-07-30, and §7 was the accurate half:** this bullet used to claim no PHI is placed in a URL at all. The console's own search route takes the needle as a QUERY PARAMETER — `GET /ui/messages/search?content=…&field_value=…` (`messagefoundry_webconsole/routes/search.py`) — so a search URL can carry an MRN into history, bookmarks and `Referer`. What holds is narrower: no *message body* is ever placed in a URL, and `Referrer-Policy: no-referrer` is set. Note that `_security.py`'s comment justifies that header on the grounds that "/ui URLs carry opaque ids only (never PHI)" — the header is still right, its stated REASON is not. Moving the needle out of the URL is tracked separately (§7). - **No caching.** Every `/ui` HTML response and every PHI JSON read is served `Cache-Control: no-store`, so a browser/proxy never retains a message body on disk. The covered set is the PHI-read route families — `/messages*`, `/dead-letters*`, `/search*`, `/logs*`, `/uploads*` (`_NO_STORE_PREFIXES` in @@ -997,6 +1038,7 @@ method exists for `Store`-protocol completeness and deliberately does nothing), | `purge_state` (`state.value`) | `[retention].state_max_age_days` | `DELETE` by `set_at` | enforced | **enforced** | **enforced** | | `purge_connection_events` (incl. `connection_event.reason`) | `[retention].connection_event_retention_hours`; 0 inherits `messages_days` | `DELETE` by `ts` (metadata-only) | enforced | **enforced** | **enforced** | | `purge_search_presets` (`search_presets.criteria`) | `[retention].search_preset_days`; `0` = keep forever | `DELETE` by the null-safe greater of `updated_at` / `last_used_at` — whole row (the criteria *is* the payload). Keys on last-**used** (#306); a row predating `last_used_at` (NULL) ages out on `updated_at` alone | enforced | **enforced** | **enforced** | +| `purge_reference_snapshots` (`reference.value` + its key columns) | `[retention].reference_snapshot_days`; `0` = keep forever | `DELETE` of whole rows for a set config **no longer declares** whose `reference_version.synced_at` predates the cutoff — eligibility is re-asserted **inside** the delete (a config reload can commit a fresh snapshot between the decision and the statement). The `reference_version` pointer **survives** with its version bumped to `purged:` and `row_count = 0`, which is what makes a cluster follower converge — `converge_reference_cache` only reloads a set whose version CHANGED, so leaving it would let a follower serve purged PHI from RAM until restart, and deleting the pointer is worse (converge only adds names present in a fresh read). **Orphan-only** — a declared set is never purged | enforced | **enforced** | **enforced** | | `purge_alert_instances` (incl. `alert_instance.reason`) | same window as connection events | `DELETE` by `resolved_at`, **RESOLVED instances only** | enforced | **enforced** | **enforced** | | `strip_embedded_documents` | per-inbound `prune_documents_after` + `prune_documents_min_bytes` (**no global default** — nothing is stripped without an override) | in-place strip of bulky base64 documents; sets `messages.documents_pruned` | enforced | **enforced** | **enforced** | | Streaming-attachment release (`release_message_attachments`) | rides the two body windows | refcount decref + GC at 0, plus a startup `sweep_orphan_attachments` | enforced | **enforced** | **enforced** | @@ -1062,8 +1104,9 @@ window purely so a replay can be richer — is precisely the defect ASVS 14.2.7 | Tier | Status | |---|---| -| `reference.value` | no purge — a snapshot is replaced only by the next sync's build-new-then-flip | -| `outbox.payload` (SQL Server legacy) | recreated on every open, **touched by no purge on any backend** — legacy PHI there is retained forever | +| `reference.value` (a **declared** set) | **orphan-only** coverage. `purge_reference_snapshots` bounds a set config has DROPPED; a set still declared is never purged whatever its age, because its snapshot is live data. The wired-set case remains unbounded — the honest gap | +| `queue.payload` (stage=`ingress`/`routed`, **DEAD**) | **no purge reaches it** — added 2026-07-30 by the ASVS 14.2.7 classification sweep, which is the only reason it was found. The happy path consumes these rows at `route_handoff`/`transform_handoff`, so they are documented as transient — but a router or handler content fault calls `dead_letter_now` (`pipeline/wiring_runner.py:4415`, `:4449`), which sets status/`last_error` and **never touches `payload`**, while both purges are scoped `Stage.OUTBOUND` (`store/store.py:8384`, `:8659`). So `messages.raw` blanks on its window and the message reads as purged while a **full raw PHI body survives here indefinitely**. Filed as its own defect; the likely fix is to let a DEAD ingress/routed row ride `[retention].dead_letter_days` exactly as a dead outbound row does | +| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs | no window — a `TemporaryDirectory` unlinks on exit, but **not** on a crash or `SIGKILL`, and `verify_after_backup` (default `true`) decrypts a full archive back out to a second temp dir on **every** run. The engine applies **no ACL** on these paths (`_secure_file` is never called on them, and on a server-DB store — the deployed posture — it applies no file ACL at all), so the cover is the operator's: FDE on the temp volume plus `TMP`/`TMPDIR` pointed at a directory the OS restricts to the service account ([§10](#10-secure-deployment--operations-checklist)) | | `users.totp_secret` | no window by design — it lives and dies with the user row | | `audit_log`, `delivered_keys`, `resend_log` | keep-forever by design (see below) | | `messages.control_id`, `messages.message_type` | kept for the life of the message row by design (dedup/routing keys) | diff --git a/harness/load/connscale/runner.py b/harness/load/connscale/runner.py index 8b37510d..f94fab94 100644 --- a/harness/load/connscale/runner.py +++ b/harness/load/connscale/runner.py @@ -72,14 +72,14 @@ # to). On a SERVER backend every (mode, count) step shares ONE database, so — mirroring the SQLite # fresh-file-per-step path — each step first EMPTIES these so the pooled arm never runs second against # the per_lane arm's residue (the carryover confound). Table lists verified against the store schemas -# (store/sqlserver.py, store/postgres.py) and match the reset the store tests use: SQL Server carries -# the legacy `outbox` table alongside the unified `queue`; the Postgres backend has no `outbox`. +# (store/sqlserver.py, store/postgres.py) and match the reset the store tests use. Neither backend has +# an `outbox` table: SQL Server's legacy one was folded into `queue` and DROPped (ASVS 14.2.7), and +# Postgres never had one. Naming it here would fail the reset with *Invalid object name*. _SQLSERVER_PIPELINE_TABLES = ( "message_events", "queue", "response", "delivered_keys", - "outbox", "messages", ) diff --git a/harness/load/shardcert.py b/harness/load/shardcert.py index 0be15449..a151c70a 100644 --- a/harness/load/shardcert.py +++ b/harness/load/shardcert.py @@ -1180,7 +1180,6 @@ async def _reset_store(env: Mapping[str, str]) -> None: cur = await conn.cursor() for table in ( "queue", - "outbox", "response", "delivered_keys", "state", diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index 49cae9b0..0ff9950d 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -2037,45 +2037,97 @@ def _serve(args: argparse.Namespace) -> int: # default threads through to the RetentionRunner (no forbidden-file edit). # messages_days moved to [security].delete_message_bodies_after_days (ADR 0118); # dead_letter_days stays [retention] plumbing — label each window at its real home. - _RETENTION_WINDOW_LABEL = { - "messages_days": "[security].delete_message_bodies_after_days", - "dead_letter_days": "[retention].dead_letter_days", - } - if not enforcing and not settings.retention.allow_unbounded_phi: - auto_bounded = [ - field - for field in ("messages_days", "dead_letter_days") - if field not in settings.retention.model_fields_set + # ASVS 14.2.7: the tier list is GENERATED from the classification in + # config/retention_classification.py, which a drift test holds equal — in both directions — to + # docs/PHI.md §2's Retention column. It used to be a two-element literal here, and the cell + # broke once because a new PHI tier landed and nobody widened it. A wider literal with no + # binding to the classification is the same defect with more characters. + from messagefoundry.config.retention_classification import ( + MIN_PHI_RETENTION_WINDOWS, + PHI_RETENTION_WINDOWS, + auto_bounded_windows, + ) + from messagefoundry.config.retention_classification import ( + unbounded_windows as _unbounded_windows, + ) + + # A FLOOR, not an emptiness check. `if not PHI_RETENTION_WINDOWS` passes for a one-element + # tuple, so a bad merge dropping most entries would leave this gate checking one window while + # reporting success — the precise shape of failure this whole change set exists to remove. + if len(PHI_RETENTION_WINDOWS) < MIN_PHI_RETENTION_WINDOWS: + print( + f"error: the PHI retention classification has shrunk to " + f"{len(PHI_RETENTION_WINDOWS)} windows (floor {MIN_PHI_RETENTION_WINDOWS}); refusing " + "to start rather than gate on a partial classification. This is a build defect, not a " + "configuration one — see messagefoundry/config/retention_classification.py.", + file=sys.stderr, + ) + return 2 + + # AUTO-BOUND. Owner ruling 2026-07-30: the three PHI-BODY windows default to 30 days when + # UNSET, on BOTH dials — previously this ran only when `not enforcing`, so on the shipped + # `enforce` posture an unset window took the refusal below instead of a default. + # + # THE SAFETY TRADE IS DELIBERATE AND WORTH STATING: a production PHI instance with an unset + # window used to REFUSE TO START, which forced an operator to choose a number. It now starts + # with 30. What survives is the fail-closed path for an EXPLICIT 0 — choosing keep-forever is + # still refused unless the audited opt-out is set. So "unbounded by accident" is still + # prevented; "unbounded by inattention" becomes "30 days by inattention". + # + # The warn-only windows are NOT auto-bounded, and that is also a ruling rather than an + # omission: `purge_state` and `purge_search_presets` key on timestamps that only move on a + # WRITE, so silently bounding them deletes live operational data a Handler is still reading. + if not settings.retention.allow_unbounded_phi: + defaulted = [ + w + for w in auto_bounded_windows() + if w.field not in getattr(settings, w.reads_from.strip("[]")).model_fields_set ] - for field in auto_bounded: - setattr(settings.retention, field, 30) - if auto_bounded: - auto_desc = ", ".join(_RETENTION_WINDOW_LABEL[field] for field in auto_bounded) + for window in defaulted: + setattr( + getattr(settings, window.reads_from.strip("[]")), + window.field, + window.auto_bound_days, + ) + if defaulted: print( - f"info: {auto_desc} defaulted ON (30 days) for a PHI instance ({env_name!r}) — " - "PHI message bodies are now bounded at rest (secure-by-default, ASVS 14.2.7). Set an " - "explicit [retention] window to override, or [security].allow_keeping_phi_indefinitely=true to " - "retain indefinitely.", + f"info: {', '.join(w.setting for w in defaulted)} defaulted ON (30 days) for a PHI " + f"instance ({env_name!r}) — these PHI tiers are now bounded at rest " + "(secure-by-default, ASVS 14.2.7). Set an explicit window to override, or " + "[security].allow_keeping_phi_indefinitely=true to retain indefinitely.", file=sys.stderr, ) - unbounded_windows = [ - field - for field, days in ( - ("messages_days", settings.retention.messages_days), - ("dead_letter_days", settings.retention.dead_letter_days), + + # REFUSE / WARN. `unbounded_windows` skips the tiers where 0 does not mean unbounded + # (`connection_event_retention_hours` INHERITS the body window; `uploads_retention_days` has a + # ge=1 floor so 0 is unrepresentable) and those whose `requires_setting` is unmet — with no + # [logging].log_dir there is nothing for the app-log sweep to sweep. + still_unbounded = _unbounded_windows(settings) + refusable = [w for w in still_unbounded if w.auto_bound_days is not None] + warn_only = [w for w in still_unbounded if w.auto_bound_days is None] + + if warn_only: + # Classified and warned, never refused. Naming the tier AND its protection level is the + # point: an operator who sees "PL-1" knows a full body is involved. + print( + "warning: these classified PHI tiers have no retention window on a PHI instance " + f"({env_name!r}) and will accumulate without bound: " + + ", ".join(f"{w.setting} ({w.level})" for w in warn_only) + + ". They are deliberately NOT defaulted — each keys on a timestamp that only moves on " + "a write, so a silent default would delete data still in use (ASVS 14.2.7).", + file=sys.stderr, ) - if days <= 0 - ] - if unbounded_windows: - windows_desc = ", ".join(_RETENTION_WINDOW_LABEL[field] for field in unbounded_windows) + + if refusable: + windows_desc = ", ".join(w.setting for w in refusable) if not settings.retention.allow_unbounded_phi: if enforcing: print( - f"error: no data-retention window is configured for {windows_desc} on a " - f"{'production ' if production else ''}PHI instance ({env_name!r}); refusing to " - "start — PHI message bodies would be retained indefinitely (unbounded PHI at " - "rest, ASVS 14.2.4). Set the window(s) to a positive number of days (e.g. 30) to " - "bound PHI at rest; or, to deliberately retain forever, set " + f"error: a data-retention window is explicitly disabled for {windows_desc} on " + f"a {'production ' if production else ''}PHI instance ({env_name!r}); refusing " + "to start — PHI message bodies would be retained indefinitely (unbounded PHI " + "at rest, ASVS 14.2.4/14.2.7). Set the window(s) to a positive number of days " + "(e.g. 30); or, to deliberately retain forever, set " "[security].allow_keeping_phi_indefinitely=true (audited).", file=sys.stderr, ) diff --git a/messagefoundry/config/retention_classification.py b/messagefoundry/config/retention_classification.py new file mode 100644 index 00000000..8b5fe28a --- /dev/null +++ b/messagefoundry/config/retention_classification.py @@ -0,0 +1,205 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The PHI retention classification the startup gate is generated from (ASVS 14.2.7). + +**Why a constant rather than a hand-typed tuple in `__main__.py`.** This cell broke once because a new +PHI tier landed and nobody widened a literal in the serve gate. A wider literal with no binding to the +classification is the same defect with more characters. So the gate's tier list is built from +:data:`PHI_RETENTION_WINDOWS`, and `tests/test_retention_classification_drift.py` asserts — in BOTH +directions — that this tuple and `docs/PHI.md` §2's Retention column describe the same set. Add a PHI +tier to the doc without adding it here, or here without the doc, and the suite reds. + +**Why the doc is the co-authority and not just prose.** `docs/PHI.md` is git-TRACKED, so a +PHI.md-anchored test actually runs in CI. (`docs/security/` is git-ignored and its doc-anchored tests +take module-level skips — a distinction that has already produced one guard which red locally and +passed in CI.) That single fact is what makes the two-way equality real rather than decorative. + +**What is deliberately NOT here.** `[retention].audit_days` is reserved and unenforced by design — +there is no `purge_audit*` on the Store protocol, and the keep-forever rationale rests on the +audit-retention requirement itself (see `docs/PHI.md` §8), not on chain-breakage. Tiers classified +`UNBOUNDED — honest gap` in §2 are absent for the same reason: this tuple describes windows that +EXIST, and inventing an entry for a tier nothing purges would be the false-coverage claim the whole +column was built to prevent. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Final + + +@dataclass(frozen=True, slots=True) +class RetentionWindow: + """One classified PHI tier and the window that bounds it.""" + + #: The operator-facing setting, spelled exactly as it appears in config and in PHI.md §2. + setting: str + #: The attribute the serve gate reads. + field: str + #: The `[section]` whose settings model actually HOLDS :attr:`field`. Usually the same section as + #: :attr:`setting` — but not always, and the exception is load-bearing: ADR 0118 moved the + #: message-body window to the operator-facing `[security].delete_message_bodies_after_days` while + #: the gate still reads `retention.messages_days` after desugaring. Modelling the two separately is + #: what lets a test resolve every field against its real model instead of exempting the ones that + #: do not fit — and an exemption is how a field name silently stops being checked. + reads_from: str + #: Protection level from PHI.md §2 — PL-1 (body) or PL-2 (identifier/fragment). + level: str + #: Auto-bound to this many days when UNSET on a PHI instance, or ``None`` to leave it alone. + #: + #: ``None`` is a deliberate per-window judgement, NOT an oversight. Auto-bounding a window whose + #: timestamp only moves on a WRITE silently deletes live operational data: `state.set_at` is never + #: refreshed by `state_get`, so a Handler's MRN→surrogate crosswalk would vanish mid-stream and the + #: next message would transform WRONGLY, post-ACK, with no ERROR disposition. `search_presets` is + #: the same shape and `retention.py` forbids exactly that inheritance in prose. These are + #: classified and WARNED, never silently bounded (owner ruling, 2026-07-30). + auto_bound_days: int | None + #: Another setting whose presence this window depends on; the gate skips it when unset. + requires_setting: tuple[str, str] | None = None + #: Whether ``0`` on this window actually means UNBOUNDED. Not universal, and the exceptions are + #: exactly the ones a `days <= 0` predicate would get wrong: + #: + #: * ``[retention].connection_event_retention_hours`` — ``0`` means **INHERIT** the message-body + #: window, so an unset value is already bounded transitively. Flagging it would refuse or warn + #: over a window that is doing its job. + #: * ``[store].uploads_retention_days`` — carries a ``ge=1`` floor, so ``0`` is unrepresentable and + #: the clause would be unfireable by construction. Kept classified, never tested. + zero_is_unbounded: bool = True + + +#: Every PL-1/PL-2 tier that HAS a window, keyed to the classification in `docs/PHI.md` §2. +#: +#: Ordered as §2 orders them, so a reviewer can read the two side by side. +PHI_RETENTION_WINDOWS: Final[tuple[RetentionWindow, ...]] = ( + # PL-1 message bodies — the packet's core intent, and the two the gate already refused on. + RetentionWindow( + setting="[security].delete_message_bodies_after_days", + field="messages_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=30, + ), + RetentionWindow( + setting="[retention].dead_letter_days", + field="dead_letter_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=30, + ), + # PL-2 orphaned reference snapshots (ADR 0006). ORPHAN-ONLY: a still-declared set is never purged + # whatever its age, so this window does NOT bound `reference.value` in general — §2 says so, and a + # plain "rides"/window claim there would be false. + RetentionWindow( + setting="[retention].reference_snapshot_days", + field="reference_snapshot_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=30, + ), + # --- classified and WARNED, never auto-bounded (see auto_bound_days above) --------------------- + RetentionWindow( + setting="[retention].state_max_age_days", + field="state_max_age_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + ), + RetentionWindow( + setting="[retention].search_preset_days", + field="search_preset_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + ), + # PL-1 only because redaction is best-effort — a lone identifier can survive it. Gated on a + # log_dir: with none configured the sweep has nothing to sweep, so refusing over it would refuse + # over a knob that cannot do anything. + RetentionWindow( + setting="[retention].app_log_days", + field="app_log_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=None, + requires_setting=("logging", "log_dir"), + ), + # PL-1 operator-uploaded diagnostic files. Already defaults to 30 with a `ge=1` floor, so it can + # never appear unbounded — kept here so the generated list matches §2 rather than quietly omitting + # a classified tier because it happens to be safe today. + RetentionWindow( + setting="[store].uploads_retention_days", + field="uploads_retention_days", + reads_from="[store]", + level="PL-1", + auto_bound_days=None, + zero_is_unbounded=False, + ), + # PL-2 `connection_event.reason`. NOT a refuse/auto-bound candidate, and the reason is a genuine + # trap: here `0` means INHERIT the message-body window, not keep-forever (settings.py, and + # retention.py resolves it that way). So an unset value is already bounded transitively, and a + # `days <= 0` predicate would refuse a start over a window that is doing its job. + RetentionWindow( + setting="[retention].connection_event_retention_hours", + field="connection_event_retention_hours", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + zero_is_unbounded=False, + ), + # PL-1 `.mfbak` archives, which on SQLite carry FULL inbound+outbound bodies. The odd one out: it + # is bounded by a COUNT (keep-N), not an age window, and its default of 7 already bounds it — the + # only way to reach unbounded is an operator explicitly typing 0, which is a deliberate choice + # (plausibly a DR requirement). So: classified and warned, never auto-bounded and never refused, + # and it only applies when `[backup].destination` is configured — with no destination there are no + # archives to bound. Owner ruling, 2026-07-30. + RetentionWindow( + setting="[backup].retention_keep", + field="retention_keep", + reads_from="[backup]", + level="PL-1", + auto_bound_days=None, + requires_setting=("backup", "destination"), + ), +) + +#: Floor for the derived list. `if not PHI_RETENTION_WINDOWS` is NOT sufficient: a bad merge dropping +#: most of the entries leaves a non-empty tuple, and a startup gate would then check two windows while +#: reporting success. +#: +#: NINE, and the number was corrected by its own drift test rather than by counting. It was first +#: written as 7 — the count I derived by hand from the classification. The two-way equality against +#: docs/PHI.md §2 immediately reported `[backup].retention_keep` and +#: `[retention].connection_event_retention_hours` as documented-but-absent. That is precisely the +#: failure this floor exists to catch, caught in the constant it protects, before either had ever run. +MIN_PHI_RETENTION_WINDOWS: Final[int] = 9 + + +def auto_bounded_windows() -> tuple[RetentionWindow, ...]: + """The windows a PHI instance silently defaults to 30 days when they are UNSET.""" + return tuple(w for w in PHI_RETENTION_WINDOWS if w.auto_bound_days is not None) + + +def warn_only_windows() -> tuple[RetentionWindow, ...]: + """The windows that are classified and WARNED but never silently bounded.""" + return tuple(w for w in PHI_RETENTION_WINDOWS if w.auto_bound_days is None) + + +def unbounded_windows(read: object) -> tuple[RetentionWindow, ...]: + """Windows whose configured value is genuinely unbounded on ``read`` (a loaded ``Settings``). + + Skips windows where ``0`` does not mean unbounded, and windows whose ``requires_setting`` + dependency is unmet — with no ``[logging].log_dir`` there is nothing for the app-log sweep to + sweep, so refusing or warning over it would fire on a knob that cannot act. + """ + out: list[RetentionWindow] = [] + for window in PHI_RETENTION_WINDOWS: + if not window.zero_is_unbounded: + continue + if window.requires_setting is not None: + section, dep = window.requires_setting + if not getattr(getattr(read, section, None), dep, None): + continue + section_obj = getattr(read, window.reads_from.strip("[]"), None) + value = getattr(section_obj, window.field, None) + if isinstance(value, int) and value <= 0: + out.append(window) + return tuple(out) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index d32764a0..acd0e132 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -1518,6 +1518,18 @@ class RetentionSettings(_Section): # in-memory state cache + table bounded. A simple global age purge; per-namespace policy is a # follow-up. 0 = keep forever (the default — state correlation data is opt-in to purge). state_max_age_days: int = 0 + # Past N days, DELETE ORPHANED reference snapshots (ADR 0006) — the rows of a set that config no + # longer declares, whose active version was synced before the cutoff. `reference.value` is PL-2 + # (PHI.md §2) and a snapshot row can be patient-keyed, but until ASVS 14.2.7 it had NO purge path at + # all: the only thing that ever replaced a snapshot was the next sync's build-new-then-flip, which + # never comes for a set nobody declares any more. 0 = keep forever. + # + # ORPHAN-SCOPED, and the limit is the honest part: a set that IS still declared is never touched + # however old its `synced_at`, because its snapshot is live data the engine serves. So the normal + # case — a wired set holding live PHI — remains purged by nothing. That is a stated residual, not a + # closed cell; do not let a classification table describe this window as covering `reference.value` + # generally, which would machine-bless a false claim. + reference_snapshot_days: int = 0 # Past N HOURS, DELETE connection_event rows (#46) — the Corepoint-style transport/lifecycle log can # be high-volume (a connect-per-message sender, a probe storm), so it has its own short window in # HOURS (not days). 0 = inherit the message-body window (messages_days), the ADR 0021 §7.5 default. @@ -1600,6 +1612,7 @@ class RetentionSettings(_Section): "app_log_days", "app_log_compress_days", "search_preset_days", + "reference_snapshot_days", ) @classmethod def _non_negative_days(cls, value: int) -> int: diff --git a/messagefoundry/pipeline/retention.py b/messagefoundry/pipeline/retention.py index c08c834a..37df3b56 100644 --- a/messagefoundry/pipeline/retention.py +++ b/messagefoundry/pipeline/retention.py @@ -177,6 +177,10 @@ class RetentionPass: # the `search_preset_days` window. Metadata only — the count, never a needle. 0 when the window is # unset (the default), so a deployment that doesn't use it has a byte-identical audit detail. search_presets_purged: int = 0 + # Orphaned reference-snapshot ROWS deleted (ADR 0006, ASVS 14.2.7) — sets config no longer + # declares. Metadata only: a count, never a key or a value. 0 when the window is unset, when no + # registry is wired, or when the registry declares no reference sets (the positive-signal skip). + reference_snapshots_purged: int = 0 # This pass hit the `max_pass_seconds` between-phase duration cap (#121, ADR 0137) and SKIPPED its # remaining phases — the skipped tail (incl. any due WAL-checkpoint/VACUUM) re-runs next interval with # its last-run marker unadvanced. Always False when the cap is off (`max_pass_seconds<=0`, the @@ -201,6 +205,7 @@ def did_work(self) -> bool: or self.app_logs_deleted > 0 or self.app_logs_compressed > 0 or self.search_presets_purged > 0 + or self.reference_snapshots_purged > 0 or self.vacuumed or self.over_limit or self.capped @@ -266,6 +271,7 @@ def enabled(self) -> bool: or s.state_max_age_days or s.connection_event_retention_hours or s.search_preset_days + or s.reference_snapshot_days or s.max_db_mb or s.wal_checkpoint_seconds or s.vacuum_time() is not None @@ -503,6 +509,43 @@ def _deadline_hit() -> bool: older_than=now - s.search_preset_days * _SECONDS_PER_DAY, now=now ) + # Orphaned reference snapshots (ADR 0006, ASVS 14.2.7): `reference.value` is PL-2 and had NO + # purge path at all — a set dropped from config kept its decryptable rows forever, because the + # only thing that ever replaced them was the next sync's build-new-then-flip, which never comes + # for a set nobody declares. + # + # THE GUARD IS POSITIVE-SIGNAL, and that is the whole design. `declared` is the keep-set, so an + # EMPTY one reads as "every set is abandoned" and would purge the entire store. `registry is + # None` does not cover it: a registry that LOADS FINE while declaring zero reference sets — a + # subset `--config`, a per-team config split, a harness-redirect run pointed at the real DB — + # yields `references == {}`. Absence-based guards fail open by construction, so this one + # requires a POSITIVE signal: at least one declared set, or the phase does not run at all. A + # registry declaring zero reference sets can never authorize purging any. + # + # The store ALSO raises on an empty `declared` rather than trusting this check — belt and + # braces, because the cost of one refactor dropping this line is every snapshot in the store. + # + # Worse without it: ReferenceSyncRunner deliberately does NOT advance `synced_at` when a source + # fetch fails, so the rows most likely to look "stale" belong to a still-wired set whose source + # is merely down — precisely the last-good copy an operator would want kept. + reference_snapshots_purged = 0 + if not _deadline_hit() and s.reference_snapshot_days > 0: + registry = self._registry_source() if self._registry_source is not None else None + declared = frozenset(registry.references) if registry is not None else frozenset() + if declared: + reference_snapshots_purged = await self._store.purge_reference_snapshots( + older_than=now - s.reference_snapshot_days * _SECONDS_PER_DAY, + declared=declared, + now=now, + ) + else: + log.warning( + "reference-snapshot retention skipped: the loaded registry declares no reference " + "sets, so there is no keep-set and a purge would delete every snapshot in the " + "store. Set [retention].reference_snapshot_days=0 to silence this, or check that " + "--config points at the configuration this store belongs to." + ) + # Application log-file retention (#120): delete app-log FILES older than `app_log_days` from # `[logging].log_dir`. Filesystem I/O (scandir/stat/unlink is blocking), so it runs off the # event loop; metadata only (mtime) — file content is never read (no PHI). A no-op unless the @@ -570,6 +613,7 @@ def _deadline_hit() -> bool: app_logs_compressed=app_logs_compressed, app_log_bytes_reclaimed=app_log_bytes_reclaimed, search_presets_purged=search_presets_purged, + reference_snapshots_purged=reference_snapshots_purged, capped=capped, ) if result.did_work: @@ -979,6 +1023,12 @@ async def _audit(self, result: RetentionPass) -> None: # only — a preset's criteria is a PHI-shaped needle and never enters the audit detail. "search_preset_days": self._settings.search_preset_days, "search_presets_purged": result.search_presets_purged, + # Orphaned reference-snapshot ROWS deleted (ADR 0006, ASVS 14.2.7) + the window. The + # COUNT only — never a set name, key or value. A set name is operator-authored config + # rather than PHI, but the ROWS are PL-2 and naming the set in a durable audit row would + # narrow which patient cohort was dropped; the count is what an assessor needs. + "reference_snapshot_days": self._settings.reference_snapshot_days, + "reference_snapshots_purged": result.reference_snapshots_purged, # Between-phase duration cap (#121, ADR 0137): the configured ceiling + whether THIS pass # hit it and skipped its remaining phases. Timing metadata only — no PHI. "max_pass_seconds": self._settings.max_pass_seconds, diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index f7baa117..27cf2cd1 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -25,7 +25,7 @@ import asyncio import logging -from collections.abc import AsyncIterator, Iterable, Mapping, Sequence +from collections.abc import AsyncIterator, Collection, Iterable, Mapping, Sequence from pathlib import Path from typing import Any, Protocol, runtime_checkable @@ -1309,6 +1309,45 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N unless ``[retention].search_preset_days`` is set.""" ... + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + """Delete ORPHANED reference snapshots — sets no longer declared in config — synced before + ``older_than``. Returns the number of ``reference`` rows deleted. + + ``reference.value`` is a versioned lookup snapshot (ADR 0006) and can hold patient-keyed rows, + so PHI.md §2 classifies it **PL-2**. Before ASVS 14.2.7 it had **no purge path at all**: a set + dropped from config left its fully-decryptable snapshot in the store indefinitely, replaced only + by the next sync's build-new-then-flip — which never comes for a set nobody declares any more. + + **Orphan-scoped by design, and that limit must be stated rather than glossed.** A set that IS + declared is never touched however old its ``synced_at``, because its snapshot is live data the + engine serves. So the normal case — a wired set holding live PHI — is still purged by nothing. + That is an honest residual, not a closed cell; do not let a classification table describe this as + `rides `, which would machine-bless a false claim. + + **``declared`` must be non-empty and implementations MUST reject an empty one.** An empty + collection reads as "every set is abandoned", so a caller that loaded a registry declaring zero + reference sets — a subset ``--config``, a per-team split, a harness redirect pointed at the real + DB — would wipe every snapshot in the store. Absence-based guards fail open by construction, so + this one is positive-signal: an empty ``declared`` is a programming error, not an instruction. + Worse, ``ReferenceSyncRunner`` deliberately does **not** advance ``synced_at`` when a source + fetch fails, so the victim would be precisely the last-good snapshot of a still-wired set whose + source is merely down. + + **Eligibility must be re-asserted INSIDE the delete statement**, not read first and trusted. The + caller computes ``declared`` outside any store lock, so a config reload can commit a fresh + patient-keyed snapshot between the decision and the delete; a purge that then fires deletes live + data and the set goes present-but-empty, returning ``None`` from ``reference(name).get(k)`` + silently. Do not assume any backend's own locking closes this — the race is at the caller level + on all three. + + **The ``reference_version`` pointer row SURVIVES.** Deleting it is invisible to + :meth:`converge_reference_cache`, which only adds/updates names present in a fresh read, so a + cluster follower would keep serving the purged PHI from RAM forever. Keeping the pointer costs a + row and makes the set read as present-but-empty; that trade is deliberate and recorded.""" + ... + async def wal_checkpoint(self) -> None: ... async def vacuum(self) -> None: ... diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 286ad007..2a03b502 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -55,7 +55,7 @@ import os import socket import time -from collections.abc import AsyncIterator, Iterable, Mapping, Sequence +from collections.abc import AsyncIterator, Collection, Iterable, Mapping, Sequence from contextlib import asynccontextmanager from time import perf_counter from types import MappingProxyType @@ -6809,6 +6809,57 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N ) return _rowcount(result) + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # See Store.purge_reference_snapshots for the full contract. ADR 0006 `reference.value` is PL-2 + # and had no purge path at all before ASVS 14.2.7. Orphan-scoped: a DECLARED set is never + # touched however old its synced_at. + if not declared: + # Positive-signal guard — an empty `declared` reads as "every set is abandoned" and would + # wipe the store. Never a legitimate instruction, so it raises rather than deleting. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + rows = await self._pool.fetch( + "SELECT name FROM reference_version WHERE synced_at < $1", older_than + ) + candidates = [r["name"] for r in rows if r["name"] not in declared] + deleted = 0 + for name in candidates: + # Eligibility RE-ASSERTED inside the delete. `declared` is computed by the caller outside + # any store lock, so a config reload can land a fresh patient-keyed snapshot between the + # SELECT above and this statement. Postgres has no equivalent of SQLite's process-wide + # `self._lock`, so this predicate is the ONLY thing standing between a routine reload and + # deleting live data — do not "simplify" it away because the SELECT already filtered. + result = await self._pool.execute( + "DELETE FROM reference WHERE name = $1" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = $1 AND v.synced_at < $2)", + name, + older_than, + ) + count = _rowcount(result) + if not count: + continue # a concurrent re-sync won the race — leave it alone + deleted += count + # KEEP the pointer, BUMP the version. converge_reference_cache only reloads a set whose + # active version DIFFERS from the one this handle reflects, so leaving the version alone + # would let every follower keep serving the purged PHI out of `_reference_cache` until the + # process restarts — and deleting the pointer outright is worse, because converge only ever + # adds/updates names present in a fresh read and would never notice. Bumping routes the + # deletion through the convergence path that already exists. + await self._pool.execute( + "UPDATE reference_version SET row_count = 0, version = 'purged:' || version" + " WHERE name = $1 AND version NOT LIKE 'purged:%'", + name, + ) + self._reference_cache.pop(name, None) + self._reference_versions.pop(name, None) + return deleted + async def purge_dead_letters( self, *, diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 5e32f145..181431f3 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -38,7 +38,15 @@ import logging import queue import time -from collections.abc import AsyncIterator, Callable, Iterable, Iterator, Mapping, Sequence +from collections.abc import ( + AsyncIterator, + Callable, + Collection, + Iterable, + Iterator, + Mapping, + Sequence, +) from contextlib import AsyncExitStack, asynccontextmanager, contextmanager from time import perf_counter from types import MappingProxyType @@ -1101,22 +1109,13 @@ def __init__(self, conn: Any, cur: Any) -> None: CREATE INDEX ix_messages_channel ON messages(channel_id, received_at)""", """IF INDEXPROPERTY(OBJECT_ID('messages'),'ix_messages_control','IndexID') IS NULL CREATE INDEX ix_messages_control ON messages(channel_id, control_id)""", - """IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox ( - id NVARCHAR(64) NOT NULL PRIMARY KEY, message_id NVARCHAR(64) NOT NULL, - channel_id NVARCHAR(256) NOT NULL, destination_name NVARCHAR(256) NOT NULL, - payload NVARCHAR(MAX) NOT NULL, status NVARCHAR(32) NOT NULL, - attempts INT NOT NULL DEFAULT 0, next_attempt_at FLOAT NOT NULL, last_error NVARCHAR(MAX) NULL, - created_at FLOAT NOT NULL, updated_at FLOAT NOT NULL)""", - """IF INDEXPROPERTY(OBJECT_ID('outbox'),'ix_outbox_ready','IndexID') IS NULL - CREATE INDEX ix_outbox_ready ON outbox(status, next_attempt_at)""", - """IF INDEXPROPERTY(OBJECT_ID('outbox'),'ix_outbox_message','IndexID') IS NULL - CREATE INDEX ix_outbox_message ON outbox(message_id)""", # Unified staged queue (ADR 0001) — ingress -> routed -> outbound, one row per stage-unit with a - # `stage` discriminator. The SQL Server backend originally shipped only the flat `outbox`; the - # staged pipeline (enqueue_ingress + the handoffs) and ALL delivery-side methods now read/write - # this table (outbox is retained only for legacy read-compat). `seq` IDENTITY is the FIFO - # insertion-order tiebreak (PG uses BIGSERIAL); owner/lease_expires_at are present for parity but - # written NULL on this single-node backend (reset_stale_inflight is the recovery path). + # `stage` discriminator. The SQL Server backend originally shipped a flat `outbox` table; it was + # RECREATED by this very `_SCHEMA` on every open and read by nothing (the staged pipeline and every + # delivery-side method read/write `queue`), so any legacy PHI body left in it was retained forever + # with no purge on any backend — see the migrate-and-drop below, and docs/PHI.md §2. `seq` IDENTITY + # is the FIFO insertion-order tiebreak (PG uses BIGSERIAL); owner/lease_expires_at are present for + # parity but written NULL on this single-node backend (reset_stale_inflight is the recovery path). """IF OBJECT_ID('queue','U') IS NULL CREATE TABLE queue ( id NVARCHAR(64) NOT NULL PRIMARY KEY, seq BIGINT IDENTITY(1,1) NOT NULL, message_id NVARCHAR(64) NOT NULL, stage NVARCHAR(16) NOT NULL, @@ -1177,6 +1176,54 @@ def __init__(self, conn: Any, cur: Any) -> None: # Sch-M lock on the hot queue table (review). lock_escalation 2 = DISABLE. """IF (SELECT lock_escalation FROM sys.tables WHERE object_id=OBJECT_ID('queue')) <> 2 ALTER TABLE queue SET (LOCK_ESCALATION = DISABLE)""", + # ASVS 14.2.7 — fold the legacy flat `outbox` into `queue` (stage=outbound) and DROP it. + # + # WHY THIS IS A PHI FIX AND NOT A TIDY-UP. The table was recreated on every open and read by + # nothing, so `outbox.payload` — a FULL transformed PHI body — was **purged by nothing on any + # backend**: `purge_old_messages` and `purge_dead_letters` both scope to `queue`. A store upgraded + # from the pre-staged-pipeline layout therefore kept every legacy body forever while `messages.raw` + # blanked on its window, so the message READ as purged. Migrating the rows in puts them under + # `[security].delete_message_bodies_after_days` / `[retention].dead_letter_days` like any other + # outbound row; dropping the table is what makes the retirement true rather than merely documented. + # + # WHY IT LIVES IN `_SCHEMA` AND NOT AN OPEN-PATH METHOD (the SQLite backend uses a method, + # `_migrate_outbox_to_queue`): on this backend `_schema_hash()` is the ONLY thing that decides + # whether the DDL batch runs at all — a current marker skips it entirely — so migration code + # outside `_SCHEMA` would be invisible to that decision. See `_schema_hash`'s docstring. + # + # WHY IT IS PLACED HERE, after the `queue` DDL rather than where the `outbox` DDL used to be: + # SQL Server defers name resolution, so an earlier placement would PARSE fine and then fail at RUN + # time against a legacy DB old enough to have `outbox` but not yet `queue` — a failed open, which + # under the batch's single transaction is a startup crash-loop, not a degraded start. + # + # The two filters are each load-bearing: + # * EXISTS(messages) — `fk_queue_message` is enforced, and an orphan row (a `message_id` with no + # surviving message) would abort the whole batch with an opaque FK error. It was unreplayable + # anyway: nothing can view or route it. Same call the SQLite migration makes. + # * NOT EXISTS(queue) — `queue.id` is the PK. Ids are uuid4 so a collision is not a real + # expectation, but the cost of being wrong is a 2627 that rolls back the batch and re-fails on + # every restart. A cheap anti-join buys immunity from that, and also makes a partially-applied + # migration (rolled back after the INSERT, before the marker) safely re-runnable. + # ORDER BY created_at feeds the `seq` IDENTITY in arrival order so a legacy backlog drains FIFO + # (ADR 0059 orders a lane by `seq` alone). MEASURED, not assumed: against SQL Server 2022 CU25, + # rows INSERTed into `outbox` in the exact reverse of their created_at order came out of the + # migration with `seq` ascending by created_at. It is still not a documented guarantee — a parallel + # plan over a large backlog may assign otherwise — so it is deliberately not asserted by a test + # that would then be flaky. Each row's own created_at is carried over verbatim either way, so a + # reorder costs ordering, never data. + f"""IF OBJECT_ID('outbox','U') IS NOT NULL + BEGIN + INSERT INTO queue (id, message_id, stage, channel_id, destination_name, payload, + status, attempts, next_attempt_at, last_error, created_at, updated_at) + SELECT o.id, o.message_id, '{Stage.OUTBOUND.value}', o.channel_id, o.destination_name, + o.payload, o.status, o.attempts, o.next_attempt_at, o.last_error, + o.created_at, o.updated_at + FROM outbox o + WHERE EXISTS (SELECT 1 FROM messages m WHERE m.id = o.message_id) + AND NOT EXISTS (SELECT 1 FROM queue q WHERE q.id = o.id) + ORDER BY o.created_at, o.id; + DROP TABLE outbox; + END""", # #47/ADR 0042: messages.documents_pruned (the "embedded doc evicted vs never present" flag). NULL on # existing rows = never pruned; COL_LENGTH-gated like the others so a re-open is a no-op. """IF COL_LENGTH('messages','documents_pruned') IS NULL @@ -2424,10 +2471,16 @@ async def _encrypt_existing_rows(self) -> None: # recognised as already-encrypted and skipped — never re-wrapped. like = f"{_ENC_MARKER_PREFIX}%" total = 0 + # `("outbox", "payload")` was here until the legacy table was migrated + DROPped (ASVS + # 14.2.7). Leaving it would fail this pass with *Invalid object name 'outbox'* on EVERY keyed + # open — and the keyed path does not run in CI (every SQL Server leg is keyless), so + # `tests/test_sqlserver_encrypt_pass_tables.py` is the only mechanism that can catch a stale + # entry here. Rows the migration folded in are covered by `("queue", "payload")` below: the + # migration carries the payload over VERBATIM, so a legacy plaintext body arrives unencrypted + # and this pass — which runs after `_ensure_schema` — seals it on the same open. for table, column in ( ("messages", "raw"), ("queue", "payload"), - ("outbox", "payload"), ( "users", "totp_secret", @@ -6008,6 +6061,66 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N raise return int(purged) if purged is not None else 0 + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # See Store.purge_reference_snapshots for the full contract. ADR 0006 `reference.value` is PL-2 + # and had no purge path at all before ASVS 14.2.7. Orphan-scoped: a DECLARED set is never + # touched however old its synced_at. + if not declared: + # Positive-signal guard — an empty `declared` reads as "every set is abandoned" and would + # wipe the store. Never a legitimate instruction, so it raises rather than deleting. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + deleted = 0 + purged_names: list[str] = [] + async with self._acquire() as conn, self._cursor(conn) as cur: + try: + await cur.execute( + "SELECT name FROM reference_version WHERE synced_at < ?", (older_than,) + ) + candidates = [r[0] for r in await cur.fetchall() if r[0] not in declared] + for name in candidates: + # Eligibility RE-ASSERTED inside the delete. `declared` is computed by the caller + # outside any lock and write_reference_snapshot takes no applock here, so a config + # reload can commit a fresh patient-keyed snapshot between the SELECT above and + # this statement. This predicate is the only thing preventing a routine reload from + # destroying live data. `older_than` is bound TWICE (T-SQL has no named params). + await cur.execute( + "DELETE FROM reference WHERE name = ?" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = ? AND v.synced_at < ?)", + (name, name, older_than), + ) + count = int(cur.rowcount or 0) + if not count: + continue # a concurrent re-sync won the race — leave it alone + deleted += count + # KEEP the pointer, BUMP the version — converge_reference_cache only reloads a set + # whose active version DIFFERS, so an unchanged version leaves every follower + # serving purged PHI from `_reference_cache` until restart, and deleting the + # pointer is worse (converge only adds/updates names present in a fresh read). + await cur.execute( + "UPDATE reference_version SET row_count = 0," + " version = 'purged:' + version" + " WHERE name = ? AND version NOT LIKE 'purged:%'", + (name,), + ) + purged_names.append(name) + await self._commit(conn) + except Exception: + await conn.rollback() + raise + # Evict only AFTER the commit — a rolled-back purge must not leave the cache claiming rows the + # store still holds (the same post-commit discipline purge_state follows below). + for name in purged_names: + self._reference_cache.pop(name, None) + self._reference_versions.pop(name, None) + return deleted + async def purge_state(self, *, older_than: float, now: float | None = None) -> int: """Delete transform-state rows last set before ``older_than`` (ADR 0005 retention), evicting them from the read-through cache post-commit. Returns the number deleted.""" diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 1699a29c..0ed1888e 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -44,7 +44,15 @@ import stat import subprocess import time -from collections.abc import AsyncIterator, Awaitable, Callable, Iterable, Mapping, Sequence +from collections.abc import ( + AsyncIterator, + Awaitable, + Callable, + Collection, + Iterable, + Mapping, + Sequence, +) from contextlib import asynccontextmanager from dataclasses import dataclass, field from enum import Enum @@ -8701,6 +8709,66 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N await self._commit() return int(cur.rowcount) + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # ADR 0006 reference snapshots: `value` is PL-2 (PHI.md §2) and until ASVS 14.2.7 had NO purge + # path at all — a set dropped from config kept its decryptable rows forever, because the only + # thing that ever replaced them was the next sync's build-new-then-flip, which never comes. + # + # Orphan-scoped: a DECLARED set is never touched however old its synced_at. Scoping it any wider + # would delete live data the engine is serving. + if not declared: + # Positive-signal guard. An empty `declared` reads as "every set is abandoned", so a caller + # holding a registry that declares zero reference sets — a subset --config, a per-team + # split, a harness redirect aimed at the real DB — would wipe the store. That is never a + # legitimate instruction, so it is an error rather than a very destructive no-op. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + async with self._lock: + cur = await self._db.execute( + "SELECT name FROM reference_version WHERE synced_at < ?", (older_than,) + ) + candidates = [r[0] for r in await cur.fetchall() if r[0] not in declared] + deleted = 0 + for name in candidates: + # The eligibility predicate is RE-ASSERTED inside the delete, not read above and + # trusted. `declared` is computed by the caller outside this lock, so a config reload + # can commit a fresh patient-keyed snapshot between the SELECT and here; without the + # EXISTS the purge would delete live rows and the set would go present-but-empty, + # returning None from reference(name).get(k) with no error at all. + cur = await self._db.execute( + "DELETE FROM reference WHERE name = ?" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = ? AND v.synced_at < ?)", + (name, name, older_than), + ) + if not cur.rowcount: + continue # a concurrent re-sync won the race — leave it alone + deleted += int(cur.rowcount) + # KEEP the pointer row, but BUMP its version. Deleting the pointer is invisible to + # converge_reference_cache (it only adds/updates names present in a fresh read), so a + # cluster follower would serve the purged PHI from RAM until restart. Keeping it + # unchanged is just as bad: converge only reloads a set whose version DIFFERS, so an + # unchanged version means the follower's populated cache is never revisited. Bumping it + # makes the deletion propagate through the mechanism that already exists, rather than + # bolting a second removal path onto every backend. + await self._db.execute( + "UPDATE reference_version SET row_count = 0, version = 'purged:' || version" + " WHERE name = ? AND version NOT LIKE 'purged:%'", + (name,), + ) + # SQLite keeps no per-set version map (it is single-node and the sole writer, which is + # why converge_reference_cache is a no-op here) — evicting the read-through cache is + # the whole of the local convergence story. The server backends, which DO track + # versions, rely on the bumped pointer above. + self._reference_cache.pop(name, None) + await self._commit() + return deleted + async def purge_dead_letters( self, *, diff --git a/tests/test_adr0071_dispatch_wiring_sqlserver.py b/tests/test_adr0071_dispatch_wiring_sqlserver.py index fa3d445f..26df7299 100644 --- a/tests/test_adr0071_dispatch_wiring_sqlserver.py +++ b/tests/test_adr0071_dispatch_wiring_sqlserver.py @@ -78,7 +78,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0071_fused_callables_sqlserver.py b/tests/test_adr0071_fused_callables_sqlserver.py index 0039385a..cf11c300 100644 --- a/tests/test_adr0071_fused_callables_sqlserver.py +++ b/tests/test_adr0071_fused_callables_sqlserver.py @@ -62,7 +62,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0075_batch_sqlserver.py b/tests/test_adr0075_batch_sqlserver.py index 27803fa9..4689abf2 100644 --- a/tests/test_adr0075_batch_sqlserver.py +++ b/tests/test_adr0075_batch_sqlserver.py @@ -63,7 +63,6 @@ async def _clear(store: Any) -> None: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0114_claim_proc_live.py b/tests/test_adr0114_claim_proc_live.py index af7cca05..21c3dfc2 100644 --- a/tests/test_adr0114_claim_proc_live.py +++ b/tests/test_adr0114_claim_proc_live.py @@ -65,7 +65,7 @@ async def _open(**flag_overrides: Any) -> SqlServerStore: async def _clean(store: SqlServerStore) -> None: # The canonical live-suite cleanup order (test_claim_fifo_heads._open_sqlserver): children # before messages (response FKs it), so no 547 on a shared live DB. - for table in ("message_events", "queue", "response", "delivered_keys", "outbox", "messages"): + for table in ("message_events", "queue", "response", "delivered_keys", "messages"): await store._execute(f"DELETE FROM {table}") # noqa: S608 - test cleanup, fixed names diff --git a/tests/test_batch_claim_fifo.py b/tests/test_batch_claim_fifo.py index 8da17274..a5ea71d8 100644 --- a/tests/test_batch_claim_fifo.py +++ b/tests/test_batch_claim_fifo.py @@ -53,7 +53,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_batch_claim_locking.py b/tests/test_batch_claim_locking.py index efb98ad0..94c49cf8 100644 --- a/tests/test_batch_claim_locking.py +++ b/tests/test_batch_claim_locking.py @@ -88,7 +88,6 @@ async def ss_store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_batch_completion.py b/tests/test_batch_completion.py index b5b6ad1a..dea5232a 100644 --- a/tests/test_batch_completion.py +++ b/tests/test_batch_completion.py @@ -29,7 +29,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_claim_fifo_heads.py b/tests/test_claim_fifo_heads.py index 8fd0850d..66e9062d 100644 --- a/tests/test_claim_fifo_heads.py +++ b/tests/test_claim_fifo_heads.py @@ -61,7 +61,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_cli.py b/tests/test_cli.py index 12b3ec97..84a4e27d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1367,17 +1367,47 @@ def test_serve_ui_default_on_public_origin_degrades_json_only( # --- #186a secure-by-default data retention (bounded PHI-body windows) ---------------------------- -def test_serve_refuses_unbounded_messages_retention_in_prod( +def test_serve_auto_bounds_an_unset_body_window_in_prod( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: - # A production PHI instance with the inbound-body window unbounded refuses to start — PHI bodies - # would accumulate forever. Egress + SMTP pre-satisfied so only the retention gate decides. - rc, _ = _run_secure_serve( + """UNSET now DEFAULTS to 30 on the enforce dial rather than refusing (owner ruling 2026-07-30). + + This test previously asserted rc == 2 for exactly this config. The change is deliberate and the + trade is real: a production PHI instance with an unset window used to refuse to start, which forced + an operator to choose a number; it now starts bounded at 30. What survives is the fail-closed path + for an EXPLICIT 0 — see the refusal test below, which is the other half of this pair. + + Mutation: drop the auto-bound block — reds, because rc becomes 2 and the window stays 0. + """ + rc, captured = _run_secure_serve( tmp_path, monkeypatch, "security.block_unlisted_outbound = true\n[retention]\ndead_letter_days = 30\n" + _SECURE_ALERTS, ) + assert rc == 0 + assert captured["retention_settings"].messages_days == 30 # type: ignore[attr-defined] + err = capsys.readouterr().err + assert "[security].delete_message_bodies_after_days" in err and "defaulted ON (30 days)" in err + + +def test_serve_refuses_an_explicitly_disabled_body_window_in_prod( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """An EXPLICIT 0 still refuses — the fail-closed path the auto-bound narrows but does not remove. + + This is the half of the posture that must not regress: "unbounded by accident" is still prevented, + because typing 0 is a deliberate act and is still refused unless the audited opt-out is set. + + Mutation: treat an explicit 0 as unset (drop `model_fields_set`) — reds, because the window is + silently defaulted to 30 and rc becomes 0. + """ + rc, _ = _run_secure_serve( + tmp_path, + monkeypatch, + "security.block_unlisted_outbound = true\nsecurity.delete_message_bodies_after_days = 0\n" + "[retention]\ndead_letter_days = 30\n" + _SECURE_ALERTS, + ) assert rc == 2 err = capsys.readouterr().err assert "[security].delete_message_bodies_after_days" in err and "refusing to start" in err @@ -1393,7 +1423,7 @@ def test_serve_refuses_unbounded_dead_letter_retention_in_prod( tmp_path, monkeypatch, "security.block_unlisted_outbound = true\nsecurity.delete_message_bodies_after_days = 30\n" - + _SECURE_ALERTS, + "[retention]\ndead_letter_days = 0\n" + _SECURE_ALERTS, ) assert rc == 2 err = capsys.readouterr().err @@ -1418,7 +1448,14 @@ def test_serve_retention_auto_bounds_in_staging( assert retention.messages_days == 30 and retention.dead_letter_days == 30 # type: ignore[attr-defined] err = capsys.readouterr().err assert "defaulted ON (30 days)" in err - assert "accumulate without bound" not in err # auto-bounded, no longer merely warns + # The AUTO-BOUNDED windows must not appear in the warn-only line — that is what "auto-bound rather + # than merely warn" means. This used to assert the phrase was absent ENTIRELY, which worked only + # while the classification held nothing but auto-bounded windows; state_max_age_days and + # search_preset_days now legitimately warn, so the narrow assertion is the one that keeps testing + # the original intent instead of the coincidence. + warn_line = next((ln for ln in err.splitlines() if "accumulate without bound" in ln), "") + assert "[security].delete_message_bodies_after_days" not in warn_line, warn_line + assert "[retention].dead_letter_days" not in warn_line, warn_line assert "refusing to start" not in err diff --git a/tests/test_cluster_failover_postgres.py b/tests/test_cluster_failover_postgres.py index de26d23d..3ac9f184 100644 --- a/tests/test_cluster_failover_postgres.py +++ b/tests/test_cluster_failover_postgres.py @@ -294,7 +294,7 @@ async def _clear_data(store: object) -> None: "SELECT tablename FROM pg_tables WHERE schemaname = ANY (current_schemas(false))" ) existing = {r["tablename"] for r in rows} - targets = [t for t in ("messages", "queue", "outbox", "response") if t in existing] + targets = [t for t in ("messages", "queue", "response") if t in existing] if targets: await conn.execute(f"TRUNCATE {', '.join(targets)} RESTART IDENTITY CASCADE") diff --git a/tests/test_cluster_failover_sqlserver.py b/tests/test_cluster_failover_sqlserver.py index fb1c1b04..925cd092 100644 --- a/tests/test_cluster_failover_sqlserver.py +++ b/tests/test_cluster_failover_sqlserver.py @@ -292,7 +292,7 @@ async def _clear_data(store: object) -> None: """Empty the message/queue data tables (FK order) so the FIFO claim starts from a known single head.""" async with store._pool.acquire() as conn: # type: ignore[attr-defined] cur = await conn.cursor() - for table in ("queue", "response", "outbox", "messages"): + for table in ("queue", "response", "messages"): await cur.execute(f"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}") await conn.commit() diff --git a/tests/test_fifo_index_migration.py b/tests/test_fifo_index_migration.py index fbbea005..36a24a76 100644 --- a/tests/test_fifo_index_migration.py +++ b/tests/test_fifo_index_migration.py @@ -58,7 +58,6 @@ async def _open_sqlserver(_: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_fixture_outbox_reset.py b/tests/test_fixture_outbox_reset.py new file mode 100644 index 00000000..fd5e8e44 --- /dev/null +++ b/tests/test_fixture_outbox_reset.py @@ -0,0 +1,277 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""No test fixture may issue an UNGUARDED ``DELETE FROM outbox`` once the legacy table is retired. + +ASVS 14.2.7 migrates the legacy SQL Server ``outbox`` table into ``queue`` and DROPs it. After that, a +fixture doing:: + + for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + await cur.execute(f"DELETE FROM {table}") + +fails with *Invalid object name 'outbox'* and takes its whole module with it. + +**Why a source scanner rather than "run the tests and see":** of the eighteen files carrying such a +reset list, **nine appear in no SQL Server CI step at all** — `test_adr0071_dispatch_wiring_sqlserver`, +`test_adr0071_fused_callables_sqlserver`, `test_adr0075_batch_sqlserver`, `test_adr0114_claim_proc_live`, +`test_batch_completion`, `test_metadata_bag`, `test_outbound_batch`, `test_shard_recovery_sqlserver`, +`test_sqlserver_sync_handoff`. A mutation planted in any of those **passes vacuously**, so "the SS leg +is green" is not evidence that the reset lists were all updated. This test needs no database and runs +on the plain leg, which is the only leg guaranteed to exist. + +The **guarded** form is fine and stays:: + + await cur.execute(f"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}") + +— it is a no-op once the table is gone. That distinction is structural, not stylistic, which is why this +scanner reads the loop body rather than grepping for the word ``outbox``. + +**The two source-level retirement guards at the bottom live here for the same reason.** They assert +things about `_SCHEMA` and about the engine package that need no database, and the live migration +suite (`tests/test_sqlserver_legacy_outbox_migration.py`) carries a MODULE-level +``skipif(not MEFOR_TEST_SQLSERVER)``. Leaving them there would have gated a pure source check behind a +running SQL Server — the same "runs on a leg that may not exist" trap this module was built to avoid. +""" + +from __future__ import annotations + +import ast +import pathlib + +import pytest + +from messagefoundry.store import Stage + +_TESTS = pathlib.Path(__file__).resolve().parent + +#: Existence-guard markers. A reset whose statement contains one of these degrades to a no-op when the +#: table is absent, so it survives the retirement untouched. +_GUARD_MARKERS = ("OBJECT_ID", "IF EXISTS", "information_schema", "in existing", "to_regclass") + +#: `"outbox"` appears in these for reasons that are not a reset list. Exempted by FILE, with the reason, +#: because a blanket substring skip would also hide a real one landing in the same file later. +_NOT_RESET_LISTS = { + # A Pydantic field-name allow-list and a forbidden-substring tuple — neither is a table name. + "test_security_doc_drift.py": "_MAPPED_MODEL_NON_PHI_FIELDS — model field names, not tables", + "test_tray_boundary.py": "_FORBIDDEN_FIELD_SUBSTRINGS — substring matching, not tables", + # Response-payload dict keys asserted in API tests. + "test_api.py": "response-body keys, not tables", + "test_api_auth.py": "response-body keys, not tables", + # This scanner and the AST guard both name the table in prose/fixtures on purpose. + "test_fixture_outbox_reset.py": "this file", + "test_sqlserver_encrypt_pass_tables.py": "synthetic fixtures naming the table deliberately", +} + + +def _unguarded_outbox_resets(path: pathlib.Path) -> list[tuple[int, str]]: + """Loops over a literal table tuple containing ``"outbox"`` whose body issues a BARE delete. + + Anchored on the `for` node and its body's source span, so the guarded and unguarded spellings are + told apart by what the statement actually does — not by whether the word ``outbox`` is nearby. + """ + source = path.read_text(encoding="utf-8") + try: + tree = ast.parse(source) + except SyntaxError: # pragma: no cover - a test file that does not parse fails elsewhere + return [] + lines = source.splitlines() + hits: list[tuple[int, str]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.For | ast.AsyncFor): + continue + if not isinstance(node.iter, ast.Tuple | ast.List | ast.Set): + continue + names = { + e.value + for e in node.iter.elts + if isinstance(e, ast.Constant) and isinstance(e.value, str) + } + if "outbox" not in names: + continue + body_lines: list[str] = [] + for stmt in node.body: + body_lines.extend(lines[stmt.lineno - 1 : (stmt.end_lineno or stmt.lineno)]) + body_src = "\n".join(body_lines) + if "DELETE FROM" not in body_src.upper(): + continue + if any(marker.upper() in body_src.upper() for marker in _GUARD_MARKERS): + continue # existence-guarded — a no-op once the table is gone + hits.append((node.lineno, body_src.strip().splitlines()[0][:100])) + return hits + + +def _scanned_files() -> list[pathlib.Path]: + return sorted(p for p in _TESTS.glob("test_*.py") if p.name not in _NOT_RESET_LISTS) + + +def test_no_fixture_issues_an_unguarded_outbox_delete() -> None: + """After the legacy table is retired, an unguarded reset is a module-wide failure. + + Mutation: add `"outbox"` to the reset tuple in `tests/test_metadata_bag.py` — reds here on the plain + leg naming the file and line, which is the only place it can be caught for the nine files that run + on no SQL Server CI step. + + This test shipped one commit ahead of the retirement carrying `xfail(strict=True)`, with the 21 + offending sites as its failure list. `strict` is what made that self-clearing: fixing the last reset + list turned the xfail into an XPASS, which strict reports as a FAILURE, so the marker could not be + left behind to rot silently green — removing it here was forced, not remembered. + """ + scanned = _scanned_files() + # Liveness receipt: report what was EXAMINED, not just what was found. A glob that silently matched + # nothing would make this pass while checking zero files. + assert len(scanned) > 100, f"only scanned {len(scanned)} test files — the glob looks broken" + + offenders: list[str] = [] + for path in scanned: + for lineno, snippet in _unguarded_outbox_resets(path): + offenders.append(f" {path.name}:{lineno} {snippet}") + + assert not offenders, ( + f"scanned {len(scanned)} test files; {len(offenders)} carry an UNGUARDED " + f"`DELETE FROM outbox` reset. Once the legacy table is dropped each of these fails with " + f"'Invalid object name' and takes its whole module with it — and nine of them run on no SQL " + f"Server CI step, so no live leg would tell you:\n" + "\n".join(offenders) + "\n\n" + "Fix by removing 'outbox' from the tuple, or by using the guarded form:\n" + " await cur.execute(f\"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}\")" + ) + + +def test_the_scanner_distinguishes_guarded_from_unguarded() -> None: + """Prove the detector's discrimination, rather than trusting it. + + A scanner that flagged everything would make the test above permanently red and get deleted; one + that flagged nothing would pass forever. Drive both spellings through it. + """ + unguarded = _TESTS / "_scan_fixture_unguarded.py" + guarded = _TESTS / "_scan_fixture_guarded.py" + unguarded.write_text( + "async def f(cur):\n" + ' for table in ("queue", "outbox", "messages"):\n' + ' await cur.execute(f"DELETE FROM {table}")\n', + encoding="utf-8", + ) + guarded.write_text( + "async def f(cur):\n" + ' for table in ("queue", "outbox", "messages"):\n' + " await cur.execute(\n" + " f\"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}\"\n" + " )\n", + encoding="utf-8", + ) + try: + assert _unguarded_outbox_resets(unguarded), "the bare DELETE was NOT flagged" + assert not _unguarded_outbox_resets(guarded), ( + "the OBJECT_ID-guarded DELETE was wrongly flagged" + ) + finally: + unguarded.unlink(missing_ok=True) + guarded.unlink(missing_ok=True) + + +@pytest.mark.parametrize("exempt", sorted(_NOT_RESET_LISTS)) +def test_every_exemption_still_exists_and_still_needs_exempting(exempt: str) -> None: + """An exemption for a file that no longer exists, or no longer mentions the table, is dead weight + that quietly widens the next person's blind spot. Fail when one goes stale.""" + path = _TESTS / exempt + assert path.exists(), f"exempted file {exempt} is gone — drop it from _NOT_RESET_LISTS" + assert "outbox" in path.read_text(encoding="utf-8"), ( + f"{exempt} no longer mentions 'outbox' — the exemption is stale and should be removed, " + f"otherwise a genuine reset list landing in this file later would be silently skipped" + ) + + +def test_the_migration_ships_in_the_schema_batch_not_open_path_code() -> None: + """No database needed. `_schema_hash()` alone decides whether the DDL batch runs at all, so a + migration outside `_SCHEMA` is invisible to that decision and would never fire on an upgrade. + + Mutation: move the statement into an open-path method — reds here, on the plain leg, rather than + silently never running against a real customer upgrade. + """ + from messagefoundry.store.sqlserver import _SCHEMA + + migrations = [s for s in _SCHEMA if "DROP TABLE outbox" in s] + assert len(migrations) == 1, ( + f"expected exactly one outbox migrate-and-drop statement in _SCHEMA, found {len(migrations)}" + ) + stmt = migrations[0] + assert "IF OBJECT_ID('outbox','U') IS NOT NULL" in stmt, ( + "the migration is not existence-guarded" + ) + assert "INSERT INTO queue" in stmt, "the statement drops the table without migrating its rows" + assert f"'{Stage.OUTBOUND.value}'" in stmt, "migrated rows must land at the outbound stage" + assert "EXISTS (SELECT 1 FROM messages" in stmt, "the FK orphan filter is gone" + assert "NOT EXISTS (SELECT 1 FROM queue" in stmt, "the PK anti-join is gone" + + # Placement matters as much as presence: SQL Server defers name resolution, so a statement placed + # before `queue` exists would parse and then fail at RUN time on a legacy DB. + queue_ddl = [i for i, s in enumerate(_SCHEMA) if "CREATE TABLE queue" in s] + assert len(queue_ddl) == 1, "could not locate the queue DDL to check migration ordering" + assert _SCHEMA.index(stmt) > queue_ddl[0], ( + "the migration runs BEFORE `queue` is created; on a legacy DB old enough to lack `queue` that " + "is a failed open inside the batch's single transaction — a startup crash-loop" + ) + + +#: The ONLY engine modules allowed to name the retired table, and how many SQL references each may +#: carry. A migration must obviously still address the table it is migrating away, so a flat ban would +#: be unsatisfiable — but "some references are fine" is how a stray read hides forever. Pinning the +#: COUNT is what keeps the carve-out honest: a new `FROM outbox` anywhere in these files moves the +#: number and reds, even though the file is exempt from the ban itself. +_MIGRATION_MODULES = { + # POSIX keys, matched against a POSIX-normalised path below. Native separators would key on + # backslashes and MISS on the ubuntu leg, reporting both migrations as stray — a guard that is red + # on one OS and green on another, which is worse than no guard. + "store/store.py": (2, "_migrate_outbox_to_queue — the SQLite fold-into-queue + DROP"), + "store/sqlserver.py": (1, "the _SCHEMA migrate-and-drop statement (ASVS 14.2.7)"), +} + + +def test_only_the_two_migrations_still_reference_the_retired_table() -> None: + """No database needed. Anywhere else, a surviving read/write is *Invalid object name* at runtime. + + The keyed encrypt pass is covered separately and structurally by + `tests/test_sqlserver_encrypt_pass_tables.py`; this is the broad sweep for everything else. + + Mutation: add a `SELECT ... FROM outbox` to any engine module — reds naming the file and line. Add + one to a MIGRATION module — still reds, on the pinned count, which is the case a plain + file-exemption list would have missed. + """ + import pathlib + import re + + root = pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" + files = sorted(root.rglob("*.py")) + # Liveness receipt: report what was EXAMINED. A glob matching nothing would pass vacuously. + assert len(files) > 40, ( + f"liveness: only {len(files)} engine modules scanned — the glob is broken" + ) + + # Statements that would actually touch the table. `outbox_id`, `outbox_for`, `outbox_by_status` + # and friends are API/method names, not the table, so the pattern is anchored on SQL keywords. + sql = re.compile(r"\b(FROM|INTO|UPDATE|TABLE|JOIN)\s+outbox\b", re.I) + stray: list[str] = [] + counts: dict[str, int] = {} + for path in files: + rel = path.relative_to(root).as_posix() + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + if not sql.search(line) or "DROP TABLE outbox" in line: + continue + if rel in _MIGRATION_MODULES: + counts[rel] = counts.get(rel, 0) + 1 + continue + stray.append(f" {rel}:{lineno}: {line.strip()[:100]}") + + assert not stray, ( + f"scanned {len(files)} engine modules; these still address the retired `outbox` table " + f"outside a migration:\n" + "\n".join(stray) + ) + for rel, (expected, reason) in _MIGRATION_MODULES.items(): + got = counts.get(rel, 0) + assert got == expected, ( + f"{rel} carries {got} SQL references to `outbox`, expected {expected} ({reason}). " + f"More means a new one crept into a module exempt from the ban; fewer means the migration " + f"itself was removed or rewritten — in which case a legacy store never gets folded in and " + f"its PHI bodies stay unpurgeable, which is the whole point of the retirement." + ) + assert "DROP TABLE outbox" in (root / rel).read_text(encoding="utf-8"), ( + f"{rel} is exempted as a MIGRATION but no longer DROPs the table — the exemption is stale" + ) diff --git a/tests/test_load_failover_postgres.py b/tests/test_load_failover_postgres.py index 98800fb0..2542a558 100644 --- a/tests/test_load_failover_postgres.py +++ b/tests/test_load_failover_postgres.py @@ -38,7 +38,6 @@ _RESET_TABLES = ( "messages", "queue", - "outbox", "response", "leader_lease", "nodes", diff --git a/tests/test_load_failover_sqlserver.py b/tests/test_load_failover_sqlserver.py index 67bcf8a6..089d6166 100644 --- a/tests/test_load_failover_sqlserver.py +++ b/tests/test_load_failover_sqlserver.py @@ -39,7 +39,6 @@ # table already exists (a fresh DB has none yet — the nodes create them empty on start). _RESET_TABLES = ( "queue", - "outbox", "response", "leader_lease", "nodes", diff --git a/tests/test_metadata_bag.py b/tests/test_metadata_bag.py index 23aa1757..c297dbd3 100644 --- a/tests/test_metadata_bag.py +++ b/tests/test_metadata_bag.py @@ -37,7 +37,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: # a clean slate — mirrors the other SS suites cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_outbound_batch.py b/tests/test_outbound_batch.py index d9673e2e..11cf964f 100644 --- a/tests/test_outbound_batch.py +++ b/tests/test_outbound_batch.py @@ -58,7 +58,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_phi_at_rest_inventory.py b/tests/test_phi_at_rest_inventory.py index 14157b6d..c6ebe56d 100644 --- a/tests/test_phi_at_rest_inventory.py +++ b/tests/test_phi_at_rest_inventory.py @@ -83,8 +83,12 @@ # exactly the opposite — so the suite stayed green while the docs said there was no purge. # # Each string is deliberately NARROWER than it looks. The bare "no purge on any backend" is NOT - # listed: it is a substring of PHI.md's legacy `outbox.payload` row, which is still TRUE. Adding - # the short form would red the suite on a correct statement. + # listed, and the reason CHANGED without changing the answer. It used to collide with PHI.md's + # legacy `outbox.payload` row, which was then still true. ASVS 14.2.7 retired that row — but the + # same edit added a PAST-TENSE account of what the retirement fixed ("...kept full outbound PHI + # bodies there that no purge on any backend reached"), which is a true statement about history and + # would red on the short form just as the old row did. Re-verified by grep at retirement time; a + # phrase whose collision merely MOVED is not a phrase that became safe to register. "nulled by no purge on any backend", "No retention purge nulls this column", "No retention purge on any backend", @@ -98,6 +102,29 @@ # uses "no retention path" (the surviving gaps say "no purge"/"keep-forever"). "no retention path at all", "no retention path exists at all", + # ASVS 14.2.7 (this change): `reference.value` gains purge_reference_snapshots, and the legacy + # SQL Server `outbox` table is migrated into `queue` and DROPped. The three statements below are + # TRUE TODAY and become false in Commits 3 and 6 — they are registered HERE, first, so that + # `test_retired_false_claims_do_not_reappear` goes RED against the UNEDITED docs and only turns + # green once the edits actually land. Registering after the edit proves nothing: it would pass on + # day one whether or not the guard works. + # + # A FOURTH candidate was DELIBERATELY STRUCK. The obvious string "no retention purge" collides + # with a still-TRUE sentence: + # docs/security/ASVS-292-289-HANDOFF-2026-07-25.md:173 + # "`pending_approvals.params` is unencrypted and has no retention purge on any backend." + # `_retired_claim_hits()` globs docs/*.md PLUS docs/security/*.md — and docs/security/ is + # GIT-IGNORED. So registering it reds on a developer's machine and passes in CI, where the file + # does not exist. A guard that is strictly weaker in CI than on a laptop is not a guard. + # Grep evidence recorded at registration time (docs/*.md | docs/security/*.md): + # "no retention purge" 1 | 1 <- STRUCK, collides with a true statement + # "**No purge path at all**" 1 | 0 + # "touched by no purge on any backend" 1 | 0 + # "is purged by nothing on any backend" 1 | 0 + # Re-run that grep before adding any further string here. + "**No purge path at all**", + "touched by no purge on any backend", + "is purged by nothing on any backend", ) #: Filename markers identifying an ASVS **scoring / evidence** artifact. Those documents quote the #: retired wording deliberately, as the record of what was found, so scanning them for it would be a @@ -302,7 +329,10 @@ def test_cipher_registry_is_enumerable_from_code() -> None: "users.totp_secret", "connection_event.reason", "alert_instance.reason", - "outbox.payload", + # `outbox.payload` was here until the legacy SQL Server table was migrated into `queue` and + # DROPped (ASVS 14.2.7). It is gone from the registry BECAUSE the cell no longer exists — + # `test_per_backend_cipher_counts_are_derived_not_hand_written` is what proves that is a real + # retirement and not a silently narrowed enumeration: it re-derives 18/17/17 from the code. "uploaded_file.body", ): assert expected in cells, f"{expected} vanished from the code-derived cipher registry" @@ -500,10 +530,16 @@ def test_guard_detects_a_planted_omission() -> None: _assert_purge_surface_documented(["purge_alert_instances"], damaged_purge) -@pytest.mark.parametrize("victim", ["shared_body.body", "outbox.payload"]) +@pytest.mark.parametrize("victim", ["shared_body.body"]) def test_guard_detects_a_planted_full_row_deletion(victim: str) -> None: - """The case the whole-section check passed: both tokens survive in §2's coverage PROSE, so - deleting their inventory ROW was undetectable until the assertion was table-scoped.""" + """The case the whole-section check passed: the token survives in §2's coverage PROSE, so + deleting its inventory ROW was undetectable until the assertion was table-scoped. + + `outbox.payload` was the second parameter until ASVS 14.2.7 retired the legacy SQL Server table. + It is dropped rather than kept because this case needs a victim that is BOTH in the code-derived + cipher registry AND named in §2's prose; a retired cell is in neither, so keeping it would fail on + the `assert victim in cells` precondition — a fixture failure, not a guard failure. + """ cells = _cipher_cells() assert victim in cells rows = _table_rows(_section(2)) diff --git a/tests/test_pooled_rider.py b/tests/test_pooled_rider.py index 4e2c0ed6..8c13c903 100644 --- a/tests/test_pooled_rider.py +++ b/tests/test_pooled_rider.py @@ -132,7 +132,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_reference_sets.py b/tests/test_reference_sets.py index 44175855..23cc9485 100644 --- a/tests/test_reference_sets.py +++ b/tests/test_reference_sets.py @@ -818,3 +818,174 @@ async def test_genuine_source_failure_still_retries_and_keeps_last_good( assert not [r for r in caplog.records if r.levelname == "ERROR"] # WARNING, not ERROR finally: await store.close() + + +# --- ASVS 14.2.7: purge_reference_snapshots (orphan-scoped) ------------------ +# +# `reference.value` is PL-2 (PHI.md §2) and until 14.2.7 had NO purge path at all: a set dropped from +# config kept its decryptable rows forever, because the only thing that ever replaced a snapshot was +# the next sync's build-new-then-flip — which never comes for a set nobody declares. + + +async def _seed(store: MessageStore, name: str, *, synced_at: float, rows: dict[str, Any]) -> None: + """Write a snapshot and backdate its active-version pointer, so age is controllable.""" + await store.write_reference_snapshot(name=name, version="v1", rows=rows) + await store._db.execute( + "UPDATE reference_version SET synced_at = ? WHERE name = ?", (synced_at, name) + ) + await store._commit() + + +async def test_purge_deletes_only_undeclared_sets(tmp_path: Path) -> None: + """The keep-set is `declared`; age alone is never sufficient. + + Mutation: drop the `r[0] not in declared` filter — reds, because the still-wired set loses its rows. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "wired", synced_at=0.0, rows={"A": "1"}) # ancient BUT still declared + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"wired"}) + assert deleted == 1 + view = store.reference_view() + assert view["wired"] == {"A": "1"}, "a DECLARED set must survive however old it is" + assert view.get("orphan") in (None, {}), "the orphan's rows must be gone" + finally: + await store.close() + + +async def test_purge_leaves_a_recent_orphan_alone(tmp_path: Path) -> None: + """Undeclared but NEWER than the cutoff — the window still applies to orphans. + + Without this, `declared` alone would drive deletion and the age window would be decorative. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=5000.0, rows={"B": "2"}) + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"other"}) + assert deleted == 0 + assert store.reference_view()["orphan"] == {"B": "2"} + finally: + await store.close() + + +async def test_purge_refuses_an_empty_declared_set(tmp_path: Path) -> None: + """An empty keep-set reads as "every set is abandoned" and would wipe the store. + + `registry is None` does not cover it: a registry that LOADS FINE while declaring zero reference + sets — a subset --config, a per-team split, a harness redirect aimed at the real DB — yields + `references == {}`. Absence-based guards fail open, so this one is positive-signal. + + Mutation: replace the raise with `return 0` — this reds, and so does the row-count assertion. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "a", synced_at=0.0, rows={"k": "1"}) + with pytest.raises(ValueError, match="non-empty"): + await store.purge_reference_snapshots(older_than=1000.0, declared=frozenset()) + assert store.reference_view()["a"] == {"k": "1"}, ( + "nothing may be deleted on the refusal path" + ) + finally: + await store.close() + + +async def test_purge_bumps_the_version_so_a_follower_converges(tmp_path: Path) -> None: + """The pointer row SURVIVES but its version CHANGES — and that is what carries the deletion. + + `converge_reference_cache` only reloads a set whose active version DIFFERS from the one a handle + reflects. Leave the version alone and every cluster follower keeps serving the purged PHI out of + RAM until restart; delete the pointer instead and converge never notices, because it only + adds/updates names present in a fresh read. Bumping routes the deletion through the mechanism that + already exists. + + Mutation: drop the UPDATE — `version` stays 'v1' and this reds. That is the single-handle proxy for + the cluster bug; the real two-handle proof lives in the Postgres/SQL Server suites, because SQLite + is single-node and its converge is legitimately a no-op. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + row = await ( + await store._db.execute( + "SELECT version, row_count FROM reference_version WHERE name = 'orphan'" + ) + ).fetchone() + assert row is not None, ( + "the pointer row must SURVIVE — deleting it is invisible to converge" + ) + assert row["version"] == "purged:v1", "the version must change or a follower never reloads" + assert row["row_count"] == 0 + finally: + await store.close() + + +async def test_purge_is_idempotent_and_does_not_double_prefix(tmp_path: Path) -> None: + """A second pass over an already-purged set must be a no-op, not a churn of 'purged:purged:v1'.""" + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + first = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + second = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + assert (first, second) == (1, 0) + row = await ( + await store._db.execute("SELECT version FROM reference_version WHERE name = 'orphan'") + ).fetchone() + assert row["version"] == "purged:v1" + finally: + await store.close() + + +async def test_a_concurrent_resync_between_decision_and_delete_is_not_destroyed( + tmp_path: Path, +) -> None: + """The TOCTOU the eligibility re-assert exists for — the race landed WHERE IT ACTUALLY IS. + + `declared` is computed by the RUNNER outside any store lock, and inside the store the eligibility + SELECT and the DELETE are separate statements. The dangerous window is BETWEEN them: a config + reload commits a fresh patient-keyed snapshot after the set has been judged eligible but before its + rows are removed. + + THE FIRST VERSION OF THIS TEST WAS WORTHLESS and is worth recording. It re-synced *before* calling + the purge, so the internal SELECT already filtered the set out and the DELETE never ran — removing + the `EXISTS` re-assert changed nothing and the mutation SURVIVED. A test that exercises the + candidate filter cannot prove the re-assert. So the re-sync is injected mid-method here, which is + the only place the race exists. + + Mutation: drop `AND EXISTS (... synced_at < ?)` from the DELETE — the fresh rows are destroyed and + this reds. Verified killed. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"OLD": "x"}) + + real_execute = store._db.execute + fired = False + + async def racing_execute(sql: str, params: Any = None): # type: ignore[no-untyped-def] + nonlocal fired + if not fired and sql.lstrip().upper().startswith("DELETE FROM REFERENCE"): + # The reload lands HERE: after eligibility was decided, before the rows are removed. + fired = True + await real_execute( + "UPDATE reference_version SET synced_at = ? WHERE name = ?", (9999.0, "orphan") + ) + return ( + await real_execute(sql, params) if params is not None else await real_execute(sql) + ) + + store._db.execute = racing_execute # type: ignore[method-assign] + try: + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + finally: + store._db.execute = real_execute # type: ignore[method-assign] + + assert fired, "the race never fired — the test proved nothing about the re-assert" + assert deleted == 0, "a set re-synced mid-purge must survive the DELETE" + rows = await ( + await store._db.execute("SELECT key FROM reference WHERE name = 'orphan'") + ).fetchall() + assert [r["key"] for r in rows] == ["OLD"], "the freshly-eligible rows were destroyed" + finally: + await store.close() diff --git a/tests/test_retention_classification_drift.py b/tests/test_retention_classification_drift.py new file mode 100644 index 00000000..aede287e --- /dev/null +++ b/tests/test_retention_classification_drift.py @@ -0,0 +1,242 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""`PHI_RETENTION_WINDOWS` and docs/PHI.md §2's Retention column must describe the SAME set. + +ASVS 14.2.7 wants retention derived from a data classification. The serve gate's tier list is built +from :data:`PHI_RETENTION_WINDOWS`; this test is what makes that constant *derived* rather than +hand-typed, by asserting two-way equality against the column. Add a PHI tier to the doc without adding +it here, or here without the doc, and this reds. + +**The parser is bounded STRUCTURALLY, and that is the whole difficulty.** §2 contains THREE contiguous +pipe blocks — a 2-column PL legend, the 7-column inventory, and a 4-column at-rest threat matrix. A +naive "every pipe row in §2" scan trips on the other two; and the natural repair (skip rows whose cell +count is wrong) DISARMS the cell-count check, so the drift test would then pass over a mangled table. +So the inventory is located by its own header and asserted UNIQUE, the cell-count check applies only +inside it, and there is a floor on the row count. +""" + +from __future__ import annotations + +import pathlib +import re + +import pytest + +from messagefoundry.config.retention_classification import ( + MIN_PHI_RETENTION_WINDOWS, + PHI_RETENTION_WINDOWS, + auto_bounded_windows, + warn_only_windows, +) +from messagefoundry.config.settings import ( + BackupSettings, + LoggingSettings, + RetentionSettings, + SecuritySettings, + StoreSettings, +) + +_PHI_MD = pathlib.Path(__file__).resolve().parents[1] / "docs" / "PHI.md" + +#: A backticked `[section].setting` — the machine-readable half of every bounded vocabulary form. +_SETTING = re.compile(r"`(\[[a-z_]+\]\.[a-z_]+)`") + +#: The prose forms, which carry NO backticked setting. Listed so an unrecognised cell is an error +#: rather than being silently read as "no window". +_PROSE_FORMS = ("UNBOUNDED — honest gap", "n/a — not PHI", "keep-forever by design") + + +def _inventory_rows() -> list[list[str]]: + """The §2 at-rest inventory as cell lists — located structurally, asserted unique.""" + lines = _PHI_MD.read_text(encoding="utf-8").splitlines() + headers = [i for i, line in enumerate(lines) if line.startswith("| Location | Backends |")] + assert len(headers) == 1, ( + f"expected exactly ONE §2 inventory header in docs/PHI.md, found {len(headers)} at lines " + f"{[i + 1 for i in headers]}. Zero means the table was renamed and this guard is asserting " + f"nothing; more than one means the scan would silently merge two tables." + ) + start = headers[0] + ncols = lines[start].count("|") - 1 + + rows: list[list[str]] = [] + i = start + 2 # skip the header and its separator + while i < len(lines) and lines[i].lstrip().startswith("|"): + line = lines[i] + cells = [c.strip() for c in line.strip().strip("|").split("|")] + assert len(cells) == ncols, ( + f"docs/PHI.md:{i + 1} has {len(cells)} cells, header has {ncols}. Do NOT 'fix' this by " + f"skipping mismatched rows — that disarms this check and lets a mangled table pass.\n" + f" {line[:120]}" + ) + rows.append(cells) + i += 1 + + # Floor + liveness receipt. A parser that silently matched nothing would make every assertion + # below pass vacuously, which is the exact failure mode this whole change set exists to remove. + assert len(rows) >= 35, ( + f"only {len(rows)} inventory rows parsed from docs/PHI.md §2 (expected ~38). The table shape " + f"changed or the scan is broken — either way the equality assertions below are worthless." + ) + return rows + + +def _documented_settings() -> set[str]: + """Every `[section].setting` named in the Retention column.""" + rows = _inventory_rows() + documented: set[str] = set() + for row in rows: + cell = row[-1] # Retention is the last column + found = _SETTING.findall(cell) + if found: + documented.update(found) + continue + assert any(form in cell for form in _PROSE_FORMS), ( + f"unrecognised Retention cell {cell!r}. Every cell must be one of the seven vocabulary " + f"forms — a bounded form naming a backticked `[section].setting`, or one of {_PROSE_FORMS}. " + f"An unparsed cell would otherwise read as 'no window' and silently drop a PHI tier out of " + f"the generated gate list." + ) + return documented + + +def test_the_constant_and_the_doc_describe_the_same_windows() -> None: + """Two-way equality. This is the control that makes the constant *derived*. + + Mutation: delete any entry from `PHI_RETENTION_WINDOWS` — reds on the doc-not-code side. Change a + §2 cell to name a window that does not exist — reds on the code-not-doc side. + """ + documented = _documented_settings() + declared = {w.setting for w in PHI_RETENTION_WINDOWS} + + doc_not_code = documented - declared + code_not_doc = declared - documented + assert not doc_not_code, ( + f"docs/PHI.md §2 classifies these windows but PHI_RETENTION_WINDOWS does not carry them: " + f"{sorted(doc_not_code)}. The serve gate is generated from the constant, so a tier documented " + f"only in the doc is NOT checked at startup." + ) + assert not code_not_doc, ( + f"PHI_RETENTION_WINDOWS carries these but docs/PHI.md §2 does not classify them: " + f"{sorted(code_not_doc)}. Either the doc lost a row or the constant invented coverage." + ) + + +#: `[section]` -> the settings model that owns it. The classification spans FIVE sections, not just +#: `[retention]`, so a check against one model would have to exempt the rest — and an exemption list is +#: how a field name silently stops being verified. +_SECTION_MODELS = { + "[retention]": RetentionSettings, + "[store]": StoreSettings, + "[backup]": BackupSettings, + "[security]": SecuritySettings, + "[logging]": LoggingSettings, +} + + +def test_every_declared_field_exists_on_its_own_settings_model() -> None: + """Each window's `field` must exist on the model for ITS section — no exemptions. + + The first version of this test checked every window against `RetentionSettings` and exempted + `[store]`/`[security]` by prefix. Adding `[backup].retention_keep` reded it, and the tempting fix + was to widen the exemption — which would have stopped verifying three of the five sections. Routing + each window to its own model instead makes the check STRONGER than before it failed. + + Mutation: rename any `field` — reds here, at import time, rather than in a serve path CI never runs. + """ + unknown_section = [ + w.setting for w in PHI_RETENTION_WINDOWS if w.reads_from not in _SECTION_MODELS + ] + assert not unknown_section, ( + f"windows in a section with no model mapping: {unknown_section} — add it to _SECTION_MODELS " + f"rather than exempting it, or the field name stops being checked" + ) + missing = [ + f"{w.setting} -> {_SECTION_MODELS[w.reads_from].__name__}.{w.field}" + for w in PHI_RETENTION_WINDOWS + if w.field not in _SECTION_MODELS[w.reads_from].model_fields + ] + assert not missing, f"windows naming a field their own settings model does not have: {missing}" + + +def test_every_requires_setting_resolves() -> None: + """A `requires_setting` naming a field that does not exist would make the gate skip forever. + + This is the `requires_setting` trap in its quiet form: the gate skips a window whose dependency is + unmet, so a MISSPELLED dependency is indistinguishable from an unmet one — the window is simply + never checked, and nothing reds. Resolve every pair against the real models. + """ + models = {"logging": LoggingSettings, "backup": BackupSettings, "store": StoreSettings} + for w in PHI_RETENTION_WINDOWS: + if w.requires_setting is None: + continue + section, field = w.requires_setting + assert section in models, f"{w.setting}: requires_setting names unknown section {section!r}" + assert field in models[section].model_fields, ( + f"{w.setting}: requires_setting ({section}, {field}) does not resolve — the gate would " + f"skip this window forever and report success" + ) + + +def test_the_floor_is_seven_not_six_and_is_enforced() -> None: + """`if not TUPLE` is not a floor — a merge dropping five of seven leaves a non-empty tuple. + + Seven because `reference_snapshot_days` made the classification-derived list seven entries; any + floor written as six was wrong the day the reference purge landed. + + Mutation: set MIN to 6 — reds, because the real length is checked against it AND against the + documented reason for the value. + """ + assert MIN_PHI_RETENTION_WINDOWS == 9, ( + "the floor changed. It is 9 — and that number was CORRECTED BY THIS TEST rather than counted: " + "it was first written as 7, and the two-way equality above immediately reported " + "[backup].retention_keep and [retention].connection_event_retention_hours as " + "documented-but-absent. If the classification genuinely grew or shrank, update this assertion " + "and say why in the same commit." + ) + assert len(PHI_RETENTION_WINDOWS) >= MIN_PHI_RETENTION_WINDOWS, ( + f"PHI_RETENTION_WINDOWS has {len(PHI_RETENTION_WINDOWS)} entries, floor is " + f"{MIN_PHI_RETENTION_WINDOWS} — a startup gate built from this would check fewer windows than " + f"the classification defines while still reporting success." + ) + + +def test_the_auto_bound_and_warn_only_split_matches_the_owner_ruling() -> None: + """The owner ruled which windows silently default and which only warn (2026-07-30). + + Auto-bounding a window whose timestamp moves only on a WRITE deletes live operational data — + `state.set_at` is never refreshed by `state_get`, so a long-lived correlation map would vanish + mid-stream. This asserts the split rather than trusting a comment. + + Mutation: give `state_max_age_days` an `auto_bound_days=30` — reds. + """ + auto = {w.setting for w in auto_bounded_windows()} + warn = {w.setting for w in warn_only_windows()} + assert auto == { + "[security].delete_message_bodies_after_days", + "[retention].dead_letter_days", + "[retention].reference_snapshot_days", + }, f"auto-bound set drifted from the owner ruling: {sorted(auto)}" + assert "[retention].state_max_age_days" in warn, ( + "state_max_age_days must NOT auto-bound — purge_state deletes by `set_at`, which a read never " + "refreshes, so a Handler's correlation map would be deleted while still in use" + ) + assert "[retention].search_preset_days" in warn, ( + "search_preset_days must NOT auto-bound — pipeline/retention.py forbids exactly that " + "inheritance in prose, and no test anchors that comment" + ) + assert not (auto & warn), "a window cannot be both auto-bound and warn-only" + + +@pytest.mark.parametrize("form", _PROSE_FORMS) +def test_each_prose_form_is_actually_used(form: str) -> None: + """The vocabulary must not contain dead forms. + + A form nothing uses is either a classification nobody applied or a leftover — both mean the legend + describes a scheme the table does not follow. `UNBOUNDED — honest gap` in particular MUST appear: + this codebase has unbounded PHI tiers, and a classification with none is flattering, not complete. + """ + cells = [row[-1] for row in _inventory_rows()] + assert any(form in cell for cell in cells), ( + f"the vocabulary form {form!r} appears in no §2 Retention cell. If a classification genuinely " + f"no longer applies, remove it from the legend in the same commit." + ) diff --git a/tests/test_seq_only_fifo.py b/tests/test_seq_only_fifo.py index dcb35af5..7db181fe 100644 --- a/tests/test_seq_only_fifo.py +++ b/tests/test_seq_only_fifo.py @@ -60,7 +60,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_shard_recovery_sqlserver.py b/tests/test_shard_recovery_sqlserver.py index a51900fe..8862dc38 100644 --- a/tests/test_shard_recovery_sqlserver.py +++ b/tests/test_shard_recovery_sqlserver.py @@ -85,7 +85,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_sqlserver_encrypt_pass_tables.py b/tests/test_sqlserver_encrypt_pass_tables.py new file mode 100644 index 00000000..32456443 --- /dev/null +++ b/tests/test_sqlserver_encrypt_pass_tables.py @@ -0,0 +1,168 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Every ciphered (table, column) the SQL Server backend sweeps must name a table it actually creates. + +**This guard exists because CI cannot catch the defect any other way.** `_encrypt_existing_rows` +early-returns at `if not self._cipher.encrypts: return` — and **every** SQL Server CI step runs +*keyless* (`MEFOR_STORE_ENCRYPTION_KEY` appears nowhere in the seven SS steps or the five PG steps of +`.github/workflows/ci.yml`). So the `SELECT ... FROM ` loop below that early return **never +executes on any CI leg**. A `(table, column)` pair naming a dropped table is therefore invisible to the +entire test suite, right up until a *keyed* production open crash-loops on `Invalid object name`. + +That is not hypothetical: ASVS 14.2.7 removes the legacy `outbox` table from `_SCHEMA`, and the pair +`("outbox", "payload")` sits ~1200 lines away in a different method. Removing one and forgetting the +other is a startup crash-loop in production and a green suite in CI. + +So this test does what CI's runtime cannot: it reads the source. No database, no driver, no key — it +runs on the plain leg, which is the only leg guaranteed to exist. + +Deliberately checking the sweep passes against the module's OWN `CREATE TABLE` set rather than against +a live database: a live check would need the SS leg (which is keyless, see above) and would pass on a +database whose schema had drifted from the code that ships. +""" + +from __future__ import annotations + +import ast +import pathlib +import re + +import pytest + +_SQLSERVER = ( + pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" / "store" / "sqlserver.py" +) + +#: The methods whose (table, column) literals must resolve. Both walk ciphered cells; both are reached +#: only on a KEYED handle, which is exactly why neither is exercised in CI. +_SWEEP_FUNCTIONS = ("_encrypt_existing_rows", "reencrypt_to_active") + +#: `CREATE TABLE [dbo].[name]` / `CREATE TABLE name` — the schema this module actually deploys. +_CREATE_TABLE = re.compile(r"CREATE TABLE\s+(?:\[?dbo\]?\.)?\[?([A-Za-z_][A-Za-z0-9_]*)\]?") + + +def _created_tables(source: str) -> set[str]: + return set(_CREATE_TABLE.findall(source)) + + +def _swept_pairs(source: str) -> list[tuple[str, str, int]]: + """Every ``(table, column)`` literal driving a sweep loop, with its line. + + Anchored on the LOOP (``for table, in (...)``), not on "any 2-string tuple in the function". + The naive version over-matched immediately and usefully: both functions also build composite AAD + tuples like ``("body", "detail")`` and ``("event_type", "connection")``, whose first element is a + COLUMN, not a table. Flagging those would have made this guard cry wolf on ~14 legitimate pairs, + and a guard that always fails gets deleted or `xfail`ed — which is how a real detector dies. + + Walks the AST rather than grepping, so a pair split across lines or carrying a trailing comment is + still found: the `("users", "totp_secret")` entry is wrapped across three lines in the real source, + and a line-based scan misses it (asserted below). + """ + tree = ast.parse(source) + found: list[tuple[str, str, int]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef): + continue + if node.name not in _SWEEP_FUNCTIONS: + continue + for inner in ast.walk(node): + if not isinstance(inner, ast.For | ast.AsyncFor): + continue + target = inner.target + # `for table, column in (...)` / `for table, ncol in (...)` — the first bound name must be + # `table`, which is what makes element[0] a table name rather than a column. + if not isinstance(target, ast.Tuple) or not target.elts: + continue + first_name = target.elts[0] + if not isinstance(first_name, ast.Name) or first_name.id != "table": + continue + if not isinstance(inner.iter, ast.Tuple | ast.List): + continue + for element in inner.iter.elts: + if not isinstance(element, ast.Tuple) or not element.elts: + continue + table = element.elts[0] + column = element.elts[1] if len(element.elts) > 1 else None + if isinstance(table, ast.Constant) and isinstance(table.value, str): + col = column.value if isinstance(column, ast.Constant) else "?" + found.append((table.value, str(col), element.lineno)) + return found + + +def test_every_swept_table_is_created_by_this_module() -> None: + """A ciphered-cell sweep may only name a table `_SCHEMA` creates. + + Mutation: add `("nonexistent_table", "x")` to `_encrypt_existing_rows` — reds here, on the plain + leg, with no database. After the legacy `outbox` table is dropped, re-adding `("outbox", "payload")` + reds here too, which is the entire reason this file exists. + """ + source = _SQLSERVER.read_text(encoding="utf-8") + created = _created_tables(source) + pairs = _swept_pairs(source) + + # Liveness receipt. A parse that silently found nothing would make the assertion below pass + # vacuously, which is precisely the failure mode this guard is meant to prevent elsewhere. + assert created, "no CREATE TABLE statements found — the regex or the module shape changed" + assert pairs, ( + f"no (table, column) literals found in {_SWEEP_FUNCTIONS} — either the functions were renamed " + f"or the AST walk is broken, and this guard is asserting nothing" + ) + + unknown = sorted({(t, c, ln) for t, c, ln in pairs if t not in created}) + assert not unknown, ( + "a ciphered-cell sweep names a table this module does not CREATE. On a KEYED open this is " + "'Invalid object name' at startup — a crash-loop — and no CI leg catches it, because every " + "SQL Server CI step is keyless and returns before the sweep.\n" + + "\n".join( + f" sqlserver.py:{ln} ({t!r}, {c!r}) <- table {t!r} is never created" + for t, c, ln in unknown + ) + + f"\n\ncreated tables: {', '.join(sorted(created))}" + ) + + +def test_the_guard_can_actually_see_an_unknown_table() -> None: + """Prove the detector fires, rather than trusting that it would. + + The test above passing tells you nothing on its own — it passes identically if `_swept_pairs` + returns garbage or `_created_tables` over-matches. Drive the same functions over a synthetic module + containing a known-bad pair and assert it is found. + """ + synthetic = """ +_SCHEMA = ["IF OBJECT_ID('queue','U') IS NULL CREATE TABLE queue (id INT)"] + +class S: + async def _encrypt_existing_rows(self) -> None: + for table, column in (("queue", "payload"), ("outbox", "payload")): + pass +""" + created = _created_tables(synthetic) + pairs = _swept_pairs(synthetic) + assert created == {"queue"}, created + assert ("outbox", "payload", 6) in [(t, c, ln) for t, c, ln in pairs], pairs + unknown = [t for t, _c, _ln in pairs if t not in created] + assert unknown == ["outbox"], f"the detector did not flag the unknown table: {unknown}" + + +@pytest.mark.parametrize("wrapped_across_lines", [True, False]) +def test_the_ast_walk_finds_pairs_a_line_scan_would_miss(wrapped_across_lines: bool) -> None: + """The real source wraps `("users", "totp_secret")` across three lines with a trailing comment. + + A grep-based implementation misses it, silently shrinking the checked set — a guard that examines + less than it claims to. Assert both spellings are found so the walk cannot regress to line-based. + """ + pair = ( + '(\n "users",\n "totp_secret",\n ), # a comment' + if wrapped_across_lines + else '("users", "totp_secret"),' + ) + synthetic = f""" +class S: + async def _encrypt_existing_rows(self) -> None: + for table, column in ( + {pair} + ): + pass +""" + pairs = [(t, c) for t, c, _ln in _swept_pairs(synthetic)] + assert ("users", "totp_secret") in pairs, f"wrapped={wrapped_across_lines}: {pairs}" diff --git a/tests/test_sqlserver_legacy_outbox_migration.py b/tests/test_sqlserver_legacy_outbox_migration.py new file mode 100644 index 00000000..01aa1af8 --- /dev/null +++ b/tests/test_sqlserver_legacy_outbox_migration.py @@ -0,0 +1,331 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The legacy SQL Server ``outbox`` table is folded into ``queue`` and DROPped (ASVS 14.2.7). + +**What was actually wrong.** The table was recreated by `_SCHEMA` on every open and read by nothing: +the staged pipeline and every delivery-side method use `queue`. So `outbox.payload` — a full outbound +PHI body on any store upgraded from the pre-staged-pipeline layout — was reached by no purge on any +backend, while `messages.raw` blanked on its own window. The message read as purged and the body +survived indefinitely. Migrating the rows in puts them under the ordinary outbound windows; dropping +the table is what makes the retirement true rather than merely documented. + +**Why this test simulates an UPGRADE rather than just checking a fresh open.** On a fresh DB the +migration statement's `IF OBJECT_ID('outbox','U') IS NOT NULL` guard is false and the body never runs +— a test that only opened a clean store would pass without executing a single line of the thing it +claims to verify. Worse, `_ensure_schema` SKIPS the whole DDL batch when `schema_meta` already records +the current hash, so creating a legacy table after a normal open and re-opening would ALSO migrate +nothing. Both are the "green because nothing was measured" failure. So each test here seeds a real +legacy table and then clears the schema marker, which is exactly the state a genuine upgrade presents: +`_SCHEMA` changed, so its hash no longer matches, so the batch re-runs. +""" + +from __future__ import annotations + +import os +from collections.abc import AsyncIterator +from typing import Any + +import pytest + +from messagefoundry.store import OutboxStatus, Stage + +pytestmark = pytest.mark.skipif( + not os.getenv("MEFOR_TEST_SQLSERVER"), + reason="set MEFOR_TEST_SQLSERVER=1 (+ MEFOR_STORE_* connection env) to run SQL Server tests", +) + +RAW = "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|MSG1|P|2.5.1\r" + +#: The legacy DDL, verbatim as it stood in `_SCHEMA` before the retirement. Kept here rather than +#: imported because the point is to reconstruct a schema the code no longer contains. +_LEGACY_OUTBOX_DDL = """CREATE TABLE outbox ( + id NVARCHAR(64) NOT NULL PRIMARY KEY, message_id NVARCHAR(64) NOT NULL, + channel_id NVARCHAR(256) NOT NULL, destination_name NVARCHAR(256) NOT NULL, + payload NVARCHAR(MAX) NOT NULL, status NVARCHAR(32) NOT NULL, + attempts INT NOT NULL DEFAULT 0, next_attempt_at FLOAT NOT NULL, last_error NVARCHAR(MAX) NULL, + created_at FLOAT NOT NULL, updated_at FLOAT NOT NULL)""" + + +async def _open_store() -> Any: + from messagefoundry.config.settings import load_settings + from messagefoundry.store.sqlserver import SqlServerStore + + settings = load_settings(environ=os.environ).store + return await SqlServerStore.open(settings) + + +async def _reset(store: Any) -> None: + """Clean slate. `outbox` is dropped defensively: a previous test in this module may have left one + behind, and it is NOT in any other suite's reset list any more (the table is retired).""" + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("IF OBJECT_ID('outbox','U') IS NOT NULL DROP TABLE outbox") + for table in ("message_events", "queue", "response", "delivered_keys", "messages"): + await cur.execute(f"DELETE FROM {table}") + await conn.commit() + + +@pytest.fixture +async def store() -> AsyncIterator[Any]: + s = await _open_store() + await _reset(s) + try: + yield s + finally: + await _reset(s) + await s.close() + + +async def _seed_legacy(store: Any, rows: list[tuple[Any, ...]], *, messages: list[str]) -> None: + """Recreate the legacy table, seed it, and put the DB into the state an UPGRADE presents. + + Clearing `schema_meta` is the load-bearing step: with a current marker `_ensure_schema` returns + before executing anything, so without this the next open would migrate nothing and every + assertion below would pass vacuously. A real upgrade reaches the same state by a different route + — `_SCHEMA` changed, so its content hash no longer matches what the marker records. + """ + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("IF OBJECT_ID('outbox','U') IS NOT NULL DROP TABLE outbox") + await cur.execute(_LEGACY_OUTBOX_DDL) + for mid in messages: + await cur.execute( + "INSERT INTO messages (id, channel_id, received_at, source_type, control_id," + " message_type, raw, status) VALUES (?,?,?,?,?,?,?,?)", + (mid, "IB", 1.0, "mllp", "C", "ADT^A01", RAW, "routed"), + ) + for row in rows: + await cur.execute( + "INSERT INTO outbox (id, message_id, channel_id, destination_name, payload, status," + " attempts, next_attempt_at, last_error, created_at, updated_at)" + " VALUES (?,?,?,?,?,?,?,?,?,?,?)", + row, + ) + await cur.execute("DELETE FROM schema_meta") + await conn.commit() + + +async def _tables(store: Any) -> set[str]: + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("SELECT name FROM sys.tables") + return {r[0] for r in await cur.fetchall()} + + +async def _queue_rows(store: Any) -> list[dict[str, Any]]: + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute( + "SELECT id, message_id, stage, channel_id, destination_name, payload, status," + " attempts, created_at, seq FROM queue ORDER BY seq" + ) + cols = [c[0] for c in cur.description] + return [dict(zip(cols, r, strict=True)) for r in await cur.fetchall()] + + +async def test_legacy_outbox_rows_migrate_into_queue_and_the_table_is_dropped(store: Any) -> None: + """The core claim: rows survive as stage='outbound' queue rows, and the table is gone. + + Mutation: delete the `DROP TABLE outbox` from the `_SCHEMA` statement — reds on the table + assertion. Delete the whole statement — reds on the row assertion first. + """ + await _seed_legacy( + store, + [ + ( + "o1", + "m1", + "IB", + "OB_A", + "PAYLOAD-1", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ), + ( + "o2", + "m1", + "IB", + "OB_B", + "PAYLOAD-2", + OutboxStatus.PENDING.value, + 3, + 9.0, + "boom", + 2.0, + 2.0, + ), + ], + messages=["m1"], + ) + + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened), ( + "the legacy table survived the schema batch — a DROP that did not fire leaves the " + "unpurgeable PHI tier exactly where it was" + ) + rows = await _queue_rows(reopened) + assert len(rows) == 2, f"expected both legacy rows folded in, got {rows}" + by_id = {r["id"]: r for r in rows} + assert set(by_id) == {"o1", "o2"}, ( + "row ids must be preserved (they are referenced elsewhere)" + ) + for r in rows: + assert r["stage"] == Stage.OUTBOUND.value + assert r["message_id"] == "m1" + assert r["channel_id"] == "IB" + # Every column carried over verbatim — a migration that silently reset attempts or dropped + # last_error would resurrect a dead row as deliverable. + assert by_id["o1"]["destination_name"] == "OB_A" + assert by_id["o1"]["payload"] == "PAYLOAD-1" + assert by_id["o2"]["destination_name"] == "OB_B" + assert by_id["o2"]["payload"] == "PAYLOAD-2" + assert by_id["o2"]["attempts"] == 3 + assert by_id["o1"]["created_at"] == 1.0 and by_id["o2"]["created_at"] == 2.0 + finally: + await reopened.close() + + +async def test_migrated_rows_are_reachable_by_the_delivery_path(store: Any) -> None: + """A migrated row is not merely present — it must be CLAIMABLE, or the backlog silently stalls. + + This is the assertion that distinguishes a real migration from rows parked in a table with the + wrong stage/status: `claim_next_fifo` filters on both. + """ + await _seed_legacy( + store, + [ + ( + "o1", + "m1", + "IB", + "OB_A", + "PAYLOAD-1", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ) + ], + messages=["m1"], + ) + reopened = await _open_store() + try: + item = await reopened.claim_next_fifo("OB_A") + assert item is not None, ( + "the migrated row is not claimable — the legacy backlog would stall" + ) + assert item.payload == "PAYLOAD-1" + finally: + await reopened.close() + + +async def test_an_orphaned_legacy_row_is_skipped_rather_than_failing_the_open(store: Any) -> None: + """`queue.message_id` is FK-enforced; an orphan would abort the whole DDL batch. + + That is not a degraded start — the batch is one transaction, so it rolls back and re-fails on + every restart: a crash-loop. The orphan was unreplayable anyway (nothing can view or route a row + whose message is gone), so it is dropped and the surviving row still migrates. + + Mutation: remove the `EXISTS (SELECT 1 FROM messages ...)` filter — this test reds with an FK + violation at open, which is precisely the crash-loop it exists to prevent. + """ + await _seed_legacy( + store, + [ + ("o1", "m1", "IB", "OB_A", "GOOD", OutboxStatus.PENDING.value, 0, 0.0, None, 1.0, 1.0), + # message 'gone' was never inserted + ( + "o2", + "gone", + "IB", + "OB_A", + "ORPHAN", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ), + ], + messages=["m1"], + ) + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened) + rows = await _queue_rows(reopened) + assert [r["id"] for r in rows] == ["o1"], ( + f"the orphan should be skipped and the good row kept, got {[r['id'] for r in rows]}" + ) + finally: + await reopened.close() + + +async def test_an_id_already_present_in_queue_does_not_abort_the_batch(store: Any) -> None: + """`queue.id` is the PK, so a colliding legacy id would raise 2627 and roll the batch back. + + uuid4 ids make a genuine collision implausible, which is exactly why this needs a test rather + than an argument: the anti-join is unreachable in normal operation, so nothing else would ever + exercise it, and a silent regression would surface only as a customer's crash-looping upgrade. + + Mutation: remove the `NOT EXISTS (SELECT 1 FROM queue ...)` filter — reds with a PK violation. + """ + await _seed_legacy( + store, + [("dup", "m1", "IB", "OB_A", "LEGACY", OutboxStatus.PENDING.value, 0, 0.0, None, 1.0, 1.0)], + messages=["m1"], + ) + # Plant a `queue` row that already owns the id, then re-clear the marker the insert did not touch. + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute( + "INSERT INTO queue (id, message_id, stage, channel_id, destination_name, payload," + " status, attempts, next_attempt_at, created_at, updated_at)" + " VALUES (?,?,?,?,?,?,?,?,?,?,?)", + ( + "dup", + "m1", + Stage.OUTBOUND.value, + "IB", + "OB_A", + "ALREADY-HERE", + OutboxStatus.PENDING.value, + 0, + 0.0, + 5.0, + 5.0, + ), + ) + await conn.commit() + + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened), ( + "the batch aborted — the table is still here" + ) + rows = await _queue_rows(reopened) + assert len(rows) == 1, f"the colliding legacy row must not be inserted twice, got {rows}" + assert rows[0]["payload"] == "ALREADY-HERE", ( + "the pre-existing queue row must win, not be lost" + ) + finally: + await reopened.close() + + +async def test_a_fresh_store_has_no_outbox_table_at_all(store: Any) -> None: + """The other half of the retirement: `_SCHEMA` must no longer CREATE the table. + + Cheap, and it is the assertion that catches a revert of the DDL removal — the migration statement + alone would then drop a table the very same batch had just recreated, which is stable but leaves + every open re-creating an empty unpurgeable tier. + """ + tables = await _tables(store) + assert "outbox" not in tables, "a fresh open recreated the retired legacy table" + assert "queue" in tables, "liveness: the table enumeration is reading a real schema" diff --git a/tests/test_sqlserver_store.py b/tests/test_sqlserver_store.py index 6c337eac..58ae589a 100644 --- a/tests/test_sqlserver_store.py +++ b/tests/test_sqlserver_store.py @@ -97,7 +97,6 @@ async def store() -> AsyncIterator[object]: # key-rotation reencrypt scan, and this suite asserts EXACT rotate # counts (the == 6 / == 2 above), so a preset left behind by one test # silently miscounts an unrelated one - "outbox", "messages", "sessions", "webauthn_credentials", # ADR 0068: FK to users(id) — must clear before users @@ -1042,7 +1041,7 @@ async def test_reencrypt_to_active_rotates_all_columns_including_state(store) -> try: async with rotated._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "response", "queue", "outbox", "messages"): + for table in ("message_events", "state", "response", "queue", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() finally: @@ -1837,7 +1836,7 @@ async def test_keyless_open_of_encrypted_state_fails_closed(store) -> None: try: async with cleanup._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "response", "queue", "outbox", "messages"): + for table in ("message_events", "state", "response", "queue", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() finally: @@ -4006,7 +4005,7 @@ async def test_the_BOUNDED_path_survives_a_keyed_reopen(store) -> None: try: async with cleanup._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "queue", "outbox", "response", "messages"): + for table in ("message_events", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") # FK order: children first await cur.execute("DELETE FROM cipher_meta WHERE key_id = ?", (key_id,)) await conn.commit() diff --git a/tests/test_sqlserver_sync_handoff.py b/tests/test_sqlserver_sync_handoff.py index 752b4b21..8ebc3738 100644 --- a/tests/test_sqlserver_sync_handoff.py +++ b/tests/test_sqlserver_sync_handoff.py @@ -44,7 +44,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_stage_dispatcher.py b/tests/test_stage_dispatcher.py index 8a188e3d..41733b85 100644 --- a/tests/test_stage_dispatcher.py +++ b/tests/test_stage_dispatcher.py @@ -76,7 +76,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_x12_rte.py b/tests/test_x12_rte.py index abba26b0..e1a56e8b 100644 --- a/tests/test_x12_rte.py +++ b/tests/test_x12_rte.py @@ -345,7 +345,6 @@ async def _open_rte_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}")