diff --git a/docs/AOAG-DEPLOYMENT.md b/docs/AOAG-DEPLOYMENT.md index 751353235..6912ca5f4 100644 --- a/docs/AOAG-DEPLOYMENT.md +++ b/docs/AOAG-DEPLOYMENT.md @@ -574,9 +574,12 @@ cost of a slower crash-failover of the engine itself. Keep the load-enforced ### 5.4 Inbound MLLP VIP / LB MessageFoundry **designs for, but does not ship,** the floating VIP / L4 load balancer -([`DEPLOYMENT.md`](DEPLOYMENT.md), [`CLUSTERING.md`](CLUSTERING.md)). An engine-managed VIP is -**proposed only, with no code** ([ADR 0056](adr/0056-engine-managed-vip-failover.md)), so never -design as if the engine moves an IP. Stand up keepalived, HAProxy, F5, or an NLB with: +([`DEPLOYMENT.md`](DEPLOYMENT.md), [`CLUSTERING.md`](CLUSTERING.md)). The engine-managed **VIP +mechanism** of [ADR 0056](adr/0056-engine-managed-vip-failover.md) is **proposed only, with no code**, +so never design as if the engine moves an IP. What that ADR has actually shipped is only its control +plane, `POST /cluster/stepdown`, which moves *leadership* on request; the address still follows because +your health check stops passing on the node that released the lease. Stand up keepalived, HAProxy, F5, +or an NLB with: - **One VIP per inbound MLLP port, with the health check a TCP connect to that port.** Only the leader binds it, so the check passes only on the active engine and the VIP follows engine diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 492e2f803..c7d39ea24 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -28866,6 +28866,370 @@ drift into a named failure at commit time rather than a red a day later. --- +## 1494. ADR 0056's planned-failover control plane is unbuilt, so a maintenance switchover means stopping the primary service + +> 🚧 **Filed 2026-09-09. Slice 1 -- the control plane -- ships with this item; the VIP mechanism does NOT.** Value **6/10** · Difficulty **4/10**. Value 6 -- it turns "reboot the primary and hope" into a first-class, audited operator action, and it is the piece of ADR 0056 that needs no privileged helper. Difficulty 4 -- the release logic already existed and was private; the work is the seam, the RBAC, and one race the ADR did not consider. + +**Cluster:** active-passive HA / operator control surface. **Priority:** P2. **Verdict:** build slice 1; +leave the VIP mechanism gated on the owner's privileged-helper decision. +**Severity:** no deployment axis (sec. 0). Zero deployments, so nothing is being drained today. This is +a missing capability, not a defect in shipped behaviour. + +### What was missing + +An operator who wants to patch the active-passive primary has, on `main` before this item, exactly one +way to move leadership: stop the engine service. That expires the lease and a standby promotes, but it +also takes the node out of the cluster, and it is a service-control action rather than an audited, +RBAC-gated one. ADR 0056 specified the alternative -- `POST /cluster/stepdown`, the leader voluntarily +releasing its lease and staying up -- and nothing had been built. + +Measured on this worktree at `a2bfdb231`: no `CLUSTER_CONTROL`, no `cluster:control`, no `stepdown` +anywhere under `messagefoundry/`, no `[cluster.vip]` settings block. The `vip` hits in `api/app.py`, +`api/models.py`, `pipeline/dr.py`, `pipeline/engine.py` and `config/settings.py` belong to ADR 0048's +disaster-recovery hook commands, which are a different mechanism. + +### What ships here + +- `ClusterCoordinator.step_down_leadership() -> tuple[bool, float | None]` on all three coordinators. + A visibility lift of `_release_leadership()`, which was already crash-correct and already demoted the + in-memory gate before touching the DB; `NullCoordinator` returns `(False, None)`. +- `CLUSTER_CONTROL` (`cluster:control`), Administrator-only and in + `CUSTOM_ROLE_FORBIDDEN_PERMISSIONS`, so "Administrator only" is enforced on every minting path rather + than merely observed of the built-in roles -- the same treatment `dr:operate` gets. +- `POST /cluster/stepdown` behind `require_step_up`, which supplies the ADR's whole decision-table row + in one wrapper: per-actor admin-write pacing, the TOTP MFA gate, the new-client-IP signal, and the + credential-recency window. +- The audit row `cluster_stepdown`, carrying `{node_id, was_leader, released_at}` **as the coordinator + returned them**. No `is_leader()` pre-read exists in the handler at all, so there is no reading for a + fence or a lost-lease tick to invalidate. `tests/test_api_cluster_stepdown.py` proves this with a + coordinator whose two answers deliberately disagree, in both directions. + +- **A release that did not happen is reported as a failure, not a drain.** The private release was + best-effort: it caught a pool error, logged a warning and returned `(was_leader=True, released_at)`, + so the endpoint answered `200` reading "drained". That is right for `stop()` -- the node is leaving + and a lease that ages out costs nothing -- and wrong for a stepdown, where the node stays up: the + lease row is untouched and still owned by it, no sibling can take it, and it renews itself back in + through the unfenced `owner = me` branch when the pause ends. On the shipped defaults (heartbeat 10, + fence 20, TTL 30) the pause ends at 20 while the lease lives to 30, and the settings validator pins + `heartbeat < fence < ttl` without ever comparing the pause to the TTL, so nothing prevents it. The + release now reports whether its write returned and `step_down_leadership()` raises, which the + endpoint maps to `503` -- the status the neighbouring DR endpoints and ADR 0056's contract already + give environment conditions -- and audits `cluster_stepdown_denied` rather than a `cluster_stepdown` + row saying the node was drained. +- **The two refusals are two exceptions, two bodies and two audit reasons.** The first cut of that fix + gave both raise sites one `except StepdownUnavailable` arm, one body ("could not release leadership; + it is still the leader") and one reason (`release-failed`) -- and that sentence is false of each in a + different way. The lock timeout fires BEFORE any release runs, on a node the handler never checked + for leadership, so it may lead nothing; it is now `StepdownLockTimeout` / `lock-timeout`, and its + message names the LOCK and the bound rather than a maintenance tick, since both coordinators take + that lock in `_maintain_leadership` AND in `step_down_leadership`. The write failure is now + `StepdownReleaseUnconfirmed` / `release-unconfirmed`. +- **The write-failure refusal is worded conditionally, because the outcome is genuinely unknown.** A + lost response to a committed `UPDATE` is indistinguishable from an `UPDATE` that never ran -- the + exception's own docstring said so while the endpoint shipped the flat certainty "it is still the + leader". On the committed branch that sentence sends an operator to fix a cluster that is already + failing over correctly. A row count cannot earn the certainty back either: the driver reports one + only on the path where it returned, and this refusal exists for the path where it raised. The body + now says the node has cleared its leadership flag, that the lease MAY still be live and ours, and + what to do next. *(Its teardown sentence was corrected twice more in this same PR -- see the two + CORRECTED paragraphs under "Corrections made while re-reading the shipped stepdown"; what it says + today is neither of the two versions this bullet has quoted.)* **An earlier cut of that body -- and + of this line -- said the + node "stopped serving", which is false.** `Engine._on_demote_edge` (`pipeline/engine.py`) is + `_graph_wake.set()` and its docstring says it deliberately does not set the runner's `_stop`; the + teardown runs afterwards on the graph-supervisor task via `_stop_graph`, whose pinned comment keeps + the connector-close phases unbounded; and every listen-type inbound (`transports/mllp.py`, + `tcp.py`, `http_listener.py`, `dicom.py`, `x12.py`) says in terms that `leader_gate` is ignored, so + those keep accepting on their own ports until teardown reaches them. On a first deployment an + operator reading "stopped serving" would begin maintenance on a node still bound to its port and + still ACKing. `docs/CLUSTERING.md` carried the same false claim and now says the same true one: + confirm quiescence with `GET /cluster/nodes` plus the connection view, never with the status code. +- **A retry of an unconfirmed release re-sends the write.** `_release_leadership` demotes the in-memory + gate before the write, so the retry hit its not-a-leader early return, sent nothing, and the endpoint + answered `409` "is not the current leader" over a lease row still live and still owned by that node + -- with the `409`'s own documented remedy (resolve the leader from `GET /cluster/nodes`) pointing + back at the same node, since that API still names it lease owner. Both coordinators now carry + `_lease_release_owed` and force the write past that early return, and re-arm the claim pause on the + retry so the successful release is not undone by the unfenced `owner = me` renew branch on the next + tick. Scoped to a release this node OWES -- and read that scope exactly, because an earlier revision + of this line stated it absolutely and its own not-fixed list below then said the opposite. + `step_down_leadership` arms the pause on `self._is_leader or owed`, so a stepdown addressed by + mistake to a standby that owes NOTHING sends nothing and arms no pause, and cannot delay the + failover the caller is trying to perform. A standby that DOES owe a write is the other branch: it + re-sends that write and arms the pause on a node that is by then a follower, which is the stale-owed + subject in the not-fixed list. +- **The lock's wait is bounded, and what the lock costs is written down.** Serializing against the tick + puts the SYNCHRONOUS in-memory demotion behind a tick's DB round trip, so a drained node keeps + answering `is_leader()` and keeps binding listeners while the call waits. Nothing bounded that wait: + `[store].command_timeout` is the only ceiling, `PostgresStore` passes `command_timeout or None` so + the documented zero-disables value removes even that, and the pool `acquire()` carries no timeout. + The wait is now bounded at `leader_fence_timeout_seconds` -- derived, not picked: past it the node's + own watchdog has concluded its DB access is not working -- and a timeout refuses with `503` without + touching leadership. The docstring that claimed the lock "adds no new stall to this method either" + was a compensating control resting on a false premise and now states the trade instead. + +Deferred on the ADR's own terms: the `force` flag, and `new_leader_eligible` in the result (at the +instant of release no standby has acquired yet, so the caller re-polls `GET /cluster/nodes`). + +### The race the ADR did not consider, and why one line of new behaviour was unavoidable + +ADR 0056 says the stepdown differs from a clean stop only in that the node "keeps running and +heartbeating afterward (demoted to standby)", and that the standby then "acquires the expired lease". +That second half does not follow from the first. + +`_release_leadership()` sets `lease_expires_at = 0` but leaves `owner` naming the releasing node. The +claim statement's renew branch is `WHERE leader_lease.owner = $2 OR ` -- and the renew half +carries **no expiry test**. So on a clean stop the ordering is safe (the maintenance loop is already +cancelled), but on a stepdown the loop is still running: the drained node's very next tick matches its +own renew branch and takes leadership straight back. Whether the drain works at all comes down to which +node's heartbeat phase lands first. The endpoint would have answered `200` either way. + +Half the fix is a bounded post-stepdown claim pause, `2 * heartbeat_seconds`, checked in exactly the +position ADR 0096's `promotable = false` short-circuit already occupies. It is a strictly stricter claim +predicate on one node, so by ADR 0096's own argument it can only make that node claim later, never +earlier. It touches neither the lease, nor the self-fence, nor the epoch token. +`tests/test_cluster_lease.py` carries the regression **and its negative control** -- clear the pause and +the same sequence hands leadership straight back, so the guard cannot silently stop measuring anything. + +**CORRECTION, 2026-09-09, same PR. This section first said the pause "cannot open a two-leader window". +That was wrong, and the way it was wrong is worth more than the sentence it replaces.** ADR 0096's +argument is about a claim PREDICATE, and it transfers intact: a stricter predicate cannot make a node +claim earlier. The pause is a stricter predicate, so the argument was correctly applied -- to a question +it does not answer. A predicate is evaluated at one instant; the window here is an INTERVAL, opened by +the fact that `_release_leadership()` suspends at its `await`, and a predicate that is true when read +says nothing about what a coroutine already past it will do when it resumes. Borrowing a neighbouring +safety argument whose subject is not the same is the defect, not the arithmetic. + +Two interleavings were reproduced against the repository's own stand-in, one `asyncio.sleep(0)` in the +fake pool, both with the pause armed: + +- a maintenance tick that STARTS inside the release's await window renews the lease the release is + expiring, and `_is_leader` goes back to true while the release then expires that same row. The node + reports leader and a sibling takes the expired lease: **both consider themselves leader**; +- a claim ALREADY IN FLIGHT when the stepdown arrives has passed the pause check before the pause was + armed, so it returns held afterwards and `_maintain_leadership` promotes on that stale result -- + leaving the node leader with a live lease no sibling can take for a full TTL. Arming the pause earlier + is measured NOT to close this one, which is what decides the fix. + +The other half of the fix is therefore mutual exclusion: `_leadership_lock`, an `asyncio.Lock` held +across the release and across the whole maintenance tick, on both DB coordinators. `pipeline/dr.py` +already holds one for its analogous promote/release pair. `stop()` deliberately does not take it -- it +cancels and gathers both loops first, so nothing competes, and taking it would queue a shutdown behind a +stepdown stalled on a hung pool. Both interleavings are pinned by regression tests carrying their +fails-without readings, and the SQL Server twin has its own (its `MERGE` carries the identical unfenced +`t.owner = ?` renew branch, and only its three FIFO claim paths are epoch-fenced, so a re-promoted +ex-leader there is not fenced out of `claim_ready` or any terminal resolve). + +**Severity, in the conditional (sec. 0): zero deployments, so nothing is drained today.** A first +deployment that used this endpoint would have hit it -- not a certainty per call, a race whose outcome +depends on where the heartbeat phase falls. + +**The cost is stated rather than hidden:** on a cluster with no other promotable node, the pause window +is leaderless. That is the honest consequence of asking the only eligible node to step down. + +**Three gaps found with the race and deliberately NOT fixed here. They are filed, so read them there:** + +- **#1507** -- `stepdown_pause_seconds` cannot see a sibling's ADR 0096 `acquire_delay_seconds` and can + be shorter than it, letting the drained node reclaim. +- **#1508** -- a self-fenced node cannot be drained at all: the endpoint answers `409` while the same + API's `lease_owner` still names that node. +- **#1509** -- the endpoint's `400` gate keys on `is_clustered()`, a constant `True` on a DB + coordinator, rather than on whether a promotable sibling exists. + +An earlier revision of this section named these by subject with no number, on the reasoning that citing +an unallocated `#N` is worse than citing nothing. That reasoning is right and the conclusion was not: +the numbers were allocated and the items filed in the same change, which is the ordering +[LEDGER-GATE.md](LEDGER-GATE.md) asks for -- allocate, then FILE, then cite. Prose that names a subject +and no number is not tracked by anything, which is how a known gap gets re-derived by the next reader. + +### What does NOT ship, and what gates it + +The VIP mechanism: `[cluster.vip]`, bind/release, the gratuitous ARP, the self-fence release path, +`mefor-net-helper.exe`, and the `vip` field on `GET /cluster/status`. All of it depends on granting the +engine network-configuration rights, which collides head-on with DEPLOY-1's least-privilege direction. +ADR 0056 chose the privileged-helper option on paper. + +**CORRECTED 2026-09-09, same PR: this said "nobody has signed off ... that decision is the gate", and +by then somebody had.** The ruling, and the standard of evidence behind it, are recorded once in +[ADR 0056](adr/0056-engine-managed-vip-failover.md)'s status block; read it there rather than here. +The correction is kept because the wrong claim was load-bearing for a reader deciding whether to build +the VIP half, not because the disagreement it describes will be visible later. + +**And the disagreement it describes was never SHIPPED, which an earlier revision of this paragraph got +wrong twice over.** It said "the two shipped records disagreed". Neither revision had reached `main`: +both the ADR index assertion and this denial were drafts inside this same pull request. This repository +also **squash-merges**, so the intermediate revisions carrying that disagreement collapse into one +commit and the evidence for the sentence does not survive the merge at all. Two lessons, and the second +is the general one: a record is not "shipped" until it is on `main`, and a correction that cites its own +branch history as evidence is citing something the merge will delete. + +### Also found while reading ADR 0056 + +**TWO of its sections are stale, not one, and the second is the one that matters.** The first revision +of this paragraph named only "Console -- High Availability page" and the ADR's status block scoped its +STALE marker to that section alone. But `### Confirm / step-up posture (console)` sits INSIDE +`## Control API -- planned failover`, the section the status block declares BUILT, so a reader arriving +at it was told the surrounding prose was current. It names `client.stepdown_node`, `poll_client`, +`_request` and `AsyncRunner`. **An earlier revision of this line called all four retired PySide6 +console symbols, and that is wrong; ADR 0056 carries the correction and this record now matches it.** +Three of the four are alive, REHOMED rather than retired: `_request` in +`messagefoundry/apiclient/client.py`, `AsyncRunner` in `harness/_async.py`, and `poll_client` in +`harness/_console_widgets.py`. +Only `client.stepdown_node` is absent from the tree, and that absence is the control showing the +check discriminates rather than matching everything. What is stale is the SEAT, not the machinery. It +also tells the confirm dialog +to promise the operator that "the VIP will move", which the paused-VIP bullet three lines up denies. +Both sections now carry their own do-not-build-from marker at the section itself, rather than relying on +a reader having read the status block first. + +Its "Console -- High Availability page" section is stale. It names `console/shell.py`, +`console/status.py` and `console/connections.py`, all of which went with the retired PySide6 desktop +console; the operator UI is the web console at `/ui`. The topology reasoning in that section still holds +(one page renders the whole cluster from any node, so Corepoint's "Viewing: Primary / Backup" toggle has +no analogue) but its construction notes point at files that do not exist. The ADR's status block now +says so; the section itself is kept for the reasoning. + +### Corrections made while re-reading the shipped stepdown, 2026-09-09 + +**The `503` no longer claims the node stopped serving, in the body or in `docs/CLUSTERING.md`.** +The trace is in the write-failure bullet above. Both now send the operator to `GET /cluster/nodes` +plus the connection view for quiescence rather than to a status code. + +**CORRECTED 2026-09-09, same PR: the replacement sentence was false too, and in a way the first fix +made easy to miss.** It said teardown's "later phases are unbounded, so this node's listeners keep +accepting until it completes". The unbounded half is true and the consequence does not follow. +`RegistryRunner._teardown_body` runs the source stop as the LAST of the three BOUNDED demote phases +-- after `_quiesce_workers_demote` and `_quiesce_dispatchers_demote`, each taking its own share of +the budget (0.7/0.7/0.3, so the shares are bounded and their sum is not the budget) -- and only then +reaches the unbounded connector-close, executor-shutdown and sandbox-close phases. MLLP, TCP, HTTP +and X12 each call `server.close()` in the synchronous prologue of their own `stop()`, so accept stops +on the first loop pass of that phase, EARLIER than "until it completes" rather than later. The node +is still not quiescent, for weaker reasons that are now what both texts say: an overrunning source is +ABANDONED rather than cancelled, DICOM releases its port inside exactly the call that gets abandoned, +established connections drain afterwards, and a message already in a handler finishes its commit and +its ACK. **The general lesson is the one this item keeps paying for:** the first fix traced the +mechanism it was thinking about -- unbounded phases -- and not the branch the sentence quantified +over, which was every listener in a phase that runs before them. + +**CORRECTED 2026-09-09, same PR: the `503` body no longer says a teardown started ON THIS CALL.** +It read "cleared its leadership flag and started tearing its graph down". `_fire_on_demote` runs only +under `if was_leader` in `step_down_leadership`, which a retry of an owed write has already cleared, +so on a repeat refusal no edge fires and no teardown starts. The direction is conservative -- it +overstates disruption, not safety -- but it is still false on that branch. The body now describes the +demotion teardown as a mechanism that runs on the graph supervisor rather than asserting one began +here, and says what IS true on every branch reaching the raise: the node has cleared its leadership +flag, and this call armed its claim pause (the pause is armed on `self._is_leader or owed`, the same +condition under which the write is attempted at all). + +**Cancellation escaped the owed-write contract, and both coordinators now arm the flag before the +write.** `_release_leadership` caught `Exception`, which cannot catch `asyncio.CancelledError` (it +derives from `BaseException`), so a cancelled lease-expiring write unwound with `_is_leader` already +cleared and `_lease_release_owed` still False. The next stepdown then took the not-a-leader early +return, sent no write, and answered `409` "not the current leader" over a lease that may still be +live and owned -- with no audit row of either kind on that path. Reachable in the shipped +configuration rather than only in theory: `RequestTimeoutMiddleware` is registered unconditionally +and its `asyncio.timeout` cancels the handler at `DEFAULT_REQUEST_TIMEOUT_SECONDS = 120.0`, over a +pool acquire the stepdown docstring documents as unbounded. Both coordinators now set the flag +BEFORE the write and clear it only on one that returned, so neither a raise nor a cancellation can +leave it clear. + +**"Expect the retry to answer `409`" is scoped to the claim pause.** It holds only for the two +`heartbeat_seconds` a stepdown declines to claim. The document contradicted itself: the bullet three +lines above already described the post-pause re-arm. Now scoped in `docs/CLUSTERING.md` and in the +endpoint docstring. + +**CORRECTED 2026-09-09, same PR: "past the pause expect `200`" was false on one of its two +branches, and the branch it was false on is the dangerous one.** The claim statement has exactly two +arms -- renew, `WHERE leader_lease.owner = $2`, carrying no expiry term, and take-over, gated on +`leader_lease.lease_expires_at + $4 < clock_timestamp()`. If a standby takes over DURING the pause, +`owner` is no longer the drained node and the lease is live, so NEITHER arm matches, +`_claim_or_renew_lease` returns not-held, and the retry reports `was_leader=false` -- a `409`, not a +`200`. The harm is that +the `409`'s own documented remedy sends the operator to step down whichever node `GET /cluster/nodes` +names as leader, which by then is the standby that took over CORRECTLY: on a first deployment an +operator following the text would drain the healthy new leader. Both texts now split the two branches +-- `200` when the row still names the drained node at the end of the pause (the write never +committed, or committed with no standby taking the lease), `409` when a standby acquired -- and both +say that the `409` there means the failover worked. The bullet above it already said, correctly, "if +it committed, a standby acquires on its next heartbeat"; the two could not both stand. + +### Found while making those corrections, recorded rather than fixed + +- The `cluster_stepdown` audit cannot tell a confirmed retry from a stepdown addressed to the wrong + node: both write `was_leader: false` with a null `released_at`. +- A stale `_lease_release_owed` on a node that is no longer leader would make the next stepdown arm + the claim pause on an innocent follower. **Reached through the documented remedy, not by operator + error:** a `503` tells the operator to retry, and a retry landing after a standby has taken over + arms the pause on a node that is by then a follower. +- `stop()`'s comment enumerating what it and `step_down_leadership` share still lists only `_is_leader` + and the owner-scoped `UPDATE`; it does not mention `_lease_release_owed`. +- The stepdown docstring's "every other True->False transition fires it" enumerates + `_maintain_leadership` and `_check_fence` and closes. `stop()` reaching `_release_leadership` is + also a True->False transition and fires no demotion edge, so the enumeration is closed over a set + that is missing a member. +- **A THIRD undocumented `503` reaches this endpoint from outside it.** `RequestTimeoutMiddleware` is + registered unconditionally in `create_app` and answers `503` at `DEFAULT_REQUEST_TIMEOUT_SECONDS` + (120.0) with its own PHI-free body. It writes NEITHER a `cluster_stepdown` nor a + `cluster_stepdown_denied` row, and nothing in the endpoint's own status list names it -- so an + operator who hits it sees a `503` this document says is one of two conditions and finds no audit + row for either. It is also the cancellation path the owed-write flag was armed early to survive. +- `docs/SECURITY.md`'s OIDC parenthetical says "the two `/ui/oidc/*` routes"; the same document names + three of them twice over (`GET`/`POST /ui/oidc/start` and `GET /ui/oidc/callback`). + +--- + +## 1495. ADR 0056's High Availability page is specified against the retired PySide6 console, so the web console has no cluster page at all + +> 🔢 **Filed 2026-09-09, found while building #1494's control plane. The number was allocated then and cited from the ADR index before this item existed, which is the defect the item below records first.** Value **4/10** · Difficulty **4/10**. Value 4 -- an operator can already read `GET /cluster/nodes` and `GET /cluster/status` and can already drive `POST /cluster/stepdown` over the API, so the gap is that nothing renders them, not that the data is missing. Difficulty 4 -- one read-mostly page over three endpoints that already exist, plus the step-up confirm flow the stepdown control needs. + +**Cluster:** web console / active-passive HA operator surface. **Priority:** P3. +**Severity:** no deployment axis (sec. 0). A missing view over shipped endpoints, not a defect in +shipped behaviour. + +### The citation came before the item, and that is recorded first on purpose + +`1495` was allocated atomically on 2026-09-09 and cited from `docs/adr/README.md`'s ADR 0056 row in the +same session -- before any `## 1495.` heading existed. The allocation record satisfied the ledger gate, +so nothing failed. But the allocation store lives under the primary checkout's git directory and is not +in git: removing the claiming worktree would have released the number while the published citation +stayed in a merged file, and the day someone legitimately re-allocated `1495` that citation would have +started resolving to unrelated work with nothing reporting a problem. Filing the item is what closes +that, and the ordering is the lesson: allocate, then FILE, then cite. + +### What is missing + +ADR 0056 specifies a read-mostly "High Availability" page -- the Corepoint A2 equivalent -- and names +`console/shell.py`, `console/status.py` and `console/connections.py` for its construction. All three +went with the retired PySide6 desktop console (BACKLOG #103); the operator UI is the web console at +`/ui`. The ADR's status block now marks that section do-not-build-from and keeps it for its topology +reasoning, which does survive the move: one page renders the whole cluster from any node, because every +node reads the same shared `nodes` and `leader_lease` rows, so Corepoint's "Viewing: Primary / Backup" +toggle has no analogue here. + +### What it would take + +Every endpoint already exists and is RBAC-gated: + +- `GET /cluster/status` and `GET /cluster/nodes` (ADR 0008) -- membership, per-node `last_seen`, + derived leadership, the lease owner and its expiry, plus each node's ADR 0096 `acquire_delay_seconds` + and `promotable`. Both read-only under `monitoring:read`. +- `POST /cluster/stepdown` (#1494) -- planned failover, `cluster:control` behind `require_step_up`. + +So the work is a page, not an API: render the membership table with a live/stale marker off `last_seen`, +show who holds the lease and when it expires, and put the stepdown behind an explicit confirm that +carries the step-up + MFA challenge the endpoint already demands. The endpoint answers `409` when the +node addressed is not the leader, and `400` when the deployment is not CLUSTERED -- which is not the +same as single-node, see #1509 -- so the page must resolve the leader from `GET /cluster/nodes` before +it offers the control rather than offering it everywhere. + +### What NOT to build here + +The VIP fields. `GET /cluster/status` has no `vip` member and the engine binds no address -- that half +of ADR 0056 is unbuilt and paused (see #1494). A page that renders a VIP owner would be rendering a +field that does not exist. + +--- + ## 1497. ADR 0157 leaves increments 0, 2 and 3 unbuilt, says increment 2 is mis-specified, and no open item carries any of it > 🔢 **Filed 2026-09-09 -- not started. Scored at filing.** Value **6/10** · Difficulty **6/10** · _big bet_. Found by an ADR-to-backlog sweep. The ADR names three unbuilt increments in its own opening blockquote and warns that one of them must not be built as written. Its only backlog reference is a closed test-flake row about a wall-clock assertion, so the engine work has no home. Value 6: on a first deployment against SQL Server this is an absent in-flight recovery path plus two unfenced write paths on a demoted node. Difficulty 6: cross-backend store work under the fence invariant, and the specification has to be repaired before anyone can build it. @@ -29075,6 +29439,155 @@ Zero non-test callers exist in the engine. **Scope, stated so nobody over-corrects.** The README index row is **honest** -- it names only `msg.set` / `msg.field`-copy / `msg.delete_segments` and the actions `set_field` / `copy_field` / `delete_segment`. `tests/test_lens_native.py` covers exactly the shipped forms, consistent with there being no read row. The over-claim is internal to the ADR file. +--- + +## 1507. The post-stepdown claim pause cannot see a sibling's acquire_delay_seconds, so a drained node can win its own lease back + +> 🔢 **Filed 2026-09-09, found while building #1494's control plane and deliberately not fixed there.** Value **4/10** · Difficulty **3/10**. Value 4 -- it turns a planned failover into a no-op on exactly the deployments that configured leader preference, which is a config an operator chose on purpose. Difficulty 3 -- the arithmetic is easy; deciding what the pause should read, and from where, is the work. + +**Cluster:** active-passive HA / planned failover. **Priority:** P3. +**Severity:** no deployment axis (sec. 0). Zero deployments, so nothing is being drained today. A first +deployment that combined `POST /cluster/stepdown` with a sibling carrying an ADR 0096 handicap would see +the stepdown appear to succeed and leadership never move. + +### The gap + +`stepdown_pause_seconds(heartbeat_seconds)` returns `2 * heartbeat_seconds`: the drained node declines +to claim or renew for that long, so a sibling has a full tick at the expired lease. That reasoning holds +only for a sibling carrying **no** ADR 0096 `acquire_delay_seconds`. + +A sibling handicapped by more than the pause is still refused when the pause ends, because +`acquire_delay` is added to the expiry side of the take-over predicate while the drained node's own +renew branch (`owner = me`) carries no delay term at all. So the drained node reclaims its own lease and +the operator who called the endpoint is still on the leader they asked to drain. The endpoint answered +`200` and the audit row says `was_leader: true` -- both true of the release, neither true of the outcome. + +The function reads `heartbeat_seconds` alone. It is module-level, shared by both DB coordinators, and +takes one scalar, so it cannot see the handicap it is being compared against. Its docstring says so +today; nothing enforces it. + +### What a fix has to decide + +Not the arithmetic -- `max(2 * heartbeat, longest sibling acquire_delay + one heartbeat)` is the obvious +shape -- but where the number comes from, and that is the real question: + +- The coordinator would have to READ the siblings' delays. `cluster_members()` already returns + `acquire_delay_seconds` and `promotable` per node, so the data exists, but it is a DB read and the + pause is currently pure arithmetic on a constructor argument with no I/O in it. Putting a round trip + inside the stepdown's critical section trades one defect for the stall #1494's lock already pays for. +- Or the SETTINGS validator refuses the combination at load time. It already pins + `heartbeat < fence < ttl` (`config/settings.py`, `_fence_ordering`), but a cluster-wide comparison is + not available to it: each node loads only its own config and a sibling's delay lives in the sibling's + file. A local validator can only warn. +- Or the pause stops being a duration: the drained node declines until it OBSERVES a different owner on + the lease row, with the current pause as a floor and the lease TTL as a ceiling. Correct, and it + reintroduces a DB read on the same path. + +Nothing here is cheap, which is why #1494 stopped rather than guessing. Pair this with #1508: both are +the pause and the fence disagreeing with the lease row, and a fix that reads the lease row could answer +both. + +## 1508. A self-fenced node cannot be drained at all: stepdown answers 409 while the same API still names it lease owner + +> 🔢 **Filed 2026-09-09, found while building #1494's control plane and deliberately not fixed there. The reviewer isolated the cause -- do not re-derive it.** Value **5/10** · Difficulty **4/10**. Value 5 -- the window is up to `ttl - fence` wide on stock defaults, and it is exactly the window an operator reaches for the endpoint in, because a node that just self-fenced is a node something is wrong with. Difficulty 4 -- one early return, but changing it means deciding what a release means on a node that already believes it is not leader. + +**Cluster:** active-passive HA / planned failover. **Priority:** P2. +**Severity:** no deployment axis (sec. 0). Zero deployments. A first deployment would see the endpoint +refuse a drain it could have performed, and refuse it with a reason the rest of the same API contradicts. + +### The defect + +The self-fence watchdog demotes on the node's own MONOTONIC clock at `leader_fence_timeout_seconds` +(20.0 by default) while the lease row stays live on the DB clock to `leader_lease_ttl_seconds` (30.0 by +default). In that window the node is not leader in memory and still owns a live lease row. + +`POST /cluster/stepdown` in that window: + +1. `step_down_leadership()` reaches `_release_leadership()`, which reads `_is_leader` -- already + `False` -- and takes the `if not was_leader: return (False, None, True)` early return; +2. so it issues **no** `UPDATE`, arms **no** claim pause, and fires no demotion edge; +3. the endpoint answers `409 not the current leader`; +4. and `GET /cluster/nodes` on the same API still reports `lease_owner` as that node, because the row is + untouched and does not expire for another ten seconds. + +An operator reading the two answers together cannot reconcile them, and the honest reading -- "this node +holds the lease and refuses to release it" -- is the correct one. + +### The cause is the early return, not the SQL, and that was measured + +The reviewer issued the method's own `UPDATE` by hand against a coordinator in exactly that state +(`_is_leader` false, row live and owned). It drained correctly: the row expired and a sibling took it. So +the write is capable of doing the job in this state, and the `if not was_leader` guard is the only thing +preventing it. That rules out the plausible alternative -- that the owner-scoped `WHERE` no longer +matches -- without anyone having to re-test it. + +### What a fix has to decide + +`was_leader` currently answers two different questions with one boolean: *did this node hold the +in-memory gate* (what the audit row should record) and *is there a lease row to expire* (what the +release should act on). They diverge exactly in the self-fence window. Options, none free: + +- **Release on the ROW, report on the GATE.** Always issue the owner-scoped `UPDATE`; keep `was_leader` + reporting the in-memory gate. Then a self-fenced node drains and honestly reports `was_leader: false` + -- but the endpoint's `409` rests on `was_leader`, so the status table has to change too, and "we + released your lease and returned 409" is worse than what it replaces. +- **Add a third answer.** `step_down_leadership()` reports whether it expired a row as well as whether + it held the gate, and the endpoint answers `200` when either is true. Truthful, and it widens a + Protocol return that three coordinators implement. +- **Refuse honestly instead.** Keep the `409` and make `GET /cluster/nodes` stop naming a self-fenced + node as `lease_owner`. That fixes the contradiction rather than the drain, and it needs the + observability read to consult in-memory fence state it does not read today. + +Pair with #1507: both are the pause and the fence disagreeing with the lease row. + +## 1509. The stepdown 400 gate asks whether clustering is enabled, not whether anything can take over + +> 🔢 **Filed 2026-09-09, found while building #1494's control plane and deliberately not fixed there.** Value **4/10** · Difficulty **2/10**. Value 4 -- the refusal it is meant to give exists to stop an operator making the cluster leaderless, and it does not give it. Difficulty 2 -- the data is already exposed; the work is one predicate plus deciding how stale a sibling may be. + +**Cluster:** active-passive HA / planned failover. **Priority:** P3. +**Severity:** no deployment axis (sec. 0). Zero deployments. A first deployment running one clustered +node could make itself leaderless for the stepdown pause by calling the endpoint. + +### The defect + +`POST /cluster/stepdown` refuses with `400` when `coordinator.is_clustered()` is false. That method is a +literal `return True` on **both** DB coordinators, and a literal `return False` only on +`NullCoordinator`: it is a property of the BACKEND, not of the deployment's size. Its own docstring says +so -- "a plain backend property, not who-is-leader". + +So the gate answers "is this a clustered BUILD", while the refusal it is written to give is "is there +anything here to take over". A `[cluster].enabled` install running one node -- the ordinary shape while a +second node is being provisioned, and the shape a test bed sits in -- passes the gate, releases its +lease, and is leaderless for `2 * heartbeat_seconds` with nothing able to promote. The same is true of a +cluster whose only sibling is `promotable = false`, or has not heartbeated within a node timeout. + +The shipped docs are not what needs fixing. `docs/CLUSTERING.md` describes the refusal as the code +gives it -- not clustered, and explicitly not "one node" -- so a reader is told the truth about a gate +that is still the wrong gate. The predicate is the work. + +### The check is available and unused + +`cluster_members()` already returns, per node: `node_id`, `last_seen`, `status`, `is_leader`, +`promotable` and `acquire_delay_seconds`. A promotable-sibling predicate is a filter over that list -- +another node, `promotable`, and `last_seen` inside `node_timeout_seconds`. + +### What a fix has to decide + +- **How stale is too stale.** `cluster_members()` already derives liveness by AND-ing the flag with a + fresh `last_seen`, so reuse that rather than inventing a second freshness rule. +- **Which status the refusal carries.** `400` says "your request was malformed", which a + single-node-clustered call is not -- the deployment state is what refuses it. `409` already means + "wrong node" here, so a third status or a distinct detail string is needed to keep the two apart. +- **Whether it can be overridden.** An operator draining the last node on purpose, to stop all leader + work before a maintenance window, is a real request. That is what ADR 0056's deferred `force` flag was + for, so this and that flag should be decided together. +- **The cost of the read.** This adds a DB round trip to a path #1494 already bounds at the fence + timeout. It runs before the coordinator is touched and before the leadership lock, so it does not + extend the critical section -- but an unreachable store then turns the refusal into a `503`, which is + the correct answer and should be written down rather than discovered. + +--- + ## 1513. the harness server fixture budgets 10 seconds for the whole engine bring-up and calls the expiry a lost port > 🔢 **Filed 2026-09-09 -- not started. Scored at filing.** Value **4/10** · Difficulty **3/10** · _fill-in_. The `server` fixture in `tests/test_harness_scenarios.py` gives the ENTIRE engine bring-up 10 seconds of wall clock, then reports the expiry as a port-bind race that its own job log refutes. Measured 2026-09-06T22:17:30Z to 2026-09-09T15:23:59Z: **1 of 600** `test (windows-2025, py3.14)` job executions carried the exact signature, **2 of 600** counting the sibling fixture. **Verdict: wall-clock assertion, not a product defect** -- the engine came up cleanly on all four attempts of the failing job. diff --git a/docs/CLUSTERING.md b/docs/CLUSTERING.md index d91525932..f36597b4c 100644 --- a/docs/CLUSTERING.md +++ b/docs/CLUSTERING.md @@ -297,6 +297,10 @@ fixed node — so a failover is transparent to senders (modulo a reconnect): > on **Linux/containerized** deployments, which it does **not** cover — use the external floating VIP / LB > described here, which stays the **cross-platform** default and the recommended posture for the strictest > split-brain guarantee. +> +> **What HAS shipped from that ADR is only the control plane** — `POST /cluster/stepdown` (below), which +> moves *leadership*. It moves no address: with the external VIP / LB the address follows on its own, +> because the health check stops passing on the node that just released the lease. - **MLLP / TCP inbound (per listener).** Use a VIP per inbound port whose health check is a **TCP connect to that port**. Because only the **primary** binds the port (the active-passive graph gating), @@ -314,8 +318,9 @@ fixed node — so a failover is transparent to senders (modulo a reconnect): There is a promotion window, as in Rhapsody (minutes-class) — quantify it from the Workstream-D failover benchmark, don't assume zero-downtime: -- **Clean stop** (graceful shutdown / planned switchover): the leaving primary **expires its lease**, so a - standby acquires on its next heartbeat — failover is prompt (≈ one `heartbeat_seconds`). +- **Clean stop** (graceful shutdown): the leaving primary **expires its lease**, so a standby acquires on + its next heartbeat — failover is prompt (≈ one `heartbeat_seconds`). A **planned switchover** that + leaves the node running takes the same path, without the shutdown — see `POST /cluster/stepdown` below. - **Crash / partition**: the primary's lease **ages out**, so a standby acquires after up to `leader_lease_ttl_seconds`. A partitioned old primary **self-fences** within `leader_fence_timeout_seconds` (< the TTL), so it stops *reporting itself* leader before the standby @@ -328,6 +333,115 @@ benchmark, don't assume zero-downtime: order survives). At-least-once delivery + idempotent re-runs mean a row interrupted mid-delivery is re-delivered after its lease expires (so downstream connections must stay idempotent). +### Planned failover — `POST /cluster/stepdown` + +Ask the current primary to hand over on purpose, before you patch or reboot it, instead of pulling the +service out from under a live feed. The node **releases its leadership lease and keeps running**, demoted +to standby: a standby acquires the expired lease on its next heartbeat and promotes its graph, and the +node you drained stays up, heartbeating, ready to take leadership back later. + +``` +POST /cluster/stepdown # body: {} — there are no options +{ "node_id": "node-a:4812:1f9c2a7b", "was_leader": true, "released_at": 1758000000.5 } +``` + +- **Permission:** `cluster:control`, a dedicated capability held by **Administrator only** and never + assignable to a custom role. Behind `require_step_up`, so the caller also passes the per-actor + admin-write pacing floor, the TOTP MFA gate and the credential-recency window. +- **`was_leader` is what the release returned**, not a reading taken before it. A fence or a lost-lease + tick can move leadership in between, so a "was this node the leader?" check made first could report a + failover that released nothing. The same returned value is what the audit row records. +- **Statuses:** `400` when the deployment is **not clustered** — `[cluster]` disabled, or a store with no + cluster coordinator — so there is no lease to release; `409` when this node is not the leader — resolve + the leader from `GET /cluster/nodes` and call it there; `403` on a missing permission, a stale step-up + or an unsatisfied second factor; `503` when the engine is not started, or when the drain could not be + achieved (see the two `503` bullets below). +- **`400` does not mean "one node".** The gate reads whether clustering is *enabled*, not how many nodes + are live, so a clustered install that happens to be running one node accepts the call and goes + leaderless for the pause. Keying the refusal on whether a promotable sibling actually exists is + [BACKLOG #1509](BACKLOG.md); until then, read `GET /cluster/nodes` first. +- **A `503` reading `lock-timeout` means nothing happened at all.** The node's leadership lock was still + held when `leader_fence_timeout_seconds` ran out, so no lease row was read or written and nothing was + demoted. This one says nothing about who leads: the endpoint takes no leader check before the + release, so it can come back from a node that leads nothing — which is why it does not tell you + leadership is where you left it either. Do not start maintenance. Retry, and if it repeats, look at + the store connection. +- **A `503` reading `release-unconfirmed` means the node HAS already stood down — and the outcome is + genuinely unknown.** It has cleared its leadership flag, and this call stopped it claiming for two + `heartbeat_seconds`. What it could not confirm is whether the write expiring its lease row + committed, because a lost response to a committed `UPDATE` is indistinguishable here from an + `UPDATE` that never ran. **It does not tell you a teardown just started**: a retry of an owed write + finds the node already demoted, and the demotion edge fires only on the call that demotes it. + - **The node is NOT quiescent when this `503` arrives, and no status code will tell you it is.** The + demotion edge only wakes the graph supervisor; the teardown itself runs on that other task + afterwards. + - **The listeners stop early in that teardown, not at the end of it.** The source stop is the last of + the three BOUNDED demote phases — each takes its own share of the demotion budget, so the shares + are bounded and their sum is not the budget — and MLLP, TCP, HTTP and X12 each close their accept + socket in the synchronous prologue of their own `stop()`, so they stop taking new connections + before the unbounded phases (connector close, executor shutdown, sandbox close) are reached at + all. **That buys less than it sounds like.** A source that overruns its share is abandoned rather + than cancelled; DICOM releases its port inside exactly the call that gets abandoned, so a DICOM + listener can still hold its port; established connections drain in the background; and a message + already inside a handler still finishes its commit and its ACK, which count-and-log requires. + Confirm quiescence with `GET /cluster/nodes` plus the connection view before you touch the node. + - **If it committed**, a standby acquires on its next heartbeat and the failover is proceeding + normally, whatever the error page says. + - **If it did not**, the lease is still live and still owned by a node that has given up leadership, + so on a first deployment nothing carries the feeds until that node renews itself back in when its + pause ends — a partitioned pool during a stepdown is the way into that window. + - **A retry re-sends the write, and answers `409` for as long as this node is not the leader.** The + first call cleared this node's in-memory leader flag before it wrote, so every later call reports + `was_leader=false`, which the endpoint turns into `409`. That holds whatever the lease row says: + the row is not what decides the status code here. + - **Only a maintenance tick can make this node leader again, and retrying prevents one.** The flag is + set in exactly one place, when a tick's claim succeeds. A tick cannot claim while the stepdown + pause holds — it returns not-held at the pause gate before touching the database — and **each retry + that re-sends an owed write re-arms that pause for another two `heartbeat_seconds`**. So retrying + promptly holds this node in `409` indefinitely, by never letting a tick through. **The remedy is to + wait, not to retry.** + - **Once the pause lapses, the next tick settles it.** If the lease row still names the drained node, + the renew arm — `owner = me`, which carries no expiry test — matches, the node becomes leader + again, and a stepdown issued *after that* answers `200`. If a standby acquired instead, the row + names the standby and its lease is live, so the renew arm cannot match and the take-over arm needs + an expiry that has not passed; the drained node stays a follower and `409` is permanent. + **That `409` is the failover having worked, not a wrong-node answer.** Do not take the generic + `409` remedy here and step down whoever `GET /cluster/nodes` now names as leader: that is the + healthy successor, and draining it undoes the failover you just achieved. + - Either way, read `GET /cluster/nodes` and confirm `lease_owner` has moved. That, not the status + code, is what tells you it is safe to start maintenance. +- **Audited** as `cluster_stepdown` in the hash-chained audit log, with the acting user and + `{node_id, was_leader, released_at}` — cluster metadata only, never message content. **Every call the + handler completes is audited under that name, the `409` included**, so count drains by `was_leader` + rather than by the action name. A `409` writes a row reading `was_leader: false, released_at: null`, + and that row IS the refusal — which is why the `409` needs no separate denied row. The refusals the + handler reaches before it can return (`400`, both `503`s) write `cluster_stepdown_denied` instead, + carrying the reason — `not-clustered`, `lock-timeout` or `release-unconfirmed` — so the two `503`s + never read as one condition. That denied row is best-effort: a `release-unconfirmed` comes from a + store that has just failed, so the endpoint keeps the `503` and its remedy rather than losing both to + an audit write that could not land either way. +- **Who leads next is not reported.** At the instant of release no standby has acquired yet, so poll + `GET /cluster/nodes` and watch `lease_owner` move rather than expecting the call to name a successor. + +**In-flight work is not drained first.** Stepdown releases leadership; it does not quiesce the graph. +Rows already claimed on the old primary are recovered by the new one through the ordinary lease/reclaim +path, so plan the switchover the same way you plan a restart. + +**The drained node stands down briefly before it contends again.** For two `heartbeat_seconds` after a +stepdown it declines to claim or renew, so a sibling wins the expired lease rather than the node you +just drained renewing itself straight back. On a cluster with no other promotable node that window is +leaderless, which is the honest consequence of asking the only eligible node to step down. + +**Two known limits, so you can plan around them rather than discover them.** A sibling whose +`acquire_delay_seconds` is longer than two heartbeats is still handicapped out when the pause ends, and +the node you drained then reclaims its own lease ([BACKLOG #1507](BACKLOG.md)). And a node that has +already **self-fenced** cannot be drained at all: it holds no leadership to release, so the call answers +`409` while `GET /cluster/nodes` still shows it as the lease owner until the lease ages out +([BACKLOG #1508](BACKLOG.md)). In that state the node has given up leadership, but do not read that as +quiet: fencing sets a flag and wakes the graph teardown, which then runs on another task, so expect the +same brief overlap a crash failover gets (above). Wait out `leader_lease_ttl_seconds` rather than +retrying the stepdown. + ### Tune the lease timings to your network The defaults (`heartbeat_seconds=10`, `leader_fence_timeout_seconds=20`, `leader_lease_ttl_seconds=30`) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index ec91872e0..2b72ef020 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -18,7 +18,7 @@ with secure defaults, and AD-group→role mapping is automatic. ## Enforcement model Authentication is **required** for the running service. The engine `serve` command always attaches an -auth layer (`[security] require_sign_in = true` by default). Of the **108** engine route objects, **90 demand a +auth layer (`[security] require_sign_in = true` by default). Of the **109** engine route objects, **91 demand a specific permission** and 18 do not — 3 are deliberately unauthenticated (`GET /auth/providers`, an unbounded capability advertisement that carries no account state and charges **no** limiter; `POST /auth/login` and `POST /auth/negotiate`, bounded by the per-IP **and** global login sliding @@ -245,7 +245,7 @@ apply. What each **adds** over plain `require()`: | `require` | 43 | nothing — the ladder itself | | `require_paced` | 16 | per-actor anti-automation pacing on **non-GET** requests (`allow_admin_write`), 429 + `Retry-After: 1` | | `require_phi_read` | 7 | the ADR 0092 PHI-read hop refusal (`enforce_phi_read_hop`) **before** any identity work, then the per-actor PHI-read budget, 429 + `Retry-After: 10` | -| `require_step_up` | 27 | the same non-GET pacing, then the **MFA gate** (403 + `X-MFA-Required: 1`), the **new-client-IP** signal, and the credential-recency window (403 + `X-Step-Up-Required: 1`) | +| `require_step_up` | 28 | the same non-GET pacing, then the **MFA gate** (403 + `X-MFA-Required: 1`), the **new-client-IP** signal, and the credential-recency window (403 + `X-Step-Up-Required: 1`) | | `require_step_up_action` | 4 | the same non-GET pacing (BACKLOG #1148), the **MFA gate**, then a **single-use, action-bound** step-up grant minted only by `POST /me/reauth` (403 + `X-Step-Up-Action: `). Promoting a route here no longer drops the pacing floor | | `require_reauth_only_action` | 4 | password step-up **without** the MFA gate — deadlock avoidance on the MFA-enrollment lanes, and on session terminate (ASVS 7.5.2), where the grant is action-bound so a login-seeded window does not unlock it. `require_reauth_only` still exists and still backs the `/ui` twin, but BACKLOG #1149 moved the last JSON route off it, so it no longer appears in this walk | | `require_service_cert` | 1 | cert-only authentication (a bearer token gets 401), and a **PHI fence** that raises at *app construction* if asked to gate `messages:view_summary` / `messages:view_raw` | @@ -254,11 +254,11 @@ apply. What each **adds** over plain `require()`: validates the handshake `Origin` against `[api].ws_allowed_origins` **before** `accept()`, then the bearer token, the must-change lockout and the permission. -### Permission catalogue (28) +### Permission catalogue (29) The catalogue is `Permission` in [`auth/permissions.py`](../messagefoundry/auth/permissions.py); the enum value **is** the wire/storage string. "Routes" counts engine route objects gated on that permission -under `create_app()` (they sum to 92, not 90, because BOTH `/messages/export` routes require two). +under `create_app()` (they sum to 93, not 91, because BOTH `/messages/export` routes require two). | Constant | Permission | PHI | Routes | Gates | |---|---|---|:--:|---| @@ -275,6 +275,7 @@ under `create_app()` (they sum to 92, not 90, because BOTH `/messages/export` ro | `CONNECTIONS_CONTROL` | `connections:control` | | 3 | `POST /connections/{name}/start`, `/stop`, `/restart` | | `CONNECTIONS_TEST` | `connections:test` | | 2 | `POST /connections/{name}/test`, `/test-credential` | | `DR_OPERATE` | `dr:operate` | | 2 | `POST /dr/activate`, `/dr/release` (ADR 0048). Never assignable to a custom role | +| `CLUSTER_CONTROL` | `cluster:control` | | 1 | `POST /cluster/stepdown` (ADR 0056) — a planned active-passive failover: the leader releases its lease and a standby promotes. A dedicated capability, not a reuse of `monitoring:read` (a read) or `connections:control` (one connection). Never assignable to a custom role | | `CONFIG_DEPLOY` | `config:deploy` | | 2 | `POST /config/reload` **and** `POST /connections/{name}/flag` | | `CONFIG_VALIDATE` | `config:validate` | | 0 | no endpoint yet (see the note below) | | `CODE_EDIT` | `code:edit` | | 0 | no endpoint yet | @@ -304,7 +305,7 @@ inheritance — where a permission came from is invisible downstream. | Role | Count | Permissions | |---|:--:|---| -| **Administrator** | 28 | **every permission** — literally `frozenset(Permission)`, so a newly added permission is granted to it automatically | +| **Administrator** | 29 | **every permission** — literally `frozenset(Permission)`, so a newly added permission is granted to it automatically | | **Operator** | 16 | `monitoring:read`, `monitoring:diagnose`, `messages:read`, `messages:view_summary`, `messages:view_raw`, `messages:replay`, `messages:resend`, `messages:edit`, `messages:export`, `messages:purge`, `connections:control`, `connections:test`, `logs:view`, `files:upload`, `files:browse`, `files:delete` | | **Deployment** | 4 | `monitoring:read`, `config:deploy`, `config:validate`, `connections:test` | | **Coding** | 4 | `monitoring:read`, `code:edit`, `config:validate`, `ai:assist` | @@ -326,11 +327,12 @@ permission at all, so every gated property comes back `null` for them. The custom-role builder **is built** and is an *additive overlay* on the six built-ins, not a replacement: -- A custom role is a named **subset of the existing 28-permission catalogue** — it can never define a +- A custom role is a named **subset of the existing 29-permission catalogue** — it can never define a new permission kind. - Its id must carry the `custom:` prefix (`CUSTOM_ROLE_ID_PREFIX`), so it can never collide with a built-in role value or be mis-routed to the built-in resolver. -- It may **never** grant `users:manage`, `approvals:approve`, `dr:operate` or `files:access_any` +- It may **never** grant `users:manage`, `approvals:approve`, `dr:operate`, `cluster:control` or + `files:access_any` (`CUSTOM_ROLE_FORBIDDEN_PERMISSIONS`) — the escalation primitives stay admin-only. - An empty set or an unknown permission string is rejected on write (`CustomRoleError`); a malformed/hand-edited persisted `roles.permissions` row decodes **defensively to the empty set**, and @@ -352,12 +354,12 @@ Managed at `GET /roles/custom` (`users:read`) and `POST` / `PUT` / `DELETE /role ### Route → permission map (engine API) -**Counting basis.** `create_app()` with no arguments builds **108 route objects** — 67 declared in -[`api/app.py`](../messagefoundry/api/app.py) (66 HTTP + 1 WebSocket) and 38 declared in +**Counting basis.** `create_app()` with no arguments builds **109 route objects** — 71 declared in +[`api/app.py`](../messagefoundry/api/app.py) (70 HTTP + 1 WebSocket) and 38 declared in [`api/auth_routes.py`](../messagefoundry/api/auth_routes.py). No other module in `api/` declares routes -and there is no `include_router` anywhere. `create_app(expose_docs=True)` yields 112 (`/openapi.json`, -`/docs`, `/docs/oauth2-redirect`, `/redoc`; off by default) and `create_app(serve_ui=True)` yields 201 -(108 + the 97 console routes + the `/ui/static` mount). Of the 108: **90 are permission-gated**, 18 are +and there is no `include_router` anywhere. `create_app(expose_docs=True)` yields 113 (`/openapi.json`, +`/docs`, `/docs/oauth2-redirect`, `/redoc`; off by default) and `create_app(serve_ui=True)` yields 210 +(109 + the 100 console routes + the `/ui/static` mount). Of the 109: **91 are permission-gated**, 18 are not. Every one is listed below — none is collapsed away. #### Functions requiring no authorization @@ -474,6 +476,7 @@ tuple: they act only on the caller's own account. | `GET` | `/approvals` | `approvals:approve` | `require` | | `POST` | `/approvals/{approval_id}/approve` | `approvals:approve` | `require_paced` — the requester can never approve their own request | | `POST` | `/approvals/{approval_id}/reject` | `approvals:approve` | `require_paced` | +| `POST` | `/cluster/stepdown` | `cluster:control` | `require_step_up` | | `POST` | `/dr/activate` | `dr:operate` | `require_paced` | | `POST` | `/dr/release` | `dr:operate` | `require_paced` | @@ -563,7 +566,7 @@ tuple: they act only on the caller's own account. | `GET` | `/logs/tail` | `logs:view` | `require_phi_read` | best-effort-redacted; writes a `logs_view` audit row | | `POST` | `/ai/chat` | `ai:assist` | `require` | **not** paced; bounded by the central AI policy | -**PHI-egress route set.** Of the 108 route objects a default `create_app()` serves, **fifteen** can put +**PHI-egress route set.** Of the 109 route objects a default `create_app()` serves, **fifteen** can put PHI on the wire: the twelve message/search rows above marked PHI (`/messages`, `/messages/{id}`, `/responses`, `/outbound`, `/attachments/{id}`, `/messages/search`, `/messages/export`, `/search/layered`, the three `/search/presets` rows, `/dead-letters`), plus @@ -585,13 +588,13 @@ rather than shown a body its permission set does not authorize. #### The `/ui` console plane (`serve_ui=True`) -When the console is served, the `/ui` plane adds **95 routes + one `/ui/static` mount** (federation off, +When the console is served, the `/ui` plane adds **100 routes + one `/ui/static` mount** (federation off, the default — the two `/ui/oidc/*` routes are registered only when `[auth].oidc_enabled`). They are -functions too, and they gate on the **same 28-permission catalogue** through parallel wrappers — +functions too, and they gate on the **same 29-permission catalogue** through parallel wrappers — `require_ui`, `require_ui_step_up`, `require_ui_reauth_only`, `require_ui_step_up_action`, `require_ui_reauth_only_action` — but authenticate by the `/ui`-confined `SameSite=Strict` **session cookie** rather than a bearer token, and refuse cross-site state changes on `Sec-Fetch-Site`/`Origin`. -**Route → permission map (`/ui` plane).** 87 of the 95 are gated; the 8 that are not are the +**Route → permission map (`/ui` plane).** 90 of the 100 carry a gate; the 10 that do not are the sign-in and re-auth entry points, listed after the table. Where the console is served it is the *sole* operator UI, so ~20 of these have no JSON counterpart from which their authorization could be inferred — `POST /ui/connections/bulk-control`, `POST /ui/connections/purge-bulk`, the @@ -717,7 +720,7 @@ re-implement the gate's checks by hand, in the gate's order (`must_change` befor factor), and neither is reachable without a live session cookie — "unauthenticated" here means "carries no `Depends` gate", not "open". -The `/ui/static` **mount** is the ninth unauthenticated served path, and it is not a route at all: +The `/ui/static` **mount** is the eleventh unauthenticated served path, and it is not a route at all: `StaticFiles` serves it with **no gate whatsoever** — no session, no permission, not even the 503 fail-closed arm that `GET /ui` returns when no `AuthService` is attached. It carries only the console's own versioned CSS/JS — no PHI, no account state, no engine data — and it is still subject to the diff --git a/docs/adr/0056-engine-managed-vip-failover.md b/docs/adr/0056-engine-managed-vip-failover.md index a74d31186..828a5b595 100644 --- a/docs/adr/0056-engine-managed-vip-failover.md +++ b/docs/adr/0056-engine-managed-vip-failover.md @@ -1,9 +1,43 @@ # ADR 0056 — Engine-managed virtual IP (VIP) failover -- **Status:** Proposed (2026-06-27) — drafted on the owner's go. **No code**; this is a **design** - decision to record the seam, the correctness argument, the privilege cost, and the operator surface - before any build. There is **no engine-managed-VIP code today** — every reference below to a bind/ - release/`/cluster/stepdown`/VIP-owner field is **proposed**, not built. +- **Status:** Partly accepted (2026-06-27; control plane built 2026-09-09, BACKLOG #1494). Read the two + halves separately, because they are at different build states and conflating them is how a reader ends + up designing for an address the engine does not move: + - **BUILT — the planned-failover control plane.** `POST /cluster/stepdown`, the `CLUSTER_CONTROL` + (`cluster:control`) permission, and the coordinator's public `step_down_leadership()` seam — the + three subsections §"Proposed endpoint", §"Coordinator seam" and §"RBAC & audit" below, **less at + least** the `force` flag and `new_leader_eligible`, which that section defers on its own terms. + Read "at least" literally: this is a pointer to what shipped, not a closed enumeration of every + sentence in those subsections, and where a line there disagrees with the code the code is current. + Two known divergences, both introduced by the build and recorded rather than left for a reader to + trip over: `503` also covers two conditions this design did not name — a lease-expiring write whose + outcome the node cannot confirm, and a leadership lock still held at the fence timeout — and the + `400` gate keys on whether clustering is ENABLED rather than on whether a promotable sibling exists + (BACKLOG #1509). + - **STALE — §"Confirm / step-up posture (console)"**, which sits INSIDE §"Control API — planned + failover" and is therefore not covered by the bullet above. **There is no cluster page and no + `client.stepdown_node`**, and the confirm dialog it specifies promises the operator that "the VIP + will move", which the paused-VIP bullet below denies. **Do not build from it**; the web console page + is BACKLOG #1495. Read the marker on the section itself for what is stale there and what is not — + the answer is not "all of it", and this bullet used to say it was. + - **PROPOSED AND PAUSED — the VIP mechanism itself.** The `[cluster.vip]` config block, bind/release, + the gratuitous ARP, the self-fence release path, `mefor-net-helper.exe`, and the `vip` field on + `GET /cluster/status`. **There is no engine-managed-VIP code today**; every reference below to a + bind/release or a VIP-owner field is proposed, not built. + + **The ruling and what backs it, recorded here because this page is the decision record.** On + **2026-09-09** the owner ruled the VIP mechanism **paused, pending a code-signing decision**: it + needs a `requireAdministrator` helper binary (`mefor-net-helper.exe`) and this repository has no + code-signing infrastructure to ship one with. **Read it at the standard it was given:** in session, + to the session that built the control plane, with **no git ref or other artifact anchoring it** — + these lines are the record, so a reader who needs it independently verified should ask the owner + rather than treat this page as the proof. Stated once here; the index row in + [`README.md`](README.md) and BACKLOG #1494 point at it rather than repeat it. + - **STALE — §"Console — High Availability page".** It targets the PySide6 desktop console + (`console/shell.py`, `console/status.py`, `console/connections.py`), which was retired. The operator + UI is the web console at `/ui`. The section is kept for its topology reasoning — the "no Viewing + toggle" argument and the read-mostly layout still hold — but its construction notes name files that + no longer exist. Do not build from them. - **ADR number:** first drafted as `0047`; that slot was reassigned on `origin/main` (to the cloud/k8s HA deployment-packaging ADR), so this was renumbered to `0056` — the next free number on `main`, which now carries through 0055. Parallel worktrees can race the number, so confirm `0056` is still free @@ -510,6 +544,25 @@ promotion; this API contract is unchanged by it. ### Confirm / step-up posture (console) +> **STALE — DO NOT BUILD FROM THIS SUBSECTION. There is no cluster page, and `client.stepdown_node` +> does not exist.** The operator UI is the web console at `/ui`, and the page is BACKLOG #1495. Step 2 +> below also has the dialog promise that "the VIP will move", which the engine does not do and is not +> going to do until the paused VIP mechanism is decided. +> +> **What is stale is the SEAT, not the machinery, and an earlier version of this marker got that +> wrong.** It asserted that every symbol it named belonged to the retired PySide6 desktop console. +> ("Named below" was also the wrong scope, and is corrected here: three of the four are named in step +> 3 below, but `AsyncRunner` is named further up, in the High Availability page section.) +> Three of the four are alive, REHOMED rather than retired: `_request` in +> `messagefoundry/apiclient/client.py` (ADR 0088 extracted the Qt-free engine client), `AsyncRunner` in +> `harness/_async.py`, and `poll_client` in `harness/_console_widgets.py` (the harness reuses view +> widgets moved out of the old console). Only `client.stepdown_node` is absent — which is the control +> showing the check discriminates rather than matching everything. +> +> **So do not discard steps 3 and 4 with the rest.** Step 3's rule outlives the console it was written +> for: carry the step-up / MFA challenge on the WRITING client, never on the read-only polling one, and +> run the call off the UI thread. So does step 4's: render the leaderless window honestly. + The failover button follows the established **privileged-write** pattern, not the read pattern: 1. **Enabled only when** `client.can("cluster:control")` **and** a live leader exists to step down; diff --git a/docs/adr/README.md b/docs/adr/README.md index 23ed890fd..ad2009a89 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -88,7 +88,7 @@ what is withheld and what you can request. | [0053](0053-free-threaded-multicore-engine.md) | Free-threaded (cp314t) multi-core engine as the committed unified-store scale path (**supersedes 0040**) — many real threads on **one** store / process / API port (vs sharding's fragmented K-DB store), the analog of Corepoint's one internally-multi-threaded engine; fits because the hot path is pure. **Phase 1 = a feasibility + scaling spike started now** (engine-path compiled-dep thread-safety — pydantic-core / cryptography / argon2-cffi / asyncpg / pyodbc; measured multi-core scaling on a concurrent-commit server DB; no invariant regression); **fallback = ADR 0037 sharding + cross-shard observability** on no-go. Reverses 0040's measure-first decline on the early-phase timing argument; **refines 0051** (brings free-threading forward of its enterprise-hardware gate; complements its durable-write levers — necessary-not-sufficient). SQLite stays single-writer; server-DB-first | **Commitment RETIRED — the cp314t path was measured and DECLINED.** BACKLOG #90 closed DECLINED 2026-07-09 (thread-hop fusion below the 10% bar) and #91 closed DECLINED 2026-07-20 (the engine is not CPU-bound: ~0.06–0.36 cores per shard). The committed scale path is this ADR's own documented fallback — [0037](0037-multi-process-sharding-l3.md) engine sharding over the [0063](0063-no-split-store-unified-store-for-sharding.md) unified store. Accepted 2026-06-29; kept as history, **not** as current direction. | | [0054](0054-low-allocation-builtins-hl7-parser.md) | Low-allocation built-ins HL7 parser (free-threading keystone, BACKLOG #88) — replace **python-hl7** as the tolerant-tier backing of the *existing* `Peek`/`Message` API with a parser over native **dict/list/str** (no per-node classes), as a behaviour-identical **drop-in**. WS3 measured the class-instance tree as the free-threading bottleneck: built-ins scale **6.44× multi-core + ~14× single-thread** vs python-hl7's 2.02× / 1× (hl7apy worse on both). MSH-eager / rest-lazy split; reads separators from MSH-1/MSH-2; preserves every `Peek`/`Message`/`SegmentGroup`/`parse_tree` method + the whole-value-no-component rule + the escape/XFORM semantics; **hl7apy `validate()` strict tier untouched**. Golden-corpus parity gate + Phase-1 python-hl7 fallback; unlocks [ADR 0053](0053-free-threaded-multicore-engine.md) and helps single-process + [ADR 0037](0037-multi-process-sharding-l3.md) sharding regardless | Accepted (2026-06-29; built + merged #655) | | [0055](0055-group-commit-durable-write.md) | Group-commit for the staged queue — the durable-write **ceiling-mover** (cut fsyncs/msg; ~7 commits/msg today). A committer coroutine coalesces N already-prepared mutations into one durable commit under the writer lock; group rollback rejects all members' futures → re-run (reuses the idempotent INFLIGHT-guarded crash-re-run). **Backend-dependent mechanism:** app-side committer on SQLite's single writer; on PG/SQL Server's concurrent pool, native `commit_delay` + concurrent submission (resolve the single-lock-vs-pool fact first — native GUC buys ~0 under single-writer serialization). `claim` poison-guard stays standalone; ACK waits on the durable ingress future; cache-publish only on member success. Build authorized now as a no-regret lever (ADR 0051 delayed-HW adjustment), proxy-measured on the 265KF (storage methodology verified); win is `synchronous=FULL`-dependent | Proposed (2026-06-29) | -| [0056](0056-engine-managed-vip-failover.md) | Engine-managed virtual IP (VIP) failover | Proposed (2026-06-27, design-only) | +| [0056](0056-engine-managed-vip-failover.md) | Engine-managed virtual IP (VIP) failover — optionally bind the VIP on leader promotion and release it on demotion/self-fence, so the address follows the leadership lease (one arbiter) instead of an external LB/VRRP health check. **Only the planned-failover control plane is built** (`POST /cluster/stepdown`, `CLUSTER_CONTROL`, `step_down_leadership()` on all three coordinators, step-up + MFA + audit; BACKLOG #1494). **The VIP mechanism itself is not built**: it needs a `requireAdministrator` helper binary and this repo has no code-signing infrastructure. The ADR's two console sections are each marked do-not-build-from — one targets the retired PySide6 console, the other a cluster page that does not exist (the web console page is BACKLOG #1495) | Partly accepted (2026-06-27; stepdown control plane built 2026-09-09 — VIP mechanism paused, ruling and its standard of evidence recorded in the ADR's own status block) | | [0057](0057-inline-step-a-fast-path.md) | Inline Step-A fast-path — collapse the routed stage for no-lookup, all-deliver, single-handler messages (B1) | Proposed (built, opt-in) | | [0058](0058-batch-claim-fifo-prefix.md) | Batch-claim the contiguous due head-prefix on the INGRESS/ROUTED FIFO claim path (B2, `fifo_claim_batch`) | Proposed (built, opt-in) | | [0059](0059-seq-only-fifo-ordering.md) | seq-only per-lane FIFO ordering (drop the `_fifo_created_at` write-time clamp; one-serial-writer-per-lane) | Proposed (built) | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index f193123ee..c5a3d7075 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -102,6 +102,8 @@ ClusterNode, ClusterNodeList, ClusterStatus, + ClusterStepdownRequest, + ClusterStepdownResult, ConfigProvenance, ConnectionEventInfo, ConnectionFlagRequest, @@ -299,7 +301,11 @@ from messagefoundry.pipeline import ConfigReloadDenied, Engine from messagefoundry.pipeline.alert_sinks import EmailTransport, notifier_from_settings from messagefoundry.pipeline.alerts import AlertSink, LoggingAlertSink -from messagefoundry.pipeline.cluster import build_coordinator +from messagefoundry.pipeline.cluster import ( + StepdownLockTimeout, + StepdownReleaseUnconfirmed, + build_coordinator, +) from messagefoundry.pipeline.connscale_shim import maybe_install_executor_shim from messagefoundry.pipeline.dr import DrActivationError from messagefoundry.pipeline.security_notify import security_notifier_from_settings @@ -5443,6 +5449,231 @@ async def cluster_nodes( lease_expires_at=lease_expires_at, ) + # --- cluster control plane: planned failover (ADR 0056 slice 1) ---------- + + @app.post("/cluster/stepdown", response_model=ClusterStepdownResult) + async def cluster_stepdown( + request: Request, + engine: Engine = Depends(_get_engine), + identity: Identity = Depends(require_step_up(Permission.CLUSTER_CONTROL)), + _body: ClusterStepdownRequest | None = Body(default=None), + ) -> ClusterStepdownResult: + """**Planned failover**: make this node release its leadership lease so a standby promotes + (maintenance drain, ADR 0056). The node keeps running and heartbeating, demoted to standby — + this is not a shutdown. + + Gated by the dedicated ``cluster:control`` permission (ADMINISTRATOR only, never assignable to + a custom role) behind ``require_step_up``, which is the composite the ADR's decision table asks + for: it charges the per-actor admin-write pacing floor, then the TOTP MFA gate, then the + new-client-IP signal and the credential-recency window. The three high-impact write neighbours + (``config:deploy``, ``messages:replay``, ``messages:purge``) sit behind the same wrapper. + + Statuses: ``400`` not clustered (refused BEFORE the coordinator is touched — there is no lease + and no standby); ``409`` this node is not the leader — the caller resolves the leader from + ``GET /cluster/nodes`` first, and see the note below on the one case where a ``409`` IS the + successful answer; ``403`` missing permission / step-up / MFA; ``503`` engine not started, + authentication not configured, or one of the two drain conditions below. + + **The ``503`` list above is "at least", not an enumeration, and the difference is load-bearing + for anyone reading a `503` off a real deployment.** ``RequestTimeoutMiddleware`` is registered + unconditionally on this app and answers ``503`` from OUTSIDE this handler at + ``DEFAULT_REQUEST_TIMEOUT_SECONDS``, with a body naming no route. Recorded on BACKLOG #1494; + the two below are the only ones this handler itself raises. + + **An absent audit row RULES OUT the drain conditions; it does not identify the timeout.** Both + drain arms call ``_denied`` before they raise, so a ``503`` carrying no ``cluster_stepdown_denied`` + row is neither of them. The converse does not follow, and an earlier revision of this docstring + asserted it. At least three other causes share that same empty trail: the ``engine not started`` + and ``authentication is not configured`` ``503``s named above are raised by the DEPENDENCIES + (``_get_engine``, ``require_step_up``), so this body never runs and neither writes a row of + either kind; and ``_denied`` is deliberately best-effort, so a store too sick to take the row + leaves a drain refusal looking exactly like one that never reached the handler. Diagnose from + the response body, which differs on every one of them, and read the audit trail as + corroboration rather than as the discriminator. + + **The two ``503``s are different answers and must not share a sentence.** An earlier build gave + both raise sites one body ("could not release leadership; it is still the leader") and one audit + reason, which was false of each in a different way. + + * ``StepdownLockTimeout`` → reason ``lock-timeout``. The coordinator's leadership lock was still + held at the fence timeout, so **nothing ran**: no lease row was read or written and nothing + was demoted. It fires before any leadership is consulted, so it can come back from a node that + leads nothing — this branch asserts nothing about who the leader is. + * ``StepdownReleaseUnconfirmed`` → reason ``release-unconfirmed``. This node **has** demoted + itself and **this call armed its claim pause** — both hold on every branch that reaches the + raise, because the pause is armed on ``self._is_leader or owed`` and the write is only + attempted under the same condition. What it could not confirm is whether the write expiring + its lease row committed. A lost response to a committed ``UPDATE`` is indistinguishable from + an ``UPDATE`` that never ran, so the body is conditional: saying "it is still the leader" is + right on one branch and, on the other, sends an operator to fix a cluster that is already + failing over correctly. + + **What this branch must NOT say is that a teardown just started.** The demotion edge fires + under ``if was_leader`` — in ``DbCoordinator.step_down_leadership`` and identically in its + SQL Server twin, the only two that reach this raise — which a RETRY has already cleared, so + a repeat refusal signals nothing new and an earlier body claiming otherwise was false on + exactly that branch. The body therefore describes the demotion teardown as a mechanism — it + runs on the graph supervisor, not in this call — rather than asserting one began here. + + Both map to ``503`` because both are environment conditions, which is what the neighbouring DR + endpoints and the ADR's own contract give that status. + + **A ``409`` after a ``release-unconfirmed`` ``503`` is the retry SUCCEEDING**, not a wrong-node + answer — *while the claim pause holds*. The coordinator re-sends the owed write on the next + stepdown; by then this node has already demoted, so it truthfully reports ``was_leader=false``. + That pause is two ``heartbeat_seconds``, 20s at the shipped default, and it is the whole scope + of that sentence. + + **Retrying promptly is the slow path, and can be an indefinite one.** The pause is armed on + ``self._is_leader or owed``, so a retry that re-sends an owed write RE-ARMS it for another two + ``heartbeat_seconds``. Nothing promotes this node except ``_maintain_leadership`` setting the + flag when its claim succeeds, and that claim returns not-held at the pause gate before it + touches the database. So an operator who retries faster than the pause expires never lets a + tick through and holds themselves in ``409``. The remedy for a ``release-unconfirmed`` ``503`` + is to WAIT and read ``GET /cluster/nodes``, not to retry in a loop. + + **Past the pause the answer is ``200`` OR ``409``, decided by who the lease row names by then + — an earlier revision promised ``200`` flatly and was false on one of the two branches.** The + claim statement has exactly two arms: renew, gated on this node still OWNING the row + (``WHERE leader_lease.owner = $2`` on Postgres, ``t.owner = ?`` in the SQL Server ``MERGE``), + which carries no expiry term; and take-over, which requires the lease to have expired. Nothing + below turns on which backend it is — both spell the same two arms. If the row still names + this node when the pause ends — the release write never committed, or it committed and no + standby took the lease — the renew arm matches on the next tick, this node leads again, and a + retry answers ``200``. If a standby acquired instead, the row names the standby and its lease + is live, so NEITHER arm matches, ``_claim_or_renew_lease`` reports not-held, this node stays a + follower, and a retry answers ``409``. **That ``409`` is the failover having worked.** Do not + take the ``409``'s generic remedy here and step down whichever node ``GET /cluster/nodes`` now + names as leader: that is the healthy successor, and draining it undoes the failover. Either + way the confirmation is the lease moving in ``GET /cluster/nodes``, not the status code. + + **Which refusals get their own audit row.** Only the ones this body reaches. ``require_step_up`` + already records the permission / step-up / MFA 403s as ``auth.permission_denied`` and the body + never runs on those, so a second denied row there would double-count. The ``409`` needs none + either — the ``cluster_stepdown`` row written from the coordinator's return already reads + ``was_leader: false``, which IS the refusal. That leaves the not-clustered ``400`` and the two + ``503``s, which nothing else would record. + """ + c = engine.coordinator + + async def _denied(reason: str, exc: Exception | None = None) -> None: + """One shape for every refusal this handler records, so the three cannot drift apart field + by field. The DISCRIMINATOR stays at the call site: each refusal supplies its own reason + and raises its own body, because that is exactly the distinction a shared arm lost once. + + **The audit write is best-effort here, and the refusal is not, because on one arm THE + STORE IS WHAT FAILED.** ``release-unconfirmed`` is raised only when the lease-expiring + write did not return, and on Postgres ``build_coordinator`` hands the coordinator the + store's own pool — the same pool ``record_audit`` borrows — so that arm issues its audit + round trip against the connection whose failure produced the refusal. Letting it raise + would unwind past the ``raise HTTPException(503)`` below and hand the caller the + catch-all's bare ``500 internal error``: no status the caller can act on, no reason, and + none of the remedy text, on a node that has already cleared its leadership flag and may + still own a live lease no standby can take. The row is lost either way — a store that + cannot take it on this path cannot take it on that one — so the guard trades nothing for + the composed answer. A failed write is logged with its reason rather than dropped + silently, and ``CancelledError`` is not an ``Exception``, so a request deadline still + unwinds this handler as before.""" + detail = {"node_id": c.node_id, "reason": reason} + if exc is not None: + detail["error"] = safe_exc(exc) + try: + await engine.store.record_audit( + "cluster_stepdown_denied", + actor=identity.username, + channel_id=None, + detail=json.dumps(detail), + client=client_ip(request), + ) + except Exception as audit_exc: + _log.warning( + "cluster: node %s refused a stepdown (%s) but could not record the " + "cluster_stepdown_denied audit row: %s", + c.node_id, + reason, + safe_exc(audit_exc), + ) + + if not c.is_clustered(): + # Single-node: no lease to release, no standby to take over. Gated here, before the + # coordinator, so the answer never depends on a NullCoordinator's no-op. + await _denied("not-clustered") + raise HTTPException( + 400, f"node {c.node_id} is not clustered; there is no lease to release" + ) + # Deliberately NO is_leader() pre-read. The release itself reports whether this node held + # leadership, and that returned value is the only thing audited or reported: a fence or a + # lost-lease tick between a pre-read and the release would otherwise record was_leader=true for + # an action that released nothing (ADR 0056, "Audit the return value, not a pre-read"). + try: + was_leader, released_at = await c.step_down_leadership() + except StepdownLockTimeout as exc: + # NOTHING RAN. No lease row was read or written, nothing was demoted, and — because the + # handler takes no is_leader() pre-read — this node may lead nothing at all. So this arm + # asserts no leadership: it names the lock and the bound, which is all that is known. + await _denied("lock-timeout", exc) + raise HTTPException( + 503, + f"node {c.node_id} could not start a stepdown: its leadership lock was still held " + "when the fence timeout ran out, so nothing was read, written or demoted and this " + "call changed nothing. Retry, and if it repeats, look at the store connection.", + ) from exc + except StepdownReleaseUnconfirmed as exc: + # THE NODE HAS ALREADY STOOD DOWN; what is unknown is the write. Record that, not a + # stepdown — but do not claim the certainty the old body did ("it is still the leader"), + # because a lost response to a committed UPDATE reads identically here to an UPDATE that + # never ran, and on the committed branch a standby is promoting while this is read. + # + # NOR does this body say the node "stopped serving", which an earlier one did. The + # demotion edge is Engine._on_demote_edge, whose whole body is _graph_wake.set() and whose + # docstring says it deliberately does NOT set the runner's _stop. The teardown runs later, + # on the graph supervisor task, via Engine._stop_graph. An operator told "stopped serving" + # would begin maintenance on a node still bound to its port and still ACKing. + # + # NOR does it say the listeners keep accepting until teardown COMPLETES, which the + # replacement body said and which the ordering refutes. RegistryRunner._teardown_body runs + # the source stop LAST of the three BOUNDED demote phases (_quiesce_workers_demote, + # _quiesce_dispatchers_demote, _stop_sources_demote — each taking its own share of the + # budget, 0.7/0.7/0.3, so the SHARES are bounded and their sum is not the budget), and only + # then reaches the unbounded connector-close, executor-shutdown and sandbox-close phases. + # MLLP, TCP, HTTP and X12 each call server.close() in their stop()'s SYNCHRONOUS prologue, + # so accept stops on the first loop pass of that phase — earlier than "until it completes", + # not later. What survives is weaker and is what the body now says: the phase is bounded, + # an overrunning source is ABANDONED rather than cancelled, DICOM releases its port inside + # the call that gets abandoned, and established connections drain afterwards. So the node + # is still not quiescent — for different reasons than the sentence gave. + # + # And the body must not claim a teardown started ON THIS CALL: _fire_on_demote runs only + # under `if was_leader`, which a retry of an owed write has already cleared. + await _denied("release-unconfirmed", exc) + raise HTTPException( + 503, + f"node {c.node_id} has cleared its leadership flag, but could not confirm that its " + "leadership lease was expired; it may still own a live lease no standby can take. " + "Demotion tears the graph down on another task, not in this call: the listener stop " + "runs under its own bounded share of the demotion budget, ahead of the unbounded " + "phases (connector close, executor shutdown, sandbox close), and a listener that " + "overruns that share is abandoned rather than cancelled while its established " + "connections drain in the background. Re-run the stepdown — a retry re-sends that " + "write — then confirm the " + "lease has moved in GET /cluster/nodes AND that this node's connections are quiet " + "before starting maintenance. This status code never means the node is quiescent.", + ) from exc + result = ClusterStepdownResult( + node_id=c.node_id, was_leader=was_leader, released_at=released_at + ) + # The audit detail IS the response body, so the two cannot drift apart field by field. + await engine.store.record_audit( + "cluster_stepdown", + actor=identity.username, + channel_id=None, + detail=json.dumps(result.model_dump()), + client=client_ip(request), + ) + if not was_leader: + raise HTTPException(409, f"node {c.node_id} is not the current leader") + return result + # --- third-tier DR standby (#61, ADR 0048) ------------------------------- @app.get("/dr/status", response_model=DrStatus) diff --git a/messagefoundry/api/models.py b/messagefoundry/api/models.py index 68a04503f..0e03c7c47 100644 --- a/messagefoundry/api/models.py +++ b/messagefoundry/api/models.py @@ -979,6 +979,34 @@ class ClusterNodeList(BaseModel): lease_expires_at: float | None +class ClusterStepdownRequest(RequestModel): + """The body of ``POST /cluster/stepdown`` (ADR 0056 slice 1) — deliberately EMPTY. + + ADR 0056 sketched a ``force`` flag and then deferred it: v1 ships a clean lease release only, and + ``409`` is the normative answer on a node that is not the leader. Because this is a + :class:`~messagefoundry.api.request_model.RequestModel`, a client that sends ``{"force": true}`` + anyway is refused with 422 rather than silently getting the un-forced behaviour it did not ask for. + The model exists (instead of no body at all) so that flag has one obvious place to land when it is + built, without changing the route's shape.""" + + +class ClusterStepdownResult(BaseModel): + """The result of a planned failover (ADR 0056 slice 1). ``node_id`` is the node the call was made + against. ``was_leader`` and ``released_at`` are what the coordinator's ``step_down_leadership()`` + RETURNED, never a prior ``is_leader()`` read: a fence or a lost-lease tick can flip leadership + between the read and the release, so a pre-read could report a failover that released nothing. + ``released_at`` is the epoch-seconds instant this node was demoted, ``None`` when it held no + leadership. Cluster metadata only — no PHI. + + The successor is deliberately NOT reported. ADR 0056 left ``new_leader_eligible`` unresolved, and + deriving it here would be a guess: at the instant of release no standby has acquired yet, so the + honest answer is for the caller to re-poll ``GET /cluster/nodes`` and watch the lease move.""" + + node_id: str + was_leader: bool + released_at: float | None + + class DrStatus(BaseModel): """Third-tier DR standby posture (#61, ADR 0048). ``enabled`` = this deployment is a DR box at all (``[dr].enabled``); ``active`` = it is currently serving under the DR run-profile (the priority feeds diff --git a/messagefoundry/api/security.py b/messagefoundry/api/security.py index 16a54b347..c4200bd6d 100644 --- a/messagefoundry/api/security.py +++ b/messagefoundry/api/security.py @@ -118,6 +118,7 @@ Permission.CONNECTIONS_CONTROL, Permission.CONNECTIONS_TEST, Permission.DR_OPERATE, + Permission.CLUSTER_CONTROL, # ADR 0056: a planned failover moves the active-passive primary Permission.CONFIG_DEPLOY, Permission.CONFIG_VALIDATE, Permission.CODE_EDIT, diff --git a/messagefoundry/auth/permissions.py b/messagefoundry/auth/permissions.py index ebd71be7e..f71dc0a56 100644 --- a/messagefoundry/auth/permissions.py +++ b/messagefoundry/auth/permissions.py @@ -44,6 +44,12 @@ class Permission(str, Enum): # noqa: UP042 "connections:test" # probe a connection's reachability (POST /connections/{name}/test) ) DR_OPERATE = "dr:operate" # promote/release a third-tier DR standby (POST /dr/activate|release, ADR 0048) + # Planned active-passive failover: make the current leader release its leadership lease so a standby + # promotes (POST /cluster/stepdown, ADR 0056). A DEDICATED capability, not a reuse of monitoring:read + # (a read) or connections:control (one connection) — this moves the whole cluster's primary. Held by + # ADMINISTRATOR only at v1 and never assignable to a custom role, so "Administrator only" is enforced + # on every minting path rather than merely observed of the built-ins. + CLUSTER_CONTROL = "cluster:control" CONFIG_DEPLOY = "config:deploy" # endpoint lands in a later effort CONFIG_VALIDATE = "config:validate" # endpoint lands in a later effort CODE_EDIT = "code:edit" # endpoint lands in a later effort @@ -191,7 +197,8 @@ def permissions_for_roles(roles: Iterable[Role]) -> frozenset[Permission]: #: ``ADMINISTRATOR`` deliberately gates (ADR 0045 D1). ``USERS_MANAGE`` is the permission that mints #: roles (a custom role holding it could grant itself admin-equivalent power); ``APPROVALS_APPROVE`` is #: dual-control release; ``DR_OPERATE`` (ADR 0048) promotes/releases a whole third-tier DR standby box -#: (a site-failover-grade action); ``FILES_ACCESS_ANY`` (ASVS 8.2.2) is the override that defeats +#: (a site-failover-grade action); ``CLUSTER_CONTROL`` (ADR 0056) moves the active-passive primary, the +#: same failover grade one tier down; ``FILES_ACCESS_ANY`` (ASVS 8.2.2) is the override that defeats #: owner-only on every uploaded file, so leaving it mintable would let a custom role read, re-inject #: and delete every operator's uploaded PHI while docs/SECURITY.md says it is Administrator-only. #: All stay admin-only. @@ -200,6 +207,7 @@ def permissions_for_roles(roles: Iterable[Role]) -> frozenset[Permission]: Permission.USERS_MANAGE, Permission.APPROVALS_APPROVE, Permission.DR_OPERATE, + Permission.CLUSTER_CONTROL, Permission.FILES_ACCESS_ANY, } ) diff --git a/messagefoundry/pipeline/cluster.py b/messagefoundry/pipeline/cluster.py index e21a13eab..8507158ca 100644 --- a/messagefoundry/pipeline/cluster.py +++ b/messagefoundry/pipeline/cluster.py @@ -79,6 +79,9 @@ "ClusterMember", "NullCoordinator", "DbCoordinator", + "StepdownUnavailable", + "StepdownLockTimeout", + "StepdownReleaseUnconfirmed", "build_coordinator", "default_node_id", ] @@ -138,12 +141,138 @@ class ClusterMember: _DEMOTE_BUDGET_CEILING = 10.0 +class StepdownUnavailable(RuntimeError): + """Base class for the two ENVIRONMENT conditions that stop a planned failover (ADR 0056 slice 1). + Both are mapped to ``503`` by ``POST /cluster/stepdown``, the status the ADR's contract and the + neighbouring DR endpoints already give environment conditions. + + **Never raised directly — raise one of the two subclasses.** They differ in what the operator may + conclude, and an earlier build lost that difference by mapping both to one message and one audit + reason. :class:`StepdownLockTimeout` fires BEFORE anything is attempted, on whatever node was + addressed, leader or not. :class:`StepdownReleaseUnconfirmed` fires only AFTER this node has + demoted itself, and only about a write whose fate it cannot see. A sentence true of one is false + of the other, so this class carries no operator-facing wording of its own. + + **Why either exists rather than a best-effort success.** The release write is best-effort on + :meth:`stop`, where the node is leaving anyway and a lease that ages out costs nothing. It is not + best-effort on a stepdown, where an operator reads the answer and then starts maintenance. + """ + + +class StepdownLockTimeout(StepdownUnavailable): + """A stepdown could not take the coordinator's ``_leadership_lock`` inside the fence timeout, so + **nothing ran**: no lease row was read or written, and no in-memory state changed. + + **Says nothing about who leads.** The endpoint takes no ``is_leader()`` pre-read, so this can fire + on a node that never held leadership — a caller who addressed the wrong node, on a box whose lock + happens to be busy. Any wording here that asserts "it is still the leader" would be a guess. + + **Do not name the holder either.** The lock is taken in exactly two places, ``_maintain_leadership`` + and :meth:`ClusterCoordinator.step_down_leadership`, so a concurrent stepdown holds it as readily + as a maintenance tick does. What is known is the lock and the bound, and that is what this says. + """ + + +class StepdownReleaseUnconfirmed(StepdownUnavailable): + """A stepdown demoted this node, then could not confirm that the write expiring its lease row + landed. **The outcome is genuinely unknown**: a lost response to a committed ``UPDATE`` is + indistinguishable here from an ``UPDATE`` that never ran. + + **So the wording is conditional, and that is the whole point of the class.** On the did-not-commit + branch this node still owns a live lease no standby can take, and it renews itself back in when the + claim pause ends. On the committed-but-lost-response branch the lease is already expired and a + sibling is promoting while the operator reads the refusal. Telling the operator "it is still the + leader" is right in one branch and wrong in the other, and the wrong branch sends them to fix a + cluster that is already failing over correctly. + + **A row count cannot settle it, which is why one is not read.** The driver reports rows affected + only on the path where it returns at all, and this class exists precisely for the path where it + raised instead. On the returning path both outcomes — the row expired, or no row matched because + the lease is not ours — leave no live lease owned by this node, so neither is a refusal. + + The in-memory demotion, the demote edge and the claim pause are already done by the time this + raises: they are the conservative direction on both branches. What the caller must NOT conclude is + that leadership definitely did, or definitely did not, move. + """ + + +async def acquire_leadership_lock( + lock: asyncio.Lock, fence_timeout_seconds: float, node_id: str +) -> None: + """Take a coordinator's ``_leadership_lock`` for a stepdown, or raise :class:`StepdownLockTimeout`. + + Module-level and shared by both coordinators for the reason :func:`stepdown_pause_seconds` is: the + BOUND is a safety-relevant timing policy, and a per-class copy is two files that can be retuned + independently with nothing failing. + + The bound is ``leader_fence_timeout_seconds``, derived rather than picked. That is exactly the + interval after which the node's own watchdog concludes its DB access is not working and demotes on + the node-local clock, so a stepdown still queued past it is racing a self-fence and can no longer + report a drain the operator can act on. Refusing leaves leadership exactly as it was found. + + Not an ``async with``: the acquire has to be wrapped in :func:`asyncio.wait_for` and the caller holds + the lock across work this function does not see, so it releases in its own ``finally``. + """ + try: + await asyncio.wait_for(lock.acquire(), timeout=fence_timeout_seconds) + except TimeoutError: + # Name the LOCK and the bound, and stop there. Both coordinators take this lock in + # _maintain_leadership and in step_down_leadership, so the holder is not knowably a maintenance + # tick; and the caller may have addressed a node that leads nothing, so leadership is not + # knowably "unchanged for the leader". Both of those were asserted here and neither was earned. + raise StepdownLockTimeout( + f"node {node_id}: this coordinator's leadership lock was still held after the " + f"{fence_timeout_seconds:.1f}s fence timeout, so the stepdown never ran — nothing was " + "read, written or demoted" + ) from None + + +def lease_release_unconfirmed(node_id: str) -> str: + """The message for a stepdown whose lease-row write did not return. Shared for the reason above: + both coordinators run the same owner-scoped expiring ``UPDATE`` and owe the operator the same + sentence. + + Conditional on purpose — see :class:`StepdownReleaseUnconfirmed`. The node demoted itself either + way; what nobody here can see is whether the write committed.""" + return ( + f"node {node_id}: this node demoted itself, then the write expiring its leadership lease row " + "did not return, so whether it committed is unknown — if it did not, this node still owns a " + "live lease no standby can take; if it did, a standby is already promoting" + ) + + 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 stepdown_pause_seconds(heartbeat_seconds: float) -> float: + """How long a node declines to claim after a VOLUNTARY stepdown (ADR 0056 slice 1). + + Module-level, and shared by both coordinators, for the reason :func:`fence_tick_seconds` is: it is + pure arithmetic on a constructor argument with no backend in it, and a per-class copy is a + safety-relevant timing constant that two files can retune independently with nothing failing. + + Two heartbeats, and read that as a floor rather than a guarantee. A sibling's acquire runs once per + ``heartbeat_seconds`` at an unrelated phase, so a full interval can elapse before it even looks at + the expired lease and a second gives it one whole interval in which to look. **That holds only for + a sibling carrying no ADR 0096 ``acquire_delay_seconds``.** A sibling handicapped by more than this + pause is still refused when the pause ends, and the drained node then wins its own lease back. This + function reads ``heartbeat_seconds`` alone, so it cannot see the handicap it is being compared + against; the gap is real, unfixed, and recorded on the stepdown's backlog item. + + **This pause covers the ticks that come AFTER the release. It does not order the release against a + tick already in flight** — :attr:`DbCoordinator._leadership_lock` does that, and the two are not + interchangeable. Reading a bounded pause as if it were mutual exclusion is exactly what left the + two-leader window this pause was once credited with closing. + + Deliberately short rather than lease-length: the cost of the pause is that a cluster with no other + promotable node is leaderless for it, which is the operator's own request but should not linger. + """ + return 2.0 * heartbeat_seconds + + def demote_stop_budget( *, lease_ttl_seconds: float, fence_timeout_seconds: float ) -> tuple[float, float]: @@ -291,6 +420,44 @@ async def leadership_lease(self) -> tuple[str | None, float | None]: leader with no lease/expiry.""" ... + async def step_down_leadership(self) -> tuple[bool, float | None]: + """Voluntarily release this node's leadership lease and **keep running** as a standby — the + planned-failover / maintenance-drain control plane behind ``POST /cluster/stepdown`` + (ADR 0056, slice 1). + + Returns ``(was_leader, released_at)``: whether this node actually held leadership at the moment + the release ran, and the epoch-seconds instant it was demoted (``None`` when it held none). **The + caller audits this return value, never a prior** :meth:`is_leader` **read** — a fence or a + lost-lease tick can flip leadership between the read and the release, and auditing the pre-read + would record ``was_leader=true`` for an action that released nothing. + + This is a **visibility lift** of the release the coordinators already run on a clean + :meth:`stop`, not a new election mechanism: the lease, the self-fence and the epoch token are + unchanged. The one difference from :meth:`stop` is that the node stays up and keeps + heartbeating, so it reports itself a standby rather than leaving. :class:`NullCoordinator` + returns ``(False, None)`` — single-node has no lease to release (and the endpoint refuses a + single-node caller before reaching here). + + **Raises one of two** :class:`StepdownUnavailable` **subclasses**, which say different things: + :class:`StepdownLockTimeout` when the coordinator's leadership lock was still held at the fence + timeout, so nothing ran at all; :class:`StepdownReleaseUnconfirmed` when this node demoted + itself but the write expiring its lease row did not return. The DB coordinators raise them; + :class:`NullCoordinator` never does. + + **A returned tuple does NOT mean a release wrote to the lease row**, and an earlier version of + this line said it did. ``(False, None)`` is the ordinary answer from a node that holds no + leadership and owes no write — nothing is sent to the DB, and :class:`NullCoordinator` returns + it with no DB at all. What a returned tuple does mean is that nothing is left unresolved: + either a write ran and returned, or there was none to run. + + **A retry re-attempts a write left unconfirmed**, so re-calling this is the remedy for + :class:`StepdownReleaseUnconfirmed`. The DB coordinators remember that a release is owed, and + the next stepdown re-sends the owner-scoped ``UPDATE`` even though the in-memory gate already + reads False — without that, the retry took an early return, sent nothing, and answered + ``(False, None)`` while the lease row was still live and still owned by this node. + """ + ... + class NullCoordinator: """The single-node default (SQLite and single-node Postgres). Every gate is ``True``, there is no @@ -363,6 +530,13 @@ async def leadership_lease(self) -> tuple[str | None, float | None]: # expiry so /cluster/nodes is byte-identical in shape to a real cluster's. return (self.node_id, None) + async def step_down_leadership(self) -> tuple[bool, float | None]: + # Single-node: there is no lease to release and no standby to promote, so this releases + # nothing and reports so. Unreachable through the API — POST /cluster/stepdown refuses a + # single-node caller with 400 before it touches the coordinator (ADR 0056) — but a truthful + # answer here keeps the Protocol honest for any direct caller. + return (False, None) + # One-time-per-process info guard: the active-passive HA feature set is COMPLETE — election (Step 4), # leader-gated WRITE singletons, leader-gated poll-source intake (Step 4b), cross-node convergence @@ -465,6 +639,29 @@ def __init__( # this node never claim/hold the lease at all. Default (0.0, True) = byte-identical to before. self._acquire_delay = acquire_delay_seconds self._promotable = promotable + # ADR 0056 slice 1: monotonic instant before which this node declines to claim or renew, set by + # step_down_leadership() so a voluntarily-drained node does not immediately re-arm itself via the + # renew branch. 0.0 = no pause, which is every path but a stepdown. + self._no_claim_until: float = 0.0 + # ADR 0056 slice 1: a lease-expiring write did not return, so this node may still own a live + # lease row it has already stopped claiming in memory. _release_leadership ARMS it before the + # write and clears it only when one returns, so neither a raise nor a cancellation can leave it + # clear. Read by step_down_leadership ALONE, to force the retry's write past the not-a-leader + # early return — without it a retry sends nothing and answers "not the leader" over a lease row + # that is still live and still ours. + self._lease_release_owed = False + # ADR 0056 slice 1: mutual exclusion between _maintain_leadership and the stepdown's release. + # BOTH of them decide leadership across an await on the pool, and a stepdown runs from an API + # handler with the maintenance loop LIVE — unlike stop(), which cancels and gathers both loops + # before it releases. Without this, a maintenance tick interleaving with the release re-promotes + # the node it just drained, in either order: a tick that STARTS in the release's await window + # renews the lease the release is expiring, and a tick already suspended inside its claim + # round-trip returns True afterwards and flips _is_leader back on. The pause above cannot close + # either — it is checked BEFORE the claim's await, so a claim already in flight has passed it. + # Held ONLY by those two coroutines: the fence watchdog stays lock-free (it must fence during a + # DB hang, and it only ever demotes), and stop() releases without it so a shutdown is never + # blocked behind an in-flight stepdown waiting on a hung pool. + self._leadership_lock = asyncio.Lock() # Monotonic clock for the fence (injectable for deterministic tests). Distinct from the DB clock # the lease uses: the fence measures a node-local elapsed duration (free of INTER-NODE skew — # that is the property being bought), the lease compares against the DB's own clock_timestamp(). @@ -537,6 +734,14 @@ async def stop(self) -> None: # Drop leadership: demote the cached gate FIRST so any concurrent is_leader() reader sees "not # leader" the instant we begin releasing, then expire the lease row so a standby can take over # immediately on a clean shutdown (best-effort — a failed release just lets the lease age out). + # Deliberately NOT under _leadership_lock. The gather above retired the maintenance loop, but + # NOT step_down_leadership(), which runs from an API handler this method never sees — so a + # shutdown concurrent with a stepdown is genuinely unserialized here. That is the trade taken + # on purpose: taking the lock would queue a shutdown behind a stepdown stalled on a hung pool, + # and the unserialized case is benign because both coroutines only ever DEMOTE (each sets + # _is_leader False and issues the same owner-scoped expiring UPDATE, which is idempotent), so + # no interleaving of the two can leave this node reporting leader. The maintenance tick was the + # dangerous competitor precisely because it can promote. await self._release_leadership() # Mark the row left rather than DELETE it: keeping a 'left' tombstone gives an operator a # visible "this node shut down cleanly" signal (vs a crashed node whose row goes stale), which @@ -903,26 +1108,35 @@ async def _maintain_leadership(self) -> None: it / our lease expired). A DB error propagates to the loop, which logs and retries — we do NOT demote on an error here; ``_last_renew_ok`` simply isn't advanced, so the fence watchdog demotes us only if the failure persists past the fence timeout (and always before the lease can expire). + + The whole tick — the claim AND the bookkeeping that reads its result — runs under + :attr:`_leadership_lock`, because the promotion decision is made on a value that crossed an + await. A stepdown that interleaved here would be undone by the tick's own stale result; see the + lock's comment in ``__init__``. """ - held = await self._claim_or_renew_lease() - if held: - self._last_renew_ok = self._monotonic() - if not self._is_leader: - self._is_leader = True - log.info("cluster: node %s acquired leadership (lease)", self.node_id) - # #145: a non-leader→leader transition is a failover / election edge — alert (never-raise). - self._alert_leadership_acquired() - elif self._is_leader: - # The lease is held by another node (or expired and taken over) — we are no longer leader. - self._is_leader = False - # Drop the held epoch: we are no longer a fenced leader, and the next acquire will read the - # (now-higher) epoch the successor bumped. Leaving a stale epoch cached would be harmless - # (the store guard already lost when the graph stopped) but clearing it keeps current_epoch() - # honest. - 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 with self._leadership_lock: + held = await self._claim_or_renew_lease() + if held: + self._last_renew_ok = self._monotonic() + if not self._is_leader: + self._is_leader = True + log.info("cluster: node %s acquired leadership (lease)", self.node_id) + # #145: a non-leader→leader transition is a failover / election edge — alert + # (never-raise). + self._alert_leadership_acquired() + elif self._is_leader: + # The lease is held by another node (or expired and taken over) — we are no longer leader. + self._is_leader = False + # Drop the held epoch: we are no longer a fenced leader, and the next acquire will read + # the (now-higher) epoch the successor bumped. Leaving a stale epoch cached would be + # harmless (the store guard already lost when the graph stopped) but clearing it keeps + # current_epoch() honest. + 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. @@ -961,6 +1175,12 @@ async def _claim_or_renew_lease(self) -> bool: # demote a node that was somehow already leader (a clean step-down), and the fence watchdog is # the backstop. At least one promotable node must exist or the cluster elects no leader. return False + if self._monotonic() < self._no_claim_until: + # JUST STEPPED DOWN (ADR 0056 slice 1): decline for a bounded window so a sibling wins the + # expired lease instead of us renewing it straight back. Same shape as the check above — + # touch no DB row, report not-held — and strictly stricter than the base predicate, so it + # can only delay a claim, never advance one. + return False row = await self._pool.fetchrow( "INSERT INTO leader_lease (lease_key, owner, lease_expires_at, leader_epoch) " "VALUES ($1, $2, EXTRACT(EPOCH FROM clock_timestamp()) + $3, 1) " @@ -1031,17 +1251,178 @@ def _check_fence(self) -> None: 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 - never sees a stale True), then expire our lease row so a standby can acquire immediately on a - clean shutdown. Safe to call when never elected (the UPDATE simply matches no owned row).""" + async def step_down_leadership(self) -> tuple[bool, float | None]: + """Release leadership and stay up as a standby (ADR 0056 slice 1). See the Protocol method. + + It calls the same :meth:`_release_leadership` ``stop()`` does, but **the ordering inside that + method is not what makes either call safe, and reading it that way is the mistake this + docstring used to make.** ``stop()`` cancels and gathers BOTH loops before it releases, so + nothing is running concurrently by the time it touches the lease. A stepdown arrives from an + API handler with both loops LIVE, so it has to supply that exclusion itself. Three things + ``stop()`` therefore does not need: + + * **Serialize against the maintenance tick** with :attr:`_leadership_lock`. Held across the + release AND the pause below, so a tick can neither start inside the release's await window + nor deliver a claim result it obtained before the release began. Both interleavings + re-promoted the node this endpoint had just drained, one of them into a two-leader window + against a sibling that had already taken the expired lease. + * **Fire the demotion edge** so the graph tears down at once instead of waiting out a whole + ``_graph_reconcile_interval`` poll (ADR 0157 Inc 5). Every other True->False transition + (``_maintain_leadership``, ``_check_fence``) fires it; a stepdown the node SURVIVES would + otherwise be the one demotion the engine learns about late. + * **Pause our own claim** for :func:`stepdown_pause_seconds`. Without it the stepdown is a + coin flip: :meth:`_release_leadership` expires ``lease_expires_at`` but leaves ``owner`` + naming us, so the renew branch (``owner = me``, which carries no expiry test) matches on our + very next maintenance tick and hands leadership straight back — a drained node re-arming + itself while the endpoint reported 200. The pause is a strictly STRICTER claim predicate on + this node only (the same shape as ADR 0096's ``acquire_delay``), so it can only make us claim + LATER, never earlier. It changes nothing about the lease, the self-fence or the epoch token. + **It is a claim predicate, not mutual exclusion**: it is evaluated before the claim's await, + so it says nothing about a claim already in flight — that is the lock's job, above. It is + armed TWICE, before the release AND again after it, taking whichever expiry is later. The + first arm is what a cancellation landing inside the pool write cannot skip; the second is + what keeps the write's own unbounded duration from being spent out of the pause, since the + lease row does not become takeable until that write commits. Neither arm buys anything + against either interleaving above and neither is credited with doing so. + + Deadlock, since the lock is new: it is taken in exactly two coroutines, neither of which calls + the other, so there is no ordering to invert. A cancelled tick releases it on the way out + (``async with`` unwinds), and ``stop()`` deliberately does not take it, so a shutdown never + queues behind a stepdown stalled on a hung pool. + + **What the lock COSTS, stated because a control resting on a false premise is worse than no + control.** An earlier draft of this docstring said waiting for an in-flight tick "adds no new + stall to this method either — it already awaits the same pool inside the release". That is + false, and in the one direction that matters: the first line of :meth:`_release_leadership` is + the SYNCHRONOUS in-memory demotion, so before this lock that demotion happened immediately and + now it happens behind a tick's DB round trip. A tick suspended in ``fetchrow`` holds the lock, + and while this call waits, the node an operator is draining still answers :meth:`is_leader` + ``True`` and still binds listeners. Nothing bounds that round trip from here either: + ``[store].command_timeout`` is the only ceiling, ``PostgresStore`` passes ``command_timeout or + None`` so the documented zero-disables value makes it unbounded, and the pool ``acquire()`` + carries no timeout at all. + + So the wait is bounded, and a timeout refuses with ``503`` rather than demoting anything — + :func:`acquire_leadership_lock` holds that bound and the reasoning behind it. + + **Retrying a stepdown whose write did not return re-sends that write.** ``_release_leadership`` + demotes the in-memory gate BEFORE the write, so on the retry its not-a-leader early return + fired, the ``UPDATE`` was never re-attempted, and the caller was told "not the current leader" + over a lease row still live and still owned by this node — with the endpoint's own remedy for + that answer pointing back at this same node. :attr:`_lease_release_owed` records the owed write + so this method can force it past that early return. "Did not return" covers a CANCELLED write + as well as a raised one — the request deadline can cancel this call mid-write — which is why + :meth:`_release_leadership` arms that flag *before* the write rather than in its ``except``. + """ + await acquire_leadership_lock(self._leadership_lock, self._fence_timeout, self.node_id) + try: + # Arm the claim pause BEFORE the release's await, not after it. Cancelling the request task + # while the release is suspended in the pool write unwinds this method correctly but would + # skip an assignment placed after the await, leaving _no_claim_until at 0.0 on a node whose + # lease row may already be expired — the very re-arm the pause exists to prevent. Reading + # _is_leader here is exact rather than a "pre-read" of the kind the endpoint refuses to + # make: nothing suspends between this read and _release_leadership's own read of the same + # attribute (awaiting a coroutine does not yield to the loop), and the only other writers + # are _maintain_leadership, which is holding-lock-excluded, and _check_fence, which is + # synchronous and therefore cannot run in that gap. + owed = self._lease_release_owed + # Held across the await because the second arm below needs the SAME predicate, and + # `self._is_leader` is already False by then — _release_leadership clears it on its first + # line, so re-reading it there would silently arm nothing. + arming = self._is_leader or owed + if arming: + # Stand down long enough that every sibling has had a full tick at the expired lease. + # The retry needs this as much as the first call does: the release expires + # `lease_expires_at` but leaves `owner` naming us, so a successful retry with no pause + # re-arms this node through the unfenced `owner = me` branch on its very next tick. + self._no_claim_until = self._monotonic() + stepdown_pause_seconds( + self._heartbeat_seconds + ) + was_leader, released_at, wrote = await self._release_leadership(force_write=owed) + if arming: + # ARMED A SECOND TIME, from the instant the write RETURNED, taking whichever expiry is + # LATER. The arm above is measured from before the write, so the write's own duration + # comes out of the pause — and nothing bounds that duration from here: the pool + # acquire() carries no timeout and `[store].command_timeout` (30s by default) is the + # only ceiling on the statement, already longer than the 20s pause at the shipped + # heartbeat. The window that matters starts when the row becomes takeable, which is the + # commit, not the call: measuring from before it would hand a sibling (2 * heartbeat - + # write duration), which can reach zero, and the drained node's own queued tick — which + # waits on this lock for the whole call — would then fall straight through the pause + # gate and renew itself back in through the unfenced `owner = me` branch, milliseconds + # after the endpoint answered 200. + # + # max(), not a replacement: the pause may only ever move LATER. A real monotonic clock + # makes the second value the larger one by construction, but an injected or coarse + # clock must not be able to SHORTEN a pause this call already promised. + self._no_claim_until = max( + self._no_claim_until, + self._monotonic() + stepdown_pause_seconds(self._heartbeat_seconds), + ) + if was_leader: + self._fire_on_demote() + # NOT nested under `was_leader`. A retry re-sending an owed write has already demoted, so it + # reports was_leader=False and would otherwise swallow a second failure into a 409. + if not wrote: + raise StepdownReleaseUnconfirmed(lease_release_unconfirmed(self.node_id)) + finally: + self._leadership_lock.release() + return (was_leader, released_at) + + async def _release_leadership( + self, *, force_write: bool = False + ) -> tuple[bool, float | None, bool]: + """Clean release: demote the cached gate first (so a concurrent is_leader() reader never sees a + stale True), then expire our lease row so a standby can acquire immediately. Safe to call when + never elected (the UPDATE simply matches no owned row). + + Returns ``(was_leader, released_at, wrote)`` — whether this node held leadership when the + release ran, the epoch-seconds instant it was demoted, and whether the lease row's ``UPDATE`` + returned. ``released_at`` is stamped at the in-memory demotion, not after the DB round trip: + that instant is when this node stopped answering :meth:`is_leader` ``True``, which is the fact + the audit trail is recording. + + **``wrote`` exists because the two callers want opposite things from a failed write.** + :meth:`stop` is best-effort — the node is leaving, so a lease that ages out at its TTL costs + nothing and a raise would break shutdown. :meth:`step_down_leadership` is not: the node stays + up possibly holding a live lease no sibling can take, so it turns ``wrote=False`` into + :class:`StepdownReleaseUnconfirmed`. The exception is raised there rather than here so this + method keeps exactly one behaviour for both callers. + + **``wrote=True`` means the driver returned, NOT that a row changed**, and the difference does + not matter to either caller: the ``UPDATE`` is owner-scoped, so it either expired our live + lease or matched nothing because the lease is not ours, and neither leaves a live lease owned + by this node. What no row count can answer is the raising path, where the driver returns no + count at all — see :class:`StepdownReleaseUnconfirmed`. + + ``force_write`` sends the ``UPDATE`` even when this node's in-memory gate already reads False. + Only :meth:`step_down_leadership` passes it, and only when :attr:`_lease_release_owed` says an + earlier write did not return. :meth:`stop` never does: it is best-effort by design, and a + no-op ``UPDATE`` from every departing follower would log a warning on a pool that is closing.""" was_leader = self._is_leader self._is_leader = False self._last_renew_ok = None self._leader_epoch = None # released: no longer a fenced leader - if not was_leader: - return - self._alert_leadership_lost("released") # #145: clean step-down (inverse → auto-resolves) + if not was_leader and not force_write: + return (False, None, True) + released_at = time.time() if was_leader else None + # #145: clean step-down (inverse -> auto-resolves). Guarded, because a forced retry alerted on + # its first pass: it is re-sending a write, not demoting a second time. + if was_leader: + self._alert_leadership_lost("released") + # OWED BEFORE THE WRITE, cleared only on a write that returned. The obvious placement — set it + # in the except arm — leaks on CANCELLATION: `asyncio.CancelledError` derives from + # BaseException, so `except Exception` below does not see it, and the method unwinds with the + # in-memory `self._is_leader = False` above already done and nothing owed. The next stepdown + # would then read owed=False, take the `not was_leader and not force_write` early return above, + # send no UPDATE, and answer "not the current leader" over a lease row that may still be live + # and still ours — with no audit row of either kind on that path. + # + # Reachable in the shipped configuration, not just in theory: `create_app` registers + # RequestTimeoutMiddleware unconditionally and its asyncio.timeout cancels the handler at + # api.request_timeout.DEFAULT_REQUEST_TIMEOUT_SECONDS (120.0), over a pool acquire + # step_down_leadership's own docstring documents as unbounded. + self._lease_release_owed = True try: # Expire the lease (set it to the epoch) only if we still own it, so a standby's next # acquire tick takes over at once instead of waiting out the full TTL. @@ -1057,6 +1438,9 @@ async def _release_leadership(self) -> None: self.node_id, safe_exc(exc), ) + return (was_leader, released_at, False) + self._lease_release_owed = False + return (was_leader, released_at, True) # --- #145 leadership-transition alerts (never-raise) --------------------- diff --git a/messagefoundry/pipeline/cluster_sqlserver.py b/messagefoundry/pipeline/cluster_sqlserver.py index 18b67fc57..d015f2ad0 100644 --- a/messagefoundry/pipeline/cluster_sqlserver.py +++ b/messagefoundry/pipeline/cluster_sqlserver.py @@ -52,7 +52,14 @@ from typing import TYPE_CHECKING, Any from messagefoundry.pipeline.alerts import AlertSink, LoggingAlertSink -from messagefoundry.pipeline.cluster import ClusterMember, default_node_id +from messagefoundry.pipeline.cluster import ( + ClusterMember, + StepdownReleaseUnconfirmed, + acquire_leadership_lock, + default_node_id, + lease_release_unconfirmed, + stepdown_pause_seconds, +) from messagefoundry.redaction import safe_exc log = logging.getLogger(__name__) @@ -122,6 +129,19 @@ def __init__( # Default (0.0, True) = byte-identical. Mirrors DbCoordinator. self._acquire_delay = acquire_delay_seconds self._promotable = promotable + # ADR 0056 slice 1: monotonic instant before which this node declines to claim or renew, set by + # step_down_leadership(). Mirrors DbCoordinator._no_claim_until — read its comment there. + self._no_claim_until: float = 0.0 + # ADR 0056 slice 1: an owed lease-expiring write. Mirrors DbCoordinator._lease_release_owed — + # read its comment there for why a retry needs it. + self._lease_release_owed = False + # ADR 0056 slice 1: mutual exclusion between _maintain_leadership and the stepdown's release. + # Mirrors DbCoordinator._leadership_lock — read its comment there for why the pause alone cannot + # close the window. The MERGE below carries the identical unfenced `t.owner = ?` renew branch, + # so the interleaving and its consequence are the same on this backend, and worse in one + # respect: only the three FIFO claim paths are epoch-fenced here (current_epoch()), so a + # re-promoted ex-leader is not fenced out of claim_ready or any terminal resolve. + self._leadership_lock = asyncio.Lock() self._monotonic = monotonic # Schema-namespace the DDL applock + the lease key, exactly as DbCoordinator does, so two # deployments sharing one database via different schemas don't contend / co-elect. @@ -170,7 +190,8 @@ async def stop(self) -> None: if tasks: await asyncio.gather(*tasks, return_exceptions=True) # Demote the cached gate FIRST (a concurrent is_leader() reader sees "not leader" at once), then - # expire the lease row so a standby can take over immediately on a clean shutdown. + # expire the lease row so a standby can take over immediately on a clean shutdown. Deliberately + # NOT under _leadership_lock, and best-effort on a failed write — see DbCoordinator.stop(). await self._release_leadership() try: await self._store._execute( @@ -424,19 +445,25 @@ async def _heartbeat_loop(self) -> None: continue async def _maintain_leadership(self) -> None: - held = await self._claim_or_renew_lease() - if held: - self._last_renew_ok = self._monotonic() # stamp for the fence watchdog - if not self._is_leader: - self._is_leader = True - log.info("cluster: node %s acquired leadership (lease)", self.node_id) - self._alert_leadership_acquired() # #145 (lockstep with DbCoordinator) - elif self._is_leader: - self._is_leader = False - 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 + # The claim AND the bookkeeping that reads its result run under the lock: the promotion decision + # is made on a value that crossed an await, so a stepdown interleaving here would be undone by + # this tick's own stale result. Mirrors DbCoordinator._maintain_leadership. + async with self._leadership_lock: + held = await self._claim_or_renew_lease() + if held: + self._last_renew_ok = self._monotonic() # stamp for the fence watchdog + if not self._is_leader: + self._is_leader = True + log.info("cluster: node %s acquired leadership (lease)", self.node_id) + self._alert_leadership_acquired() # #145 (lockstep with DbCoordinator) + elif self._is_leader: + self._is_leader = False + 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 @@ -460,6 +487,11 @@ async def _claim_or_renew_lease(self) -> bool: # NON-PROMOTABLE: never acquire or renew, so this node can never become/remain leader. Touch no # DB row — _maintain_leadership demotes a somehow-already-leader node; the fence is the backstop. return False + if self._monotonic() < self._no_claim_until: + # JUST STEPPED DOWN (ADR 0056 slice 1): decline for a bounded window so a sibling wins the + # expired lease instead of this node renewing it straight back via the un-delayed t.owner = me + # branch. Mirrors DbCoordinator._claim_or_renew_lease — read its comment there. + return False row = await self._store._fetchone( "SET NOCOUNT ON;" f" DECLARE @now FLOAT = {_DB_NOW};" @@ -518,14 +550,67 @@ def _check_fence(self) -> None: self._alert_leadership_lost("self-fenced") # #145 (inverse → auto-resolves) self._fire_on_demote() # ADR 0157 Inc 5 - async def _release_leadership(self) -> None: + async def step_down_leadership(self) -> tuple[bool, float | None]: + """Release leadership and stay up as a standby (ADR 0056 slice 1). Mirrors + :meth:`~messagefoundry.pipeline.cluster.DbCoordinator.step_down_leadership` — read its + docstring for why the release is serialized against the maintenance tick, why the demotion + edge fires, why this node pauses its own claim (and why the pause is not the exclusion), why the + pause is armed BOTH before and after the release, what the lock costs, why a retry re-sends an + owed write, and why an unconfirmed lease write raises + :class:`~messagefoundry.pipeline.cluster.StepdownReleaseUnconfirmed` here but not on + :meth:`stop`.""" + await acquire_leadership_lock(self._leadership_lock, self._fence_timeout, self.node_id) + try: + # Armed before the release's await so a cancellation inside the pool write cannot skip it, + # and armed on a retry too, which needs the pause as much as the first call does. The pause + # length, the lock's bound and the refusal text are all the SHARED module-level policy, not + # copies: a per-class copy of a safety-relevant timing constant (or of the sentence an + # operator acts on) is two files that can be retuned independently. + owed = self._lease_release_owed + arming = self._is_leader or owed # held across the await; _is_leader is False by then + if arming: + self._no_claim_until = self._monotonic() + stepdown_pause_seconds( + self._heartbeat_seconds + ) + was_leader, released_at, wrote = await self._release_leadership(force_write=owed) + if arming: + # Armed a SECOND time from the instant the write returned, taking the later expiry, so + # the write's own unbounded duration is not spent out of the pause. DbCoordinator's + # step_down_leadership carries the full reasoning; keep the two in lockstep. + self._no_claim_until = max( + self._no_claim_until, + self._monotonic() + stepdown_pause_seconds(self._heartbeat_seconds), + ) + if was_leader: + self._fire_on_demote() + if not wrote: # NOT nested under was_leader — a retry has already demoted + raise StepdownReleaseUnconfirmed(lease_release_unconfirmed(self.node_id)) + finally: + self._leadership_lock.release() + return (was_leader, released_at) + + async def _release_leadership( + self, *, force_write: bool = False + ) -> tuple[bool, float | None, bool]: + """``(was_leader, released_at, wrote)`` — mirrors ``DbCoordinator._release_leadership``, + including the demote-the-cached-gate-before-the-DB ordering, the stamp taken at the in-memory + demotion, the ``wrote`` flag its two callers read in opposite directions, ``force_write``, + which re-sends an owed ``UPDATE`` past the not-a-leader early return, and the arm-before-the- + write ordering that keeps a CANCELLED write from unwinding with nothing owed.""" was_leader = self._is_leader self._is_leader = False self._last_renew_ok = None self._leader_epoch = None # released: no longer a fenced leader (H1) - if not was_leader: - return - self._alert_leadership_lost("released") # #145: clean step-down (inverse → auto-resolves) + if not was_leader and not force_write: + return (False, None, True) + released_at = time.time() if was_leader else None + # #145: clean step-down (inverse -> auto-resolves), guarded exactly as DbCoordinator guards it. + if was_leader: + self._alert_leadership_lost("released") + # Armed before the write, cleared only on one that returned — `except Exception` cannot catch + # asyncio.CancelledError, so an owed flag set in the arm below would leak on cancellation. + # DbCoordinator._release_leadership carries the full reasoning; keep the two in lockstep. + self._lease_release_owed = True try: await self._store._execute( "UPDATE leader_lease SET lease_expires_at = 0 WHERE lease_key = ? AND owner = ?", @@ -538,6 +623,9 @@ async def _release_leadership(self) -> None: self.node_id, safe_exc(exc), ) + return (was_leader, released_at, False) + self._lease_release_owed = False + return (was_leader, released_at, True) # --- #145 leadership-transition alerts (never-raise; lockstep with DbCoordinator) ---- diff --git a/tests/test_api_cluster_stepdown.py b/tests/test_api_cluster_stepdown.py new file mode 100644 index 000000000..1e85b6a9e --- /dev/null +++ b/tests/test_api_cluster_stepdown.py @@ -0,0 +1,491 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""``POST /cluster/stepdown`` — the planned-failover control plane (ADR 0056 slice 1, BACKLOG #1494). + +ADR 0056 AC-9 names this file. It covers the whole status table (200 / 400 / 403 / 409 / 422 / 503), +both shapes of the ``503`` (no engine, and a drain the coordinator could not achieve), the +content of the audit row, and — the load-bearing one — that the audited ``was_leader`` comes from what +``step_down_leadership()`` RETURNED and never from a prior ``is_leader()`` read. A fence or a +lost-lease tick can flip leadership between a pre-read and the release, so a pre-read would record a +failover that released nothing. + +The discriminating fixture is :class:`_StandinCoordinator`, which lets the two answers DISAGREE. A test +suite whose coordinator always answers the same way on both cannot tell the two implementations apart, +which is the failure mode this file exists to rule out. +""" + +from __future__ import annotations + +import json +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from pathlib import Path +from typing import Any + +import httpx +import pytest + +from messagefoundry.api import create_app +from messagefoundry.auth import Permission, Role +from messagefoundry.auth.permissions import ( + BUILTIN_ROLE_PERMISSIONS, + CUSTOM_ROLE_FORBIDDEN_PERMISSIONS, + CustomRoleError, + validate_custom_role_permissions, +) +from messagefoundry.auth.service import AuthService +from messagefoundry.auth.tokens import hash_token +from messagefoundry.config.settings import AuthSettings +from messagefoundry.pipeline import Engine +from messagefoundry.pipeline.cluster import ( + ClusterCoordinator, + NullCoordinator, + StepdownLockTimeout, + StepdownReleaseUnconfirmed, +) +from messagefoundry.store import MessageStore + +PW = "a-strong-test-passphrase" # >=15 chars, no vendor terms — satisfies the ASVS password policy + + +class _StandinCoordinator(NullCoordinator): + """A coordinator whose ``is_leader()`` and ``step_down_leadership()`` are set SEPARATELY. + + That separation is the point. ``step_down`` is the tuple the release returns; ``leader`` is what a + pre-read would have seen. Setting them to disagree reproduces the fence/lost-lease race the ADR + warns about, and is the only way a test can prove which one the handler audited. + + Subclasses :class:`NullCoordinator` — the house pattern for a coordinator stand-in — so the three + answers these tests vary are the only three written here, and a future Protocol method does not + have to be hand-copied in. + """ + + def __init__( + self, + *, + clustered: bool = True, + leader: bool = True, + step_down: tuple[bool, float | None] = (True, 1_700_000_000.5), + raises: Exception | None = None, + ) -> None: + super().__init__("node-a") + self._clustered = clustered + self._leader = leader + self._step_down = step_down + self._raises = raises + self.step_down_calls = 0 + + def is_leader(self) -> bool: + return self._leader + + def is_clustered(self) -> bool: + return self._clustered + + async def step_down_leadership(self) -> tuple[bool, float | None]: + self.step_down_calls += 1 + if self._raises is not None: + raise self._raises + return self._step_down + + +# --- harness ------------------------------------------------------------------ + + +async def _engine(tmp_path: Path, coordinator: ClusterCoordinator | None = None) -> Engine: + """A started SQLite engine holding ``coordinator``. ``Engine.create`` is the documented + tests/embedding path and forwards ``coordinator`` straight through, so nothing here re-implements + the constructor's keyword list.""" + eng = await Engine.create(tmp_path / "stepdown.db", poll_interval=0.02, coordinator=coordinator) + await eng.start() + return eng + + +async def _service(store: MessageStore, settings: AuthSettings | None = None) -> AuthService: + service = AuthService( + store, settings or AuthSettings(require_mfa=False, login_rate_limit_enabled=False) + ) + await service.initialize() + return service + + +def _client(engine: Engine | None, service: AuthService) -> httpx.AsyncClient: + transport = httpx.ASGITransport(app=create_app(engine, auth=service)) # type: ignore[arg-type] + return httpx.AsyncClient(transport=transport, base_url="http://t") + + +async def _add(service: AuthService, username: str, *roles: Role) -> None: + user_id = await service.create_local_user( + username=username, + password=PW, + display_name=None, + email=None, + roles=[r.value for r in roles], + actor="test", + ) + user = await service.store.get_user(user_id) + assert user is not None and user.password_hash is not None + # Admin-created accounts force first-login rotation; clear it for a usable test login. + await service.store.set_password( + user_id, password_hash=user.password_hash, must_change_password=False + ) + + +async def _login(c: httpx.AsyncClient, username: str) -> str: + r = await c.post( + "/auth/login", json={"username": username, "password": PW, "provider": "local"} + ) + assert r.status_code == 200, r.text + return str(r.json()["token"]) + + +def _auth(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +def _rotated(response: httpx.Response, token: str) -> str: + """The bearer to use AFTER an elevation call (ASVS 7.2.4). + + A successful elevation re-keys the session and returns the new token in the body, so every later + request has to carry it -- keeping the old one would 401 and quietly turn a real assertion into a + test of an expired token. A refusal rotates nothing and the incoming token is handed back. Same + helper, same wording, as ``tests/test_step_up.py`` and ``tests/test_api_auth.py``.""" + if response.status_code != 200: + return token + fresh = response.json().get("token") + assert isinstance(fresh, str) and fresh, "an elevation route returned no rotated token" + return fresh + + +@asynccontextmanager +async def _admin( + tmp_path: Path, + coordinator: ClusterCoordinator | None = None, + settings: AuthSettings | None = None, +) -> AsyncIterator[tuple[Engine, httpx.AsyncClient, str]]: + """A started engine, an API client, and a signed-in Administrator's bearer token. + + Every test below needs those three and nothing else varies but the coordinator, so the scaffold + lives here once rather than as a try/finally in each body.""" + engine = await _engine(tmp_path, coordinator) + try: + service = await _service(engine.store, settings) + await _add(service, "boss", Role.ADMINISTRATOR) + async with _client(engine, service) as c: + yield engine, c, await _login(c, "boss") + finally: + await engine.stop() + + +async def _rows(engine: Engine, action: str) -> list[dict[str, object]]: + return [r for r in await engine.store.list_audit(limit=200) if r["action"] == action] + + +# --- the permission itself --------------------------------------------------- + + +def test_cluster_control_is_administrator_only() -> None: + # A dedicated capability, NOT a reuse of monitoring:read (a read) or connections:control (one + # connection): a planned failover moves the whole cluster's primary (ADR 0056). + assert Permission.CLUSTER_CONTROL in BUILTIN_ROLE_PERMISSIONS[Role.ADMINISTRATOR] + for role in (Role.OPERATOR, Role.DEPLOYMENT, Role.CODING, Role.VIEWER, Role.AUDITOR): + assert Permission.CLUSTER_CONTROL not in BUILTIN_ROLE_PERMISSIONS[role] + + +def test_cluster_control_not_assignable_to_a_custom_role() -> None: + # "Administrator only" is enforced on every minting path, not merely observed of the built-ins. + assert Permission.CLUSTER_CONTROL in CUSTOM_ROLE_FORBIDDEN_PERMISSIONS + with pytest.raises(CustomRoleError): + validate_custom_role_permissions(["cluster:control"]) + + +# --- AC-9: the status table + the audit row ---------------------------------- + + +async def test_stepdown_rbac_audit_and_status_codes(tmp_path: Path) -> None: + """ADR 0056 AC-9, end to end on one clustered node.""" + coord = _StandinCoordinator(clustered=True, leader=True) + async with _admin(tmp_path, coord) as (engine, c, boss): + # 403 — an OPERATOR holds connections:control but not cluster:control (deny-by-default), and + # require_step_up records that denial itself, so the handler never runs. + service = await _service(engine.store) + await _add(service, "op", Role.OPERATOR) + op = await _login(c, "op") + denied = await c.post("/cluster/stepdown", headers=_auth(op), json={}) + assert denied.status_code == 403 + assert "cluster:control" in denied.json()["detail"] + assert coord.step_down_calls == 0 + assert any( + "/cluster/stepdown" in str(row["detail"] or "") + for row in await engine.store.list_audit(limit=200) + if "denied" in str(row["action"] or "") + ) + + # 403 — step-up recency. The window is back-dated, so a session that holds the permission is + # still refused until it re-proves its credential. + await service.store.mark_session_reauthed(hash_token(boss), now=0.0) + stale = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert stale.status_code == 403 + assert stale.headers.get("X-Step-Up-Required") == "1" + assert coord.step_down_calls == 0 + reauth = await c.post("/me/reauth", headers=_auth(boss), json={"password": PW}) + assert reauth.status_code == 200 + # A successful re-auth ROTATES the session (ASVS 7.2.4), so the old bearer stops resolving. + # Without this rebind every assertion below silently becomes a test of an expired token: the + # 422 arrives as a 401 and the 200/409 arms never reach the handler at all. + boss = _rotated(reauth, boss) + + # 422 — the deferred `force` flag is refused rather than silently ignored (RequestModel). + forced = await c.post("/cluster/stepdown", headers=_auth(boss), json={"force": True}) + assert forced.status_code == 422 + assert coord.step_down_calls == 0 + + # 200 — the happy path. The body reports what the coordinator returned. + ok = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert ok.status_code == 200, ok.text + assert ok.json() == { + "node_id": "node-a", + "was_leader": True, + "released_at": 1_700_000_000.5, + } + assert coord.step_down_calls == 1 + # ...and there is no successor field: ADR 0056 left new_leader_eligible unresolved and the + # caller re-polls /cluster/nodes instead of being handed a guess. + assert "new_leader_eligible" not in ok.json() + + # The granted audit row: the acting user, cluster metadata only, no PHI. Its detail is the + # RESPONSE body itself, so the two cannot drift apart field by field. + rows = await _rows(engine, "cluster_stepdown") + assert len(rows) == 1 + assert rows[0]["actor"] == "boss" + assert rows[0]["channel_id"] is None + assert json.loads(str(rows[0]["detail"])) == ok.json() + + # 409 — this node is not the leader. Normative, not an idempotent retry. + coord._step_down = (False, None) + conflict = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert conflict.status_code == 409 + assert "not the current leader" in conflict.json()["detail"] + assert coord.step_down_calls == 2 + + +async def test_stepdown_audits_the_returned_was_leader_not_a_pre_read(tmp_path: Path) -> None: + # THE DISCRIMINATING CASE. is_leader() says True (what a pre-read would have seen) while the + # release reports it held nothing — the fence / lost-lease tick landing between the two. A handler + # that audited the pre-read would write was_leader=true for an action that released nothing, and + # would answer 200. Both halves are asserted, so neither can drift alone. + coord = _StandinCoordinator(clustered=True, leader=True, step_down=(False, None)) + async with _admin(tmp_path, coord) as (engine, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 409 + rows = await _rows(engine, "cluster_stepdown") + assert len(rows) == 1 + assert json.loads(str(rows[0]["detail"])) == { + "node_id": "node-a", + "was_leader": False, + "released_at": None, + } + + +async def test_stepdown_does_not_pre_read_is_leader_at_all(tmp_path: Path) -> None: + # The mirror of the case above, and the positive control for it: is_leader() says False while the + # release reports it really did hold the lease. A handler with an is_leader() 409 gate would refuse + # here without ever calling the coordinator; this one calls it and answers 200. + coord = _StandinCoordinator(clustered=True, leader=False, step_down=(True, 42.0)) + async with _admin(tmp_path, coord) as (_eng, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 200, r.text + assert r.json()["was_leader"] is True and r.json()["released_at"] == 42.0 + assert coord.step_down_calls == 1 + + +async def test_single_node_is_refused_before_the_coordinator_is_touched(tmp_path: Path) -> None: + # 400 on a single node, gated BEFORE the coordinator (ADR 0056): there is no lease to release and + # no standby to promote, so the answer must not depend on a NullCoordinator's no-op. The refusal is + # audited because it is one the HANDLER reaches — require_step_up records the 403s, not this. + coord = _StandinCoordinator(clustered=False, leader=True) + async with _admin(tmp_path, coord) as (engine, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 400 + assert "not clustered" in r.json()["detail"] + assert coord.step_down_calls == 0 + assert not await _rows(engine, "cluster_stepdown") + denied = await _rows(engine, "cluster_stepdown_denied") + assert len(denied) == 1 + assert json.loads(str(denied[0]["detail"])) == { + "node_id": "node-a", + "reason": "not-clustered", + } + + +async def test_default_single_node_engine_is_refused(tmp_path: Path) -> None: + # The same 400 through the SHIPPED single-node path (NullCoordinator), not only the stand-in — so + # the gate is proven against the coordinator an operator actually runs on SQLite. + async with _admin(tmp_path) as (_eng, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 400 + + +async def test_an_unconfirmed_release_is_503_and_is_not_audited_as_a_stepdown( + tmp_path: Path, +) -> None: + # The failure the coordinator can no longer hide. If the write did not land, the lease row is live + # and still owned by a node that has already demoted, so no standby can take it. Reporting + # 200/was_leader=true there would send an operator into a node that may still hold the lease, + # which is the whole point of asking. + # + # 503, not 409 or 500: this is an ENVIRONMENT condition, the status the neighbouring DR endpoints + # and ADR 0056's own contract already give those. 409 would be wrong in the other direction -- it + # says "you addressed the wrong node", and the caller would go and address a different one. + coord = _StandinCoordinator( + raises=StepdownReleaseUnconfirmed("the lease-expiring write did not return") + ) + async with _admin(tmp_path, coord) as (engine, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 503 + detail_text = r.json()["detail"] + assert coord.step_down_calls == 1 + + # CONDITIONAL, because the outcome is genuinely unknown: a lost response to a committed UPDATE + # is indistinguishable here from an UPDATE that never ran. The retired body asserted "it is + # still the leader", which on the committed branch sends an operator to fix a cluster that is + # already failing over correctly. + assert "could not confirm" in detail_text and "may still own a live lease" in detail_text + assert "it is still the leader" not in detail_text + # ...and it says the two things an operator has to act on: a retry is what re-sends the write, + # and the node is NOT quiescent yet. + assert "retry" in detail_text.lower() + assert "cleared its leadership flag" in detail_text + assert "bounded share of the demotion budget" in detail_text + assert "unbounded" in detail_text and "connector close" in detail_text + assert "never means the node is quiescent" in detail_text + + # THREE RETIRED CLAIMS, pinned negatively because each shipped once and this test asserted two + # of them back. They are separate defects and must not be collapsed into one assertion. + # + # 1. "stopped serving" — Engine._on_demote_edge only sets _graph_wake and deliberately does not + # set the runner's _stop, so a node answering this 503 is still bound to its port and still + # ACKing. An operator who read it would begin maintenance on a live node. + assert "stopped serving" not in detail_text + # 2. "listeners keep accepting until it completes" — the ordering refutes it. + # RegistryRunner._teardown_body runs _stop_sources_demote LAST of the three BOUNDED demote + # phases and only THEN reaches the unbounded connector-close, executor-shutdown and + # sandbox-close phases; MLLP/TCP/HTTP/X12 each call server.close() in their stop()'s + # synchronous prologue. Accept stops EARLIER than that sentence said, not later. + assert "keep accepting until" not in detail_text + # 3. "started tearing its graph down" — DbCoordinator.step_down_leadership fires the demotion + # edge only under `if was_leader`, which a retry of an owed write has already cleared. On a + # repeat refusal no edge fires and no teardown starts, so this body may not assert one did. + assert "started tearing its graph down" not in detail_text + + # No cluster_stepdown row: nothing was stepped down, and a row carrying was_leader would be + # answering the wrong question. The denied row records what actually happened. + assert not await _rows(engine, "cluster_stepdown") + denied = await _rows(engine, "cluster_stepdown_denied") + assert len(denied) == 1 + detail = json.loads(str(denied[0]["detail"])) + assert detail["node_id"] == "node-a" and detail["reason"] == "release-unconfirmed" + assert denied[0]["actor"] == "boss" + + +async def test_a_refusal_survives_an_audit_write_that_fails_with_the_store(tmp_path: Path) -> None: + # THE REFUSAL'S OWN AUDIT ROW GOES THROUGH THE STORE THAT PRODUCED THE REFUSAL. A + # `release-unconfirmed` is raised only when the lease-expiring write did not return, and on + # Postgres build_coordinator hands the coordinator `store._pool` -- the same pool record_audit + # borrows. So the arm most likely to reach this write is exactly the one whose store is sick. + # + # Unguarded, that raise unwound past the handler's own `raise HTTPException(503, ...)` and the + # app's catch-all (`@app.exception_handler(Exception)`) answered a bare 500 "internal error": no + # reason, no remedy text, and no row either, on a node that has already cleared its leadership flag + # and may still own a live lease no standby can take. The row is unwritable on both paths, so the + # guard trades nothing for it. + # + # VACUITY CONTROL, MEASURED: remove the try/except around record_audit in the handler's `_denied` + # and this fails -- but NOT at the status assertion, and the difference is worth knowing before you + # read a failure here. The measured failure is the RuntimeError propagating out of the POST itself: + # Starlette routes a handler registered for Exception through ServerErrorMiddleware, which sends + # the 500 and then RE-RAISES so a server can log it, and httpx.ASGITransport surfaces that raise + # rather than the response. A real client over uvicorn gets the 500; this harness gets the + # exception. Either way the composed refusal is gone, which is what the assertions below pin. + coord = _StandinCoordinator( + raises=StepdownReleaseUnconfirmed("the lease-expiring write did not return") + ) + async with _admin(tmp_path, coord) as (engine, c, boss): + real = engine.store.record_audit + + async def _sick_store(action: str, **kwargs: Any) -> None: + # Only the denied row fails, so this stands in for a store that broke during the release + # rather than one that was never usable -- the auth rows written earlier still landed. + if action == "cluster_stepdown_denied": + raise RuntimeError("the audit write went to the pool that had already failed") + await real(action, **kwargs) + + engine.store.record_audit = _sick_store # type: ignore[method-assign] + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + + # The composed refusal survives intact: the status a caller branches on, and the remedy. + assert r.status_code == 503, "a failed audit write replaced the refusal with a bare 500" + detail_text = r.json()["detail"] + assert "could not confirm" in detail_text and "retry" in detail_text.lower() + + # And the row really is gone. The guard keeps the answer; it does not rescue the row, and the + # handler's docstring says so rather than leaving a reader to infer the row always lands. + engine.store.record_audit = real # type: ignore[method-assign] + assert not await _rows(engine, "cluster_stepdown_denied") + + +async def test_a_lock_timeout_is_its_own_503_and_asserts_no_leadership(tmp_path: Path) -> None: + # THE SECOND RAISE SITE, which used to share the first one's body and audit reason. It fires BEFORE + # any release runs -- no lease row read, none written, nothing demoted -- and, because the handler + # deliberately takes no is_leader() pre-read, it can come back from a node that leads nothing. So + # every sentence the other branch owes the operator is wrong here, and one shared arm gave them + # both anyway. + coord = _StandinCoordinator( + leader=False, # the node this refusal can reach: it leads nothing + raises=StepdownLockTimeout("the leadership lock was still held at the fence timeout"), + ) + async with _admin(tmp_path, coord) as (engine, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 503 + detail_text = r.json()["detail"] + + # It names the lock and says nothing ran. It must NOT claim this node leads, must NOT claim it + # demoted, and must not borrow the release branch's language. + assert "leadership lock" in detail_text and "changed nothing" in detail_text + assert "still the leader" not in detail_text + assert "demoted itself" not in detail_text and "could not confirm" not in detail_text + + assert not await _rows(engine, "cluster_stepdown") + denied = await _rows(engine, "cluster_stepdown_denied") + assert len(denied) == 1 + detail = json.loads(str(denied[0]["detail"])) + # A DISTINCT reason, so the audit log can tell an operator which condition they hit. One reason + # for both would make "was this node drained?" unanswerable from the record. + assert detail["reason"] == "lock-timeout" + + +async def test_stepdown_is_503_without_an_engine(tmp_path: Path) -> None: + # 503 when no engine is bound — the embedded / not-yet-started shape. Auth still needs a store, but + # the app is built with engine=None, so there is deliberately no Engine here at all. + store = await MessageStore.open(tmp_path / "no-engine.db") + try: + service = await _service(store) + await _add(service, "boss", Role.ADMINISTRATOR) + async with _client(None, service) as c: + boss = await _login(c, "boss") + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 503 + finally: + await store.close() + + +async def test_mfa_pending_session_cannot_step_down(tmp_path: Path) -> None: + # ADR 0056's decision table asks for step-up PLUS TOTP MFA. require_step_up is the composite that + # supplies both, so an MFA-required session that has not verified its second factor is refused with + # X-MFA-Required before anything else — and the coordinator is never called. + coord = _StandinCoordinator() + settings = AuthSettings(require_mfa=True, login_rate_limit_enabled=False) + async with _admin(tmp_path, coord, settings) as (_eng, c, boss): + r = await c.post("/cluster/stepdown", headers=_auth(boss), json={}) + assert r.status_code == 403 + assert r.headers.get("X-MFA-Required") == "1" + assert coord.step_down_calls == 0 diff --git a/tests/test_cluster.py b/tests/test_cluster.py index 526fbde73..5b117fd07 100644 --- a/tests/test_cluster.py +++ b/tests/test_cluster.py @@ -104,6 +104,12 @@ async def leadership_lease(self) -> tuple[str | None, float | None]: # suite. Present so this stand-in still structurally satisfies the ClusterCoordinator protocol. return (None, None) + async def step_down_leadership(self) -> tuple[bool, float | None]: + # A follower holds no leadership to release (ADR 0056 slice 1), so the honest answer is the + # same one NullCoordinator gives. Present so this stand-in still structurally satisfies the + # ClusterCoordinator protocol. + return (False, None) + # --- NullCoordinator (the byte-identical default) --------------------------- @@ -1093,3 +1099,60 @@ class _Store: coord = SqlServerCoordinator(_Store(), "N", acquire_delay_seconds=5.0, promotable=False) assert coord._acquire_delay == 5.0 and coord._promotable is False + + +# --- ADR 0056 slice 1: the planned-failover seam (backend-agnostic units) ---- + + +async def test_null_coordinator_step_down_releases_nothing() -> None: + # Single-node holds no lease and has no standby, so the seam reports (False, None) rather than + # pretending a failover happened. POST /cluster/stepdown never reaches here (it refuses a + # single-node caller with 400 first), but the Protocol answer must still be truthful. + c = NullCoordinator("solo") + assert await c.step_down_leadership() == (False, None) + assert c.is_leader() is True # and single-node stays leader, byte-identically + + +async def test_sqlserver_step_down_mirrors_the_postgres_seam() -> None: + # The SQL Server coordinator is the DbCoordinator's lockstep sibling, so the seam must behave the + # same: expire the lease we own, report (True, released_at), demote the cached gate, drop the H1 + # token, fire the demotion edge, and pause this node's own claim so it cannot renew itself back in. + # (The DbCoordinator half is proven against the fake lease pool in tests/test_cluster_lease.py.) + from messagefoundry.pipeline.cluster_sqlserver import SqlServerCoordinator + + class _Settings: + db_schema = None + + class _Store: + _settings = _Settings() + + def __init__(self) -> None: + self.executed: list[str] = [] + + async def _execute(self, sql: str, params: object = None) -> None: + self.executed.append(sql) + + async def _fetchone(self, *a: object, **k: object) -> object: + raise AssertionError("a paused node must not query the store to claim") + + store = _Store() + coord = SqlServerCoordinator(store, "N", heartbeat_seconds=10.0, monotonic=lambda: 0.0) + fired: list[int] = [] + coord.set_on_demote(lambda: fired.append(1)) + coord._is_leader = True + coord._last_renew_ok = 0.0 + coord._leader_epoch = 3 + + was_leader, released_at = await coord.step_down_leadership() + + assert was_leader is True and released_at is not None + assert coord.is_leader() is False and coord.current_epoch() is None + assert any("UPDATE leader_lease" in sql for sql in store.executed) + assert fired == [1] + assert coord._no_claim_until == 20.0 # two heartbeats + # Inside the pause the claim short-circuits before touching the store (_fetchone would raise). + assert await coord._claim_or_renew_lease() is False + # A second stepdown releases nothing and issues no further write. + writes = len(store.executed) + assert await coord.step_down_leadership() == (False, None) + assert len(store.executed) == writes diff --git a/tests/test_cluster_lease.py b/tests/test_cluster_lease.py index c43afd8fe..708dbf40f 100644 --- a/tests/test_cluster_lease.py +++ b/tests/test_cluster_lease.py @@ -15,13 +15,29 @@ The split-brain guarantee — a partitioned old leader self-fences BEFORE a standby can acquire — is proven directly in :func:`test_fence_fires_before_standby_can_acquire`. The live behaviour against a real Postgres lands with the failover suite (Increment 3). + +The last section covers the SQL Server twin, against its own stand-in over the same shared lease row. +It is here rather than in the gated SQL Server failover suite because the defect it pins is an asyncio +ordering one, not a T-SQL one: the ``MERGE`` carries the identical unfenced ``t.owner = ?`` renew +branch, so the same interleaving re-promotes a drained node there, and a test that only runs when +``MEFOR_TEST_SQLSERVER`` is set would leave the twin unguarded on every ordinary run. """ from __future__ import annotations +import asyncio +import time +from collections.abc import Callable + import pytest -from messagefoundry.pipeline.cluster import DbCoordinator +from messagefoundry.pipeline.cluster import ( + DbCoordinator, + StepdownLockTimeout, + StepdownReleaseUnconfirmed, + StepdownUnavailable, +) +from messagefoundry.pipeline.cluster_sqlserver import SqlServerCoordinator class _Clock: @@ -36,63 +52,128 @@ def __call__(self) -> float: class _FakeLeaseDB: - """The shared single-row ``leader_lease`` table + the DB clock the lease arithmetic uses.""" + """The shared single-row ``leader_lease`` table, the DB clock the lease arithmetic uses, and the + two row mutations both backends' claim/release statements perform. + + The mutations live HERE rather than in each stand-in because the two backends run the same lease + semantics through different SQL — PG's ``INSERT ... ON CONFLICT``, T-SQL's ``MERGE ... HOLDLOCK`` — + and a per-stand-in copy is two models of one lease that can drift apart while both keep passing. + Each stand-in still asserts its OWN statement's shape; only the row arithmetic is shared. + """ def __init__(self, db_clock: _Clock) -> None: self._db_clock = db_clock # {"owner": str, "lease_expires_at": float, "leader_epoch": int} self.row: dict[str, object] | None = None + def claim(self, owner: object, ttl: float, delay: float) -> dict[str, object] | None: + """Acquire-or-renew, returning the ``(owner, leader_epoch)`` the statement would OUTPUT, or + ``None`` when another node holds a live lease. + + The H1 epoch: 1 on a fresh insert, +1 on a take-over of an expired/foreign lease, UNCHANGED on + a renew (``owner == me``). The ADR 0096 ``delay`` handicaps the take-over predicate only — it is + added to the expiry side, so a renew is never delayed. + """ + now = self._db_clock() + row = self.row + if row is None: + self.row = {"owner": owner, "lease_expires_at": now + ttl, "leader_epoch": 1} + return {"owner": owner, "leader_epoch": 1} + expired = float(row["lease_expires_at"]) + delay < now # type: ignore[arg-type] + if row["owner"] == owner or expired: + if row["owner"] != owner: + row["leader_epoch"] = int(row["leader_epoch"]) + 1 # type: ignore[arg-type] + row["owner"] = owner + row["lease_expires_at"] = now + ttl + return {"owner": owner, "leader_epoch": row["leader_epoch"]} + return None # another node holds a live lease + + def release(self, owner: object) -> None: + """Expire our own lease row (the release ``UPDATE ... WHERE lease_key AND owner``).""" + row = self.row + if row is not None and row["owner"] == owner: + row["lease_expires_at"] = 0.0 + class _FakeLeasePool: """One node's view of the pool over a shared :class:`_FakeLeaseDB`. Emulates the two statements the coordinator issues for the lease; ``fail=True`` makes every call raise to simulate this node being - partitioned from (or the DB hung for) THIS node only — the other node's pool keeps working.""" + partitioned from (or the DB hung for) THIS node only — the other node's pool keeps working. + + ``yield_in_fetchrow`` / ``yield_in_execute`` make the named statement SUSPEND before it touches the + row, which a real pool does at every round trip and this stand-in otherwise never does. They are + opt-in per test because a stand-in that never yields quietly hides every ordering defect in the code + under test: without one of these set, an ``await`` on these methods returns without ever handing + control back to the loop, so two coroutines that genuinely interleave in production run to + completion one after the other here and every concurrency test passes by construction. + + ``on_execute`` is a synchronous probe called at the instant the release statement runs. It exists + because every other test here reads ``is_leader()`` only after the whole call has returned, which is + blind to WHEN inside the call the demotion happened. + + ``hang_in_execute`` suspends the release statement on an event the test owns, BEFORE the row is + touched. ``fail`` cannot stand in for it: a raise is caught by the coordinator's ``except + Exception`` and a CANCELLATION is not, which is the whole distinction the cancellation tests pin. + """ def __init__(self, db: _FakeLeaseDB) -> None: self._db = db self.fail = False + self.yield_in_fetchrow = False + self.yield_in_execute = False + self.hang_in_execute: asyncio.Event | None = None + self.on_execute: Callable[[], None] | None = None + # Records the ARGUMENTS of each release statement, which ``on_execute`` cannot: the + # release-retry tests ask "was a second UPDATE sent at all", and a row that already reads + # released cannot distinguish a re-sent write from a write that never happened twice. + self.on_execute_args: Callable[[tuple[object, ...]], None] | None = None async def fetchrow(self, sql: str, *args: object) -> dict[str, object] | None: + if self.yield_in_fetchrow: + await asyncio.sleep(0) # the claim round trip is in flight; let another task run if self.fail: raise RuntimeError("partitioned from db") - # Mirrors _claim_or_renew_lease's INSERT ... ON CONFLICT ... WHERE owner OR expired RETURNING, - # INCLUDING the H1 leader_epoch maintenance: epoch 1 on a fresh INSERT, +1 on a take-over of an - # expired/foreign lease, UNCHANGED on a renew (owner == me). RETURNS owner + leader_epoch. - # The 4th arg is the ADR-0096 acquire_delay: a take-over requires the lease to have been expired - # for `delay` seconds (added to the expiry side); a renew (owner == me) is never delayed. + # Mirrors _claim_or_renew_lease's INSERT ... ON CONFLICT ... WHERE owner OR expired RETURNING. + # The 4th arg is the ADR-0096 acquire_delay. assert "leader_lease" in sql and "INSERT" in sql assert "leader_epoch" in sql, "claim SQL must maintain the H1 fencing epoch" assert "$4" in sql, "claim SQL must carry the acquire_delay handicap param" _lease_key, owner, ttl, delay = args - now = self._db._db_clock() - row = self._db.row - if row is None: - self._db.row = { - "owner": owner, - "lease_expires_at": now + float(ttl), # type: ignore[arg-type] - "leader_epoch": 1, # fresh acquire on an empty table - } - return {"owner": owner, "leader_epoch": 1} - expired = float(row["lease_expires_at"]) + float(delay) < now # type: ignore[arg-type] - if row["owner"] == owner or expired: - # Renew (owner == me) keeps the epoch; a take-over of an expired/foreign lease bumps it. - if row["owner"] != owner: - row["leader_epoch"] = int(row["leader_epoch"]) + 1 # type: ignore[arg-type] - row["owner"] = owner - row["lease_expires_at"] = now + float(ttl) # type: ignore[arg-type] - return {"owner": owner, "leader_epoch": row["leader_epoch"]} - return None # another node holds a live lease + return self._db.claim(owner, float(ttl), float(delay)) # type: ignore[arg-type] async def execute(self, sql: str, *args: object) -> None: + if self.yield_in_execute: + await asyncio.sleep(0) # the release round trip is in flight; let another task run + if self.hang_in_execute is not None: + # Suspended INSIDE the write and before the row moves, which is where a request deadline + # cancels a real one. Nothing sets this event; the test cancels the awaiting task instead. + await self.hang_in_execute.wait() + if self.on_execute is not None: + self.on_execute() # a reader observing the coordinator DURING the release window + if self.on_execute_args is not None: + self.on_execute_args(args) # a counter of the statements actually SENT if self.fail: raise RuntimeError("partitioned from db") # Mirrors _release_leadership's UPDATE ... SET lease_expires_at=0 WHERE lease_key AND owner. assert "leader_lease" in sql and "UPDATE" in sql _lease_key, owner = args - row = self._db.row - if row is not None and row["owner"] == owner: - row["lease_expires_at"] = 0.0 + self._db.release(owner) + + +async def _cancel_once_suspended(task: asyncio.Task[object]) -> None: + """Let ``task`` reach its suspended write, then cancel it there and absorb the CancelledError. + + The loop is why this is a helper rather than a bare ``cancel()``: cancelling after a single + scheduler pass would cancel a coroutine that had not yet reached the pool, which proves nothing + about a write cancelled mid-flight. The ``done()`` check is the control — without it a test that + cancelled too early, or too late, would still pass. + """ + for _ in range(5): + await asyncio.sleep(0) + assert not task.done(), "the call never reached the suspended write, so nothing was cancelled" + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task def _coord( @@ -102,12 +183,14 @@ def _coord( node: str = "A", ttl: float = 30.0, fence: float = 20.0, + heartbeat: float = 10.0, acquire_delay_seconds: float = 0.0, promotable: bool = True, ) -> DbCoordinator: return DbCoordinator( pool, node, + heartbeat_seconds=heartbeat, leader_lease_ttl_seconds=ttl, leader_fence_timeout_seconds=fence, acquire_delay_seconds=acquire_delay_seconds, @@ -465,3 +548,783 @@ async def test_promotable_standby_takes_over_from_non_promotable_gap() -> None: await ha._maintain_leadership() # HA acquires the empty lease assert ha.is_leader() is True assert db.row is not None and db.row["owner"] == "HA" + + +# --- ADR 0056 slice 1: planned failover (step_down_leadership) --------------- + + +async def test_step_down_returns_the_release_and_expires_the_lease() -> None: + # The public seam reports what it actually did — (was_leader, released_at) — and expires the lease + # row exactly as the clean-stop release does. released_at is wall-clock (the audit trail's units), + # not the injected monotonic clock, so it is only bracketed here. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + a = _coord(_FakeLeasePool(db), _Clock(0.0), node="A") + await a._maintain_leadership() + assert a.is_leader() is True + + before = time.time() + was_leader, released_at = await a.step_down_leadership() + after = time.time() + + assert was_leader is True + assert released_at is not None and before <= released_at <= after + assert a.is_leader() is False + assert a.current_epoch() is None # released: no longer a fenced leader (H1) + assert db.row is not None and db.row["lease_expires_at"] == 0.0 + + +async def test_step_down_on_a_non_leader_releases_nothing() -> None: + # The endpoint's 409 rests on this: a node that never held the lease reports (False, None) and + # leaves a live sibling's lease untouched. This is what makes the pre-read unnecessary. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + a = _coord(_FakeLeasePool(db), _Clock(0.0), node="A") + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B") + await a._maintain_leadership() # A leads + + assert await b.step_down_leadership() == (False, None) + assert a.is_leader() is True + assert db.row is not None and db.row["owner"] == "A" + assert db.row["lease_expires_at"] == 30.0 # untouched + + +async def test_step_down_pauses_this_node_so_a_standby_wins_the_expired_lease() -> None: + # THE REGRESSION THIS PAUSE EXISTS FOR. _release_leadership expires lease_expires_at but leaves + # `owner` naming us, and the claim statement's renew branch (owner = me) carries NO expiry test — + # so without the pause the drained node's very next maintenance tick renews and takes leadership + # straight back, whichever node happens to tick first. The negative control below proves the fake + # pool really would hand it back, so this is not a vacuous pass. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + mono_a = _Clock(0.0) + a = _coord(_FakeLeasePool(db), mono_a, node="A", heartbeat=10.0) + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", heartbeat=10.0) + await a._maintain_leadership() + + await a.step_down_leadership() + assert a._no_claim_until == 20.0 # two heartbeats on the injected monotonic clock + + # A's own next tick, inside the window: it declines rather than renewing itself back in. + db_clock.t = mono_a.t = 10.0 + await a._maintain_leadership() + assert a.is_leader() is False + assert db.row is not None and db.row["owner"] == "A" # row untouched, still expired + assert db.row["lease_expires_at"] == 0.0 + + # The standby's tick inside the same window wins the expired lease and bumps the epoch (H1). + await b._maintain_leadership() + assert b.is_leader() is True + assert db.row["owner"] == "B" and db.row["leader_epoch"] == 2 + + # And the pause is bounded: past it, A contends normally again (it just cannot beat a live lease). + mono_a.t = 21.0 + await a._maintain_leadership() + assert a.is_leader() is False # B's lease is live, so the ordinary predicate refuses A + + +async def test_without_the_pause_the_drained_node_renews_itself_back_in() -> None: + # NEGATIVE CONTROL for the test above: clear the pause and the same sequence hands leadership + # straight back to the node that was just drained. If this ever stops reproducing, the pause is no + # longer measuring what it claims to. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + mono_a = _Clock(0.0) + a = _coord(_FakeLeasePool(db), mono_a, node="A", heartbeat=10.0) + await a._maintain_leadership() + await a.step_down_leadership() + + a._no_claim_until = 0.0 # the pause removed + db_clock.t = mono_a.t = 10.0 + await a._maintain_leadership() + assert a.is_leader() is True # re-armed itself; the planned failover did nothing + + +async def test_a_slow_release_write_is_not_spent_out_of_the_claim_pause() -> None: + # THE PAUSE IS MEASURED FROM WHEN THE WRITE RETURNED, NOT FROM BEFORE IT. The lease row only + # becomes takeable at that write's commit, so a pause armed only ahead of it gives a sibling + # (2 * heartbeat - write duration), not two heartbeats. Nothing bounds that duration from the + # coordinator: the pool acquire() carries no timeout and [store].command_timeout — 30s by default — + # already outlasts the 20s pause at the shipped heartbeat, and the cancellation tests below rest on + # the same write reaching a 120s request deadline. Spend the pause inside the write and the drained + # node's own next tick falls through the gate at _claim_or_renew_lease, matches the unfenced + # `owner = me` renew branch over the row it just expired, and is leader again moments after the + # endpoint answered 200. + # + # Why no test above can see it: _Clock only moves when a test moves it, and none of them moves it + # across the release, so every other release here costs zero simulated time. + # + # VACUITY CONTROL, both legs MEASURED: delete the second arm (the `max(...)` after + # _release_leadership returns) from DbCoordinator.step_down_leadership and this fails at the pause + # (`20.0 == 45.0`); silence that leg as well and it fails at the behavioural one (`True is False`), + # the drained node having taken its own leadership back. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + pool = _FakeLeasePool(db) + mono = _Clock(0.0) + a = _coord(pool, mono, node="A", heartbeat=10.0) + await a._maintain_leadership() + + def _slow_write() -> None: + # 25 simulated seconds pass INSIDE the write — past the 20.0 the first arm promised. + mono.t = db_clock.t = 25.0 + + pool.on_execute = _slow_write + await a.step_down_leadership() + + assert a._no_claim_until == 45.0, "the release write was spent out of the claim pause" + + # The consequence that number stands for: A's own next tick still declines, so the drain holds. + await a._maintain_leadership() + assert a.is_leader() is False, "the drained node renewed itself back in over a spent pause" + assert db.row is not None and db.row["owner"] == "A" # row untouched, still expired + assert db.row["lease_expires_at"] == 0.0 + + # And the sibling still gets its tick at the expired lease, which is what the pause is sized for. + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", heartbeat=10.0) + await b._maintain_leadership() + assert b.is_leader() is True + + +async def test_step_down_fires_the_demotion_edge_and_leaves_the_node_running() -> None: + # A stepdown is a demotion the node SURVIVES, so the engine must learn about it on the same edge + # every other True->False transition uses (ADR 0157 Inc 5) rather than waiting out a reconcile + # poll. And the coordinator's background tasks are untouched — this is not a stop(). + db = _FakeLeaseDB(_Clock(0.0)) + a = _coord(_FakeLeasePool(db), _Clock(0.0), node="A") + fired: list[int] = [] + a.set_on_demote(lambda: fired.append(1)) + await a._maintain_leadership() + + await a.step_down_leadership() + assert fired == [1] + assert a._stop.is_set() is False # still running; the maintenance loop keeps heartbeating + + # A second stepdown on the now-demoted node releases nothing and fires nothing more. + assert await a.step_down_leadership() == (False, None) + assert fired == [1] + + +async def test_a_tick_inside_the_release_window_cannot_re_promote_the_drained_node() -> None: + # THE TWO-LEADER WINDOW. A stepdown runs from an API handler with the maintenance loop LIVE, and + # _release_leadership() SUSPENDS at its UPDATE. A tick that starts in that window matches the claim + # statement's unfenced `owner = me` renew branch, takes the lease back, and flips _is_leader on + # again — after which the release's own UPDATE expires the row it just renewed. The node then + # reports leader while a sibling can take the expired lease, so BOTH consider themselves leader, + # and the endpoint answered 200. + # + # THIS TEST DOES NOT ISOLATE THE LOCK, and an earlier comment here claiming "the lock is what + # closes it" read a conjunction as one term. Measured by reverting one mechanism at a time: + # removing the claim pause kills this test, removing the mutual exclusion does NOT. The pause is + # armed before the release, and it is the first thing the tick checks, so a tick that STARTS in the + # release window is turned away by the pause and never reaches the lock at all. + # The test below is the one that isolates the lock: it puts the claim in flight BEFORE the pause is + # armed, so the pause cannot close it and only mutual exclusion can. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + mono_a = _Clock(0.0) + pool_a = _FakeLeasePool(db) + a = _coord(pool_a, mono_a, node="A", heartbeat=10.0) + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", heartbeat=10.0) + await a._maintain_leadership() + assert a.is_leader() is True + + pool_a.yield_in_execute = True # the release suspends mid-UPDATE, as a real pool does + await asyncio.gather(a.step_down_leadership(), a._maintain_leadership()) + + assert a.is_leader() is False, "a tick in the release window re-promoted the drained node" + assert db.row is not None and db.row["lease_expires_at"] == 0.0 # the release still won the row + + # And the drain actually transfers: the standby takes the expired lease and is the ONLY leader. + db_clock.t = 1.0 + await b._maintain_leadership() + assert b.is_leader() is True + assert a.is_leader() is False, "two leaders at once" + + +async def test_a_claim_already_in_flight_cannot_re_promote_after_the_release() -> None: + # The OTHER interleaving, and the one that decides the fix. Here the maintenance tick is already + # suspended inside its claim round trip when the stepdown begins, so it has ALREADY passed the + # _no_claim_until check. The pause IS now armed before the release, and it still changes nothing + # here: the claim returns "held" afterwards and _maintain_leadership promotes on that stale result, + # leaving the node leader with a LIVE lease no sibling can take for a full TTL. Only mutual + # exclusion orders these two, and removing it is measured to kill this test and no other. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + mono_a = _Clock(0.0) + pool_a = _FakeLeasePool(db) + a = _coord(pool_a, mono_a, node="A", heartbeat=10.0) + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", heartbeat=10.0) + await a._maintain_leadership() + assert a.is_leader() is True + + pool_a.yield_in_fetchrow = True # the claim is in flight when the stepdown arrives + await asyncio.gather(a._maintain_leadership(), a.step_down_leadership()) + + assert a.is_leader() is False, "an in-flight claim re-promoted the drained node" + assert db.row is not None and db.row["lease_expires_at"] == 0.0, ( + "the release must win the row; a renew landing after it leaves the lease live for a full TTL" + ) + + db_clock.t = 1.0 + await b._maintain_leadership() + assert b.is_leader() is True + assert a.is_leader() is False, "two leaders at once" + + +async def test_a_failed_release_write_reports_failure_instead_of_a_drain() -> None: + # REPLACES test_step_down_survives_a_failed_release_write, which asserted the DEFECT as correct. + # That test read the release write as "best-effort — the lease ages out on its own", which is true + # of stop() (the node is leaving) and false of a stepdown (the node stays up). With the pool + # partitioned the UPDATE never lands, so the lease row stays LIVE and still owned by this node: no + # sibling can take it, and when the pause ends this node renews itself back in through the unfenced + # `owner = me` branch. The old test asserted (True, released_at), which the endpoint turned into a + # 200 reading "drained" — telling an operator to start maintenance on the node that is still leader. + # + # Arithmetic on the shipped defaults, which is why the pause does not save it: heartbeat 10, fence + # 20, ttl 30, so the pause ends at 20 while the lease lives to 30. The settings validator pins + # heartbeat < fence < ttl and never compares the pause to the ttl. + db = _FakeLeaseDB(_Clock(0.0)) + pool = _FakeLeasePool(db) + mono = _Clock(0.0) + a = _coord(pool, mono, node="A", heartbeat=10.0) + await a._maintain_leadership() + + pool.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + + # The conservative half still holds: this node stops CALLING itself leader either way, and the + # pause is armed, because a lost response to a committed UPDATE is indistinguishable from an + # UPDATE that never ran and the possibly-released reading is the safe one. + assert a.is_leader() is False + assert a._no_claim_until == 20.0 + # ...and the fact the caller must be told: the lease row was NOT expired. + assert db.row is not None and db.row["lease_expires_at"] == 30.0 + + # The consequence a 200 would have hidden. The lease outlives the pause, so past it this node takes + # its own leadership back and the "drained" node is the leader again. + pool.fail = False + mono.t = 21.0 + await a._maintain_leadership() + assert a.is_leader() is True + + +async def test_a_retry_re_sends_the_write_the_first_stepdown_could_not_confirm() -> None: + # THE REFUSAL THE PREVIOUS TEST PINS IS ONLY HALF AN ANSWER: it tells the operator to retry, and + # the retry has to work. _release_leadership demotes the in-memory gate BEFORE the write, so on the + # second call `was_leader` reads False and its not-a-leader early return fired — the UPDATE was + # never re-sent, the caller got (False, None), and the endpoint turned that into a 409 "this node + # is not the current leader" over a lease row still live and still owned by that very node. The + # 409's own documented remedy (resolve the leader from GET /cluster/nodes) then pointed straight + # back here, because this node IS what that API still names as lease owner. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + pool = _FakeLeasePool(db) + mono = _Clock(0.0) + a = _coord(pool, mono, node="A", heartbeat=10.0) + await a._maintain_leadership() + + pool.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + assert db.row is not None and db.row["lease_expires_at"] == 30.0 # live, and ours + + # THE MEASUREMENT. Count the release statements the pool sees, so "the retry re-sent it" is read + # off the wire rather than inferred from the row. Reverting force_write leaves this at 1 and the + # row at 30.0 — the vacuity control for this test. + releases: list[tuple[object, ...]] = [] + pool.fail = False + pool.on_execute_args = releases.append + assert await a.step_down_leadership() == (False, None) + assert len(releases) == 1, "the retry did not re-send the release write" + assert db.row["lease_expires_at"] == 0.0, "the retry did not expire the lease it still owned" + + # And the pause is re-armed on the retry, for the same reason it is armed on the first call: the + # release expires the lease but leaves `owner` naming us, so the unfenced `owner = me` renew branch + # would otherwise hand leadership straight back on this node's very next tick. + assert a._no_claim_until == 20.0 + await a._maintain_leadership() + assert a.is_leader() is False + + # A sibling can now take it, which is the whole point of retrying. + db_clock.t = 1.0 # the expired lease is only takeable once the DB clock is past it + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B") + await b._maintain_leadership() + assert b.is_leader() is True + + +async def test_the_retry_arms_a_fresh_pause_measured_from_the_retrys_own_clock() -> None: + # THE `or owed` DISJUNCT, which the two retry tests around this one cannot see. They assert + # `_no_claim_until == 20.0` after the retry and read that as evidence the retry re-armed the pause, + # but their monotonic clock never moves: 20.0 is already there from the FIRST, failed stepdown, so + # the assertion holds identically with the disjunct deleted. Deleting it leaves this whole file, + # test_cluster.py and test_api_cluster_stepdown.py passing. Advancing the clock past that first + # pause is what separates the two answers. + # + # What the disjunct holds up: the retry's write DOES land — force_write sends it past the + # not-a-leader early return — expiring `lease_expires_at` while leaving `owner` naming this node. + # Without a fresh pause the very next tick matches the unfenced `owner = me` renew branch and hands + # leadership back to the node the endpoint has already reported drained. + # + # VACUITY CONTROL, both legs MEASURED: delete `or owed` from `arming` in + # DbCoordinator.step_down_leadership and this fails at the pause (`20.0 == 120.0`); silence that + # leg as well and it fails at the behavioural one (`True is False`). + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + pool = _FakeLeasePool(db) + mono = _Clock(0.0) + a = _coord(pool, mono, node="A", heartbeat=10.0) + await a._maintain_leadership() + + pool.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + assert a._no_claim_until == 20.0 # armed by the first call, on a clock still at 0.0 + + # PAST that first pause, which is the step no other retry test takes. + mono.t = db_clock.t = 100.0 + pool.fail = False + assert await a.step_down_leadership() == (False, None) + + assert a._no_claim_until == 120.0, "the retry did not re-arm the claim pause" + await a._maintain_leadership() + assert a.is_leader() is False, "the drained node re-armed itself as leader after the retry" + + # And the sibling takes the lease the retry finally expired. + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", heartbeat=10.0) + await b._maintain_leadership() + assert b.is_leader() is True + + +async def test_a_retry_that_fails_again_refuses_rather_than_answering_not_the_leader() -> None: + # The same early return also SWALLOWED a second failure. With the write forced but still failing, + # the retry must raise again — not return (False, None), which the endpoint renders as a 409 + # meaning "you addressed the wrong node" for a node that may still hold a live lease. + db = _FakeLeaseDB(_Clock(0.0)) + pool = _FakeLeasePool(db) + a = _coord(pool, _Clock(0.0), node="A", heartbeat=10.0) + await a._maintain_leadership() + + pool.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + assert db.row is not None and db.row["lease_expires_at"] == 30.0 + + +async def test_a_cancelled_release_still_owes_the_write_so_the_retry_re_sends_it() -> None: + # THE HOLE THE TWO TESTS ABOVE DID NOT COVER. They partition the pool, so the write RAISES and the + # coordinator's `except Exception` arm records the owed write. A CANCELLATION takes neither arm: + # asyncio.CancelledError derives from BaseException, so `except Exception` never sees it and the + # method unwound with _is_leader already cleared and _lease_release_owed still False. The next + # stepdown then read owed=False, took the not-a-leader early return, sent NO write, and answered + # 409 "not the current leader" over a lease row still live and still ours — the exact defect the + # retry mechanism exists to prevent, reached by a different door and with no audit row either way. + # + # Not hypothetical in the shipped configuration: RequestTimeoutMiddleware is registered + # unconditionally and its asyncio.timeout cancels the handler at DEFAULT_REQUEST_TIMEOUT_SECONDS + # (120.0), over a pool acquire the stepdown docstring documents as unbounded. + # + # VACUITY CONTROL, both legs MEASURED rather than reasoned: move the `self._lease_release_owed = + # True` in _release_leadership back into its `except Exception` arm and this test fails at the + # owed assertion (`False is True`); silence that one line as well and it fails at the release + # count instead (`0 == 1`), which is the leg that proves the retry really did send nothing. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + pool = _FakeLeasePool(db) + mono = _Clock(0.0) + a = _coord(pool, mono, node="A", heartbeat=10.0) + await a._maintain_leadership() + assert a.is_leader() is True + + pool.hang_in_execute = ( + asyncio.Event() + ) # never set: the write suspends until the task is cancelled + await _cancel_once_suspended(asyncio.create_task(a.step_down_leadership())) + + # The in-memory demotion happened (it precedes the write) and the row never moved, which is exactly + # the state the owed flag exists to record. + assert a.is_leader() is False + assert db.row is not None and db.row["lease_expires_at"] == 30.0, "the cancelled write landed" + assert a._lease_release_owed is True, "a cancelled write left nothing owed" + + # THE MEASUREMENT: the retry re-sends the write, counted off the wire rather than inferred. + releases: list[tuple[object, ...]] = [] + pool.hang_in_execute = None + pool.on_execute_args = releases.append + assert await a.step_down_leadership() == (False, None) + assert len(releases) == 1, "the retry after a cancelled release sent no write" + assert db.row["lease_expires_at"] == 0.0, "the retry did not expire the lease it still owned" + assert a._lease_release_owed is False, "a write that returned must clear the owed flag" + + # And the pause is armed on that retry too, so the released lease is not re-taken by this node on + # its next tick — the same reason the raise-path retry arms it. + assert a._no_claim_until == 20.0 + db_clock.t = 1.0 + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B") + await b._maintain_leadership() + assert b.is_leader() is True + + +async def test_a_stepdown_on_a_node_that_never_led_sends_nothing_and_arms_no_pause() -> None: + # The counterweight to the two tests above: forcing the write is scoped to a release this node + # OWES, never to every stepdown. A caller who addresses a standby by mistake must not cost that + # standby a DB round trip or two heartbeats of declining to claim — that would delay the very + # failover they are trying to perform. + db = _FakeLeaseDB(_Clock(0.0)) + a = _coord(_FakeLeasePool(db), _Clock(0.0), node="A", heartbeat=10.0) + b_pool = _FakeLeasePool(db) + b = _coord(b_pool, _Clock(0.0), node="B", heartbeat=10.0) + await a._maintain_leadership() # A leads; B never has + + releases: list[tuple[object, ...]] = [] + b_pool.on_execute_args = releases.append + assert await b.step_down_leadership() == (False, None) + assert releases == [], "a stepdown on a node that owes no release still wrote to the lease row" + assert b._no_claim_until == 0.0, "an innocent standby was handicapped by someone else's mistake" + assert a.is_leader() is True + + +async def test_the_release_demotes_before_it_writes() -> None: + # ORDERING GUARD. _release_leadership's first line is the SYNCHRONOUS `self._is_leader = False`, + # ahead of the awaited lease write, so no reader can see a stale True while the release is in + # flight — is_leader() gates listener binding and the whole graph. Moving that assignment after the + # await passes every other test in this file and on both backends, because they all read + # is_leader() only once the call has returned; only a probe INSIDE the release window sees it. + db = _FakeLeaseDB(_Clock(0.0)) + pool = _FakeLeasePool(db) + a = _coord(pool, _Clock(0.0), node="A") + await a._maintain_leadership() + assert a.is_leader() is True + + seen: list[bool] = [] + pool.on_execute = lambda: seen.append(a.is_leader()) + await a.step_down_leadership() + assert seen == [False], "a reader inside the release window saw the node still reporting leader" + + +async def test_a_cancelled_stepdown_still_arms_the_claim_pause() -> None: + # The pause is armed BEFORE the release's await, not after it. Cancel the request task while the + # release is suspended in the pool write and `async with` unwinds correctly — but an assignment + # placed after that await never runs, leaving _no_claim_until at 0.0 on a node whose lease row may + # already be expired. The endpoint's handler is a bare await with no shield and no timeout, so any + # client disconnect or server shutdown lands exactly there. + db = _FakeLeaseDB(_Clock(0.0)) + pool = _FakeLeasePool(db) + a = _coord(pool, _Clock(0.0), node="A", heartbeat=10.0) + await a._maintain_leadership() + + pool.yield_in_execute = True # suspend inside the release, then cancel there + task = asyncio.ensure_future(a.step_down_leadership()) + await asyncio.sleep(0) # let the task reach the suspension point + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert a._no_claim_until == 20.0, "a cancelled stepdown skipped the pause" + assert a.is_leader() is False + # And the pause does its job: this node's own next tick declines rather than renewing itself in. + await a._maintain_leadership() + assert a.is_leader() is False + + +async def test_the_lock_wait_is_bounded_and_refuses_rather_than_demoting() -> None: + # The lock puts the in-memory demote behind a DB round trip: a maintenance tick suspended in + # fetchrow holds it, and step_down_leadership blocks BEFORE _release_leadership's first line. That + # wait was unbounded — [store].command_timeout is the only ceiling and PostgresStore passes + # `command_timeout or None`, so the documented zero-disables value removes even that, while the raw + # pool acquire() carries no timeout at all. It is bounded here at the fence timeout, and a timeout + # REFUSES: it must not demote a node whose release it never ran. + db = _FakeLeaseDB(_Clock(0.0)) + pool = _FakeLeasePool(db) + a = _coord(pool, _Clock(0.0), node="A", fence=0.05) + await a._maintain_leadership() + + await a._leadership_lock.acquire() # stand in for a tick suspended mid-round-trip + try: + with pytest.raises(StepdownLockTimeout) as caught: + await a.step_down_leadership() + finally: + a._leadership_lock.release() + + assert a.is_leader() is True, "a refused stepdown must leave leadership exactly as it found it" + assert a._no_claim_until == 0.0 + assert db.row is not None and db.row["lease_expires_at"] == 30.0 + # ITS OWN TYPE, not the write-failure one. Both used to be StepdownUnavailable, so the endpoint's + # single `except` arm gave both the same body ("could not release leadership; it is still the + # leader") and the same audit reason — a sentence that is false here twice over: nothing was + # released, and this branch runs before any leader check, so it fires on a node that leads nothing. + assert not isinstance(caught.value, StepdownReleaseUnconfirmed) + assert isinstance(caught.value, StepdownUnavailable) # still one family for a catch-all caller + text = str(caught.value) + assert "maintenance tick" not in text, ( + "the message names a maintenance tick as the holder; both coordinators take this lock in " + "_maintain_leadership AND in step_down_leadership, so the holder is not knowable from here" + ) + assert "leadership lock" in text and "still the leader" not in text + + +async def test_a_stepdown_refused_by_the_lock_can_come_from_a_node_that_leads_nothing() -> None: + # WHY THE LOCK-TIMEOUT WORDING MAY NOT ASSERT LEADERSHIP. The endpoint deliberately takes no + # is_leader() pre-read, so this refusal reaches a caller who addressed a standby whose lock happens + # to be busy. Nothing in that path ever read who the leader is. + db = _FakeLeaseDB(_Clock(0.0)) + a = _coord(_FakeLeasePool(db), _Clock(0.0), node="A") + await a._maintain_leadership() + b = _coord(_FakeLeasePool(db), _Clock(0.0), node="B", fence=0.05) + assert b.is_leader() is False + + await b._leadership_lock.acquire() + try: + with pytest.raises(StepdownLockTimeout) as caught: + await b.step_down_leadership() + finally: + b._leadership_lock.release() + + assert "still the leader" not in str(caught.value) + assert a.is_leader() is True and b.is_leader() is False + + +# --- ADR 0056 slice 1: the SQL Server twin ---------------------------------- + + +class _FakeSqlLeaseStore: + """The SQL Server sibling of :class:`_FakeLeasePool` over the SAME :class:`_FakeLeaseDB`. + + Emulates only the two statements ``SqlServerCoordinator`` issues for the lease: the + ``MERGE ... WHEN MATCHED AND (t.owner = ? OR t.lease_expires_at + ? < @now)`` acquire/renew + (``_fetchone``) and the release ``UPDATE`` (``_execute``), with the same opt-in suspension and + release-window probe the Postgres stand-in carries and for the same reasons — a stand-in that never + yields cannot exhibit an ordering defect, and a test that reads state only after the call cannot see + where inside it the demotion landed. + + **``_execute`` carries the same hooks as its Postgres sibling on purpose.** Without them the + release-window interleaving simply cannot be EXPRESSED against this backend, so a claim that both + interleavings are pinned on both coordinators would have been half true with nothing failing. + """ + + _settings = None + + def __init__(self, db: _FakeLeaseDB) -> None: + self._db = db + self.fail = False + self.yield_in_fetchone = False + self.yield_in_execute = False + self.on_execute: Callable[[], None] | None = None + # Read _FakeLeasePool.hang_in_execute: a raise and a cancellation take different arms. + self.hang_in_execute: asyncio.Event | None = None + + async def _fetchone(self, sql: str, params: tuple[object, ...]) -> dict[str, object] | None: + if self.yield_in_fetchone: + await asyncio.sleep(0) # the MERGE round trip is in flight; let another task run + if self.fail: + raise RuntimeError("partitioned from db") + assert "MERGE leader_lease" in sql, "not the claim statement" + assert "leader_epoch" in sql, "claim SQL must maintain the H1 fencing epoch" + # Positional params of the MERGE: (lease_key, owner, delay, owner, ttl, owner, ...). + owner, delay, ttl = params[1], params[2], params[4] + return self._db.claim(owner, float(ttl), float(delay)) # type: ignore[arg-type] + + async def _execute(self, sql: str, params: tuple[object, ...]) -> None: + if self.yield_in_execute: + await asyncio.sleep(0) # the release round trip is in flight; let another task run + if self.hang_in_execute is not None: + await self.hang_in_execute.wait() # suspended inside the write, before the row moves + if self.on_execute is not None: + self.on_execute() # a reader observing the coordinator DURING the release window + if self.fail: + raise RuntimeError("partitioned from db") + assert "leader_lease" in sql and "UPDATE" in sql, "not the release statement" + _lease_key, owner = params + self._db.release(owner) + + +def _sql_coord( + store: _FakeSqlLeaseStore, node: str, mono: _Clock | None = None +) -> SqlServerCoordinator: + # Same timings as _coord above, so the two backends' tests are comparable at a glance. `mono` is + # passed in when a test needs to move this node's monotonic clock past its own stepdown pause. + return SqlServerCoordinator( + store, # type: ignore[arg-type] + node, + heartbeat_seconds=10.0, + leader_lease_ttl_seconds=30.0, + leader_fence_timeout_seconds=20.0, + monotonic=mono or _Clock(0.0), + ) + + +async def test_sqlserver_step_down_is_serialized_against_an_in_flight_claim() -> None: + # The twin carries the identical unfenced `t.owner = ?` renew branch, so the same interleaving + # re-promotes the drained node — and the consequence is worse here than on Postgres: only the three + # FIFO claim paths are epoch-fenced on SQL Server, so claim_ready and every terminal resolve would + # still accept writes from the ex-leader this endpoint just drained. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + store = _FakeSqlLeaseStore(db) + a = _sql_coord(store, "A") + b = _sql_coord(_FakeSqlLeaseStore(db), "B") + await a._maintain_leadership() + assert a.is_leader() is True + + store.yield_in_fetchone = True # the claim is in flight when the stepdown arrives + await asyncio.gather(a._maintain_leadership(), a.step_down_leadership()) + + assert a.is_leader() is False, "an in-flight claim re-promoted the drained node" + assert db.row is not None and db.row["lease_expires_at"] == 0.0 + + db_clock.t = 1.0 + await b._maintain_leadership() + assert b.is_leader() is True + assert a.is_leader() is False, "two leaders at once" + + +async def test_sqlserver_a_tick_inside_the_release_window_cannot_re_promote() -> None: + # The OTHER interleaving on the twin, which the committed suite could not express: its stand-in had + # a hook on the MERGE only, so a tick STARTING inside the release's await window had nowhere to + # start. Now that _execute suspends too, the Postgres half's release-window case has its sibling + # here, and "both interleavings are pinned on both backends" is a claim the suite actually carries. + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + store = _FakeSqlLeaseStore(db) + a = _sql_coord(store, "A") + b = _sql_coord(_FakeSqlLeaseStore(db), "B") + await a._maintain_leadership() + assert a.is_leader() is True + + store.yield_in_execute = True # the release suspends mid-UPDATE, as a real driver does + await asyncio.gather(a.step_down_leadership(), a._maintain_leadership()) + + assert a.is_leader() is False, "a tick in the release window re-promoted the drained node" + assert db.row is not None and db.row["lease_expires_at"] == 0.0 + + db_clock.t = 1.0 + await b._maintain_leadership() + assert b.is_leader() is True + assert a.is_leader() is False, "two leaders at once" + + +async def test_sqlserver_release_demotes_before_it_writes_and_reports_a_failed_write() -> None: + # The twin's half of the two defects the Postgres tests above pin: the demotion is synchronous and + # lands before the awaited write, and a write that raises is reported rather than dressed up as a + # drain. Same reasoning, same consequences — the T-SQL release carries the same owner-scoped UPDATE. + db = _FakeLeaseDB(_Clock(0.0)) + store = _FakeSqlLeaseStore(db) + mono = _Clock(0.0) + a = _sql_coord(store, "A", mono) + await a._maintain_leadership() + + seen: list[bool] = [] + store.on_execute = lambda: seen.append(a.is_leader()) + await a.step_down_leadership() + assert seen == [False], "a reader inside the release window saw the node still reporting leader" + + # And a partitioned release refuses instead of reporting the drain it did not achieve. + mono.t = 21.0 # past this node's own stepdown pause, so it may claim again + await a._maintain_leadership() # take leadership back (the row is expired and owned by A) + assert a.is_leader() is True + store.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + assert a.is_leader() is False + + # And the twin's half of the retry: the forced re-send lands on this backend too, so the operator + # the refusal tells to retry gets the same outcome on SQL Server as on Postgres. + store.fail = False + assert db.row is not None and db.row["lease_expires_at"] != 0.0 + assert await a.step_down_leadership() == (False, None) + assert db.row["lease_expires_at"] == 0.0, ( + "the SQL Server retry did not re-send the release write" + ) + + +async def test_sqlserver_cancelled_release_still_owes_the_write() -> None: + # The twin of test_a_cancelled_release_still_owes_the_write_so_the_retry_re_sends_it. It is here + # for the reason the module docstring gives for the other SQL Server tests: the defect is an + # asyncio one, not a T-SQL one — `except Exception` cannot catch CancelledError on either backend — + # so pinning it only on Postgres would leave the twin unguarded on every ordinary run. + # + # VACUITY CONTROL, both legs MEASURED: move `self._lease_release_owed = True` back into the + # `except Exception` arm of SqlServerCoordinator._release_leadership and this test fails at the + # owed assertion (`False is True`); silence that one line too and it fails at the row instead + # (`30.0 == 0.0`), the retry having sent nothing. + db = _FakeLeaseDB(_Clock(0.0)) + store = _FakeSqlLeaseStore(db) + a = _sql_coord(store, "A", _Clock(0.0)) + await a._maintain_leadership() + assert a.is_leader() is True + + store.hang_in_execute = asyncio.Event() # never set + await _cancel_once_suspended(asyncio.create_task(a.step_down_leadership())) + + assert a.is_leader() is False + assert db.row is not None and db.row["lease_expires_at"] == 30.0, "the cancelled write landed" + assert a._lease_release_owed is True, "a cancelled write left nothing owed" + + store.hang_in_execute = None + assert await a.step_down_leadership() == (False, None) + assert db.row["lease_expires_at"] == 0.0, "the retry after a cancelled release sent no write" + assert a._lease_release_owed is False + + +async def test_sqlserver_a_slow_release_write_is_not_spent_out_of_the_claim_pause() -> None: + # The twin of test_a_slow_release_write_is_not_spent_out_of_the_claim_pause, here for the reason + # the module docstring gives: the pause is armed in this coordinator's own step_down_leadership, + # and the MERGE carries the identical unfenced `t.owner = ?` renew branch, so a pause spent inside + # the write re-promotes the drained node here too. + # + # VACUITY CONTROL, MEASURED: delete the second arm (the `max(...)` after _release_leadership + # returns) from SqlServerCoordinator.step_down_leadership and this fails at the pause + # (`20.0 == 45.0`); silence that leg too and it fails at the behavioural one (`True is False`). + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + store = _FakeSqlLeaseStore(db) + mono = _Clock(0.0) + a = _sql_coord(store, "A", mono) + await a._maintain_leadership() + + def _slow_write() -> None: + mono.t = db_clock.t = 25.0 # 25 simulated seconds INSIDE the write + + store.on_execute = _slow_write + await a.step_down_leadership() + + assert a._no_claim_until == 45.0, "the release write was spent out of the claim pause" + await a._maintain_leadership() + assert a.is_leader() is False, "the drained node renewed itself back in over a spent pause" + assert db.row is not None and db.row["lease_expires_at"] == 0.0 + + b = _sql_coord(_FakeSqlLeaseStore(db), "B") + await b._maintain_leadership() + assert b.is_leader() is True + + +async def test_sqlserver_the_retry_arms_a_fresh_pause_from_its_own_clock() -> None: + # The twin of test_the_retry_arms_a_fresh_pause_measured_from_the_retrys_own_clock. The SQL Server + # retry test above asserts the row and the owed flag but never reads _no_claim_until at all, so the + # `or owed` disjunct is unguarded on this backend even once Postgres has a probe for it. + # + # VACUITY CONTROL, MEASURED: delete `or owed` from `arming` in + # SqlServerCoordinator.step_down_leadership and this fails at the pause (`20.0 == 120.0`); silence + # that leg too and it fails at the behavioural one (`True is False`). + db_clock = _Clock(0.0) + db = _FakeLeaseDB(db_clock) + store = _FakeSqlLeaseStore(db) + mono = _Clock(0.0) + a = _sql_coord(store, "A", mono) + await a._maintain_leadership() + + store.fail = True + with pytest.raises(StepdownReleaseUnconfirmed): + await a.step_down_leadership() + assert a._no_claim_until == 20.0 + + mono.t = db_clock.t = 100.0 # past the first call's pause + store.fail = False + assert await a.step_down_leadership() == (False, None) + + assert a._no_claim_until == 120.0, "the retry did not re-arm the claim pause" + await a._maintain_leadership() + assert a.is_leader() is False, "the drained node re-armed itself as leader after the retry" diff --git a/tests/test_security_doc_drift.py b/tests/test_security_doc_drift.py index 46381ebf3..f59394e7c 100644 --- a/tests/test_security_doc_drift.py +++ b/tests/test_security_doc_drift.py @@ -23,11 +23,12 @@ from __future__ import annotations import ast +import inspect import re from pathlib import Path import pytest -from fastapi.routing import APIRoute +from fastapi.routing import APIRoute, APIWebSocketRoute from pydantic import BaseModel from messagefoundry.api.app import create_app @@ -61,9 +62,11 @@ # BACKLOG #1184 (ASVS 14.2.1) added three JSON routes -- POST /messages/search, POST /messages/export # and POST /uploads/{file_id}/messages/search -- and two /ui routes, POST /ui/messages/search/run and # POST /ui/uploaded-logs/file/{file_id}/filter, so the needle can travel in a body instead of a URL. -_ROUTES_DEFAULT = 108 -_ROUTES_WITH_DOCS = 112 -_ROUTES_WITH_UI = 209 +# BACKLOG #1494 (ADR 0056 slice 1) added one JSON route -- POST /cluster/stepdown, the planned-failover +# control plane -- so each basis moved by one. +_ROUTES_DEFAULT = 109 +_ROUTES_WITH_DOCS = 113 +_ROUTES_WITH_UI = 210 #: The ``/ui`` routes that legitimately carry no gate: the sign-in, re-auth and second-factor entry #: points. The three ``/ui/reauth*`` routes authenticate the session cookie MANUALLY — a gate @@ -721,11 +724,50 @@ def test_route_count_parity() -> None: assert len(create_app(expose_docs=True).routes) == _ROUTES_WITH_DOCS +def test_the_counting_basis_per_module_split_matches_the_declaring_modules() -> None: + """The counting basis' per-module split is measured against the modules, not just asserted. + + RULE: the two module counts must add to the total, and each must match the module that actually + declares those routes. + + The totals were pinned from the day this file was written. **The split was not**, and it shipped + reading "68 declared in api/app.py (67 HTTP + 1 WebSocket) and 38 declared in api/auth_routes.py" + against a pinned total of 109 — an arithmetic claim that sums to 106, sitting three lines above a + number CI checks every run. Prose arithmetic beside a tested number is exactly the shape that + drifts unnoticed, so it is derived here instead of re-approved by eye. + """ + routes = create_app().routes + + def _module_of(route: object) -> str: + endpoint = getattr(route, "endpoint", None) + return getattr(inspect.getmodule(endpoint), "__name__", "") if endpoint is not None else "" + + app_module, auth_module = "messagefoundry.api.app", "messagefoundry.api.auth_routes" + in_app = [r for r in routes if _module_of(r) == app_module] + in_auth = [r for r in routes if _module_of(r) == auth_module] + ws_in_app = [r for r in in_app if isinstance(r, APIWebSocketRoute)] + assert len(in_app) + len(in_auth) == _ROUTES_DEFAULT, ( + f"{_ROUTES_DEFAULT - len(in_app) - len(in_auth)} route object(s) are declared somewhere other " + "than api/app.py and api/auth_routes.py, which the counting-basis paragraph says is nowhere. " + "Name the third module in the doc, or stop declaring routes there." + ) + sentence = ( + f"builds **{_ROUTES_DEFAULT} route objects** — {len(in_app)} declared in " + "[`api/app.py`](../messagefoundry/api/app.py) " + f"({len(in_app) - len(ws_in_app)} HTTP + {len(ws_in_app)} WebSocket) and {len(in_auth)} " + "declared in [`api/auth_routes.py`](../messagefoundry/api/auth_routes.py)." + ) + assert " ".join(sentence.split()) in " ".join(_doc_text().split()), ( + "docs/SECURITY.md's counting-basis sentence no longer matches the measured split. It should " + f"read: {sentence}" + ) + + def test_route_count_parity_with_the_console_mounted() -> None: pytest.importorskip("messagefoundry_webconsole") assert len(create_app(serve_ui=True).routes) == _ROUTES_WITH_UI, ( "the /ui plane's route count changed; update docs/SECURITY.md's counting basis and the " - "'94 routes + one /ui/static mount' statement in the same change." + "'100 routes + one /ui/static mount' statement in the same change." ) @@ -875,8 +917,9 @@ def _ui_route_rows() -> list[tuple[str, str, tuple[str, ...], str | None]]: def test_every_ui_route_appears_in_the_ui_route_map() -> None: - """The console plane is 96 of the 201 served functions and the SOLE operator UI in the deployed - posture, so 8.1.1's "every function" includes it. + """The console plane is 100 of the 210 route objects a ``serve_ui=True`` app serves — 209 endpoint + functions plus the one ``/ui/static`` mount — and the SOLE operator UI in the deployed posture, so + 8.1.1's "every function" includes it. RULE: a ``/ui`` route needs a row stating its permission and its wrapper, in both directions. ~20 of them have no JSON counterpart from which the authorization could be inferred. @@ -1074,8 +1117,9 @@ def test_ungated_routes_are_exactly_the_reviewed_allowlist() -> None: gated = [r for r in rows if r[2]] assert len(gated) == len(rows) - len(no_gate) - len(permissionless) # 87 -> 90: BACKLOG #1184's three needle-bearing POSTs, each gated exactly as its GET sibling. - assert len(gated) == 90, ( - f"{len(gated)} permission-gated routes, not 90 — update the doc's totals." + # 90 -> 91: BACKLOG #1494's POST /cluster/stepdown, gated on the new cluster:control. + assert len(gated) == 91, ( + f"{len(gated)} permission-gated routes, not 91 — update the doc's totals." )