Stop the high water mark stalling forever behind idle advisory-lock sessions (#5108, #5125) - #5163
Merged
Merged
Conversation
…le advisory-lock sessions The gap-liveness gate (#4953/#4977) will not skip a sequence gap while any transaction that could still fill it looks alive, and its open-transaction clause counts every client backend whose transaction predates the gap. A session parked idle-in-transaction on an advisory lock trips that clause forever because it never commits. #5057's allocation fence rules such sessions out by proof — but the fence lived only in the detector instance's memory, so any detector that started after the gap formed had none, fell back to the unfenced behavior, and the mark never advanced again. Both open reports are that failure at DEFAULT settings. Persist the fence. No history is needed in the database: the lookup wants the newest observation whose allocated high is at or below the mark, and both values only move forward, so a single row maintained incrementally is exactly equivalent. Promote it while the store is caught up; the moment a gap opens, allocation passes the mark, promotion stops, and the row freezes at the last moment nothing above the mark had been handed out. It is a reserved mt_event_progression row rather than a table of its own on purpose — this repairs a default-configuration stall, so it has to engage on every existing database at upgrade with no DDL migration in the way. The write re-reads the sequence inside its own statement rather than trusting the poll's reading, because a timestamp stamped against a stale allocation reading can sit later than the moment it claims, and a fence that is too late wrongly excludes a genuine reserver. That alone is not enough, which the restart test proved: the replacement daemon's OWN leadership lock is a new session that postdates the fence, so it is never quiescent and holds the gap by itself. That is #5125 exactly, and it also dominates the #5108 restart case. So rule out the daemon's own advisory-lock connections structurally — they exist only to hold the lock and never append. Identified server-side from pg_locks against Marten's own deterministic lock ids (ProjectionLockIds.Compute over this store's schema, shards and DaemonLockId, so nothing has to be plumbed from the running coordinator), restricted to backends with no xid so anything that has actually written is never excluded. This one needs no fence, so it applies to the per-tenant path too, where no sound fence can ever exist. Two guards the reserved row requires: TryCorrectProgressInDatabaseAsync updates mt_event_progression with no WHERE clause and would restamp the fence into a claim that was never observed, and ProjectionProgressStatement returns every row, which would surface high-water bookkeeping as a projection shard in AllProjectionProgress(). Proof tests in DaemonTests.ManualOnly drive real HotCold daemons against real advisory locks and deliberately slow idle-in-transaction sessions. Both stall for the full 60s timeout on master and pass in ~8s here. DaemonTests 267/267, EventSourcingTests 1626/1626, TenantPartitionedEvents 238/238, all net10.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Recovery from a race-heavy field of dead gaps was a serial grind: the skip advanced to the next gap edge, so N gaps cost N detection cycles and each one re-paid StaleSequenceThreshold from a fresh observation. The reporter measured a mark crawling 38 → 426 over 21 seconds against ~50 gaps in 489 sequences, with recovery time growing linearly in write contention. One liveness verdict already covers the entire span. Every sequence number at or below the ceiling recorded when the gap was first observed was handed out at or before that moment — nextval runs inside the reserving transaction, so its xact_start cannot postdate it — and the probe establishes that no transaction from before the observation is still running. So all of those numbers are now either committed or permanently dead, which is the same argument that already licensed skipping the first gap, applied to all of them at once. The ReservedCeiling clamp that keeps the skip away from newer, unproven reservations is unchanged. Keeps the long-standing GH-2681 empty-tail behavior: when nothing at all is committed above the mark, the dead span runs to the end of the reserved range and the ceiling is still the target — that is what the old null walk expressed, and dropping it regressed six tests. detecting_the_high_water_mark's multi-gap assertion encoded the old one-gap-per-cycle behavior and now pins the new one. DaemonTests 268/268, EventSourcingTests 1626/1626, TenantPartitionedEvents 238/238, ManualOnly proof tests 2/2, all net10.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The end-to-end proof lives in DaemonTests.ManualOnly, which CI does not run, so pin the same behavior at detector level where it will actually guard the fix. - a fresh detector inherits the fence its predecessor persisted (the deploy/rebalance shape from #5108 §1) - the daemon's own advisory-lock session does not hold a dead gap, on a store gapped before any detector polled, where no fence can exist (#5125) - a persisted fence does not skip past a genuinely live reserver - correcting progress leaves the fence row alone - the fence row is not reported as projection progress Verified against master's production code: the four behavioral tests fail, and the safety test — the live reserver must still hold — passes, which is the signature it should have. DaemonTests 273/273 net10.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The async-daemon page still described pre-9.16.1 wall-clock skipping as the expected behavior and documented neither the evidence-based hold, the SkipStaleGapsDespiteLiveTransactionsAfter cap, nor AdvanceHighWaterMarkToLatestAsync as the recovery path — so the documented contract contradicted what the daemon actually does, which is how #5108 was reported. Adds a section covering why the mark holds under a hole, what proves a gap dead, that the proof is durable across restarts, and how to use the cap and the recovery API (both with their event-loss consequences stated plainly). Also replaces the UseAdvisoryLockTransaction warning: leaked and live leadership lock sessions are both recognized now, so the workaround it prescribed is only needed on older versions, which it now says. markdownlint and cspell both clean across docs/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 3, 2026
jeremydmiller
added a commit
that referenced
this pull request
Aug 3, 2026
…5165) SkipStaleGapsDespiteLiveTransactionsAfter is documented as the bounded override for the transaction-evidence hold: once a gap has been stuck past the cap, the skip proceeds even though a candidate reserver still looks alive. But it measured from _stuckGap.Since, an in-memory observation that dies with the detector instance. Under managed-distribution agent churn or restart cycles shorter than the cap, the clock reset every cycle and the documented bound never fired — the exact unbounded stall the cap exists to rule out. Record the first sighting durably instead: a reserved mt_event_progression row holding the pinned mark and the moment it was first seen pinned. The mark only ever advances, so while it has not moved that timestamp stays a sound LOWER bound on how long the gap has been stuck, which is the direction a cap needs. Keying the row on the mark makes it self- invalidating — a different mark is a different gap, and the clock restarts. The stale threshold deliberately stays on the in-memory clock, so a fresh detector still gives a just-appeared gap a full settle window, and a null cap (the default) is untouched. Server time only, and that is the point. The obvious alternative — dating the stall from the earliest committed event above the mark, as the original PR did — reads mt_events.timestamp, which is client-written in Rich append mode (the default) and skewed by the server's UTC offset in Quick mode (#5136). A behind-clock client or a non-UTC server could then age a gap past the cap and drop a live append's events. Tests are Laurence Gillian's from #5109, carried over unchanged in substance. Four of the six already passed once #5163 landed; the two detector-churn cases are what this commit fixes. DaemonTests 280/280 net10.0. Co-authored-by: Laurence Gillian <lau@eddlondon.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 4, 2026
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
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.
Fixes #5108. Fixes #5125.
The root cause is shared
Both reports are the same failure at default settings: the high water mark pins under a dead sequence gap and never advances again, with
AdvanceHighWaterMarkToLatestAsync()as the only recovery.The gap-liveness gate (#4953/#4977) will not skip a gap while any transaction that could still fill it looks alive, and its open-transaction clause counts every client backend whose transaction predates the gap. A session parked
idle in transactionon an advisory lock trips that clause forever, because it never commits.#5057 already introduced the sound answer — the allocation fence: the latest server time at which the sequence had handed out nothing above the mark. Everything in the gap was allocated later, so any session that has provably executed nothing since cannot be holding it. That works. The problem is that the fence lived only in the detector instance's memory (
_allocationHistory), so a detector that started after the gap formed had none, silently degraded to the pre-#5057 behavior, and never advanced.The fix
1. The fence is durable (
ecda70074)No history is needed in the database. The lookup wants the newest observation whose allocated high is at or below the mark, and both values only move forward, so a single row maintained incrementally is exactly equivalent. Promote it while the store is caught up; the instant a gap opens, allocation passes the mark, promotion stops, and the row freezes at the last moment nothing above the mark had been handed out.
It's a reserved
mt_event_progressionrow rather than a table of its own, deliberately: this repairs a default-configuration stall, so it has to engage on every existing database the moment Marten is upgraded, with no DDL migration standing between the user and the fix.The write re-reads the sequence inside its own statement rather than trusting the poll's reading — a timestamp stamped against a stale allocation reading can sit later than the moment it claims, and a fence that is too late wrongly excludes a genuine reserver.
Two guards the reserved row requires:
TryCorrectProgressInDatabaseAsyncupdates the table with no WHERE clause and would restamp the fence into a claim that was never observed.ProjectionProgressStatementreturns every row, which would surface high-water bookkeeping as a projection shard inAllProjectionProgress().2. The daemon's own advisory-lock sessions are ruled out structurally (
ecda70074)This turned out to be essential rather than a niche add-on, and the test is what proved it: with only the durable fence, the restart case still stalled, because the replacement daemon's own leadership lock is a new session that postdates the fence and is therefore never quiescent. So #5125's mechanism dominates #5108 §1 too.
Those connections exist only to hold the lock and never append, so they can never be a gap's reserver. Identified server-side from
pg_locksagainst Marten's own deterministic lock ids —ProjectionLockIds.Computeover this store's schema, shards andDaemonLockId— so nothing has to be plumbed out of the running coordinator. Further restricted to backends with no xid, so anything that has actually written is never excluded. This one needs no fence, so it also applies to the per-tenant path, where no sound fence can ever exist.A blanket "holds an advisory lock" exclusion would not be sound:
RichEventAppenderreserves sequence numbers throughEventSequenceFetcheras a separate statement before its inserts, so a genuine Rich-mode reserver can be sitting there with no xid and no relation lock. That window is exactly why the open-transaction clause exists, and it is also why the tempting "nobackend_xid⇒ not a reserver" shortcut does not hold on its own.3. The whole dead span clears in one cycle (
893c13811)Addresses #5108 §2. The skip used to advance to the next gap edge, so N gaps cost N detection cycles and each re-paid
StaleSequenceThresholdfrom a fresh observation — reported as a mark grinding 38 → 426 over 21 seconds against ~50 gaps.One liveness verdict already covers the entire span: every sequence number at or below the ceiling recorded at observation was handed out at or before that moment (nextval runs inside the reserving transaction, so its
xact_startcannot postdate it), and the probe just established that no transaction from before the observation is still alive. Same argument that already licensed skipping the first gap, applied to all of them at once. TheReservedCeilingclamp is unchanged.Tests
End-to-end proof (
src/DaemonTests.ManualOnly/Coordination/high_water_advances_past_dead_gaps_with_slow_transactions.cs) — real HotCold daemons, real advisory locks, deliberately slow idle-in-transaction sessions, driven through actual daemon polling. Both scenarios stall for the full 60s timeout on master and pass in ~8s here. ManualOnly because they are slow by construction.CI-level regression coverage (
Bug_5108_durable_allocation_fence,Bug_5108_dead_gap_span_clears_in_one_cycle) since CI does not run ManualOnly. Checked against master's production code: the four behavioral tests fail, and the one that passes is the safety test — a genuinely live reserver must still hold the gap — which is the signature it should have.Reviewer notes
detecting_the_high_water_mark's multi-gap assertion encoded the old one-gap-per-cycle behavior and now pins the new one. Dropping the long-standing Daemon enters unrecoverable state if event sequence is higher than high water mark #2681 empty-tail fallback regressed six tests, so that path is preserved explicitly.Unable to attain a global lock). Not from this change.UseAdvisoryLockTransaction = falseguidance is now historical — leaked and live leadership lock sessions are both recognized, so the docs note it as a workaround for older versions only.now() at time zone 'utc'written intotimestamptzcolumns) was found while reviewing SkipStaleGapsDespiteLiveTransactionsAfter survives detector restarts (evidence-based stall onset) #5109. This PR deliberately does no arithmetic onmt_events.timestamp, so the two are independent.Relationship to #5109
This supersedes #5109. That PR left the liveness verdict alone and instead made the
SkipStaleGapsDespiteLiveTransactionsAftercap's clock durable — but the cap is opt-in and lossy by design, so it cannot fix either report's default configuration, and its evidence query is inert in #5125's exact shape (nothing committed above the mark). Once the default path recovers on its own, the cap-clock defect stops mattering.Credit where due: @uniquelau's detector-churn test in #5109 is the best executable statement of the problem, and their diagnosis of the churn reset is correct. Worth salvaging that test.
🤖 Generated with Claude Code