Skip to content

GH-4028: make UseDurableInbox() on a Redis stream a real durable inbox - #4031

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-4028/redis-durable-inbox
Aug 23, 2026
Merged

GH-4028: make UseDurableInbox() on a Redis stream a real durable inbox#4031
jeremydmiller merged 1 commit into
mainfrom
gh-4028/redis-durable-inbox

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #4028 (option B). Found while auditing arrival-side batching for #4026.

The defect

RedisStreamEndpoint implemented IDatabaseBackedEndpoint to get Redis-native scheduled retries out of DurableReceiver. That marker does two things in DurableReceiver, and Redis only wanted one:

  1. routes scheduled retries to endpoint.ScheduleRetryAsync (intended);
  2. sets ShouldPersistBeforeProcessing = false and completes the delivery on receipt — which for Redis is XACK before the handler runs, with no inbox write at all.

So ListenToRedisStream(...).UseDurableInbox() was at-most-once on a crash — the exact semantics the mode name says it avoids — and the Redis test suite ran "durable" listeners with no message store configured, which only worked because of this. With a real store configured, every completion also issued a mark-as-handled UPDATE against a row that never existed.

The fix

  • RedisStreamEndpoint is no longer IDatabaseBackedEndpoint. A Durable Redis listener now behaves like every other durable transport: INSERT into the inbox before the stream entry is acknowledged, handled from the inbox, scheduled retries parked in the inbox. It requires a configured message store, as everywhere else.
  • Redis-native scheduled retries are kept for the non-durable modes via RedisStreamListener : ISupportNativeScheduling (NativeSchedulingEnabled => Mode != Durable): park the copy in the stream's scheduled sorted set, then ACK the original (that order so a crash in between costs a duplicate, not a loss). This is strictly better than before for Buffered (was in-memory scheduling, lost on crash) and Inline (was the inbox, which needed a store). RedisStreamEndpoint.ScheduleRetryAsync stays as the method behind it, with docs explaining why.
  • A Durable listener deliberately reports NativeSchedulingEnabled == false: MessageContext.ReScheduleAsync prefers a listener-level rescheduler and never completes the envelope in that path, so re-adding the same envelope id to the stream would leave the inbox row orphaned and the redelivered copy would be discarded as a duplicate. The inbox is the only correct place for a durable scheduled retry.

Tests

Result
new durable_inbox_is_real_4028 (Redis + Postgres inbox): (a) with the handler parked, the inbox shows the row Incoming and the consumer group's pending list is already empty — durable, then acked; (b) a ScheduleRetry lands in the inbox as Scheduled, the Redis scheduled sorted set stays empty, and the retry still runs; (c) the endpoint is not IDatabaseBackedEndpoint 3/3
EndToEndRetryTests, rate_limiting_end_to_end — both assert the Redis-native scheduled set, i.e. the non-durable path, and both used UseDurableInbox() with no store; switched to BufferedInMemory() (the code path they were really testing)
DatabaseBackedEndpointTests, NativeSchedulingRetryTests — now assert the endpoint is not database-backed and the listener owns native scheduling; the ScheduleRetryAsync sorted-set tests are unchanged
full Wolverine.Redis.Tests 147 / 147
dotnet build wolverine.slnx -c Release -f net9.0 clean

Docs / behaviour change

New "Durable Inbox" section in docs/guide/messaging/transports/redis.md with a warning box describing the pre-6.30 behaviour and the migration: if you were running UseDurableInbox() on Redis without a store you now need one; if you actually wanted Redis-native scheduled retries without a database, the default BufferedInMemory() (or ProcessInline()) listener is that. The scheduling sample no longer pairs UseDurableInbox() with "scheduled natively in Redis".

This is a behaviour change for anyone on Redis "durable" today — but the behaviour they had was not durable, so I'd argue it's a bug fix with a loud note rather than a major-version item. Flagging it explicitly so you can disagree.

🤖 Generated with Claude Code

RedisStreamEndpoint was IDatabaseBackedEndpoint so that DurableReceiver would route scheduled
retries to the endpoint's Redis-native sorted set. That marker also told DurableReceiver to skip
the inbox INSERT and complete the delivery on receipt, so a "durable" Redis listener never wrote
the inbox and XACKed every message before its handler ran -- at-most-once on a crash, the opposite
of what the mode promises -- and the Redis tests ran UseDurableInbox() with no message store at all.

Option B from the issue: drop IDatabaseBackedEndpoint. A Durable Redis listener now goes through
the real inbox like every other transport -- INSERT before ACK, handled from the inbox, scheduled
retries parked in the inbox -- and therefore requires a message store. Redis-native scheduled
retries are kept for the modes where they belong: RedisStreamListener implements
ISupportNativeScheduling with NativeSchedulingEnabled => Mode != Durable, parking the copy in the
scheduled sorted set and then acknowledging the original; that is an improvement for Buffered
(previously in-memory, lost on crash) and Inline (previously the inbox, which needed a store). A
Durable listener deliberately reports false there: a listener-level reschedule bypasses the inbox
and re-adding the same envelope id to the stream would collide with the inbox row and be
discarded as a duplicate on redelivery.

Tests: new durable_inbox_is_real_4028 (Redis + Postgres inbox: the row is Incoming while the
handler is parked, the stream entry is already acknowledged, and a scheduled retry lands in the
inbox with the Redis scheduled set untouched, then retries). EndToEndRetryTests and
rate_limiting_end_to_end move to BufferedInMemory (they assert the Redis-native scheduled set,
which is the non-durable path now); DatabaseBackedEndpointTests / NativeSchedulingRetryTests
assert the endpoint is NOT IDatabaseBackedEndpoint and the listener is ISupportNativeScheduling.
Full Wolverine.Redis.Tests 147/147. Docs: new "Durable Inbox" section with the behaviour change
called out, and the scheduling sample no longer pairs UseDurableInbox() with "scheduled natively
in Redis".

Closes #4028

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jeremydmiller

Copy link
Copy Markdown
Member Author

Reviewed. The fix is right and the reasoning in the description holds up — one real defect, which CI will not catch.

ClearAllWolverineStorageAsync silently stops resetting Redis streams

StorageExtensions.cs:39-43 selects the queues it resets with exactly the marker this PR removes:

var queues = transport.Endpoints()
    .OfType<IBrokerQueue>()
    .Where(x => x is IDatabaseBackedEndpoint)
    .ToArray();

if (queues.Length == 0) continue;

With RedisStreamEndpoint no longer IDatabaseBackedEndpoint, that array is empty for RedisTransport, the whole transport is continued, and Redis stream endpoints never get their SetupAsync/PurgeAsync. StorageExtensions.cs isn't in the diff.

Three things make this worth fixing rather than just noting:

  • The comment immediately above it becomes false. StorageExtensions.cs:37 enumerates the covered transports as "PostgreSQL, SQL Server, MySQL, Oracle, SQLite, and Redis streams".
  • Nothing catches it. There is no Redis implementation of ClearAllWolverineStorageCompliance — only Sqlite, Oracle, Postgresql, SqlServer and MySql — and no Redis test calls the helper. This can go green across all 37 checks with the regression in.
  • docs/guide/testing.md:835 presents that helper as the integration-test reset, so a Redis user following the docs silently starts carrying stream entries between runs.

The root cause is the same shape as the bug this PR fixes: IDatabaseBackedEndpoint carries three unrelated jobs on one marker — the native-retry route (DurableReceiver.cs:201-205), the skip-the-inbox/complete-on-receipt switch (DurableReceiver.cs:52), and this reset selector. The PR correctly separates the first two and inherits the third by accident. Fix is either a distinct marker meaning "has queue state to reset", or naming RedisStreamEndpoint explicitly in StorageExtensions — plus correcting that comment either way.

What I checked and found correct

  • The new test genuinely fails without a store. This was my main concern going in, since a Wolverine host with no store configured silently resolves NullMessageStore — which is precisely how the old "durable" Redis tests asserted durable behaviour while proving nothing. durable_inbox_is_real_4028 configures PersistMessagesWithPostgresql and resolves IMessageStore to assert on inbox rows directly, so it cannot pass against NullMessageStore. That is the load-bearing part of this PR.
  • Connection string comes from Servers.PostgresConnectionString, not a literal — no hardcoded 5432 to fail in CI.
  • The Buffered path is actually covered, not just implied by the Durable one. EndToEndRetryTests and rate_limiting_end_to_end move to BufferedInMemory() and assert the Redis scheduled sorted set, exercising the new ISupportNativeScheduling seam on the listener. Worth calling out because DurableReceiver and BufferedReceiver implement that interface separately.
  • The ordering in MoveToScheduledUntilAsync is the right way round — park in the sorted set, then CompleteAsync, so a crash between them costs a duplicate rather than a loss. The inline comment says so, which is the kind of thing that gets silently "tidied" later without one.
  • NativeSchedulingEnabled => Mode != Durable is justified and the stated reason holds: a listener-level reschedule never completes the envelope, so re-adding the same envelope id to the stream would orphan the inbox row and the redelivery would be discarded as a duplicate.

On the behaviour-change question

Agree with the framing in the description. What users had was not durable — it ACKed before the handler ran — so restoring the documented meaning is a bug fix, not a breaking change to a working feature. The loud docs warning is the right treatment. The one group who genuinely lose something are those running UseDurableInbox() on Redis with no store purely to get native scheduled retries; the migration note pointing them at BufferedInMemory() covers that, and they now get better crash behaviour than before.

CI was 37/37 pending when I looked, so this is a read of the diff rather than of a green run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redis Streams: UseDurableInbox() acks on receipt and never writes the inbox — IDatabaseBackedEndpoint makes it at-most-once, not durable

1 participant