diff --git a/HANDOFF-ha-recheck.md b/HANDOFF-ha-recheck.md new file mode 100644 index 0000000..1d68bd0 --- /dev/null +++ b/HANDOFF-ha-recheck.md @@ -0,0 +1,127 @@ +# HANDOFF — HA construct re-check / ADR 0157 + +Written 2026-08-02 on owner stop-work (weekly usage cap). Branch `claude/ha-recheck-adr0157`. + +## 1. STATE + +| | | +|---|---| +| Branch | `claude/ha-recheck-adr0157` (follow-up branch; its predecessor merged as #129) | +| PR | **[#139](https://github.com/MEFORORG/MessageFoundry/pull/139)** — OPEN, all commits pushed | +| Uncommitted | none | +| Claim | `ha-recheck` (this worktree) | + +Earlier PR **#129 is MERGED** (`884036f8`, merged by the owner) — the doc corrections are on `main`. + +## 2. DONE + +| SHA | What | +|---|---| +| `bc9ccd73` | Corrected 5 classes of false split-brain claim in code + docs (in #129, on `main`) | +| `6c81c65e` | The third false-premise site `bc9ccd73` missed (in #129, on `main`) | +| `547b088b` | **ADR 0157** — demotion safety, Proposed, + index row (ledger gate passed) | +| `c20295c0` | **fix(delivery)** — leadership loss dead-lettered an un-sent row (strand) | +| `850e3f7b` | **ADR 0157 C3/C4 corrections** — both were strand-direction as written | + +`c20295c0` is mutation-verified in both directions: revert the body to `mark_failed` → both tests FAIL; +restore → both PASS. + +Gates at last run: `ruff check` clean · `ruff format --check` clean (1020 files) · `mypy` **no issues, +260 source files** · **125 passed** across dispatcher / pooled-rider / wiring / cluster suites. + +## 3. IN FLIGHT — nothing half-built in code. One spec ready to apply. + +**No partial edits exist.** The store's terminal-write path and the teardown ordering are untouched. + +**The applyable spec is saved (79 KB):** +`/ADR-0157-implementation-spec.md` + +It covers Inc 1 (Postgres fences, C1+C5+C4) and Inc 4/5 (bounded demotion, C6). It opens with a +**five-item STOP list** — read that first. Its line numbers are re-derived from source because +**the ADR's own line numbers are stale**. + +**Not yet designed:** the SQL Server increment. See §5 — the ADR's premise for it is wrong. + +## 4. BLOCKED ON + +**Nothing external.** The collision-gate deadlock is cleared: PR #133 merged, and I advanced the +**primary checkout** (it was 5 commits behind, 0 dirty, pure fast-forward), which is what actually puts +the fix in force — the hook resolves live from the primary, not from `main`. + +Verify before assuming: + +```bash +grep -c MatchedDirty /scripts/hooks/collision_gate.ps1 +``` + +Non-zero = in force. It was **2** when I checked. + +**Owner decisions are made** (C1 = terminal writes only, fail-open; C6 = yes, bounded, abandon-don't- +await). The remaining work is implementation against the corrected ADR. + +## 5. RETRACTIONS — things I asserted tonight that were wrong + +**These matter more than the wins. Every one was caught by a peer or by a check, never by me noticing.** + +1. **My own ADR's C3 was a strand.** I wrote that a fenced terminal write should roll back and leave the + row INFLIGHT for recovery. On SQL Server there is **no periodic in-flight recovery at all**, so that + is an unbounded strand — the exact outcome the ADR forbids, produced by its own fence. + **Corrected in `850e3f7b`:** roll back, then re-pend via unguarded `release_claimed`. +2. **My own ADR's C4 was a silent total halt.** "Delete the `set_leader_epoch(None)` clear" alone is + unsafe once C5 lands: `_reconcile_graph` has only two branches, so `is_leader() and running` matches + neither, forever, holding a stale epoch → a live leader claiming nothing, no exception, no alert. + **Corrected in `850e3f7b`:** delete the clear **and** re-stamp on every reconcile pass while leader. +3. **I amplified a wrong CI number.** I reported #133's windows-2025 leg as **28:14** and said the old + 26:00 cap "would have killed it by 134s", calling it the strongest number of the night. + **Wrong — that is the JOB; `step_timeout` gates the STEP, which was 24:51, UNDER the old cap by 69s.** + #133 was **not** a second casualty; **#119 remains the only PR that cap killed.** I accepted it + because it arrived as "a measurement correcting an estimate", which I treated as strictly improving. + It is not: *a measurement is better than an estimate only when it measures the same quantity.* +4. **I mis-attributed BACKLOG #333** to the ASVS-scorecard session. It is the **Public repo security + review** session's, filed in `b4665b10`. Verify attribution from `git log`, not from conversation. +5. **My merge-priority reasoning was wrong.** I argued #133 should merge before #129 "because it + unblocks everyone". It does not unblock anyone *on merge* — the gate resolves from the primary + checkout, so merging collects nothing until the primary advances. I had both measurements in hand + and built the ordering on the conclusion anyway. +6. **I overstated a tally.** I said "one unit confusion, four sessions, six retractions". Audited: the + job/step unit accounts for **three** of six. Exact form: *three of six from one unit confusion; all + six caught by a peer, none by the author.* +7. **ADR scope drift — measured at 142 of 510 lines (28%)** on a subject allocated to ADR 0158, after I + had written the guard against exactly that drift in my own notes. Trimmed to 319 lines. +8. **Four instrument errors**, three caught before acting: + - `Get-FileHash` over a shell-redirected temp file reported DIFFERS for byte-identical files (the + redirection re-encodes). `git hash-object` vs `git rev-parse origin/main:` is the like-for-like + form. I **nearly refuted a correct peer claim** with this. + - A `.git/…` path relative to a **linked worktree** resolves to nothing (`.git` is a *file* there). + `alloc.ps1` joins from `--git-common-dir`. I nearly reported a correct ledger as missing. + - A regex with `**` on the wrong side of a word reported a surviving correction as absent. + - **My first version of the `c20295c0` tests was vacuous** — asserting the row is `PENDING` with + `attempts=0` passes against the pre-fix code, because a seeded row is *already* in that state. + Only mutation testing caught it. Both tests now wait on a spy (positive signal). + +## 6. TRAPS + +- **`reset_stale_inflight` is startup/DR-only.** `RegistryRunner.reload()` is a quiesce-and-swap and + calls no recovery, so a reload leaves cancelled prefixes INFLIGHT until the next process start. + The codebase contradicts itself here: **`wiring_runner.py:1800` is right** ("strands INFLIGHT forever + … startup/DR-only"); **`:4718` is wrong** ("recovered … on the next start/reload"). + ➡️ **Therefore the ADR's SQL Server increment is mis-specified.** It proposes an owner-blind, + age-based periodic sweep. The verified defect is narrower — *no recovery at graph re-start* — and the + right fix is a scoped reset there. An age sweep on SQL Server has **no `owner` column populated** to + discriminate with, so it would re-pend live rows. **Do not build the ADR's version as written.** +- **Local `pytest` SILENTLY SKIPS the Postgres and SQL Server legs.** 97 skips in my last run. A green + local run proves nothing about either backend. +- **`mypy` reports 21 phantom errors** unless the venv has the full extras. Bootstrap with + `pip install --constraint constraints.lock -e ".[dev,harness,postgres,sqlserver,fhir,dicom,x12,xml,webauthn,sftp,vault,otel]"`. +- **Pre-commit needs `ruff` on PATH** in this worktree, or the ruff hooks fail with "Executable not + found". Activate `.venv` or prepend the main checkout's. **Never `--no-verify`.** +- **The ADR's line numbers are stale.** Use the spec's re-derived ones. +- **Three parties have touched `wiring_runner.py`** (this fix, PR #136, an ASVS anchor). All far apart, + but whoever rebases last should read it rather than trust a clean auto-merge. +- **`git status` rewrites the index of the repo it inspects** — `overlap.ps1` now uses + `--no-optional-locks`. Relevant if you write tooling that walks peer worktrees. + +## 7. NEXT STEP + +Apply the spec's Inc 1 (Postgres fences) against the **corrected** C3/C4, then Inc 4/5. Re-scope the +SQL Server increment per §6 before building it. Everything is decided; nothing is ambiguous. diff --git a/docs/ASVS-L2-PHASE0-CHANGES.md b/docs/ASVS-L2-PHASE0-CHANGES.md index 474cfaa..aa40278 100644 --- a/docs/ASVS-L2-PHASE0-CHANGES.md +++ b/docs/ASVS-L2-PHASE0-CHANGES.md @@ -314,8 +314,8 @@ tables in [`CONNECTIONS.md`](CONNECTIONS.md) §"Resource management & limits" (A | DICOMweb STOW-RS destination (`dicomweb`) | outbound | HTTPS; port from the base URL (ADR 0025 Phase 2) | `verify_tls` default true (false needs `MEFOR_ALLOW_INSECURE_TLS`) | static bearer or HTTP Basic | **yes** — `url` per Connection | `DICOMweb(url, study_uid, timeout_seconds, verify_tls)`, `[egress].allowed_http` | | DICOM C-STORE SCP (`dimse`) | inbound | DICOM upper layer over TCP, **default port 104**, bound to `[inbound].bind_host` | opt-in TLS with opt-in mTLS via `tls_ca_file`; a **non-loopback cleartext SCP is refused at start** unless `serve --allow-insecure-bind` | `calling_ae_allowlist` (AE-Title gate) + `require_called_ae_title` (default true) + the `[inbound].source_ip_allowlist` IP gate | bind host is `[inbound].bind_host`; the modality dials in | `DICOM(ae_title, port, calling_ae_allowlist, require_called_ae_title, max_associations, max_pdu_size, max_object_bytes, timeout_seconds)` | | DICOM C-STORE SCU + C-ECHO destination (`dimse`) | outbound | DICOM upper layer over TCP, **default port 104** | opt-in TLS (`tls`, `tls_allow_expired`) | the association's calling / `called_ae_title` AE Titles | **yes** — `host`/`port`/`called_ae_title` per Connection | `DICOM(host, port, called_ae_title, timeout_seconds, connect_timeout)`, `[egress].allowed_tcp` | -| SMTP email destination (`email`) | outbound | SMTP, **default port 587** (STARTTLS submission); port 465 selects implicit TLS (ADR 0029) | `use_tls` default true (STARTTLS); false needs `MEFOR_ALLOW_INSECURE_TLS` | optional SMTP AUTH `username` / `password` (`env()`) | **yes** — `host`/`port` per Connection | `Email(host, port, sender, recipients, use_tls, username, password, timeout_seconds)`, `[egress].allowed_smtp` | -| Direct-Project S/MIME destination (`direct`) | outbound | SMTP to a HISP relay, **default port 587** (465 = implicit TLS), ADR 0085 | STARTTLS on by default; the sign-then-encrypt S/MIME body protects PHI **independently of** session TLS | the sender's S/MIME `signing_key` + `signing_cert` (+ optional `signing_key_password`), the partner `recipient_cert` chaining to `trust_anchor`, plus optional SMTP AUTH | **yes** — `host` / `recipient_cert` / `trust_anchor` per Connection | `Direct(host, port, signing_key, signing_cert, recipient_cert, trust_anchor, use_tls, timeout_seconds)`, `[egress].allowed_direct` | +| SMTP email destination (`email`) | outbound | SMTP, **default port 587** (STARTTLS submission); port 465 selects implicit TLS (ADR 0029) | `use_tls` default true (STARTTLS); false needs `MEFOR_ALLOW_INSECURE_TLS`. The server certificate **is verified** (`tls_verify` default true, #323) — chain + hostname + strict RFC 5280, anchored to the OS roots, a per-connection `tls_ca_file`, or `[tls].internal_ca_file`; `tls_verify=false` needs the **clamped** escape and also refuses SMTP AUTH | optional SMTP AUTH `username` / `password` (`env()`) | **yes** — `host`/`port` per Connection | `Email(host, port, sender, recipients, use_tls, username, password, timeout_seconds)`, `[egress].allowed_smtp` | +| Direct-Project S/MIME destination (`direct`) | outbound | SMTP to a HISP relay, **default port 587** (465 = implicit TLS), ADR 0085 | STARTTLS on by default **and the relay certificate is verified** (`tls_verify` default true, #323; `tls_ca_file` is the TLS-hop CA, distinct from `trust_anchor`, which is the partner's S/MIME CA); the sign-then-encrypt S/MIME body protects PHI **independently of** session TLS | the sender's S/MIME `signing_key` + `signing_cert` (+ optional `signing_key_password`), the partner `recipient_cert` chaining to `trust_anchor`, plus optional SMTP AUTH | **yes** — `host` / `recipient_cert` / `trust_anchor` per Connection | `Direct(host, port, signing_key, signing_cert, recipient_cert, trust_anchor, use_tls, timeout_seconds)`, `[egress].allowed_direct` | | DATABASE destination, poll source, and `db_lookup` read connection (`database`) | outbound (the poll source also dials out, for inbound data) | ODBC — TDS for the `sqlserver` dialect, **default port 1433** | the `sqlserver` dialect enforces Driver-18 TLS (`encrypt` default true, `trust_server_certificate` default false); the `generic` dialect delegates TLS to the operator's `odbc_params` | SQL / Integrated / Entra; statements are parameterized | **yes** — `server` / `database` / statement per Connection | `Database(...)`, `DatabasePoll(...)`, `DatabaseLookup(...)` with `connect_timeout`, `pool_max`, `acquire_timeout`; `[egress].allowed_db` | | Reference-set sync dial-out — `DatabaseRef` (`database`) | outbound (periodic) | ODBC, **default port 1433** (ADR 0006) | as above | as above | **yes** — `server` / `statement` per reference set | `DatabaseRef(...)` + `Reference(name, refresh_seconds)`; `[egress].allowed_db` | | Internal sources that **open no socket**: `timer`, `loopback`, `passthrough` | inbound (internal only) | none — clock-driven (ADR 0011), re-ingress-only (ADR 0013), and Handler-fed pass-through respectively | n/a | n/a — they reach no external system | no | `Timer(...)`, `Loopback()`, `PassThrough()` | diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 43d8a66..c228972 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5261,7 +5261,9 @@ sourced — **#1 (SQL Server concurrency)** and **#2 (console off-thread)** — **Why:** Real gap. The alert SMTP sink (EmailTransport in pipeline/alert_sinks.py) calls starttls() with the default SSL context and exposes only email_use_tls plus the global MEFOR_ALLOW_INSECURE_TLS cleartext escape, so there is no per-mail-server option to keep TLS on yet unconditionally trust a self-signed/mismatched certificate. -**Why it is an anti-feature:** unconditionally trusting an SMTP server's certificate defeats TLS. The engine's `EmailAlertSink` uses STARTTLS with a verifying context by design; the only escape (`MEFOR_ALLOW_INSECURE_TLS`) is global and deliberately loud. Recorded for parity completeness, not as a want. +**Why it is an anti-feature:** unconditionally trusting an SMTP server's certificate defeats TLS. The only escape (`MEFOR_ALLOW_INSECURE_TLS`) is global and deliberately loud. Recorded for parity completeness, not as a want. + +> ⚠️ **CORRECTED 2026-08-01 — this item previously asserted "The engine's `EmailAlertSink` uses STARTTLS with a verifying context by design." That was FALSE**, and it is the shape [`CLAUDE.md`](../CLAUDE.md) §11 names as worst: *a compensating control resting on a false premise*. `smtplib.starttls()` with no context falls back to `ssl._create_stdlib_context`, which **is** `ssl._create_unverified_context` (`CERT_NONE`, `check_hostname=False`) — so the alert sink was encrypting without authenticating, and a reader of this item would have concluded alert email was TLS-verified when it was not. #323 fixed the two **connectors** (`EmailDestination` / `DirectDestination`); it did **not** fix the alert sink, so the **Why:** paragraph above remains accurate and this item's premise stays false until #323's alerts-cell residual lands. Do not re-assert verification here before then. **Nearest existing mechanism:** EmailTransport / send_plain_email in pipeline/alert_sinks.py (SMTP alert sink, built by notifier_from_settings from AlertsSettings in config/settings.py); its only TLS knob is email_use_tls (STARTTLS on/off) plus the global MEFOR_ALLOW_INSECURE_TLS / insecure_tls_allowed() escape, which permits CLEARTEXT SMTP — not a keep-TLS-but-trust-any-cert override. @@ -7397,7 +7399,15 @@ MEFOR_FORBIDDEN_TOKENS=scripts/security/scan-tokens.local.txt.example \ ## 323. SMTP TLS is unverified on all three send paths -> 🔢 **Filed 2026-08-01 — not started.** Value **8/10** · Difficulty **4/10** · _fill-in_. All three SMTP send paths call `starttls()` / `SMTP_SSL()` with **no** SSL context, so Python 3.14's stdlib default applies (`CERT_NONE`, `check_hostname=False`) — the EMAIL destination puts Handler PHI *and* the SMTP AUTH password over an encrypted-but-unauthenticated hop, while the #201 revocation guard, the cleartext-credential rule and `DEPLOYMENT.md` all describe that same hop as **verified**. +> 🚧 **Status 2026-08-01 — PARTIALLY SHIPPED, 2 of 3 cells** (PR #132). The two **connectors** verify: `EmailDestination` and `DirectDestination` build an explicit verifying context via the new `tls_policy.build_smtp_tls_context()` and pass it on both `smtplib` arms, with per-connection `tls_verify` / `tls_ca_file` / `tls_check_hostname` on the `Email()` and `Direct()` factories. Proven by **negative control**: with the code change stashed and the tests kept, all eight new assertions in `tests/test_email_destination.py` go **red** — the pre-existing "STARTTLS was issued" assertions stayed green throughout the insecure period and could never have caught this. ⚠️ **The alerts cell is NOT fixed**: `pipeline/alert_sinks.py:384` still calls `smtp.starttls()` bare, so alert + security-notify email remains unverified. That is the residual below, and it is why [#139](#139)'s premise is still false. +> +> 🔢 **Filed 2026-08-01.** Value **8/10** · Difficulty **4/10** · _fill-in_. All SMTP send paths called `starttls()` / `SMTP_SSL()` with **no** SSL context, so Python 3.14's stdlib default applied (`ssl._create_stdlib_context` **is** `ssl._create_unverified_context` — `CERT_NONE`, `check_hostname=False`) — the EMAIL destination put Handler PHI *and* the SMTP AUTH password over an encrypted-but-unauthenticated hop, while the #201 revocation guard, the cleartext-credential rule and `DEPLOYMENT.md` all described that same hop as **verified**. + +**Residual — the alerts cell (~1 layer).** `pipeline/alert_sinks.py` `send_plain_email` and `pipeline/security_notify.py` need the same context, plumbed from new `[alerts].email_tls_verify` / `email_tls_ca_file`, plus a `[security].allow_unverified_alert_smtp_tls` acknowledgment switch at the serve gate. It needs an **acknowledgment switch rather than the clamp** because the contextvar hop posture is never stamped for that cell — which is why it was deferred rather than folded in. The shared `refuse_unverified_smtp_tls()` helper belongs in `config/settings.py` at that point; the connectors currently **inline** the refusal, matching the `mllp.py` / `remotefile.py` house style. Then: register the deviation in `security_loosenings()` (see [#333](#333)), add a `checks.py` advisory, and correct `docs/PHI.md` and [#139](#139). + +**Also landed, called out rather than folded in silently.** `transports/direct.py`'s cleartext arm read the **unclamped** `insecure_tls_allowed()` while its sibling arm one branch away read the clamped `weakened_tls_escape_permitted_here()`. Two different escapes in one connector is how the next bug gets written, so it now reads the clamped one. Strictly **adds** refusals (ADR 0092 decision 5). Partially closes the [#329](#329) concern. + +**Correction to this item's own text.** The "Migration risk, stated plainly" framing filed with this item presumed existing deployments. The owner confirmed on 2026-08-01 that **there are none**, so secure-by-default was simply correct and no phased rollout, CHANGELOG breaking entry or migration guide was warranted. Do not resurrect that framing from this item's history. **Cluster:** Security & Compliance. **Priority:** P1. **Verdict:** build. **Severity:** high. @@ -7759,6 +7769,12 @@ What it *is*: the realistic failure is a dev/CI environment variable riding into The AI-broker cell has an additional argument: `transports/smart.py:126-149` moved the *same* question — a credential on a cleartext token endpoint — off the raw escape and onto `refuse_cleartext_credential_hop` in commit `a3015196`, with a comment describing exactly this defect (*"It used to read the raw, UNCLAMPED `MEFOR_ALLOW_INSECURE_TLS`"*). `ai_broker.py:140` is the un-migrated twin of a cell fixed days ago. +**Additionally — converting all five is what makes the property *checkable*, not just true.** While these five remain, "no unclamped escape survives on an enforcing PHI posture" is five separate per-site facts, each verifiable only by opening the site and reading it, and each silently falsified by a sixth cell added later. Convert them all and it collapses into **one repo-wide invariant**: the raw `insecure_tls_allowed()` becomes unreachable outside `config/settings.py`'s own clamp, so the absence of the raw predicate is checkable everywhere at once, with `weakened_tls_escape_permitted_here` as the thing that must still be present. Today the property is a convention enforced by review; afterwards it is an invariant enforced by a grep — and a *new* unclamped cell fails immediately instead of waiting for the next audit to enumerate it. + +That distinction matters concretely for the ASVS record. The scorecard's absence-claim mechanism runs regexes over the whole `*.py` corpus and **cannot scope a grep to one file**, so a per-connector claim ("`direct.py`'s escape is clamped") is not expressible and has to be carried as stated-but-unchecked prose. A repo-wide claim is expressible and machine-verified on every commit. So this item is not only five leaks to plug: it is the difference between a security property that must be re-audited by hand and one that a gate can hold. *(Framing contributed by the ADR 0156 ASVS-sweep session, 2026-08-02.)* + +**Scope note, because the count is moving and two censuses will disagree.** #323 routes `transports/direct.py` and `transports/email.py` through the clamp, taking the remaining set to four once it lands — so a census taken on that branch disagrees with one taken on `main`, and neither is wrong. Measured at `main` by counting **`ast.Call` nodes**, not matching lines: six real call sites outside `config/settings.py` — `auth/ldap.py`, `pipeline/alert_sinks.py`, `transports/ai_broker.py`, `transports/database.py`, `transports/direct.py`, `transports/remotefile.py`. `transports/database.py` is the documented unstamped fallback, excluded above; `transports/mllp.py` matches a naive grep for the raw name but its occurrence is **prose inside a docstring, not a call at all**. A line-based census reports it as a further site; an AST-based one does not — which is the instrument distinction, not a detail about this item. + **Proposed:** convert all five, but note that a blanket swap to `weakened_tls_escape_permitted_here()` would silently fix only two of them. 1. **In-gate cells — a one-line swap each.** `remotefile.py:375` and `direct.py:170` are built inside `build_check_registry`/`wiring_runner`'s `active_hop_posture` scope (`config/tls_policy.py:587-603`; the stamping sites are all in `pipeline/wiring_runner.py`), so `weakened_tls_escape_permitted_here()` reads a real posture there — byte-identical to how `remotefile.py:176`/`:577` and `email.py:134` already behave. @@ -8178,7 +8194,9 @@ ADR 0144:193-195 records the decorated-scope trade, but justifies it with an **` **The third gap the audit named is narrower than described.** The non-recursive `base.glob("*.py")` at `checks.py:893`/`:898` (ADR 0144:196) is **not** an unscanned execution path. `load_config` globs `directory.glob("*.py")` non-recursively too (`config/wiring.py:3969`, and `:4392` for `validate_config`), and `_SiblingHelperFinder.find_spec` returns `None` for any dotted name and serves only `_`-prefixed top-level helpers from the config dir (`wiring.py:3902`, `:3908-3912`). A `.py` in a config subdirectory is therefore neither executed by the loader nor importable by a sibling — and `_assert_safe_config_source` is non-recursive for the same reason (`wiring.py:4194`, `:4324`). The lint's file set already equals the executable set. A recursive walk here would make the lint report on files the safe-source ownership gate never vets — an asymmetry in the other direction. #226 (`docs/BACKLOG.md:6917`) already parks recursion as a *loader* question; it belongs there, not here. -**Why:** Bounded, and bounded hard. The lint is advisory by default (`checks.py:953`, `ok=not strict, required=strict`), so a finding blocks nobody unless an adopter opts into `--strict-handler-security` on their own CI. It governs code the adopter's own administrator authors, inside a directory whose write access is already the trust boundary (`_assert_safe_config_source`, `wiring.py:4194`/`:4324`) — anyone who can drop a `.py` there already has arbitrary in-process execution under the engine account, so this is **not** a privilege boundary and evading it buys an attacker nothing they did not already have. ADR 0144:171-174 and the `_check_handler_security` docstring (`checks.py:878`) both say so: "a filter, not a fix." There is no PHI-exposure path and no runtime behaviour change of any kind. +**Why:** Bounded, and bounded hard. The lint is advisory by default (`checks.py:953`, `ok=not strict, required=strict`), so a finding blocks nobody unless an adopter opts into `--strict-handler-security` on their own CI. It governs code the adopter's own administrator authors, inside a directory whose write access is already the trust boundary (`_assert_safe_config_source`, `wiring.py:4194`/`:4324`) — anyone who can drop a `.py` there already has arbitrary in-process execution under the engine account, so this is **not** a privilege boundary and evading it buys an attacker nothing they did not already have. + +> ⚠️ **Rationale amended 2026-08-01 (ADR 0087 sandbox session) — the severity is right, the reason was not.** "The author already has in-process execution" is true at the **default** `[sandbox].mode=off`, and **false** under `mode=subprocess`, where the entire premise is that the author is *not* trusted with it. A severity floor resting on a posture-specific claim reads as settled and misleads the next reader. The rationale that holds in **both** postures: the lint is advisory and pre-deployment; under `mode=off` the author already has in-process execution, and under `mode=subprocess` an evasion still only reaches **host** actions the sandbox does not confine — `DEFAULT_FORBIDDEN_MODULES` (`pipeline/sandbox.py:84-95`) blocks `socket`, `ssl`, `asyncio`, `multiprocessing`, the I/O-bearing `messagefoundry.*` subpackages and `cryptography`, but **not `os` or `subprocess`** (verified at HEAD). ADR 0087 confines the **address space** (the child cannot reach the parent's DEK, audit chain or sockets), not the **host**; OS-level default-deny is ADR 0147, *Proposed with no code*. So an evasion reaches neither the DEK nor the audit chain in either posture. **Re-score upward when ADR 0147 lands**, at which point the lint becomes load-bearing for exactly the class OS confinement is meant to close. ADR 0144:171-174 and the `_check_handler_security` docstring (`checks.py:878`) both say so: "a filter, not a fix." There is no PHI-exposure path and no runtime behaviour change of any kind. What it *is*: an adopter who turns on the strict gate gets a **green build** on a Handler containing `getattr(os, "system")`, and gets a green build on a transforms helper logging `msg.raw` at INFO. Gap (2) is the one that actually costs something, because the miss is not a malicious bypass — it is the ordinary fallible-author case ADR 0144 exists for, landing in the exact file the project's own layout guidance created. Gap (1) is mostly a claim-hygiene problem: the ADR asserts the false negative in prose and no test proves it, so nobody notices if a future change silently widens or narrows it. diff --git a/docs/PHI.md b/docs/PHI.md index 974e20d..09c50ab 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -913,7 +913,7 @@ with materially different PHI profiles, so they get their own rows; stream 4 is | **8. `alert_instance` table — default on wherever an `[alerts]` notifier exists** | resolvable operator alerts: `connection_stopped`, `queue_buildup`, `lane_stuck`, `message_stall`, `saturation`, `connection_error`, `content_match`, `storage_threshold`, `cert_expiry`, `secret_rotation`, `bootstrap_admin_expiring` (the UNCLAIMED first-run bootstrap admin nearing its auto-disable deadline — ASVS 6.4.5; its payload carries only the ISO deadline plus whole hours remaining, never the password or any secret), `integrity_drift`, `update_available`, `backup_failed`, `rcsi_off_degraded`, `leadership_acquired`, `dr_activated`, `gcm_invocations` (the per-key AES-GCM invocation bound crossing its 2^31 soft warn — ASVS 11.3.4; its payload carries a one-way `key_id` fingerprint plus counters, never key bytes) The three reachable **inverse** signals — `connection_restored`, `leadership_lost`, `dr_released` — are never rows here: `_record_state` routes an inverse through `_AUTO_RESOLVE` to `resolve_alert_instances_for`, never to `upsert_alert_instance`. (A fourth mapped key, `connection_started`, is emitted by no code path today.) | rows: `event_type`, `connection`, `severity`, `status`, `first_seen`, `last_seen`, `count`, `reason`, `acked_by`, `acked_at`, `resolved_at`, `suspended_until`, `escalation_tier` | the store database, **all three backends** | the operator alert list — acknowledge / resolve / suspend. Durable state is recorded **before** any suppression or throttle return, so a muted alert still leaves a record | `GET /alerts/active` under **`monitoring:diagnose`** (**not** a PHI permission) with the same per-channel scope; ack/resolve/suspend/**resume** are POSTs on the same tier, and the separate read-only `GET /alerts/rules` view sits on its own gate | shares the connection-event window; **only RESOLVED instances are DELETEd**, by `resolved_at` — an open or acknowledged condition is never aged out from under an operator | **`reason` is free text** taken from the event's `detail`/`reason`/`label`: `safe_text(reason)[:200]` then cipher-encrypted (AAD `("alert_instance","reason",event_type,connection)` — the de-dup grain, so one AAD covers both the INSERT and the re-fire UPDATE). `content_match` is **PHI-free by contract**: the sink method takes no value parameter, only the connection, an operator label and an optional rule id | | **9. `response` rows with `kind='ack_sent'` — DEFAULT ON** (`[diagnostics].response_sent = true`) | the ACK/NAK the engine returned to an inbound sender, under a sentinel destination `\x1fack:` | rows: `ack_code` (`AA`/`AE`/`AR`/`CA`/`CE`/`CR`), `ack_phase` (`decode`/`parse`/`strict`/`ingest`), `outcome`, `body`, `detail` | the store database | "what did we actually reply, and why" — the operator's answer to a sender disputing an ACK | `GET /messages/{id}/responses` under `messages:read` + `require_phi_read`; the `body` only for a caller who also holds `messages:view_raw`; every read writes a `response.read` audit row | `body`, `detail` and `resp_headers` are set to `NULL` in place by `purge_message_bodies` on the message-body window, on all three backends | **PHI fail-safe:** the ACK **body** is stored **only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext — and every NAK passes no body at all, so the offending field value is never persisted. The disposition metadata (`ack_code`/`ack_phase`/`outcome`) is non-PHI and always captured; `detail` is `safe_text`-scrubbed, 200-char bounded and encrypted | | **10. `[alerts]` webhook transport** (off by default — `webhook_url` unset) | one HTTPS POST per alert, carrying every non-underscore event key as JSON | JSON | the operator's webhook endpoint (Slack/Teams/PagerDuty/custom) | operator notification | **`https` only** — a plaintext `http://` webhook URL is refused at construction unless the raw `MEFOR_ALLOW_INSECURE_TLS` escape is set (and then a warning is logged); note this path reads the **unclamped** escape, unlike the connectors. Redirects are refused; an optional `webhook_allowed_hosts` egress allowlist gates the host | the endpoint's | **carries the alert's `detail`/`reason` free text** (`safe_exc()`-scrubbed at the emit sites, but **not** re-run through `safe_text` on this path). Internal `_`-prefixed keys (per-rule recipients, rule id, cooldown) are stripped before send, so recipient addresses never cross the wire | -| **11. `[alerts]` SMTP transport — operator alert list** (off unless `email_smtp_host` + `email_from` + ≥1 `email_to`) | one email per alert; default subject `[MessageFoundry] — `, default body every non-underscore event key as `k: v` | plain text (always kept — never HTML-only); optional HTML alternative | the operators' mailboxes | operator notification | `smtp_allowed_hosts` egress allowlist; the SMTP password comes from `MEFOR_ALERTS_EMAIL_PASSWORD` or a `[secrets]` provider, never the config file; per-send timeout `email_timeout` | the mail system's | carries the same `detail`/`reason` free text as the webhook. #138 operator templates are constrained to a **closed non-PHI variable allowlist** validated fail-closed at config load. **Transport caveat, stated plainly:** `send_plain_email` calls `smtp.starttls()` with **no SSL context**, so Python's stdlib default applies — `check_hostname = False`, `verify_mode = CERT_NONE`. The hop is therefore **encrypted but unauthenticated (MITM-able)** when `email_use_tls` is true (the default), and cleartext when it is false. There is **no hop gradient or attestation on this path**: PR #1163 hardened the EMAIL *message destination* connector, not the `[alerts]` SMTP path | +| **11. `[alerts]` SMTP transport — operator alert list** (off unless `email_smtp_host` + `email_from` + ≥1 `email_to`) | one email per alert; default subject `[MessageFoundry] — `, default body every non-underscore event key as `k: v` | plain text (always kept — never HTML-only); optional HTML alternative | the operators' mailboxes | operator notification | `smtp_allowed_hosts` egress allowlist; the SMTP password comes from `MEFOR_ALERTS_EMAIL_PASSWORD` or a `[secrets]` provider, never the config file; per-send timeout `email_timeout` | the mail system's | carries the same `detail`/`reason` free text as the webhook. #138 operator templates are constrained to a **closed non-PHI variable allowlist** validated fail-closed at config load. **Transport caveat, stated plainly — and this path is now the ONLY one left:** `send_plain_email` calls `smtp.starttls()` with **no SSL context**, so Python's stdlib default applies (`ssl._create_stdlib_context` **is** `ssl._create_unverified_context` — `check_hostname = False`, `verify_mode = CERT_NONE`). The hop is therefore **encrypted but unauthenticated (MITM-able)** when `email_use_tls` is true (the default), and cleartext when it is false. There is **no hop gradient or attestation on this path**. ⚠️ **Do not generalise this row to the message connectors.** [#323](BACKLOG.md) fixed the **EMAIL** and **DIRECT** *message destinations* — both now build an explicit verifying context (chain + hostname + strict RFC 5280, anchored to the OS roots, a per-connection `tls_ca_file`, or `[tls].internal_ca_file`), with `tls_verify=false` refused unless the **clamped** escape permits it. The `[alerts]` cell was deliberately deferred because it needs an acknowledgment switch rather than that clamp (the contextvar hop posture is never stamped here), so it is the residual on #323 — not an oversight, and not evidence that SMTP is unverified engine-wide | | **12. Per-user security-event SMTP notifier** — **posture-mandatory on a PHI instance** | `account_locked`, `login_after_failures`, `password_changed`, `password_reset`, `email_changed`, `roles_changed`, `account_disabled`, `mfa_enabled`, `mfa_disabled`, `admin_action_new_ip` | plain-text email | the **affected user's own** mailbox | ASVS 6.3.5 / 6.3.7 out-of-band notification of security-relevant account changes | shares stream 11's SMTP transport and therefore its caveat. On a PHI instance with auth enabled `serve` **refuses to start (exit 2) under `[security].enforcement = enforce`** when no effective channel exists; the explicit, **audited** opt-out is `[alerts].security_notifications_required = false` | the mail system's | the body carries the account username, a fixed description, optionally the failed-attempt count or the new email on file, and the source IP — **no message data, no secrets**. Dispatch is a bounded background queue; a failed send is logged, never raised (the event is still in `audit_log`) | | **13. `LoggingAlertSink` fallback** (when no `[alerts]` transport is configured) | every alert **this state-less sink implements**, at `WARNING` — `leadership_lost` / `dr_released` at `INFO`, and `connection_restored` is a **deliberate no-op** (a recovery needs no page and there is no instance to auto-resolve), so a lane recovery produces no record on this stream at all. `content_match` exists only on `NotifierAlertSink` and has no fallback-path record | — | folds into stream 1 | so alerts are never silent | inherits stream 1's | inherits stream 1's | includes the `detail`/`reason` free text, and therefore inherits stream 1's filters, ACL, forwarder and retention | diff --git a/docs/adr/0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md b/docs/adr/0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md new file mode 100644 index 0000000..49eca5c --- /dev/null +++ b/docs/adr/0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md @@ -0,0 +1,457 @@ +# ADR 0157 — Demotion safety: fence scope on post-claim writes, and a bounded graph stop + +**Status:** Proposed **Date:** 2026-08-01 + +> Proposed only. Nothing here is built. The two questions that need an owner decision are stated in +> **Decision** as C1 (do post-claim writes carry a precondition, and which ones) and C6 (does demotion +> get an enforced deadline, and what happens to an inbound that cannot meet it). + +--- + +## Context — what the re-check found + +The HA construct is **active-passive only**: N engine processes against one shared server database, one +leader plus warm standbys, no broker. Gated on `[cluster].enabled`, `[store].backend` in +`{postgres, sqlserver}` (SQLite rejected at config load), `[store].pool_size >= 2`. Engine sharding and +`[cluster]` are mutually exclusive and fail closed ([`__main__.py:2372`](../../messagefoundry/__main__.py)). + +A re-check of the leadership-lease construct found the lease algebra **sound** — expiry is evaluated on +the DB clock on both backends, acquire/renew is one atomic statement, and the `leader_epoch` fencing +token is real and genuinely checked inside the claim transaction. Scope B (failover vs the count-and-log +invariant) and scope C (Postgres/SQL Server divergence) were probed and cleared, not assumed. + +**The invariant that decides everything:** at-least-once **permits duplication and forbids stranding or +loss**. A change converting a possible strand into a possible duplicate is an improvement; the reverse is +unacceptable however elegant. + +### F1 — the epoch fence guards *some* claims, and nothing after them + +The guard is `AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$8) <= $9`, spliced +only when a held epoch is cached: [`postgres.py:2694`](../../messagefoundry/store/postgres.py) and the +sibling FIFO claims; SQL Server twins in [`sqlserver.py`](../../messagefoundry/store/sqlserver.py). + +**"The claim is fenced" is true only of the FIFO claims.** `claim_ready` — the UNORDERED path — carries +no epoch predicate on **either** backend. + +Post-claim writes resolve by bare id, unguarded: + +| write | Postgres | note | +|---|---|---| +| `dead_letter_now` | `:3179-3187` (`WHERE id=$5`) | assigns a **terminal** disposition, then calls `_maybe_finalize_message` | +| `mark_done` | `:3197-3203` (`WHERE id=$3`) | | +| `mark_failed` | `:4125-4133` (`WHERE id=$5`) | decides dead-letter from an `attempts` the *successor* may have incremented | +| `complete_with_response` | `:3288-3294` | not idempotent — inserts a fresh response artifact | + +Contrast [`release_claimed`](../../messagefoundry/store/postgres.py) at `:3098-3104`, two methods away, +which **does** carry `AND status=$4`. + +The sharp write is not `mark_done`. It is `dead_letter_now` — a demoted node assigning a terminal +disposition and finalizing the message, breaching *"the store finalizer is the single authority"* +(CLAUDE.md §2). A DEAD row is never re-claimed, so H2 skip-and-complete cannot heal it. + +Also unguarded: the cross-owner stranded-lease reclaim that is the **first** statement of each FIFO claim +transaction (`postgres.py:2981-2991` and twins). An epoch-rejected claim returns an empty result set +rather than raising, so the transaction still **commits** the re-pend. + +### F2 — demotion budgets detection only, never the stop + +`_check_fence` ([`cluster.py`](../../messagefoundry/pipeline/cluster.py)) sets `_is_leader = False` and +`_leader_epoch = None` and nothing else. It cancels no listener, no worker, no in-flight send. + +**The real budget on stock defaults** (heartbeat 10.0, fence 20.0, ttl 30.0): + +- detection = fence 20.0 + up to one `_fence_tick` (1.0) + up to one graph poll (1.0) → **20–22 s** +- lease expires at **30 s on the DB clock** +- ⇒ **≈ 8.0 s**, *minus* the renew round-trip remainder. The fence baseline is stamped **after** the renew + returns, and that round trip is bounded only by `[store].command_timeout = 30` + ([`settings.py:505`](../../messagefoundry/config/settings.py)) — which **exactly equals** + `leader_lease_ttl_seconds = 30.0` (`settings.py:2904`). `_fence_ordering` relates heartbeat/fence/ttl + only. **The margin can be zero or negative and no validator notices.** + +Teardown cost against that ~8.0 s: `_teardown_unsafe` sets `_stop` +([`wiring_runner.py:2481`](../../messagefoundry/pipeline/wiring_runner.py)) then runs the **sequential** +source loop at `:2497-2498` *before* the dispatchers at `:2505-2508`. Each socket listener costs up to +**10.0 s** — 5.0 client grace plus 5.0 `server.wait_closed()`, both off `_CLIENT_SHUTDOWN_GRACE = 5.0` +([`mllp.py:111`](../../messagefoundry/transports/mllp.py)), a module constant with no config surface and +no relation to lease timing. Every File/RemoteFile/Database/DICOM inbound is **unbounded**: `_stop.set()` +then `await asyncio.gather(self._task, return_exceptions=True)` with no cancel and no timeout +([`file.py:448-454`](../../messagefoundry/transports/file.py)). `_stop_graph` wraps nothing in `wait_for`. + +So **one** blocked socket inbound already exceeds the budget; N serialize linearly, and the project +targets **1,500 connections**. On the Windows/NSSM deployment target, `mllp.py:1373-1379` documents an +observed ProactorEventLoop wedge (#55) that burns the full cap. + +Partial mitigation that does not close it: `_stop` is shared, so no *new* rows are claimed during the +overrun. The residual is one in-flight episode per PROCESSING lane across up to 256 lanes. + +### F3 — a false premise, now corrected in all three places + +`postgres.py`'s `recover_inflight_on_promotion` and `engine.py`'s SQL Server `reset_stale_inflight` call +both justified themselves with *"the prior leader has stopped processing"*. F2 shows that does not +follow. Corrected in `bc9ccd73`; a **third** site missed by that commit was corrected in `6c81c65e`. + +That the first correction's enumeration was incomplete is itself the defect class it was fixing — see +**Consequences**. + +### The asymmetry that decides sequencing + +Postgres stamps a row lease on every claim and the leader runs a periodic `reclaim_expired_leases` at +`reclaim_interval_seconds = 30.0` against `lease_ttl_seconds = 60.0`. A Postgres row left INFLIGHT is +**latency (~90 s worst case), never a strand**. + +**SQL Server has no periodic in-flight recovery at all.** `reclaim_expired_leases` is defined on the +Postgres store alone (**zero** definitions in `sqlserver.py`); the runner is gated on +`reclaims_inflight() and hasattr(self.store, "reclaim_expired_leases")` +([`engine.py:1062`](../../messagefoundry/pipeline/engine.py)) — and `SqlServerCoordinator.reclaims_inflight()` +returns **True**, so the `hasattr` is the sole exclusion. Its only recovery is the on-promotion +`reset_stale_inflight`. **A SQL Server row left INFLIGHT outside a promotion is an unbounded strand** — +and `stage_dispatcher.py:918-919` already banks the cancel path on that recovery +(*"leave the whole prefix INFLIGHT for reset_stale_inflight"*). + +This is a strand that exists **today**, with no HA scenario involved. + +--- + +## Decision — the demotion-safety contract + +**C1 — Fence direction: guard writes that make a claimed row TERMINAL; never guard a write that returns +a claimed row to PENDING.** + +Guarded: `mark_done`, `dead_letter_now`, `complete_with_response`, `mark_failed`'s **DEAD branch**, and +the batch twins. Not guarded: `release_claimed`, `reschedule_claimed`, `mark_failed`'s **retry branch**, +and every stage handoff. + +The dominant write on the demotion path is the L1 pre-send bail (`wiring_runner.py:4138-4145`), which +hands the row to the successor. Fencing it leaves the row INFLIGHT instead — a recovery delay on +Postgres, a **strand** on SQL Server, on the one path that today hands over instantly. An ex-leader +re-pending a row the successor is mid-delivering is duplicate-direction (**permitted**); fencing it is +strand-direction (**forbidden**). + +**C2 — Two predicates of opposite polarity, each named once.** + +- `_EPOCH_GUARD_CLAIM` — today's form, **fail-closed** on a missing lease row. Declining work is free. +- `_EPOCH_GUARD_RESOLVE` — **fail-open** on a missing lease row: + `COALESCE((SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key = :key), :held) <= :held`. + +On a resolve the polarity inverts: a rejected `mark_done` leaves the row INFLIGHT, which on SQL Server is +a strand. Reusing the claim idiom verbatim would ship a mass-strand bug. Extract both as per-backend +constants and test that neither appears at the other's sites. + +**No `status='inflight'` conjunct.** `reclaim_expired_leases` is owner-blind by its own docstring and can +re-pend the current leader's own long-running row; today that leader's `mark_done` still lands. A status +conjunct would reject it and force a genuine re-send — a duplicate manufactured on exactly the long-hold +WAN lanes. The epoch does not fire there (same node, same term), so the conjunct would be the only thing +firing, and firing is worse. + +**C3 — A fenced write is all-or-nothing, and it RE-PENDS the row. It must not leave it INFLIGHT.** +Zero rows affected ⇒ roll back the enclosing transaction (discarding the `delivered_keys` row, the +`message_events` row and the `_maybe_finalize_message` call), **then re-pend the row with an unguarded +`release_claimed([id])` in a fresh transaction**, bump a `fenced_write` counter, log WARNING. + +> **Corrected 2026-08-02, before any implementation.** An earlier revision of this clause ended at +> "roll back … return via the existing `if row is None: return` shape", leaving the row INFLIGHT for +> recovery to collect. **That is strand-direction and must not ship.** On Postgres it costs ~90 s of +> latency via `reclaim_expired_leases`. On SQL Server there is **no periodic in-flight recovery at all** +> (`grep -c reclaim_expired_leases messagefoundry/store/sqlserver.py` → **0**; the `hasattr` gate in +> `engine.py` is the sole exclusion), so the row is stranded until the next promotion — precisely the +> outcome this ADR exists to prevent, produced by its own fence. The re-pend is invariant-legal by C1's +> own rule: `release_claimed` is a *return-to-PENDING* write, which C1 explicitly forbids fencing, and it +> is `status='inflight'`-guarded so it is idempotent. It converts the fence's own residue from a possible +> **strand** into a certain **duplicate**, which is the only direction the invariant permits. +> +> Rejected alternative: preferring `release_claimed` over `mark_done` on the demotion path generally. +> That manufactures a duplicate even when the epoch was **not** bumped — the common self-fence-without- +> takeover case — where the fail-open guard would correctly have let the write land. + +Note honestly *why*: a persisted ledger row would record a **true** fact (`mark_done` is reached only +after a successful send), and the successor's H2 skip would then resolve the row without re-sending — so +rollback is not "avoiding a loss". We roll back because a ledger row without a resolved queue row is a +half-applied disposition asserted by a node that is not the authority. **Cost booked: one extra duplicate +per fenced resolve.** + +**C4 — A demoted node retains its stale epoch; it does not clear it — AND the epoch is re-stamped on +every reconcile pass while leader.** `_stop_graph` currently calls `set_leader_epoch(None)`, reasoning +that a demoted node should carry no stale token. **The polarity is backwards:** the guard string is +omitted entirely when the epoch is `None`, so `None` means *no fence* while a stale token fails closed. +Delete the clear and correct the comment. + +> **Corrected 2026-08-02.** "Delete the clear" **alone is unsafe once C5 lands**, and the failure is +> silent and total. `set_leader_epoch` has exactly one push site, inside `_start_graph`, and +> `_reconcile_graph` has only two branches: `is_leader() and not running` → start, and +> `not is_leader() and running` → stop. If the node demotes and re-acquires with a bumped epoch *during* +> a slow `_start_graph`, the post-bring-up recheck sees `is_leader()` True, so no stop fires — and +> thereafter the state `is_leader() and running` matches **neither branch, forever**, with the store +> still holding the pre-demotion epoch. Today that half-works because `claim_ready` is unfenced. After +> C5 fences every claim path, it is a live leader that claims **nothing**, silently, with no exception +> and no alert. The clause therefore requires a second half: **re-stamp `current_epoch()` on every +> reconcile pass where the node is leader and the graph is running.** That is safe and idempotent — +> `_is_leader` flips False→True only in `_maintain_leadership`, immediately after the renew refreshed +> `_leader_epoch`, with no intervening await, so `is_leader()` implies `current_epoch()` is non-`None`. +> +> **Also required in the same commit:** `tests/test_cluster_graph_gating.py` asserts the demotion push +> is `(None, None)`. That assertion encodes the behaviour being reversed and must be inverted here, or +> the tree goes red on a test that is documenting the old contract. + +**C5 — Fence every claim path, including `claim_ready`.** With `claim_ready` open, a demoted node in +teardown overrun can claim a **fresh** row after the successor bumped the epoch, send it, have its +`mark_done` fenced by C1, and on SQL Server leave a row with `owner=NULL`, no lease, and +`updated_at > promoted_at` — invisible to any promotion-scoped recovery. Fencing every claim path is what +makes the design's central argument true: *after the successor's bump an ex-leader can claim nothing, so +every row it still holds was already re-pended by promotion recovery.* + +**C6 — Demotion gets its own bounded teardown, distinct from clean shutdown.** `TeardownReason{SHUTDOWN, +DEMOTE}` as a parameter on the same `_teardown_unsafe` body — never a forked function. Under SHUTDOWN the +path is statement-for-statement today's. Under DEMOTE: bounded, **concurrent** source stop; then a +cooperative dispatcher `quiesce()` before any hard cancel; edge-triggered from the fence rather than +waiting on the graph poll. + +> **Three constraints added 2026-08-02 from adversarial review; each makes the difference between the +> increment working and being actively harmful.** +> +> 1. **The concurrent stop must be one phase-level `asyncio.wait(tasks, timeout=budget)`, not a +> semaphore with a per-source `wait_for`.** A semaphore of width *C* bounds the phase at +> `ceil(N/C) × budget`, not `max()` — roughly **63 s at the 1,500-connection target against an ~8 s +> margin**, i.e. the fix would not fix it. `asyncio.wait` also never cancels its awaitables, so +> *abandon-don't-cancel* becomes a property of the primitive rather than of a flag someone can drop. +> Creating all N tasks eagerly is cheap: every socket source closes its listening socket in its +> **synchronous prologue**, so accept stops at task-creation time. +> 2. **`quiesce()` must not gate the lane drain on the claimer/sweep loops exiting.** The fence fires +> *because* renews are failing — i.e. the pool is degraded — i.e. the claimer is parked inside a claim +> against `[store].command_timeout`. A design that waits for it drains zero serializers on the exact +> trigger it was built for. +> 3. **`self._running = False` must execute on every path** — `try/finally`, with the `finally` doing +> *only* that. Do not clear the rest of the state there: a cancelled teardown would orphan live +> listeners. Leaving `_running=True` (today's behaviour on a cancelled teardown) wedges re-promotion +> permanently; leaving `_running=False` with dirty state makes the rebind path silently skip every +> source. Both failure modes need pinned tests, plus a residual-state re-teardown at the top of +> `start()` and a third branch in `_reconcile_graph`. + +**The constraint stated in the brief does not exist.** `grep -c "D3" docs/adr/0066-*.md` → **0**. "ADR +0066 D3" appears only as a code comment (`wiring_runner.py:2499`). There is no ratified decision to +amend: this ADR **corrects a comment**, it does not supersede ADR 0066. + +**Single-node SQLite is byte-identical, structurally.** `MessageStore.set_leader_epoch` is a hard +`return None`; the guard string is only emitted in the two server dialects; `build_coordinator` returns +the NullCoordinator whose `current_epoch()` is `None`; and `[cluster].enabled` rejects SQLite at config +load, so `_stop_graph` never runs and `reason` is always SHUTDOWN. + +--- + +## Why this and not the alternatives + +**An `owner =` predicate.** Dead on SQL Server, whose claims write `owner=NULL` — always-false (fatal) or +always-true (useless). And `_owner` is per-store-**instance** (`host:pid:uuid4()[:8]`), not per-term, so +it cannot separate a term-N straggler from a term-N+2 worker in the same process. + +**Bare `status='inflight'` as the fence.** The successor re-pends then re-claims, so the predicate is +true again. The protected interval is the PENDING gap — milliseconds. + +**A per-claim token.** The precise mechanism, and the only thing that closes the re-promotion residual +below. Rejected **for this ADR only**: it changes the `Store` protocol signatures of +`mark_done`/`mark_failed`/`dead_letter_now` and every caller, plus a migration and a backfill question for +rows already INFLIGHT at upgrade. File it; do not fold it in. + +**Fencing writes that ADMIT a message** (ingress commit, stage handoffs). Rejected outright. Fencing an +ingress commit converts a permitted duplicate into a **lost** message and breaks count-and-log. A +demoting node that ACKs and persists during teardown is behaving *correctly*. + +**Wiring the SQL Server sweep through `LeaderMaintenanceRunner`.** Rejected — it would silently break SQL +Server. Satisfying the `hasattr` gate makes `_leader_maintenance` non-None, the promotion path's `if` +wins, the unconditional `reset_stale_inflight` becomes **dead code**, and `recover_on_promotion` calls a +method that does not exist on SQL Server. The sweep must be a **distinct capability**, additive to the +on-promotion reset. + +**A lease-anchored absolute teardown deadline.** Rejected as a *correctness* anchor: it makes the +monotonic clock load-bearing on the one platform the code already warns about (monotonic measures elapsed +**awake** time on Windows while the DB clock never suspends), and with `command_timeout` equal to +`leader_lease_ttl_seconds` a stalled pool drives the margin non-positive, degrading the "deadline" into an +unconditional hard cancel during precisely a DB-caused failover. A deadline that fires on every demotion +is not a deadline. + +**Hard-cancel-first as the default demotion ordering.** Rejected as default, retained as timeout +fallback. `_stop.set()` already halts new claim rounds and the L1 bail halts egress the instant the fence +flips, so cancelling first buys little while converting up to 256 in-flight lane episodes into INFLIGHT +residue on **every** failover — the exact row state SQL Server cannot recover. + +**Wrapping `stop()` / `_teardown_unsafe` in `wait_for` from outside.** Rejected, and this is the sharpest +implementation trap found. `self._running = False` is the **last** statement of `_teardown_unsafe` +(`wiring_runner.py:2610`), and `_reconcile_graph`'s bring-up branch is `is_leader() and not running`. A +cancelled teardown leaves `_running = True` and the node can **never re-promote, silently, with no +exception**. Any bound goes *inside*, guarding only the source phase. + +--- + +## Increments + +Each is independently shippable and ships with the test that would fail before it. + +**Inc 0 — make the margin real.** Clamp the lease renew's own statement timeout well below +`(ttl − fence)` instead of inheriting `command_timeout`; capture `t_issue` *before* the renew and stamp +the fence baseline from it rather than after the round trip; config-load warning when +`command_timeout >= (ttl − fence)` — noting it fires on **stock defaults today** (30 vs 30), so ship it +with a defaults change or rely on the clamp alone. +*Test:* slow-renew fixture asserting the detection margin stays positive. + +**Inc 1 — Postgres: fence every claim path + every terminal resolve.** `_EPOCH_GUARD_CLAIM` onto +`claim_ready`; `_EPOCH_GUARD_RESOLVE` onto the four resolves, the batch forms, and the RESPONSE-stage +re-ingress dead-letters. Drop `set_leader_epoch(None)` from `_stop_graph` (C4). Add an **additional** +own-owner promotion-recovery statement — do **not** widen `owner IS DISTINCT FROM`, which would re-open +engine-shard theft (ADR 0073). No DDL: `leader_lease.leader_epoch` already exists and back-fills +additively. +*Tests:* GAP 1 below; a **structural** test enumerating every `UPDATE queue SET status` site and asserting +each is guarded or in a reviewed allowlist. + +**Inc 2 — SQL Server: periodic in-flight recovery. Blocking for Inc 3, not for Inc 1.** A leader-gated, +age-based sweep (`status='inflight' AND updated_at < @cutoff`) reached through a **new** capability, not +by satisfying the `hasattr` gate; restructure the promotion path so the unconditional +`reset_stale_inflight` still runs. Its own named cutoff setting, sized **above the longest legitimate +claim-to-terminal hold**, not merely above skew. +*Independently valuable: it closes a strand that exists today with no HA scenario involved.* + +**Inc 3 — SQL Server: the same fences.** `claim_ready` and the terminal resolves. No stored-procedure +redeploy — every disposition is ad-hoc SQL. **Gate:** pin empirically, on the live CI leg, whether a +coroutine cancelled mid-`execute` leaves an aioodbc transaction committed or rolled back +(`except Exception: await conn.rollback()` does **not** catch `CancelledError`). If it commits, say so +rather than claiming a proof. + +**Inc 4 — `TeardownReason` + bounded, concurrent source stop (DEMOTE only).** The enum lands **here**: +`_teardown_unsafe` is the single shutdown path, so bounding it unguarded would change single-node SQLite +shutdown, which the parity constraint forbids. Snapshot `list(self._sources.values())` (the live-dict +iteration across awaits is a latent `RuntimeError`), gather under a semaphore, wrap each `source.stop()` +in `wait_for` **at the call site** — not by editing transport constants, which would make `transports/` +know about clustering and violate the one-way dependency rule (§4). +*Tests:* N parked sources → wall clock is max(), not sum(); a re-promotion after an abandoned stop cannot +double-bind; SHUTDOWN remains byte-identical. + +**Inc 5 — DEMOTE ordering + cooperative quiesce + edge trigger.** Under DEMOTE only, move the dispatcher +block above the source loop and split `d.stop()` into a cooperative `quiesce()` (no `task.cancel()`, so +serializers reach a terminal transition and leave zero rows INFLIGHT) with a hard-cancel fallback. Add a +sync, never-raise `on_demote` hook fired from `_check_fence` — safe because it is pure in-memory, so +`.cancel()`/`Event.set()` preserve its no-DB-I/O property. +*Tests:* **the `_running` regression test** (after a deadline-expired teardown the node can still +re-promote); both orderings asserted in one test so they cannot drift. + +--- + +## Consequences + +**What this buys.** A durable predicate evaluated at write time against the authoritative lease row, +which holds when the timing argument fails — and the timing argument *has* failed once already (F3). The +fence can reject an ex-leader's `mark_done`; it cannot un-send its HL7. **If only one thing ships, ship +the fence.** + +**Detection is not constraint.** Preconditions on the write are the **detection** half; something has to +make the peer stop, or detection only narrows the window. An ADR that shipped C1 alone would let a +reviewer conclude the problem was solved while a demoted leader is still mid-write. + +### What remains true after this ships + +1. **The fence's coverage is narrower than "demotion".** The epoch bumps only on a **fresh acquire** and + is unchanged on a renew by the same owner. Three cases: (a) demotion *with* takeover → fence armed — + the split-brain case that matters; (b) self-fence with no takeover → fence inert, but there is no + competing writer; (c) promote → demote → **re-promote in the same process** → the store's cached epoch + is per-store-**instance**, so a term-N straggler evaluates `N+2 <= N+2` and **passes**. Case (c) is a + genuine residual that only a per-claim token closes. Anything stronger than this paragraph repeats the + F3 pattern. +2. **The ex-leader is never "quiescent."** Do not use that word. A message mid-handler finishes its commit + and its ACK during the grace, and Inc 4 abandons an overrunning `stop()`. That is *correct* under + count-and-log — the body is durable, the successor drains it, the ACK is honest — and it is a + split-brain-shaped surprise that today's unbounded wait merely hides. +3. **Duplicates go up on the failover path, and the quiesce budget is a guess.** ADR 0066 §11 records + failover duplicate/ordering paths as **unmeasured**; this is the first thing to exercise them. Measure + before adopting the budget. +4. **Recovery re-pends burn retry attempts.** `reclaim_expired_leases` does not decrement; only + `release_claimed`/`reschedule_claimed` do. A flapping leader can dead-letter deliverable traffic + without a single delivery having failed. Pre-existing, amplified here. +5. **Two silent ordering traps.** Restoring `set_leader_epoch(None)` to `_stop_graph` looks like tidying + and disarms the fence; wrapping teardown from outside makes a node permanently un-re-promotable. Both + need pinned tests, not comments. +6. **Rolling upgrade skew.** No DDL, so the upgrade is clean — but the guard protects only when the + **straggler** runs new code, and in the natural sequence the straggler is the *old* leader. The fence + is live only after the last node is upgraded. +7. **DR can invert the guard.** `leader_epoch` restarts at **1** whenever the row is absent. After a cold + restore, a fresh leader holds 1 while a retained stale node caches 5, and `1 <= 5` **passes** — the + guard is not merely disarmed, it favours the stale node. The restore path must force the epoch forward. +8. **A new `[cluster]`-scoped setting is invisible to the posture completeness floor.** + `tests/test_security_posture_defaults.py:203` iterates `SecuritySettings.model_fields` only, so no + `[store]`- or `[cluster]`-scoped deviation can trip it (BACKLOG #333). Inc 0's and Inc 2's settings land + into that gap. +9. **Out of scope, deliberately.** The in-claim cross-owner reclaim statements stay unfenced this pass — + Postgres-only exposure. Self-theft by the owner-blind sweep is a **lease-sizing** bug, not a leadership + bug. + +**Hot path:** the guard is a non-correlated scalar subquery on a PK-keyed one-row table spliced into an +UPDATE that already runs — zero added round trips, against methods that already pay a `SELECT`, a ledger +insert, an event insert and a finalizer call. Expected unmeasurable at 520 ev/s; bench it anyway. + +### Why this needs a mechanism, not a documented rule + +F3 was not an isolated slip. On the day this ADR was written, several sessions working unrelated +subsystems each hit the same reasoning failure — a control whose justification was asserted rather than +enforced. **That general class, its taxonomy and its instances are ADR 0158's subject, not this one's.** +This section keeps only what bears on the decision above. + +Three findings from it are load-bearing here: + +- **The discipline was already written down and did not bind.** CLAUDE.md §11 already forbids a + compensating control resting on a false premise, and the project already had standing guidance to make + a gate fail on purpose before trusting it. F3 happened anyway — and the commit that corrected F3 + enumerated the affected sites and **missed one**, which is the same defect one level up. A rule that + has already failed in this exact file is not a remedy for this exact file. +- **Attention does not enforce it, including expert attention aimed directly at it.** One session built a + tool specifically to catch this class, wrote the discipline into its own docstrings, and shipped two + instances of it inside that tool; both were caught by a mechanism, neither by review. +- **Detection is a stopgap, demonstrated.** A PR fixing a CI-capacity failure was itself stalled by a + merge-queue failure *while two watchers ran specifically to catch that*. The watchers fired correctly. + The stall happened anyway. + +**Applied to this ADR:** C1 (a precondition on the write) is *detection* — it establishes that a write is +illegitimate at the moment it is attempted. C6 (a bounded, enforced demotion stop) is *constraint* — it is +what actually makes the peer stop. Shipping C1 alone narrows the window and leaves a reviewer entitled to +conclude the problem is solved while a demoted leader is still mid-write. That is why both are in the +Decision, and it is the one thing this ADR should not be talked out of. + +**The specific gap that argues for C6:** "the ex-leader has stopped" is a state with **no clearing +evidence**. Nothing anywhere observes it. The fence *fires* on an observable basis — renew timeout +elapsed — and then reports nothing about whether the work it was fencing actually ceased. A control needs +an observable basis for firing *and* for clearing; today this one has only the first. + +*See ADR 0158 for the general class, its instances across subsystems, and the proposed lint.* + +--- + +## Test gaps this closes + +**GAP 1 — the post-send fenced write.** The interleaving that decides adoption: the ex-leader's `send()` +returns AA, then its `mark_done` is fenced. No test exists, because these methods currently cannot fail. + +- Both server backends: claim under epoch N; bump `leader_lease.leader_epoch` out of band; call + `mark_done`. Assert the row is still INFLIGHT; **no `delivered_keys` row**; no `message_events` row; the + message is not finalized; `fenced_write` incremented; AlertSink fired once. +- Repeat for `dead_letter_now` (must not flip a successor-delivered row to DEAD — a false terminal is + functionally a strand until a human intervenes), `mark_failed`'s DEAD branch, `complete_with_response`, + and both batch forms. +- **Negative twin** — same interleaving, epoch **not** bumped: the write must land. *A green guard is + evidence only if it has been made to fail on purpose first.* +- **Mass-strand regression for C2** — delete the `leader_lease` row with the epoch armed, call + `mark_done`, assert the write **lands**. Written against the claim's fail-closed predicate this test + fails; that is the point. +- **Direction test for C1** — with the epoch bumped, `mark_failed`'s retry branch and `release_claimed` + must **still land**. +- **Recovery closure** — after a fenced write the row is resolved within a bounded time. On SQL Server + this **cannot pass before Inc 2**, which mechanically enforces the sequencing. + +**GAP 2 — independent node and DB clocks.** These are two clocks today and no fixture drives them apart: +`tests/test_cluster_lease.py:222-249` sets `a_mono.t` and `db_clock.t` in lockstep by hand, so it is +structurally incapable of seeing either the round-trip term or the `_fence_tick`. Construct: (a) fence +fires while the lease is still valid; (b) lease already expired when the fence fires; (c) node clock +frozen relative to the DB clock; (d) node self-fenced but the lease still live with no successor — writes +must still **land**. No case may produce a strand; duplicates permitted. + +Plus `command_timeout >= leader_lease_ttl_seconds` (the stock 30/30 collision) with a stalled renew: +assert the config warning fires and the fence still rejects post-claim writes — i.e. Inc 1 holds where +Inc 5's timing argument does not. + +**Standing gate, every increment:** `ruff check` + `ruff format --check`, `mypy` strict, `pytest`, and the +Windows CI legs. Local pytest **silently skips** the Postgres and SQL Server legs, so a green local run +proves nothing about anything above; delete `message_events` before any soak. diff --git a/docs/adr/README.md b/docs/adr/README.md index fd25ef9..c8fe38a 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -183,3 +183,4 @@ what is withheld and what you can request. | [0154](0154-synchronous-captured-downstream-reply-and-intake-authentication-for-the-inbound-http-listener-adr-0023-deferred-tail.md) | **Synchronous captured-downstream-reply and intake authentication for the inbound HTTP listener** — the ADR 0023 deferred tail: `reply_from` blocks the HTTP turn on a **committed** ADR 0013 `response` row (never an in-flight `DeliveryResponse`), and `intake_auth` (API key / bearer / mTLS subject) adds a peer control behind a posture-keyed gate. Also closes a live hole — `check_http_tls_exposure` returns early on truthy `tls`, so an off-loopback `Http(tls=True)` listener authenticates nobody today | **Accepted (2026-07-31)** — owner-ratified at rev 5; authorises **increment A only** (intake-auth + peer-control gate), which is **built and merged** (2026-08-01, `f2ef0ea9`); sync-reply (increment B) deferred pending a customer | | [0155](0155-dast-dynamic-security-testing-of-the-running-engine.md) | **DAST — dynamic security testing of the running engine** (BACKLOG #318) — no DAST had ever run against this project: every security test was static or in-process, leaving the [Secure_Development_Standards](../Secure_Development_Standards.md) §6.1 *Dynamic* tier row empty. Increment 1 builds a **self-run, authenticated authorization sweep** with **no new dependency**: one `uvicorn` listener on loopback in front of a real Engine + real AuthService, both identities minted over the wire through `POST /auth/login`, and the authorization expectation **derived from the live route table** by a single shared `require*()`-closure walk ([`scripts/security/route_gates.py`](../../scripts/security/route_gates.py), hoisted out of the security doc-drift guard so exactly one derivation of *is this route gated* exists in the tree) rather than a hand-kept list that goes stale the day a route lands. Measured shape: 105 route rows, 100 gated, 87 permission-gated, exactly 5 anonymous. Three passes — **negative** (every gated HTTP row sent with no credential and with an invalid bearer; anything but 401 is a finding), **authorized reach** (how many gated `GET` rows a *privileged* token got past authentication and authorization on — the positive number that stops a wall of 401s reading as *all endpoints protected*), and **viewer BFLA** (anything but a refusal, **including 404**, is a finding, because a 404 on a matched path template means the caller got past authorization into resource lookup). The receipt names what it examined and **fails closed** — below any floor it exits **2 (could not measure)**, never 0 — and records method, path template, status codes, counts and the *relaxed* posture it scanned, never a body, header or token. Two canaries are built from **supported configuration, not source patches** (authentication disabled at the target; the low-privilege identity over-granted while the expectation set stays the viewer's), avoiding the patch-rot failure where a canary silently stops applying; CI runs both **before** the real scan and requires each to exit exactly **1** (findings) *with* its receipt on disk — a 0 (blind), a 2 (could not measure, which is what a neutered canary actually produces) or a crash fails the job and the real scan never runs. The inversion is the design: the sweep and both canaries run as ordinary pytest in the **existing required** test legs, so a change that blinds the detector reds a PR, while the nightly workflow is advisory — **not** a required context (it has no `pull_request` trigger, so it cannot report on a PR) and deliberately **not** `continue-on-error`. **Scope boundary: see the ADR's *Scope boundary* section** — it is stated once there, verbatim, and this row deliberately carries a pointer and no wording of its own. Rejected/deferred: schemathesis (the shipped OpenAPI declares no `securitySchemes` and no per-operation `security`, so `ignored_auth` would pass on every operation having probed none of them; adoption also needs a fifth DEP-1 lock over a ~30-distribution closure that could pull a shipped runtime floor down), ZAP (a SHA-pinned action still pulls a mutable `:stable` image, and the published images do not install the web console), nuclei (template matcher; `pip install nuclei` is an abandoned unrelated package), Dredd/RESTler/CATS, in-process ASGI transport (bypasses the HTTP parser, leaves `request.client` unset), the unauthenticated MLLP/TCP/X12/DICOM ingress plane, the `/ui` console plane, a TLS black-box target, and non-`GET` reach/BFLA | Accepted (2026-07-31) — increment 1 built; advisory, not a required context | | [0156](0156-asvs-scorecard-as-data-a-derived-count-verified-evidence-anchors-and-a-fail-closed-drift-gate.md) | **ASVS scorecard as data — a derived count, verified evidence anchors, and a fail-closed drift gate** — the ASVS score is maintained as prose, and nothing checks it. One re-anchoring session (2026-08-01) re-derived the headline count **6 times**, found **12 residuals of record factually false at HEAD** (five of them *absence* claims that had silently stopped being true), and found **10 cells missing from an enumeration described as "arithmetic-checked and complete"** — which survived because the arithmetic closed to 345 and closure was read as proof. **Closure only proves the four buckets sum, not that every cell landed in one.** Decision: hold one `[[cell]]` record per requirement (all 345) in `asvs-scorecard.toml`; **compute** the count so no document can state one; assert **every corpus id appears exactly once** (the check whose absence cost ten cells); machine-verify each cell's `evidence` anchor by asserting an expected **token** still resolves, so code movement reds a test instead of rotting a sentence; require an absence claim to record the **search that proved it plus a positive control that must still hit**, because a grep naming the wrong token returns zero and reads exactly like proof; make **`unverified` a first-class verdict** so inherited-versus-verified Pass is countable (~219 Passes have never been read against the requirement text); and **fail closed rather than skip**. Tool + schema + fixture tests live in this repo and run in public CI; the real scorecard lives in the vault with a vault-CI job — which closes **ASVS 15.1.3**, currently open precisely because six `*_doc_drift` modules assert against documents that `git ls-files docs/security/` shows are **not present** in the tree where CI runs. Rejected: *keep prose and review harder* (every false residual **read as true**; the project's own standard says the mitigation must be structural, not diligence), *one document to rule them all* (that is the current lineage, and it produces five documents asserting three counts), and *publish the vault documents so the guards see them* (attacker roadmap, `SECURITY-DOCS-POLICY.md`). Explicitly **does not** make the score correct — only consistent, derived and drift-detecting; adversarial verification remains the only cure for a wrong verdict | **Accepted (2026-08-01)** — built and merged the same day. **§7 was amended at ratification**: it proposed a vault CI job, and the actions API showed every vault workflow `disabled_manually` (last run 2026-07-27; two vault PRs merged that day with zero checks), so a CI-only design would have shipped dead. Built instead as a vault **pre-commit hook** plus one **narrow new workflow**. That §7 was a confident, unchecked claim about system state is the ADR own thesis applied to itself | +| [0157](0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md) | **Demotion safety — fence scope on post-claim writes, and a bounded graph stop** — an HA re-check found the leadership lease itself **sound** (DB-clock expiry on both backends, atomic acquire/renew, a real `leader_epoch` token checked inside the claim transaction; scopes B and C were probed and cleared, not assumed) and two things wrong around it. **F1:** the epoch fence guards *some* claims and **nothing after them** — `claim_ready` (the UNORDERED path) carries no epoch predicate on either backend, and every post-claim disposition write (`mark_done`, `mark_failed`, `dead_letter_now`, `complete_with_response`, plus batch twins) resolves by bare `id` with no epoch, owner or status precondition, while `release_claimed` two methods away *does* carry `AND status=$4`. The sharp write is `dead_letter_now`: a demoted node assigning a **terminal** disposition and finalizing the message, breaching the store finalizer's single authority — and a DEAD row is never re-claimed, so H2 skip-and-complete cannot heal it. **F2:** demotion budgets **detection only, never the stop** — `_check_fence` flips a boolean and cancels no listener, worker or in-flight send; `engine.py`'s graph-poll interval is the *only* arithmetic consumer of `(ttl − fence)` in the package and it sizes a poll. Measured budget on stock defaults is **≈8.0 s** (fence 20 + a 1.0 s fence tick + a 1.0 s poll against a 30 s DB-clock expiry) *minus* the renew round trip, which is bounded only by `[store].command_timeout = 30` — **exactly equal** to `leader_lease_ttl_seconds`, so the margin can reach zero and `_fence_ordering` (ordering-only) never notices. Against that, teardown stops inbounds **sequentially** at up to 10.0 s per socket listener (5.0 client grace + 5.0 `wait_closed`, off a module constant unrelated to lease timing) and **unbounded** for file/DB/DICOM inbounds, at a 1,500-connection target. **Decision (6 clauses):** guard writes that make a row **TERMINAL**, never one that returns it to PENDING (fencing the L1 hand-over would convert a permitted duplicate into a forbidden **strand**); two predicates of **opposite polarity** (claim fail-closed, resolve fail-**open**, because a rejected resolve leaves the row INFLIGHT); no `status` conjunct; a demoted node **retains** its stale epoch (`None` means *no fence*, so clearing it disarms the guard); fence every claim path incl. `claim_ready`; and a `TeardownReason{SHUTDOWN,DEMOTE}` bounded, concurrent, edge-triggered demotion stop. Rejected: an `owner=` predicate (dead on SQL Server, which claims `owner=NULL`), a per-claim token (correct, but changes the `Store` protocol — filed, not folded in), fencing writes that ADMIT a message (converts a duplicate into a **loss**), a lease-anchored absolute deadline (makes the monotonic clock load-bearing on Windows, and degrades to an unconditional cancel at the 30/30 collision), and wrapping teardown in `wait_for` **from outside** — `self._running = False` is the last statement of `_teardown_unsafe`, so a cancelled teardown leaves the node **permanently un-re-promotable, silently**. Also records the sequencing asymmetry: Postgres bounds a stranded INFLIGHT row at ~90 s via its periodic sweep, while **SQL Server has no periodic in-flight recovery at all** (`reclaim_expired_leases` is Postgres-only; the runner's `hasattr` gate is the sole exclusion), so the same row is an **unbounded strand today**, with no HA scenario involved. Corrects a code comment attributing a teardown-ordering constraint to "ADR 0066 D3" — that decision does not exist (`grep -c D3` → 0). Single-node SQLite byte-identical (structurally: `set_leader_epoch` is a hard `return None`). #26-clean | **Proposed (2026-08-01)** — nothing built; owner decisions needed on C1 (which writes carry a precondition) and C6 (does demotion get an enforced deadline) | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index 4cba84d..6a526e4 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -4142,6 +4142,7 @@ async def stats( # (or a future one) reports 0 rather than 500ing the stats read. committed_txns=getattr(engine.store, "committed_txns", 0), body_copies=getattr(engine.store, "body_copies", 0), + fenced_writes=getattr(engine.store, "fenced_writes", 0), ) @app.get("/metrics") diff --git a/messagefoundry/api/metrics.py b/messagefoundry/api/metrics.py index de3c3ca..704d88f 100644 --- a/messagefoundry/api/metrics.py +++ b/messagefoundry/api/metrics.py @@ -198,6 +198,7 @@ class _Snapshot: pool: PoolStatus | None = None committed_txns: int = 0 body_copies: int = 0 + fenced_writes: int = 0 # ADR 0114 AC-7's degraded gauge. None when the backend has no fifo_claim_proc lever or the flag # is off — the gauges are then ABSENT rather than 0, so a scrape can tell "not requested" from # "requested and degraded" (a constant 0 on every SQLite fleet would be pure alert noise). @@ -233,6 +234,7 @@ async def gather_snapshot(engine: Engine) -> _Snapshot: pool = engine.store.pool_status() committed_txns = int(getattr(engine.store, "committed_txns", 0)) body_copies = int(getattr(engine.store, "body_copies", 0)) + fenced_writes = int(getattr(engine.store, "fenced_writes", 0)) return _Snapshot( sync_replies=sync_replies, version=__version__, @@ -249,6 +251,7 @@ async def gather_snapshot(engine: Engine) -> _Snapshot: pool=pool, committed_txns=committed_txns, body_copies=body_copies, + fenced_writes=fenced_writes, claim_proc=engine.store.claim_proc_status(), ) @@ -419,6 +422,14 @@ def collect(self) -> Iterable[Any]: ) body_copies.add_metric([], float(s.body_copies)) yield body_copies + # ADR 0157 C3 split-brain signal. A counter rather than a gauge because it only ever grows; + # alert on rate(...) > 0, not on an absolute value. + fenced = CounterMetricFamily( + "messagefoundry_store_fenced_writes", + "Terminal queue resolves rejected by the leader-epoch fence (process lifetime).", + ) + fenced.add_metric([], float(s.fenced_writes)) + yield fenced # ADR 0114 AC-7 degraded gauge. Emitted ONLY when [store].fifo_claim_proc is on: a constant # 0 on every fleet that never asked for the lever is noise a scraper cannot alert on, and diff --git a/messagefoundry/api/models.py b/messagefoundry/api/models.py index b8a3e12..815c493 100644 --- a/messagefoundry/api/models.py +++ b/messagefoundry/api/models.py @@ -488,6 +488,11 @@ class StatsResponse(BaseModel): # backend reports 0 (counting is wired on SQLite + SQL Server). committed_txns: int = 0 body_copies: int = 0 + # ADR 0157 C3: terminal queue resolves rejected by the H1 leader-epoch fence (Postgres only; 0 + # elsewhere). Non-zero == a superseded ex-leader was stopped mid-write. MUST be declared here: + # this model takes Pydantic's default extra='ignore', so an undeclared kwarg is dropped SILENTLY + # and /stats would never grow the field. + fenced_writes: int = 0 class MetricsHistorySample(BaseModel): diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 66ab3b1..ffbe826 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -62,6 +62,7 @@ "TrustAnchorMode", "TrustAnchorPolicy", "active_hop_posture", + "build_smtp_tls_context", "build_verifying_client_context", "cleartext_acceptance_audit_sink", "current_hop_posture", @@ -919,3 +920,61 @@ def build_verifying_client_context( return ctx # pinned / per-connection: ONLY this CA (no load_default_certs), matching forward_tls_ca_file. return ssl.create_default_context(purpose, cafile=anchor.cafile) + + +def build_smtp_tls_context( + *, + host: str, + cell: str, + verify: bool = True, + ca_file: str | None = None, + check_hostname: bool = True, + trust_anchor_policy: TrustAnchorPolicy | None = None, +) -> ssl.SSLContext: + """Build the TLS context for an outbound SMTP hop (#323) — STARTTLS or implicit ``SMTP_SSL``. + + ``smtplib`` accepts no context by default, and its fallback is + :func:`ssl._create_stdlib_context`, which **is** ``ssl._create_unverified_context`` — measured on + CPython 3.14.6: ``verify_mode=CERT_NONE``, ``check_hostname=False``. So every certificate was + accepted and the encrypted session was unauthenticated: an on-path attacker presenting any + certificate read the message body (PHI) and the SMTP AUTH credential. This is the SMTP sibling of + :func:`~messagefoundry.transports.remotefile._ftps_ssl_context` and the MLLP outbound arm, and is + deliberately built here rather than in ``transports/`` because ``pipeline/alert_sinks.py`` is a + third caller and a transport must not import ``pipeline/`` (ADR 0029's one-way rule). + + ``verify=False`` is NOT gated here — the caller refuses it against the clamped + :func:`~messagefoundry.config.settings.weakened_tls_escape_permitted_here` first, matching how + MLLP/FTPS inline that check. This module cannot read it: ``config.settings`` imports *from* here, + so the dependency runs one way only. + + ``trust_anchor_policy`` (#190, ADR 0093) supplies the instance ``[tls]`` internal-CA fallback when + the connection names no ``ca_file`` of its own. It only chooses WHICH roots verify the peer — it + never turns verification off. + """ + if verify: + anchor = resolve_trust_anchor( + connection_ca_file=ca_file, + host=host, + policy=trust_anchor_policy if trust_anchor_policy is not None else TrustAnchorPolicy(), + ) + ctx = build_verifying_client_context(anchor) + else: + ctx = ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=ca_file) + ctx.minimum_version = ssl.TLSVersion.TLSv1_2 + if verify: + ctx.check_hostname = check_hostname + else: + logger.warning( + "%s TLS certificate verification is DISABLED (tls_verify=false) — the SMTP session to %s " + "is encrypted but UNAUTHENTICATED and MITM-able; trusted-network dev/test only.", + cell, + host, + ) + # Order is load-bearing: check_hostname must go False BEFORE verify_mode, or ssl raises. + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector=cell) # assert forward secrecy (ASVS 12.1.2) + if verify: # nothing to strict-validate on the CERT_NONE path (ASVS 12.1.4) + harden_verify_flags(ctx) + return ctx diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 11a6496..87ce5cb 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -1785,6 +1785,9 @@ def Email( username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret) password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret) use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS + tls_verify: bool = True, # verify the server cert (#323); False (dev only) needs the escape + tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP server against (not a secret) + tls_check_hostname: bool = True, # match the cert against `host` (leave on) timeout_seconds: float = 30.0, encoding: str = "utf-8", ) -> ConnectionSpec: @@ -1793,9 +1796,18 @@ def Email( text); this delivers it as a plain-text SMTP message to ``host:port`` from ``sender`` to ``recipients`` with a static ``subject``. STARTTLS by default (``use_tls=True``) on the ``587`` submission port; port ``465`` is implicit TLS (``SMTP_SSL``). Optional ``username``/``password`` do - SMTP ``AUTH`` (over TLS only — a cleartext-credential config is refused). Disabling TLS - (``use_tls=False``) is MITM-able and refused unless ``MEFOR_ALLOW_INSECURE_TLS`` is set (loud - warning), like LDAPS / SQL Server / MLLP. The egress host is gated by ``[egress].allowed_smtp``. Put + SMTP ``AUTH`` (over a **verified** TLS session only — a cleartext- or unverified-credential config + is refused). Disabling TLS (``use_tls=False``) is MITM-able and refused unless + ``MEFOR_ALLOW_INSECURE_TLS`` is set (loud warning), like LDAPS / SQL Server / MLLP. + + **The server certificate is verified** (``tls_verify=True``, #323) — chain, hostname and strict RFC + 5280 flags, anchored to the OS roots, a per-connection ``tls_ca_file``, or the instance-wide + ``[tls].internal_ca_file`` (ADR 0093). ``smtplib``'s own default context verifies **nothing** + (``CERT_NONE``/``check_hostname=False``), so before #323 ``use_tls=True`` bought encryption without + authentication. Point ``tls_ca_file`` at your relay's CA PEM for a private-CA server; + ``tls_verify=False`` is a trusted-network dev/test escape, refused on an enforcing production-PHI + instance even with ``MEFOR_ALLOW_INSECURE_TLS``, and it also refuses SMTP ``AUTH``. + The egress host is gated by ``[egress].allowed_smtp``. Put secrets in ``env()`` (``username``/``password``), never inline. Delivery is at-least-once, so a retry re-sends the email — a mailbox has no idempotency key, so a rare duplicate is possible and accepted (a duplicate beats a drop). ADR 0029.""" @@ -1810,6 +1822,9 @@ def Email( "username": username, "password": password, "use_tls": use_tls, + "tls_verify": tls_verify, + "tls_ca_file": tls_ca_file, + "tls_check_hostname": tls_check_hostname, "timeout_seconds": timeout_seconds, "encoding": encoding, }, @@ -1836,6 +1851,9 @@ def Direct( username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret) password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret) use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS + tls_verify: bool = True, # verify the relay's cert (#323); False (dev only) needs the escape + tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP/HISP relay against + tls_check_hostname: bool = True, # match the cert against `host` (leave on) timeout_seconds: float = 30.0, encoding: str = "utf-8", ) -> ConnectionSpec: @@ -1844,7 +1862,14 @@ def Direct( **body** (content-agnostic — an HL7 string, a CDA/XML document, plain text); this **signs** it with ``signing_key``/``signing_cert``, **encrypts** the signed blob to the partner's ``recipient_cert`` (which must chain to ``trust_anchor``), and submits the S/MIME message to ``host:port`` over - STARTTLS. All cert/key material is loaded + validated at construction (fail loud). The egress host + STARTTLS. All cert/key material is loaded + validated at construction (fail loud). + + **The relay's TLS certificate is verified** (``tls_verify=True``, #323 — ``smtplib``'s own default + verifies nothing). Note the two trust settings are unrelated and easy to confuse: ``trust_anchor`` + is the CA the **partner's S/MIME certificate** must chain to (message-layer), while ``tls_ca_file`` + is the CA the **SMTP relay's TLS certificate** must chain to (transport-layer). The S/MIME body + protects the clinical payload either way, but the SMTP session still carries envelope metadata and + any ``AUTH`` credential, which is why the transport hop is verified too. The egress host is gated by ``[egress].allowed_direct``. Put secrets in ``env()`` (``signing_key_password``, ``username``/``password``), never inline. Delivery is at-least-once, so a retry re-sends — a Direct mailbox has no idempotency key, so a rare duplicate is possible and accepted (a duplicate beats a @@ -1865,6 +1890,9 @@ def Direct( "username": username, "password": password, "use_tls": use_tls, + "tls_verify": tls_verify, + "tls_ca_file": tls_ca_file, + "tls_check_hostname": tls_check_hostname, "timeout_seconds": timeout_seconds, "encoding": encoding, }, diff --git a/messagefoundry/pipeline/cluster.py b/messagefoundry/pipeline/cluster.py index 4bbb456..cf31537 100644 --- a/messagefoundry/pipeline/cluster.py +++ b/messagefoundry/pipeline/cluster.py @@ -167,14 +167,27 @@ def current_epoch(self) -> int | None: imports the coordinator, ARCH-6). Cheap + synchronous (cached state). :class:`NullCoordinator` returns ``None`` — single-node is unfenced (there is no second writer to fence). - **Scope of the fence — read this before relying on it.** The guard is appended to the claim - ``UPDATE`` and to nothing else. It does NOT cover (a) the cross-owner stranded-lease reclaim that - runs as the *first* statement of the same claim transaction, which commits even when the guarded - claim matches zero rows, or (b) any post-claim disposition write (``mark_done`` / ``mark_failed`` - / ``dead_letter_now`` / ``complete_with_response``), each of which resolves its row by ``id`` - alone with no epoch, owner, or status precondition. So a demoted ex-leader that is still inside - a send cannot *claim* anything, but can still *write* — including over a row the live leader has - already resolved. Do not read this token as a general write fence.""" + **Scope of the fence — read this before relying on it. It differs BY BACKEND (ADR 0157).** + + On **Postgres**: every claim path is guarded fail-CLOSED (an unvalidatable claim declines, which + is free — the row stays PENDING), and every TERMINAL resolve is guarded fail-OPEN with a re-pend + fallback (a rejected resolve rolls the whole disposition back and returns the row to PENDING, so + the residue is a duplicate rather than a strand). Deliberately UNGUARDED there: writes that + return a row to PENDING (``release_claimed`` / ``reschedule_claimed`` — fencing those would turn + a permitted duplicate into a forbidden strand), the cross-owner stranded-lease reclaim that runs + as the first statement of the same claim transaction, the bring-up dead-letter sweeps, and the + operator paths. + + On **SQL Server**: only the three FIFO claim paths carry the guard. ``claim_ready`` and every + terminal resolve are still unfenced there (ADR 0157 Inc 3 closes that), so a demoted SQL Server + node stops claiming FIFO lanes but can still drain an UNORDERED lane and still write over a row + the live leader has resolved. + + The residual case that passes on **both** backends: promote → demote → re-promote *in the same + process*. ``leader_epoch`` bumps only on a fresh acquire, so the re-promoted node's held epoch + equals the current one and every guard passes. Only a per-claim token would close that. + + Do not read this token as a general write fence.""" ... def lease_key(self) -> str | None: diff --git a/messagefoundry/pipeline/engine.py b/messagefoundry/pipeline/engine.py index 628151e..3481963 100644 --- a/messagefoundry/pipeline/engine.py +++ b/messagefoundry/pipeline/engine.py @@ -1245,10 +1245,26 @@ async def _stop_graph(self) -> None: loops keep running (a follower still converges its caches), so only the runner is stopped.""" if self._registry_runner is not None: await self._registry_runner.stop() - # H1: clear the held epoch on demotion so a freshly-demoted node carries no (now-stale) fencing - # token if it were to claim before promotion re-pushes the current one. Defensive — the runner is - # already stopped, so no claim is in flight — but it keeps set_leader_epoch's lifecycle honest. - self.store.set_leader_epoch(None) + # ADR 0157 C4 — do NOT clear the held epoch here. + # + # The store OMITS the guard string ENTIRELY when the epoch is None, so `set_leader_epoch(None)` + # means NO FENCE, not "a safe null token". Clearing it on demotion therefore DISARMS the guard at + # exactly the moment a superseded ex-leader may still be mid-teardown and mid-write. A RETAINED + # stale epoch is what fails closed on every claim and rejects every terminal resolve. + # + # Safe because _start_graph pushes current_epoch() unconditionally on promotion (:1178) AND + # _reconcile_graph re-stamps it on every leader+running pass (below), and no non-graph path + # resolves a CLAIMED row — the operator paths are PENDING/DEAD/DONE-scoped. + # + # Cross-backend, stated precisely because the obvious stronger claim is false: on Postgres every + # claim path and every terminal resolve is now fenced. On SQL Server only the three FIFO claim + # paths carry a guard — `claim_ready` and every terminal resolve are still UNFENCED there until + # ADR 0157 Inc 3 — so retaining the epoch stops a demoted SQL Server node claiming FIFO lanes and + # nothing more. That is still strictly better than today (where the clear disarmed even those), + # but it is NOT "a demoted SS node claims nothing". + # + # PINNED INVARIANT, not tidiness: restoring the clear looks like cleanup and silently disarms the + # fence. See test_stop_graph_retains_the_leader_epoch. log.info("engine graph stopped — this node is now standby") async def _reconcile_graph(self) -> None: @@ -1265,7 +1281,24 @@ async def _reconcile_graph(self) -> None: # graph running for a whole extra poll cycle. if not self._coordinator.is_leader(): await self._stop_graph() - elif not self._coordinator.is_leader() and running: + elif self._coordinator.is_leader() and running: + # ADR 0157 D2 — RE-STAMP the held epoch. _start_graph is the ONLY other push site, so a + # slow bring-up that spans demote -> foreign takeover -> re-acquire lands here with the + # store holding a STALE epoch and NEITHER of the other branches ever matching again. + # Before C5 that half-worked (claim_ready was unfenced); with C5 it is a live leader that + # claims NOTHING, silently, with no exception and no alert. Pure in-memory and + # idempotent; bounds staleness to one poll. is_leader() implies current_epoch() is + # non-None: _maintain_leadership sets _is_leader True only after _claim_or_renew_lease + # refreshed _leader_epoch, with no await in between (cluster.py:935 -> :861). + self.store.set_leader_epoch( + self._coordinator.current_epoch(), lease_key=self._coordinator.lease_key() + ) + elif not self._coordinator.is_leader() and ( + running or self._registry_runner.has_residual_state + ): + # ADR 0157 D7: `running` alone is not enough once _teardown_unsafe clears _running in a + # finally — a raised or cancelled teardown leaves live listeners bound on a standby with + # no branch that would ever converge them. await self._stop_graph() async def _graph_supervisor_loop(self) -> None: diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 46515da..60c5491 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -1091,6 +1091,19 @@ def __init__( def running(self) -> bool: return self._running + @property + def has_residual_state(self) -> bool: + """Anything still built after a teardown that did not complete (ADR 0157 D7). + + Deliberately NOT the same expression as ``stop()``'s ``had_state``, and the two differences are + both load-bearing. It omits ``_running`` because this is asked precisely when ``_running`` is + already False (the ``finally`` in ``_teardown_unsafe`` clears it on every path, including a raise + or an outside cancel). It ADDS ``_dispatchers`` because they are cleared early in teardown, before + the destination ``aclose()`` / executor-shutdown awaits — so a cancel landing between those leaves + ``_dispatchers`` populated while the other three are not. + """ + return bool(self._sources or self._workers or self._destinations or self._dispatchers) + @property def fusion_active(self) -> bool: """Whether ADR 0071 B5 thread-hop fusion is EFFECTIVELY active this run (SQL Server, pooled, @@ -4032,7 +4045,10 @@ async def _delivery_worker(self, name: str) -> None: items = [head] if head is not None else [] else: # UNORDERED lanes are intentionally NOT lane-owned — concurrent draining across - # nodes is fine, so claim_ready stays unchanged. + # nodes is fine for ORDERING. That is a statement about lane ownership, not about + # who may claim at all: claim_ready is leader-epoch fenced like every other claim + # path (ADR 0157 C5), so a superseded ex-leader draining an unordered lane claims + # nothing. items = await self.store.claim_ready( limit=self.claim_limit, destination_name=name ) @@ -4197,21 +4213,38 @@ async def _process_delivery_item( # ONLY, but leadership can be lost (a self-fence) BETWEEN claiming this row and the # send below. A cheap, SYNCHRONOUS is_leader() read (cached state — no DB round-trip) # closes that narrow window: a node that has stopped being leader must not emit egress - # as a stale ex-leader. We do NOT drop the row — re-queue it via the existing retry - # (mark_failed → PENDING with backoff) so the new leader delivers it (count-and-log, - # REL-4). This is a cheap fast-path guard, NOT the authority: the durable backstop is + # as a stale ex-leader. + # + # RELEASE the claim (attempts--, next_attempt_at UNCHANGED, no last_error) rather than + # mark_failed: losing leadership is NOT a delivery failure and must not spend a retry. + # Under a finite RetryPolicy.max_attempts, mark_failed here re-reads the claim's own + # attempts++ and dead-letters on `attempts >= max_attempts` — writing terminal DEAD on a + # row that was NEVER SENT. The new leader never sees a DEAD row, so that is a STRAND, + # which the count-and-log invariant forbids (duplication is permitted; loss is not). + # release_claimed is guarded `status='inflight'`, so it is idempotent if the row already + # resolved. + # + # Then STOP the lane rather than resolving it: a demoted node must stop claiming, and a + # release without a stop would hot-spin — release applies no backoff, so the row is + # immediately due again — for however long teardown takes, which is not bounded against + # the fence-to-expiry margin (ADR 0157 F2). _teardown_unsafe clears the dispatchers and + # workers and start() rebuilds them, so promotion re-arms the lane; a STOPPED lane never + # outlives the term that stopped it. + # + # This is a cheap fast-path guard, NOT the authority: the durable backstop is # H1's store-checked leader_epoch fence, which rejects a superseded ex-leader's claim # at the DB inside the claim transaction even if this in-memory check raced. On the # single-node NullCoordinator is_leader() is always True, so this never fires and the # delivery path is byte-identical. if not self._coordinator.is_leader(): - retry_until = await self._mark_failed_and_arm( + await self.store.release_claimed([item.id]) + log.warning( + "delivery worker %r: leadership lost before send; released %d claimed row(s) " + "un-errored (no attempt spent) and stopped the lane for the new leader", name, - item.id, - "leadership lost before send; re-queued for the new leader", - retry, + 1, ) - return _ItemOutcome.PROCESSED, retry_until + return _ItemOutcome.STOPPED, None try: if self._simulate.get(name, False): # Shadow / parallel-run (#15): suppress the real egress entirely — no bytes/ @@ -4454,11 +4487,19 @@ async def _process_delivery_batch( await self._maybe_alert_buildup(name) await self._maybe_alert_stall(name) return _ItemOutcome.PROCESSED, retry_until + # L1 batch twin — see the single-item path for the full rationale. Same reasoning, and the + # stakes are higher here: mark_batch_failed decides ONE disposition from the head's + # attempts and applies it to all N, so a finite retry cap dead-letters the whole batch on a + # leadership change, un-sent. if not self._coordinator.is_leader(): - retry_until = await self._mark_batch_failed_and_arm( - name, ids, "leadership lost before send; re-queued for the new leader", retry + await self.store.release_claimed(ids) + log.warning( + "delivery worker %r: leadership lost before send; released %d claimed row(s) " + "un-errored (no attempt spent) and stopped the lane for the new leader", + name, + len(ids), ) - return _ItemOutcome.PROCESSED, retry_until + return _ItemOutcome.STOPPED, None # Frame + send inside ONE try so a FRAMING error (an unparseable / non-HL7 head member — MLLP is # payload-agnostic, ADR 0004, so a non-hl7v2 feed can reach here) routes to the internal-error # policy below (dead-letter / STOP) instead of stranding every claimed row INFLIGHT forever with diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index b50fc1b..f7baa11 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -235,6 +235,14 @@ class QueueStore(StoreLifecycle, Protocol): committed_txns: int body_copies: int + #: ``fenced_writes`` = TERMINAL queue resolves REJECTED by the H1 leader-epoch fence since store open + #: (ADR 0157 C3). Additive and monotone like the two above, read the same ``getattr(store, name, 0)`` + #: way, and **Postgres-only**: it is the one backend where terminal resolves carry the fence, so the + #: SQLite and SQL Server handles expose it at a permanent 0 for protocol / ``/stats`` uniformity. A + #: non-zero value means a superseded ex-leader tried to resolve a row the current leader owns and was + #: stopped — the split-brain signal an operator actually wants paged on. + fenced_writes: int + # --- write path ---------------------------------------------------------- async def enqueue_message( self, @@ -587,16 +595,23 @@ async def reschedule_claimed( ... def set_leader_epoch(self, epoch: int | None, *, lease_key: str | None = None) -> None: - """Push this node's currently-held **leader epoch** (the H1 fencing token) into the store so the - FIFO claim can fence a superseded ex-leader, **inside** the existing single claim transaction. + """Push this node's currently-held **leader epoch** (the H1 fencing token) into the store so a + superseded ex-leader can be fenced **inside** the existing statement's own transaction. - The engine calls this on promotion/demotion: it reads the value from the cluster coordinator - (:meth:`ClusterCoordinator.current_epoch` / :meth:`ClusterCoordinator.lease_key`) and pushes it + The engine calls this on **promotion** — and re-stamps it on every leader+running reconcile pass + (ADR 0157 D2) — reading the value from the cluster coordinator + (:meth:`ClusterCoordinator.current_epoch` / :meth:`ClusterCoordinator.lease_key`) and pushing it here, so the **store never imports the coordinator** (the one-way ARCH-6 dependency direction). - ``epoch=None`` disables the guard (single-node / not yet leader / demoted) — the claim is then - byte-identical to before H1. With a non-``None`` epoch the server-DB backends add - ``leader_lease.leader_epoch <= :held`` to the claim's UPDATE so a paused/superseded ex-leader - (whose held epoch is now strictly older than the live leader's) claims **0 rows**. + + ``epoch=None`` **OMITS THE GUARD ENTIRELY** — it means *no fence*, not "a safe null token", and + the emitted SQL is then character-identical to pre-H1. That is why demotion deliberately does + **not** clear it (ADR 0157 C4): clearing on demotion would disarm the guard at precisely the + moment a superseded ex-leader is most likely to still be writing. + + With a non-``None`` epoch the server-DB backends splice a ``leader_lease.leader_epoch`` + comparison. Which statements carry it, and with which polarity, differs by backend — see + :meth:`ClusterCoordinator.current_epoch` for the authoritative scope. It is **not** a general + write fence. Cheap + synchronous (it only stamps cached state — no DB round-trip). A **no-op on SQLite** (single active node — no second writer to fence).""" diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 564a6a0..286ad00 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -175,6 +175,48 @@ _FINALIZE_LOCK_PREFIX = "mefor_finalize:" _OUTBOUND_LANE_LOCK_PREFIX = "mefor_outlane:" +# H1 epoch-fence guard templates (ADR 0157 C1/C2). Templates rather than literals because the guard +# lands at a different `$N` at every site; `{h}` is bound once and referenced twice, already the house +# idiom (see the twice-referenced `$5` at claim_next_fifo). `leader_lease.leader_epoch` is BIGINT, so +# `COALESCE(bigint, $N)` types `$N` as bigint with no cast. +# +# THE TWO POLARITIES ARE OPPOSITE ON PURPOSE. Do not unify them. + +#: CLAIM guard — FAIL-CLOSED. A missing leader_lease row yields NULL and `NULL <= $h` is false, so an +#: unvalidatable claim is DECLINED. Declining is free: the row stays PENDING and any node may claim it. +#: NEVER use this on a write that RESOLVES a claimed row. +_EPOCH_GUARD_CLAIM = ( + " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=${k}) <= ${h}" +) + +#: TERMINAL-RESOLVE guard — FAIL-OPEN, and the inversion is the whole point. Rejecting a resolve leaves +#: the row INFLIGHT; reusing the claim idiom here would mass-strand every in-flight row the moment the +#: lease row went missing. COALESCE makes a MISSING ROW resolve to "current", so the write LANDS. +#: Fail-open covers a missing lease ROW, not a missing lease TABLE — an absent table raises out to the +#: caller (row stays INFLIGHT, recovery takes it), and it cannot be absent while an epoch is armed (only +#: a started coordinator creates it; only the engine arms it). +#: NO `status='inflight'` conjunct: reclaim_expired_leases is owner-BLIND by its own docstring and can +#: re-pend the CURRENT leader's own long-running row; today that leader's mark_done still lands. A status +#: conjunct would be the only thing firing there — on long-hold WAN lanes, same node, same term, where +#: the epoch cannot fire — and firing is worse. +_EPOCH_GUARD_RESOLVE = ( + " AND COALESCE((SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=${k}), ${h})" + " <= ${h}" +) + + +class _FencedWrite(Exception): + """Sentinel raised INSIDE the enclosing ``conn.transaction()`` so asyncpg rolls the whole + disposition back — the queue flip, the ``delivered_keys`` row, the ``message_events`` row and the + ``_maybe_finalize_message`` call go together (ADR 0157 C3). Caught immediately outside; the method + returns its existing no-op value, then RE-PENDS the row (ADR 0157 D1). Never escapes the store.""" + + def __init__(self, method: str, outbox_ids: tuple[str, ...]) -> None: + super().__init__(method) + self.method = method + self.outbox_ids = outbox_ids + + # Schema (PostgreSQL). All DDL is `IF NOT EXISTS`, run once under the schema advisory lock so # concurrent opens don't race on CREATE. Epoch timestamps are DOUBLE PRECISION; ids TEXT (uuid4 hex); # bodies/PHI columns TEXT; booleans BOOLEAN; auto-ids BIGSERIAL. The `queue.seq` BIGSERIAL is the FIFO @@ -840,10 +882,14 @@ def __init__( self._cluster_state_convergence: bool = False # H1 fencing token: the leader epoch this node currently holds + the leader_lease row to validate # it against, both pushed by the engine on promotion via set_leader_epoch() (the store NEVER - # imports the coordinator — ARCH-6). None disables the claim's epoch guard (single-node / not yet - # leader), keeping claim_next_fifo byte-identical to pre-H1. + # imports the coordinator — ARCH-6). None OMITS BOTH GUARDS ENTIRELY — None means NO FENCE, not a + # safe null token (ADR 0157 C4) — keeping every claim and every terminal resolve byte-identical to + # pre-H1/pre-0157 on a single node. self._leader_epoch: int | None = None self._lease_key: str | None = None + # ADR 0157 C3/D1 — TERMINAL resolves rejected by the H1 fence since store open. Additive and + # monotone; read with getattr(store, "fenced_writes", 0) exactly like committed_txns above. + self.fenced_writes = 0 @classmethod async def open( @@ -1890,6 +1936,69 @@ async def _record_delivered_key( now, ) + def _resolve_guard(self, first: int) -> tuple[str, list[Any]]: + """``(sql_suffix, extra_args)`` for a TERMINAL resolve whose next free placeholder is ``$first`` + (ADR 0157 C1/C2). + + ``("", [])`` when unfenced — the emitted SQL is then CHARACTER-IDENTICAL to pre-0157 and the arg + list is unchanged. That identity is the SQLite/single-node parity anchor, and it is pinned by + ``test_unfenced_terminal_sql_is_character_identical``.""" + if self._leader_epoch is None: + return "", [] + return ( + _EPOCH_GUARD_RESOLVE.format(k=first, h=first + 1), + [self._lease_key, self._leader_epoch], + ) + + async def _exec_terminal( + self, + conn: Any, + method: str, + ids: tuple[str, ...], + sql: str, + *args: Any, + checked: bool = True, + ) -> None: + """Run a TERMINAL resolve UPDATE, raising :class:`_FencedWrite` when the epoch fence rejected it. + + ``checked`` is False when no guard was spliced (``mark_failed``'s retry branch), which makes "the + retry branch is never inspected" a property of the code rather than of an argument. Unfenced, + today's code does not read the rowcount at all — do not start: a zero rowcount there is the + already-vanished-row case the callers treat as an idempotent no-op.""" + result = await conn.execute(sql, *args) + if not checked or _rowcount(result) != 0: + return + raise _FencedWrite(method, ids) + + async def _after_fenced_write(self, exc: _FencedWrite) -> None: + """Count, log, and RE-PEND after the H1 fence rejected a terminal resolve (ADR 0157 C3 + D1). + + The transaction is already rolled back by the time this runs. The re-pend happens in a FRESH + transaction so the fence's own residue is a bounded DUPLICATE rather than an INFLIGHT row — + which on Postgres is ~90s of ``reclaim_expired_leases`` latency and on SQL Server (which has NO + periodic in-flight recovery at all) is an UNBOUNDED STRAND. ``release_claimed`` is + ``status='inflight'``-guarded so it is idempotent and cannot disturb an already-resolved row, and + it is the exact write C1 forbids FENCING — so re-pending here is invariant-legal by C1's own + rule. Best-effort: a raise here leaves the row INFLIGHT for recovery (the pre-D1 state).""" + self.fenced_writes += 1 + log.warning( + "H1 FENCE rejected a terminal %s for %d row(s): this node holds leader_epoch %s but " + "leader_lease has advanced. The write was rolled back WHOLE (no delivered_keys row, no " + "message_events row, no finalize) and the row(s) re-pended for the current leader — an " + "accepted duplicate, never a strand (ADR 0157 C1/C3/D1).", + exc.method, + len(exc.outbox_ids), + self._leader_epoch, + ) + try: + await self.release_claimed(list(exc.outbox_ids)) + except Exception: # noqa: BLE001 — recovery still covers the row; never mask the fence + log.warning( + "fenced %s: re-pend failed; row(s) left INFLIGHT for recovery", + exc.method, + exc_info=True, + ) + async def _insert_message( self, conn: Any, @@ -2606,7 +2715,30 @@ async def claim_ready( undecryptable payload is dead-lettered and dropped (poison-row containment), not raised.""" now = time.time() if now is None else now lease_until = now + self._settings.lease_ttl_seconds # Track B Step 2: stamp the lease + claim_args: list[Any] = [ + stage, # $1 + OutboxStatus.PENDING.value, # $2 + now, # $3 (due cutoff + updated_at) + channel_id, # $4 + destination_name, # $5 + limit, # $6 + OutboxStatus.INFLIGHT.value, # $7 + self._owner, # $8 + lease_until, # $9 + ] + # H1 FENCING TOKEN (ADR 0157 C5) — the UNORDERED claim is fenced exactly like the three FIFO + # claims: a superseded ex-leader's UPDATE matches 0 rows and it claims nothing. When fenced, the + # lease key + held epoch ride as $10/$11. + epoch_guard = "" + if self._leader_epoch is not None: + epoch_guard = _EPOCH_GUARD_CLAIM.format(k=10, h=11) + claim_args.extend([self._lease_key, self._leader_epoch]) # All filters are bound; explicit ::text casts let asyncpg type the optional-filter idiom. + # The guard rides the UPDATE, not the CTE: that is placement parity with all three FIFO claims, + # and it evaluates in the same snapshot as the write it gates. The honest cost of that choice is + # that a fenced claim's CTE still briefly SKIP LOCKED-hides those rows from a concurrent claimer; + # transient and self-correcting (the txn ends immediately), so it is accepted rather than hidden. + # `attempts` is untouched either way — a declined claim consumes no retry. sql = ( "WITH due AS (" " SELECT id FROM queue" @@ -2617,20 +2749,9 @@ async def claim_ready( ")" " UPDATE queue q SET status=$7, attempts=attempts+1, updated_at=$3," " owner=$8, lease_expires_at=$9" - " FROM due WHERE q.id=due.id RETURNING q.*" - ) - rows = await self._fetchall( - sql, - stage, - OutboxStatus.PENDING.value, - now, - channel_id, - destination_name, - limit, - OutboxStatus.INFLIGHT.value, - self._owner, - lease_until, + f" FROM due WHERE q.id=due.id{epoch_guard} RETURNING q.*" ) + rows = await self._fetchall(sql, *claim_args) items: list[OutboxItem] = [] for row in rows: try: @@ -2641,9 +2762,12 @@ async def claim_ready( return items def set_leader_epoch(self, epoch: int | None, *, lease_key: str | None = None) -> None: - # H1: the engine pushes the held leader epoch + lease key here on promotion/demotion (read from - # the coordinator — the store never imports it, ARCH-6). Stamps cached state only; the next - # claim_next_fifo validates it inside its single claim txn. epoch=None disables the guard. + # H1: the engine pushes the held leader epoch + lease key here on promotion (read from the + # coordinator — the store never imports it, ARCH-6). Stamps cached state only; the next claim or + # terminal resolve validates it inside that statement's own txn. + # epoch=None OMITS BOTH GUARDS ENTIRELY — None means NO FENCE, not a safe null token. That is why + # _stop_graph deliberately does NOT clear it on demotion (ADR 0157 C4): clearing would DISARM the + # guard at exactly the moment a superseded ex-leader may still be mid-teardown and mid-write. self._leader_epoch = epoch self._lease_key = lease_key @@ -2698,9 +2822,7 @@ async def claim_next_fifo( # `NULL <= $held` is false → fail-closed (no claim) rather than racing without a fence. epoch_guard = "" if self._leader_epoch is not None: - epoch_guard = ( - " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$8) <= $9" - ) + epoch_guard = _EPOCH_GUARD_CLAIM.format(k=8, h=9) head_sql = ( "WITH head AS (" f" SELECT id, next_attempt_at FROM queue WHERE stage=$1 AND {lane_col}=$2 AND status=$3" @@ -2828,9 +2950,7 @@ async def claim_next_fifo_batch( # When fenced, the lease key + held epoch are $9/$10 (after the fixed args + the LIMIT $8). epoch_guard = "" if self._leader_epoch is not None: - epoch_guard = ( - " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$9) <= $10" - ) + epoch_guard = _EPOCH_GUARD_CLAIM.format(k=9, h=10) claim_args.extend([self._lease_key, self._leader_epoch]) # Two-level CTE: the inner FOR UPDATE locks the N oldest pending rows (no window, no SKIP LOCKED -> # BLOCK on a producer-locked head, never skip — strict per-lane FIFO #285); the outer window @@ -2946,9 +3066,7 @@ async def claim_fifo_heads( lease_until, # $8 ] if self._leader_epoch is not None: - epoch_guard = ( - " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$9) <= $10" - ) + epoch_guard = _EPOCH_GUARD_CLAIM.format(k=9, h=10) claim_args.extend([self._lease_key, self._leader_epoch]) claim_sql = ( "WITH cand AS MATERIALIZED (" # STEP 1: plain MVCC snapshot read, non-locking @@ -3180,73 +3298,57 @@ async def dead_letter_now(self, outbox_id: str, error: str, now: float | None = error ) # PHI chokepoint (#120) — incl. the f"undecryptable payload: {exc}" callers now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is None: - return - await conn.execute( - "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," - " owner=NULL, lease_expires_at=NULL WHERE id=$5", - OutboxStatus.DEAD.value, - now, - self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), - now, - outbox_id, - ) - await self._event(conn, row["message_id"], "dead", row["destination_name"], error, now) - await self._maybe_finalize_message(conn, row["message_id"], now) + guard, guard_args = self._resolve_guard(6) # ADR 0157 C1 — TERMINAL + try: + async with self._timed_acquire() as conn, conn.transaction(): + row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) + if row is None: + return + await self._exec_terminal( + conn, + "dead_letter_now", + (outbox_id,), + "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," + " owner=NULL, lease_expires_at=NULL WHERE id=$5" + guard, + OutboxStatus.DEAD.value, + now, + self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), + now, + outbox_id, + *guard_args, + checked=bool(guard), + ) + await self._event( + conn, row["message_id"], "dead", row["destination_name"], error, now + ) + await self._maybe_finalize_message(conn, row["message_id"], now) + except _FencedWrite as exc: + # The sharpest of the terminal writes: a DEAD row is never re-claimed, so H2's + # skip-and-complete cannot heal a false dead-letter the way it heals a false DONE. + await self._after_fenced_write(exc) + return async def mark_done(self, outbox_id: str, now: float | None = None) -> None: now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is None: - return - await conn.execute( - "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," - " owner=NULL, lease_expires_at=NULL WHERE id=$3", - OutboxStatus.DONE.value, - now, - outbox_id, - ) - # H2: record the idempotency-ledger row in THIS same txn as the DONE flip. - await self._record_delivered_key( - conn, - outbox_id=outbox_id, - message_id=row["message_id"], - destination_name=row["destination_name"], - handler_name=row["handler_name"], - now=now, - ) - await self._event( - conn, - row["message_id"], - "delivered", - row["destination_name"], - f"attempt {row['attempts']}", - now, - ) - await self._maybe_finalize_message(conn, row["message_id"], now) - - async def mark_batch_done(self, outbox_ids: Sequence[str], now: float | None = None) -> None: - """Complete N delivered outbound rows in ONE transaction — the batch counterpart of - :meth:`mark_done` (ADR 0082). All N flip ``DONE`` together; each writes its H2 ledger row + - ``delivered`` event, and the finalizer runs once per distinct ``message_id``. A vanished member - is skipped; a crash before commit rolls all N back to ``INFLIGHT``.""" - now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - finalize: dict[str, None] = {} - for outbox_id in outbox_ids: + guard, guard_args = self._resolve_guard(4) # ADR 0157 C1 — TERMINAL + try: + async with self._timed_acquire() as conn, conn.transaction(): row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) if row is None: - continue # vanished member — idempotent no-op - await conn.execute( + return + await self._exec_terminal( + conn, + "mark_done", + (outbox_id,), "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," - " owner=NULL, lease_expires_at=NULL WHERE id=$3", + " owner=NULL, lease_expires_at=NULL WHERE id=$3" + guard, OutboxStatus.DONE.value, now, outbox_id, + *guard_args, + checked=bool(guard), ) + # H2: record the idempotency-ledger row in THIS same txn as the DONE flip. await self._record_delivered_key( conn, outbox_id=outbox_id, @@ -3263,9 +3365,65 @@ async def mark_batch_done(self, outbox_ids: Sequence[str], now: float | None = N f"attempt {row['attempts']}", now, ) - finalize[row["message_id"]] = None - for message_id in sorted(finalize): # H-8 canonical order (see below) - await self._maybe_finalize_message(conn, message_id, now) + await self._maybe_finalize_message(conn, row["message_id"], now) + except _FencedWrite as exc: + await self._after_fenced_write(exc) + return + + async def mark_batch_done(self, outbox_ids: Sequence[str], now: float | None = None) -> None: + """Complete N delivered outbound rows in ONE transaction — the batch counterpart of + :meth:`mark_done` (ADR 0082). All N flip ``DONE`` together; each writes its H2 ledger row + + ``delivered`` event, and the finalizer runs once per distinct ``message_id``. A vanished member + is skipped; a crash before commit rolls all N back to ``INFLIGHT``.""" + now = time.time() if now is None else now + # ADR 0157 C1 — TERMINAL. Rendered ONCE for the whole loop: a fence on ANY member raises out and + # rolls all N back, which is exactly the all-or-nothing contract the docstring already promises. + guard, guard_args = self._resolve_guard(4) + try: + async with self._timed_acquire() as conn, conn.transaction(): + finalize: dict[str, None] = {} + for outbox_id in outbox_ids: + row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) + if row is None: + continue # vanished member — idempotent no-op + await self._exec_terminal( + conn, + "mark_batch_done", + # ALL N, not the prefix walked so far: the rollback undoes every member, and + # members past the raise were never touched — but all N are still INFLIGHT from + # the claim, so all N need re-pending. release_claimed is status-guarded, so a + # vanished member is a harmless no-op. + tuple(outbox_ids), + "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," + " owner=NULL, lease_expires_at=NULL WHERE id=$3" + guard, + OutboxStatus.DONE.value, + now, + outbox_id, + *guard_args, + checked=bool(guard), + ) + await self._record_delivered_key( + conn, + outbox_id=outbox_id, + message_id=row["message_id"], + destination_name=row["destination_name"], + handler_name=row["handler_name"], + now=now, + ) + await self._event( + conn, + row["message_id"], + "delivered", + row["destination_name"], + f"attempt {row['attempts']}", + now, + ) + finalize[row["message_id"]] = None + for message_id in sorted(finalize): # H-8 canonical order (see below) + await self._maybe_finalize_message(conn, message_id, now) + except _FencedWrite as exc: + await self._after_fenced_write(exc) + return async def complete_with_response( self, @@ -3286,90 +3444,108 @@ async def complete_with_response( finalizer (it scans ``queue`` only). When ``reingress_to`` is set (Increment 2) the same transaction also inserts the drainable ``Stage.RESPONSE`` work-row (identical to SQLite).""" now = time.time() if now is None else now - async with self._timed_acquire() as conn: # noqa: SIM117 - async with conn.transaction(): - row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is None: - return - message_id = row["message_id"] - destination_name = row["destination_name"] - await conn.execute( - "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," - " owner=NULL, lease_expires_at=NULL WHERE id=$3", - OutboxStatus.DONE.value, - now, - outbox_id, - ) - seq = await conn.fetchval( - "SELECT COALESCE(MAX(response_seq), 0) + 1 FROM response" - " WHERE message_id=$1 AND destination_name=$2", - message_id, - destination_name, - ) - headers_json = encode_response_headers(response_headers) # #154 - await conn.execute( - "INSERT INTO response" - " (message_id, destination_name, response_seq, body, outcome, detail," - " resp_headers, captured_at)" - " VALUES ($1,$2,$3,$4,$5,$6,$7,$8)", - message_id, - destination_name, - seq, - self._enc( - body, aad=cell_aad("response", "body", message_id, destination_name, seq) - ), - outcome, - self._enc( - detail, - aad=cell_aad("response", "detail", message_id, destination_name, seq), - ), - self._enc( - headers_json, - aad=cell_aad("response", "resp_headers", message_id, destination_name, seq), - ), - now, - ) - if reingress_to is not None: - # ADR 0013 Increment 2: drainable Stage.RESPONSE work-row in the SAME txn (orphan-free) - # — a token referencing the immutable artifact by its PK, on the loopback inbound's lane. - artifact_ref = f"{message_id}\x1f{destination_name}\x1f{seq}" - # ingest-time (ADR 0009) + metrics only; per-lane FIFO orders by seq — ADR 0059. - work_created = now - # Hoist the row id so the artifact-ref payload binds to its own (queue, payload, id) cell. - work_row_id = uuid4().hex + guard, guard_args = self._resolve_guard(4) + try: + async with self._timed_acquire() as conn: # noqa: SIM117 + async with conn.transaction(): + row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) + if row is None: + return + message_id = row["message_id"] + destination_name = row["destination_name"] + # ADR 0157 C1 — TERMINAL, and the guard rides THIS (the first) statement deliberately: + # a rejection then discards the `response` artifact, the Stage.RESPONSE re-ingress + # work-row, the ledger row and the finalize TOGETHER. Do not try to keep the response + # row — an artifact with no resolved queue row is precisely the half-applied state C3 + # rejects. The successor re-sends and captures a fresh reply. + await self._exec_terminal( + conn, + "complete_with_response", + (outbox_id,), + "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," + " owner=NULL, lease_expires_at=NULL WHERE id=$3" + guard, + OutboxStatus.DONE.value, + now, + outbox_id, + *guard_args, + checked=bool(guard), + ) + seq = await conn.fetchval( + "SELECT COALESCE(MAX(response_seq), 0) + 1 FROM response" + " WHERE message_id=$1 AND destination_name=$2", + message_id, + destination_name, + ) + headers_json = encode_response_headers(response_headers) # #154 await conn.execute( - "INSERT INTO queue" - " (id, message_id, stage, channel_id, destination_name, handler_name, payload," - " status, attempts, next_attempt_at, created_at, updated_at)" - " VALUES ($1,$2,$3,$4,NULL,NULL,$5,$6,0,$7,$8,$9)", - work_row_id, + "INSERT INTO response" + " (message_id, destination_name, response_seq, body, outcome, detail," + " resp_headers, captured_at)" + " VALUES ($1,$2,$3,$4,$5,$6,$7,$8)", message_id, - Stage.RESPONSE.value, - reingress_to, - self._enc(artifact_ref, aad=cell_aad("queue", "payload", work_row_id)), - OutboxStatus.PENDING.value, + destination_name, + seq, + self._enc( + body, + aad=cell_aad("response", "body", message_id, destination_name, seq), + ), + outcome, + self._enc( + detail, + aad=cell_aad("response", "detail", message_id, destination_name, seq), + ), + self._enc( + headers_json, + aad=cell_aad( + "response", "resp_headers", message_id, destination_name, seq + ), + ), now, - work_created, + ) + if reingress_to is not None: + # ADR 0013 Increment 2: drainable Stage.RESPONSE work-row in the SAME txn (orphan-free) + # — a token referencing the immutable artifact by its PK, on the loopback inbound's lane. + artifact_ref = f"{message_id}\x1f{destination_name}\x1f{seq}" + # ingest-time (ADR 0009) + metrics only; per-lane FIFO orders by seq — ADR 0059. + work_created = now + # Hoist the row id so the artifact-ref payload binds to its own (queue, payload, id) cell. + work_row_id = uuid4().hex + await conn.execute( + "INSERT INTO queue" + " (id, message_id, stage, channel_id, destination_name, handler_name, payload," + " status, attempts, next_attempt_at, created_at, updated_at)" + " VALUES ($1,$2,$3,$4,NULL,NULL,$5,$6,0,$7,$8,$9)", + work_row_id, + message_id, + Stage.RESPONSE.value, + reingress_to, + self._enc(artifact_ref, aad=cell_aad("queue", "payload", work_row_id)), + OutboxStatus.PENDING.value, + now, + work_created, + now, + ) + # H2: idempotency-ledger row joins this SAME txn as the DONE flip + the response artifact. + await self._record_delivered_key( + conn, + outbox_id=outbox_id, + message_id=message_id, + destination_name=destination_name, + handler_name=row["handler_name"], + now=now, + ) + await self._event( + conn, + message_id, + "delivered", + destination_name, + f"attempt {row['attempts']} (response {outcome})", now, ) - # H2: idempotency-ledger row joins this SAME txn as the DONE flip + the response artifact. - await self._record_delivered_key( - conn, - outbox_id=outbox_id, - message_id=message_id, - destination_name=destination_name, - handler_name=row["handler_name"], - now=now, - ) - await self._event( - conn, - message_id, - "delivered", - destination_name, - f"attempt {row['attempts']} (response {outcome})", - now, - ) - await self._maybe_finalize_message(conn, message_id, now) + await self._maybe_finalize_message(conn, message_id, now) + except _FencedWrite as exc: + await self._after_fenced_write(exc) + return async def ingress_handoff( self, @@ -3391,6 +3567,11 @@ class _Noop(Exception): pass now = time.time() if now is None else now + # ADR 0157 C1 — both DEAD branches below are TERMINAL on a claimed RESPONSE work-row AND finalize + # the origin message, so both are guarded. The SUCCESS path is deliberately NOT guarded: it + # *admits* a message, and fencing it would convert a permitted duplicate into a LOST re-ingress, + # breaking count-and-log. + guard, guard_args = self._resolve_guard(6) try: async with self._timed_acquire() as conn: # noqa: SIM117 async with conn.transaction(): @@ -3418,9 +3599,12 @@ class _Noop(Exception): # the token + ERROR the origin in THIS transaction and CONSUME it (return True ⇒ # the conn.transaction() commits) — never re-loop. Mirrors the SQLite branch and # the depth-cap branch (NOT the _Noop rollback, which would leave the token live). - await conn.execute( + await self._exec_terminal( + conn, + "ingress_handoff(corrupt-ref)", + (response_row_id,), "UPDATE queue SET status=$1, last_error=$2, next_attempt_at=$3," - " updated_at=$4 WHERE id=$5", + " updated_at=$4 WHERE id=$5" + guard, OutboxStatus.DEAD.value, self._enc( "re-ingress work-row reference is corrupt/unparseable", @@ -3429,6 +3613,8 @@ class _Noop(Exception): now, now, response_row_id, + *guard_args, + checked=bool(guard), ) await self._event( conn, origin_id, "dead", None, "re-ingress ref corrupt", now @@ -3468,9 +3654,12 @@ class _Noop(Exception): child_depth = int(origin_meta.get("correlation_depth", 0) or 0) + 1 root = origin_meta.get("correlation_root_id") or origin_id if child_depth > correlation_depth_cap: - await conn.execute( + await self._exec_terminal( + conn, + "ingress_handoff(depth-cap)", + (response_row_id,), "UPDATE queue SET status=$1, last_error=$2, next_attempt_at=$3," - " updated_at=$4 WHERE id=$5", + " updated_at=$4 WHERE id=$5" + guard, OutboxStatus.DEAD.value, self._enc( f"re-ingress correlation depth exceeded " @@ -3480,6 +3669,8 @@ class _Noop(Exception): now, now, response_row_id, + *guard_args, + checked=bool(guard), ) await self._event( conn, @@ -3569,6 +3760,12 @@ class _Noop(Exception): await self._maybe_finalize_message(conn, origin_id, now) except _Noop: return False + except _FencedWrite as exc: + # Same shape as _Noop: the transaction rolled back, so the work-row is untouched and still + # claimed. D1 re-pends it. Returning False only gates a wake in _process_response_item, and + # the response worker's next claim is itself fenced — so there is no hot spin. + await self._after_fenced_write(exc) + return False return True async def response_body_for_work_row(self, response_row_id: str) -> str | None: @@ -4116,96 +4313,131 @@ async def mark_failed( arms the per-lane retry wake on a float — WS-C; see the base contract).""" error = safe_text(error) # PHI chokepoint (#120) now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is None: - return None - attempts = row["attempts"] - # max_attempts None = retry forever; a finite cap dead-letters once exhausted. - if retry.max_attempts is not None and attempts >= retry.max_attempts: - status, next_at, event = OutboxStatus.DEAD.value, now, "dead" - else: - backoff = min( - retry.max_backoff_seconds, - retry.backoff_seconds * (retry.backoff_multiplier ** (attempts - 1)), - ) - status, next_at, event = OutboxStatus.PENDING.value, now + backoff, "failed" - await conn.execute( - "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," - " owner=NULL, lease_expires_at=NULL WHERE id=$5", - status, - next_at, - self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), - now, - outbox_id, - ) - await self._event( - conn, - row["message_id"], - event, - row["destination_name"], - f"attempt {attempts}: {error}", - now, - ) - if status == OutboxStatus.DEAD.value: - await self._maybe_finalize_message(conn, row["message_id"], now) - return None - return next_at - - async def mark_batch_failed( - self, - outbox_ids: Sequence[str], - error: str, - retry: RetryPolicy, - now: float | None = None, - ) -> float | None: - """Re-pend (or dead-letter) N outbound rows that failed **as a unit** — the batch counterpart of - :meth:`mark_failed` (ADR 0082). One disposition, decided from the head member's attempts and - applied identically to all N (same ``next_attempt_at`` → re-claimed as the identical prefix, or - all dead-letter together). Returns the shared ``next_attempt_at`` or ``None`` on dead-letter.""" - error = safe_text(error) # PHI chokepoint (#120) - now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - present = [] - for outbox_id in outbox_ids: + try: + async with self._timed_acquire() as conn, conn.transaction(): row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is not None: - present.append((outbox_id, row)) - if not present: - return None - head_attempts = present[0][1]["attempts"] - if retry.max_attempts is not None and head_attempts >= retry.max_attempts: - status, next_at, event = OutboxStatus.DEAD.value, now, "dead" - else: - backoff = min( - retry.max_backoff_seconds, - retry.backoff_seconds * (retry.backoff_multiplier ** (head_attempts - 1)), + if row is None: + return None + attempts = row["attempts"] + # max_attempts None = retry forever; a finite cap dead-letters once exhausted. + if retry.max_attempts is not None and attempts >= retry.max_attempts: + status, next_at, event = OutboxStatus.DEAD.value, now, "dead" + else: + backoff = min( + retry.max_backoff_seconds, + retry.backoff_seconds * (retry.backoff_multiplier ** (attempts - 1)), + ) + status, next_at, event = OutboxStatus.PENDING.value, now + backoff, "failed" + # ADR 0157 C1 — guard the DEAD branch ONLY. The retry branch returns the row to PENDING; + # fencing THAT would leave it INFLIGHT instead — converting a permitted duplicate into a + # forbidden strand. The suffix is "" on the retry branch, so its statement and arg list + # are byte-identical to pre-0157, and checked=False means it is never even inspected. + # ONE statement with a conditional suffix, not two: the DEAD/retry decision is already + # computed above, strictly before the UPDATE. + guard, guard_args = ( + self._resolve_guard(6) if status == OutboxStatus.DEAD.value else ("", []) ) - status, next_at, event = OutboxStatus.PENDING.value, now + backoff, "failed" - finalize: dict[str, None] = {} - for outbox_id, row in present: - await conn.execute( + await self._exec_terminal( + conn, + "mark_failed(dead)", + (outbox_id,), "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," - " owner=NULL, lease_expires_at=NULL WHERE id=$5", + " owner=NULL, lease_expires_at=NULL WHERE id=$5" + guard, status, next_at, self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), now, outbox_id, + *guard_args, + checked=bool(guard), ) await self._event( conn, row["message_id"], event, row["destination_name"], - f"attempt {row['attempts']}: {error}", + f"attempt {attempts}: {error}", now, ) if status == OutboxStatus.DEAD.value: - finalize[row["message_id"]] = None - for message_id in sorted(finalize): # H-8 canonical order (see below) - await self._maybe_finalize_message(conn, message_id, now) - return None if status == OutboxStatus.DEAD.value else next_at + await self._maybe_finalize_message(conn, row["message_id"], now) + return None + return next_at + except _FencedWrite as exc: + await self._after_fenced_write(exc) + # None is what the DEAD branch returns today, so _mark_failed_and_arm arms no retry timer. + # Correct: D1 re-pended the row and only the SUCCESSOR may claim it. + return None + + async def mark_batch_failed( + self, + outbox_ids: Sequence[str], + error: str, + retry: RetryPolicy, + now: float | None = None, + ) -> float | None: + """Re-pend (or dead-letter) N outbound rows that failed **as a unit** — the batch counterpart of + :meth:`mark_failed` (ADR 0082). One disposition, decided from the head member's attempts and + applied identically to all N (same ``next_attempt_at`` → re-claimed as the identical prefix, or + all dead-letter together). Returns the shared ``next_attempt_at`` or ``None`` on dead-letter.""" + error = safe_text(error) # PHI chokepoint (#120) + now = time.time() if now is None else now + try: + async with self._timed_acquire() as conn, conn.transaction(): + present = [] + for outbox_id in outbox_ids: + row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) + if row is not None: + present.append((outbox_id, row)) + if not present: + return None + head_attempts = present[0][1]["attempts"] + if retry.max_attempts is not None and head_attempts >= retry.max_attempts: + status, next_at, event = OutboxStatus.DEAD.value, now, "dead" + else: + backoff = min( + retry.max_backoff_seconds, + retry.backoff_seconds * (retry.backoff_multiplier ** (head_attempts - 1)), + ) + status, next_at, event = OutboxStatus.PENDING.value, now + backoff, "failed" + # ADR 0157 C1 — the identical DEAD-branch-only split as mark_failed, decided ONCE from + # head_attempts and rendered ONCE for the whole loop: a fence on any member raises out + # and rolls all N back, matching the all-or-nothing contract the docstring promises. + guard, guard_args = ( + self._resolve_guard(6) if status == OutboxStatus.DEAD.value else ("", []) + ) + finalize: dict[str, None] = {} + for outbox_id, row in present: + await self._exec_terminal( + conn, + "mark_batch_failed(dead)", + tuple(oid for oid, _row in present), + "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," + " owner=NULL, lease_expires_at=NULL WHERE id=$5" + guard, + status, + next_at, + self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), + now, + outbox_id, + *guard_args, + checked=bool(guard), + ) + await self._event( + conn, + row["message_id"], + event, + row["destination_name"], + f"attempt {row['attempts']}: {error}", + now, + ) + if status == OutboxStatus.DEAD.value: + finalize[row["message_id"]] = None + for message_id in sorted(finalize): # H-8 canonical order (see below) + await self._maybe_finalize_message(conn, message_id, now) + return None if status == OutboxStatus.DEAD.value else next_at + except _FencedWrite as exc: + await self._after_fenced_write(exc) + return None async def dead_letter_batch( self, outbox_ids: Sequence[str], error: str, now: float | None = None @@ -4214,27 +4446,39 @@ async def dead_letter_batch( :meth:`dead_letter_now` (ADR 0082 decision #1: a permanent envelope reject dead-letters all N).""" error = safe_text(error) # PHI chokepoint (#120) now = time.time() if now is None else now - async with self._timed_acquire() as conn, conn.transaction(): - finalize: dict[str, None] = {} - for outbox_id in outbox_ids: - row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) - if row is None: - continue - await conn.execute( - "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," - " owner=NULL, lease_expires_at=NULL WHERE id=$5", - OutboxStatus.DEAD.value, - now, - self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), - now, - outbox_id, - ) - await self._event( - conn, row["message_id"], "dead", row["destination_name"], error, now - ) - finalize[row["message_id"]] = None - for message_id in sorted(finalize): # H-8 canonical order (see below) - await self._maybe_finalize_message(conn, message_id, now) + guard, guard_args = self._resolve_guard( + 6 + ) # ADR 0157 C1 — TERMINAL, once for the whole loop + try: + async with self._timed_acquire() as conn, conn.transaction(): + finalize: dict[str, None] = {} + for outbox_id in outbox_ids: + row = await conn.fetchrow("SELECT * FROM queue WHERE id=$1", outbox_id) + if row is None: + continue + await self._exec_terminal( + conn, + "dead_letter_batch", + tuple(outbox_ids), + "UPDATE queue SET status=$1, next_attempt_at=$2, last_error=$3, updated_at=$4," + " owner=NULL, lease_expires_at=NULL WHERE id=$5" + guard, + OutboxStatus.DEAD.value, + now, + self._enc(error, aad=cell_aad("queue", "last_error", outbox_id)), + now, + outbox_id, + *guard_args, + checked=bool(guard), + ) + await self._event( + conn, row["message_id"], "dead", row["destination_name"], error, now + ) + finalize[row["message_id"]] = None + for message_id in sorted(finalize): # H-8 canonical order (see below) + await self._maybe_finalize_message(conn, message_id, now) + except _FencedWrite as exc: + await self._after_fenced_write(exc) + return async def pending_depth( self, name: str, *, stage: str = Stage.OUTBOUND.value diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 924d8b7..e279f4e 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -1695,6 +1695,10 @@ def __init__( # insert helpers; no new lock, no commit-boundary change. See tests/test_live_cost_counters.py. self.committed_txns = 0 self.body_copies = 0 + # ADR 0157 C3: protocol/`/stats` uniformity only. SQL Server's terminal resolves are NOT yet + # epoch-fenced (that is ADR 0157 Inc 3), so this stays 0 on this backend until then. Declared + # because Store (base.py) requires it and open_store() returns this class as a Store. + self.fenced_writes = 0 async def _commit(self, conn: Any) -> None: """Commit a durable **write**-path transaction and count it (A1 live cost counters). A bare async diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 30ff15c..06824b5 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -1821,6 +1821,10 @@ def __init__( # (before the committer below) so its note_commit hook can bump committed_txns from batch start. self.committed_txns = 0 self.body_copies = 0 + # ADR 0157 C3: protocol/`/stats` uniformity only. SQLite never fences a terminal resolve — + # set_leader_epoch is a hard no-op here and [cluster].enabled is rejected on SQLite at config + # load — so this is a permanent 0 and there is no increment site in this module. + self.fenced_writes = 0 self._cipher: Cipher = cipher or IdentityCipher() # HKDF-derived HMAC key for the tamper-evident audit chain (#190). None → the chain stays the # keyless SHA-256 chain (byte-identical to a pre-#190 / unencrypted store). Held only in memory; diff --git a/messagefoundry/transports/direct.py b/messagefoundry/transports/direct.py index 1209f5a..59663a7 100644 --- a/messagefoundry/transports/direct.py +++ b/messagefoundry/transports/direct.py @@ -46,6 +46,7 @@ import asyncio import logging import smtplib +import ssl from collections.abc import Mapping from email.message import EmailMessage from pathlib import Path @@ -57,7 +58,11 @@ from cryptography.hazmat.primitives.serialization import pkcs7 from messagefoundry.config.models import ConnectorType, Destination -from messagefoundry.config.settings import INSECURE_TLS_ESCAPE_ENV, insecure_tls_allowed +from messagefoundry.config.settings import ( + INSECURE_TLS_ESCAPE_ENV, + weakened_tls_escape_permitted_here, +) +from messagefoundry.config.tls_policy import build_smtp_tls_context from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -140,6 +145,13 @@ def __init__(self, config: Destination) -> None: self.username: str | None = str(username) if username else None self.password: str | None = str(password) if password else None self.use_tls = bool(s.get("use_tls", True)) + # #323: server-certificate verification on the TLS hop, kept byte-identical to + # EmailDestination's spelling (this connector's SMTP core is a deliberate copy, not an import — + # the one-way dependency rule — so the two must not drift). + self.tls_verify = bool(s.get("tls_verify", True)) + tls_ca_file = s.get("tls_ca_file") + self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None + self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) self.timeout: float = float(s.get("timeout_seconds", 30.0)) self.encoding: str = str(s.get("encoding", "utf-8")) @@ -167,11 +179,26 @@ def __init__(self, config: Destination) -> None: # SMTP is refused unless the project-wide dev escape is set, and credentials are NEVER sent over # a cleartext channel. if not self.use_tls: - if not insecure_tls_allowed(): + # #323: read through the CLAMPED escape, not the raw insecure_tls_allowed(). This call site + # was the last unclamped one in the file, sitting one branch away from the clamped arm + # below — two different escapes in one connector is how the next bug gets written. The + # change strictly ADDS refusals (ADR 0092 decision 5): an enforcing production-PHI instance + # can no longer silence a cleartext Direct hop with the blunt process-wide env var. + # + # THE ESCAPE STILL EXISTS — it is CLAMPED, not removed. Stated because this file now has + # NO CALL to `insecure_tls_allowed()` (only these comments mention it), and reading that + # as "this connector has no escape" would be a FALSE ABSENCE claim. + # MEFOR_ALLOW_INSECURE_TLS still governs this hop: weakened_tls_escape_permitted_here() is + # that same env var plus the production-PHI clamp. Any absence claim about the raw call is + # scoped to THIS FILE and never repo-wide — `insecure_tls_allowed()` remains live at call + # sites in auth/ldap.py, pipeline/alert_sinks.py, transports/{ai_broker,database,mllp}.py + # and config/settings.py (docs/DEPLOYMENT.md enumerates the ones the clamp does not yet + # cover — see #329). Verified by grep at the time of writing, not assumed. + if not weakened_tls_escape_permitted_here(): raise ValueError( "Direct destination use_tls=false submits over cleartext SMTP; refused unless " - f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use STARTTLS " - "(the default)" + f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only, and refused on a " + "production-PHI instance even with the escape, #200) — use STARTTLS (the default)" ) if self.username is not None: raise ValueError( @@ -183,6 +210,40 @@ def __init__(self, config: Destination) -> None: "network in CLEARTEXT (dev/trusted-network only)", self.host, ) + elif not self.tls_verify: + # #323 arm 2 — same shape and wording as EmailDestination's, deliberately. + if not weakened_tls_escape_permitted_here(): + raise ValueError( + "Direct destination tls_verify=false disables server-certificate verification on " + f"the SMTP hop to {self.host} — the session is encrypted but UNAUTHENTICATED. The " + "S/MIME body still protects the clinical payload, but envelope metadata and any " + "SMTP AUTH credential are exposed to an on-path attacker presenting any " + "certificate. Use a trusted CA (tls_ca_file, or [tls].internal_ca_file for the " + f"instance), or set {INSECURE_TLS_ESCAPE_ENV}=1 to allow it on a trusted-network " + "bind (refused on a production-PHI instance even with the escape, #200)." + ) + if self.username is not None: + raise ValueError( + "Direct destination sends SMTP AUTH credentials over an UNVERIFIED TLS session " + "(tls_verify=false); refused — credentials require a verified TLS session" + ) + # Built once at construction (fail-fast), reused by every send. None when TLS is off entirely. + # DIRECT does not take a RevocationHopGuard even though the hop now verifies: adding it would + # make the enumerated count eight and force four "seven verifying hops" docs to change, and the + # clinical payload is S/MIME-protected at the message layer so the PHI argument is materially + # weaker than EMAIL's (ADR 0085). Recorded rather than silently omitted. + self._tls_context: ssl.SSLContext | None = ( + build_smtp_tls_context( + host=self.host, + cell="Direct destination", + verify=self.tls_verify, + ca_file=self.tls_ca_file, + check_hostname=self.tls_check_hostname, + trust_anchor_policy=config.trust_anchor_policy, + ) + if self.use_tls + else None + ) def _load_private_key(self, value: Any, password: Any) -> Any: """Load the sender's signing private key (PEM/DER, optionally passphrase-protected). PHI/secret- @@ -317,10 +378,13 @@ def _connect(self) -> smtplib.SMTP: """Open an SMTP connection, applying STARTTLS / implicit TLS per config (identical posture to EmailDestination). The caller closes it (``with`` / ``quit``).""" if self.port == 465 and self.use_tls: - return smtplib.SMTP_SSL(self.host, self.port, timeout=self.timeout) + # context= is REQUIRED (#323) — SMTP_SSL's own default is an unverified stdlib context. + return smtplib.SMTP_SSL( + self.host, self.port, timeout=self.timeout, context=self._tls_context + ) smtp = smtplib.SMTP(self.host, self.port, timeout=self.timeout) if self.use_tls: - smtp.starttls() + smtp.starttls(context=self._tls_context) return smtp def _send(self, payload: str) -> None: diff --git a/messagefoundry/transports/email.py b/messagefoundry/transports/email.py index 0a47cff..81749a4 100644 --- a/messagefoundry/transports/email.py +++ b/messagefoundry/transports/email.py @@ -44,6 +44,7 @@ import asyncio import logging import smtplib +import ssl from collections.abc import Mapping from email.message import EmailMessage from typing import Any @@ -53,7 +54,7 @@ INSECURE_TLS_ESCAPE_ENV, weakened_tls_escape_permitted_here, ) -from messagefoundry.config.tls_policy import RevocationHopGuard +from messagefoundry.config.tls_policy import RevocationHopGuard, build_smtp_tls_context from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -108,6 +109,12 @@ def __init__(self, config: Destination) -> None: self.username: str | None = str(username) if username else None self.password: str | None = str(password) if password else None self.use_tls = bool(s.get("use_tls", True)) + # #323: server-certificate verification on the TLS hop. Defaults ON — smtplib's own default is + # an UNVERIFIED context, so before this the encrypted session was unauthenticated. + self.tls_verify = bool(s.get("tls_verify", True)) + tls_ca_file = s.get("tls_ca_file") + self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None + self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) self.timeout: float = float(s.get("timeout_seconds", 30.0)) self.encoding: str = str(s.get("encoding", "utf-8")) @@ -169,22 +176,66 @@ def __init__(self, config: Destination) -> None: connection=config.name, ) self._hop_guard.enforce_construction() + elif not self.tls_verify: + # #323 arm 2: TLS is on but verification is off. Refused unless the CLAMPED escape permits + # it (#200, ADR 0092 decision 2) — inlined here exactly as the MLLP and FTPS verify-off arms + # inline it (remotefile.py:176, mllp.py:547), not routed through a shared helper, because + # the sibling cells do it this way and a third spelling is how the next bug gets written. + # NOTE the raw insecure_tls_allowed() is deliberately NOT used: the blunt process-wide + # escape must not silence a verify-off PHI hop on an enforcing instance. + if not weakened_tls_escape_permitted_here(): + raise ValueError( + "Email destination tls_verify=false disables server-certificate verification on " + f"the SMTP hop to {self.host} — the session is encrypted but UNAUTHENTICATED, so " + "an on-path attacker presenting any certificate reads the message body (PHI) and " + "any SMTP AUTH credential. Use a trusted CA (tls_ca_file, or [tls].internal_ca_file " + f"for the instance), or set {INSECURE_TLS_ESCAPE_ENV}=1 to allow it on a " + "trusted-network bind (refused on a production-PHI instance even with the escape, " + "#200)." + ) + # Credentials over an unauthenticated channel are refused for the same reason they are + # refused over cleartext: an on-path attacker who can present any cert can capture the + # AUTH exchange. The use_tls=false arm above states the cleartext half of this rule. + if self.username is not None: + raise ValueError( + "Email destination sends SMTP AUTH credentials over an UNVERIFIED TLS session " + "(tls_verify=false); refused — credentials require a verified TLS session" + ) + # No RevocationHopGuard here: a hop that does not verify the certificate at all has nothing + # whose revocation status could matter, and the composition rule forbids two gates deciding + # the same hop (docs/DEPLOYMENT.md §"composition"). The refusal above is that hop's gate. else: - # #201 (ADR 0078 amendment): STARTTLS (587) / implicit-TLS SMTP_SSL (465) verifies the server - # cert, but smtplib's default SSL context does NO OCSP/CRL revocation (stdlib ssl has none) — - # so a revoked-but-unexpired SMTP-server cert is still accepted. The message body carries PHI - # over this verified hop, so refuse an off-loopback production-PHI hop unless revocation is - # attested (loopback / synthetic / non-prod / attested byte-identical). Same posture-keyed - # RevocationHopGuard as the REST/SOAP/FHIR/DICOMweb https paths; SMTP is a different construction - # seam (smtplib, not the urllib/ssl scheme), so it calls the guard directly rather than the - # https-scheme-keyed refuse_unrevoked_verified_hop wrapper. Composes with the cleartext refusal - # above (that gate keys on use_tls=false; this one only on the verifying use_tls=true path). + # #201 (ADR 0078 amendment): the hop now genuinely verifies the server cert (#323 — it did + # not before; smtplib's default context is ssl._create_unverified_context), but stdlib ssl + # does NO OCSP/CRL revocation, so a revoked-but-unexpired SMTP-server cert is still + # accepted. The message body carries PHI over this verified hop, so refuse an off-loopback + # production-PHI hop unless revocation is attested (loopback / synthetic / non-prod / + # attested byte-identical). Same posture-keyed RevocationHopGuard as the REST/SOAP/FHIR/ + # DICOMweb https paths; SMTP is a different construction seam (smtplib, not the urllib/ssl + # scheme), so it calls the guard directly rather than the https-scheme-keyed + # refuse_unrevoked_verified_hop wrapper. Composes with the two refusals above: this arm is + # reached only when use_tls=true AND tls_verify=true, which is the guard's documented + # precondition (tls_policy.py: "the caller has already built a verifying context") — that + # precondition was VIOLATED by this call site before #323. RevocationHopGuard.capture( host=self.host, cell="Email destination (verified SMTP TLS, no revocation check)", description="delivers over verified SMTP TLS but performs no certificate revocation checking", attested=config.tls_revocation_attested, ).enforce_construction() + # Built once at construction (fail-fast), reused by every send. None when TLS is off entirely. + self._tls_context: ssl.SSLContext | None = ( + build_smtp_tls_context( + host=self.host, + cell="Email destination", + verify=self.tls_verify, + ca_file=self.tls_ca_file, + check_hostname=self.tls_check_hostname, + trust_anchor_policy=config.trust_anchor_policy, + ) + if self.use_tls + else None + ) async def send( self, payload: str, *, metadata: Mapping[str, str] | None = None @@ -210,10 +261,13 @@ def _connect(self) -> smtplib.SMTP: if self.port == 465 and self.use_tls: # Port 465 is implicit TLS — the whole session is wrapped from connect, so SMTP_SSL rather # than SMTP+STARTTLS (ADR 0029 §D3). The submission port (587) keeps STARTTLS below. - return smtplib.SMTP_SSL(self.host, self.port, timeout=self.timeout) + # context= is REQUIRED (#323): SMTP_SSL's own default is an unverified stdlib context. + return smtplib.SMTP_SSL( + self.host, self.port, timeout=self.timeout, context=self._tls_context + ) smtp = smtplib.SMTP(self.host, self.port, timeout=self.timeout) if self.use_tls: - smtp.starttls() + smtp.starttls(context=self._tls_context) return smtp def _send(self, payload: str) -> None: diff --git a/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 590626e..55ff4e0 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -228,7 +228,15 @@ "messagefoundry/transports/dicomweb.py": frozenset({"secrets"}), # ADR 0085: DIRECT-HISP outbound S/MIME — cryptography.serialization.pkcs7 SIGN then ENCRYPT of the # Handler body + x509 recipient-cert / trust-anchor cross-validation at construction. No new dependency. - "messagefoundry/transports/direct.py": frozenset({"cryptography"}), + # #323: ssl = the SMTP/HISP relay's TRANSPORT hop, a separate concern from the S/MIME message + # layer above. The context is built by tls_policy.build_smtp_tls_context and handed to smtplib, + # which otherwise defaults to ssl._create_stdlib_context -- which IS _create_unverified_context + # (CERT_NONE / check_hostname=False). CERT_NONE only under the CLAMPED tls_verify=false escape. + "messagefoundry/transports/direct.py": frozenset({"cryptography", "ssl"}), + # #323: EMAIL outbound STARTTLS (587) / implicit TLS (465). Same factory, same reason as above -- + # an explicit verifying context anchored to the OS roots, a per-connection tls_ca_file, or + # [tls].internal_ca_file (ADR 0093), because smtplib's own default verifies nothing. + "messagefoundry/transports/email.py": frozenset({"ssl"}), # ADR 0129 (#142): hashlib = sha256 of a source file's identity (name+mtime+size) as a HASHED dedup # key for the leave-in-place processed_files ledger — a DERIVED id, never a cleartext filename (which # can embed an MRN), never logged. Not a secret/keyed primitive. diff --git a/tests/test_adr0157_fence_scope.py b/tests/test_adr0157_fence_scope.py new file mode 100644 index 0000000..d4ba465 --- /dev/null +++ b/tests/test_adr0157_fence_scope.py @@ -0,0 +1,340 @@ +"""Structural gate for ADR 0157's H1 epoch-fence scope on ``store/postgres.py``. + +**Deliberately NOT env-gated.** Every other Postgres test needs ``MEFOR_TEST_POSTGRES`` and a live +server, so on a developer laptop they silently skip — 147 tests that prove nothing about a laptop run. +This file parses source, so it runs on *every* leg and is the only thing standing between "the fence +covers every write it must" and a silent regression. + +**Why AST and not a source regex (ADR 0157 D3).** ``claim_fifo_heads`` splits its claim across two +adjacent string literals with an inline ``# STEP 5`` comment between them:: + + " UPDATE queue q" # STEP 5: claim exactly the kept prefixes + " SET status=$6, attempts=attempts+1, ..." + +A raw-slice regex for ``UPDATE queue (?:q )?SET status`` **cannot see** that site — the one site C5's +whole argument depends on. CPython folds implicit concatenation at parse time, so reconstructing from +the AST makes it visible. + +**Why this file keys on the EMITTED SQL, not on a name reference.** The first version of this gate +asked "does this method mention ``_EPOCH_GUARD_CLAIM``?". Deleting ``{epoch_guard}`` from +``claim_fifo_heads``' SQL — the precise regression it exists to catch — left that mention intact +(``epoch_guard`` is still *computed*, just no longer *interpolated*), and the gate stayed green. So the +disposition below is derived from the reconstructed statement text: the guard must appear in the same +SQL expression as the write it gates. That mutation now fails, which is the only reason to trust it. +""" + +from __future__ import annotations + +import ast +import pathlib +import re + +from messagefoundry.store.postgres import _EPOCH_GUARD_CLAIM, _EPOCH_GUARD_RESOLVE + +_SOURCE = pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" / "store" / "postgres.py" + +#: The write this gate is about: any statement that moves a ``queue`` row's ``status``. +_STATUS_WRITE = re.compile(r"UPDATE queue (?:q )?SET status") + +#: How a spliced guard shows up in a reconstructed SQL expression: ``{epoch_guard}`` for the f-string +#: claim sites, ``{guard}`` for the ``"..." + guard`` terminal-resolve sites. +_SPLICED = ("{epoch_guard}", "{guard}") + + +def _sql_text(node: ast.AST) -> str | None: + """Reconstruct a string-valued expression, marking interpolated/concatenated parts as ``{expr}``. + + Returns ``None`` for anything that is not a string expression, which is what lets the caller pick + *maximal* SQL expressions and skip their descendants — so one ``UPDATE`` is counted once. + """ + if isinstance(node, ast.Constant): + return node.value if isinstance(node.value, str) else None + if isinstance(node, ast.JoinedStr): + out: list[str] = [] + for value in node.values: + if isinstance(value, ast.Constant) and isinstance(value.value, str): + out.append(value.value) + elif isinstance(value, ast.FormattedValue): + out.append("{" + ast.unparse(value.value) + "}") + return "".join(out) + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): + left, right = _sql_text(node.left), _sql_text(node.right) + if left is None and right is None: + return None + return (left if left is not None else "{" + ast.unparse(node.left) + "}") + ( + right if right is not None else "{" + ast.unparse(node.right) + "}" + ) + return None + + +def _sql_expressions(fn: ast.AST) -> list[str]: + """Every maximal string expression in ``fn``, so each SQL statement is one entry.""" + found: list[str] = [] + + def visit(node: ast.AST) -> None: + text = _sql_text(node) + if text is not None: + found.append(text) + return # maximal: do not descend into the pieces of this same expression + for child in ast.iter_child_nodes(node): + visit(child) + + for child in ast.iter_child_nodes(fn): + visit(child) + return found + + +def _calls_self(fn: ast.AST, method: str) -> bool: + """True if ``fn`` contains a ``self.(...)`` call.""" + return any( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == method + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "self" + for node in ast.walk(fn) + ) + + +def _observed() -> dict[str, tuple[int, int]]: + """``{method: (status_writes, of_which_carry_a_spliced_guard)}``.""" + tree = ast.parse(_SOURCE.read_text(encoding="utf-8")) + found: dict[str, tuple[int, int]] = {} + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + writes = [sql for sql in _sql_expressions(fn) if _STATUS_WRITE.search(sql)] + if not writes: + continue + guarded = sum(1 for sql in writes if any(marker in sql for marker in _SPLICED)) + found[fn.name] = (len(writes), guarded) + return found + + +#: THE PINNED CLASSIFICATION (ADR 0157 §2.2). ``{method: (status_writes, guarded_writes)}``. +#: +#: This map IS the test. Adding an ``UPDATE queue ... SET status`` anywhere in ``postgres.py`` without a +#: row here fails, which is the point: the failure mode this guards against is a *new* terminal write +#: added later that silently escapes the fence. Every method with an unguarded write must appear in +#: :data:`_UNGUARDED_REASONS` — an unexplained hole is indistinguishable from an oversight six months on. +_EXPECTED: dict[str, tuple[int, int]] = { + # --- CLAIM paths: fail-CLOSED. A superseded ex-leader claims nothing. --- + "claim_ready": (1, 1), + # 3 writes = the guarded claim + the in-claim cross-owner reclaim + the H2 skip-and-complete. + # Only the CLAIM carries the guard: the reclaim and the H2 completion are deliberately unguarded + # (ADR 0157 §2.2 — the H2 row is non-None only because the guarded claim already matched). + "claim_next_fifo": (3, 1), + "claim_next_fifo_batch": (2, 1), + # The split-literal site (D3): the one a source-slice regex cannot see. + "claim_fifo_heads": (3, 1), + # --- Writes that return a row to PENDING: deliberately UNGUARDED (C1). --- + "release_claimed": (1, 0), + "reschedule_claimed": (1, 0), + "reset_stale_inflight": (1, 0), + "reclaim_expired_leases": (1, 0), + "recover_inflight_on_promotion": (1, 0), + # --- TERMINAL resolves: fail-OPEN guarded, spliced via the _resolve_guard helper. --- + "dead_letter_now": (1, 1), + "mark_done": (1, 1), + "mark_batch_done": (1, 1), + "complete_with_response": (1, 1), + # Both DEAD branches guarded; the success path deliberately is not (it ADMITS a message, and + # fencing it would convert a permitted duplicate into a LOST re-ingress). + "ingress_handoff": (2, 2), + # ONE statement with a conditional suffix: the guard is "" on the retry branch, which re-pends. + "mark_failed": (1, 1), + "mark_batch_failed": (1, 1), + "dead_letter_batch": (1, 1), + # --- Bring-up sweeps + operator paths: unguarded, allowlisted. --- + "dead_letter_missing_destinations": (1, 0), + "dead_letter_missing_handlers": (1, 0), + "replay": (1, 0), + "replay_dead": (1, 0), + "cancel_queued": (1, 0), +} + +#: The methods whose TERMINAL resolves are fenced. Kept separate from :data:`_EXPECTED` because the +#: guard being *spliced* and the rowcount being *inspected* are two different facts, and a resolve that +#: has the first without the second is a guard that can never fire. +_RESOLVE_FENCED = frozenset( + { + "dead_letter_now", + "mark_done", + "mark_batch_done", + "complete_with_response", + "ingress_handoff", + "mark_failed", + "mark_batch_failed", + "dead_letter_batch", + } +) + +#: Every unguarded status write needs a reason, in words, here. C1's invariant is that fencing a write +#: which returns a row to PENDING converts a *permitted duplicate* into a *forbidden strand*, so several +#: of these are unguarded on purpose and must never "grow" a guard. +_UNGUARDED_REASONS: dict[str, str] = { + "release_claimed": ( + "C1: re-pends an INFLIGHT row. Fencing it would leave the row INFLIGHT instead — a strand. It is" + " also the write _after_fenced_write itself uses to recover (D1), so guarding it would make the" + " fence's own residue unrecoverable." + ), + "reschedule_claimed": ( + "C1: same as release_claimed, with a durable backoff deadline instead of an unchanged one." + ), + "reset_stale_inflight": "Startup/DR recovery. Runs before any epoch is armed.", + "reclaim_expired_leases": ( + "Owner-BLIND periodic recovery, by its own docstring. Bounded to already-expired row leases." + ), + "recover_inflight_on_promotion": "Runs on the successor, at promotion, by definition un-fenced.", + "dead_letter_missing_destinations": ( + "ADR 0157 D11: also writes over PENDING rows, where D1's re-pend fallback is meaningless. Runs" + " only from _start_graph, and the successor re-runs the identical sweep. Flagged for the owner," + " not silently skipped." + ), + "dead_letter_missing_handlers": "ADR 0157 D11: the twin of dead_letter_missing_destinations.", + "replay": "Operator action. Guarding it would leave an operator on a standby unable to act.", + "replay_dead": ( + "Operator action, as replay: it revives a DEAD row to PENDING, which is re-pend direction, not a" + " terminal resolve." + ), + "cancel_queued": "Operator action; binds PENDING rows only, never a claimed row.", + # The three claim methods below each carry unguarded writes ALONGSIDE their guarded claim. + "claim_next_fifo": ( + "Carries 2 unguarded writes beside its guarded claim: the in-claim cross-owner lease reclaim" + " (re-pend direction, ADR 0157 Consequence 9) and the H2 skip-and-complete, which is reachable" + " only because the guarded claim UPDATE already matched." + ), + "claim_next_fifo_batch": "The in-claim cross-owner lease reclaim, as claim_next_fifo.", + "claim_fifo_heads": ( + "The in-claim cross-owner lease reclaim and the H2 skip-and-complete, as claim_next_fifo." + ), +} + + +def test_every_queue_status_write_is_classified() -> None: + """The pinned table equals reality — no site added, removed or re-scoped without this diff. + + Compared whole rather than key-by-key so a *new* unclassified write fails just as loudly as a + changed one. + """ + assert _observed() == _EXPECTED + + +def test_every_unguarded_status_write_carries_a_written_reason() -> None: + """An unexplained hole in the fence is indistinguishable from an oversight later.""" + unguarded = {name for name, (writes, guarded) in _EXPECTED.items() if guarded < writes} + assert unguarded == set(_UNGUARDED_REASONS) + assert all(len(reason) > 40 for reason in _UNGUARDED_REASONS.values()) + + +def test_every_fenced_terminal_resolve_actually_inspects_the_rowcount() -> None: + """Splicing the guard is only half of it — an unchecked guard is a guard that cannot fire. + + ``_exec_terminal`` is the only place the rowcount is read, so a resolve that renders the suffix but + calls ``conn.execute`` directly would look fenced in a diff and silently let every superseded write + land. It must also CATCH its own sentinel, or ``_FencedWrite`` escapes the store as an exception + instead of becoming a counted, re-pended no-op (ADR 0157 C3/D1). Pinned as a triple, not as three + independent facts. + """ + tree = ast.parse(_SOURCE.read_text(encoding="utf-8")) + seen: set[str] = set() + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + if fn.name not in _RESOLVE_FENCED: + continue + seen.add(fn.name) + assert _calls_self(fn, "_resolve_guard"), f"{fn.name} does not splice the resolve guard" + assert _calls_self(fn, "_exec_terminal"), f"{fn.name} never inspects the rowcount" + handlers = [ + handler + for node in ast.walk(fn) + if isinstance(node, ast.Try) + for handler in node.handlers + if isinstance(handler.type, ast.Name) and handler.type.id == "_FencedWrite" + ] + assert handlers, f"{fn.name} can raise _FencedWrite but never catches it" + assert seen == set(_RESOLVE_FENCED) + + +def test_the_claim_guard_is_referenced_only_by_the_claim_paths() -> None: + """C2's explicit ask: the two polarities are opposite, so using the wrong one is the hazard.""" + tree = ast.parse(_SOURCE.read_text(encoding="utf-8")) + referrers: dict[str, set[str]] = {"_EPOCH_GUARD_CLAIM": set(), "_EPOCH_GUARD_RESOLVE": set()} + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + for node in ast.walk(fn): + if isinstance(node, ast.Name) and node.id in referrers: + referrers[node.id].add(fn.name) + assert referrers["_EPOCH_GUARD_CLAIM"] == { + "claim_ready", + "claim_next_fifo", + "claim_next_fifo_batch", + "claim_fifo_heads", + } + # The resolve guard is spliced through exactly one helper, so every terminal resolve inherits the + # fail-open polarity from a single place and cannot drift site by site. + assert referrers["_EPOCH_GUARD_RESOLVE"] == {"_resolve_guard"} + assert referrers["_EPOCH_GUARD_CLAIM"] & referrers["_EPOCH_GUARD_RESOLVE"] == set() + + +def test_guard_polarity_is_pinned() -> None: + """The inversion IS the design (ADR 0157 §2.1), and it reads like a copy-paste slip. + + CLAIM is fail-closed: a missing lease row yields NULL, ``NULL <= $h`` is false, the claim declines + — free, because the row stays PENDING and any node may take it. RESOLVE is fail-open: COALESCE + resolves a missing row to "current" so the write LANDS, because *rejecting* a resolve strands an + INFLIGHT row. Pasting the claim guard onto a resolve site mass-strands every in-flight row the + moment the lease row vanishes. + """ + assert "COALESCE" in _EPOCH_GUARD_RESOLVE + assert "COALESCE" not in _EPOCH_GUARD_CLAIM + # Neither guard may carry a status conjunct: reclaim_expired_leases is owner-blind and can re-pend + # the CURRENT leader's own long-running row, where a status conjunct would fire and the epoch cannot. + assert "status" not in _EPOCH_GUARD_CLAIM + assert "status" not in _EPOCH_GUARD_RESOLVE + + +def test_extracting_the_claim_guard_changed_no_emitted_sql() -> None: + """The extraction to a shared constant must be a pure refactor. + + Pinned against the literal exactly as it stood before ADR 0157, at all three placeholder numberings + the claim sites use. If this drifts, shipped claim paths changed behaviour under cover of a tidy-up. + """ + assert ( + _EPOCH_GUARD_CLAIM.format(k=8, h=9) + == " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$8) <= $9" + ) + assert ( + _EPOCH_GUARD_CLAIM.format(k=9, h=10) + == " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$9) <= $10" + ) + assert ( + _EPOCH_GUARD_CLAIM.format(k=10, h=11) + == " AND (SELECT ll.leader_epoch FROM leader_lease ll WHERE ll.lease_key=$10) <= $11" + ) + + +def test_the_resolve_guard_binds_its_epoch_placeholder_twice() -> None: + """COALESCE's fallback and the comparand must be the SAME placeholder. + + ``COALESCE(ll.leader_epoch, $h) <= $h`` is what makes a missing row fail *open*. Binding two + different placeholders would type-check, run, and quietly compare an epoch against the lease key. + """ + rendered = _EPOCH_GUARD_RESOLVE.format(k=4, h=5) + assert rendered.count("$5") == 2 + assert rendered.count("$4") == 1 + assert rendered.endswith("$5") + + +def test_the_split_literal_claim_site_is_still_split() -> None: + """Guards D3's premise, not just its conclusion. + + ``claim_fifo_heads``' claim is written as two adjacent literals with a comment between. If a later + tidy-up joins them, the AST route above still works — but the *reason* this file exists stops being + demonstrable, and a reviewer would rightly ask why it is not a two-line regex. Fail loudly instead, + so the choice is re-made deliberately. + """ + source = _SOURCE.read_text(encoding="utf-8") + assert re.search(r'" UPDATE queue q"\s*#[^\n]*\n\s*" SET status=\$6', source) is not None diff --git a/tests/test_adr0157_postgres_fence.py b/tests/test_adr0157_postgres_fence.py new file mode 100644 index 0000000..7bfb28e --- /dev/null +++ b/tests/test_adr0157_postgres_fence.py @@ -0,0 +1,457 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ADR 0157 — the H1 leader-epoch fence on TERMINAL resolves (C1/C3/D1) and on ``claim_ready`` (C5). + +**Gated** on ``MEFOR_TEST_POSTGRES`` like the rest of the Postgres suite, so a laptop run skips this +file entirely. The *structural* half of ADR 0157 — which writes carry which guard — is pinned by +``tests/test_adr0157_fence_scope.py``, which is deliberately NOT gated and runs on every leg. + +Every test drives the same interleaving: claim a row while holding epoch N, bump the authoritative +``leader_lease`` to N+1 behind this handle's back (exactly what a standby's fresh acquire does), then +attempt the terminal write. The fence must reject it, roll the WHOLE disposition back, and RE-PEND the +row (D1) so the fence's own residue is a bounded duplicate rather than a strand. + +The invariant that decides every assertion here: at-least-once **permits duplication and forbids +stranding or loss**. A change turning a possible strand into a possible duplicate is an improvement; +the reverse is unacceptable however elegant. +""" + +from __future__ import annotations + +import os +from collections.abc import AsyncIterator + +import pytest + +from messagefoundry.config.models import RetryPolicy +from messagefoundry.store import MessageStatus, OutboxStatus, Stage + +pytestmark = pytest.mark.skipif( + not os.getenv("MEFOR_TEST_POSTGRES"), + reason="set MEFOR_TEST_POSTGRES=1 (+ MEFOR_STORE_* connection env) to run Postgres tests", +) + +RAW = "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|MSG1|P|2.5.1\r" + +_LEASE_KEY = "public:mefor_cluster_leader" + +#: Cleared between tests, children before parents. ``leader_lease`` is included here **on purpose**: +#: the main Postgres suite's ``_TABLES`` omits it, so a row seeded by one test survives into the next +#: and any test whose premise is "no lease row" silently stops testing that. Owning the cleanup here +#: makes each test's precondition explicit instead of inherited from file order. +_TABLES = ("message_events", "delivered_keys", "response", "queue", "messages", "leader_lease") + + +@pytest.fixture +async def store() -> AsyncIterator[object]: + from messagefoundry.config.settings import load_settings + from messagefoundry.store.postgres import PostgresStore + + settings = load_settings(environ=os.environ).store + s = await PostgresStore.open(settings) + await _reset(s) + yield s + await s.close() + + +async def _reset(store) -> None: + async with store._pool.acquire() as conn: + await conn.execute( + "CREATE TABLE IF NOT EXISTS leader_lease (" + " lease_key TEXT PRIMARY KEY, owner TEXT, lease_expires_at DOUBLE PRECISION NOT NULL," + " leader_epoch BIGINT NOT NULL DEFAULT 0)" + ) + for table in _TABLES: + await conn.execute(f"DELETE FROM {table}") + store.set_leader_epoch(None) + store.fenced_writes = 0 + + +async def _seed_epoch(store, epoch: int) -> None: + """Set the authoritative ``leader_lease.leader_epoch`` — in production the coordinator owns it.""" + async with store._pool.acquire() as conn: + await conn.execute( + "INSERT INTO leader_lease (lease_key, owner, lease_expires_at, leader_epoch)" + " VALUES ($1, 'live', 9e18, $2)" + " ON CONFLICT (lease_key) DO UPDATE SET leader_epoch = EXCLUDED.leader_epoch", + _LEASE_KEY, + epoch, + ) + + +async def _drop_lease_row(store) -> None: + async with store._pool.acquire() as conn: + await conn.execute("DELETE FROM leader_lease WHERE lease_key = $1", _LEASE_KEY) + + +async def _ledger_count(store, outbox_id: str) -> int: + async with store._pool.acquire() as conn: + return int( + await conn.fetchval("SELECT COUNT(*) FROM delivered_keys WHERE outbox_id=$1", outbox_id) + ) + + +async def _events(store, message_id: str) -> list[str]: + async with store._pool.acquire() as conn: + rows = await conn.fetch("SELECT event FROM message_events WHERE message_id=$1", message_id) + return [r["event"] for r in rows] + + +async def _claim_one(store, dest: str = "OB1", *, epoch: int = 5): + """Claim a row as the CURRENT leader holding ``epoch``.""" + await _seed_epoch(store, epoch) + store.set_leader_epoch(epoch, lease_key=_LEASE_KEY) + claimed = await store.claim_next_fifo(dest, now=200.0) + assert claimed is not None + return claimed + + +async def _superseded(store, epoch: int = 6) -> None: + """A standby took over and bumped the epoch — this handle's held token is now stale.""" + await _seed_epoch(store, epoch) + + +# --- the fence fires: terminal resolves roll back WHOLE and re-pend -------------- + + +async def test_fenced_mark_done_rolls_back_and_repends(store) -> None: + """The whole disposition goes together, and the row comes back PENDING (C3 + D1). + + Mutation that must break it: drop ``+ guard`` from mark_done's UPDATE — the row flips DONE and the + H2 ledger row appears, i.e. the superseded node records a delivery the successor will make again. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _superseded(store) + + await store.mark_done(claimed.id) # returns normally — a fenced write is NOT an exception + + outbox = (await store.outbox_for(mid))[0] + assert outbox["status"] == OutboxStatus.PENDING.value # D1: re-pended, NOT left INFLIGHT + assert outbox["attempts"] == 0 # release_claimed undoes the claim's increment + assert await _ledger_count(store, claimed.id) == 0 + assert "delivered" not in await _events(store, mid) + msg = await store.get_message(mid) + assert msg is not None and msg["status"] != MessageStatus.PROCESSED.value + assert store.fenced_writes == 1 + + +async def test_fenced_writes_land_when_the_epoch_is_current(store) -> None: + """The negative twin — the point of a fence is that it does NOT fire on the true leader. + + Mutation: flip ``<=`` to ``<`` in the resolve guard. Equality IS the true-leader case, so every + assertion here fails while the fenced tests still pass. That asymmetry is why these two exist as a + pair: either one alone can be satisfied by a guard that is simply always-on or always-off. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + # NO _superseded() — this node is still the current leader. + + await store.mark_done(claimed.id) + + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.DONE.value + assert await _ledger_count(store, claimed.id) == 1 + assert "delivered" in await _events(store, mid) + assert store.fenced_writes == 0 + + +async def test_resolve_guard_is_fail_open_on_a_missing_lease_row(store) -> None: + """The most important test in this file: the resolve guard's polarity is INVERTED vs the claim's. + + A missing ``leader_lease`` row must let a terminal resolve LAND. Reusing the claim's fail-closed + idiom here would mass-strand every in-flight row the instant that row went missing. Mutation: paste + ``_EPOCH_GUARD_CLAIM`` at the resolve site — ``NULL <= 5`` is false and this fails. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _drop_lease_row(store) # the row vanishes while we still hold epoch 5 + + await store.mark_done(claimed.id) + + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.DONE.value + assert await _ledger_count(store, claimed.id) == 1 + assert store.fenced_writes == 0 + + +async def test_lease_key_none_with_an_armed_epoch_disagrees_by_design(store) -> None: + """``set_leader_epoch(5, lease_key=None)`` is representable, and the two guards must disagree. + + ``COALESCE(NULL, 5) <= 5`` → the resolve lands; ``NULL <= 5`` → the claim declines. Pins the + asymmetry against a future tidy-up that unifies the two templates into one. + """ + await store.enqueue_message(channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0) + store.set_leader_epoch(None) + claimed = await store.claim_next_fifo("OB1", now=200.0) + assert claimed is not None + + store.set_leader_epoch(5, lease_key=None) # armed epoch, nothing to validate it against + await store.mark_done(claimed.id) + assert store.fenced_writes == 0 # the resolve LANDED + + await store.enqueue_message(channel_id="IB", raw=RAW, deliveries=[("OB2", "p")], now=100.0) + assert await store.claim_next_fifo("OB2", now=200.0) is None # the claim DECLINED + + +async def test_fenced_dead_letter_now_writes_no_false_terminal(store) -> None: + """The sharpest write of the set: a DEAD row is never re-claimed, so H2 cannot heal a false one. + + A superseded node dead-lettering a row the live leader is about to deliver loses it permanently — + strand-direction, which the at-least-once invariant forbids outright. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _superseded(store) + + await store.dead_letter_now(claimed.id, "boom") + + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.PENDING.value + assert "dead" not in await _events(store, mid) + msg = await store.get_message(mid) + assert msg is not None and msg["status"] != MessageStatus.ERROR.value + assert store.fenced_writes == 1 + + +async def test_fenced_complete_with_response_persists_no_artifact(store) -> None: + """The guard rides the FIRST statement, so the reply artifact never outlives its queue row. + + Mutation: move ``_exec_terminal`` after the response INSERT — the artifact then persists with no + resolved queue row, precisely the half-applied state C3 rejects. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _superseded(store) + + await store.complete_with_response(claimed.id, body="ACK", outcome="ok", reingress_to="IB_LOOP") + + async with store._pool.acquire() as conn: + responses = int( + await conn.fetchval("SELECT COUNT(*) FROM response WHERE message_id=$1", mid) + ) + work_rows = int( + await conn.fetchval("SELECT COUNT(*) FROM queue WHERE stage=$1", Stage.RESPONSE.value) + ) + assert responses == 0 # no artifact + assert work_rows == 0 # no re-ingress work-row + assert await _ledger_count(store, claimed.id) == 0 + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.PENDING.value + assert store.fenced_writes == 1 + + +async def test_mark_failed_dead_branch_fenced_retry_branch_lands(store) -> None: + """Both branches in ONE test, because the split between them IS the design (C1). + + The DEAD branch is terminal and fenced. The retry branch RE-PENDS, and fencing that would leave the + row INFLIGHT — converting a permitted duplicate into a forbidden strand. Mutation: make the guard + unconditional and the retry half fails. + """ + mid_a = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed_a = await _claim_one(store) + await _superseded(store) + + # Row A: retries exhausted → the DEAD branch → fenced. + assert await store.mark_failed(claimed_a.id, "boom", RetryPolicy(max_attempts=1)) is None + assert (await store.outbox_for(mid_a))[0]["status"] == OutboxStatus.PENDING.value + assert store.fenced_writes == 1 + + # Row B: retry-forever → the PENDING branch. Claim it as the CURRENT leader (epoch 6), then leave + # the epoch stale-free: the retry branch must land regardless, and must not be counted. + mid_b = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB2", "p")], now=100.0 + ) + store.set_leader_epoch(6, lease_key=_LEASE_KEY) + claimed_b = await store.claim_next_fifo("OB2", now=200.0) + assert claimed_b is not None + await _superseded(store, 7) # superseded again, mid-flight + + next_at = await store.mark_failed(claimed_b.id, "transient", RetryPolicy(max_attempts=None)) + assert isinstance(next_at, float) # rescheduled, not dead-lettered + assert (await store.outbox_for(mid_b))[0]["status"] == OutboxStatus.PENDING.value + assert store.fenced_writes == 1 # UNCHANGED — the retry branch is never inspected + + +async def test_repend_writes_land_under_a_bumped_epoch(store) -> None: + """C1's other direction, as a test, so nobody "completes" the fence by guarding these. + + ``release_claimed`` / ``reschedule_claimed`` return a row to PENDING. Fencing them would leave + INFLIGHT the one path that today hands over instantly — and would break D1's own recovery, which + is itself a ``release_claimed``. + """ + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _superseded(store) + + await store.release_claimed([claimed.id]) + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.PENDING.value + + store.set_leader_epoch(6, lease_key=_LEASE_KEY) + claimed2 = await store.claim_next_fifo("OB1", now=200.0) + assert claimed2 is not None + await _superseded(store, 7) + await store.reschedule_claimed([claimed2.id], 999.0) + row = (await store.outbox_for(mid))[0] + assert row["status"] == OutboxStatus.PENDING.value + assert row["next_attempt_at"] == 999.0 + assert store.fenced_writes == 0 # neither is a terminal resolve + + +async def test_fenced_batch_is_all_or_nothing_and_repends_every_member(store) -> None: + """A fence on ANY member rolls back ALL N, and D1 re-pends ALL N — including those never walked. + + Members past the raise were never touched by the UPDATE, but they ARE still INFLIGHT from the + claim, so a sentinel carrying only the walked prefix would strand the tail. Mutation: carry the + walked prefix instead of ``tuple(outbox_ids)`` and members 2..N stay INFLIGHT. + """ + mid = await store.enqueue_message( + channel_id="IB", + raw=RAW, + deliveries=[("OB1", "p1"), ("OB1", "p2"), ("OB1", "p3")], + now=100.0, + ) + await _seed_epoch(store, 5) + store.set_leader_epoch(5, lease_key=_LEASE_KEY) + batch = await store.claim_next_fifo_batch("OB1", now=200.0, stage=Stage.OUTBOUND.value, limit=3) + assert len(batch) == 3 + await _superseded(store) + + await store.mark_batch_done([b.id for b in batch]) + + outbox = await store.outbox_for(mid) + assert {o["status"] for o in outbox} == {OutboxStatus.PENDING.value} + for member in batch: + assert await _ledger_count(store, member.id) == 0 + assert store.fenced_writes == 1 # once per CALL, not once per member + + +# --- C5: the UNORDERED claim path ----------------------------------------------- + + +async def test_claim_ready_is_fenced(store) -> None: + """C5. Fails before ADR 0157 — ``claim_ready`` carried no epoch guard at all.""" + await _seed_epoch(store, 5) + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + + store.set_leader_epoch(3, lease_key=_LEASE_KEY) # superseded ex-leader + assert await store.claim_ready(now=200.0) == [] + row = (await store.outbox_for(mid))[0] + assert row["status"] == OutboxStatus.PENDING.value + assert row["attempts"] == 0 # a declined claim consumes no retry + assert row["owner"] is None + + store.set_leader_epoch(5, lease_key=_LEASE_KEY) # the current leader + assert len(await store.claim_ready(now=200.0)) == 1 + + +async def test_claim_ready_unfenced_with_no_lease_row_at_all(store) -> None: + """The single-node arm: epoch None means NO fence, so claim_ready behaves exactly as pre-0157. + + Its precondition is set explicitly rather than inherited from an earlier test's leftover row. + """ + await _drop_lease_row(store) + await store.enqueue_message(channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0) + store.set_leader_epoch(None) + assert len(await store.claim_ready(now=200.0)) == 1 + + +# --- parity + recovery ---------------------------------------------------------- + + +async def test_unfenced_terminal_sql_is_character_identical(store) -> None: + """The parity anchor: with no epoch armed, the emitted SQL and arg list are exactly pre-0157. + + This is what lets ADR 0157 claim single-node and SQLite behaviour is untouched — otherwise pure + assertion. Captures the real statement rather than reasoning about the code. + """ + captured: list[tuple[str, tuple[object, ...]]] = [] + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + store.set_leader_epoch(None) + claimed = await store.claim_next_fifo("OB1", now=200.0) + assert claimed is not None + + real_acquire = store._timed_acquire + + class _Spy: + def __init__(self, conn) -> None: + self._conn = conn + + def __getattr__(self, name): + return getattr(self._conn, name) + + async def execute(self, sql, *args): + captured.append((sql, args)) + return await self._conn.execute(sql, *args) + + class _Ctx: + def __init__(self, inner) -> None: + self._inner = inner + + async def __aenter__(self): + return _Spy(await self._inner.__aenter__()) + + async def __aexit__(self, *exc): + return await self._inner.__aexit__(*exc) + + store._timed_acquire = lambda *a, **k: _Ctx(real_acquire(*a, **k)) + try: + await store.mark_done(claimed.id) + finally: + store._timed_acquire = real_acquire + + updates = [(sql, args) for sql, args in captured if sql.startswith("UPDATE queue SET status")] + assert updates, "mark_done issued no queue status UPDATE" + sql, args = updates[0] + assert sql == ( + "UPDATE queue SET status=$1, last_error=NULL, updated_at=$2," + " owner=NULL, lease_expires_at=NULL WHERE id=$3" + ) + assert len(args) == 3 # no guard args appended + assert (await store.outbox_for(mid))[0]["status"] == OutboxStatus.DONE.value + + +async def test_fenced_write_is_recovered_without_a_sweep(store) -> None: + """D1's payoff, and the reason the fence's own residue is not a SQL Server strand. + + A second handle claims the fenced row IMMEDIATELY — no ``reclaim_expired_leases``, no + ``recover_inflight_on_promotion``, no waiting out a 60s row lease. Mutation: replace + ``_after_fenced_write``'s ``release_claimed`` with ``pass`` and the row stays INFLIGHT, so the + successor's claim returns None and this fails. + """ + from messagefoundry.config.settings import load_settings + from messagefoundry.store.postgres import PostgresStore + + mid = await store.enqueue_message( + channel_id="IB", raw=RAW, deliveries=[("OB1", "p")], now=100.0 + ) + claimed = await _claim_one(store) + await _superseded(store) + await store.mark_done(claimed.id) + assert store.fenced_writes == 1 + + successor = await PostgresStore.open(load_settings(environ=os.environ).store) + try: + successor.set_leader_epoch(6, lease_key=_LEASE_KEY) # the new current epoch + taken = await successor.claim_next_fifo("OB1", now=300.0) + assert taken is not None, "the fenced row was not immediately re-claimable" + await successor.mark_done(taken.id) + assert await _ledger_count(store, taken.id) == 1 # delivered EXACTLY once, by the successor + msg = await successor.get_message(mid) + assert msg is not None and msg["status"] == MessageStatus.PROCESSED.value + finally: + await successor.close() diff --git a/tests/test_alert_recipients.py b/tests/test_alert_recipients.py index e0615cf..a4b2348 100644 --- a/tests/test_alert_recipients.py +++ b/tests/test_alert_recipients.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio +import ssl from typing import Any import pytest @@ -128,7 +129,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: pass def send_message(self, msg: Any) -> None: diff --git a/tests/test_alert_sinks.py b/tests/test_alert_sinks.py index ca29775..7402570 100644 --- a/tests/test_alert_sinks.py +++ b/tests/test_alert_sinks.py @@ -5,6 +5,7 @@ from __future__ import annotations import asyncio +import ssl import time from typing import Any @@ -244,7 +245,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: sent["tls"] = True def login(self, user: str, password: str) -> None: diff --git a/tests/test_alert_templates.py b/tests/test_alert_templates.py index 56f6d15..60d7491 100644 --- a/tests/test_alert_templates.py +++ b/tests/test_alert_templates.py @@ -6,6 +6,7 @@ from __future__ import annotations +import ssl from typing import Any import pytest @@ -99,7 +100,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: pass def send_message(self, msg: Any) -> None: diff --git a/tests/test_cluster_graph_gating.py b/tests/test_cluster_graph_gating.py index cf326e7..7844c8b 100644 --- a/tests/test_cluster_graph_gating.py +++ b/tests/test_cluster_graph_gating.py @@ -124,8 +124,9 @@ async def test_reconcile_starts_on_promotion_and_stops_on_demotion(tmp_path: Pat class _EpochCoordinator(_FlipCoordinator): """A flippable clustered coordinator that ALSO reports an H1 leader epoch + lease key, so the test - can prove the engine pushes them into the store on promotion (and clears on demotion) — the - store↔coordinator wiring (the store never imports the coordinator; the engine pushes — ARCH-6).""" + can prove the engine pushes them into the store on promotion, RE-STAMPS them while leader+running + (ADR 0157 D2), and RETAINS them on demotion (C4) — the store↔coordinator wiring (the store never + imports the coordinator; the engine pushes — ARCH-6).""" def __init__(self, *, epoch: int, lease_key: str) -> None: super().__init__(leader=False) @@ -141,8 +142,13 @@ def lease_key(self) -> str | None: async def test_engine_pushes_leader_epoch_into_store_on_promotion(tmp_path: Path) -> None: # H1 store↔coordinator wiring: on promotion the engine reads the coordinator's held epoch + lease key - # and pushes them into the store (store.set_leader_epoch), BEFORE workers drain; on demotion it pushes - # None to clear the fence. The store never imports the coordinator — the engine is the one-way bridge. + # and pushes them into the store (store.set_leader_epoch), BEFORE workers drain. The store never + # imports the coordinator — the engine is the one-way bridge. + # + # ADR 0157 C4 INVERTED THE DEMOTION HALF of this test. It used to assert that demotion pushes + # (None, None) "to clear the fence". That is exactly backwards: the store OMITS the guard entirely + # when the epoch is None, so clearing on demotion DISARMS the fence at the one moment it matters — + # while a superseded ex-leader may still be mid-teardown and mid-write. The epoch is now RETAINED. cfgdir = tmp_path / "cfg" _minimal_graph(cfgdir, tmp_path) coord = _EpochCoordinator(epoch=7, lease_key="public:mefor_cluster_leader") @@ -167,8 +173,45 @@ def _spy(epoch: int | None, *, lease_key: str | None = None) -> None: assert (7, "public:mefor_cluster_leader") in pushes coord.leader = False - await eng._reconcile_graph() # demotion → push (None, ...) to clear the fence - assert pushes[-1] == (None, None) + await eng._reconcile_graph() # demotion → the held epoch is RETAINED, not cleared + # The pinned invariant, asserted as a membership test rather than on pushes[-1] alone: a + # background supervisor poll can interleave here, and D2 re-stamps on every leader+running pass, + # so the list length is nondeterministic — but a (None, None) must NEVER appear at all. + assert (None, None) not in pushes + assert pushes[-1] == (7, "public:mefor_cluster_leader") + finally: + await eng.stop() + + +async def test_reconcile_restamps_the_epoch_while_leader_and_running(tmp_path: Path) -> None: + # ADR 0157 D2. _start_graph is the only OTHER push site, so without this branch a bring-up that spans + # demote → foreign takeover → re-acquire leaves the store holding a STALE epoch with neither of the + # other two branches ever matching again — a live leader that (post-C5) claims NOTHING, silently. + # Mutation: delete the `is_leader() and running` elif in _reconcile_graph and this fails. + cfgdir = tmp_path / "cfg" + _minimal_graph(cfgdir, tmp_path) + coord = _EpochCoordinator(epoch=7, lease_key="public:mefor_cluster_leader") + eng = await Engine.create(tmp_path / "restamp.db", poll_interval=0.05, coordinator=coord) + eng.add_registry(load_config(cfgdir)) + + pushes: list[tuple[int | None, str | None]] = [] + orig = eng.store.set_leader_epoch + + def _spy(epoch: int | None, *, lease_key: str | None = None) -> None: + pushes.append((epoch, lease_key)) + orig(epoch, lease_key=lease_key) + + eng.store.set_leader_epoch = _spy # type: ignore[method-assign] + await eng.start() + try: + coord.leader = True + await eng._reconcile_graph() # promotion → (7, key) + assert eng._registry_runner is not None and eng._registry_runner.running + + # The lease was re-acquired with a bumped epoch while this node stayed leader+running. + coord._epoch = 9 + await eng._reconcile_graph() + assert pushes[-1] == (9, "public:mefor_cluster_leader") finally: await eng.stop() diff --git a/tests/test_direct_transport.py b/tests/test_direct_transport.py index 6dfab1a..a2ad782 100644 --- a/tests/test_direct_transport.py +++ b/tests/test_direct_transport.py @@ -22,6 +22,7 @@ import datetime import smtplib +import ssl from email.message import EmailMessage from pathlib import Path from typing import Any @@ -158,12 +159,18 @@ class _FakeSMTP: instances: list[_FakeSMTP] = [] def __init__( - self, host: str, port: int, timeout: float = 0.0, fail_at: str | None = None + self, + host: str, + port: int, + timeout: float = 0.0, + fail_at: str | None = None, + context: ssl.SSLContext | None = None, ) -> None: self.host = host self.port = port self.timeout = timeout self.fail_at = fail_at + self.tls_context = context # #323 — see test_email_destination._FakeSMTP self.started_tls = False self.logged_in: tuple[str, str] | None = None self.sent: list[EmailMessage] = [] @@ -179,9 +186,10 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *exc: Any) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: if self.fail_at == "starttls": raise smtplib.SMTPException("STARTTLS not supported") + self.tls_context = context self.started_tls = True def ehlo_or_helo_if_needed(self) -> None: @@ -208,8 +216,10 @@ def _install_fake( ) -> type[_FakeSMTP]: _FakeSMTP.instances = [] - def factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: - return _FakeSMTP(host, port, timeout, fail_at=fail_at) + def factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: + return _FakeSMTP(host, port, timeout, fail_at=fail_at, context=context) monkeypatch.setattr(direct_mod.smtplib, "SMTP", factory) monkeypatch.setattr(direct_mod.smtplib, "SMTP_SSL", factory) diff --git a/tests/test_email_destination.py b/tests/test_email_destination.py index 45e1f9e..75bd4a5 100644 --- a/tests/test_email_destination.py +++ b/tests/test_email_destination.py @@ -7,6 +7,7 @@ from __future__ import annotations import smtplib +import ssl from email.message import EmailMessage from typing import Any @@ -33,12 +34,22 @@ class _FakeSMTP: instances: list[_FakeSMTP] = [] def __init__( - self, host: str, port: int, timeout: float = 0.0, fail_at: str | None = None + self, + host: str, + port: int, + timeout: float = 0.0, + fail_at: str | None = None, + context: ssl.SSLContext | None = None, ) -> None: self.host = host self.port = port self.timeout = timeout self.fail_at = fail_at + # #323: the context the connector handed us — on 465 via SMTP_SSL(context=), on 587 via + # starttls(context=). Recorded (not ignored) so a test can assert it actually verifies: + # smtplib's own default is ssl._create_unverified_context, so "a context was passed" is the + # whole point and a fake that silently swallowed it would hide a regression. + self.tls_context = context self.started_tls = False self.logged_in: tuple[str, str] | None = None self.sent: list[EmailMessage] = [] @@ -54,9 +65,10 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *exc: Any) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: if self.fail_at == "starttls": raise smtplib.SMTPException("STARTTLS not supported") + self.tls_context = context self.started_tls = True def ehlo_or_helo_if_needed(self) -> None: @@ -83,8 +95,10 @@ def _install_fake( ) -> type[_FakeSMTP]: _FakeSMTP.instances = [] - def factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: - return _FakeSMTP(host, port, timeout, fail_at=fail_at) + def factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: + return _FakeSMTP(host, port, timeout, fail_at=fail_at, context=context) monkeypatch.setattr(email_mod.smtplib, "SMTP", factory) monkeypatch.setattr(email_mod.smtplib, "SMTP_SSL", factory) @@ -158,13 +172,17 @@ async def test_port_465_uses_implicit_tls_not_starttls(monkeypatch: pytest.Monke # On 465 the whole session is wrapped (SMTP_SSL), so no explicit STARTTLS is issued. captured: dict[str, str] = {} - def ssl_factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: + def ssl_factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: captured["which"] = "SMTP_SSL" - return _FakeSMTP(host, port, timeout) + return _FakeSMTP(host, port, timeout, context=context) - def plain_factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: + def plain_factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: captured["which"] = "SMTP" - return _FakeSMTP(host, port, timeout) + return _FakeSMTP(host, port, timeout, context=context) _FakeSMTP.instances = [] monkeypatch.setattr(email_mod.smtplib, "SMTP_SSL", ssl_factory) @@ -450,3 +468,131 @@ def test_email_and_smtp_factories_exported() -> None: assert spec.settings["host"] == "smtp.partner.org" assert spec.settings["port"] == 587 # STARTTLS submission default assert "Email" in mf.__all__ and "SMTP" in mf.__all__ + + +# --- #323: the TLS hop actually VERIFIES ------------------------------------------------------------ +# +# These are the regression fence for #323. Before it, both arms called smtplib with NO context, and +# smtplib's fallback is ssl._create_stdlib_context — which IS ssl._create_unverified_context +# (CERT_NONE / check_hostname=False). So `use_tls=True` bought encryption without authentication and +# every certificate was accepted. Asserting "STARTTLS was issued" (the pre-existing tests) could never +# catch that: it was issued, over an unverified session. These assert the CONTEXT, which is the only +# thing that distinguishes the fixed state from the broken one. Run them against the pre-fix connector +# and they go red on `tls_context is None`. + + +async def test_starttls_gets_a_verifying_context(monkeypatch: pytest.MonkeyPatch) -> None: + _install_fake(monkeypatch) + await EmailDestination(_dest()).send("body") + [smtp] = _FakeSMTP.instances + assert smtp.started_tls is True + ctx = smtp.tls_context + assert ctx is not None, "starttls() was called with no context — smtplib would not verify" + assert ctx.verify_mode is ssl.CERT_REQUIRED + assert ctx.check_hostname is True + assert ctx.minimum_version is ssl.TLSVersion.TLSv1_2 + + +async def test_implicit_tls_465_gets_a_verifying_context(monkeypatch: pytest.MonkeyPatch) -> None: + # The 465 arm builds SMTP_SSL, a different smtplib entry point with its own unverified default. + _install_fake(monkeypatch) + await EmailDestination(_dest(port=465)).send("body") + [smtp] = _FakeSMTP.instances + assert smtp.started_tls is False # implicit TLS — no explicit STARTTLS + ctx = smtp.tls_context + assert ctx is not None, "SMTP_SSL was constructed with no context — it would not verify" + assert ctx.verify_mode is ssl.CERT_REQUIRED + assert ctx.check_hostname is True + + +def test_tls_verify_false_refused_without_escape(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv(INSECURE_TLS_ESCAPE_ENV, raising=False) + with pytest.raises(ValueError, match="tls_verify=false"): + EmailDestination(_dest(tls_verify=False)) + + +def test_tls_verify_false_refuses_credentials_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # An unverified session is as bad as cleartext for an AUTH exchange: an on-path attacker + # presenting any certificate captures it. Mirrors the use_tls=false credential refusal. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(ValueError, match="UNVERIFIED"): + EmailDestination(_dest(tls_verify=False, username="svc", password="pw")) + + +async def test_tls_verify_false_with_escape_builds_an_unverified_context( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + _install_fake(monkeypatch) + await EmailDestination(_dest(tls_verify=False)).send("body") + [smtp] = _FakeSMTP.instances + ctx = smtp.tls_context + assert ctx is not None # still a context — just a deliberately non-verifying one + assert ctx.verify_mode is ssl.CERT_NONE + assert ctx.check_hostname is False + + +def test_tls_verify_false_refused_on_enforcing_phi_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The clamp (#200, ADR 0092 decision 2): the blunt process-wide escape must NOT silence a + # verify-off PHI hop on an enforcing instance. This is why the arm reads + # weakened_tls_escape_permitted_here() and not the raw insecure_tls_allowed(). + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with ( + active_hop_posture(HopPosture(enforcing=True, is_phi=True)), + pytest.raises(ValueError, match="tls_verify=false"), + ): + EmailDestination(_dest(tls_verify=False)) + + +async def test_tls_ca_file_pins_that_ca_only( + monkeypatch: pytest.MonkeyPatch, tmp_path: Any +) -> None: + # A per-connection CA wins verbatim, and pins to ONLY that CA — no system bundle (ADR 0093 + # precedence #1). This is the supported route for a private-CA relay, and the reason + # tls_verify=false should almost never be needed. + import datetime + + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import rsa + from cryptography.x509.oid import NameOID + + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Test Relay CA")]) + now = datetime.datetime.now(datetime.UTC) + cert = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(days=1)) + .not_valid_after(now + datetime.timedelta(days=3650)) + .add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + .sign(key, hashes.SHA256()) + ) + ca = tmp_path / "relay-ca.pem" + ca.write_bytes(cert.public_bytes(serialization.Encoding.PEM)) + + _install_fake(monkeypatch) + await EmailDestination(_dest(tls_ca_file=str(ca))).send("body") + [smtp] = _FakeSMTP.instances + ctx = smtp.tls_context + assert ctx is not None + assert ctx.verify_mode is ssl.CERT_REQUIRED + loaded = ctx.get_ca_certs() + assert len(loaded) == 1, ( + "a per-connection CA must pin to ONLY that CA, not augment system roots" + ) + assert dict(x[0] for x in loaded[0]["subject"])["commonName"] == "Test Relay CA" + + +def test_tls_ca_file_that_does_not_exist_fails_loudly(tmp_path: Any) -> None: + # Silently falling back to system roots on an unreadable CA would be the worst outcome: the + # operator believes they pinned, and did not. + with pytest.raises((FileNotFoundError, ssl.SSLError, OSError)): + EmailDestination(_dest(tls_ca_file=str(tmp_path / "nope.pem"))) diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index 014639a..c55864d 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -4,6 +4,7 @@ from __future__ import annotations +import contextlib import inspect import ssl import types @@ -21,6 +22,7 @@ InsecureHopRefused, _is_forward_secret, active_hop_posture, + build_smtp_tls_context, current_hop_posture, enforce_insecure_hop, fips_attestation, @@ -611,3 +613,138 @@ def test_the_call_site_scan_examined_real_files() -> None: assert sites >= 5, ( f"expected the known TLS context sites, found {sites} — the scan is not landing" ) + + +# --- #323: build_smtp_tls_context REFUSES an untrusted peer (behavioural, not attribute) ------------ +# +# The connector tests assert this context's ATTRIBUTES (verify_mode, check_hostname). That is not the +# same claim as "it refuses a bad certificate", and an attribute check cannot establish it — the whole +# defect being fixed was a context whose attributes were never inspected by anyone. These drive a REAL +# TLS handshake against a locally-minted self-signed server so the refusal is OBSERVED. Run against the +# pre-#323 code path (smtplib's default context) every REFUSED case below returns HANDSHAKE OK, which +# is precisely the bug. +# +# No network: binds 127.0.0.1 on an ephemeral port, and the server thread only completes a handshake +# and closes — it speaks no SMTP, because the property under test is the TLS layer, not the protocol. + + +@pytest.fixture(scope="module") +def _tls_peer(tmp_path_factory: pytest.TempPathFactory) -> tuple[int, str]: + """A local TLS server with a self-signed 'localhost' cert. Yields (port, ca_pem_path).""" + import datetime + import socket + import threading + + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import rsa + from cryptography.x509.oid import NameOID + + tmp = tmp_path_factory.mktemp("smtp_tls") + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "localhost")]) + now = datetime.datetime.now(datetime.UTC) + cert = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(days=1)) + .not_valid_after(now + datetime.timedelta(days=365)) + .add_extension(x509.SubjectAlternativeName([x509.DNSName("localhost")]), critical=False) + .sign(key, hashes.SHA256()) + ) + cert_p, key_p = tmp / "server.pem", tmp / "server.key" + cert_p.write_bytes(cert.public_bytes(serialization.Encoding.PEM)) + key_p.write_bytes( + key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + ) + + srv = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + srv.load_cert_chain(cert_p, key_p) + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen(8) + + def _accept_loop() -> None: + while True: + try: + conn, _ = listener.accept() + except OSError: + return # listener closed at teardown + threading.Thread(target=_handshake, args=(conn,), daemon=True).start() + + def _handshake(conn: socket.socket) -> None: + # A client that (correctly) refuses us aborts mid-handshake, so OSError here is the EXPECTED + # path for the refusal tests, not an error -- suppress on both legs. + with contextlib.suppress(OSError): + srv.wrap_socket(conn, server_side=True).close() + with contextlib.suppress(OSError): + conn.close() + + threading.Thread(target=_accept_loop, daemon=True).start() + yield listener.getsockname()[1], str(cert_p) + listener.close() + + +def _handshake_result(ctx: ssl.SSLContext, port: int, server_hostname: str) -> str: + import socket + + try: + with ( + socket.create_connection(("127.0.0.1", port), timeout=10) as sock, + ctx.wrap_socket(sock, server_hostname=server_hostname), + ): + return "ok" + except ssl.SSLCertVerificationError: + return "refused" + except OSError: # a reset mid-refusal still means the client did not accept the peer + return "refused" + + +def test_smtp_context_refuses_an_untrusted_peer(_tls_peer: tuple[int, str]) -> None: + """THE regression fence for #323. Pre-fix this returned 'ok' — any certificate was accepted.""" + port, _ = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination") + assert _handshake_result(ctx, port, "localhost") == "refused" + + +def test_smtp_context_accepts_a_peer_signed_by_the_connection_ca( + _tls_peer: tuple[int, str], +) -> None: + """tls_ca_file is the supported route for a private-CA relay — it must actually work, or the + only remaining escape would be turning verification off.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", ca_file=ca) + assert _handshake_result(ctx, port, "localhost") == "ok" + + +def test_smtp_context_enforces_hostname(_tls_peer: tuple[int, str]) -> None: + """A trusted chain is not enough: the cert must match the host we meant to reach.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", ca_file=ca) + assert _handshake_result(ctx, port, "wrong.example") == "refused" + + +def test_smtp_context_check_hostname_false_relaxes_only_the_name( + _tls_peer: tuple[int, str], +) -> None: + """tls_check_hostname=False drops the name check while KEEPING chain validation.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context( + host="localhost", cell="Email destination", ca_file=ca, check_hostname=False + ) + assert _handshake_result(ctx, port, "wrong.example") == "ok" + + +def test_smtp_context_verify_false_accepts_anything(_tls_peer: tuple[int, str]) -> None: + """The escape genuinely disables verification — asserted so nobody 'hardens' it into a + silently-broken state where the escape no longer connects and operators cannot tell why.""" + port, _ = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", verify=False) + assert _handshake_result(ctx, port, "localhost") == "ok" diff --git a/tests/test_wiring_engine.py b/tests/test_wiring_engine.py index f4ddc57..d4d28c3 100644 --- a/tests/test_wiring_engine.py +++ b/tests/test_wiring_engine.py @@ -735,10 +735,10 @@ async def aclose(self) -> None: async def test_delivery_requeues_when_leadership_lost_before_send( store: MessageStore, tmp_path: Path ) -> None: - # L1: a delivery worker that has LOST leadership since claiming the row must re-queue it (mark_failed - # → PENDING with backoff), never emit egress as a stale ex-leader, and never drop it. The cheap - # synchronous is_leader() gate fires before connector.send(); the row stays deliverable for the new - # leader (count-and-log), and the connector is never called. + # L1: a delivery worker that has LOST leadership since claiming the row must RELEASE the claim — + # back to PENDING with the claim's attempts++ undone, next_attempt_at untouched and NO last_error — + # never emit egress as a stale ex-leader, and never drop it. Losing leadership is not a delivery + # failure, so it must not spend a retry (see the finite-cap test below for why that matters). inbox, outdir = tmp_path / "in", tmp_path / "out" inbox.mkdir() reg = _retry_registry( @@ -754,28 +754,40 @@ async def test_delivery_requeues_when_leadership_lost_before_send( channel_id="file_in", raw=ADT, deliveries=[("file_out", "OUT|body")], control_id="C1" ) coord.leader = False + # POSITIVE SIGNAL. A seeded row is already PENDING/attempts=0, so asserting that state proves + # nothing — it is indistinguishable from the worker never running. Spy on release_claimed so the + # test waits for the guard to actually FIRE. (An earlier revision of this test asserted the row + # state alone and passed against the pre-fix code; the signal is the whole point.) + released: list[list[str]] = [] + real_release = store.release_claimed + + async def _spy_release(ids: list[str], now: float | None = None) -> None: + released.append(list(ids)) + await real_release(ids, now) + + store.release_claimed = _spy_release # type: ignore[method-assign] await runner.start() try: - # Poll for the OBSERVABLE re-queue: the row carries the leadership-lost last_error and is back to - # PENDING — a positive signal, not "nothing happened" (we don't assert absence by waiting). elapsed = 0.0 - while True: - cur = await store._db.execute( - "SELECT status, last_error FROM queue WHERE message_id=?", (mid,) - ) - row = await cur.fetchone() - if ( - row is not None - and row["status"] == OutboxStatus.PENDING.value - and "leadership lost" in (row["last_error"] or "") - ): - break + while not released: await asyncio.sleep(0.02) elapsed += 0.02 - assert elapsed < 3.0, ( - "row was not re-queued with the leadership-lost error within timeout" - ) + assert elapsed < 3.0, "the L1 guard never released the claim (release_claimed uncalled)" + cur = await store._db.execute( + "SELECT status, attempts, last_error FROM queue WHERE message_id=?", (mid,) + ) + row = await cur.fetchone() + assert row is not None + assert row["status"] == OutboxStatus.PENDING.value + assert row["attempts"] == 0, ( + f"the claim's attempts++ must be undone (got {row['attempts']})" + ) + assert not row["last_error"], ( + f"a release must not write last_error (got {row['last_error']!r}) — " + "a leadership change is not a delivery failure" + ) finally: + store.release_claimed = real_release # type: ignore[method-assign] await runner.stop() assert dest.calls == 0 # no egress emitted as a stale ex-leader # The message was NOT dead-lettered or delivered — it stays in the pipeline for the new leader. @@ -783,6 +795,68 @@ async def test_delivery_requeues_when_leadership_lost_before_send( assert (await store.stats()).get(OutboxStatus.DONE.value) is None +async def test_leadership_lost_before_send_does_not_dead_letter_under_a_finite_retry_cap( + store: MessageStore, tmp_path: Path +) -> None: + # ADR 0157, the strand this fix exists for. With a FINITE RetryPolicy.max_attempts, routing the L1 + # bail through mark_failed re-read the claim's own attempts++ and took the DEAD branch — writing a + # terminal dead-letter on a row that was NEVER SENT. The new leader never sees a DEAD row, so the + # message is stranded: not delivered, not deliverable, and recoverable only by an operator replay. + # At-least-once permits duplication; it forbids stranding. Releasing the claim cannot dead-letter. + # + # Non-vacuous: revert the body to `_mark_failed_and_arm(...)` and this fails on the DEAD assertion — + # attempts is already at the cap when the guard fires, which is exactly the shipped shape. + inbox, outdir = tmp_path / "in", tmp_path / "out" + inbox.mkdir() + reg = _retry_registry(inbox, outdir, RetryPolicy(backoff_seconds=30.0, max_attempts=1)) + coord = _FlipCoordinator(leader=True) + runner = RegistryRunner(reg, store, poll_interval=0.02, coordinator=coord) + dest = _NeverSends() + runner._destinations["file_out"] = dest + mid = await store.enqueue_message( + channel_id="file_in", raw=ADT, deliveries=[("file_out", "OUT|body")], control_id="C1" + ) + coord.leader = False + # Same positive signal as above: wait for the guard to fire, then assert the outcome. Waiting on + # row state alone would pass trivially, because the seeded row is already PENDING/attempts=0. + released: list[list[str]] = [] + real_release = store.release_claimed + + async def _spy_release(ids: list[str], now: float | None = None) -> None: + released.append(list(ids)) + await real_release(ids, now) + + store.release_claimed = _spy_release # type: ignore[method-assign] + await runner.start() + try: + elapsed = 0.0 + while not released: + await asyncio.sleep(0.02) + elapsed += 0.02 + assert elapsed < 3.0, ( + "the L1 guard never released the claim — under the pre-fix mark_failed body this " + "row is dead-lettered instead, un-sent" + ) + cur = await store._db.execute( + "SELECT status, attempts FROM queue WHERE message_id=?", (mid,) + ) + row = await cur.fetchone() + assert row is not None + assert row["status"] == OutboxStatus.PENDING.value, ( + f"expected PENDING, got {row['status']!r} — DEAD here is the strand this test exists for" + ) + assert row["attempts"] == 0, ( + f"the claim's attempts++ must be undone (got {row['attempts']}) — otherwise a " + "leadership change spends a retry the delivery never used" + ) + finally: + store.release_claimed = real_release # type: ignore[method-assign] + await runner.stop() + assert dest.calls == 0 + # The decisive assertion: nothing dead-lettered, so the row survives for the new leader. + assert (await store.stats()).get(OutboxStatus.DEAD.value) is None + + async def test_delivery_proceeds_while_leader(store: MessageStore, tmp_path: Path) -> None: # The complement: while this node IS leader, the L1 gate is a no-op and delivery proceeds normally — # so the guard never blocks the happy path. (Single-node NullCoordinator is always leader → identical.)