feat(cluster): planned-failover control plane -- POST /cluster/stepdown (BACKLOG #1494) - #1004
Conversation
github-actions
Bot
added
the
ci-red
CI reds triaged: both are filed, known, and not this changeNeither red is a regression here, and I have not re-run either leg — the harness failure's own message forbids rerunning-until-green without recording it. 1. Six failures, all in That is exactly the signature #1489 records. Evidence it is not mine, beyond the item: 2. Two failures, both Child stdout and stderr were both empty. This PR touches nothing under 3. What passed, and why it is the part that matters here
|
c6406da to
7c6f837
Compare
Round 7: four sentences, each traced to the branches it quantifies overRebased onto 1. "Past the pause expect 200" — BLOCKER, correctedBranches enumerated, and the file:line traced for each:
The old text promised Same two arms on SQL Server: 2. The teardown window — corrected a second time, and my own first replacement was wrong too
Caught before pushing, on my own replacement: I first wrote "inside the bounded demotion budget". 3. Repeat release-unconfirmed 503 — corrected
4. #1494's self-contradiction — corrected
Also in scope
Filed in #1494, not fixedThe audit's inability to tell a confirmed retry from a wrong-node stepdown; the stale-owed pause on Gates
Test run was complete, not banner-truncated: the worktree venv was missing the
One test was red after the rebase and is fixed in Legs only a hosted runner sees — Vacuity, per changed assertion, against the round-6 body
The last three were already pinned in round 6 and are not controls for this change. Collision gateNot refused. It emitted non-blocking notices on |
Full local suite on
|
Lander review of #1004, before mergingYour hold is released and I took that as releasing your hold, not as permission to merge unread. I verified your three claims first, then read the diff at Your three claims all hold, for the record:
Blocking: the claim pause is armed before the release write, so the write spends it
if self._is_leader or owed:
self._no_claim_until = self._monotonic() + stepdown_pause_seconds(
self._heartbeat_seconds
)
was_leader, released_at, wrote = await self._release_leadership(force_write=owed)The pause is armed from the instant before the write, and nothing re-arms it after. The lease row only becomes takeable when that And the write has no bound worth the name -- your own docstring says so, three ways:
What happens when the pause is spent. WHERE leader_lease.owner = $2No expiry term. The drained node owns the row it just zeroed, so it renews its own lease within milliseconds of the endpoint answering Your comment at
You reasoned carefully at
No test can see it. One thing I am not claiming, because the verifier cut it: the 503 body's "this call armed its claim pause" ( Should fix:
|
…n seam ADR 0056 slice 1, part 1 of 2. Adds `ClusterCoordinator.step_down_leadership() -> tuple[bool, float | None]` so a planned failover can release leadership without stopping the node. It reuses `_release_leadership()` verbatim in both DB coordinators, so the ordering that makes the release safe -- demote the cached gate BEFORE touching the DB, so a concurrent `is_leader()` reader never sees a stale true -- is the same one `stop()` runs. `NullCoordinator` returns `(False, None)`. Two things `stop()` does not need, because it has already cancelled its loops: - Fire the ADR 0157 demotion edge, so the graph tears down at once rather than waiting out a reconcile poll. Every other true-to-false transition fires it; a stepdown the node survives would otherwise be the one demotion the engine learned about late. - Pause this node's own claim for two heartbeats. Without it the stepdown is a coin flip: the release expires `lease_expires_at` but leaves `owner` naming us, and the claim statement's renew branch (`owner = me`) carries no expiry test, so the drained node's next maintenance tick renews and takes leadership straight back. The pause sits in the same position as ADR 0096's `promotable = false` short-circuit and is a strictly stricter claim predicate on one node, so it can only delay a claim, never advance one, and cannot open a two-leader window. The lease, the self-fence and the epoch token are unchanged. The pause length is a module-level `stepdown_pause_seconds()` shared by both coordinators, for the reason `fence_tick_seconds()` already is: a per-class copy of a safety-relevant timing constant is two files that can be retuned independently with nothing failing. Tests carry the regression AND its negative control -- clear the pause and the same sequence hands leadership back -- so the guard cannot silently stop measuring anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (BACKLOG #1494)
ADR 0056 slice 1, part 2 of 2. An operator draining the active-passive primary
for maintenance had exactly one way to move leadership: stop the service. This
adds the audited, RBAC-gated alternative -- the leader releases its lease and
keeps running as a standby.
- `CLUSTER_CONTROL` (`cluster:control`): a dedicated capability, not a reuse of
`monitoring:read` (a read) or `connections:control` (one connection). Held by
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 treatment `dr:operate` already 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` carries `{node_id, was_leader, released_at}`
exactly as `step_down_leadership()` RETURNED them. The handler takes no
`is_leader()` pre-read at all, so there is no reading for a fence or a
lost-lease tick to invalidate; the detail is the response body itself, so the
two cannot drift apart. The single-node 400 gets its own denied row because
nothing else records it; the 403s do not, because `require_step_up` already
writes them and the body never runs.
Deferred on the ADR's own terms: the `force` flag (an empty `RequestModel`
refuses it with 422 rather than ignoring it) and `new_leader_eligible` in the
result -- at the instant of release no standby has acquired, so the caller
re-polls `GET /cluster/nodes`.
The VIP mechanism itself stays proposed: no `[cluster.vip]`, no bind/release,
no privileged helper, no `vip` field on `GET /cluster/status`. The ADR's status
block now separates the two halves and marks its console section stale (it
names the retired PySide6 desktop console).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…OG #1494)
The row read "Proposed (2026-06-27, design-only)", which was true for main but
stops being true when PR 1004 lands. CLAUDE.md requires the index row to move in
the same commit as the work, so it rides here rather than in a docs-only PR.
The row now separates the two halves that this ADR keeps conflating:
- the planned-failover CONTROL PLANE is built (POST /cluster/stepdown,
CLUSTER_CONTROL, step_down_leadership() on all three coordinators);
- the VIP MECHANISM is not built and is not being built. It needs a
requireAdministrator helper binary and this repo carries no code-signing
infrastructure. Paused by owner ruling 2026-09-09.
It also points at the ADR's stale console section, which names the retired
PySide6 console. The replacement page is BACKLOG #1495 against the web console.
Deliberately does not cite a merge that has not happened.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tick (BACKLOG #1494) step_down_leadership() and _maintain_leadership both decide leadership across an await on the pool, and a stepdown runs from an API handler with both loops LIVE. The claim pause alone did not order them, so a maintenance tick interleaving with the release re-promoted the node the endpoint had just drained. Reproduced two ways against the repository's own lease stand-in, one asyncio.sleep(0) in the fake pool, with the pause armed in both: - a tick that STARTS inside the release's await window renews the lease the release is expiring. _is_leader goes back to true and the release then expires that same row, so a sibling takes it while this node still reports leader -- two leaders at once; - a claim ALREADY IN FLIGHT when the stepdown arrives has passed the _no_claim_until check before the pause was armed, 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. Zero deployments (CLAUDE.md sec. 0), so nothing is drained today. A first deployment using this endpoint would have hit it, with the outcome depending on where the heartbeat phase fell. WHY THE LOCK AND NOT ARM-THE-PAUSE-FIRST. Both candidates were measured rather than argued. Arming _no_claim_until before _release_leadership() closes the first interleaving and is measured NOT to close the second: with the arm-first variant patched in and the lock removed, the in-flight-claim test still failed while the release-window test passed. It also cannot reach the right DB end state on its own. A post-await re-check of the pause would leave the lease LIVE whenever the renew landed after the release, so the cluster would sit leaderless for a full TTL instead of failing over at once, which is the point of a planned failover. The pause keeps a job the lock does not do -- declining the ticks that come after the release -- so both stay, and the docstrings now say which does which. The lock costs nothing the release did not already cost: step_down_leadership already awaits the same pool inside _release_leadership, so a hung DB stalled it before this change. Deadlock: 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 as `async with` unwinds; and stop() deliberately does not take it, because it cancels and gathers both loops first, and taking it would queue a shutdown behind a stepdown stalled on a hung pool. The fence watchdog stays lock-free -- it must fence during a DB hang and it only ever demotes. is_leader(), the hot path, is untouched and still synchronous. pipeline/dr.py already holds the same shape of lock for its analogous promote/release pair. Applied to both DB coordinators. The SQL Server MERGE carries the identical unfenced `t.owner = ?` renew branch, and the consequence is worse there: only its three FIFO claim paths are epoch-fenced, so a re-promoted ex-leader is not fenced out of claim_ready or any terminal resolve. THREE RECORDS ASSERTED THE OPPOSITE OF THE MEASURED BEHAVIOUR AND ARE CORRECTED. stepdown_pause_seconds' docstring: "a second guarantees it has" is a floor, not a guarantee, and the function cannot see an ADR 0096 acquire_delay larger than itself. step_down_leadership's docstring: it claimed to inherit stop()'s ordering, when what makes that ordering sufficient in stop() is the cancel-and- gather that precedes it, which does not hold here. BACKLOG #1494's "cannot open a two-leader window": that applied ADR 0096's stricter-predicate argument correctly to a question it does not answer, because a predicate is read at an instant and the window is an interval opened by an await. Also here, both on records rather than code: - ADR 0056's index row asserted an owner ruling that existed in no repository artifact, while BACKLOG #1494 said in the opposite direction that nobody had signed off. The ruling is real -- 2026-09-09, pause the VIP mechanism pending a code-signing decision, because it needs a requireAdministrator helper binary and this repo has no code-signing infrastructure. It is now recorded once in the ADR's own status block together with the standard of evidence behind it: given in session, no git ref anchors it, those lines are the record. The index row and #1494 point at that record instead of asserting or denying it. - BACKLOG #1495 is filed. The number was allocated and cited from the ADR index before any "## 1495." heading existed. The allocation store is not in git, so removing this worktree would have released the number while the published citation stayed in a merged file, to start resolving to unrelated work the day someone re-allocated it. Two gaps found with the race are recorded in #1494 and deliberately NOT fixed here: the pause can be shorter than a sibling's configured acquire_delay, and a self-fenced node cannot be drained at all while the endpoint's 400 gate keys on is_clustered() rather than on whether a promotable sibling exists. Gates run before this commit: ruff check, ruff format --check, mypy strict (289 files), the ledger and backlog-status gates (the ledger gate verified against a positive control), and the cluster, API, auth, backlog and doc suites -- 278 and 134 passed. Both regression tests carry their fails-without readings, taken by reverting the lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…LOG #1494)
Four defects in the ADR 0056 slice 1 control plane, all in the release path.
1. A FAILED RELEASE WRITE REPORTED SUCCESS. _release_leadership caught a pool
error, logged, and returned (was_leader=True, released_at), so the endpoint
answered 200 with was_leader=true and audited the node as drained. The lease
row was untouched and still owned by that node, so no sibling could take it
and the node renewed itself back in through the unfenced `owner = me` branch
when the pause ended. On the shipped defaults (heartbeat 10, fence 20, ttl
30) the pause ends at 20 and the lease lives to 30; the settings validator
pins heartbeat < fence < ttl and never compares the pause to the ttl.
Best-effort is right for stop(), where the node is leaving and a lease that
ages out costs nothing, and wrong for a stepdown, where an operator reads the
answer and then starts maintenance. So _release_leadership now returns
whether its write landed and step_down_leadership raises StepdownUnavailable,
which the endpoint maps to 503 -- the status the neighbouring DR endpoints
and ADR 0056's own contract give environment conditions -- and audits
cluster_stepdown_denied rather than a cluster_stepdown row claiming a drain.
409 would be wrong in the other direction: it says "you addressed the wrong
node" and would send the caller to a different one.
test_step_down_survives_a_failed_release_write encoded the defect as correct
("the lease ages out on its own"), so it is replaced rather than amended.
2. A CANCELLED STEPDOWN SKIPPED THE PAUSE. The _no_claim_until assignment sat
after the awaited release, so cancelling the request task inside the pool
write unwound correctly and never armed it. The API handler is a bare await
with no shield and no timeout, so any cancellation lands exactly there. The
pause is now armed BEFORE the release. Reading _is_leader there is exact, not
a pre-read: nothing suspends between that read and _release_leadership's own
read of the same attribute, _maintain_leadership is excluded by the lock, and
_check_fence is synchronous.
Measured: this does NOT weaken the lock. Removing only the maintenance tick's
lock still fails test_a_claim_already_in_flight_cannot_re_promote_after_the_
release and nothing else -- that claim has already passed the pause check, so
only mutual exclusion orders it. Arming earlier does now close the OTHER
interleaving on its own, so the comment crediting the lock with closing that
one is corrected: it pinned a conjunction, and now pins the pause.
3. THE LOCK'S WAIT WAS UNBOUNDED, and its docstring denied the cost. 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. [store].command_timeout was the only ceiling,
PostgresStore passes `command_timeout or None` so the documented
zero-disables value removes even that, and the pool acquire() has no timeout.
Bounded now at leader_fence_timeout_seconds -- derived, not picked: past it
the node's own watchdog has concluded its DB access is not working, so a
stepdown still queued is racing a self-fence. A timeout refuses without
touching leadership. The claim that the lock "adds no new stall to this
method either" was a control resting on a false premise and now states the
trade. stop()'s new comment claiming the gather retired "the only coroutine
that competes" was also made false by this PR and is corrected.
4. THE DEMOTE-BEFORE-THE-WRITE ORDERING WAS PINNED BY NO TEST. Moving
`self._is_leader = False` after the awaited write passes every cluster test
on both backends, because they all read is_leader() only after the call
returns. Added a probe inside the release window, on both coordinators.
Also gives the SQL Server stand-in the _execute hooks its Postgres sibling has,
so the release-window interleaving can be expressed against the twin at all --
the committed suite could not, while #1494 read as claiming parity.
Vacuity readings, each mutant reverting one mechanism:
demote after the write -> kills the two new ordering probes
maintenance tick's lock -> kills the in-flight-claim test, only
claim pause -> kills the release-window tests
pause armed after the release -> kills the cancellation test, only
failed write reports True -> kills both failed-write tests
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ACKLOG #1494) Subject cites #1494 only. The three items below are FILED by this commit, not built by it, and the claim gate reads a subject #N as a build claim -- claiming them would tell the next session they are in progress when nothing is being built on them. Three shipped records contradicted each other or the code, and three known gaps were tracked by nothing but prose. CLUSTERING.md said the endpoint answers 400 "on a single node". It does not: the gate reads is_clustered(), a literal True on BOTH DB coordinators and False only on NullCoordinator, so it asks whether clustering is ENABLED. A one-node CLUSTERED install gets 200 and goes leaderless for the pause. The sentence now describes the code, cites #1509 for the underlying gate defect, and documents the failed-drain 503 and the two known limits (#1507, #1508) an operator would otherwise discover during a maintenance window. ADR 0056's status block claimed the built control plane was the whole of section "Control API -- planned failover" minus two deferrals. That section runs to its own subsection "Confirm / step-up posture (console)", which is unbuilt, names client.stepdown_node / poll_client / _request / AsyncRunner (all retired PySide6 console symbols), and tells the confirm dialog to promise the operator that "the VIP will move" -- which the paused-VIP bullet three lines up denies. The STALE marker scoped staleness to a DIFFERENT section, so a reader arriving at that subsection was told the surrounding prose was current. The enumeration is now "at least" rather than a completeness claim (SDS-3.6), it names the two divergences the build introduced, and BOTH stale sections carry their own do-not-build-from marker at the section itself rather than relying on a reader having read the status block first. This is the same defect class the earlier fix was for, reappearing in the fix. disagreed" -- neither revision had reached main, and this repository squash-merges, so the evidence for that sentence does not survive the merge at all. And it restated the owner's VIP ruling bare in the sentence immediately before saying the ruling is recorded once elsewhere (SDS-3.5); the restatement is cut and the link kept. SECURITY.md's console-plane numbers are re-derived rather than trusted. Measured on this tree: create_app() = 109 route objects, expose_docs = 113, serve_ui = 210, of which the console plane is 100 routes plus the one /ui/static mount, 90 of those 100 carrying a gate. The counting basis had DROPPED its count instead of correcting it; "95 routes", "87 of the 95 are gated", "the 8 that are not" and "the ninth unauthenticated served path" were all stale, the last three contradicting the same document's own "Unauthenticated /ui routes (10)". The "same 28-permission catalogue" line was made wrong by this PR's own new permission. test_security_doc_drift.py carried three mutually inconsistent console-route numbers (constant 210, docstring 201/96, message 94); the constant was right and the prose is now measured to match it. The three gaps #1494 named by subject are filed as items, so something tracks them: #1507 (the pause cannot see a sibling's ADR 0096 acquire_delay_seconds), the early return as the cause -- recorded so nobody re-derives it), #1509 (the 400 gate keys on is_clustered() rather than on a promotable sibling). Numbers allocated before filing and cited after, which is the LEDGER-GATE ordering. NOT FIXED HERE: docs/adr/README.md's ADR 0056 Status cell still reads "Proposed (2026-06-27, design-only; ...)" against the ADR's own "Partly accepted", and still says "the ADR's console section" singular. A live session (claude/adr-review-d77264) holds uncommitted changes to that file and the collision gate refused the edit; overriding it is not a Builder's call. The exact replacement text was mailed to that session and is in the PR body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…send the write (BACKLOG #1494)
The 503 that replaced the swallowed release failure created two problems in its
own refusal path. Both are fixed here; the asyncio.Lock and the 503 itself are
unchanged.
ONE ARM GAVE TWO CONDITIONS ONE SENTENCE. A single `except StepdownUnavailable`
mapped both raise sites to the body "could not release leadership; it is still
the leader" and the reason "release-failed". That is false of each in a
different way. The lock acquire fires BEFORE any release runs, on a node the
handler never checks for leadership, so it can come back from a node that leads
nothing. It is now StepdownLockTimeout / "lock-timeout", and its message names
the LOCK and the bound instead of a maintenance tick -- both coordinators take
that lock in _maintain_leadership AND in step_down_leadership, so the holder is
not knowable from there. The write failure is StepdownReleaseUnconfirmed /
"release-unconfirmed". StepdownUnavailable stays as the shared base so a
catch-all caller still works.
THE FAILED-WRITE REFUSAL NOW EARNS ITS CERTAINTY BY NOT CLAIMING ANY. "wrote"
means "the driver returned", never "the row changed", so on a lost response to
a committed UPDATE the operator read "it is still the leader" while the lease
was expired and a sibling was promoting. The body is conditional now: the node
demoted itself and stopped serving, the lease MAY still be live and ours, and
here is what to do. A row count cannot settle it -- the driver reports one only
on the path where it returned, and this refusal exists for the path where it
raised -- so that is written down rather than half-implemented.
AND THE RETRY IT RECOMMENDS NOW WORKS. _release_leadership demotes the
in-memory gate before the write, so a retry hit its not-a-leader early return,
re-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 pointing back at the same node, since GET /cluster/nodes
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 a successful release is not undone by the unfenced `owner = me` renew branch
on the next tick. Scoped to a release this node OWES: a stepdown addressed to a
standby by mistake still sends nothing and arms no pause.
docs/CLUSTERING.md's 503 bullet is rewritten as two. The old one told the
operator nothing changed, not to start maintenance, and to retry -- but on the
write-failure branch the node has already demoted, armed the pause and fired
the demotion edge, which reaches Engine._on_demote_edge and stops the graph. So
a first deployment hitting a partitioned pool during a stepdown would leave
that node serving nothing while no sibling can take the live lease. The bullets
say that, and each keeps only the advice true of its own branch.
Tests, with the vacuity control for each:
- the retry re-sends the write, counted off the stand-in pool rather than
inferred from the row (revert force_write: 3 tests fail, execute count
stays at 1)
- a retry that fails again raises rather than answering (False, None)
- a stepdown on a node that owes nothing sends nothing and arms no pause
(force unconditionally: that test fails)
- the lock timeout has its own type, its own message and its own audit
reason, and can come from a node that leads nothing (restore the old
wording: 2 tests fail)
- the two API refusals carry distinct bodies and reasons (collapse the arms
back to one: both fail)
Also drops #1509's citation of a docs/CLUSTERING.md sentence that was born
inside this PR's branch history and never reached main. This repo
squash-merges, so that evidence is deleted at merge -- which #1494's own
corrected paragraph already forbids.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y (BACKLOG #1494)
THE STALE MARKER ON THE "Confirm / step-up posture (console)" SECTION WAS TOO
WIDE, AND THAT COSTS ITS ONE REAL INSTRUCTION. It asserted that every symbol
the section names belonged to the retired PySide6 desktop console. Measured
against this tree, three of the four are alive and were REHOMED, not retired:
_request messagefoundry/apiclient/client.py (ADR 0088 extracted the
Qt-free engine client)
AsyncRunner harness/_async.py
poll_client harness/_console_widgets.py (the harness reuses view
widgets moved out of the
old console)
stepdown_node absent <- the control showing the check discriminates rather
than matching everything
CLAUDE.md says both halves directly: the harness "reuses a few view widgets
rehomed from the old console", and ADR 0088 extracted the Qt-free client. A
reader who checks the four symbols, finds three alive and discounts the whole
marker is the failure this causes -- and the marker's real payload is
DO-NOT-BUILD-FROM, which must land. So the symbol claim is dropped, the warning
is kept, and step 3's mechanism is explicitly NOT retired with it: carry the
step-up / MFA challenge on the writing client and never on the read-only polling
one, and run the call off the UI thread. That rule outlives the console it was
written for, as does step 4's leaderless-window rule.
THE INDEX ROW ASSERTED DESIGN-ONLY AND BUILT IN ONE SENTENCE. docs/adr/README.md
line 91 read "Proposed (2026-06-27, design-only; stepdown control plane built
2026-09-09 ...)" while ADR 0056's own first line reads "Partly accepted". The
cell now matches the page. A previous attempt was refused by the collision gate
while another session held the file; that session has since committed, so this
is the retry.
AND THE STATUS BLOCK CITED TWO DRAFT STATES FROM THIS PR'S OWN BRANCH HISTORY,
in the past tense, about a README state that is still live. This repo
squash-merges, so evidence that exists only inside a branch is deleted at merge.
The sentence is dropped; the ruling and its standard of evidence, which are the
durable content, stay.
Separately, docs/SECURITY.md's counting basis claimed 109 route objects as "68
declared in api/app.py (67 HTTP + 1 WebSocket) and 38 declared in
api/auth_routes.py" -- which sums to 106, three lines above a total CI checks on
every run. Measured two ways that agree: a per-declaring-module walk of
create_app().routes, and a decorator census of the two source files, both
returning 71 (70 HTTP + 1 WebSocket) plus 38. The paragraph is corrected and
tests/test_security_doc_drift.py now derives the split instead of leaving it to
be re-approved by eye. Vacuity control: restoring the shipped "68" fails that
test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ellation hole (BACKLOG #1494) Four repairs to the shipped stepdown. Three are prose that a first deployment would act on wrongly; one is a real defect on both coordinators. THE 503 AND THE DOC CLAIMED THE NODE STOPPED SERVING. It has not. Engine._on_demote_edge 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 mllp, tcp, http_listener, dicom and x12 each say leader_gate is ignored, so a demoted node's listen-type inbounds keep accepting on their own ports until teardown reaches them. An operator reading "stopped serving" would begin maintenance on a node still bound to its port and still ACKing. The body and docs/CLUSTERING.md now say the node cleared its leadership flag and STARTED tearing its graph down, that teardown is not finished when the response is sent and its later phases are unbounded, and that quiescence is confirmed with GET /cluster/nodes plus the connection view, never with the status code. The API test that pinned the false sentence now pins the true one and asserts the retired claim is absent. CANCELLATION ESCAPED THE OWED-WRITE CONTRACT. _release_leadership caught Exception, which cannot catch asyncio.CancelledError, so a cancelled lease-expiring write 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 that may still be live and owned, with no audit row of either kind. Reachable in the shipped configuration: create_app registers RequestTimeoutMiddleware 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 arm the flag BEFORE the write and clear it only on one that returned, so neither a raise nor a cancellation can leave it clear. One new test per backend, each with both vacuity legs measured. "EXPECT THE RETRY TO ANSWER 409" HELD ONLY INSIDE THE CLAIM PAUSE. Past two heartbeat_seconds the claim SQL's owner = me renew branch, which carries no expiry term, puts the node back in on its own next tick and the retry answers 200. The document already described that re-arm three lines above, so it contradicted itself. Scoped in docs/CLUSTERING.md and in the endpoint docstring. THE RETIRED-SYMBOL CLAIM SURVIVED IN THE LEDGER. #1494 still called client.stepdown_node, poll_client, _request and AsyncRunner all retired PySide6 console symbols, a claim the PR had already corrected in ADR 0056. Re-measured with the control: _request in messagefoundry/apiclient/client.py, AsyncRunner in harness/_async.py and poll_client in harness/_console_widgets.py are all present, and only stepdown_node is absent, which is what shows the check discriminates. ADR 0056's marker also said "every symbol named below" while AsyncRunner is named above it; the scope word is fixed, the retraction is not touched. Four subjects found while doing this are recorded in #1494 rather than fixed: the audit cannot tell a confirmed retry from a wrong-node stepdown; a stale owed flag could pause an innocent follower; stop()'s comment does not enumerate the new field; docs/adr/README.md says "console section" singular where the ADR now carries two markers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a merge side (BACKLOG #1494) Rebase artifact, recorded rather than folded away. Resolving this branch's SECURITY.md conflict against main, I re-counted instead of choosing a side and found the per-module split was stale on BOTH: main read 67 declared in api/app.py (66 HTTP + 1 WebSocket) against a 108 total, and an earlier commit on this branch carried that forward as 68 (67 HTTP + 1 WebSocket) against 109. Neither sums: 68 + 38 is 106, not 109. Measured against the built app: create_app() returns 109 route objects, 71 from api/app.py (70 HTTP plus the one WebSocket, /ws/stats) and 38 from api/auth_routes.py; expose_docs yields 113; serve_ui yields 210, which is 109 plus 100 console routes plus the /ui/static mount. A later commit on this branch had already corrected the split to 71 and added test_the_counting_basis_per_module_split_matches_the_declaring_modules, which derives the sentence from the modules; that test is the independent confirmation, and it is what caught the naming of /ws/stats I had added inside the sentence it matches exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es over (BACKLOG #1494) Four sentences shipped in earlier rounds are true on one branch and false on another. Each is corrected to the branch the code actually takes. 1. "Past the pause expect 200" is false once a standby takes over. The claim statement has two arms -- renew, WHERE leader_lease.owner = $2, and take-over, gated on lease_expires_at + $4 < clock_timestamp(). A sibling that acquires during the pause matches neither, so _claim_or_renew_lease reports not-held and the retry answers 409. The 409's own documented remedy then sends the operator to drain the healthy new leader. Both texts now split the branches. 2. "Listeners keep accepting until teardown completes" has the ordering backwards. _teardown_body runs _stop_sources_demote last of the three phases inside the demotion budget and only then reaches the unbounded connector, executor and sandbox phases; MLLP, TCP, HTTP and X12 each call server.close() in their stop()'s synchronous prologue. What survives is weaker: an overrunning source is abandoned, DICOM releases its port inside that call, and established connections drain afterwards. 3. The release-unconfirmed 503 no longer claims a teardown started on this call. _fire_on_demote runs under `if was_leader`, which a retry has already cleared. The body now states what holds on every branch reaching the raise. 4. #1494 said a mis-addressed stepdown "arms no pause" absolutely while its own not-fixed list said the opposite. step_down_leadership arms on `self._is_leader or owed`, so an owed retry does arm on a follower. Also: drop "Leadership is exactly as you found it" from the lock-timeout bullet, which the same bullet's next sentence contradicts; and make the ADR index row say ADR 0056 carries two console do-not-build-from sections, not one. Recorded in #1494 rather than fixed: a third undocumented 503 from RequestTimeoutMiddleware that writes no audit row of either kind; the stepdown docstring's closed True->False enumeration missing stop(); the stale-owed pause reached through the documented remedy; and SECURITY.md's "two" /ui/oidc/* routes. Severity in the conditional: zero deployments, so nothing is drained today. A first deployment reading the retired text would have drained a healthy leader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wn suite The rebase onto main brought ASVS packet D, which rotates the session on a successful re-auth (auth/service.py audits auth.session_rotated). The stepdown RBAC test re-authed and then kept using the OLD bearer, so every assertion after that line silently became a test of an expired token: the deferred-force arm arrived as 401 instead of 422, and the 200 and 409 arms never reached the handler. Rebinds the token through the same _rotated helper tests/test_step_up.py and tests/test_api_auth.py already use, with the same wording, rather than a fourth private spelling of it. Control: without the rebind the arm reads `assert 401 == 422` and the suite is 1 failed, 10 passed; with it, 11 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ole budget (BACKLOG #1494) Caught while re-tracing my own replacement text, before pushing it. The corrected sentence said the listener stop "runs inside the bounded demotion budget". _teardown_body hands _quiesce_workers_demote and _quiesce_dispatchers_demote 0.7 of the budget EACH and _stop_sources_demote the remaining 0.3 (_DEMOTE_QUIESCE_SHARE = 0.7), so each share is bounded and the worst-case sum is 1.7x the budget, not the budget. Reading "inside the budget" as a total is exactly the over-broad reading this round exists to stop. All three texts and the pinning assertion now say "its own bounded share of the demotion budget". The ordering claim they were carrying is unchanged and still holds: the source stop is the last of those three bounded phases and the unbounded connector-close, executor-shutdown and sandbox-close phases follow it. Not a new finding filed against the runner: 1.7x is what the code has always done and engine.py's own comment already scopes "bounded" to the source and dispatcher phases rather than to a total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… still shipped it (BACKLOG #1494) The correction landed in the endpoint body and docstring and stopped there. Two prose sites still asserted, of a release-unconfirmed 503, that the node "STARTED tearing its graph down" -- false on a repeat refusal, where _fire_on_demote is skipped under `if was_leader` and nothing new is signalled. - docs/CLUSTERING.md's parent bullet now says what holds on both branches: the node HAS cleared its leadership flag and THIS call stopped it claiming for two heartbeat_seconds, and it explicitly denies that a teardown just started. - BACKLOG #1494's write-failure bullet quoted the round-6 body as current. It now quotes only the parts that survived and points at the two CORRECTED paragraphs for the teardown sentence, rather than describing a body that no longer ships. Fixing one of three sites and leaving two is the same defect this round is about, one level up: the sentence was corrected where it was being read, not everywhere it was asserted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…raise The teardown paragraph cited DbCoordinator.step_down_leadership alone. Its SQL Server twin (pipeline/cluster_sqlserver.py) has the identical shape -- same `self._is_leader or owed` arm, same `if was_leader: _fire_on_demote()`, same unnested `if not wrote: raise` -- so a reader given one name could reasonably infer the other differs. It does not, and that is worth one clause. NullCoordinator never raises this exception, so those two are the whole set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gres The past-the-pause paragraph quoted `WHERE leader_lease.owner = $2` alone. That is the Postgres spelling; SQL Server carries the same arm as `WHEN MATCHED AND (t.owner = ? OR t.lease_expires_at + ? < @now)` (pipeline/cluster_sqlserver.py:501). An operator on SQL Server would grep the quoted literal, find nothing, and have no way to tell whether the paragraph applied to them. Both arms are now named as a shape with each backend's literal beside it, and the paragraph says outright that nothing below it turns on the backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ACKLOG #1494) The status line closed over "engine not started, authentication not configured, or one of the two drain conditions". A third 503 reaches this route from outside the handler: RequestTimeoutMiddleware is registered unconditionally in create_app and answers 503 at DEFAULT_REQUEST_TIMEOUT_SECONDS with a body naming no route. That was recorded on #1494 as found-not-fixed this round -- but leaving the enumeration CLOSED over a set I had just measured as incomplete is the SDS-3.6 defect, and it is a different thing from building the missing audit row. So the gap stays filed and unfixed; only the closure is lifted, with the one discriminator an operator can actually use: a 503 carrying neither a cluster_stepdown nor a cluster_stepdown_denied row is the middleware's, not either drain condition. Beyond the round's brief, deliberately: the brief said file this and not fix it, and I read that as covering the audit gap rather than licensing a sentence I knew to be false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A trap in the fix shape I gave you. Read this before the round lands.The natural mirror of the first arm is a no-op on the ordinary stepdown, and it passes every existing test. if self._is_leader or owed: # placed AFTER the await -- never fires
On a first-call stepdown, The predicate has to be a value captured before the await: owed = self._lease_release_owed
arming = self._is_leader or owed # capture once, use for BOTH armsThree more from the same work
Measured with the write costing 25 simulated seconds at On finding 4, which is in a file you are editing right nowThe Mutation results, all legs, actual pass and fail: Nothing was pushed to this branch. |
… returns
The pause was armed only from the instant BEFORE the release write, so the
write's own duration came out of it. The lease row does not become takeable
until that UPDATE commits, so the window a sibling actually gets was
(2 * heartbeat - write duration), not two heartbeats -- and nothing bounds that
duration from the coordinator: the pool acquire() carries no timeout and
[store].command_timeout, 30s by default, already outlasts the 20.0s pause at the
shipped heartbeat_seconds of 10.0. A write slower than the pause would leave
zero protection, and the drained node's own maintenance tick -- queued on
_leadership_lock for the whole call -- would then fall through the pause gate,
match the unfenced `WHERE leader_lease.owner = $2` renew arm over the row it had
just expired, and be leader again moments after the endpoint answered
200 {was_leader: true}.
Keep the existing arm ahead of the await, because a cancellation landing inside
the pool write must not skip it, and add a second arm after the release returns,
taking whichever expiry is later. Both coordinators, since the SQL Server MERGE
carries the identical unfenced `t.owner = ?` renew arm.
Two test-quality gaps in the same mechanism, both measured rather than reasoned:
- No test could see the defect above, because _Clock only moves when a test
moves it and none moved it across the release, so every release cost zero
simulated time. The new tests advance the injected monotonic clock inside the
write via the stand-in's on_execute probe.
- The `or owed` disjunct that arms the pause on a retry was covered by nothing.
The two retry tests assert _no_claim_until == 20.0 after the retry, but their
clock never moves, so 20.0 is already there from the first, failed stepdown
and the assertion reads identically with the disjunct deleted. Deleting it
from both coordinators left all 109 tests in test_cluster_lease.py,
test_cluster.py and test_api_cluster_stepdown.py passing. The new tests
advance the clock past that first pause, which separates the two answers.
Four tests, one per defect per backend. Each was run against its own mutant:
removing the second arm fails them at `20.0 == 45.0` and, with that leg
silenced, at `True is False`; deleting `or owed` fails them at `20.0 == 120.0`
and then at `True is False`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd narrow the 503 discriminator Two defects on POST /cluster/stepdown, both about the audit row. The refusal's audit row went through the store that produced the refusal. `release-unconfirmed` is raised only when the lease-expiring write did not return, and build_coordinator hands DbCoordinator `store._pool` -- the same pool PostgresStore.record_audit borrows. `_denied` was a bare await, so a store-level failure would raise past the handler's own `raise HTTPException(503, ...)` and leave the catch-all to answer a bare 500 "internal error": no reason, no remedy text, and no row either, on a node that had already cleared its leadership flag and might still own a live lease no standby could take. Guard the write instead. The row is unwritable on both paths -- a store that cannot take it before the raise cannot take it after -- so the guard costs nothing and keeps the status, the reason and the remedy the operator acts on. A failed write is logged with its reason. CancelledError is not an Exception, so a request deadline still unwinds the handler as before. `_denied` is the shared shape for all three refusals, so guarding the helper covers the 400 and both 503s in one place rather than singling out the arm that surfaced it. And the docstring's audit-based 503 discriminator was false in its positive half. It told a reader that a 503 with no row of either kind IS the middleware timeout. Three other causes share that empty trail: the `engine not started` and `authentication is not configured` 503s the same docstring names three lines earlier are raised by the dependencies, so the body never runs; and the guard above makes a drain refusal look the same when the store cannot take its row. Only the negative half is entailed -- both drain arms call `_denied`, so an absent row does rule them out. Narrowed to that, and pointed at the response body, which differs on every one of them. One test, run against its own mutant: removing the try/except fails it, though at the propagated RuntimeError rather than the status, because Starlette re-raises after ServerErrorMiddleware sends the 500 and httpx.ASGITransport surfaces the raise. The comment records that, since it is what a future reader will see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed drain IS recorded as a drain, on one path the doc did not count. The audited bullet named the handler-reached refusals as the 400 and both 503s and concluded that "a failed drain is never recorded as a drain". The 409 is also reached by the handler, and the handler writes its cluster_stepdown row from the coordinator's return BEFORE raising it, so a call that released nothing lands a row under the success action name. An auditor counting rows by action name -- which that sentence licenses -- would over-count handovers, and it bites hardest on the path this section spends most of its words on: the retry after a release-unconfirmed 503, which the section itself says answers 409. The row discloses the truth in its own fields, so the fix is to the sentence. Say that every call the handler completes is audited under that name, that a 409 therefore writes one reading was_leader false, and that counting drains means reading was_leader rather than the action name. Note too that the denied row is best-effort, since the endpoint now keeps a release-unconfirmed 503 rather than losing both the answer and the row to a store that has already failed. A self-fenced node is not "already not doing leader work". _check_fence clears the leadership flag, fires the demotion edge, and nothing more; Engine's demote edge only sets _graph_wake and deliberately does not set the runner's _stop, so the teardown runs afterwards on the graph supervisor with its later phases unbounded and an overrunning source abandoned rather than cancelled. The watchdog's own docstring forbids the premise in as many words, and this same document already says the opposite of the same self-fence in the crash-failover bullet above. Say what is true of the state and point at that bullet rather than restating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
df7b7aa to
64f3f25
Compare
…ms the pause (BACKLOG #1494)
Traced per branch before writing. Each claim cites the line it rests on.
WHAT WAS FALSE. docs/CLUSTERING.md said "past the pause the answer is 200 or
409, decided by who the lease row names by then". The row does not decide it.
The endpoint raises 409 on `not was_leader`, and `_release_leadership` clears
`_is_leader` on its first lines, so every call after the first reports
was_leader=false whatever the row holds.
WHAT IS TRUE, and the branch each part covers:
* `_is_leader = True` is set in exactly ONE place, inside
`_maintain_leadership` when `_claim_or_renew_lease` returns held. Control:
5 assignments to `_is_leader` in that file, so an instrument finding only
this one would not have been reading the False ones.
* While the pause holds, `_claim_or_renew_lease` returns not-held at the
pause gate BEFORE any database access, so no tick can promote and every
retry answers 409.
* The pause is armed on `self._is_leader or owed`, so a retry re-sending an
owed write RE-ARMS it for another two heartbeat_seconds. An operator
retrying faster than the pause expires never lets a tick through and holds
themselves in 409. Operator-driven, and documented nowhere.
* Once the pause lapses the next tick settles it: the renew arm (owner = me,
no expiry term) matches if the row still names this node, which promotes
it, and a stepdown issued AFTER that answers 200. If a standby acquired,
the renew arm cannot match and the take-over arm needs an expiry that has
not passed, so 409 is permanent and correct.
The api/app.py docstring already routed 200 through the tick correctly, so it
is NOT rewritten; only the missing livelock is added. Three earlier rounds
introduced a defect by rewriting prose that was already right.
Falsified by the handles-real-patient-data session tracing the sentence. The
ending was settled with the Lander after we disagreed: it read 409-forever
because it assumed no tick intervened, which holds inside the pause and is
wrong after it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Lander: enqueuing. I re-read at All six findings resolved. Verified at the current head, not taken on report: The one commit I had not read is CI is a real population, not an empty read: Three limits I am merging in full knowledge of, because you stated them and they are right:
The thing worth keeping from this one is not the fix. Round six moved the pause arming before the write to close a cancellation hole, and that fix opened this one. Ordinary. Round seven then ran under a brief declaring the code settled and out of scope, so its verifiers were told not to look at the place the defect was. A scope boundary in a review brief is a claim about where defects are not, and it needs the same evidence as any other claim. Found by reading a diff by hand. No check on this repository could have. |
|
CI failed while this pull request was in the merge queue, so the queue ejected it. Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying. https://github.com/MEFORORG/MessageFoundry/actions/runs/34431613117 |
|
Evicted from the queue. Re-enqueued once, and here is the control that clears this PR. The batch failed one job, This PR does touch auth, so "unrelated" needed proving rather than asserting. Its whole auth delta is additive: one new enum value The decisive control is the other PR this test failed on today. Over every failed 1001 has zero auth content and fails the same test in the same job on the same runner OS. A PR that cannot possibly affect login timing fails it, so the failure is environmental. One thing I am flagging rather than dismissing. One re-run. If it evicts again on the same job I stop and treat it as signal. |
1 participant
Builds slice 1 of ADR 0056 (BACKLOG #1494): the planned-failover control plane. The VIP mechanism itself is not built and stays proposed.
What this ships
1. The coordinator seam.
ClusterCoordinator.step_down_leadership() -> tuple[bool, float | None], returning(was_leader, released_at). Both DB coordinators reuse_release_leadership()verbatim, so the ordering that makes the release safe — demote the cached gate before touching the DB, so a concurrentis_leader()reader never sees a stale true — is the same onestop()runs.NullCoordinatorreturns(False, None).2. RBAC.
CLUSTER_CONTROL=cluster:control. A dedicated capability, ADMINISTRATOR-only, and inCUSTOM_ROLE_FORBIDDEN_PERMISSIONS— so "Administrator only" is enforced on every minting path rather than merely observed of the built-in roles, which is the treatment its closest analogdr:operatealready gets. Also added to_GRANT_AUDIT_PERMISSIONS.3.
POST /cluster/stepdown. 200 / 400 single-node / 403 permission-step-up-MFA / 409 not-leader / 503 no-engine-or-no-auth, plus 422 on the deferredforceflag.4. Audit.
cluster_stepdownwith{node_id, was_leader, released_at}as the coordinator returned them. There is nois_leader()pre-read in the handler at all, so there is no reading for a fence or a lost-lease tick to invalidate. The audit detail is the response body itself, so the two cannot drift apart field by field.5. Docs.
CLUSTERING.mdgains a planned-failover section; the ADR 0056 note there and the "proposed only, with no code" sentence inAOAG-DEPLOYMENT.mdnow separate the control plane from the VIP.SECURITY.md's catalogue, role matrix, route map, gate-wrapper table and counting basis all move with the new permission and route.The gate composition I chose, and why
require_step_up(Permission.CLUSTER_CONTROL), alone. ADR 0056's decision table asks forrequire_step_up()plus TOTP MFA, and in this codebase that is one wrapper, not two:require_step_upcharges the per-actor admin-write pacing floor (BACKLOG #193), then the MFA gate, then the new-client-IP signal, then the credential-recency window. The neighbouring DR endpoints userequire_paced, which is the same pacing floor without MFA or step-up — right fordr:operate, wrong here, because the ADR explicitly asks for the second factor.require_step_up_actionwas the other candidate and is deliberately not used: its single-use action binding is reserved for durable-takeover ops (disable-MFA, session terminate), and it would make an operator re-auth per stepdown during a maintenance window with no attack it closes that the session window does not.Deferred, per the ADR's own terms
force— the ADR defers it;409is the normative non-leader answer.ClusterStepdownRequestis an emptyRequestModel, so{"force": true}gets a 422 rather than being silently ignored.new_leader_eligible— dropped. The ADR lists it unresolved ("derive the successor cheaply, or drop it and have the console re-poll"). At the instant of release no standby has acquired yet, so any successor named here would be a guess; the caller re-pollsGET /cluster/nodesand watcheslease_ownermove.One thing the ADR did not consider — read this part
ADR 0056 says a stepdown differs from a clean stop only in that the node "keeps running and heartbeating afterward", and that the standby then "acquires the expired lease". The second half does not follow from the first.
_release_leadership()setslease_expires_at = 0but leavesownernaming the releasing node, and the claim statement's renew branch isWHERE leader_lease.owner = $2 OR <expired>— the renew half carries no expiry test. On a clean stop that is safe, becausestop()has already cancelled the maintenance loop. On a stepdown the loop is still running, so the drained node's very next tick matches its own renew branch and takes leadership straight back. Which node wins comes down to whose heartbeat phase lands first, and the endpoint would have answered200either way.The fix is a bounded post-stepdown claim pause of two heartbeats, checked in exactly the position ADR 0096's
promotable = falseshort-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, and cannot open a two-leader window. It touches neither the lease, nor the self-fence, nor the epoch token. The length is a module-levelstepdown_pause_seconds()shared by both coordinators, for the same reasonfence_tick_seconds()already is.tests/test_cluster_lease.pycarries 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. The cost is stated inCLUSTERING.mdrather than hidden — a cluster with no other promotable node is leaderless for that window, which is the honest consequence of asking the only eligible node to step down.This is the one place I went beyond "a visibility lift of existing logic". Flagging it for the reviewer explicitly; if you would rather ship the seam without it, the pause is three lines per coordinator plus its two tests.
Also found stale
console/shell.py,console/status.pyandconsole/connections.py— all retired with the PySide6 desktop console. The ADR's status block now says so. The section is kept for its topology reasoning (one page renders the whole cluster from any node, so Corepoint's "Viewing" toggle has no analogue) but nothing should be built from its construction notes. No console work is in this PR.docs/SECURITY.md's counting basis saidcreate_app(serve_ui=True)yields 201. The real number was 209 before this change and 210 after; the same sentence's own arithmetic ("108 + the 97 console routes + the mount") did not reach 201 either. Corrected to 210, and the console-route count replaced with "the console routes" rather than pinning a second number nothing tests.Not done — needs another hand
docs/adr/README.md's index row for 0056 still reads "Proposed (2026-06-27, design-only)". The collision gate refused my edit: sessiona78b190ein worktreeadr-review-d77264(branchclaude/adr-review-d77264) has uncommitted changes to that file. I did not override it. The ADR's own status block is updated, so the substance is recorded; the index row wants this in a follow-up, coordinated with that session:Explicitly out of scope
[cluster.vip], the VIP controller, bind/release, gratuitous ARP, the self-fence release path,mefor-net-helper.exe, thevipfield onGET /cluster/status, and any console/UI work. The privileged-helper decision gates all of it and it is the owner's.Checks
Ran locally, green:
/simplify(four review agents; findings applied — see below)ruff check .andruff format --check .mypy messagefoundry messagefoundry_webconsole --exclude 'messagefoundry/tray/'(strict), 289 filespytestontest_api_cluster_stepdown,test_cluster,test_cluster_lease,test_security_doc_drift,test_dast_auth_sweep,test_adr0157_demote_teardown,test_alert_failover,test_cluster_graph_gating,test_leader_tasks,test_retention,test_wiring_engine,test_settings,test_api,test_api_auth,test_dr_rbac,test_custom_roles,test_auth_core,test_step_up,test_api_request_models_forbid_extra,test_upload_apiscripts/docs/backlog_status_check.pyApplied from
/simplify:_stepdown_pause_secondslifted to a shared module-level helper; the denied-audit closure inlined at its single call site; the audit detail derived from the response model;_StandinCoordinatorsubclassesNullCoordinator; the test harness moved ontoEngine.createand a shared scaffold. Skipped: consolidating the five inline SQL Server store fakes intests/test_cluster.pyand factoring a shared step-up back-dating helper — both are refactors of pre-existing test code outside this diff.Legs a hosted runner must read after my process exits — I could not see any of these:
windows-service-smoke(NSSM)tests/test_cluster_lease.pyproves the pause against a fake lease pool andtests/test_cluster.pyproves the SQL Server seam against a fake store, so the live_claim_or_renew_leaseinteraction with_no_claim_untilis only exercised on those legs.mypy --platform win32packaging/messagefoundry-webconsole/testspytestrun, which had not finished when I pushedDo not merge on my say-so. Not auto-merged.
Review round 2 — eight findings applied (2026-09-09)
Two commits:
1969f1f8(code) and0f36ae5f(docs and ledger). Read the commit messages; this section is the reader's checklist.What changed, and the disposition chosen for each
1. HIGH — a failed release write reported success.
_release_leadershipcaught the pool error, logged, and returned(was_leader=True, released_at); the endpoint answered200withwas_leader=trueand audited the node as drained, while the lease row stayed live and owned by that node. Disposition:503. The release now reports whether its write landed,step_down_leadership()raisesStepdownUnavailable, and the endpoint maps it to503with acluster_stepdown_deniedaudit row carryingreason: release-failed.503because this is an environment condition — the status the neighbouring DR endpoints and ADR 0056's own contract already give those.409would be wrong in the other direction: it means "you addressed the wrong node", and the caller would go and address a different one.test_step_down_survives_a_failed_release_writeencoded the defect as correct ("the lease ages out on its own", which is true ofstop()and false of a stepdown) and is replaced, with the reason stated in the new test.2. MEDIUM — a cancelled stepdown skipped the pause. The pause is now armed before the release, not in a
finally. Reading_is_leaderthere is exact rather than a pre-read: nothing suspends between that read and_release_leadership's own read of the same attribute,_maintain_leadershipis excluded by the lock, and_check_fenceis synchronous.F1 still holds, measured rather than argued. Reverting only the maintenance tick's
async with self._leadership_lockfailstest_a_claim_already_in_flight_cannot_re_promote_after_the_releaseand nothing else. That claim has already passed the pause check before the pause is armed, so only mutual exclusion orders it. Arming earlier does now close the other interleaving on its own, so the comment crediting the lock with closing that one is corrected — it pinned a conjunction and now pins the pause.3. MEDIUM — the lock's cost. The docstring's "adds no new stall to this method either" was false and is replaced with the trade. The wait is now bounded at
leader_fence_timeout_secondsand a timeout refuses with503without touching leadership. Derived, not picked: past the fence timeout the node's own watchdog has concluded its DB access is not working, so a stepdown still queued is racing a self-fence and can no longer report a drain the operator can act on. Bounding it also closes the genuinely unbounded case —[store].command_timeoutwas the only ceiling,PostgresStorepassescommand_timeout or Noneso the documented zero-disables value removed even that, and the raw poolacquire()carries no timeout.4. MEDIUM — the demote-before-the-write ordering. Guard tests added on both coordinators, probing
is_leader()from inside the release window.5. Three contradicting records. (a)
CLUSTERING.mdnow describes what the gate does —400on a deployment that is not clustered, which is not the same as one node — and cites #1509. (b) NOT FIXED — see the blocker below. (c) ADR 0056's enumeration is now an "at least" pointer (SDS-3.6), names the two divergences the build introduced, and both stale sections carry a do-not-build-from marker at the section itself.6. #1507, #1508 and #1509 filed with status banners; #1494's unnumbered prose replaced with citations to them.
7 and 8. All applied:
stop()'s comment, the release-window test comment, #1494's "two shipped records disagreed" (neither was onmain, and this repo squash-merges, so the evidence would not survive), #1494's bare restatement of the owner ruling (SDS-3.5). Item 8's parity claim is fixed by adding the missing hook rather than weakening the sentence:_FakeSqlLeaseStore._executenow carries the same three hooks as its Postgres sibling, and the twin has both interleavings plus a release-window probe and a failed-write test.One thing I could not do
docs/adr/README.md's ADR 0056 Status cell still readsProposed (2026-06-27, design-only; ...)against the ADR's ownPartly accepted, and still says "the ADR's console section" singular. A live session (claude/adr-review-d77264) holds uncommitted changes to that file and the collision gate refused the edit. Overriding a coordination gate is not a Builder's call, so I mailed that session the exact replacement instead:Partly accepted (2026-06-27; control plane built 2026-09-09 -- VIP mechanism paused, ruling and its standard of evidence recorded in the ADR's own status block)The ADR's two console sections name the retired PySide6 console and are marked do-not-build-fromWhoever lands this must confirm that edit happened. It is the one half of finding 5 that is still open.
Vacuity readings for every new test
Each row is one mutant reverting one mechanism, everything else intact.
_is_leader = Falsemoved after the awaited writetest_the_release_demotes_before_it_writes,test_sqlserver_release_demotes_before_it_writes_and_reports_a_failed_write_maintain_leadership's lock removed (stepdown's kept)test_a_claim_already_in_flight_cannot_re_promote_after_the_release— and nothing elsestepdown_pause_secondsreturns 0test_a_cancelled_stepdown_still_arms_the_claim_pause— and nothing elsewrote=Truewait_foraround the lock acquire removedtest_the_lock_wait_is_bounded_and_refuses_rather_than_demotingThe new API test (
test_a_drain_that_did_not_happen_is_503_and_is_not_audited_as_a_stepdown) is vacuity-checked by construction: its stand-in raises, so it fails on any handler that does not catch and re-map.The route count, measured on this tree rather than trusted
None of the four numbers in
tests/test_security_doc_drift.pyagreed. Measured withcreate_app:create_app()create_app(expose_docs=True)create_app(serve_ui=True)/ui/staticmountThe constant (210) was right; the docstring (201/96) and the failure message (94 — that is the count of distinct
/uipaths, not routes) were not, andSECURITY.mdhad dropped its count instead of correcting it. Four further numbers inSECURITY.mdwere stale independently of this PR and are corrected in the same pass, because leaving them made the document contradict itself: "95 routes", "87 of the 95 are gated", "the 8 that are not", and "the ninth unauthenticated served path" — the last three against the same document's own "Unauthenticated/uiroutes (10)". The "same 28-permission catalogue" line was made wrong by this PR's own new permission and is now 29.Checks run this round
Green locally:
ruff format(2 files reformatted, then clean),ruff check .,mypy messagefoundrystrict (273 files), the pre-commit hook set on both commits,scripts/docs/backlog_status_check.py(724 items, 6 pre-existing advisory warnings, none mine).pytest, green:test_cluster_lease,test_cluster,test_api_cluster_stepdown,test_security_doc_drift,test_api,test_api_auth,test_auth_core,test_settings,test_cluster_graph_gating(425 passed), plus a whole-tree doc-guard sweep —-k "doc or docs or ledger or backlog or security_static or operator_docs or adr", 1456 passed, 184 skipped.Not run, and unchanged from round 1: the full suite in one pass, the Postgres and SQL Server store/cluster legs,
windows-service-smoke,mypy --platform win32, and the web console suite. The Postgres and SQL Server legs still matter most: every cluster test here runs against a fake pool or a fake store, so the live_claim_or_renew_leaseinteraction with_no_claim_untiland the new bounded acquire are only exercised there.Do not merge on my say-so. Not auto-merged. The
docs/BACKLOG.mdconflict predates these commits and is the Lander's.