Skip to content

Commit ed8a09d

Browse files
committed
revert(retention): undo #1212's default change -- it deletes PHI against an explicit opt-out
REFUTED BY SESSION B, VERIFIED BY ME, AND THEY ARE RIGHT. Restores all seven touched files to origin/main content. Forward-only; no history rewrite, so the two reverted commits stay readable as the record. THE DEFECT, measured not argued: gate guard: if not settings.retention.allow_unbounded_phi: RetentionSettings(allow_unbounded_phi=True).messages_days -> 60 allow_unbounded_phi is the AUDITED, DELIBERATE keep-forever opt-out. When it is true the auto-bound block is SKIPPED ENTIRELY, so the field keeps its model default. At 0 that matched the operator's explicit choice. At 60 the RetentionRunner starts and PURGES PHI BODIES AFTER 60 DAYS on exactly the instances whose operator consciously said keep them forever. Silent PHI deletion against an explicit audited decision, landing ONLY on the path where someone opted in on purpose. That is the inversion of the item's intent, and it is worse than the problem #1212 set out to fix. AND THE CHANGE WAS A NO-OP WHERE IT WAS AIMED. The auto-bound keys on model_fields_set -- whether the operator WROTE the key, not its value -- so an operator who says nothing still gets 30 and always did. I had already found and documented that half. What I MISSED is the half above: I reasoned about the branch where the gate RUNS and never about the branch where it is SKIPPED. Session B checked the skip. A SECOND PREMISE OF MINE WAS ALSO STALE: PR #71 inverted the prod gate, so a production PHI instance with an unset window no longer refuses to start -- it starts, auto-bounded at 30. tests/test_cli.py:1619 is now test_serve_auto_bounds_an_unset_body_window_in_prod asserting rc == 0. So the 'more secure by default, less explicit by default' trade I raised for the owner WAS ALREADY MADE and was never mine to weigh. IF THE 60 IS STILL WANTED, the value to change is auto_bound_days in the retention classification tuple -- what the gate actually applies -- NOT the field default. Note that moving it 30 -> 60 LOOSENS the shipped PHI bound rather than tightening it, which sharpens the owner question rather than resolving it. Also: retention_classification.py carries nine windows behind MIN_PHI_RETENTION_WINDOWS = 9 with a two-way drift test against docs/PHI.md section 2, so any real window change must move the tuple AND the doc together. The knowledge is the deliverable here; the code was wrong. Ledger amendment to follow so #1212 records the refutation rather than reading as unbuilt.
1 parent 58d3add commit ed8a09d

7 files changed

Lines changed: 34 additions & 214 deletions

File tree

docs/CONFIGURATION.md

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -699,35 +699,20 @@ Enforced by the engine's retention/purge task ([pipeline/retention.py](../messag
699699
A purge **NULLs the PHI *body*** past its window while **keeping the message row** (counts,
700700
disposition, and the audit trail stay intact — the Mirth Data-Pruner pattern); it never deletes a
701701
`messages` row and never touches a body still in flight. The *row* survives; its PHI *columns* do not
702-
`messages.metadata` is nulled in the same statement as the body (ASVS 14.2.7).
703-
704-
**Two layers set the PHI-body windows, and the tighter one wins.** The **model default** is `60` days
705-
(#1212) — it replaced keep-forever, so an instance that configures nothing is bounded rather than
706-
unbounded, and the retention runner now **starts on a fully-defaulted engine** where it previously
707-
stayed off. On top of that, `serve`'s **posture gate auto-bounds an *unset* window to `30` days on a
708-
PHI instance** (owner ruling 2026-07-30), and **all three built-in environment names (`dev`,
709-
`staging`, `prod`) derive PHI**. So in practice: **a PHI instance that sets nothing gets 30; a
710-
non-PHI instance that sets nothing gets 60; an explicit value always wins over both.** The gate keys
711-
on whether the field was *set*, not on its value, which is why the model default does not soften it.
712-
713-
The remaining `[retention]` fields still default to `0`/`""` = keep/off, and `0` continues to mean
714-
keep-forever wherever it is set *explicitly* — only the default moved. Note the semantic flip that
715-
follows on a non-PHI instance: a config that omits the window used to mean *keep forever* and now
716-
means *purge at 60 days*.
717-
718-
The posture gate means retention is **not opt-in on a PHI instance**: under
719-
`[security].enforcement = enforce` (the default) an unbounded
702+
`messages.metadata` is nulled in the same statement as the body (ASVS 14.2.7). The raw `[retention]` fields still default to
703+
`0`/`""` = keep/off, **but `serve` applies a posture gate on top of them, so retention is *not*
704+
opt-in on a PHI instance**: under `[security].enforcement = enforce` (the default) an unbounded
720705
`[security].delete_message_bodies_after_days` or `[retention].dead_letter_days` **refuses to start
721706
(exit 2)**; on a non-enforcing PHI instance each *unset* window is auto-bounded to **30 days**. All
722707
three built-in environment names (`dev`, `staging`, `prod`) derive PHI. The audited opt-out is
723708
`[security].allow_keeping_phi_indefinitely = true`. See [PHI.md §8](PHI.md#8-retention--purge).
724709
| Key | Type | Default | Notes |
725710
|---|---|---|---|
726-
| `messages_days` | | *(60)* | **→ moved to `[security].delete_message_bodies_after_days`** (ADR 0118) — set it there; no longer accepted in `[retention]`, and setting it here raises. The model default shown in parentheses is **60 days** (#1212), not keep-forever — but on a **PHI** instance `serve`'s posture gate auto-bounds an unset window to **30** first, and every built-in environment derives PHI, so 60 is what a **non-PHI** instance gets. The `[security]` desugar is presence-gated **per key**, so merely having a `[security]` block changes nothing here — only setting `delete_message_bodies_after_days` writes through. |
727-
| `dead_letter_days` | int | `60` | past N days, null the bodies of **dead-lettered** outbound rows (their own window — a dead row stays replayable until purged). `0` = keep. Defaulted to 60 by #1212 alongside `messages_days`; kept a **separate** window because a site that wants longer to replay a dead row must be able to say so without also extending live-body retention. |
711+
| `messages_days` | | | **→ moved to `[security].delete_message_bodies_after_days`** (ADR 0118) — set it there; no longer accepted in `[retention]`. |
712+
| `dead_letter_days` | int | `0` | past N days, null the bodies of **dead-lettered** outbound rows (their own window — a dead row stays replayable until purged). `0` = keep |
728713
| `allow_unbounded_phi` | | | **→ moved to `[security].allow_keeping_phi_indefinitely`** (ADR 0118) — set it there; no longer accepted in `[retention]`. |
729714
| `state_max_age_days` | int | `0` | past N days, **delete** transform-state entries (ADR 0005) last written before the cutoff — keeps the in-memory state cache + table bounded. A simple global age purge (by `set_at`); per-namespace policy is a follow-up. `0` = keep |
730-
| `connection_event_retention_hours` | int | `0` | past N **hours**, **delete** `connection_event` rows (the `[diagnostics]` #46 transport/lifecycle log — high-volume under a connect-per-message sender or a probe storm, so its own short window in **hours**, not days). `0` = inherit the `messages_days` body window (the ADR 0021 §7.5 default) — which since #1212 means these rows, and resolved operator-alert instances on the same window, are **bounded at 60 days on a defaulted config** rather than kept forever. |
715+
| `connection_event_retention_hours` | int | `0` | past N **hours**, **delete** `connection_event` rows (the `[diagnostics]` #46 transport/lifecycle log — high-volume under a connect-per-message sender or a probe storm, so its own short window in **hours**, not days). `0` = inherit the `messages_days` body window (the ADR 0021 §7.5 default). |
731716
| `app_log_days` | int | `0` | past N days, **delete** application **log files** (`.log`/`.txt`, one level) from the configured `[logging].log_dir` (#120). The supervisor (NSSM `AppRotateBytes`) rotates the daily logs by **size** but never by **age**, so the log dir grows unbounded; this bounds it (by file mtime, so the currently-written file is never eligible). `0` = keep. **No-op unless `[logging].log_dir` is set.** Metadata only — file content is never read. While `app_log_compress_days` is on, the same window also ages out the `*.log.gz`/`*.txt.gz` archives that setting produces — so compressing a log doesn't make it immortal; with compression off the eligible set is exactly what it was |
732717
| `app_log_compress_days` | int | `0` | past N days, **gzip** application **log files** (`.log`/`.txt`, one level — the same selection as `app_log_days`, by mtime, so the currently-written file is never eligible) in `[logging].log_dir` to `<name>.gz` (#119). The log stays readable (`gzip -d`) at a fraction of the disk, so a long-running box keeps far more history for the same footprint. Each file is **free-space prechecked** (`shutil.disk_usage` must show room for the source **plus** its archive plus a `max(10%, 1 MiB)` margin — short, and the file is **skipped and logged**, never attempted) and each written archive is **integrity-validated** — staged to an **exclusively created, randomly named** temp file beside it (`tempfile.mkstemp`: `O_CREAT\|O_EXCL`, so it never truncates an existing file, never follows a symlink, and never collides with a sibling engine shard compressing the same directory), `fsync`ed, re-read **off disk**, decompressed and compared **byte-for-byte** against the original, renamed into place, and then **validated again at `<name>.gz` itself** — and it is that last check, on the bytes actually sitting where the log used to be, that authorizes removing the original. Any failure leaves the original **in place**, does not count it as compressed, and logs it; an existing `<name>.gz` is never clobbered. The archive inherits the source's mtime, so `app_log_days` still ages it out. Files over 64 MiB are skipped (the codec is in-memory), and so is a file whose archive would not be **smaller** than it (an empty or already-compressed log — compressing must never *cost* disk). Names/counts/sizes are logged, **never file content**. `0` = never compress. **No-op unless `[logging].log_dir` is set.** Set it **shorter** than `app_log_days` — a longer window compresses nothing, since the delete sweep runs first |
733718
| `search_preset_days` | int | `0` | past N days, **delete** saved-search presets (ADR 0136) neither used nor edited since the cutoff. The stored `criteria` is the operator's own content/`field_value` needle — **PHI-shaped by construction**, encrypted at rest — so it needs a window like any other PHI tier (ASVS 14.2.7). The whole **row** is deleted, not blanked: a preset's entire payload *is* its criteria. **Keys on last-USED** (BACKLOG #306) — the cutoff is compared against the *later* of `updated_at` (written by a save) and `last_used_at` (written by a recall), so a preset you run daily but never re-save is **kept**. A preset last touched before the `last_used_at` column existed has it NULL and ages out on `updated_at` alone. `0` = keep forever (the default) |

docs/PHI.md

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,47 +1021,12 @@ still default to `0`, but `serve` applies a posture gate on top of them:
10211021
- The canonical operator-facing home of the message-body window is now
10221022
**`[security].delete_message_bodies_after_days`**; its *model* default is 30, but the desugar
10231023
is **presence-gated** — only an EXPLICITLY-set switch is written through — so an **unset**
1024-
switch leaves `[retention].messages_days` at its own default, and the posture gate above
1025-
(auto-bound / refusal) is what actually bounds a *declared* PHI instance. An explicitly-set value
1026-
writes through onto `[retention].messages_days`. `[retention].dead_letter_days` stays at its own
1027-
home.
1028-
- **`[retention].messages_days` and `[retention].dead_letter_days` themselves default to `60`
1029-
days** (#1212), not to keep-forever. `0` still means keep-forever for anyone who sets it
1030-
explicitly — only the *default* moved. **The auto-bound above is tighter and takes precedence on a
1031-
PHI instance:** it keys on whether the field was *set*, not on its value, so an unset window on a
1032-
PHI instance still resolves to **30**. The 60 is therefore the floor for a **non-PHI** instance;
1033-
a PHI instance is held at 30. *(The two numbers come from two owner rulings — 30 on 2026-07-30 for
1034-
the PHI auto-bound, 60 on 2026-08-10 for the model default. Aligning them is a live question, not
1035-
a settled design; see #1212.)*
1024+
switch leaves `[retention].messages_days` at **0**, and the posture gate above (auto-bound /
1025+
refusal) is what actually bounds a PHI instance. An explicitly-set value writes through onto
1026+
`[retention].messages_days`. `[retention].dead_letter_days` stays at its own home.
10361027

10371028
So a PHI instance cannot run with PHI-body retention "off" without a loud, audited opt-out.
10381029

1039-
**Why a bounded default when the posture gate already exists.** The gate above binds a PHI instance
1040-
*that is declared as one*. The bounded default is what covers the case the gate cannot see — a
1041-
`data_class` that is absent, wrong, or drifts away from PHI while real PHI keeps arriving. In that
1042-
case the posture gate never fires and, with a keep-forever default, bodies would accumulate for the
1043-
life of the instance with nothing reporting it. The default is the floor under the gate, not a
1044-
replacement for it.
1045-
1046-
**The semantic flip, stated loudly because the config file does not change shape.** Before #1212 an
1047-
*omitted* window meant *keep forever*; afterwards the identical file means *purge at 60 days*. Same
1048-
config, opposite behaviour. There are zero deployments, which is precisely why the correct end state
1049-
is taken directly rather than staged behind a compatibility shim.
1050-
1051-
**Two further windows move with it, by inheritance rather than by their own defaults.**
1052-
`[retention].connection_event_retention_hours` and resolved operator-alert instances both inherit
1053-
`messages_days` when unset (ADR 0021 §7.5), so both become bounded at 60 days. Both are metadata
1054-
tiers that back no count, and both were previously unbounded on a defaulted config.
1055-
1056-
**And the runner now starts by default.** `RetentionRunner.enabled` is true when any window is set,
1057-
so a fully-defaulted engine previously left it off. It now runs. That is not a side effect to be
1058-
tidied away — a bound that nothing enforces is not a bound.
1059-
1060-
**`[retention].audit_days` is deliberately excluded and stays `0`.** The audit log is a
1061-
tamper-evident hash chain and HIPAA expects roughly six-year retention, so keep-forever there is the
1062-
correct behaviour. A sweep that "fixed all the zero defaults" would convert a compliance property
1063-
into a defect while appearing to finish the job.
1064-
10651030
**The pass itself.** It is **leader-gated twice** — at entry and again immediately before the purges, so
10661031
a node demoted mid-pass never nulls PHI as a stale ex-leader. An optional between-phase wall-clock cap
10671032
(`[retention].max_pass_seconds`, default 0 = off) bounds one pass: once hit, the remaining phases are

messagefoundry/config/settings.py

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1522,29 +1522,9 @@ def _non_negative_staleness(cls, value: float) -> float:
15221522
class RetentionSettings(_Section):
15231523
"""``[retention]`` — data-retention + SQLite maintenance (PHI.md §8, ASVS 14.2.x).
15241524
1525-
Enforced by the engine's :class:`~messagefoundry.pipeline.retention.RetentionRunner`.
1526-
1527-
**The two PHI-body windows are BOUNDED BY DEFAULT (60 days); every other window defaults to
1528-
``0``/``""`` = keep/off** (#1212). ``0`` still means keep-forever for anyone who sets it
1529-
explicitly — only the *default* moved. Two consequences follow and are deliberate, because a
1530-
bound nothing enforces is not a bound:
1531-
1532-
* The runner now **starts by default**, where a fully-defaulted config previously left it off.
1533-
* ``connection_event_retention_hours`` and resolved alert instances **inherit** ``messages_days``
1534-
when unset, so they are bounded at 60 days too rather than kept forever.
1535-
1536-
``audit_days`` is deliberately **excluded** and stays ``0``: the audit log is a tamper-evident
1537-
hash chain with a ~6-year HIPAA expectation, so keep-forever there is the correct behaviour and
1538-
not an unfixed default.
1539-
1540-
**THIS DEFAULT IS NOT WHAT A PHI INSTANCE RESOLVES TO.** ``serve``'s posture gate auto-bounds an
1541-
*unset* PHI-body window to **30** days (owner ruling 2026-07-30) and keys on
1542-
``model_fields_set`` -- whether the field was SET -- not on its value. So the 60 here never
1543-
softens that gate; it is the floor for a **non-PHI** instance, and every built-in environment
1544-
name derives PHI. Do not read this constant as "the shipped retention window" without checking
1545-
``__main__``'s gate.
1546-
1547-
A purge **NULLs the PHI *body*** of a message/dead-letter while **keeping the message ROW**
1525+
Enforced by the engine's :class:`~messagefoundry.pipeline.retention.RetentionRunner`. Every window
1526+
defaults to ``0``/``""`` = keep/off, so an existing deployment is unchanged until an operator opts
1527+
in. A purge **NULLs the PHI *body*** of a message/dead-letter while **keeping the message ROW**
15481528
(counts + disposition + audit stay intact — the Mirth Data-Pruner pattern); it never deletes a
15491529
``messages`` row, and never touches a body still in flight (at-least-once is preserved). The row
15501530
survives; its PHI *columns* do not. Tiers that carry nothing but PHI and back no count (transform
@@ -1554,14 +1534,11 @@ class RetentionSettings(_Section):
15541534
# Past N days, null inbound bodies (raw/summary/error/metadata) of fully-resolved messages,
15551535
# keeping the message ROW. `metadata` rides this same window (ASVS 14.2.7) — it is operator-
15561536
# attached PHI (#150 SetMeta), not disposition, so it can never outlive the body.
1557-
# 0 = keep forever; DEFAULT 60 (#1212) — unbounded PHI-body retention is not a safe default, and
1558-
# an operator who says nothing should get a bound rather than forever.
1559-
messages_days: int = 60
1537+
# 0 = keep forever.
1538+
messages_days: int = 0
15601539
# Past N days, null the bodies of DEAD (dead-lettered) outbound rows — their own window because a
1561-
# dead row stays replayable until its body is purged. 0 = keep forever; DEFAULT 60 (#1212), same
1562-
# reasoning. Kept a SEPARATE window rather than inheriting: a site that wants longer to replay a
1563-
# dead row must be able to say so without also extending live-body retention.
1564-
dead_letter_days: int = 60
1540+
# dead row stays replayable until its body is purged. 0 = keep forever.
1541+
dead_letter_days: int = 0
15651542
# Past N days, DELETE transform-state entries (ADR 0005) last written before the cutoff — keeps the
15661543
# in-memory state cache + table bounded. A simple global age purge; per-namespace policy is a
15671544
# follow-up. 0 = keep forever (the default — state correlation data is opt-in to purge).

tests/test_cli.py

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import pytest
1111

1212
from messagefoundry.__main__ import main
13-
from messagefoundry.config.settings import RetentionSettings
1413

1514
SAMPLES_CONFIG = Path(__file__).resolve().parents[1] / "samples" / "config"
1615
ADT_A01 = (
@@ -1696,20 +1695,6 @@ def test_serve_retention_auto_bounds_in_staging(
16961695
assert rc == 0
16971696
retention = captured["retention_settings"]
16981697
assert retention.messages_days == 30 and retention.dead_letter_days == 30 # type: ignore[attr-defined]
1699-
# #1212 INTERACTION, pinned so the two numbers cannot drift apart unnoticed. The model default is
1700-
# 60 and the PHI auto-bound is 30, from two separate owner rulings (60 on 2026-08-10, 30 on
1701-
# 2026-07-30). The gate keys on model_fields_set — whether the field was SET — so the model
1702-
# default never softens it, and 30 is what a PHI instance resolves to. If anyone aligns the two,
1703-
# this assertion fails and forces the docs in PHI.md §8 and CONFIGURATION.md to move with them
1704-
# rather than silently describing a layering that no longer exists.
1705-
assert RetentionSettings().messages_days == 60, (
1706-
"the #1212 model default moved; reconcile it with the 30-day PHI auto-bound above and "
1707-
"update PHI.md §8 + CONFIGURATION.md, which document the two as deliberately different"
1708-
)
1709-
assert retention.messages_days != RetentionSettings().messages_days, (
1710-
"the PHI auto-bound and the model default have converged, so this test no longer proves the "
1711-
"gate fired at all — it would pass identically if the auto-bound were deleted"
1712-
)
17131698
err = capsys.readouterr().err
17141699
assert "defaulted ON (30 days)" in err
17151700
# The AUTO-BOUNDED windows must not appear in the warn-only line — that is what "auto-bound rather

tests/test_embedded_document_pruning.py

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -351,31 +351,17 @@ async def test_audit_records_strip_counts(store: MessageStore) -> None:
351351

352352
async def test_runner_enabled_for_document_pruning_only(store: MessageStore) -> None:
353353
"""The runner starts (``enabled``) when an inbound sets ``prune_documents_after`` even with NO
354-
global ``[retention]`` window — document pruning has no global window.
355-
356-
BOTH arms zero the two PHI-body windows explicitly, and that is load-bearing rather than tidy.
357-
Since #1212 they default to 60, so a bare ``RetentionSettings()`` satisfies ``enabled``'s
358-
OR-chain on its own: the first assertion would pass WITHOUT the document-prune window
359-
contributing anything — testing nothing about the knob it names — and the second could never
360-
reach False at all. Zeroing them is what keeps this a test of ``prune_documents_after``.
361-
"""
362-
off_globals = {"messages_days": 0, "dead_letter_days": 0}
354+
``[retention]`` settings — document pruning has no global window."""
363355
reg = _registry()
364356
runner = RetentionRunner(
365-
store,
366-
RetentionSettings(**off_globals),
367-
clock=lambda: 5 * DAY,
368-
registry_source=lambda: reg,
357+
store, RetentionSettings(), clock=lambda: 5 * DAY, registry_source=lambda: reg
369358
)
370359
assert runner.enabled is True
371360

372361
# And with neither global retention nor any document-prune window, it is disabled.
373362
empty = Registry()
374363
empty.add_inbound(build_inbound_connection("IB", MLLP(port=2702), router="r"))
375364
off = RetentionRunner(
376-
store,
377-
RetentionSettings(**off_globals),
378-
clock=lambda: 5 * DAY,
379-
registry_source=lambda: empty,
365+
store, RetentionSettings(), clock=lambda: 5 * DAY, registry_source=lambda: empty
380366
)
381367
assert off.enabled is False

0 commit comments

Comments
 (0)