Make the stale-gap cap clock survive detector restarts (#5109) - #5165
Merged
Conversation
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>
Member
Author
|
Local sweep complete on net10.0: DaemonTests 280/280, EventSourcingTests 1627/1627, TenantPartitionedEventsTests 238/240 (2 pre-existing skips). |
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.
Carries forward the defect and the tests from #5109 by @uniquelau, with a different mechanism.
The defect (their diagnosis, confirmed)
SkipStaleGapsDespiteLiveTransactionsAfteris 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.#5163 did not address this — it repaired the allocation fence, which is a different mechanism. Confirmed empirically: applying their test file to current master, 4 of 6 pass (#5163 handles those scenarios) and the 2 detector-churn cases fail. This fixes those two.
The mechanism
Record the first sighting durably: a reserved
mt_event_progressionrow 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 — 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. A null cap (the default) is untouched.
Why not the original approach
#5109 dated the stall from the timestamp of the earliest committed event above the mark. That 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 — see #5136, which was found while reviewing that PR. A behind-clock client or a non-UTC server could age a gap past the cap and drop a live append's events. Using the mark's own pinning time keeps everything in server time and needs no trust in event timestamps at all.The original PR's
last_updated-only rejection still stands and its guard test (idle_store_first_live_append_is_not_cap_skipped) is carried over intact.Tests
All six tests are @uniquelau's from #5109, unchanged in substance, with co-authorship on the commit:
cap_skips_a_pre_existing_dead_gap_despite_an_idle_listener_and_no_allocation_historyidle_store_first_live_append_is_not_cap_skipped(guard)live_reserver_is_not_cap_skipped_before_the_cap_genuinely_elapses(guard)running_daemon_resumed_after_the_gap_formed_skips_within_the_capdetector_churn_does_not_postpone_the_cap_foreverconjoined_tenancy_routes_store_global_and_the_cap_still_bounds_detector_churnDaemonTests 280/280 net10.0. Further suites running locally; will report.
Housekeeping
The new reserved row is excluded from
ProjectionProgressStatementand from the blanket UPDATE inTryCorrectProgressInDatabaseAsync, same as the allocation fence. #5164'sisProgressRowallowlist already ignores unrecognised bookkeeping names, soWaitForNonStaleProjectionDataAsyncneeded no change — which is that hardening paying for itself immediately.🤖 Generated with Claude Code