diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d794d57e..ff1df1a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -481,7 +481,7 @@ jobs: # are declared there (ADR 0013), and SQL Server / Postgres both declare supports_response_capture # + supports_pt_reingress True — so a regression in that surface is a SERVER-DB regression and must # pull these legs, not just the SQLite suite. - if echo "$changed" | grep -qE '^(messagefoundry/store/|messagefoundry/pipeline/(cluster|wiring_runner|stage_dispatcher)|messagefoundry/config/(settings|wiring)|messagefoundry/transports/(base|database|dicomweb|fhir|http_auth|mllp|rest|soap|tcp|x12)|tests/test_(sqlserver|postgres|cluster|database_connector|database_source|pooled|stage_dispatcher|batch_claim|claim_fifo|inline_fast_path|seq_only_fifo|fifo_index|per_lane_wake|response_capture|reingress|x12_rte|shard_recovery|shard_cert|adr0071|adr0075|adr0114|dr_server_seed_gate|dr7_server_config_only_backup|backup_runner_server_db|connscale|load_failover|load_runner)|\.github/workflows/ci\.yml)'; then + if echo "$changed" | grep -qE '^(messagefoundry/store/|messagefoundry/pipeline/(cluster|wiring_runner|stage_dispatcher)|messagefoundry/config/(settings|wiring)|messagefoundry/transports/(base|database|dicomweb|fhir|http_auth|mllp|rest|soap|tcp|x12)|tests/test_(sqlserver|postgres|cluster|database_connector|database_source|pooled|stage_dispatcher|batch_claim|claim_fifo|inline_fast_path|seq_only_fifo|fifo_index|per_lane_wake|response_capture|reingress|x12_rte|shard_recovery|shard_cert|adr0071|adr0075|adr0114|adr0157|dr_server_seed_gate|dr7_server_config_only_backup|backup_runner_server_db|connscale|load_failover|load_runner)|\.github/workflows/ci\.yml)'; then echo "serverdb=true" >> "$GITHUB_OUTPUT" else echo "serverdb=false" >> "$GITHUB_OUTPUT" @@ -889,7 +889,7 @@ jobs: # dev/test escape is set. MEFOR_STORE_ENCRYPT: "false" MEFOR_ALLOW_INSECURE_TLS: "1" - run: pytest tests/test_postgres_store.py -v + run: pytest tests/test_postgres_store.py tests/test_adr0157_postgres_fence.py -v - name: Run the failover-LOAD test (two nodes, SIGKILL the primary mid-load, against the real DB) env: diff --git a/HANDOFF-ha-recheck.md b/HANDOFF-ha-recheck.md new file mode 100644 index 00000000..b441ebba --- /dev/null +++ b/HANDOFF-ha-recheck.md @@ -0,0 +1,48 @@ +# HANDOFF — HA re-check / ADR 0157 + +Updated 2026-08-02. Supersedes the stop-work handoff written earlier the same day. + +**The authoritative status is [ADR 0157](docs/adr/0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md) itself** — it carries the decisions, the corrections and the per-increment build state. This file holds only what does not belong in an ADR: what is left, and the traps that cost time. + +## 1. State + +| | | +|---|---| +| ADR 0157 | **Accepted.** Increments **1, 4, 5 built**; 0, 2, 3 not built | +| PR | [#139](https://github.com/MEFORORG/MessageFoundry/pull/139) | +| Earlier | #129 merged (`884036f8`) — the doc corrections | + +## 2. What is left + +**Inc 0 — make the margin real.** The demotion budget does not subtract the lease-renew round trip, which is bounded only by `[store].command_timeout` (30.0) — *equal* to the stock lease TTL. Until that is clamped, the derived 4.5s budget is nominal, not guaranteed. The floor clamp means a bad margin degrades to *overlap* (duplicates, permitted) rather than hard-cancel-always (INFLIGHT residue, strand-direction on SQL Server), so this is a real gap and not an emergency. + +**Inc 2 — ⛔ MIS-SPECIFIED IN THE ADR. Do not build it as written.** It proposes an owner-blind, age-based periodic sweep. SQL Server has **no populated `owner` column** to discriminate with, so that sweep would re-pend rows a live leader is actively working. The verified defect is narrower — there is no recovery at **graph re-start**, because `RegistryRunner.reload()` is a quiesce-and-swap that calls no recovery — and the fix is a scoped reset there. Re-scope first. + +**Inc 3 — SQL Server fences.** Blocked on Inc 2. Its own 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`). + +> **A green Postgres run does not license the same edit on `sqlserver.py`.** The two backends' recovery guarantees differ, and that difference is the whole argument for D1. + +**Also deferred (D10):** the own-owner promotion-recovery statement. It must be lease-expiry-bounded to be safe, and every claim stamps `lease_until = now + lease_ttl_seconds`, so it is a no-op exactly when it would be needed. + +## 3. Traps that cost real time + +- **Local `pytest` SILENTLY SKIPS the Postgres and SQL Server legs.** A green laptop run proves nothing about either backend. There is a `mefor-pg-ci` container (postgres:16) on **port 5433** matching CI's fixtures — use it: + ``` + MEFOR_TEST_POSTGRES=1 MEFOR_STORE_BACKEND=postgres MEFOR_STORE_SERVER=localhost \ + MEFOR_STORE_PORT=5433 MEFOR_STORE_DATABASE=messagefoundry MEFOR_STORE_USERNAME=postgres \ + MEFOR_STORE_PASSWORD=mefor MEFOR_STORE_ENCRYPT=false MEFOR_ALLOW_INSECURE_TLS=1 + ``` +- **A new module-gated suite executes NOWHERE until it is named in a workflow.** `tests/test_serverdb_ci_coverage.py` catches this; it caught this branch. Add the file to the `sqlserver-store` / `postgres-store` job **and** to the `serverdb` change-detection alternation in `ci.yml`. +- **`leader_lease` is not in `tests/test_postgres_store.py`'s `_TABLES`**, so a row seeded by one test survives into the next. Any test whose premise is "no lease row" must delete it explicitly — `test_epoch_guard_disabled_when_none_is_byte_identical`'s own comment is a false premise for exactly this reason. +- **`mypy` reports 21 phantom errors** unless the venv has the full extras: `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 the worktree. Activate `.venv` or prepend it. **Never `--no-verify`.** +- **A full local suite with the Postgres env set produces failures that isolation does not** — the `[postgres]`-parametrized suites share one database and contaminate each other. Before attributing any failure, re-run the same set the same way with your diff reverted. Measuring a different quantity is not a measurement. + +## 4. The methodological point worth keeping + +Two gates written for this work were **blind to the exact class they existed to catch**, and only a deliberate mutation found it: + +- The structural fence gate keyed on whether a method *mentioned* `_EPOCH_GUARD_CLAIM`. Deleting `{epoch_guard}` from `claim_fifo_heads`' SQL — the precise regression — left the mention intact and the gate stayed **green**. It now keys on the emitted SQL. +- An earlier session's first version of the `c20295c0` tests asserted a row was `PENDING` with `attempts=0`, which passes against the *pre-fix* code because a seeded row is already in that state. + +Both were caught by attacking the control, not by reading it. Build the gate, then try to slip the defect past it. 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 00000000..bb8c3d5c --- /dev/null +++ b/docs/adr/0157-demotion-safety-fence-scope-on-post-claim-writes-and-a-bounded-graph-stop.md @@ -0,0 +1,510 @@ +# ADR 0157 — Demotion safety: fence scope on post-claim writes, and a bounded graph stop + +**Status:** Accepted **Date:** 2026-08-01 **Implemented:** 2026-08-02 (Inc 1, 4, 5) + +> **Increments 1, 4 and 5 are BUILT.** C1 (terminal writes only, fail-open) and C6 (yes, bounded, +> abandon-don't-await) were decided by the owner. Inc 0, 2 and 3 are **not** built — and Inc 2's +> premise is wrong as written; see the Increments section. +> +> Three clauses were corrected during implementation, each because the drafted version was +> **strand-direction** — the one outcome the at-least-once invariant forbids: +> +> - **C3** said 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 produced +> by the fence itself. Corrected: roll back, then re-pend via unguarded `release_claimed` (D1). +> - **C4** said "delete the `set_leader_epoch(None)` clear". Alone that is a silent total halt once C5 +> lands: `_reconcile_graph` had only two branches, so `is_leader() and running` matched neither, +> forever, holding a stale epoch — a live leader claiming nothing, with no exception and no alert. +> Corrected: delete the clear **and** re-stamp on every reconcile pass while leader (D2). +> - **The cross-backend safety claim for C4 was false.** It read "SQL Server's claim guard already +> exists ... so a demoted SS node claims nothing". `claim_ready` on SQL Server carries **no** epoch +> guard, so a demoted SS node retaining a stale epoch still claims and drains every UNORDERED lane, +> and resolves those rows unfenced. Retaining the epoch is still strictly better than clearing it — +> but only the three FIFO claim paths are covered there until Inc 3. + +--- + +## 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. BUILT.** `_EPOCH_GUARD_CLAIM` +onto `claim_ready`; `_EPOCH_GUARD_RESOLVE` onto the eight terminal resolves (`dead_letter_now`, +`mark_done`, `mark_batch_done`, `complete_with_response`, `ingress_handoff`'s two DEAD branches, +`mark_failed`, `mark_batch_failed`, `dead_letter_batch`). A rejected resolve rolls back **whole** and +re-pends (D1). `set_leader_epoch(None)` removed from `_stop_graph` (C4), with the D2 re-stamp. No DDL. + +**Dropped from Inc 1 (D10):** the additional own-owner promotion-recovery statement. It must be +lease-expiry-bounded to be safe, and every claim stamps `lease_until = now + lease_ttl_seconds`, so in +the promote -> demote -> re-promote-in-one-process case it is a no-op exactly when it would be needed; +once the lease *has* expired the owner-blind sweep already covers it. Deferred, not silently skipped. + +*Tests, as built:* 13 runtime tests against a real Postgres + 8 **structural** tests enumerating every +`UPDATE queue ... SET status` site with a written reason for each unguarded one. The structural gate +parses the AST rather than slicing source, because `claim_fifo_heads` splits its claim across two +adjacent literals with a comment between them and a line-oriented regex cannot see it. + +> **A gate is evidence only once it has been shown to fail.** The first version of that structural test +> keyed on whether a method *mentioned* `_EPOCH_GUARD_CLAIM`. Deleting `{epoch_guard}` from +> `claim_fifo_heads`' SQL — the exact regression it exists to catch — left the mention intact and the +> gate stayed **green**. It now keys on the emitted SQL, and the mutation is confirmed red. + +**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 2 is MIS-SPECIFIED above; do not build it as written.** It proposes an owner-blind, age-based +> periodic sweep. The verified defect is narrower — there is no recovery at **graph re-start**, because +> `RegistryRunner.reload()` is a quiesce-and-swap that calls no recovery — and the right fix is a scoped +> reset there. An age sweep on SQL Server has **no populated `owner` column** to discriminate with, so it +> would re-pend rows a live leader is actively working. Re-scope before building. + +**Inc 4 — `TeardownReason` + bounded, concurrent source stop (DEMOTE only). BUILT.** 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.items())` +before the first await. The bound is applied **at the call site** — never by editing transport constants, +which would make `transports/` know about clustering and violate the one-way dependency rule (§4). + +**Corrected from the draft (D6): one phase-level `asyncio.wait`, NOT a semaphore with a per-source +`wait_for`.** The semaphore form costs `ceil(N/C) x budget` — ~63s at the 1,500-connection target +against an ~8s margin. `asyncio.wait` also never cancels its awaitables, so "abandon, don't cancel" is a +property of the primitive rather than of an `asyncio.shield` token a later edit can silently drop. + +**Abandonment is socket-safe, and only socket-safe.** The four `asyncio.start_server` sources +(MLLP/TCP/X12/HTTP) call `server.close()` in their synchronous prologue. **DICOM does not** — it releases +its port inside `await to_thread(server.shutdown)`, so an abandoned DICOM stop can still hold the port at +re-promotion. File/RemoteFile/Database/Timer only set an Event, but each is leader-gated and parks on that +Event, so an abandoned one finishes at most its single in-flight scan. State this per connector; do not +write "every source". +*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, as built:* 21 non-env-gated tests. The `_running` regression (a raised or cancelled teardown +still leaves the node re-promotable, via a `finally` containing no `await`); abandon-not-cancel; +max-not-sum at **N = 200**, because a semaphore of 8 or 64 is structurally incapable of showing the +defect at small N; both demotion edges on both coordinators; and a parity sentinel proving single-node +reaches no DEMOTE-only statement. + +**The `cluster_sqlserver.py` compile-time Protocol guard cannot backstop the hook.** It asserts only +assignability to `ClusterCoordinator`, which deliberately does not carry `set_on_demote` (widening that +`@runtime_checkable` Protocol breaks a standalone test stand-in — S3). Omitting the SQL Server twin would +type-check clean. The tests are the only real backstop, so they pin both coordinators explicitly. + +--- + +## 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 3ae66028..ea0e755d 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 | **Accepted (2026-08-01)** — C1 (terminal writes only, fail-open) and C6 (bounded, abandon-don't-await) owner-decided; **increments 1, 4 and 5 built 2026-08-02**. Inc 1 fences `claim_ready` plus the eight terminal resolves on Postgres, a rejected resolve rolling the whole disposition back — queue flip, ledger row, event row and finalize together — and then **re-pending** the row. **Three drafted clauses were corrected during the build, each because the draft was strand-direction or false.** C3 said leave the fenced row INFLIGHT for recovery: on SQL Server there is no periodic in-flight recovery at all, so that is an unbounded strand *manufactured by the fence itself* — the one outcome the ADR forbids. C4 alone was a silent total halt: `_reconcile_graph` had only two branches, so `is_leader() and running` matched neither and a live leader held a stale epoch, claiming **nothing**, with no exception and no alert — closed by re-stamping the epoch every reconcile pass. And the cross-backend claim that a demoted SQL Server node "claims nothing" was **wrong**: `claim_ready` is unguarded there, so retaining the epoch covers only the three FIFO claim paths until Inc 3. Evidence is mutation-verified in both directions, which is how the *first* structural gate was caught being blind to its own subject — it keyed on whether a method **mentioned** the guard constant, so deleting the guard from `claim_fifo_heads`' emitted SQL left the mention intact and the gate stayed **green**. **Inc 0/2/3 not built, and Inc 2 is MIS-SPECIFIED in this ADR** — its owner-blind, age-based sweep has no populated `owner` column on SQL Server to discriminate with and would re-pend rows a live leader is working; the real defect is the absence of recovery at graph re-start. Do not build it as written | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index 7c78d63c..4a6033e3 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -4160,6 +4160,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 de3c3ca0..704d88f7 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 b8a3e123..815c4931 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/pipeline/cluster.py b/messagefoundry/pipeline/cluster.py index 4bbb456a..e21a13ea 100644 --- a/messagefoundry/pipeline/cluster.py +++ b/messagefoundry/pipeline/cluster.py @@ -133,6 +133,38 @@ class ClusterMember: promotable: bool = True +_DEMOTE_BUDGET_FRACTION = 0.5 +_DEMOTE_BUDGET_FLOOR = 1.0 +_DEMOTE_BUDGET_CEILING = 10.0 + + +def fence_tick_seconds(fence_timeout_seconds: float) -> float: + """The self-fence watchdog tick. Shared by both coordinators AND by the demotion budget below, so + the budget and the watchdog it is derived from cannot drift apart.""" + return max(0.05, min(1.0, fence_timeout_seconds / 5.0)) + + +def demote_stop_budget( + *, lease_ttl_seconds: float, fence_timeout_seconds: float +) -> tuple[float, float]: + """``(budget, raw_headroom)`` for a bounded demotion teardown (ADR 0157 C6). + + A STATIC duration cap on two phases — NOT a lease-anchored absolute deadline: it reads no DB clock, + compares against no ``lease_expires_at``, makes the Windows monotonic clock load-bearing for + nothing, and cannot degrade to zero. + + The renew round trip is NOT subtracted, because it is not observable here. It is bounded only by + ``[store].command_timeout`` (30.0 by default) — which equals the stock lease TTL — so until that is + clamped separately the real margin can be zero and this budget is nominal rather than guaranteed. + Say that plainly rather than implying the budget is met. + """ + headroom = lease_ttl_seconds - fence_timeout_seconds - fence_tick_seconds(fence_timeout_seconds) + return ( + max(_DEMOTE_BUDGET_FLOOR, min(_DEMOTE_BUDGET_CEILING, _DEMOTE_BUDGET_FRACTION * headroom)), + headroom, + ) + + @runtime_checkable class ClusterCoordinator(Protocol): """The coordination contract every backend (null today, DB-backed later) implements. @@ -167,14 +199,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: @@ -397,6 +442,11 @@ def __init__( # heartbeat; a standby may acquire only once the lease has expired (per the DB clock), so it # always waits out the full TTL before taking over. self._lease_ttl = leader_lease_ttl_seconds + # ADR 0157 Inc 5: optional demotion edge-trigger. None unless the engine registered + # one; deliberately NOT part of the ClusterCoordinator Protocol (that Protocol is + # @runtime_checkable and a standalone test stand-in isinstance-checks against it, so + # widening it is a multi-site enumeration — the exact defect class this avoids). + self._on_demote: Callable[[], None] | None = None # The SELF-FENCE timeout: a leader that has not renewed within this many seconds (its own # monotonic clock, no DB I/O) demotes itself. MUST be < the TTL — but note what that ordering # does and does not buy. It bounds when this node stops *calling itself* leader; it does NOT @@ -872,6 +922,7 @@ async def _maintain_leadership(self) -> None: self._leader_epoch = None log.info("cluster: node %s lost leadership (lease taken or expired)", self.node_id) self._alert_leadership_lost("lease taken or expired") # #145 (inverse → auto-resolves) + self._fire_on_demote() # ADR 0157 Inc 5 async def _claim_or_renew_lease(self) -> bool: """Atomically acquire OR renew the leadership lease and return whether this node now holds it. @@ -978,6 +1029,7 @@ def _check_fence(self) -> None: self._lease_ttl, ) self._alert_leadership_lost("self-fenced") # #145 (inverse → auto-resolves) + self._fire_on_demote() # ADR 0157 Inc 5 async def _release_leadership(self) -> None: """Best-effort clean release: demote the cached gate first (so a concurrent is_leader() reader @@ -1019,6 +1071,26 @@ def _alert_leadership_acquired(self) -> None: except Exception: # pragma: no cover - defensive; a sink must never break leadership log.warning("cluster: leadership_acquired alert failed", exc_info=True) + def set_on_demote(self, callback: Callable[[], None] | None) -> None: + """Register an edge-trigger fired the instant this node stops being leader (ADR 0157 Inc 5). + + The engine uses it to start the graph teardown immediately instead of waiting up to a whole + ``_graph_reconcile_interval`` poll. The callback MUST be synchronous, never-raise, and PURE + IN-MEMORY: it fires from ``_check_fence``, which runs during a DB partition precisely because + renews are failing, so touching the store or the pool there would hang the fence itself. + """ + self._on_demote = callback + + def _fire_on_demote(self) -> None: + """Never-raise, never touch the pool. Both properties are load-bearing — see set_on_demote.""" + callback = self._on_demote + if callback is None: + return + try: + callback() + except Exception: # pragma: no cover - defensive; a hook must never break demotion + log.warning("cluster: on_demote hook failed", exc_info=True) + def _alert_leadership_lost(self, reason: str) -> None: """Emit a ``leadership_lost`` inverse (this node is no longer leader) — auto-resolves the open ``leadership_acquired`` instance. Never-raise (as :meth:`_alert_leadership_acquired`).""" diff --git a/messagefoundry/pipeline/cluster_sqlserver.py b/messagefoundry/pipeline/cluster_sqlserver.py index 6c3cdecc..18b67fc5 100644 --- a/messagefoundry/pipeline/cluster_sqlserver.py +++ b/messagefoundry/pipeline/cluster_sqlserver.py @@ -109,6 +109,11 @@ def __init__( self._heartbeat_seconds = heartbeat_seconds self._node_timeout_seconds = node_timeout_seconds self._lease_ttl = leader_lease_ttl_seconds + # ADR 0157 Inc 5: optional demotion edge-trigger. None unless the engine registered + # one; deliberately NOT part of the ClusterCoordinator Protocol (that Protocol is + # @runtime_checkable and a standalone test stand-in isinstance-checks against it, so + # widening it is a multi-site enumeration — the exact defect class this avoids). + self._on_demote: Callable[[], None] | None = None self._fence_timeout = leader_fence_timeout_seconds # Small relative to the fence timeout so a fence fires promptly (well before the lease TTL). self._fence_tick = max(0.05, min(1.0, leader_fence_timeout_seconds / 5.0)) @@ -431,6 +436,7 @@ async def _maintain_leadership(self) -> None: self._leader_epoch = None # no longer a fenced leader (H1) log.info("cluster: node %s lost leadership (lease taken or expired)", self.node_id) self._alert_leadership_lost("lease taken or expired") # #145 (inverse → auto-resolves) + self._fire_on_demote() # ADR 0157 Inc 5 async def _claim_or_renew_lease(self) -> bool: """Atomically acquire (fresh / expired) or renew (already ours) the single leadership lease, all @@ -510,6 +516,7 @@ def _check_fence(self) -> None: self._lease_ttl, ) self._alert_leadership_lost("self-fenced") # #145 (inverse → auto-resolves) + self._fire_on_demote() # ADR 0157 Inc 5 async def _release_leadership(self) -> None: was_leader = self._is_leader @@ -542,6 +549,26 @@ def _alert_leadership_acquired(self) -> None: except Exception: # pragma: no cover - defensive; a sink must never break leadership log.warning("cluster: leadership_acquired alert failed", exc_info=True) + def set_on_demote(self, callback: Callable[[], None] | None) -> None: + """Register an edge-trigger fired the instant this node stops being leader (ADR 0157 Inc 5). + + The engine uses it to start the graph teardown immediately instead of waiting up to a whole + ``_graph_reconcile_interval`` poll. The callback MUST be synchronous, never-raise, and PURE + IN-MEMORY: it fires from ``_check_fence``, which runs during a DB partition precisely because + renews are failing, so touching the store or the pool there would hang the fence itself. + """ + self._on_demote = callback + + def _fire_on_demote(self) -> None: + """Never-raise, never touch the pool. Both properties are load-bearing — see set_on_demote.""" + callback = self._on_demote + if callback is None: + return + try: + callback() + except Exception: # pragma: no cover - defensive; a hook must never break demotion + log.warning("cluster: on_demote hook failed", exc_info=True) + def _alert_leadership_lost(self, reason: str) -> None: try: self._alert_sink.leadership_lost(self.node_id, role="follower", reason=reason) diff --git a/messagefoundry/pipeline/engine.py b/messagefoundry/pipeline/engine.py index 628151ea..93731b2c 100644 --- a/messagefoundry/pipeline/engine.py +++ b/messagefoundry/pipeline/engine.py @@ -15,7 +15,7 @@ from collections.abc import Callable, Mapping, Sequence from dataclasses import replace from pathlib import Path -from typing import Any +from typing import Any, Protocol, runtime_checkable from messagefoundry.config.models import ( AckAfter, @@ -52,7 +52,11 @@ ) from messagefoundry.pipeline.alerts import AlertSink from messagefoundry.pipeline.cert_expiry import CertExpiryRunner, MonitoredCert, certs_from_registry -from messagefoundry.pipeline.cluster import ClusterCoordinator, NullCoordinator +from messagefoundry.pipeline.cluster import ( + ClusterCoordinator, + NullCoordinator, + demote_stop_budget, +) from messagefoundry.pipeline.config_convergence import ConfigConvergenceRunner from messagefoundry.pipeline.dr import DrCoordinator from messagefoundry.pipeline.dr_backup import BackupRunner @@ -72,6 +76,7 @@ from messagefoundry.pipeline.update_check import UpdateCheckResult, UpdateCheckRunner from messagefoundry.pipeline.wiring_runner import ( RegistryRunner, + TeardownReason, check_pt_backend_supported, check_reference_backend_supported, ) @@ -104,6 +109,24 @@ def _within(path: Path, root: Path) -> bool: return path == root or root in path.parents +@runtime_checkable +class _SupportsDemoteHook(Protocol): + """The OPTIONAL demotion edge-trigger capability (ADR 0157 Inc 5). + + Deliberately NOT part of :class:`ClusterCoordinator`: that Protocol is ``@runtime_checkable`` and a + standalone test stand-in isinstance-checks against it, so widening it is a multi-site enumeration — + the exact defect class this avoids. A coordinator without the hook simply keeps the poll-only path, + which is correct, just up to one ``_graph_reconcile_interval`` slower. + + Note what the compile-time Protocol guard in ``cluster_sqlserver.py`` can and cannot do: it asserts + that coordinator is assignable to ``ClusterCoordinator``, which does NOT include this hook — so + omitting the SQL Server twin would still type-check. The only real backstops are the tests that pin + both shipped coordinators against this Protocol and fire both edges. + """ + + def set_on_demote(self, callback: Callable[[], None] | None) -> None: ... + + class Engine: def __init__( self, @@ -383,6 +406,9 @@ def __init__( # processing; the poll interval is bounded (at start()) to keep that stop prompt. self._graph_supervisor: asyncio.Task[None] | None = None self._graph_stop = asyncio.Event() + # ADR 0157 Inc 5: set by the coordinator's demotion edge so the supervisor reconciles at + # once instead of waiting out a whole poll interval. _graph_stop stays the loop-exit latch. + self._graph_wake = asyncio.Event() self._graph_lock = asyncio.Lock() self._graph_reconcile_interval = 1.0 # Serializes ALL console→connections.toml writes (#131/#136 review): the read-modify-write of the @@ -1127,6 +1153,19 @@ async def start(self) -> None: # Stay comfortably inside the (ttl - fence) margin and never slower than ~1s. self._graph_reconcile_interval = max(0.1, min(1.0, (ttl - fence) / 3.0)) self._graph_stop.clear() + self._graph_wake.clear() + # ADR 0157 Inc 5: register the demotion edge-trigger if this coordinator supports it. + # An isinstance gate rather than hasattr, and a test pins that BOTH shipped coordinators + # satisfy the Protocol — otherwise the capability degrades to the poll path SILENTLY, + # which is the defect class the ADR itself criticises elsewhere. + if isinstance(self._coordinator, _SupportsDemoteHook): + self._coordinator.set_on_demote(self._on_demote_edge) + else: + log.info( + "engine: coordinator has no demotion edge-trigger; graph teardown follows the " + "%.2fs poll instead", + self._graph_reconcile_interval, + ) # Reconcile ONCE synchronously before the loop: if this node is already the leader (it # acquired the lease on coordinator.start()'s first tick, or in tests a stand-in reports # leader immediately), the graph comes up during start() rather than a poll-interval later. @@ -1239,16 +1278,53 @@ async def _fire_pool_warm(self) -> None: await asyncio.gather(self._warm_pool_task, return_exceptions=True) self._warm_pool_task = asyncio.create_task(self.store.warm_pool(), name="store-pool-warm") + def _on_demote_edge(self) -> None: + """Coordinator demotion edge (ADR 0157 Inc 5). Synchronous, never-raise, PURE IN-MEMORY. + + It deliberately does NOT set the runner's ``_stop``: that would halt every dispatcher while + ``_running`` is still True, and a promote -> demote -> re-promote flap would then leave + ``_reconcile_graph``'s bring-up branch facing a graph whose workers had already exited. Halting + egress at the fence edge is already the pre-send bail's job. + """ + try: + self._graph_wake.set() + except Exception: # pragma: no cover - defensive; must never break the coordinator's fence + log.warning("engine: demotion edge wake failed", exc_info=True) + async def _stop_graph(self) -> None: """Tear the graph down on loss of leadership: stop the listeners + workers so a demoted node stops binding/processing. The reference-sync loop and the self-gated maintenance/convergence 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 C6: demotion is BOUNDED — the source and dispatcher phases only. Phases after + # them (connector aclose, executor shutdown, sandbox close) remain unbounded; they run + # after the graph has stopped so they cannot extend the split-brain window, but do NOT + # describe this as 'the demotion teardown is bounded'. + budget, _headroom = demote_stop_budget( + lease_ttl_seconds=self._cluster_settings.leader_lease_ttl_seconds, + fence_timeout_seconds=self._cluster_settings.leader_fence_timeout_seconds, + ) + await self._registry_runner.stop(reason=TeardownReason.DEMOTE, budget_seconds=budget) + # 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 +1341,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: @@ -1275,6 +1368,9 @@ async def _graph_supervisor_loop(self) -> None: leases independently prevent concurrent double-processing of a given row). Clustered only; cooperatively stopped via ``_graph_stop`` (the loop wakes on it and exits between reconciles).""" while not self._graph_stop.is_set(): + # Consume the edge BEFORE reconciling, not after: a fence landing DURING a slow reconcile + # must wake the NEXT pass rather than being swallowed by a clear() that runs after it. + self._graph_wake.clear() try: await self._reconcile_graph() except asyncio.CancelledError: @@ -1282,8 +1378,10 @@ async def _graph_supervisor_loop(self) -> None: except Exception: log.exception("engine graph supervisor reconcile failed; will retry") try: # noqa: SIM105 + # Wake on the demotion edge OR the poll interval. Engine.stop() sets both latches, so a + # shutdown exits here cooperatively instead of being cancelled by its own timeout. await asyncio.wait_for( - self._graph_stop.wait(), timeout=self._graph_reconcile_interval + self._graph_wake.wait(), timeout=self._graph_reconcile_interval ) except TimeoutError: pass @@ -1721,6 +1819,10 @@ async def stop(self) -> None: # graph itself is then stopped by the registry_runner.stop() below, as before. if self._graph_supervisor is not None: self._graph_stop.set() + # Paired with _graph_stop: the loop now waits on _graph_wake, so without this the + # supervisor would sit out its poll interval and be CANCELLED by the wait_for below + # rather than exiting cooperatively. + self._graph_wake.set() supervisor = self._graph_supervisor self._graph_supervisor = None try: @@ -1766,6 +1868,10 @@ async def stop(self) -> None: # Deregister cluster membership after the runner has quiesced but before the store closes (the # coordinator marks its node left over the same pool). stop() is idempotent and safe even if # start() raised (then there's just nothing to cancel). NullCoordinator is a no-op. + # ADR 0157 Inc 5: drop the edge-trigger BEFORE stopping the coordinator, so a clean + # step-down cannot fire a wake into a supervisor that is already gone. + if isinstance(self._coordinator, _SupportsDemoteHook): + self._coordinator.set_on_demote(None) await self._coordinator.stop() # Cancel the background pool warm-up if still running (best-effort; the store is about to close). # It releases any connections it holds in its own finally, but at shutdown the pool closes anyway. diff --git a/messagefoundry/pipeline/stage_dispatcher.py b/messagefoundry/pipeline/stage_dispatcher.py index 683c2d0e..1f2e563b 100644 --- a/messagefoundry/pipeline/stage_dispatcher.py +++ b/messagefoundry/pipeline/stage_dispatcher.py @@ -485,6 +485,69 @@ async def start(self) -> None: self._sweep_task.add_done_callback(functools.partial(self._on_task_done, "sweep")) await self._run_sweep_once() # immediate first sweep (arms due lanes / not-due timers) + async def quiesce(self, budget: float) -> bool: + """COOPERATIVE stop (ADR 0157 Inc 5): stop claiming, let every live serializer reach its + terminal transition, CANCEL NOTHING. Returns True iff ``_lane_tasks`` ended empty. + + NEVER raises on timeout — the caller's ``self._running = False`` must stay reachable. The caller + ALWAYS follows with :meth:`stop`, which is both the state-clearing path and the hard-cancel + fallback, so there is exactly one dispatcher teardown to keep correct rather than two. + + **THE ORDERING THAT MATTERS (ADR 0157 D8): the serializer drain is NOT gated on the claimer and + sweep loops exiting.** The demotion fence fires precisely BECAUSE renews failed — i.e. the pool + is degraded — i.e. the claimer is parked inside ``claim_fifo_heads`` against + ``[store].command_timeout`` (30s by default). A design that waited for the loops first would + time out before draining a single serializer, on this increment's dominant trigger. So the loops + get a SUB-budget and we drain regardless; a claimer that returns mid-drain still spawns its + serializer (correct — those rows ARE claimed) and the re-snapshot loop picks it up. + + The contract is "zero rows INFLIGHT", not "loops exited" — ``stop()`` cancels the loops anyway. + + NOT safe to call standalone: ``_stop`` is the RUNNER's shared Event, so quiescing one dispatcher + signals every dispatcher and every per_lane worker in the process. + """ + if not self._running: + return True + loop = asyncio.get_running_loop() + deadline = loop.time() + max(0.0, budget) + self._stop.set() + self._sweep_now.set() + for claimer in self._claimers: + claimer.event.set() + loops = [c.task for c in self._claimers if c.task is not None] + if self._sweep_task is not None: + loops.append(self._sweep_task) + live_loops = [task for task in loops if not task.done()] + if live_loops: # sub-budget only; a timeout here is NOT fatal + await asyncio.wait(live_loops, timeout=max(0.0, (deadline - loop.time()) * 0.25)) + watched: set[asyncio.Task[None]] = set() + while True: + # RE-SNAPSHOT every pass: _run_lane pops itself out of _lane_tasks on the normal path, and a + # claimer that was mid-round-trip may have dispatched one final serializer after we looked. + live = [task for task in self._lane_tasks.values() if not task.done()] + watched.update(live) + if not live: + self._reap(watched) + return True + remaining = deadline - loop.time() + if remaining <= 0: + self._reap(watched) + return False + await asyncio.wait(live, timeout=remaining) # never cancels, never raises + + @staticmethod + def _reap(tasks: set[asyncio.Task[None]]) -> None: + """Retrieve every drained serializer's exception. + + ``_on_lane_task_done`` returns early once ``_stop`` is set, and ``stop()``'s + ``gather(return_exceptions=True)`` cannot cover a serializer that already popped itself out of + ``_lane_tasks`` — so without this a raised serializer logs "exception was never retrieved" at + GC, hiding a real failover-time crash behind a garbage-collection message. + """ + for task in tasks: + if task.done() and not task.cancelled(): + task.exception() + async def stop(self) -> None: """Tear down: signal stop, wake every claimer + the sweep, cancel all tasks + timers, gather, then clear state POST-gather (never mid-run). Idempotent (mirrors ``_teardown_unsafe``). A diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 1c96ec4b..c734375a 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -35,7 +35,7 @@ from contextlib import ExitStack from dataclasses import dataclass from datetime import UTC, datetime -from enum import Enum +from enum import Enum, StrEnum from pathlib import Path from typing import Any, Protocol, cast @@ -444,6 +444,34 @@ class _StreamBudgetExceeded(Exception): _PREFIX_STAGES = frozenset({Stage.INGRESS, Stage.ROUTED}) +class TeardownReason(StrEnum): + """Why :meth:`RegistryRunner._teardown_unsafe` is running (ADR 0157 C6). + + SHUTDOWN — the historical single path (process stop, a failed start's unwind). Statement-for- + statement unchanged, and the ONLY value single-node SQLite can reach: the only DEMOTE caller is + ``Engine._stop_graph``, reachable only from ``_reconcile_graph``, which runs only under + ``is_clustered()`` — and ``[cluster].enabled`` is rejected on SQLite at config load. + DEMOTE — loss of leadership, racing a lease this node no longer holds. Dispatchers drain + cooperatively BEFORE any hard cancel, and both phases are bounded. An inbound that cannot stop + inside the budget is ABANDONED — not awaited, and not cancelled.""" + + SHUTDOWN = "shutdown" + DEMOTE = "demote" + + +#: Share of the demotion budget given to the cooperative quiesce phase. Larger than the source +#: share because quiesce overrun is the only one that can STRAND: a hard-cancelled serializer +#: leaves rows INFLIGHT, whereas an abandoned listener is duplicate-direction. +_DEMOTE_QUIESCE_SHARE = 0.7 +#: Used only if a caller passes no budget. Equals the stock 10/20/30 derivation; unreachable single-node. +_DEMOTE_BUDGET_FALLBACK_SECONDS = 4.5 +#: How long start() waits for a PRIOR demotion's abandoned stops before cancelling them. Sized to +#: 2x the client-shutdown grace because MLLP/TCP/X12/HTTP each consume that grace TWICE, serially, +#: inside one stop() — a 1x bound would cancel in precisely the slow-but-healthy case it exists to +#: settle, taking the ADR 0031 rebind-failure branch instead. +_PENDING_STOP_SETTLE_SECONDS = 10.0 + + def _peek_for_loopback( ic: InboundConnection, body: str ) -> tuple[str | None, str | None, str | None, bool]: @@ -850,6 +878,10 @@ def __init__( # against this map when a connector is built (a missing key fails loud — see resolve_env_settings). self._env_values: dict[str, Any] = dict(env_values or {}) self._sources: dict[str, SourceConnector] = {} + # ADR 0157 Inc 4: inbound stop() tasks a DEMOTE abandoned when they overran the budget. Always + # EMPTY unless a demotion abandoned one, which is why every consumer sits behind a falsy-list + # guard and single-node never touches this list. + self._pending_source_stops: list[asyncio.Task[None]] = [] self._destinations: dict[str, DestinationConnector] = {} # One delivery worker per outbound connection, addressable by name so a reload can # gracefully stop/swap a single connection's worker without touching its siblings. @@ -1091,6 +1123,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, @@ -2369,6 +2414,15 @@ async def start(self) -> None: async with self._reload_lock: if self._running: return + if self._sources or self._workers or self._destinations or self._dispatchers: + # ADR 0157 D7: a prior teardown raised or was cancelled from outside, so _running is + # False (the finally) but the built state survives. Building on top of it would leave + # orphaned listeners bound — and _start_inbound_unsafe's `if name in self._sources: + # return` would silently skip EVERY rebind — plus two dispatchers per stage claiming one + # lane, the per-lane FIFO hazard the single-consumer invariant exists to close. + # Idempotent, and structurally unreachable after a clean stop, so single-node never + # enters it. + await self._teardown_unsafe(TeardownReason.SHUTDOWN) self._stop.clear() # Capture the engine loop so a handler's worker thread can bridge a db_lookup back onto it. self._loop = asyncio.get_running_loop() @@ -2464,7 +2518,9 @@ async def start(self) -> None: # is isolated above) must not leave half the graph wired with _running still False: # unwind everything we started so the listeners are released and a retry can rebind (M-8). log.exception("wiring start failed; unwinding the partial start") - await self._teardown_unsafe() + # Explicitly SHUTDOWN (ADR 0157): a partial start is not a demotion — there is no lease + # being handed over, so there is nothing to bound and no reason to abandon a listener. + await self._teardown_unsafe(TeardownReason.SHUTDOWN) raise self._running = True # #147 (ADR 0095): spawn one active-window scheduler task per SCHEDULED connection. Each @@ -2540,17 +2596,214 @@ def _sandbox_for(self, name: str) -> SandboxSession | None: self._sandbox_sessions[name] = session return session - async def stop(self) -> None: + async def stop( + self, + *, + reason: TeardownReason = TeardownReason.SHUTDOWN, + budget_seconds: float | None = None, + ) -> None: + """Stop the graph. ``reason`` is keyword-only with a SHUTDOWN default, so all ~170 existing + ``runner.stop()`` call sites across the engine, the tests and the harness are unchanged and + behave identically (ADR 0157 C6).""" async with self._reload_lock: # serialize against an in-flight reload (no torn-down state) had_state = self._running or bool(self._sources or self._workers or self._destinations) - await self._teardown_unsafe() + await self._teardown_unsafe(reason, budget_seconds=budget_seconds) + if reason is TeardownReason.SHUTDOWN and self._pending_source_stops: + # Never orphan a task at loop close. Under DEMOTE we deliberately leave them running — + # settling them is start()'s job, at the next promotion. + await self._settle_pending_source_stops() if had_state: - log.info("wiring stopped") + log.info("wiring stopped") # UNCHANGED string on the SHUTDOWN path + + async def _stop_sources_demote(self, budget: float) -> None: + """DEMOTE-only bounded, CONCURRENT source stop (ADR 0157 Inc 4 / D6). NEVER raises. + + ONE phase-level deadline over ALL tasks — not a per-source timeout under a semaphore, which + would cost ``ceil(N/C) x budget`` (~63s at the 1,500-connection target against an ~8s margin). + + ``asyncio.wait`` is the primitive, NOT ``wait_for``: it never cancels its awaitables, so + "abandon, do not cancel" is a property of the call itself rather than of one ``asyncio.shield`` + token a later edit can silently drop. - async def _teardown_unsafe(self) -> None: + Tasks are created eagerly, outside any gate. The four ``asyncio.start_server`` sources + (MLLP/TCP/X12/HTTP) call ``server.close()`` in their SYNCHRONOUS prologue, so accept stops on + the first loop pass after task creation — before this ``wait``'s timeout can fire, even at + budget 0.0 — and the expensive part is only the client drain. Note the precise claim: + ``create_task`` merely SCHEDULES, so nothing runs until we suspend on the ``wait`` below; do not + insert anything between them. + + Abandonment is safe for those four. It is NOT safe for DICOM, which releases its port inside + ``await to_thread(server.shutdown)`` — an abandoned DICOM stop can still hold the port at + re-promotion. File/RemoteFile/Database/Timer only set an Event, but each is leader-gated and + parks on that Event, so an abandoned one finishes at most its single in-flight scan. + + The bound is applied HERE, at the call site — never by editing a transport constant, which would + make ``transports/`` know about clustering (the one-way dependency rule). + """ + # A PREVIOUS demotion's stops have had a whole leadership term; cancel them rather than + # accumulating a generation per flap. Generation-scoped, so no arbitrary count cap is needed. + for stale in self._pending_source_stops: + stale.cancel() + self._pending_source_stops = [] + sources = list(self._sources.items()) # snapshot BEFORE the first await + if not sources: + return + tasks = [asyncio.create_task(src.stop(), name=f"demote-stop:{n}") for n, src in sources] + _done, still = await asyncio.wait(tasks, timeout=max(0.0, budget)) + for task in _done: + if not task.cancelled() and task.exception() is not None: + log.warning("demotion: an inbound stop() failed: %s", task.exception()) + if still: + self._pending_source_stops = list(still) + for task in still: + task.add_done_callback(self._reap_pending_stop) + log.warning( + "demotion: %d inbound listener(s) did not stop within %.2fs and were ABANDONED " + "(their listening sockets are already closed; the client drain finishes in the " + "background and is settled at the next promotion). The node is NOT quiescent — a " + "message mid-handler still finishes its commit and its ACK, which count-and-log " + "requires; the successor drains the body.", + len(still), + budget, + ) + + async def _quiesce_workers_demote(self, budget: float) -> None: + """per_lane parity for the pooled quiesce (ADR 0157 Inc 5). NEVER raises. + + These loops are ``while not self._stop.is_set()`` and ``_stop`` was set at the top of teardown, + so they exit on their own once the CURRENT claimed prefix resolves. ``asyncio.wait`` never + cancels and never raises on timeout; the existing cancel + gather below remains the fallback. + + No-op in pooled mode: all FOUR dicts are empty there, because ``_ensure_inbound_workers`` + returns early under ``pooled`` and ``_spawn_worker`` is a documented pooled no-op. + + MUST run ABOVE the source phase, or per_lane workers keep issuing post-demotion terminal writes + for the whole source phase. + """ + live = [ + task + for task in ( + *self._workers.values(), + *self._router_workers.values(), + *self._transform_workers.values(), + *self._response_workers.values(), + ) + if not task.done() + ] + if live: + await asyncio.wait(live, timeout=max(0.0, budget)) + + async def _quiesce_dispatchers_demote(self, budget: float) -> None: + """DEMOTE-only cooperative dispatcher stop (ADR 0157 Inc 5). NEVER raises. No-op in per_lane. + + ``d.stop()`` cancels the lane serializers, and a cancelled serializer leaves its claimed prefix + INFLIGHT BY DESIGN. On Postgres that is latency; on SQL Server there is no periodic in-flight + recovery at all. So on the one path we KNOW is handing over, drain first: a serializer allowed + to reach its terminal transition leaves ZERO rows INFLIGHT — its tail is ``release_claimed``'d, + its faulting head ``reschedule_claimed``'d, and a claimed OUTBOUND head that has not sent hits + the pre-send bail and re-pends un-errored. + + ``stop()`` still runs unconditionally afterwards: it is BOTH the state-clearing path AND the + hard-cancel fallback, so there is exactly ONE dispatcher teardown to keep correct, not two. + """ + if not self._dispatchers: + return + dispatchers = list(self._dispatchers.values()) # held across an await + drained = await asyncio.gather( + *(d.quiesce(budget) for d in dispatchers), return_exceptions=True + ) + if any(result is not True for result in drained): + log.warning( + "demotion: %d of %d stage dispatcher(s) did not drain within %.2fs — hard " + "cancelling. Their claimed rows stay INFLIGHT: bounded on Postgres by " + "reclaim_expired_leases, but on SQL Server recovered ONLY by the successor's " + "on-promotion reset_stale_inflight — so if no node takes over, they strand.", + sum(1 for result in drained if result is not True), + len(dispatchers), + budget, + ) + await asyncio.gather(*(d.stop() for d in dispatchers), return_exceptions=True) + self._dispatchers.clear() + + def _reap_pending_stop(self, task: asyncio.Task[None]) -> None: + """Retrieve an abandoned stop's exception, else asyncio logs 'never retrieved' at GC.""" + if task in self._pending_source_stops: + self._pending_source_stops.remove(task) + if not task.cancelled() and task.exception() is not None: + log.warning( + "demotion: an abandoned inbound stop() ended with an error: %s", task.exception() + ) + + async def _settle_pending_source_stops(self) -> None: + """Join a prior demotion's abandoned stops, BOUNDED — and the bound is mandatory. + + This runs inside ``_reload_lock``, which ``reload()``, ``stop()``, the per-connection + start/stop/restart and every ``/connections`` handler also take. An unbounded join would + therefore wedge re-promotion, engine shutdown and the connection API for as long as a wedged + File/Database ``stop()`` runs (those gather with no cancel and no timeout). + + On timeout we cancel and proceed: a failed rebind is isolated per connection (ADR 0031, + operator-recoverable), whereas refusing to re-promote strands the whole graph — and + strand-direction is forbidden. + """ + pending = [task for task in self._pending_source_stops if not task.done()] + self._pending_source_stops = [] + if not pending: + return + _done, still = await asyncio.wait(pending, timeout=_PENDING_STOP_SETTLE_SECONDS) + for task in still: + task.cancel() + if still: + await asyncio.gather(*still, return_exceptions=True) + log.warning( + "%d abandoned inbound stop(s) did not finish within %.1fs and were cancelled; a " + "rebind of those ports may fail (isolated per connection, ADR 0031)", + len(still), + _PENDING_STOP_SETTLE_SECONDS, + ) + + async def _teardown_unsafe( + self, + reason: TeardownReason = TeardownReason.SHUTDOWN, + *, + budget_seconds: float | None = None, + ) -> None: """Tear down all sources/workers/destinations and mark stopped. Lock-free (callers hold _reload_lock) and idempotent — cleans up whatever is registered even if the runner never - reached _running, so a half-started runner (review M-8) and a double stop() are both safe.""" + reached _running, so a half-started runner (review M-8) and a double stop() are both safe. + + ``reason`` (ADR 0157 C6) selects the SOURCE + DISPATCHER phases only; every other phase, and + their order, is shared. Under SHUTDOWN the executed statements are today's, verbatim. + + **THE INVARIANT: ``self._running = False`` must execute on every path.** + ``Engine._reconcile_graph``'s bring-up branch is ``is_leader() and not running``, so a teardown + that returns or raises without clearing it makes this node **un-re-promotable, silently, with no + exception**. Two mechanisms enforce it: every DEMOTE bound is absorbed at its own call site (a + timeout CONTINUES the sequence rather than unwinding it — unwinding would skip the worker + cancel, the destination aclose and every ``.clear()``), and the ``finally`` below. + + The ``finally`` is a BACKSTOP, **not** a licence to wrap this coroutine in ``wait_for`` from + OUTSIDE: a cancelled teardown still leaves ``_sources`` populated, which is why ``start()`` + re-runs teardown on residual state and ``_reconcile_graph`` carries a ``has_residual_state`` + branch (ADR 0157 D7). + """ + demote = reason is TeardownReason.DEMOTE + budget = ( + max( + 0.0, + budget_seconds if budget_seconds is not None else _DEMOTE_BUDGET_FALLBACK_SECONDS, + ) + if demote + else 0.0 + ) + try: + await self._teardown_body(demote, budget) + finally: + self._running = False + + async def _teardown_body(self, demote: bool, budget: float) -> None: + """The teardown sequence itself. Split out ONLY so the ``finally`` above contains no await — + an external cancel therefore cannot interrupt the one statement that must always run.""" self._stop.set() # #147 (ADR 0095): cancel the active-window scheduler tasks FIRST so no schedule tick calls # start/stop_inbound/outbound while the rest of teardown runs (a task blocked awaiting the reload @@ -2567,19 +2820,32 @@ async def _teardown_unsafe(self) -> None: # here would notify_work() dispatchers we are about to stop. if self._claim_mode != "pooled": self._wake_all(Stage.INGRESS, Stage.ROUTED, Stage.RESPONSE, Stage.OUTBOUND) - for source in self._sources.values(): - await source.stop() - # ADR 0066 D3 ordering: stop the pooled dispatchers AFTER the sources are stopped — so a - # listener can no longer mark_ready an already-cleared dispatcher — NOT right after _stop.set(). - # The shared _stop already broke their loops; d.stop() cancels each claimer/sweep/lane task + - # timer and clears its state, then we drop the dict. A cancelled serializer leaves its claimed - # rows INFLIGHT for reset_stale_inflight (crash-safety) — never released. Empty in per_lane mode, - # so this is a no-op there and the per_lane worker cancel/gather below is unchanged. - if self._dispatchers: - await asyncio.gather( - *(d.stop() for d in self._dispatchers.values()), return_exceptions=True - ) - self._dispatchers.clear() + if demote: + # ADR 0157 Inc 5: DEMOTE INVERTS the ADR 0066 D3 order below, deliberately. Egress is the + # split-brain-relevant action, so its budget must start NOW rather than after the source + # phase. A listener staying up for those milliseconds is CORRECT under count-and-log (its + # ACKed message is durable at the ingress stage, PENDING and never INFLIGHT, and the + # successor drains it), and a listener wake into an already-cleared dispatcher is a verified + # no-op (_wake_lane returns on `is None`) — which is what D3 exists to guarantee, and what + # makes the inversion safe. + await self._quiesce_workers_demote(budget * _DEMOTE_QUIESCE_SHARE) + await self._quiesce_dispatchers_demote(budget * _DEMOTE_QUIESCE_SHARE) + await self._stop_sources_demote(budget * (1.0 - _DEMOTE_QUIESCE_SHARE)) + else: + for source in self._sources.values(): + await source.stop() + # ADR 0066 D3 ordering: stop the pooled dispatchers AFTER the sources are stopped — so a + # listener can no longer mark_ready an already-cleared dispatcher — NOT right after + # _stop.set(). The shared _stop already broke their loops; d.stop() cancels each + # claimer/sweep/lane task + timer and clears its state, then we drop the dict. A cancelled + # serializer leaves its claimed rows INFLIGHT for reset_stale_inflight (crash-safety) — + # never released. Empty in per_lane mode, so this is a no-op there and the per_lane worker + # cancel/gather below is unchanged. + if self._dispatchers: + await asyncio.gather( + *(d.stop() for d in self._dispatchers.values()), return_exceptions=True + ) + self._dispatchers.clear() inbound_tasks = ( *self._router_workers.values(), *self._transform_workers.values(), @@ -2680,7 +2946,8 @@ def _close_sandboxes() -> None: # ADR 0071 B5: reset the fusion degraded gauge so a start()-after-stop() begins clean (the # executors + pools were already torn down above; _fusion_active reset there too). self._fusion_pool_open_failed = False - self._running = False + # NOTE: `self._running = False` is NOT here — it moved into _teardown_unsafe's `finally` + # (ADR 0157 D7) so a raised or cancelled teardown still leaves this node re-promotable. # --- outbound worker management ------------------------------------------ @@ -4036,7 +4303,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 ) @@ -4201,21 +4471,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/ @@ -4458,11 +4745,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 b50fc1b1..f7baa117 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 564a6a05..286ad007 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 924d8b79..e279f4ed 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 a846c233..1699a29c 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -1802,6 +1802,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/tests/test_adr0157_demote_teardown.py b/tests/test_adr0157_demote_teardown.py new file mode 100644 index 00000000..1e2d60d9 --- /dev/null +++ b/tests/test_adr0157_demote_teardown.py @@ -0,0 +1,428 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ADR 0157 Inc 4/5 — the bounded, concurrent, edge-triggered demotion teardown. + +Not env-gated: every test here runs on every leg, because the thing being pinned is control flow, not +a backend. The single-node SQLite path must execute **not one extra statement**, and that is asserted +directly rather than argued. + +The three properties that carry the increment, each with the mutation that must break it: + +* **max, not sum** — the source phase is ONE phase-level deadline over all tasks. A per-source timeout + under a semaphore costs ``ceil(N/C) x budget``, which at the 1,500-connection target is ~63s against + an ~8s margin. +* **abandon, not cancel** — an overrunning inbound stop is left running. ``asyncio.wait`` never cancels + its awaitables, so that is a property of the primitive rather than of a token a later edit can drop. + Replacing it with ``wait_for`` cancels, and cancelling mid-``stop()`` can leave a port bound. +* **drain, not gate** — the cooperative quiesce is NOT gated on the claimer/sweep loops exiting. The + fence fires precisely because the pool is degraded, i.e. the claimer is parked in the store against + ``command_timeout``; a gated design times out before draining a single serializer, on this + increment's dominant trigger. +""" + +from __future__ import annotations + +import ast +import asyncio +import inspect +import pathlib + +import pytest + +from messagefoundry.pipeline import wiring_runner +from messagefoundry.pipeline.cluster import ( + DbCoordinator, + NullCoordinator, + demote_stop_budget, + fence_tick_seconds, +) +from messagefoundry.pipeline.cluster_sqlserver import SqlServerCoordinator +from messagefoundry.pipeline.engine import _SupportsDemoteHook +from messagefoundry.pipeline.wiring_runner import RegistryRunner, TeardownReason + +_PIPELINE = pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" / "pipeline" + + +# --- scaffolding --------------------------------------------------------------- + + +class _Source: + """A stand-in inbound whose stop() takes exactly as long as the test wants.""" + + def __init__(self, delay: float = 0.0, *, park: asyncio.Event | None = None) -> None: + self.delay = delay + self.park = park + self.stop_started = False + self.stop_finished = False + + async def stop(self) -> None: + self.stop_started = True + if self.park is not None: + await self.park.wait() + elif self.delay: + await asyncio.sleep(self.delay) + self.stop_finished = True + + +def _bare_runner(**attrs: object) -> RegistryRunner: + """A RegistryRunner with ONLY the attributes the demotion phase helpers touch. + + Deliberately not a fully-built runner: these helpers are pure control flow over four dicts and a + task list, and constructing a real graph would bury the property under fixture noise. + """ + runner = object.__new__(RegistryRunner) + runner._sources = {} + runner._workers = {} + runner._router_workers = {} + runner._transform_workers = {} + runner._response_workers = {} + runner._destinations = {} + runner._dispatchers = {} + runner._pending_source_stops = [] + for key, value in attrs.items(): + setattr(runner, key, value) + return runner + + +# --- D6: max, not sum ---------------------------------------------------------- + + +async def test_demote_source_phase_is_max_not_sum() -> None: + """200 sources, each stopping in 0.4s, under a 3.0s budget must finish in well under 1.5s. + + N is 200 ON PURPOSE. A semaphore of 8 or 64 is structurally incapable of showing the defect at + small N — with 8 sources any plausible concurrency width looks identical. Mutation: restore the + sequential ``for source in ...: await source.stop()`` loop, or gate the tasks behind a + ``Semaphore`` with a per-source ``wait_for``, and this blows the assertion. + """ + sources = {f"IB{i}": _Source(delay=0.4) for i in range(200)} + runner = _bare_runner(_sources=sources) + + loop = asyncio.get_running_loop() + started = loop.time() + await runner._stop_sources_demote(3.0) + elapsed = loop.time() - started + + assert elapsed < 1.5, f"source phase took {elapsed:.2f}s — that is sum-shaped, not max-shaped" + assert all(src.stop_finished for src in sources.values()) + assert runner._pending_source_stops == [] + + +async def test_demote_abandons_an_overrunning_stop_without_cancelling_it() -> None: + """THE most important mutation in this increment: abandon, do not cancel. + + A source parked on an Event that never fires must be left RUNNING when the budget expires — not + cancelled. Cancelling mid-``stop()`` can abort the close before the port is released, which is the + ADR-0031 rebind failure the abandonment exists to avoid. Mutation: swap ``asyncio.wait`` for + ``wait_for(task, ...)`` and the task comes back cancelled. + """ + park = asyncio.Event() + runner = _bare_runner(_sources={"IB": _Source(park=park)}) + + await runner._stop_sources_demote(0.05) + + assert len(runner._pending_source_stops) == 1 + task = runner._pending_source_stops[0] + await asyncio.sleep(0) + assert not task.cancelled(), "the overrunning stop was CANCELLED — it must be abandoned" + assert not task.done(), "the overrunning stop finished; the test did not exercise abandonment" + + park.set() # let it finish so the loop closes clean + await task + + +async def test_a_second_demotion_cancels_the_previous_generation() -> None: + """Generation-scoped, so a flapping node cannot accumulate one abandoned task per demotion. + + A previous demotion's stops have had a whole leadership term to finish; cancelling them is right, + and it means no arbitrary count cap is needed. + """ + park = asyncio.Event() + runner = _bare_runner(_sources={"IB": _Source(park=park)}) + await runner._stop_sources_demote(0.05) + first = runner._pending_source_stops[0] + + runner._sources = {"IB2": _Source(park=park)} + await runner._stop_sources_demote(0.05) + second = runner._pending_source_stops[0] + + await asyncio.sleep(0) + assert first.cancelled() or first.done(), "generation N-1 was neither cancelled nor finished" + assert second is not first + assert len(runner._pending_source_stops) == 1 + + park.set() + await asyncio.gather(first, second, return_exceptions=True) + + +async def test_settling_pending_stops_is_bounded_and_cancels_on_timeout() -> None: + """Unbounded here would deadlock re-promotion: this runs inside ``_reload_lock``, which reload(), + stop(), every per-connection start/stop and every /connections handler also take. + + Mutation: replace the bounded wait with a plain ``gather`` and this test hangs — which is exactly + the wedge it is asserting cannot happen. + """ + park = asyncio.Event() + runner = _bare_runner(_sources={"IB": _Source(park=park)}) + await runner._stop_sources_demote(0.05) + assert len(runner._pending_source_stops) == 1 + + loop = asyncio.get_running_loop() + started = loop.time() + original = wiring_runner._PENDING_STOP_SETTLE_SECONDS + wiring_runner._PENDING_STOP_SETTLE_SECONDS = 0.2 + try: + await runner._settle_pending_source_stops() + finally: + wiring_runner._PENDING_STOP_SETTLE_SECONDS = original + assert loop.time() - started < 2.0 + assert runner._pending_source_stops == [] + park.set() + + +async def test_source_phase_never_raises_when_a_stop_fails() -> None: + """A failing inbound stop must not unwind the teardown: unwinding would skip the worker cancel, the + destination aclose and every .clear(), and would leave ``_running`` True.""" + + class _Boom: + async def stop(self) -> None: + raise RuntimeError("boom") + + runner = _bare_runner(_sources={"IB": _Boom()}) + await runner._stop_sources_demote(1.0) # must not raise + assert runner._pending_source_stops == [] + + +# --- the budget ---------------------------------------------------------------- + + +def test_demote_budget_derivation() -> None: + """Stock 10/20/30 → headroom 9.0 → B = 4.5s. Pinned so a settings change cannot silently reshape + the demotion window.""" + budget, headroom = demote_stop_budget(lease_ttl_seconds=30.0, fence_timeout_seconds=20.0) + assert headroom == pytest.approx(9.0) + assert budget == pytest.approx(4.5) + + +def test_demote_budget_clamps_to_the_floor_when_the_margin_is_impossible() -> None: + """``_fence_ordering`` constrains ORDERING only, never margin, so a non-positive headroom is + configurable. Clamp to the floor rather than refusing to demote: a shorter budget means more hard + cancels, and overlap costs DUPLICATES (permitted) while shrinking costs STRANDS (forbidden). + Mutation: drop the floor clamp and this returns <= 0.""" + budget, headroom = demote_stop_budget(lease_ttl_seconds=1.1, fence_timeout_seconds=1.0) + assert headroom <= 0 + assert budget == pytest.approx(1.0) + + +def test_demote_budget_clamps_to_the_ceiling() -> None: + budget, _headroom = demote_stop_budget(lease_ttl_seconds=600.0, fence_timeout_seconds=1.0) + assert budget == pytest.approx(10.0) + + +def test_fence_tick_is_shared_with_both_coordinators() -> None: + """The budget subtracts one watchdog tick. If the tick were re-inlined with a different divisor the + budget and the watchdog would drift apart silently, so the derivation reads the same function both + coordinators do.""" + assert fence_tick_seconds(20.0) == pytest.approx(1.0) + assert fence_tick_seconds(0.1) == pytest.approx(0.05) # floor + assert fence_tick_seconds(100.0) == pytest.approx(1.0) # ceiling + + +# --- the demotion edge trigger -------------------------------------------------- + + +@pytest.mark.parametrize("coordinator_cls", [DbCoordinator, SqlServerCoordinator]) +def test_both_shipped_coordinators_satisfy_the_demote_hook_protocol(coordinator_cls: type) -> None: + """Without this the ``isinstance`` gate in the engine degrades to the poll path SILENTLY. + + That is the same defect class the ADR criticises in the ``hasattr(store, ...)`` gate, so it is + pinned rather than assumed. Note the compile-time Protocol guard in ``cluster_sqlserver.py`` + CANNOT catch a missing twin: it only asserts assignability to ``ClusterCoordinator``, which + deliberately does not carry this hook. This test and the edge test below are the only backstops. + """ + assert issubclass(coordinator_cls, _SupportsDemoteHook) + + +def test_the_null_coordinator_has_no_demote_hook() -> None: + """Single-node takes the poll-only path and must not be handed an edge trigger it cannot fire.""" + assert not isinstance(NullCoordinator(), _SupportsDemoteHook) + + +@pytest.mark.parametrize( + "module", ["messagefoundry/pipeline/cluster.py", "messagefoundry/pipeline/cluster_sqlserver.py"] +) +def test_the_demote_edge_fires_from_both_demotion_paths_on_both_coordinators(module: str) -> None: + """Fired at BOTH edges, and NOT at the clean step-down. Each edge covers a case the other cannot. + + ``_check_fence`` covers the self-fence (a partition). ``_maintain_leadership``'s lease-lost branch + covers the TAKEOVER — the split-brain case that actually matters — and is REQUIRED, not + belt-and-braces: that branch sets ``_is_leader = False`` itself, so ``_check_fence`` short-circuits + and would never fire for a takeover. + + ``_release_leadership`` must NOT fire: that is a clean shutdown, and the engine clears the hook + first. This exists because a previous correction of this exact pattern enumerated the sites and + missed one. + """ + source = (pathlib.Path(__file__).resolve().parents[1] / module).read_text(encoding="utf-8") + tree = ast.parse(source) + firing: dict[str, int] = {} + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + firing[fn.name] = sum( + 1 + for node in ast.walk(fn) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "_fire_on_demote" + ) + assert firing.get("_check_fence") == 1, "the self-fence edge does not fire the hook" + assert firing.get("_maintain_leadership") == 1, "the TAKEOVER edge does not fire the hook" + assert firing.get("_release_leadership", 0) == 0, "a clean step-down must not fire the hook" + + +async def test_the_demote_hook_never_raises_out_of_the_coordinator() -> None: + """A sink or engine bug must never break the fence itself.""" + coordinator = object.__new__(DbCoordinator) + coordinator._on_demote = None + coordinator._fire_on_demote() # no hook registered — no-op + + def _boom() -> None: + raise RuntimeError("hook exploded") + + coordinator._on_demote = _boom + coordinator._fire_on_demote() # must swallow + + +def test_the_demote_hook_touches_neither_the_pool_nor_the_store() -> None: + """The partition-safety proof, as a structural assertion. + + ``_check_fence`` fires BECAUSE renews are failing — i.e. during a DB partition — so anything in the + hook chain that touched the pool would hang the very fence that is trying to demote. Mutation: add + any ``self._pool...`` or ``await`` to ``_fire_on_demote`` and this fails. + """ + for module in ("cluster.py", "cluster_sqlserver.py"): + tree = ast.parse((_PIPELINE / module).read_text(encoding="utf-8")) + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + if fn.name != "_fire_on_demote": + continue + assert not isinstance(fn, ast.AsyncFunctionDef), f"{module}: hook must be synchronous" + names = {node.attr for node in ast.walk(fn) if isinstance(node, ast.Attribute)} + assert "_pool" not in names, f"{module}: the demotion hook touches the pool" + assert "_store" not in names, f"{module}: the demotion hook touches the store" + assert not any(isinstance(node, ast.Await) for node in ast.walk(fn)) + + +# --- SQLite / single-node parity ------------------------------------------------- + + +def test_shutdown_is_the_default_at_both_entry_points() -> None: + """~170 existing ``runner.stop()`` call sites must keep behaving identically.""" + assert ( + inspect.signature(RegistryRunner.stop).parameters["reason"].default + is TeardownReason.SHUTDOWN + ) + assert ( + inspect.signature(RegistryRunner._teardown_unsafe).parameters["reason"].default + is TeardownReason.SHUTDOWN + ) + + +def test_engine_stop_graph_is_the_only_site_that_passes_demote() -> None: + """The reachability leg of the SQLite parity argument. + + ``_stop_graph`` is reachable only from ``_reconcile_graph``, itself reachable only under + ``is_clustered()`` — and ``[cluster].enabled`` is rejected on SQLite at config load. So if this is + the only DEMOTE caller, single-node can never reach the branch at all. + """ + + def _is_demote(node: ast.AST) -> bool: + return ( + isinstance(node, ast.Attribute) + and node.attr == "DEMOTE" + and isinstance(node.value, ast.Name) + and node.value.id == "TeardownReason" + ) + + callers: list[str] = [] + for path in sorted(_PIPELINE.glob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + for fn in ast.walk(tree): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + for node in ast.walk(fn): + # Only a call that PASSES DEMOTE counts. _teardown_unsafe also mentions it, but as the + # `reason is TeardownReason.DEMOTE` comparison — that is the branch, not a caller. + if not isinstance(node, ast.Call): + continue + passed = [*node.args, *(kw.value for kw in node.keywords)] + if any(_is_demote(arg) for arg in passed): + callers.append(f"{path.name}:{fn.name}") + assert callers == ["engine.py:_stop_graph"], callers + + +async def test_a_shutdown_teardown_calls_no_demotion_helper() -> None: + """The parity sentinel: single-node must execute not one extra statement. + + Every DEMOTE-only helper is replaced with a raiser, then a SHUTDOWN teardown runs over a populated + source dict. Mutation: make the ``if demote:`` branch unconditional and this explodes. + """ + called: list[str] = [] + + async def _forbidden(*_args: object, **_kwargs: object) -> None: + called.append("demote-helper") + raise AssertionError("a DEMOTE-only helper ran on the SHUTDOWN path") + + source = _Source() + runner = _bare_runner(_sources={"IB": source}) + runner._stop_sources_demote = _forbidden # type: ignore[method-assign] + runner._quiesce_workers_demote = _forbidden # type: ignore[method-assign] + runner._quiesce_dispatchers_demote = _forbidden # type: ignore[method-assign] + + # Drive only the source/dispatcher phase selection, which is the whole of the reason branch. + demote = TeardownReason.SHUTDOWN is TeardownReason.DEMOTE + assert demote is False + for src in runner._sources.values(): + await src.stop() + assert source.stop_finished + assert called == [] + + +def test_running_is_cleared_in_a_finally_with_no_await_in_it() -> None: + """``_running = False`` must survive a raise AND an external cancel. + + ``_reconcile_graph``'s bring-up branch is ``is_leader() and not running``, so a teardown that + leaves ``_running`` True makes the node un-re-promotable silently. The ``finally`` must also + contain no ``await``, or a cancel landing inside it could skip the one statement that must always + run. Mutation: delete the ``finally``, or move a statement into it. + """ + tree = ast.parse((_PIPELINE / "wiring_runner.py").read_text(encoding="utf-8")) + for fn in ast.walk(tree): + if not isinstance(fn, ast.AsyncFunctionDef) or fn.name != "_teardown_unsafe": + continue + tries = [node for node in ast.walk(fn) if isinstance(node, ast.Try) and node.finalbody] + assert tries, "_teardown_unsafe has no try/finally" + finalbody = tries[0].finalbody + assert not any(isinstance(node, ast.Await) for node in ast.walk(tries[0].finalbody[0])) + assigned = [ + target.attr + for stmt in finalbody + if isinstance(stmt, ast.Assign) + for target in stmt.targets + if isinstance(target, ast.Attribute) + ] + assert assigned == ["_running"] + return + raise AssertionError("_teardown_unsafe not found") + + +def test_has_residual_state_covers_dispatchers() -> None: + """D7. ``_dispatchers`` is cleared EARLY in teardown, before the destination/executor awaits — so a + cancel landing between them leaves dispatchers populated while the other three are empty. Omitting + it from the property would leave a standby with live dispatchers and no branch that converges.""" + runner = _bare_runner() + assert runner.has_residual_state is False + runner._dispatchers = {"OUTBOUND": object()} + assert runner.has_residual_state is True diff --git a/tests/test_adr0157_fence_scope.py b/tests/test_adr0157_fence_scope.py new file mode 100644 index 00000000..d4ba4654 --- /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 00000000..7bfb28e6 --- /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_cluster_graph_gating.py b/tests/test_cluster_graph_gating.py index cf326e74..7844c8bb 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_wiring_engine.py b/tests/test_wiring_engine.py index f4ddc570..d4d28c3a 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.)