ha: ADR 0157 fencing increments 1 and 4/5, plus two records that denied their own commit (stranded branch) - #430
Closed
wshallwshall wants to merge 17 commits into
Closed
ha: ADR 0157 fencing increments 1 and 4/5, plus two records that denied their own commit (stranded branch)#430wshallwshall wants to merge 17 commits into
wshallwshall wants to merge 17 commits into
Conversation
…d graph stop) Status: Proposed. Nothing built. Two clauses need an owner decision: C1 (which post-claim writes carry a precondition) and C6 (does demotion get an enforced deadline, and what happens to an inbound that cannot meet it). An HA re-check found the leadership lease itself SOUND -- DB-clock expiry on both backends, atomic acquire/renew, a real leader_epoch token checked inside the claim transaction. Scopes B (failover vs count-and-log) and C (Postgres vs SQL Server divergence) were probed and CLEARED, not assumed. Two things around it are wrong: F1 -- the epoch fence guards SOME claims and nothing after them. claim_ready (the UNORDERED path) carries no epoch predicate on either backend, and every post-claim disposition write resolves by bare id with no epoch, owner or status precondition -- while release_claimed two methods away does carry AND status. The sharp one is dead_letter_now: a demoted node assigning a TERMINAL disposition and finalizing the message, breaching the store finalizer's single authority, on a row a DEAD marker means nothing will ever re-claim. F2 -- demotion budgets DETECTION, never the STOP. _check_fence flips a boolean and cancels no listener, worker or in-flight send. Measured budget on stock defaults is ~8.0s, minus a renew round trip bounded only by command_timeout=30 -- exactly equal to leader_lease_ttl_seconds, so the margin can reach zero and the validator (ordering-only) never notices. Against that, teardown stops inbounds SEQUENTIALLY at up to 10.0s per socket listener and UNBOUNDED for file/DB/DICOM inbounds, at a 1,500-connection target. Decision is six clauses. The two that invert the obvious fix: guard writes that make a row TERMINAL and never one that returns it to PENDING (fencing the L1 hand-over converts a permitted duplicate into a forbidden strand), and the resolve predicate must fail OPEN where the claim predicate fails closed (a rejected resolve leaves the row INFLIGHT, which on SQL Server is a strand). Also records the sequencing asymmetry found while verifying: SQL Server has NO periodic in-flight recovery at all -- reclaim_expired_leases is Postgres-only and the runner's hasattr gate is the sole exclusion -- so a row left INFLIGHT outside a promotion is an unbounded strand TODAY, with no HA scenario involved. Postgres bounds the same case at roughly reclaim_interval + lease_ttl. Corrects a code comment attributing a teardown-ordering constraint to "ADR 0066 D3"; that decision does not exist (grep -c D3 -> 0). Single-node SQLite is byte-identical, structurally. The general silent-controls class this belongs to is ADR 0158's subject, not this one's -- cited, not restated.
The L1 "leadership lost before send" guard re-queued an already-claimed outbound row through store.mark_failed -- the identical call a real transport failure makes. The claim had already spent an attempt (attempts=attempts+1), mark_failed re-read that post-increment value, and under a finite RetryPolicy.max_attempts it took the DEAD branch: a terminal dead-letter written on a row that was NEVER SENT. The new leader never sees a DEAD row, so the message is neither delivered nor deliverable -- recoverable only by an operator replay, and on a PHI instance only until [retention].dead_letter_days purges the body. At-least-once permits duplication and forbids stranding; this stranded. The comment directly above the branch asserted the opposite of what the code did: "We do NOT drop the row -- re-queue it via the existing retry (mark_failed -> PENDING with backoff)". True only while max_attempts is None. Release the claim instead: attempts-- , next_attempt_at UNCHANGED, no last_error, guarded status='inflight' so it is idempotent. The correct primitive already existed 50 lines below, used by the credential-fault path. Return STOPPED, not PROCESSED. _to_lane_result maps (PROCESSED, None) to RESOLVED, which advances to the next item -- and since a release applies no backoff the row is immediately due again, so the lane would hot-spin for the whole teardown window, which is not bounded against the fence-to-expiry margin (ADR 0157 F2). A STOPPED lane cannot outlive its term: _teardown_unsafe clears the dispatchers and workers, and start() rebuilds them on promotion. The batch twin matters more, not less: mark_batch_failed decides ONE disposition from the head's attempts and applies it to all N members. TESTS -- and the first version of them was vacuous, which is worth recording. Asserting the row is PENDING with attempts=0 proves nothing: a seeded row is already in that state, so the assertion passed against the pre-fix code. Both tests now wait on a POSITIVE SIGNAL (a spy on release_claimed) before asserting outcome. Verified by mutation: reverting the body to mark_failed makes both tests FAIL, and restoring it makes both PASS. A green test is evidence only after it has been made to fail on purpose. Single-node is unaffected: NullCoordinator.is_leader() is always True, so the branch never fires and the delivery path is byte-identical. ruff + mypy clean; 125 passed across the dispatcher, pooled-rider, wiring and cluster suites (the Postgres/SQL Server legs skip locally -- CI covers them).
Adversarial design review of the increments, run BEFORE implementing them, found two clauses of this ADR wrong in the one direction the invariant forbids. Corrected here, in the design record, before any code was written against them. C3 said a fenced terminal write should roll back and return, leaving the row INFLIGHT for recovery to collect. That is a STRAND. On Postgres it costs ~90s of latency via reclaim_expired_leases. On SQL Server there is NO periodic in-flight recovery at all -- reclaim_expired_leases has zero occurrences in sqlserver.py and the hasattr gate in engine.py is the sole exclusion -- so the row waits for the next promotion. The ADR's own fence would have produced exactly the outcome the ADR exists to prevent. Corrected: a fenced write rolls back and then RE-PENDS via an unguarded release_claimed. That is invariant-legal by C1's own rule (a return-to-PENDING write, which C1 forbids fencing) and is status='inflight'-guarded, so it is idempotent. It converts the fence's residue from a possible strand into a certain duplicate -- the only direction permitted. C4 said "delete the set_leader_epoch(None) clear from _stop_graph". Alone, that is unsafe once C5 fences every claim path, and the failure is silent and total. set_leader_epoch has one push site, inside _start_graph, and _reconcile_graph has only two branches. A demote-and-re-acquire during a slow _start_graph leaves the node in `is_leader() and running`, which matches neither branch forever, with the store holding a stale epoch. Today that half-works because claim_ready is unfenced; after C5 it is a live leader that claims nothing, with no exception and no alert. Corrected: delete the clear AND re-stamp current_epoch() on every reconcile pass while leader and running. Safe and idempotent -- _is_leader flips False->True only immediately after the renew refreshed _leader_epoch, with no intervening await. Also names the existing test that encodes the old contract and must be inverted in the same commit. C6 gains three constraints, each the difference between the increment working and being harmful: the concurrent source stop must be one phase-level asyncio.wait, not a semaphore with per-source wait_for (a semaphore bounds the phase at ceil(N/C) x budget -- ~63s at the 1,500-connection target against an ~8s margin, i.e. not a fix); quiesce() must not gate the lane drain on the claimer loops exiting, because the fence fires precisely when the claimer is parked against command_timeout; and _running = False must execute on every path via try/finally, with the finally doing only that. No code yet. This is the design record catching its own errors before they reached the reliability core, which is what the adversarial pass was for.
… cap) State, landed SHAs, spec location, and the two sections that matter most. RETRACTIONS -- eight, including two clauses of ADR 0157 that I wrote and that were wrong in the strand direction: C3 left a fenced row INFLIGHT, which on SQL Server has no periodic recovery; C4 deleted the epoch clear without a re-stamp, which silently halts a live leader once C5 lands. Also a CI number I amplified as a step figure when it was a job figure, a mis-attributed BACKLOG item, a merge ordering built on a conclusion my own measurements contradicted, and four instrument errors -- three caught before acting, one (a vacuous test) caught only by mutation. TRAPS -- chiefly that the ADR's SQL Server increment is MIS-SPECIFIED. The codebase contradicts itself on whether reload recovers INFLIGHT rows; wiring_runner.py:1800 is right and :4718 is wrong, so the real defect is no recovery at graph re-start, not a missing periodic sweep. An owner-blind age sweep on SQL Server has no owner column to discriminate with and would re-pend live rows. Do not build it as written.
…e (ADR 0157 Inc 1)
The H1 leader-epoch token guarded SOME claims and nothing after them. A demoted ex-leader still
inside a send could not CLAIM anything, but could still WRITE — including over a row the live
leader had already resolved. This closes that on Postgres.
Two guard templates with DELIBERATELY OPPOSITE polarity (_EPOCH_GUARD_CLAIM / _EPOCH_GUARD_RESOLVE):
CLAIM is fail-CLOSED. A missing leader_lease row yields NULL, `NULL <= $held` is false, the claim
declines. Declining is free — the row stays PENDING and any node may take it.
RESOLVE is fail-OPEN, via COALESCE. Rejecting a resolve leaves the row INFLIGHT, so reusing the
claim idiom here would mass-strand every in-flight row the moment the lease row went missing.
That inversion reads like a copy-paste slip, so it is pinned by a test in both directions.
C5 adds the guard to claim_ready (the UNORDERED path, previously unfenced). C1/C3 guard the eight
terminal resolves: dead_letter_now, mark_done, mark_batch_done, complete_with_response,
ingress_handoff's two DEAD branches, mark_failed, mark_batch_failed, dead_letter_batch. A rejected
resolve raises _FencedWrite INSIDE the transaction, so the queue flip, the delivered_keys row, the
message_events row and the finalize roll back TOGETHER — never half-applied.
Deliberately UNGUARDED, and tested as such: release_claimed / reschedule_claimed and the other
re-pend paths. Fencing a write that returns a row to PENDING converts a permitted DUPLICATE into a
forbidden STRAND, which the at-least-once invariant rules out outright.
D1 — a fenced resolve then RE-PENDS via an unguarded release_claimed in a fresh transaction. The
drafted design left the row INFLIGHT for recovery; that is ~90s of latency on Postgres and, on SQL
Server (no periodic in-flight recovery at all), an unbounded strand. This makes the fence's own
residue a bounded duplicate on both backends.
C4 — _stop_graph no longer clears the held epoch on demotion. set_leader_epoch(None) OMITS the guard
entirely, so clearing it disarmed the fence at exactly the moment a superseded ex-leader is most
likely to still be writing. D2 adds a re-stamp on every leader+running reconcile: without it, a slow
bring-up spanning demote -> takeover -> re-acquire leaves a live leader holding a stale epoch that
(post-C5) claims NOTHING, silently. D7 adds has_residual_state so a raised teardown converges.
NOT a general write fence, and the ADR's cross-backend claim was wrong: on SQL Server only the three
FIFO claim paths carry a guard — claim_ready and every terminal resolve stay unfenced there until
Inc 3. Retaining the epoch under C4 is still strictly better than clearing it, but it does not make
a demoted SQL Server node claim nothing. Corrected in cluster.py's scope docstring.
Counter surface is six sites across three files, not two: StatsResponse takes Pydantic's default
extra='ignore', so an undeclared kwarg would have been dropped SILENTLY and /stats would never have
grown the field.
Evidence:
- 13 runtime tests against a real Postgres, and the full 147-test Postgres suite still green.
- 8 structural tests (NOT env-gated, so they run on every leg) pinning which writes carry which
guard, complete with a written reason for every unguarded one.
- Mutation-verified in both directions. The FIRST version of the structural gate keyed on whether
a method mentioned the guard constant; deleting {epoch_guard} from claim_fifo_heads' SQL — the
exact regression it exists to catch — left that mention intact and the gate stayed GREEN. It now
keys on the emitted SQL. Mutations confirmed red: guard dropped from claim_fifo_heads / from
claim_ready / from mark_done; `<=` -> `<` (4 failures); resolve guard made fail-closed (2);
D1 re-pend removed (5).
…e (ADR 0157 Inc 4/5)
Demotion budgeted DETECTION and never the STOP. A fenced ex-leader tore its graph down on an
unbounded, sequential path with no deadline at all, so "the node stopped being leader" and "the node
stopped writing" were separated by however long the slowest listener took.
TeardownReason{SHUTDOWN, DEMOTE} splits the source + dispatcher phases only; every other phase and
their order stay shared, and SHUTDOWN executes today's statements verbatim.
D6 — the source phase is ONE phase-level asyncio.wait over all tasks, not a per-source wait_for under
a semaphore. The semaphore form costs ceil(N/C) x budget: ~63s at the 1,500-connection target against
an ~8s margin. asyncio.wait also never cancels its awaitables, so "abandon, don't cancel" is a
property of the primitive rather than of an asyncio.shield token a later edit can silently drop.
An inbound that overruns is ABANDONED, not cancelled and not awaited. Cancelling mid-stop() can abort
the close before the port is released. The abandoned task is generation-scoped and settled, bounded,
at the next promotion — inside _reload_lock, so an unbounded join there would wedge re-promotion, the
engine shutdown and the whole /connections API.
Inc 5 inverts the ADR 0066 D3 order under DEMOTE only: egress is the split-brain-relevant action, so
its budget starts immediately rather than after the source phase. StageDispatcher.quiesce() lets each
serializer reach its terminal transition and leave ZERO rows INFLIGHT, where the hard cancel leaves
them claimed — latency on Postgres, an unbounded strand on SQL Server. stop() is untouched and still
runs after, as both the state-clearing path and the hard-cancel fallback.
D8 — the drain is NOT gated on the claimer/sweep loops exiting. The fence fires BECAUSE renews
failed, i.e. the pool is degraded, i.e. the claimer is parked in the store against command_timeout. A
gated design times out before draining a single serializer, on this increment's dominant trigger.
D7 — `self._running = False` moves into a finally containing no await. _reconcile_graph's bring-up
branch is `is_leader() and not running`, so a teardown that raises or is cancelled from outside would
otherwise leave the node un-re-promotable, silently, with no exception.
Edge trigger: a sync, never-raise, pure in-memory on_demote hook on both coordinators, fired at BOTH
demotion edges. The lease-lost branch is REQUIRED, not belt-and-braces — it sets _is_leader = False
itself, so _check_fence short-circuits and the TAKEOVER (the case that matters) would get no edge at
all. Not fired on the clean step-down.
CORRECTIONS to the drafted design and the ADR, each verified against source rather than assumed:
- "EVERY socket source closes in its synchronous prologue" is false. The four asyncio.start_server
sources do; DICOM releases its port inside `await to_thread(server.shutdown)`, so an abandoned
DICOM stop can hold the port at re-promotion. TimerSource was missing from the inventory
entirely (11 source connectors are registered, the spec accounted for 8).
- "accept stops at task creation" is false — create_task only SCHEDULES. It stops on the first loop
pass, before the wait's timeout can fire. The conclusion survives; the wording would mislead an
edit that inserted anything between create_task and the wait.
- _PENDING_STOP_SETTLE_SECONDS was 1x the client-shutdown grace, but MLLP/TCP/X12/HTTP each consume
that grace TWICE serially inside one stop() — it would have cancelled in precisely the slow-but-
healthy case it exists to settle. Now 2x.
- The cluster_sqlserver.py compile-time Protocol guard CANNOT backstop a missing hook: it asserts
only assignability to ClusterCoordinator, which deliberately does not carry set_on_demote. The
ADR cited it as the remedy for a defect it structurally cannot see. The tests are the backstop.
- has_residual_state does NOT mirror stop()'s had_state, contrary to the drafted docstring. It drops
_running (already False) and adds _dispatchers (cleared early, so a cancel between teardown phases
leaves them populated). Both deviations are deliberate; the claim of mirroring was not.
Also wires tests/test_adr0157_postgres_fence.py into the postgres-store CI job. The repo's own
test_serverdb_ci_coverage gate caught that those 13 tests, being MEFOR_TEST_POSTGRES-gated, would
otherwise have executed NOWHERE — not on a PR, not on push, not on the nightly.
Evidence: 21 non-env-gated tests, mutation-verified. abandon->cancel (3 failures); sequential source
loop (times out — 200 sources x 0.4s, the sum-shaped defect made visible); finally deleted (1);
_dispatchers dropped from has_residual_state (1). Baseline 21 pass. mypy strict clean at 260 files.
The stop-work handoff described Inc 1 as a spec waiting to be applied. Increments 1, 4 and 5 are built, so leaving it would put a document on main asserting the opposite of the code beside it. Trimmed to the two things that do not belong in an ADR — what is left (Inc 0/2/3, with the warning that Inc 2 is mis-specified) and the traps: local pytest silently skipping both server-DB legs, a module-gated suite executing nowhere until a workflow names it, leader_lease surviving between tests because it is absent from _TABLES, and the fact that a full local run with the Postgres env set contaminates suites that pass in isolation. Everything else now lives in ADR 0157, stated once.
…hat built it The ADR body was updated to Accepted/implemented; its row in the index was not. So this branch was about to land a summary asserting the opposite of both the ADR it summarises and the code beside it, and nothing in the tree checks that the two agree. The row now carries the three corrections the build forced, because a status of Accepted without them reads as though the design shipped as drafted, and it did not: C3 would have left a fenced row INFLIGHT, which on SQL Server is an unbounded strand manufactured by the fence itself; C4 alone was a silent total halt until the epoch was re-stamped each reconcile; and the claim that a demoted SQL Server node claims nothing was false, since claim_ready is unguarded there. It also records that Inc 2 is mis-specified in the ADR, so the next reader meets the warning in the index rather than after implementing it. Held this back while the collision gate showed another live session with uncommitted changes to the file; landing it now that its tree is clean.
This was referenced Aug 17, 2026
…by another route
Pre-resolving the mechanical half of this PR's conflict so only the semantic half is left
for its author. Lander-authored, joint-evaluated with the Liaison.
THE COMPUTED TARGET SAID 159 AND 159 WOULD HAVE BEEN WRONG. The standing rule for this
index is target = C + (B - A) entries. Here A=150, B=151, C=158, giving 159 -- but the
single entry this branch adds is ADR 0157, and 0157 is ALREADY ON MAIN, landed
independently while this PR sat. Compared by ADR NUMBER rather than by row text, the
branch's set is a strict subset of main's: it adds nothing main lacks. The two 0157 rows
are byte-identical.
So applying the formula would have appended a DUPLICATE 0157. The formula assumes the
branch's addition is not already present; when an ADR reaches main by another route,
B - A double-counts it.
THE ROBUST FORM, which is what was actually used here:
target = the UNION OF ADR NUMBERS, not a count of rows
#430: |numbers(main) union numbers(branch)| = 158 (branch is a subset)
Correct resolution is therefore main's index verbatim, 158 rows, and this commit is that.
Not touched: tests/test_adr0157_fence_scope.py, an add/add conflict that is a genuine
design question for this branch's author.
Collaborator
Author
Closing as a SOURCE, not discarding the workLander disposition, 2026-08-21, on the owner's go. The work is preserved and this close is reversible. Durability first. The tip is tagged on Pushed and read back peeled ( The pre-existing tag was not enough, which is why a new one exists. Why close rather than rescue. Measured against
The base is 19 days old. Resolving one add/add conflict would produce a branch whose other 20 files were written against a tree that has moved 507 commits underneath them -- and with Note the instrument, because it nearly misled me here. A two-dot diff reports this branch as touching 838 files. The three-dot figure is 21. Anything scoping a re-cut off the two-dot number is wrong by a factor of forty. What is still wanted should be re-cut against current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opened by a different session to make stranded work visible. Not reviewed by me, not armed for auto-merge.
Existed only in a local checkout — never on
origin, no PR — while claimha-recheck-inc145pointed at a worktree directory that no longer exists. Its last commit is dated 2026-08-02, making it the oldest of the six stranded branches at 15 days. Backed asrefs/tags/rescue/branch/...on the private remote.What is here
16 commits (8 of them merges from
main), 21 files, +3098/−350 — the largest diff of the six by line count. ADR 0157 high-availability fencing work:Those last two are the same defect class this repository keeps finding — a record asserting an absence that the very same commit disproves.
Sequencing
7 files overlap with
claude/hopeful-chaplygin-3158c6and 7 withclaude/1245-coalesce-mutation-check. All three are large; whichever lands second and third need real rebases. This branch already carries 8 merges frommain, but the newest is from 2026-08-02, so it is ~15 days and roughly 60 commits behind.BACKLOG.mdis untouched — this work cites ADR increments rather than backlog numbers, so the backlog-hygiene gate may not apply. CI has not run against amainthis recent for this content, so treat any failure as new information rather than a regression.🤖 Generated with Claude Code