fix(subscriptions): release relational gaps once the skip timeout expires - #588
Merged
Conversation
…ires A gap in the global position sequence holds an all-stream subscription until the missing position becomes visible, which is what keeps concurrent appends from being skipped. Some positions never arrive though: a rolled back append still consumes the sequence value. GapSkipTimeoutMs exists to bound that wait, but DetectGap rebuilt the gap with FirstSeen = UtcNow on every poll, so the timer restarted before it could ever expire. The only escape left was GapAgeThresholdMs, default one hour, measured against the event after the gap. A live subscription therefore stalled for up to an hour on a permanent gap. Keep the original FirstSeen when the gap is one we are already holding, and match it by position so an unrelated gap's timer no longer survives the subscription moving on. Refs #222 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
PR Summary by QodoFix relational subscription gap skip timeout
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b74e9e7ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Options.GapAgeThresholdMs == null || newGapAge.TotalMilliseconds < Options.GapAgeThresholdMs.Value) { | ||
| return new(expectedNext, DateTime.UtcNow); | ||
| } | ||
| return DateTime.UtcNow - previousGap.FirstSeen < TimeSpan.FromMilliseconds(Options.GapSkipTimeoutMs.Value) ? previousGap : null; |
There was a problem hiding this comment.
When PostgreSQL sets GapHandlingTimeoutMs equal to the default GapSkipTimeoutMs (or sufficiently close that one poll crosses both thresholds), this returns null as soon as the skip timeout expires. PollOnce subsequently calls HandleGapTimeout only when gap != null, so the configured tombstone is never created even though the gap persisted for the handling duration; later subscriptions must each wait for and skip the same permanent gap. Perform the timeout remediation before clearing the expired gap, or otherwise preserve enough state to invoke it.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Confirmed, and reproduced: with GapHandlingTimeoutMs = GapSkipTimeoutMs = 500, the release cleared gap on the same poll that crossed both thresholds, so the remediation block — gated on gap != null — never ran. No tombstone was created, and start then advanced past the position so it was never retried. TombstonesCreationTest uses 500 against the 5000 default, which is why it stayed green.
Fixed in 81d7a80 by tracking remediation on the gap and giving it precedence over the skip:
if (DateTime.UtcNow - previousGap.FirstSeen < TimeSpan.FromMilliseconds(Options.GapSkipTimeoutMs.Value)) return previousGap;
// Remediation resolves the position safely, unlike skipping it, so it gets its chance first.
return Options.GapHandlingTimeoutMs != null && !previousGap.RemediationAttempted ? previousGap : null;GapRemediationBeforeSkipTest covers it — it failed with Expected to be 1 but found 0 tombstones before the change.
Test Results 45 files ± 0 45 suites ±0 14m 14s ⏱️ +22s Results for commit 457919f. ± Comparison against base commit a978e32. This pull request removes 9 and adds 21 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Two escape hatches exist for a gap in the global position sequence, and they are not equally safe. GapHandlingTimeoutMs inserts a tombstone, which conflicts with a committed row and blocks on an in-flight one, so it can never displace a real event. GapSkipTimeoutMs advances past the position on elapsed time alone, so an append slower than the timeout has its event skipped. Releasing the gap cleared it before the remediation check in PollOnce, which is gated on a non-null gap, so configuring GapHandlingTimeoutMs at or above GapSkipTimeoutMs silently produced no tombstones at all and the subscription skipped a position it could have resolved. Track whether remediation ran and let it take precedence over the skip. Default GapSkipTimeoutMs to null. It never took effect before, so every deployment already behaves this way; leaving it at 5 sec would instead turn a visible stall into silently skipped events on upgrade. Skipping on time alone is now opt-in, and the docs say what the options do. Refs #222 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The age threshold sat below the held-gap branch, which returns without reaching it, so it only ever ran on the poll that first observed a gap. With GapSkipTimeoutMs defaulting to null that branch returns unconditionally, and a gap first seen while the event after it was still young was never released at all — an indefinite stall where the previous behaviour bounded it at GapAgeThresholdMs. Evaluate the threshold ahead of the held-gap branch and on every poll. GapIgnoreTest missed this because it only covers a gap that was already older than the threshold before the subscription started, which is released on first detection. The two timeout tests asserted eventual release and remediation but not that either waited for its configured timeout, so an implementation that acted immediately would also pass. Both now assert the hold before the release, and the shared arrange and wait steps move to the fixture. Refs #222 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Follows up on #424, refs #222.
The problem
Gap detection holds an all-stream subscription when it sees a hole in the global position sequence, which is what stops concurrent appends from being skipped — transaction (a) takes position 5, (b) takes 6 and commits first, and the subscription waits for 5 rather than running past it. That part works and is covered by
SubscriptionGapsDetection.Postgres_ShouldNotSkipEvents.Some positions are never filled, though: a rolled back append still consumes the identity value. Releasing such a gap was broken.
GapSkipTimeoutMsnever fired —DetectGaprebuilt the gap withFirstSeen = DateTime.UtcNowon every poll, restarting the timer before it could elapse — so release fell toGapAgeThresholdMs, default one hour and measured against theCreatedtimestamp of the event after the gap. A subscription at the head stalled for up to an hour on a permanent gap.This lives in the shared relational base, so Postgres, SQL Server and SQLite are all affected.
The fix
Preserve a held gap's
FirstSeenso the skip timeout can expire, and match held gaps by position so an unrelated gap's timer doesn't survive the subscription moving on.Evaluate
GapAgeThresholdMson every poll, ahead of the held-gap branch. It previously sat below that branch, which returns without reaching it, so it only ran on the poll that first observed a gap. Age release deliberately precedes remediation: the option exists so replays don't recreate tombstones that were deliberately deleted.Let remediation resolve a gap before anything abandons it. The two escape hatches are not equally safe.
try_insert_tombstoneinserts at the gap position withoverriding system valueandon conflict do nothingagainst the uniqueglobal_position: a committed row makes it a no-op, an in-flight row makes it block until that transaction resolves. It cannot displace a real event.GapSkipTimeoutMshas no such interlock — it advances on elapsed time alone, so an append slower than the timeout has its event skipped.Previously the release cleared
gapbefore the remediation check inPollOnce, which is gated on a non-null gap, so settingGapHandlingTimeoutMsat or aboveGapSkipTimeoutMssilently produced no tombstones and skipped a position it could have resolved. Remediation is now tracked onDetectedGap.RemediationAttemptedand takes precedence.GapSkipTimeoutMsnow defaults tonull, making time-based skipping opt-in. Note this is only safe together with the age-threshold reordering above — without it, a null skip timeout returns unconditionally at the held-gap branch and stalls indefinitely.The XML docs described behaviour the code did not have, and have been corrected.
Tests
GapSkipTimeoutTest— asserts the hold at a quarter of the timeout, then the release. Before the fix: stalled the full 10s deadline with the checkpoint parked atPosition = 2.GapRemediationBeforeSkipTest— both timeouts equal, so remediation only runs if it takes precedence. Before the fix:Expected to be 1 but found 0tombstones.GapAgeReleaseWhileRunningTest— a gap detected while the following event is young, ageing out while subscribed, both timeouts null. Before the reordering:Expected to be 5 but found 2.GapIgnoreTestdid not cover this, as it only tests a gap already old at startup.Each was verified to fail before its fix and pass after. Locally on net10.0: Postgres 52/52, SQLite 43/43, 0 warnings. SQL Server is excluded on macOS and runs in CI.
Follow-ups, not in this PR
ExecutePollCycledoesTask.Delay(InitialDelayMs * retryCount++)— the first retry waits 0ms and the delay then grows unbounded.Eventuous.Sql.Basehas no test project of its own to cover it.HandleGapTimeoutnorShouldSkipEvent, so they have no safe resolution available — only the opt-in time-based skip.🤖 Generated with Claude Code