From dcb14e1b306114390f169f37b511347d39320090 Mon Sep 17 00:00:00 2001 From: Pavel Borisov Date: Thu, 13 Aug 2026 23:50:34 +0200 Subject: [PATCH 1/2] test(subscriptions): reproduce the resubscribe and teardown defects A seconds-long store blip wedged twenty subscriptions for seven hours, and nothing in the suite could have caught it. Twelve of these fail on every run and the thirteenth whenever the race lands, so the fix has something to prove and the next regression has somewhere to land. The provider suites carry the restart contract itself: a fake transport can show the framework calls teardown once, but only real infrastructure can show a given broker survives being restarted. Co-Authored-By: Claude Opus 5 (1M context) --- .../SubscriptionRestartBase.cs | 102 +++ .../ResubscribeConcurrencyTests.cs | 638 ++++++++++++++++++ .../TransportTeardownTests.cs | 70 ++ .../TeardownTests.cs | 41 ++ .../Subscriptions/SubscriptionRestartTests.cs | 25 + .../Subscriptions/SubscriptionRestartTests.cs | 25 + .../Subscriptions/PollFailureTests.cs | 94 +++ .../Subscriptions/SubscriptionRestartTests.cs | 24 + 8 files changed, 1019 insertions(+) create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs create mode 100644 src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs create mode 100644 src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/SubscriptionRestartTests.cs create mode 100644 src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/SubscriptionRestartTests.cs create mode 100644 src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs create mode 100644 src/SqlServer/test/Eventuous.Tests.SqlServer/Subscriptions/SubscriptionRestartTests.cs diff --git a/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs b/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs new file mode 100644 index 000000000..3970a3819 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs @@ -0,0 +1,102 @@ +using DotNet.Testcontainers.Containers; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Sut.App; +using Eventuous.Tests.Persistence.Base.Fixtures; +using static Eventuous.Sut.App.Commands; +using static Eventuous.Sut.Domain.BookingEvents; + +namespace Eventuous.Tests.Subscriptions.Base; + +/// +/// The two properties the resubscribe path relies on from every transport. Since a drop now stops the +/// previous run before starting the next, teardown runs on a connection that will be used again, and a +/// transport whose resources are single-use has to rebuild them rather than restart them. Stated here so +/// each provider suite can assert it against real infrastructure. +/// +public abstract class SubscriptionRestartBase( + SubscriptionFixtureBase fixture + ) : SubscriptionTestBase(fixture) + where TContainer : DockerContainer + where TSubscription : EventSubscription + where TSubscriptionOptions : SubscriptionOptions + where TCheckpointStore : class, ICheckpointStore { + const int BatchSize = 5; + + static readonly TimeSpan ConsumeTimeout = TimeSpan.FromSeconds(30); + + /// + /// Unsubscribing twice must not throw. The framework only calls transport teardown for a live run, but + /// a provider may still be asked to release resources it has already released — through an explicit + /// stop, or by a drop landing while shutdown is in flight. + /// + protected async Task ShouldTolerateRepeatedUnsubscribe() { + await fixture.StartSubscription(); + await fixture.StopSubscription(); + await fixture.StopSubscription(); + } + + /// + /// Subscribing again after a full stop must consume newly produced events. Asserts by event identity, + /// so a replay of the first batch can't pass for the second. + /// + protected async Task ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { + var started = false; + + try { + var first = (await GenerateAndHandleCommands(BatchSize)).Select(ToEvent).ToList(); + await fixture.StartSubscription(); + started = true; + await Assert.That(await WaitForEvents(first, cancellationToken)).IsTrue(); + + await fixture.StopSubscription(); + started = false; + WriteLine("Subscription stopped, starting it again on the same instance"); + + await fixture.StartSubscription(); + started = true; + + var second = (await GenerateAndHandleCommands(BatchSize)).Select(ToEvent).ToList(); + var consumed = await WaitForEvents(second, cancellationToken); + + await fixture.StopSubscription(); + started = false; + + await Assert.That(consumed).IsTrue(); + } finally { + if (started) { + try { + await fixture.StopSubscription(); + } catch (Exception ex) { WriteLine("Cleanup: failed to stop the subscription: {0}", ex.Message); } + } + } + } + + async Task WaitForEvents(List expected, CancellationToken cancellationToken) { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(ConsumeTimeout); + + try { + while (true) { + var handled = fixture.Handler.Handled; + if (expected.All(handled.Contains)) return true; + + await Task.Delay(200, cts.Token); + } + } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { return false; } + } + + async Task> GenerateAndHandleCommands(int count) { + var commands = Enumerable.Range(0, count).Select(_ => DomainFixture.CreateImportBooking()).ToList(); + var service = new BookingService(fixture.EventStore); + + foreach (var cmd in commands) { + var result = await service.Handle(cmd, default); + result.ThrowIfError(); + } + + return commands; + } + + static BookingImported ToEvent(ImportBooking cmd) => new(cmd.RoomId, cmd.Price, cmd.CheckIn, cmd.CheckOut); +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs new file mode 100644 index 000000000..b8ece0979 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs @@ -0,0 +1,638 @@ +using System.Collections.Concurrent; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Eventuous.Tools; +using Microsoft.Extensions.Logging; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// One failure has to cost one resubscribe. A transport drop reaches Dropped once for the pump's read +/// and once for every in-flight message on the same dead connection; a handler deferring by throwing +/// under ThrowOnError produces the same burst. Either way: one drop cycle, one live transport, no +/// accumulation, and a checkpoint that keeps moving afterwards. +/// +public class ResubscribeConcurrencyTests { + /// + /// A whole page of messages fails while the pump keeps delivering, so Dropped is called once per + /// failing message, all inside one drop window. + /// + [Test] + public async Task Burst_of_nacks_produces_a_single_resubscribe(CancellationToken ct) { + var logs = new RecordingLoggerFactory(); + + const int messageCount = 8; + + var handler = new DeferringHandler(_ => true); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "burst-of-nacks", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(handler), + logs, + concurrencyLimit: 4, + // Only the first run delivers: a replacement redelivering the same failing messages would + // legitimately open a second drop window. + pump: async (sub, transport, start, token) => { + if (transport.Index == 0) { + for (var i = 0; i < messageCount; i++) await sub.Deliver(start + (ulong)i, token).NoContext(); + } + + await Task.Delay(Timeout.Infinite, token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(500) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + // All of them have to fail before the resubscribe fires, or this proves nothing about + // concurrent drops. + (await WaitUntil(() => handler.HandledCount >= messageCount, TimeSpan.FromSeconds(5))) + .ShouldBeTrue($"all {messageCount} messages should have been handled and nacked, got {handler.HandledCount}"); + + (await WaitUntil(() => subscription.SubscribeCalls > 1, TimeSpan.FromSeconds(5))).ShouldBeTrue("the subscription should have resubscribed"); + + // Give any extra resubscribes scheduled by the other nacks time to show up before asserting. + await Task.Delay(TimeSpan.FromSeconds(1), ct); + + await subscription.Unsubscribe(_ => { }, ct); + + subscription.SubscribeCalls.ShouldBe(2, "one initial subscribe plus exactly one resubscribe for the whole drop cycle"); + logs.Count("Resubscribing").ShouldBe(1, "a burst of nacks is one drop cycle, so it gets one 'Resubscribing' line"); + logs.Count("Dropped:").ShouldBe(1, "the drop is reported once per cycle, not once per failing message"); + } + + /// + /// No handler involvement at all: the connection dies, so the pump's read throws and every in-flight + /// operation on it fails at the same moment. One transport failure, one resubscribe. + /// + [Test] + public async Task Transport_failure_produces_a_single_resubscribe(CancellationToken ct) { + const int inFlight = 4; + const ulong last = 7; + + var logs = new RecordingLoggerFactory(); + var store = new NoOpCheckpointStore(); + var handler = new ConnectionBoundHandler(t => t == 0); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "transport-failure", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + store, + new ConsumePipe().AddDefaultConsumer(handler), + logs, + concurrencyLimit: inFlight, + pump: async (sub, transport, start, token) => { + if (transport.Index > 0) { + // The store is back. Nothing about this run is unusual. + for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + + return; + } + + for (var i = 0; i < inFlight; i++) await sub.Deliver((ulong)i, token, transport.Index).NoContext(); + + // Wait until they're in flight, so the failure hits all of them at once. + await WaitUntil(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); + + // HTTP/2 INTERNAL_ERROR: the connection is gone, and everything using it fails together. + handler.KillConnection(transport.Index); + + // What AllStreamSubscription.PumpMessages does when MoveNextAsync throws. + sub.Drop(new IOException("The HTTP/2 server closed the connection")); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(300) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + // (c) The checkpoint has to keep moving afterwards. + var recovered = await WaitUntil( + async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, + TimeSpan.FromSeconds(15) + ); + + // Let any resubscribe scheduled by the other nacks arrive before counting. + await Task.Delay(TimeSpan.FromSeconds(1), ct); + await subscription.Unsubscribe(_ => { }, ct); + + recovered.ShouldBeTrue("the subscription must commit again after the connection comes back"); + + // (a) One transport failure, one drop cycle, one resubscribe. + subscription.SubscribeCalls.ShouldBe(2, "one initial subscribe plus exactly one resubscribe for the whole connection failure"); + logs.Count("Resubscribing").ShouldBe(1, "the pump's drop and the in-flight nacks are one drop cycle between them"); + logs.Count("Dropped:").ShouldBe(1, "the drop is reported once per cycle, not once per failed operation"); + + // (b) The run that owned the dead connection is gone. + var transports = subscription.Transports.ToArray(); + transports[0].IsDisposed.ShouldBeTrue("the dead connection should have been disposed"); + transports[0].PumpExited.ShouldBeTrue("the pump reading the dead connection should have exited"); + subscription.MaxLivePumps.ShouldBe(1, "there should never be more than one message pump alive at a time"); + } + + /// + /// The same failure over and over — a rolling restart, or a flapping node. Nothing may accumulate, + /// and the checkpoint has to end up where it would have without any of it. + /// + [Test] + public async Task Repeated_transport_failures_do_not_accumulate(CancellationToken ct) { + const int cycles = 25; + const int inFlight = 4; + const ulong last = 7; + + var store = new NoOpCheckpointStore(); + var handler = new ConnectionBoundHandler(t => t < cycles); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "repeated-transport-failure", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + store, + new ConsumePipe().AddDefaultConsumer(handler), + new RecordingLoggerFactory(), + concurrencyLimit: inFlight, + pump: async (sub, transport, start, token) => { + if (transport.Index >= cycles) { + // The store settles down and the subscription catches up. + for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + + return; + } + + for (var i = start; i < start + inFlight && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + + await WaitUntil(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); + handler.KillConnection(transport.Index); + sub.Drop(new IOException("The HTTP/2 server closed the connection")); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(10) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + var recovered = await WaitUntil( + async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, + TimeSpan.FromSeconds(60) + ); + + await subscription.Unsubscribe(_ => { }, ct); + + recovered.ShouldBeTrue($"the checkpoint should have caught up after {cycles} connection failures"); + + subscription.SubscribeCalls.ShouldBe(cycles + 1, "each connection failure should cost exactly one resubscribe"); + subscription.MaxLivePumps.ShouldBe(1, "message pumps accumulated across connection failures"); + subscription.Transports.Count(t => !t.IsDisposed).ShouldBe(0, "every dead connection should have been disposed"); + subscription.Transports.Count.ShouldBe(subscription.SubscribeCalls, "each subscribe should create exactly one connection"); + } + + /// + /// The dropped run's pump must be gone before the next one starts. An orphan keeps reading into the + /// same consume pipe, and every message it delivers is a duplicate. + /// + [Test] + public async Task Resubscribe_stops_the_previous_transport_and_pump(CancellationToken ct) { + var deliveries = new ConcurrentQueue<(int Transport, ulong Position)>(); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "stop-previous-pump", + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new CountingHandler()), + new RecordingLoggerFactory(), + concurrencyLimit: 1, + pump: async (sub, transport, start, token) => { + var position = start; + + while (!token.IsCancellationRequested) { + deliveries.Enqueue((transport.Index, position)); + await sub.Deliver(position++, token).NoContext(); + await Task.Delay(5, token).NoContext(); + } + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(100) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await WaitUntil(() => deliveries.Count > 3, TimeSpan.FromSeconds(5))).ShouldBeTrue("the first pump should be delivering"); + + subscription.Drop(new InvalidOperationException("Simulated transport drop")); + + (await WaitUntil(() => deliveries.Any(d => d.Transport == 1), TimeSpan.FromSeconds(5))).ShouldBeTrue("the replacement pump should be delivering"); + + await Task.Delay(TimeSpan.FromMilliseconds(300), ct); + await subscription.Unsubscribe(_ => { }, ct); + + var transports = subscription.Transports.ToArray(); + transports.Length.ShouldBe(2); + + transports[0].IsDisposed.ShouldBeTrue("the dropped transport should have been disposed before the new one was created"); + transports[0].PumpExited.ShouldBeTrue("the dropped run's message pump should have exited"); + + // Once the replacement pump has delivered anything, the old one must never be heard from again. + var recorded = deliveries.ToArray(); + var firstOnNew = Array.FindIndex(recorded, d => d.Transport == 1); + var afterSwitch = recorded.Skip(firstOnNew); + + afterSwitch.ShouldAllBe(d => d.Transport == 1, "two live pumps were dispatching into the same pipe"); + subscription.MaxLivePumps.ShouldBe(1, "there should never be more than one message pump alive at a time"); + } + + /// + /// Two contexts sharing a sequence number collapse into one entry in the commit handler's position + /// set, which then refuses to commit past the resulting hole. + /// + [Test] + public async Task Concurrent_context_creation_yields_unique_sequences() { + const int threads = 8; + const int perThread = 2000; + + var subscription = new PumpingSubscription( + new() { SubscriptionId = "unique-sequences" }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new CountingHandler()), + new RecordingLoggerFactory(), + concurrencyLimit: 1, + pump: (_, _, _, token) => Task.Delay(Timeout.Infinite, token) + ); + + var results = new ulong[threads][]; + + await Task.WhenAll( + Enumerable.Range(0, threads) + .Select(t => Task.Run(() => { + var mine = new ulong[perThread]; + + for (var i = 0; i < perThread; i++) mine[i] = subscription.CreateContext(0).Sequence; + + results[t] = mine; + } + ) + ) + ); + + var all = results.SelectMany(x => x).ToArray(); + + all.Length.ShouldBe(threads * perThread); + all.Distinct().Count().ShouldBe(all.Length, "sequence numbers must be unique across concurrent context creation"); + all.Order().ShouldBe(Enumerable.Range(0, all.Length).Select(i => (ulong)i), "sequence numbers must be a gapless monotonic run"); + + // Each thread must see its own values increase, so the sequence is monotonic per producer too. + foreach (var mine in results) mine.ShouldBeInOrder(SortDirection.Ascending); + } + + /// + /// The steady state of a deferring handler: the same message fails on every redelivery, the + /// checkpoint holds at the last one before it, and nothing accumulates while that goes on. + /// + [Test] + public async Task Repeated_nacks_hold_a_stable_checkpoint_without_accumulating(CancellationToken ct) { + const int cycles = 100; + const ulong failAt = 3; + + var stored = new ConcurrentQueue(); + var store = new NoOpCheckpointStore(); + store.CheckpointStored += (_, cp) => stored.Enqueue(cp.Position); + + var handler = new DeferringHandler(context => context.GlobalPosition >= failAt); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "stable-checkpoint", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + store, + new ConsumePipe().AddDefaultConsumer(handler), + new RecordingLoggerFactory(), + concurrencyLimit: 1, + pump: async (sub, _, start, token) => { + for (var i = start; i < start + 5 && !token.IsCancellationRequested; i++) await sub.Deliver(i, token).NoContext(); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(10) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await WaitUntil(() => subscription.SubscribeCalls > cycles, TimeSpan.FromSeconds(60))) + .ShouldBeTrue($"the subscription should have gone through {cycles} drop cycles, it did {subscription.SubscribeCalls - 1}"); + + await subscription.Unsubscribe(_ => { }, ct); + + // The failing message is never acked, so the checkpoint parks on the one before it. A corrupted + // sequence shows up as a checkpoint that never commits, or one that jumps past the failure. + stored.ShouldNotBeEmpty("the messages before the failing one should have been committed"); + stored.ShouldAllBe(p => p < failAt, "the checkpoint must never advance past the message that keeps failing"); + stored.Last().ShouldBe(failAt - 1, "the checkpoint should settle on the last message before the failing one"); + (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position.ShouldBe(failAt - 1); + + // Only the messages ahead of the failure, plus the flush when the first commit handler is + // replaced. More than that means the checkpoint is being rewritten every cycle. + stored.Count.ShouldBeLessThanOrEqualTo((int)failAt + 1, $"the checkpoint should settle, not be rewritten across {cycles} cycles"); + + handler.HandledCount.ShouldBeGreaterThanOrEqualTo(cycles, "the failing message must be redelivered on every cycle"); + + // Nothing accumulates: one transport and one pump at a time, no matter how many cycles. + subscription.MaxLivePumps.ShouldBe(1, "message pumps accumulated across drop cycles"); + subscription.Transports.Count(t => !t.IsDisposed).ShouldBe(0, "every transport should have been disposed by the time the subscription stops"); + subscription.Transports.Count.ShouldBe(subscription.SubscribeCalls, "each subscribe should create exactly one transport"); + } + + /// + /// Nothing commits past a gap in the sequence, so a subscription whose checkpoint stalled needs a + /// clean commit handler from the next subscribe — or it processes forever without ever committing. + /// + [Test] + public async Task Subscription_commits_again_after_a_commit_gap(CancellationToken ct) { + const ulong failAt = 3; + const ulong last = 7; + + var store = new NoOpCheckpointStore(); + var defer = true; + + // Fails on one message until the test lets it through, exactly like a precondition that resolves. + var handler = new DeferringHandler(context => defer && context.GlobalPosition == failAt); + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "recover-after-gap", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + store, + new ConsumePipe().AddDefaultConsumer(handler), + new RecordingLoggerFactory(), + concurrencyLimit: 1, + pump: async (sub, _, start, token) => { + for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token).NoContext(); + + await Task.Delay(Timeout.Infinite, token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(50) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + // The messages after the failing one ack out of order, leaving a hole nothing commits past. + var stalled = await WaitUntil( + async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == failAt - 1, + TimeSpan.FromSeconds(10) + ); + + stalled.ShouldBeTrue("the checkpoint should have stalled just before the failing message"); + + // The precondition resolves. Nothing else changes — no restart, no manual intervention. + defer = false; + + var recovered = await WaitUntil( + async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, + TimeSpan.FromSeconds(15) + ); + + await subscription.Unsubscribe(_ => { }, ct); + + recovered.ShouldBeTrue("a subscription whose checkpoint stalled must commit again once the failing message succeeds"); + } + + static Task WaitUntil(Func condition, TimeSpan timeout) => WaitUntil(() => Task.FromResult(condition()), timeout); + + static async Task WaitUntil(Func> condition, TimeSpan timeout) { + var deadline = DateTime.UtcNow + timeout; + + while (DateTime.UtcNow < deadline) { + if (await condition()) return true; + + await Task.Delay(20); + } + + return await condition(); + } + + const string TransportKey = "transport"; + + record TestOptions : SubscriptionWithCheckpointOptions; + + /// + /// Stands in for a transport connection, recording only whether the subscription got rid of it and + /// whether the pump reading it has finished. + /// + sealed class FakeTransport(int index) : IAsyncDisposable { + public int Index { get; } = index; + public bool IsDisposed { get; private set; } + public bool PumpExited { get; set; } + + public ValueTask DisposeAsync() { + IsDisposed = true; + + return default; + } + } + + /// + /// Shaped like the real catch-up subscriptions: Subscribe creates a transport and hands it to a pump + /// on its own task, Unsubscribe drops the transport and waits for the pump. Pump body per test. + /// + sealed class PumpingSubscription( + TestOptions options, + ICheckpointStore checkpointStore, + ConsumePipe pipe, + ILoggerFactory? loggerFactory, + int concurrencyLimit, + Func pump + ) + : EventSubscriptionWithCheckpoint(options, checkpointStore, pipe, concurrencyLimit, SubscriptionKind.All, loggerFactory, null, null) { + readonly ConcurrentQueue _transports = []; + + FakeTransport? _transport; + Task? _pumpTask; + int _subscribeCalls; + int _livePumps; + int _maxLivePumps; + + public IReadOnlyCollection Transports => _transports; + public int SubscribeCalls => Volatile.Read(ref _subscribeCalls); + public int MaxLivePumps => Volatile.Read(ref _maxLivePumps); + + public TimeSpan ResubscribeDelay { get; init; } = TimeSpan.FromMilliseconds(100); + + public void Drop(Exception exception) => Dropped(DropReason.SubscriptionError, exception); + + public MessageConsumeContext CreateContext(ulong position, int transport = 0) + => new MessageConsumeContext( + Guid.NewGuid().ToString(), + "TestEvent", + "application/json", + "test-stream", + position, + position, + position, + Sequence++, + DateTime.UtcNow, + new { Position = position }, + new(), + Options.SubscriptionId, + CancellationToken.None + ) { LogContext = Log } + .WithItem(TransportKey, transport); + + public ValueTask Deliver(ulong position, CancellationToken cancellationToken, int transport = 0) { + var context = CreateContext(position, transport); + context.CancellationToken = cancellationToken; + + return HandleInternal(context); + } + + // The production delays are 2s and 10s; a hundred cycles of those would take half an hour. + protected override Task Resubscribe(TimeSpan delay, CancellationToken cancellationToken) => base.Resubscribe(ResubscribeDelay, cancellationToken); + + protected override async ValueTask Subscribe(CancellationToken cancellationToken) { + var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); + var start = position == null ? 0 : position.Value + 1; + + var transport = new FakeTransport(Interlocked.Increment(ref _subscribeCalls) - 1); + _transports.Enqueue(transport); + _transport = transport; + + _pumpTask = Task.Run( + async () => { + TrackPumpStarted(); + + try { await pump(this, transport, start, cancellationToken).NoContext(); } catch (Exception) { + // The pump ending is what the tests observe, not how it ended + } finally { + Interlocked.Decrement(ref _livePumps); + transport.PumpExited = true; + } + }, + CancellationToken.None + ); + } + + protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { + var transport = _transport; + var pump = _pumpTask; + _transport = null; + _pumpTask = null; + + // As in the real subscriptions: cancelling a source shutdown already disposed is benign. + try { Stopping.Cancel(false); } catch (ObjectDisposedException) { } + + if (transport != null) await transport.DisposeAsync().NoContext(); + + if (pump != null) await Task.WhenAny(pump, Task.Delay(TimeSpan.FromSeconds(5), cancellationToken)).NoContext(); + } + + void TrackPumpStarted() { + var live = Interlocked.Increment(ref _livePumps); + + int observed; + + do { + observed = Volatile.Read(ref _maxLivePumps); + + if (observed >= live) return; + } while (Interlocked.CompareExchange(ref _maxLivePumps, live, observed) != observed); + } + } + + /// + /// Defers by throwing when a precondition isn't met, so the message isn't acknowledged and gets + /// redelivered after the resubscribe. + /// + sealed class DeferringHandler(Func shouldDefer) : BaseEventHandler { + int _handled; + + public int HandledCount => Volatile.Read(ref _handled); + + public override ValueTask HandleEvent(IMessageConsumeContext context) { + if (!shouldDefer(context)) return new(EventHandlingStatus.Success); + + Interlocked.Increment(ref _handled); + + throw new InvalidOperationException($"Precondition not met for {context.Stream}:{context.GlobalPosition}"); + } + } + + /// + /// Does I/O over the same connection the subscription reads from, so it fails the moment that + /// connection dies — which is how one transport failure becomes a batch of simultaneous nacks. + /// + sealed class ConnectionBoundHandler(Func holdsUntilConnectionDies) : BaseEventHandler { + readonly ConcurrentDictionary _dead = []; + + int _inFlight; + + public int InFlight => Volatile.Read(ref _inFlight); + + public void KillConnection(int transport) => Killed(transport).TrySetResult(); + + TaskCompletionSource Killed(int transport) => _dead.GetOrAdd(transport, _ => new(TaskCreationOptions.RunContinuationsAsynchronously)); + + public override async ValueTask HandleEvent(IMessageConsumeContext context) { + var transport = context.Items.GetItem(TransportKey); + var killed = Killed(transport).Task; + + if (killed.IsCompleted) throw Dead(transport); + + if (!holdsUntilConnectionDies(transport)) return EventHandlingStatus.Success; + + // Hold the operation open, so the failure catches the whole batch at once. + Interlocked.Increment(ref _inFlight); + + try { await killed.WaitAsync(context.CancellationToken).NoContext(); } finally { Interlocked.Decrement(ref _inFlight); } + + throw Dead(transport); + } + + static IOException Dead(int transport) => new($"The HTTP/2 server closed the connection (transport {transport})"); + } + + sealed class CountingHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } + + sealed class RecordingLoggerFactory : ILoggerFactory { + readonly ConcurrentQueue _messages = []; + + public int Count(string contains) => _messages.Count(m => m.Contains(contains)); + + public ILogger CreateLogger(string categoryName) => new RecordingLogger(_messages); + + public void AddProvider(ILoggerProvider provider) { } + + public void Dispose() { } + + sealed class RecordingLogger(ConcurrentQueue messages) : ILogger { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + => messages.Enqueue(formatter(state, exception)); + } + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs new file mode 100644 index 000000000..ac6c2f9d4 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs @@ -0,0 +1,70 @@ +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// A transport's Unsubscribe releases whatever its Subscribe built, and some of what it releases is +/// single-use: a Google Pub/Sub SubscriberClient cannot be started twice, and an Azure Service Bus +/// processor is replaced rather than reused. So teardown has to run once per run, and it is the +/// framework's job to make sure of it — otherwise every transport has to be defensively idempotent. +/// +public class TransportTeardownTests { + [Test] + public async Task Teardown_runs_once_when_unsubscribe_is_called_twice(CancellationToken ct) { + var subscription = new CountingSubscription(); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + await subscription.Unsubscribe(_ => { }, ct); + await subscription.Unsubscribe(_ => { }, ct); + + subscription.Stops.ShouldBe(1, "the transport was torn down again with nothing left to tear down"); + } + + [Test] + public async Task Teardown_does_not_run_when_the_subscription_never_started(CancellationToken ct) { + var subscription = new CountingSubscription(); + + await subscription.Unsubscribe(_ => { }, ct); + + subscription.Stops.ShouldBe(0, "a transport that was never subscribed has nothing to release"); + } + + record TestOptions : SubscriptionOptions; + + /// + /// Counts what the framework asks of a transport. It holds no resources, because the point is the + /// call pattern rather than what a real transport would do with it. + /// + sealed class CountingSubscription() + : EventSubscription( + new() { SubscriptionId = "transport-teardown" }, + new ConsumePipe().AddDefaultConsumer(new NoOpHandler()), + null, + null + ) { + int _starts; + int _stops; + + public int Starts => Volatile.Read(ref _starts); + public int Stops => Volatile.Read(ref _stops); + + protected override ValueTask Subscribe(CancellationToken cancellationToken) { + Interlocked.Increment(ref _starts); + + return default; + } + + protected override ValueTask Unsubscribe(CancellationToken cancellationToken) { + Interlocked.Increment(ref _stops); + + return default; + } + } + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } +} diff --git a/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs b/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs new file mode 100644 index 000000000..45d97643e --- /dev/null +++ b/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs @@ -0,0 +1,41 @@ +using Eventuous.GooglePubSub.Subscriptions; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; + +namespace Eventuous.Tests.GooglePubSub; + +/// +/// A SubscriberClient cannot be started twice — the SDK documents StartAsync as callable once per +/// instance — so the subscription builds a new one on every Subscribe. That only holds together if +/// teardown releases the one it stopped and never touches it again. No emulator is involved: the +/// constructor only resolves resource names, so teardown can be exercised on its own. +/// +/// Both tests assert by completing. An exception out of teardown fails them. +/// +public class TeardownTests { + [Test] + public async Task Teardown_without_a_subscribe_completes(CancellationToken ct) { + var subscription = Create(); + + // Reachable in production, not only from a test: Subscribe records the run before awaiting the + // transport, so a Subscribe that throws part-way leaves shutdown tearing down a run that never + // got as far as its subscriber task. + await subscription.Unsubscribe(_ => { }, ct); + } + + [Test] + public async Task Teardown_twice_completes(CancellationToken ct) { + var subscription = Create(); + + await subscription.Unsubscribe(_ => { }, ct); + await subscription.Unsubscribe(_ => { }, ct); + } + + static GooglePubSubSubscription Create() + => new("test-project", "test-topic", "test-subscription", new ConsumePipe().AddDefaultConsumer(new NoOpHandler())); + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } +} diff --git a/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/SubscriptionRestartTests.cs b/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/SubscriptionRestartTests.cs new file mode 100644 index 000000000..66d51c7a2 --- /dev/null +++ b/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/SubscriptionRestartTests.cs @@ -0,0 +1,25 @@ +using Eventuous.KurrentDB.Subscriptions; +using Eventuous.Tests.KurrentDB.Subscriptions.Fixtures; +using Eventuous.Tests.Subscriptions.Base; +using Testcontainers.KurrentDb; + +namespace Eventuous.Tests.KurrentDB.Subscriptions; + +public class SubscriptionRestart() + : SubscriptionRestartBase( + new CatchUpSubscriptionFixture( + _ => { }, + new("$all"), + false + ) + ) { + [Test] + public async Task Esdb_ShouldTolerateRepeatedUnsubscribe() { + await ShouldTolerateRepeatedUnsubscribe(); + } + + [Test] + public async Task Esdb_ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { + await ShouldConsumeAfterResubscribe(cancellationToken); + } +} diff --git a/src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/SubscriptionRestartTests.cs b/src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/SubscriptionRestartTests.cs new file mode 100644 index 000000000..c12490206 --- /dev/null +++ b/src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/SubscriptionRestartTests.cs @@ -0,0 +1,25 @@ +using Eventuous.Postgresql; +using Eventuous.Postgresql.Subscriptions; +using Eventuous.Tests.Subscriptions.Base; +using Testcontainers.PostgreSql; + +namespace Eventuous.Tests.Postgres.Subscriptions; + +[NotInParallel] +public class SubscriptionRestart() + : SubscriptionRestartBase( + new SubscriptionFixture( + _ => { }, + false + ) + ) { + [Test] + public async Task Postgres_ShouldTolerateRepeatedUnsubscribe() { + await ShouldTolerateRepeatedUnsubscribe(); + } + + [Test] + public async Task Postgres_ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { + await ShouldConsumeAfterResubscribe(cancellationToken); + } +} diff --git a/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs b/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs new file mode 100644 index 000000000..1981b40ef --- /dev/null +++ b/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs @@ -0,0 +1,94 @@ +using Eventuous.Redis; +using Eventuous.Redis.Subscriptions; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Shouldly; +using StackExchange.Redis; + +namespace Eventuous.Tests.Redis.Subscriptions; + +/// +/// The polling loop is the whole subscription: nothing else reads Redis. These tests need no server — +/// ReadEvents is the seam, and a failure there is the same shape as a failure from the driver. +/// +public class PollFailureTests { + /// + /// StackExchange.Redis reconnects underneath us, so a dropped connection should cost at most a poll. + /// That only helps if something polls again. + /// + [Test] + public async Task Poll_failure_is_followed_by_another_poll(CancellationToken ct) { + var subscription = new FailingSubscription(failuresBeforeSuccess: 1); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + var polledAgain = await WaitUntil(() => subscription.Polls > 1, TimeSpan.FromSeconds(10)); + + await subscription.Unsubscribe(_ => { }, ct); + + polledAgain.ShouldBeTrue($"the subscription should keep polling after a failure, it polled {subscription.Polls} time(s)"); + } + + /// + /// A subscription that has stopped consuming has to say so, or the host reports it healthy forever. + /// + [Test] + public async Task Poll_failure_is_reported_as_a_drop(CancellationToken ct) { + var subscription = new FailingSubscription(failuresBeforeSuccess: 1); + + DropReason? reason = null; + await subscription.Subscribe(_ => { }, (_, r, _) => reason = r, ct); + + var reported = await WaitUntil(() => reason != null, TimeSpan.FromSeconds(10)); + + await subscription.Unsubscribe(_ => { }, ct); + + reported.ShouldBeTrue("a failed poll should be reported through the dropped callback, so health checks see it"); + } + + static async Task WaitUntil(Func condition, TimeSpan timeout) { + var deadline = DateTime.UtcNow + timeout; + + while (DateTime.UtcNow < deadline) { + if (condition()) return true; + + await Task.Delay(20); + } + + return condition(); + } + + record TestOptions : RedisSubscriptionBaseOptions; + + /// + /// Fails its first failuresBeforeSuccess polls the way the driver would, then returns nothing. + /// The database is never touched, so no server is involved. + /// + sealed class FailingSubscription(int failuresBeforeSuccess) + : RedisSubscriptionBase( + () => null!, + new() { SubscriptionId = "redis-poll-failure", MaxPageSize = 10 }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new NoOpHandler()), + SubscriptionKind.All, + null + ) { + int _polls; + + public int Polls => Volatile.Read(ref _polls); + + protected override Task ReadEvents(IDatabase database, long position) { + var poll = Interlocked.Increment(ref _polls); + + if (poll <= failuresBeforeSuccess) throw new RedisConnectionException(ConnectionFailureType.SocketFailure, "Simulated connection failure"); + + return Task.FromResult(Array.Empty()); + } + } + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } +} diff --git a/src/SqlServer/test/Eventuous.Tests.SqlServer/Subscriptions/SubscriptionRestartTests.cs b/src/SqlServer/test/Eventuous.Tests.SqlServer/Subscriptions/SubscriptionRestartTests.cs new file mode 100644 index 000000000..ddc2db722 --- /dev/null +++ b/src/SqlServer/test/Eventuous.Tests.SqlServer/Subscriptions/SubscriptionRestartTests.cs @@ -0,0 +1,24 @@ +using Eventuous.SqlServer.Subscriptions; +using Eventuous.Tests.Subscriptions.Base; +using Testcontainers.MsSql; + +namespace Eventuous.Tests.SqlServer.Subscriptions; + +[NotInParallel] +public class SubscriptionRestart() + : SubscriptionRestartBase( + new SubscriptionFixture( + _ => { }, + false + ) + ) { + [Test] + public async Task SqlServer_ShouldTolerateRepeatedUnsubscribe() { + await ShouldTolerateRepeatedUnsubscribe(); + } + + [Test] + public async Task SqlServer_ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { + await ShouldConsumeAfterResubscribe(cancellationToken); + } +} From 6aa1cac1ee10a09f06ecbd22c20f11caf385d2ce Mon Sep 17 00:00:00 2001 From: Pavel Borisov Date: Fri, 14 Aug 2026 03:49:35 +0200 Subject: [PATCH 2/2] refactor(subscriptions): one supervisor loop per subscription MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subscribe returns while the work carries on, so a transport that is naturally a loop had to spawn one, and that loop's failure could not propagate — it went sideways through Dropped(), callable from any thread at any time. The state machine, the gate, the run generation and the drop cycle all existed to collapse those writers back into one. Moving the loop's lifetime from the drop to the subscription removes the need for them. A transport now says only how to connect. Whatever it acquires is registered on the run as it is acquired and released in reverse when the run stops, so a failure or an acknowledgement arriving late names the run that produced it instead of reaching whichever run is current. Pumps belong to transports, which report their own death. Teardown is one policy: a graceful attempt on TeardownTimeout, then whatever is left is started and abandoned rather than skipped; Unsubscribe's token bounds only the caller's wait, never the stopping. Also fixes: a supervisor dying after connecting reported no drop, leaving health green; handlers cancelled by teardown were acknowledged, advancing the checkpoint past everything in flight; RabbitMQ delivered contexts with a default token, so the stopping guard never matched, and a nack on a closed channel killed a filter reader for good; $all disposed its subscription twice and released without honouring the teardown budget; a faulted channel reader skipped the final forced checkpoint commit; test fixtures abandoned their containers. Deletes SubscriptionLifecycle, TaskRunner, CheckpointRun, ChannelFullException, Dropped, Resubscribe, Stopping, Generation, ResetSequence, IsDropped, MonitorSubscriberTask and DropReason.Stopped. Co-Authored-By: Claude Opus 5 (1M context) --- Eventuous.slnx | 5 + ...26-08-14-subscription-supervisor-design.md | 205 ++++++ .../Subscriptions/ServiceBusSubscription.cs | 114 ++-- .../SendAndReceive.cs | 4 + .../src/Eventuous.Shared/Tools/TaskRunner.cs | 49 -- .../Channels/ChannelExtensions.cs | 29 +- .../Channels/ChannelFullException.cs | 6 - .../Channels/ChannelWorkerBase.cs | 48 +- .../Channels/ChannelWorkers.cs | 4 +- .../Checkpoints/CheckpointCommitHandler.cs | 4 +- .../src/Eventuous.Subscriptions/DropReason.cs | 9 +- .../EventSubscription.cs | 312 ++++++--- .../EventSubscriptionWithCheckpoint.cs | 140 ++-- .../Filters/AsyncHandlingFilter.cs | 31 +- .../IMessageSubscription.cs | 8 + .../Logging/SubscriptionLogging.cs | 56 +- .../SubscriptionOptions.cs | 30 + .../SubscriptionRun.cs | 135 ++++ .../Fixtures/StoreFixtureBase.cs | 40 +- .../Fixtures/SubscriptionFixtureBase.cs | 20 +- .../Fixtures/TestEventHandler.cs | 14 +- .../SubscriptionRestartBase.cs | 13 +- .../AssemblyInfo.cs | 7 + .../CancelledMessageTests.cs | 218 +++++++ .../CapturingLoggerFactory.cs | 47 ++ ...heckpointCommitHandlerBackpressureTests.cs | 16 +- .../CheckpointCommitHandlerLifecycleTests.cs | 149 +++++ .../CompositionHandlerTests.cs | 4 +- .../RegistrationTests.cs | 4 +- .../ResubscribeConcurrencyTests.cs | 555 +++++++++++----- .../ResubscribeOnHandlerFailureTests.cs | 146 +---- .../SubscriptionRunTests.cs | 179 ++++++ .../SubscriptionShutdownTests.cs | 157 ++--- .../SupervisorSettingsTests.cs | 82 +++ .../SupervisorTests.cs | 598 ++++++++++++++++++ .../Transitions.cs | 27 + .../TransportPump.cs | 31 + .../TransportTeardownTests.cs | 24 +- .../Eventuous.Tests.Subscriptions/Wait.cs | 25 + .../RegistrationTests.cs | 4 +- .../CloudRunPubSubSubscription.cs | 15 +- .../Subscriptions/GooglePubSubSubscription.cs | 74 ++- .../PubSubTests.cs | 22 +- .../TeardownTests.cs | 41 -- .../Subscriptions/KafkaBasicSubscription.cs | 5 +- .../Subscriptions/AllStreamSubscription.cs | 198 +++--- .../KurrentDBCatchUpSubscriptionBase.cs | 19 - .../Subscriptions/KurrentDBMappings.cs | 2 +- .../PersistentSubscriptionBase.cs | 42 +- .../Subscriptions/StreamSubscription.cs | 20 +- .../Fixtures/PersistentSubscriptionFixture.cs | 16 +- .../Subscriptions/RabbitMqSubscription.cs | 110 ++-- .../Subscriptions/RedisSubscriptionBase.cs | 66 +- .../Subscriptions/PollFailureTests.cs | 4 +- .../Subscriptions/SqlSubscriptionBase.cs | 84 +-- .../ExcludeOnMacOs.cs | 7 + .../Subscriptions/PollFailureTests.cs | 153 +++++ .../Subscriptions/SubscriptionRestartTests.cs | 104 +++ 58 files changed, 3439 insertions(+), 1092 deletions(-) create mode 100644 docs/plans/2026-08-14-subscription-supervisor-design.md delete mode 100644 src/Core/src/Eventuous.Shared/Tools/TaskRunner.cs delete mode 100644 src/Core/src/Eventuous.Subscriptions/Channels/ChannelFullException.cs create mode 100644 src/Core/src/Eventuous.Subscriptions/SubscriptionRun.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/AssemblyInfo.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/CancelledMessageTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/CapturingLoggerFactory.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerLifecycleTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionRunTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/SupervisorSettingsTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/SupervisorTests.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/Transitions.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/TransportPump.cs create mode 100644 src/Core/test/Eventuous.Tests.Subscriptions/Wait.cs delete mode 100644 src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs create mode 100644 src/SqlServer/test/Eventuous.Tests.SqlServer/ExcludeOnMacOs.cs create mode 100644 src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/PollFailureTests.cs create mode 100644 src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/SubscriptionRestartTests.cs diff --git a/Eventuous.slnx b/Eventuous.slnx index 1af57601b..eaf52cf36 100644 --- a/Eventuous.slnx +++ b/Eventuous.slnx @@ -7,6 +7,11 @@ + + + + + diff --git a/docs/plans/2026-08-14-subscription-supervisor-design.md b/docs/plans/2026-08-14-subscription-supervisor-design.md new file mode 100644 index 000000000..4e6f8d9c4 --- /dev/null +++ b/docs/plans/2026-08-14-subscription-supervisor-design.md @@ -0,0 +1,205 @@ +# Subscription Supervisor Design + +## Goal + +One universal shape for every subscription: a transport says how to connect — registering whatever it needs +torn down as it acquires it — and `EventSubscription` owns a single sequential loop that decides when to +retry. A reader should be able to verify the lifecycle by reading `EventSubscription.Subscribe` and +`RunSubscriptionLoop` alone, without holding a state machine, a generation counter and a drop cycle in their +head at once. + +`Subscribe(OnSubscribed, OnDropped, CancellationToken)` returns once the subscription is up, and everything +after that runs on one background loop per subscription — not one per drop. A transport's own failure signal +(a dropped connection, a dead poll loop) can fire from any thread, at any time, and there can be several +in flight if a connection flaps; collapsing them onto a single loop means there is exactly one place that +decides "retry" or "give up", instead of N concurrent retry attempts racing each other. + +## The contract + +```csharp +/// Connects the transport. Returns once up, throws if it can't come up. Called once per run and must be +/// repeatable on the same instance — reassign fields rather than assume them unset. +protected abstract ValueTask Connect(SubscriptionRun run); +``` + +`Connect` returning is readiness — `Subscribe` waits for the first `Connect` to return (or throw) before it +returns to its own caller. `Connect` throwing on the *first* attempt propagates out of `Subscribe`, unretried +— a caller who asks for a subscription that cannot come up should see the exception, not a silent retry loop. +A later `Connect` failure (on resubscribe) is an ordinary drop and is retried. + +There is no `Disconnect` method to override. Teardown is registration-based: a transport calls +`run.OnDisconnect(...)` for each handle as it acquires it, and those callbacks run when the run ends — +in reverse registration order (see [Teardown](#teardown)). This means a `Connect` that fails part-way still +releases whatever it already took, and a transport never has to remember a matching "undo" method — it just +registers the undo next to the acquire. + +A transport with its own loop (polling, a long-lived read) starts it on its own task and registers a join: + +```csharp +// SqlSubscriptionBase.Connect +var pumping = Task.Run(async () => { + try { + await Poll(run, start, run.Token).NoContext(); + } catch (Exception) when (run.Token.IsCancellationRequested) { + // This run's own token asked for it: graceful, not a drop. + } catch (Exception e) { + run.Fail(DropReason.ServerError, e); + } +}, CancellationToken.None); + +// No handle of its own to release: registered purely to join the loop before the next Connect starts. +run.OnDisconnect(_ => new(pumping)); +``` + +A callback-driven transport just connects, wires the drop callback to `run.Fail`, and registers the handle it +was given: + +```csharp +// StreamSubscription.Connect +var subscription = await Client.SubscribeToStreamAsync( + Options.StreamName, fromStream, (_, @event, ct) => HandleEvent(@event, ct), + Options.ResolveLinkTos, HandleDrop, Options.Credentials, run.Token) + .NoContext(); + +run.OnDisconnect(_ => { subscription.Dispose(); return default; }); + +void HandleDrop(global::KurrentDB.Client.StreamSubscription _, SubscriptionDroppedReason reason, Exception? ex) + => run.Fail(KurrentDBMappings.AsDropReason(reason), ex); +``` + +Both shapes end the same way: whatever ends the run — a reported drop, a dying loop, or the run's own token +being cancelled by shutdown — is observed once, by the supervisor, through `run.Ended`. + +## The run + +`SubscriptionRun` is the identity of one connect attempt. Every signal that can arrive late — a failure +report from a dead connection, an ack for a message dispatched two runs ago, a sequence number drawn by a +loop that is still winding down — is addressed to the run it was created under, not to "whichever run is +current". A run that has already ended is simply a run nobody is listening to any more; there is no separate +generation counter or gate to check, because the fencing is object identity. + +```csharp +public class SubscriptionRun { + public CancellationToken Token { get; } // cancelled when this run ends; the token a transport gives its I/O + public Task Ended => _ended.Task; // completed when this run is over, for any reason + + public ulong NextSequence() => Interlocked.Increment(ref _sequence) - 1; + + public void Fail(DropReason reason, Exception? exception) { + if (Interlocked.CompareExchange(ref _failure, new(reason, exception), null) is not null) return; + _ended.TrySetResult(); + } + + public void OnDisconnect(Func release) { ... } +} +``` + +`Fail` is total: a compare-exchange and a `TrySetResult`, safe from any thread, safe on a run retired long +ago, safe inside a catch or a finally. It does not cancel, log, or invoke `OnDropped` — the supervisor reports +the drop, once, on its own stack, after observing `Ended`. That is what makes one flapping connection produce +one log line and one `OnDropped` call no matter how many messages or callbacks call `Fail` concurrently: +first writer wins, everyone else's call is a no-op. + +`Ended` covers both a reported failure and shutdown through the same arm — the run's `CancellationTokenSource` +is registered to complete `_ended` too, so a cancelled lifetime resolves `Ended` exactly as `Fail` does. The +supervisor waits on one thing regardless of why the run is over. + +`SubscriptionRun` is derivable (`protected internal` constructor) so a subscription can hang per-attempt state +off the run — `EventSubscriptionWithCheckpoint` does this for its commit handler (see +[Checkpoints](#checkpoints)) — instead of a field the next run would silently overwrite. + +## Teardown + +`SubscriptionRun.Stop(graceful, log)` is called once per run, from the supervisor's loop, whether the run is +being replaced or is the last one: + +```csharp +internal async ValueTask Stop(CancellationToken graceful, LogContext log) { + try { await _cts.CancelAsync().NoContext(); } catch (Exception e) { log.SubscriptionDisconnectFailed(e); } + + try { await Disconnect(graceful, log).NoContext(); } + finally { _cts.Dispose(); } +} +``` + +The token is cancelled first, so production stops before anything else happens. Then every handle registered +through `OnDisconnect` releases — in **reverse registration order**: the last thing acquired is released +first, the first thing acquired is released last. This is why +`EventSubscriptionWithCheckpoint.CreateRun` registers the commit handler's disposal *before* `Connect` runs: +being registered first means it releases last, after every transport handle the subclass goes on to register. +Acknowledgements still landing while the transport is being torn down need the commit handler alive to +receive them; releasing it early would drop those commits on the floor. + +**Teardown always runs to completion.** Every release is awaited; none is ever abandoned, skipped or left +running behind the supervisor's back. `Stop` returns only once the last one has finished. + +`SubscriptionOptions.TeardownTimeout` — five seconds by default, one shared budget for the whole teardown — +is therefore **advisory, not a deadline**. The token it cancels means *"this teardown can no longer be +graceful: drop what is optional and finish quickly"*, not *"stop"*. A release that can honour that speeds up; +a release with essential work ignores it and runs to completion, and teardown waits. + +```csharp +for (var i = releases.Count - 1; i >= 0; i--) { + try { await releases[i](graceful).NoContext(); } + catch (Exception e) { log.SubscriptionDisconnectFailed(e); } +} +``` + +The alternative — bounding the wait and carrying on — was tried and removed. Because the commit handler +releases last, it was always the first thing cut off, so an overrunning transport join left the final +checkpoint flush running detached while the supervisor moved on to the retry delay and the next run's +`GetCheckpoint`. Two writers for one checkpoint key, and no store guards against a stale one landing second, +so the checkpoint could move *backwards*. Waiting is what makes the flush-then-read ordering a fact rather +than a hope. + +Each release is guarded individually, so one that throws is logged and the ones behind it still run. + +## Checkpoints + +`EventSubscriptionWithCheckpoint.CreateRun` is `sealed`, so every run reaching that class carries its own +`CheckpointCommitHandler`: + +```csharp +sealed class CheckpointedRun(CancellationToken lifetime, CheckpointCommitHandler checkpoint) : SubscriptionRun(lifetime) { + internal CheckpointCommitHandler Checkpoint { get; } = checkpoint; +} + +protected sealed override SubscriptionRun CreateRun(CancellationToken lifetime) { + var run = new CheckpointedRun(lifetime, new(Options.SubscriptionId, CheckpointStore, ...)); + + // Registered first, before Connect, so it's the first OnDisconnect registration — and release order + // reverses registration order, so it releases LAST, after every transport handle. + run.OnDisconnect(_ => run.Checkpoint.DisposeAsync()); + + return run; +} +``` + +Because the cast in `Ack`/`Nack` is guaranteed by construction (every run handed to this class is a +`CheckpointedRun`), there is no null check or lookup at the commit site — the run passed into +`HandleInternal` is the one whose handler will receive the ack. + +`CheckpointCommitHandler.Commit` returns `ValueTask`, not `ValueTask`: + +```csharp +public ValueTask Commit(CommitPosition position, CancellationToken cancellationToken) +``` + +`false` means the handler had already stopped (its batching worker was closed) when the commit was attempted +— the position was never accepted into the pipeline. The caller must treat that as "not committed": in +`Ack`, a `false` result logs `MessageFromPreviousRunIgnored` and returns without acknowledging the message +further. This is the mechanism that keeps a late ack — one that completes after its run has already ended and +its handler disposed — from silently advancing a checkpoint it has no business advancing. + +## Options + +```csharp +public TimeSpan RetryDelay { get; set; } = TimeSpan.FromSeconds(2); // SubscriptionOptions +public TimeSpan TeardownTimeout { get; set; } = TimeSpan.FromSeconds(5); // SubscriptionOptions +``` + +`RetryDelay` is how long the supervisor waits between a drop and the next `Connect`. `TeardownTimeout` is the +single budget described above, spent once per run in `Stop`. Both are on `SubscriptionOptions` rather than +overridable members on the transport base class — they are operating numbers an operator should be able to +reach through configuration, not implementation details a transport author overrides in code. Both fall back +to their defaults, with a logged warning, if set to an unusable negative value. diff --git a/src/Azure/src/Eventuous.Azure.ServiceBus/Subscriptions/ServiceBusSubscription.cs b/src/Azure/src/Eventuous.Azure.ServiceBus/Subscriptions/ServiceBusSubscription.cs index daa30259a..59f50284c 100644 --- a/src/Azure/src/Eventuous.Azure.ServiceBus/Subscriptions/ServiceBusSubscription.cs +++ b/src/Azure/src/Eventuous.Azure.ServiceBus/Subscriptions/ServiceBusSubscription.cs @@ -28,21 +28,39 @@ public ServiceBusSubscription(ServiceBusClient client, ServiceBusSubscriptionOpt _defaultErrorHandler = Options.ErrorHandler ?? DefaultErrorHandler; _processorStrategy = Options.SessionProcessorOptions is not null - ? new SessionProcessorStrategy(client, Options, HandleSessionMessage, _defaultErrorHandler) - : new StandardProcessorStrategy(client, Options, HandleMessage, _defaultErrorHandler); + ? new SessionProcessorStrategy(client, Options, HandleSessionMessage, HandleError) + : new StandardProcessorStrategy(client, Options, HandleMessage, HandleError); } /// - /// Subscribes to the Service Bus queue or topic. + /// Runs the configured error handler, then ends the run if the processor has stopped receiving for good. /// - /// - /// + /// + /// The SDK's receive loop exits on a dead connection, raises this once and never restarts — untranslated, + /// the supervisor parks forever. In a finally so a throwing user handler can't suppress the recovery. + /// + async Task HandleError(SubscriptionRun run, ProcessErrorEventArgs arg) { + try { + await _defaultErrorHandler(arg).NoContext(); + } finally { + if (arg is { ErrorSource: ServiceBusErrorSource.Receive, Exception: ObjectDisposedException }) { + run.Fail(DropReason.ServerError, arg.Exception); + } + } + } + + /// + /// Starts processing the Service Bus queue or topic. The processor is recreated on every call, so its + /// message handler is wired up here, closing over this run rather than looking one up later. + /// + /// /// - protected override ValueTask Subscribe(CancellationToken cancellationToken) - => _processorStrategy.Start(cancellationToken); + protected override ValueTask Connect(SubscriptionRun run) + => _processorStrategy.Start(run); - Task HandleMessage(ProcessMessageEventArgs arg) + Task HandleMessage(SubscriptionRun run, ProcessMessageEventArgs arg) => ProcessMessageAsync( + run, arg.Message, msg => arg.CompleteMessageAsync(msg, arg.CancellationToken), msg => arg.AbandonMessageAsync(msg, null, arg.CancellationToken), @@ -52,8 +70,9 @@ Task HandleMessage(ProcessMessageEventArgs arg) arg.CancellationToken ); - Task HandleSessionMessage(ProcessSessionMessageEventArgs arg) + Task HandleSessionMessage(SubscriptionRun run, ProcessSessionMessageEventArgs arg) => ProcessMessageAsync( + run, arg.Message, msg => arg.CompleteMessageAsync(msg, arg.CancellationToken), msg => arg.AbandonMessageAsync(msg, null, arg.CancellationToken), @@ -64,6 +83,7 @@ Task HandleSessionMessage(ProcessSessionMessageEventArgs arg) ); async Task ProcessMessageAsync( + SubscriptionRun run, ServiceBusReceivedMessage msg, Func completeMessage, Func abandonMessage, @@ -100,7 +120,7 @@ CancellationToken ct 0, 0, 0, - Sequence++, + run.NextSequence(), msg.EnqueuedTime.UtcDateTime, evt, AsMeta(applicationProperties), @@ -147,61 +167,61 @@ Task DefaultErrorHandler(ProcessErrorEventArgs arg) { return Task.CompletedTask; } - /// - /// Unsubscribes from the Service Bus queue or topic and stops processing messages. - /// - /// - /// - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => _processorStrategy.Stop(cancellationToken); - interface IServiceBusProcessorStrategy { - ValueTask Start(CancellationToken cancellationToken); - ValueTask Stop(CancellationToken cancellationToken); + ValueTask Start(SubscriptionRun run); } sealed class StandardProcessorStrategy( - ServiceBusClient client, - ServiceBusSubscriptionOptions options, - Func handleMessage, - Func handleError + ServiceBusClient client, + ServiceBusSubscriptionOptions options, + Func handleMessage, + Func handleError ) : IServiceBusProcessorStrategy { - ServiceBusProcessor? _processor; + public ValueTask Start(SubscriptionRun run) { + var processor = options.QueueOrTopic.MakeProcessor(client, options); + processor.ProcessMessageAsync += arg => handleMessage(run, arg); + processor.ProcessErrorAsync += arg => handleError(run, arg); - public ValueTask Start(CancellationToken cancellationToken) { - _processor = options.QueueOrTopic.MakeProcessor(client, options); - _processor.ProcessMessageAsync += handleMessage; - _processor.ProcessErrorAsync += handleError; + run.OnDisconnect(ct => Stop(processor, ct)); - return new(_processor.StartProcessingAsync(cancellationToken)); + return new(processor.StartProcessingAsync(run.Token)); } - public ValueTask Stop(CancellationToken cancellationToken) - => _processor is not null - ? new(_processor.StopProcessingAsync(cancellationToken)) - : ValueTask.CompletedTask; + // Disposed in a finally because it releases the AMQP link, even if StopProcessingAsync throws. + static async ValueTask Stop(ServiceBusProcessor processor, CancellationToken cancellationToken) { + try { + await processor.StopProcessingAsync(cancellationToken).NoContext(); + } finally { + await processor.DisposeAsync().NoContext(); + } + } } sealed class SessionProcessorStrategy( - ServiceBusClient client, - ServiceBusSubscriptionOptions options, - Func handleSessionMessage, - Func handleError + ServiceBusClient client, + ServiceBusSubscriptionOptions options, + Func handleSessionMessage, + Func handleError ) : IServiceBusProcessorStrategy { - ServiceBusSessionProcessor? _sessionProcessor; + public ValueTask Start(SubscriptionRun run) { + var sessionProcessor = options.QueueOrTopic.MakeSessionProcessor(client, options); + sessionProcessor.ProcessMessageAsync += arg => handleSessionMessage(run, arg); + sessionProcessor.ProcessErrorAsync += arg => handleError(run, arg); - public ValueTask Start(CancellationToken cancellationToken) { - _sessionProcessor = options.QueueOrTopic.MakeSessionProcessor(client, options); - _sessionProcessor.ProcessMessageAsync += handleSessionMessage; - _sessionProcessor.ProcessErrorAsync += handleError; + run.OnDisconnect(ct => Stop(sessionProcessor, ct)); - return new(_sessionProcessor.StartProcessingAsync(cancellationToken)); + return new(sessionProcessor.StartProcessingAsync(run.Token)); } - public ValueTask Stop(CancellationToken cancellationToken) - => _sessionProcessor is not null - ? new(_sessionProcessor.StopProcessingAsync(cancellationToken)) - : ValueTask.CompletedTask; + // Same as the standard processor: dispose in finally, left unbounded since teardown bounds it centrally. + static async ValueTask Stop(ServiceBusSessionProcessor sessionProcessor, CancellationToken cancellationToken) { + try { + await sessionProcessor.StopProcessingAsync(cancellationToken).NoContext(); + } finally { + await sessionProcessor.DisposeAsync().NoContext(); + } + } } } diff --git a/src/Azure/test/Eventuous.Tests.Azure.ServiceBus/SendAndReceive.cs b/src/Azure/test/Eventuous.Tests.Azure.ServiceBus/SendAndReceive.cs index 96e8bd378..a49931abf 100644 --- a/src/Azure/test/Eventuous.Tests.Azure.ServiceBus/SendAndReceive.cs +++ b/src/Azure/test/Eventuous.Tests.Azure.ServiceBus/SendAndReceive.cs @@ -1,9 +1,13 @@ using Eventuous.Azure.ServiceBus.Producers; using Eventuous.Azure.ServiceBus.Subscriptions; using Eventuous.Producers; +using TUnit.Core.Enums; namespace Eventuous.Tests.Azure.ServiceBus; +// The Service Bus emulator brings its own SQL Server along, so it cannot start on macOS for the same reason +// the SQL Server suite cannot. +[ExcludeOn(OS.MacOs)] [NotInParallel] [TopicAndQueueSource] public class SendAndReceive { diff --git a/src/Core/src/Eventuous.Shared/Tools/TaskRunner.cs b/src/Core/src/Eventuous.Shared/Tools/TaskRunner.cs deleted file mode 100644 index 8627cd5b8..000000000 --- a/src/Core/src/Eventuous.Shared/Tools/TaskRunner.cs +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright (C) Eventuous HQ OÜ. All rights reserved -// Licensed under the Apache License, Version 2.0. - -namespace Eventuous.Tools; - -public sealed class TaskRunner(Func taskFactory) : IDisposable { - readonly CancellationTokenSource _stopSource = new(); - - Task? _runner; - - public TaskRunner Start() { - _runner = Task.Run(Run); - - return this; - - async Task Run() => await taskFactory(_stopSource.Token).NoThrow(); - } - - /// - /// Stops the running task, considering the cancellation token provided as an argument. - /// The code of this function closely resembles BackgroundService.StopAsync function. - /// - /// - public async ValueTask Stop(CancellationToken cancellationToken) { - if (_runner == null) return; - - try { - await _stopSource.CancelAsync(); - } finally { - var state = new TaskCompletionSource(); - var registration = cancellationToken.Register((s => (((TaskCompletionSource)s!)).SetCanceled(cancellationToken)), state); - - try { - await Task.WhenAny(_runner, state.Task).NoContext(); - } finally { - await registration.DisposeAsync().NoContext(); - } - - // ReSharper disable once RedundantAssignment - registration = new(); - _runner = null; - } - } - - public void Dispose() { - _stopSource.Dispose(); - _runner?.Dispose(); - } -} diff --git a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelExtensions.cs b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelExtensions.cs index 55f159fe4..da029678f 100644 --- a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelExtensions.cs +++ b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelExtensions.cs @@ -34,12 +34,6 @@ CancellationToken cancellationToken } } - public ValueTask Write(T element, bool throwOnFull, CancellationToken cancellationToken) { - return throwOnFull ? WriteOrThrow() : channel.Writer.WriteAsync(element, cancellationToken); - - ValueTask WriteOrThrow() => !channel.Writer.TryWrite(element) ? throw new ChannelFullException() : default; - } - public async ValueTask Stop( CancellationTokenSource cts, Task[] readers, @@ -49,15 +43,22 @@ public async ValueTask Stop( var incompleteReaders = readers.Where(r => !r.IsCompleted).ToArray(); - if (readers.Length > 0) { - cts.CancelAfter(TimeSpan.FromSeconds(10)); - await Task.WhenAll(incompleteReaders).NoContext(); + try { + // Only incomplete readers need a deadline; arming one for readers already done would cancel + // a drain that's already finished. + if (incompleteReaders.Length > 0) { + cts.CancelAfter(TimeSpan.FromSeconds(10)); + await Task.WhenAll(incompleteReaders).NoContext(); + } + } finally { + // In a finally: finalize is the only forced checkpoint commit, and matters most when the + // drain above times out (a batch reader rethrows that cancellation). The reader's own failure + // still propagates afterwards, so a broken shutdown isn't traded away for the flush. + if (finalize != null) { + using var ts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + await finalize(ts.Token).NoContext(); + } } - - if (finalize == null) return; - - using var ts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); - await finalize(ts.Token).NoContext(); } } diff --git a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelFullException.cs b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelFullException.cs deleted file mode 100644 index 147bda5ac..000000000 --- a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelFullException.cs +++ /dev/null @@ -1,6 +0,0 @@ -// Copyright (C) Eventuous HQ OÜ. All rights reserved -// Licensed under the Apache License, Version 2.0. - -namespace Eventuous.Subscriptions.Channels; - -public class ChannelFullException() : Exception("Channel worker unable to write to the channel because it's full"); \ No newline at end of file diff --git a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkerBase.cs b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkerBase.cs index 7860d0ff4..895e16464 100644 --- a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkerBase.cs +++ b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkerBase.cs @@ -14,26 +14,38 @@ abstract class ChannelWorkerBase : IAsyncDisposable { public Func? OnDispose { get; set; } - public ValueTask Write(T element, CancellationToken cancellationToken) - => _stopping ? default : _channel.Write(element, _throwOnFull, cancellationToken); - - bool _stopping; + volatile bool _stopping; readonly Channel _channel; - readonly bool _throwOnFull; - protected ChannelWorkerBase(Channel channel, Func processor, int concurrencyLevel, bool throwOnFull = false) { + protected ChannelWorkerBase(Channel channel, Func processor, int concurrencyLevel) { _channel = channel; - _throwOnFull = throwOnFull; _readerTasks = [.. Enumerable.Range(0, concurrencyLevel).Select(_ => Task.Run(() => processor(_cts.Token)))]; } /// - /// Idempotent. The commit handler worker is disposed by both the resubscribe and the shutdown - /// paths, which can run concurrently, so a second call is expected rather than a programming - /// error. It must not re-enter the shutdown: by then the CTS is disposed, and cancelling it - /// again throws out of host shutdown. Every caller awaits - /// the same task, so none of them returns before the final checkpoint flush, and a shutdown that - /// failed is reported to whoever awaits it instead of being left on a task nobody observes. + /// Queues an element and reports whether the worker took it. A stopping worker takes nothing — the + /// caller must not count a refused element as processed. + /// + public async ValueTask Write(T element, CancellationToken cancellationToken) { + if (_stopping) return false; + + try { + await _channel.Writer.WriteAsync(element, cancellationToken).NoContext(); + + return true; + } catch (ChannelClosedException) { + // The flag is set just before the channel completes, so a writer that got past it can still + // find it closed — same event, caught here rather than propagated. + return false; + } + } + + /// + /// Idempotent: a worker can outlive the thing disposing it (the handling filter's worker is released + /// with the pipe, a commit handler's with its run), so a second call is expected, not a bug. Must not + /// re-enter shutdown — cancelling an already-disposed CTS throws + /// out of host shutdown. Every caller awaits the same task, so a failed shutdown is reported to all of + /// them rather than left unobserved. /// public ValueTask DisposeAsync() { if (Interlocked.Exchange(ref _disposing, 1) == 0) _ = StopWorker(); @@ -48,9 +60,9 @@ async Task StopWorker() { await _channel.Stop(_cts, _readerTasks, OnDispose).NoContext(); } finally { - // Release the readers even when the graceful stop above failed: they hold _cts.Token, - // and Stop armed a ten-second timer on it, so both outlive the worker unless cancelled - // here. Cancelling runs their callbacks, which is why this can't be allowed to throw. + // Runs even if the graceful stop failed: readers hold _cts.Token (Stop armed a ten-second + // timer on it) and outlive the worker unless cancelled here. Cancelling runs their + // callbacks, so this can't be allowed to throw. await _cts.CancelAsync().NoThrow(); await Task.WhenAll(_readerTasks).NoThrow(); _cts.Dispose(); @@ -59,8 +71,8 @@ async Task StopWorker() { _disposed.TrySetResult(); } catch (Exception e) { - // Broad on purpose. DisposeAsync hands _disposed.Task to every caller, so completing it is the - // only thing that ever releases them; an exception escaping here would strand all of them. + // Broad on purpose: every caller awaits _disposed.Task, so an escaping exception here would + // strand all of them. _disposed.TrySetException(e); } } diff --git a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkers.cs b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkers.cs index 6fc8a8a62..381a60b4f 100644 --- a/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkers.cs +++ b/src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkers.cs @@ -14,5 +14,5 @@ namespace Eventuous.Subscriptions.Channels; sealed class ConcurrentChannelWorker(Channel channel, ProcessElement process, int concurrencyLevel) : ChannelWorkerBase(channel, token => channel.Read(process, token), concurrencyLevel); -class BatchedChannelWorker(Channel channel, ProcessElement> processor, int maxCount, TimeSpan maxTime, bool throwOnFull = false) - : ChannelWorkerBase(channel, token => channel.ReadBatches(processor, maxCount, maxTime, token), 1, throwOnFull); +class BatchedChannelWorker(Channel channel, ProcessElement> processor, int maxCount, TimeSpan maxTime) + : ChannelWorkerBase(channel, token => channel.ReadBatches(processor, maxCount, maxTime, token), 1); diff --git a/src/Core/src/Eventuous.Subscriptions/Checkpoints/CheckpointCommitHandler.cs b/src/Core/src/Eventuous.Subscriptions/Checkpoints/CheckpointCommitHandler.cs index aef53c1cf..a95e52e5a 100644 --- a/src/Core/src/Eventuous.Subscriptions/Checkpoints/CheckpointCommitHandler.cs +++ b/src/Core/src/Eventuous.Subscriptions/Checkpoints/CheckpointCommitHandler.cs @@ -90,9 +90,9 @@ async ValueTask Process(IReadOnlyList list, CancellationToken ca /// /// Position to commit /// Cancellation token - /// + /// False if the handler stopped without taking the position — don't treat that as a commit. [PublicAPI] - public ValueTask Commit(CommitPosition position, CancellationToken cancellationToken) { + public ValueTask Commit(CommitPosition position, CancellationToken cancellationToken) { // No _positions access here — that read moved to the worker thread's Process (AI-1329). This // runs on the ack caller thread and must never touch the worker-owned, non-thread-safe set. position.LogContext?.PositionReceived(position); diff --git a/src/Core/src/Eventuous.Subscriptions/DropReason.cs b/src/Core/src/Eventuous.Subscriptions/DropReason.cs index 17707da6a..9f04bc19c 100644 --- a/src/Core/src/Eventuous.Subscriptions/DropReason.cs +++ b/src/Core/src/Eventuous.Subscriptions/DropReason.cs @@ -1,7 +1,6 @@ -namespace Eventuous.Subscriptions; +namespace Eventuous.Subscriptions; public enum DropReason { - Stopped, - ServerError, - SubscriptionError -} \ No newline at end of file + ServerError = 1, + SubscriptionError = 2 +} diff --git a/src/Core/src/Eventuous.Subscriptions/EventSubscription.cs b/src/Core/src/Eventuous.Subscriptions/EventSubscription.cs index 75b320ba0..01c9aad94 100644 --- a/src/Core/src/Eventuous.Subscriptions/EventSubscription.cs +++ b/src/Core/src/Eventuous.Subscriptions/EventSubscription.cs @@ -17,21 +17,18 @@ namespace Eventuous.Subscriptions; using Logging; public abstract class EventSubscription : IMessageSubscription, IAsyncDisposable where T : SubscriptionOptions { - [PublicAPI] - public bool IsRunning { get; set; } - - [PublicAPI] - public bool IsDropped { get; set; } - protected internal T Options { get; } - IEventSerializer EventSerializer { get; } - internal ConsumePipe Pipe { get; } - protected ILoggerFactory? LoggerFactory { get; } - protected LogContext Log { get; } - protected CancellationTokenSource Stopping { get; set; } = new(); + IEventSerializer EventSerializer { get; } + internal ConsumePipe Pipe { get; } + protected ILoggerFactory? LoggerFactory { get; } + protected LogContext Log { get; } + + Session? _session; + int _disposed; - protected ulong Sequence; + [PublicAPI] + public bool IsRunning => Volatile.Read(ref _session) is not null; protected EventSubscription( T options, @@ -48,35 +45,173 @@ protected EventSubscription( Log = Logger.CreateContext(options.SubscriptionId, loggerFactory); } - OnSubscribed? _onSubscribed; - OnDropped? _onDropped; - public string SubscriptionId => Options.SubscriptionId; public async ValueTask Subscribe(OnSubscribed onSubscribed, OnDropped onDropped, CancellationToken cancellationToken) { - if (IsRunning) return; + // Otherwise a new run could deliver into a pipe that's already disposed. + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); + + var lifetime = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + var finishedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var settings = SupervisorSettings.From(Options, Log); + var session = new Session(lifetime, finishedTcs, onSubscribed, onDropped, settings); + + // Refused, not silently ignored: serving a second caller would take the run away from the first + // without telling it. + if (Interlocked.CompareExchange(ref _session, session, comparand: null) is not null) { + lifetime.Dispose(); + throw new InvalidOperationException($"Subscription {SubscriptionId} is already running. Unsubscribe before subscribing again."); + } + + // Guarded because CreateRun can throw: a session published with no supervisor to clear it is an + // unstoppable subscription whose DisposeAsync never returns. + SubscriptionRun? run = null; + + try { + run = CreateRun(lifetime.Token); + await Connect(run).NoContext(); + } catch { + if (run is not null) { + using var graceful = GracefulStop(settings); + await run.Stop(graceful.Token, Log).NoContext(); + } - Stopping = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + // Cleared before rethrowing so an immediate retry isn't refused. + Interlocked.CompareExchange(ref _session, null, session); + finishedTcs.TrySetResult(); + lifetime.Dispose(); + + throw; + } - _onSubscribed = onSubscribed; - _onDropped = onDropped; - await Subscribe(Stopping.Token).NoContext(); - IsRunning = true; Log.SubscriptionStarted(); - onSubscribed(Options.SubscriptionId); + ReportConnected(session); + + _ = Task.Run(() => RunSubscriptionLoop(session, run), CancellationToken.None); } public async ValueTask Unsubscribe(OnUnsubscribed onUnsubscribed, CancellationToken cancellationToken) { - IsRunning = false; - await Unsubscribe(cancellationToken).NoContext(); + await StopSession(cancellationToken).NoContext(); + Log.SubscriptionStopped(); - onUnsubscribed(Options.SubscriptionId); - await Finalize(cancellationToken); - Sequence = 0; - Stopping.Dispose(); + onUnsubscribed(SubscriptionId); + } + + /// + /// Cancels the running session, if any, and waits for its supervisor to finish. + /// bounds only this wait — teardown has its own budget. + /// + async ValueTask StopSession(CancellationToken cancellationToken) { + if (Volatile.Read(ref _session) is not { } session) return; + + // Guarded: cancelling runs whatever the transport registered on the token, and its failure + // shouldn't cost the caller its stop report. + try { + await session.Lifetime.CancelAsync().NoContext(); + } catch (ObjectDisposedException) { + // Already disposed means the supervisor already finished; nothing left to cancel. + } catch (Exception e) { + Log.SubscriptionDisconnectFailed(e); + } + + try { + await session.Finished.Task.WaitAsync(cancellationToken).NoContext(); + } catch (OperationCanceledException) { + // Logged, not thrown: this runs on the host's shutdown token from IHostedService.StopAsync, + // and throwing would abort every service queued behind it. + Log.SubscriptionStopTimedOut(); + } + + // Discarded even on timeout, or a teardown that outlived the caller would refuse every later Subscribe. + Interlocked.CompareExchange(ref _session, null, session); + } + + /// + /// The subscription's lifecycle from the first successful connect onward, sequential and single-threaded. + /// arrives already connected, so every failure here is a drop to report and + /// recover from, never a caller still waiting on the first attempt. + /// + async Task RunSubscriptionLoop(Session session, SubscriptionRun run) { + var lifetime = session.Lifetime.Token; + var settings = session.Settings; + + try { + while (true) { + // Fires on a reported drop, a dying pump, or shutdown cancelling the run's token. + await run.Ended.NoContext(); + + // Skipped during shutdown: a transport whose client reacts to token cancellation would + // double-report otherwise; Fail is first-wins, so this only fires for a genuine failure. + if (run.Failure is { } failure && !lifetime.IsCancellationRequested) ReportConnectionDropped(session, failure); + + // Same teardown whether replaced or final — a stop is a resubscribe that doesn't come back. + using (var graceful = GracefulStop(settings)) { + await run.Stop(graceful.Token, Log).NoContext(); + } + + if (lifetime.IsCancellationRequested) break; + + Log.SubscriptionWillResubscribe(settings.RetryDelay); + + try { + await Task.Delay(settings.RetryDelay, lifetime).NoContext(); + } catch (OperationCanceledException) when (lifetime.IsCancellationRequested) { + break; + } + + Log.SubscriptionResubscribing(); + + // Outside the try: failing here is the supervisor's fault, not a transport drop, and there's no + // live run to carry it — blaming the one just released would re-report its stale failure. + run = CreateRun(lifetime); + + try { + await Connect(run).NoContext(); + Log.SubscriptionResubscribed(); + ReportConnected(session); + } catch (Exception e) { + // Handled by the top of the loop, same path as a mid-run drop. + run.Fail(DropReason.ServerError, e); + } + } + } catch (OperationCanceledException) when (lifetime.IsCancellationRequested) { + // Shutdown landed mid-run or mid-delay. + } catch (Exception e) { + Log.SubscriptionSuperviseFailed(e); + + // Only chance to report: health is wired to these two callbacks, so dying silently here would + // leave health green. SubscriptionError since this is the supervisor's failure, not the transport's. + if (!lifetime.IsCancellationRequested) ReportConnectionDropped(session, new(DropReason.SubscriptionError, e)); + } finally { + Interlocked.CompareExchange(ref _session, null, session); + session.Finished.TrySetResult(); + + // Unregisters the session from the caller's long-lived token; otherwise each subscribe cycle + // leaks a registration the GC can't reclaim. An Unsubscribe that races this finds the source + // already disposed, which is the answer it wants. + session.Lifetime.Dispose(); + } + } + + void ReportConnected(Session session) { + try { session.OnSubscribed(SubscriptionId); } catch (Exception e) { Log.SubscriptionCallbackFailed(e); } + } + + void ReportConnectionDropped(Session session, Failure failure) { + Log.SubscriptionDropped(failure.Reason, failure.Exception); + + try { session.OnDropped(SubscriptionId, failure.Reason, failure.Exception); } catch (Exception e) { Log.SubscriptionCallbackFailed(e); } } - protected virtual ValueTask Finalize(CancellationToken cancellationToken) => default; + /// + /// The budget a run gets to stop itself in, on the one path that has no caller waiting to supply one. + /// + CancellationTokenSource GracefulStop(SupervisorSettings settings) => new(settings.TeardownTimeout); + + /// + /// Creates the run for one attempt. Override to attach attempt-scoped state a base run field can't hold. + /// + protected virtual SubscriptionRun CreateRun(CancellationToken lifetime) => new(lifetime); // ReSharper disable once CognitiveComplexity // ReSharper disable once CyclomaticComplexity @@ -129,7 +264,7 @@ protected async ValueTask Handler(IMessageConsumeContext context) { } if (context.WasIgnored() && activity != null) activity.ActivityTraceFlags = ActivityTraceFlags.None; - } catch (OperationCanceledException e) when (Stopping.IsCancellationRequested) { + } catch (OperationCanceledException e) when (context.CancellationToken.IsCancellationRequested) { Log.MessageIgnoredWhenStopping(e); } catch (Exception e) { context.Nack(SubscriptionId, e); } @@ -178,84 +313,65 @@ protected async ValueTask Handler(IMessageConsumeContext context) { } } - // TODO: Passing the handler function would allow decoupling subscribers from handlers - protected abstract ValueTask Subscribe(CancellationToken cancellationToken); - - protected abstract ValueTask Unsubscribe(CancellationToken cancellationToken); + /// + /// Connects the transport. Returns once up, throws if it can't come up. Called once per run and must be + /// repeatable on the same instance — reassign fields rather than assume them unset. + /// + /// + /// A transport with its own polling or reading loop starts it here on a task of its own, reports the + /// loop's death as this run's failure (unless itself ended it), and + /// registers an callback that awaits the loop so the next + /// Connect never overlaps it. A callback-driven transport just connects and returns. Register each + /// acquired handle on via as it's + /// acquired, so a Connect that fails part-way still releases what was taken. + /// + protected abstract ValueTask Connect(SubscriptionRun run); - [PublicAPI] - protected virtual async Task Resubscribe(TimeSpan delay, CancellationToken cancellationToken) { - await Task.Delay(delay, cancellationToken).NoContext(); - - while (IsRunning && IsDropped && !cancellationToken.IsCancellationRequested) { - try { - Log.SubscriptionResubscribing(); + public async ValueTask DisposeAsync() { + // Exchange, not check-then-set: prevents two concurrent disposals both reaching the pipe (disposing + // it twice double-disposes every filter). + if (Interlocked.Exchange(ref _disposed, 1) != 0) return; - await Subscribe(cancellationToken).NoContext(); + // Before the pipe, since a live run delivers into it. Unbounded wait is safe because teardown + // carries its own budget. + await StopSession(CancellationToken.None).NoContext(); - IsDropped = false; - _onSubscribed?.Invoke(Options.SubscriptionId); + await Pipe.DisposeAsync().NoContext(); - Log.SubscriptionResubscribed(); - } catch (OperationCanceledException) { } catch (Exception e) { - Log.SubscriptionResubscribeFailed(e); - await Task.Delay(1000, cancellationToken).NoContext(); - } - } + GC.SuppressFinalize(this); } - protected void Dropped(DropReason reason, Exception? exception) { - if (!IsRunning) return; - - Log.SubscriptionDropped(reason, exception); - - IsDropped = true; - _onDropped?.Invoke(Options.SubscriptionId, reason, exception); - - // Read the token here rather than inside the background task below: Unsubscribe disposes - // Stopping, and reading .Token from a disposed source throws, which the task would surface as - // a spurious warning plus an unobserved exception. A token captured before the dispose stays - // usable afterwards, so hoisting the read is what makes the resubscribe safe. Losing the race - // outright means shutdown already got there, and there's nothing left to resubscribe to. - CancellationToken stopping; - - try { stopping = Stopping.Token; } catch (ObjectDisposedException) { return; } - - // Same reasoning for a token that's merely cancelled, which is the state Unsubscribe leaves it - // in for most of shutdown. Resubscribing from there can't succeed, and it isn't free: the - // checkpoint subscription's Resubscribe disposes the commit handler before it ever looks at the - // token, putting a second disposer in the race with Finalize. - if (stopping.IsCancellationRequested) return; - - Task.Run( - async () => { - // Check again: Unsubscribe may have cancelled between the check above and this task - // getting scheduled. It doesn't close the race — Resubscribe still disposes the commit - // handler before it looks at the token — but it keeps the common case out of it. - if (stopping.IsCancellationRequested) return; - - var delay = reason == DropReason.Stopped ? TimeSpan.FromSeconds(10) : TimeSpan.FromSeconds(2); - Log.SubscriptionWillResubscribe(delay); - - try { await Resubscribe(delay, stopping).NoContext(); } catch (Exception e) { - Log.WarnLog?.Log(e.Message); - - throw; - } - } - ); - } + /// + /// Everything one call brought: the run token, stop signal, callbacks, and + /// settings. One record, so Unsubscribe reads a consistent set rather than independently-moving + /// fields. + /// + sealed record Session(CancellationTokenSource Lifetime, TaskCompletionSource Finished, OnSubscribed OnSubscribed, OnDropped OnDropped, SupervisorSettings Settings); +} - bool _disposed; +/// +/// validated once, at , rather +/// than on every use, since options are mutable and an operator should hear about a bad setting once per +/// subscribe, not once per reconnect. +/// +internal readonly record struct SupervisorSettings(TimeSpan RetryDelay, TimeSpan TeardownTimeout) { + public static SupervisorSettings From(SubscriptionOptions options, LogContext log) { + var retryDelay = options.RetryDelay; + + // InfiniteTimeSpan is exempt: both Task.Delay and CancellationTokenSource accept it as "never". + if (retryDelay < TimeSpan.Zero && retryDelay != Timeout.InfiniteTimeSpan) { + log.SubscriptionRetryDelayInvalid(retryDelay, SubscriptionOptions.DefaultRetryDelay); + retryDelay = SubscriptionOptions.DefaultRetryDelay; + } - public async ValueTask DisposeAsync() { - if (_disposed) return; + var teardownTimeout = options.TeardownTimeout; - await Pipe.DisposeAsync().NoContext(); + if (teardownTimeout < TimeSpan.Zero && teardownTimeout != Timeout.InfiniteTimeSpan) { + log.SubscriptionTeardownTimeoutInvalid(teardownTimeout, SubscriptionOptions.DefaultTeardownTimeout); + teardownTimeout = SubscriptionOptions.DefaultTeardownTimeout; + } - // Stopping.Dispose(); - _disposed = true; - GC.SuppressFinalize(this); + return new(retryDelay, teardownTimeout); } } diff --git a/src/Core/src/Eventuous.Subscriptions/EventSubscriptionWithCheckpoint.cs b/src/Core/src/Eventuous.Subscriptions/EventSubscriptionWithCheckpoint.cs index bb39def65..70b8fcd5f 100644 --- a/src/Core/src/Eventuous.Subscriptions/EventSubscriptionWithCheckpoint.cs +++ b/src/Core/src/Eventuous.Subscriptions/EventSubscriptionWithCheckpoint.cs @@ -1,7 +1,6 @@ // Copyright (C) Eventuous HQ OÜ. All rights reserved // Licensed under the Apache License, Version 2.0. -using System.Runtime.CompilerServices; using Microsoft.Extensions.Logging; namespace Eventuous.Subscriptions; @@ -35,9 +34,6 @@ public abstract class EventSubscriptionWithCheckpoint( static ConsumePipe ConfigurePipe(ConsumePipe pipe, int concurrencyLimit) => PipelineIsAsync(pipe) ? pipe : pipe.AddFilterFirst(new AsyncHandlingFilter((uint)concurrencyLimit)); - EventPosition? LastProcessed { get; set; } - CheckpointCommitHandler? CheckpointCommitHandler { get; set; } - protected ICheckpointStore CheckpointStore { get; } = Ensure.NotNull(checkpointStore); protected SubscriptionKind Kind { get; } = kind; @@ -52,14 +48,56 @@ EventPosition GetPositionFromContext(IMessageConsumeContext context) SubscriptionKind.Stream => EventPosition.FromContext(context) }; - protected async ValueTask HandleInternal(IMessageConsumeContext context) { + /// + /// A run carrying this attempt's own commit handler, so an acknowledgement reaches the handler that + /// dispatched it, and the base class never has to know checkpoints exist. + /// + sealed class CheckpointedRun(CancellationToken lifetime, CheckpointCommitHandler checkpoint) : SubscriptionRun(lifetime) { + internal CheckpointCommitHandler Checkpoint { get; } = checkpoint; + } + + /// + /// Sealed so every run reaching this class carries a commit handler, letting find one + /// without a lookup or a null check. + /// + protected sealed override SubscriptionRun CreateRun(CancellationToken lifetime) { + var run = new CheckpointedRun( + lifetime, + new( + Options.SubscriptionId, + CheckpointStore, + TimeSpan.FromMilliseconds(Options.CheckpointCommitDelayMs), + Options.CheckpointCommitBatchSize, + LoggerFactory + ) + ); + + // Registered first, before Connect, so it's the first OnDisconnect registration — and since release + // order reverses registration order, it releases LAST, after every transport handle. That's + // checkpoint durability: acks in flight must land before the handler that commits them stops. + // Moving this into or after Connect would release the handler too early. + run.OnDisconnect(_ => run.Checkpoint.DisposeAsync()); + + return run; + } + + /// + /// Cast holds by construction: every run reaching this class comes from the sealed . + /// + static CheckpointCommitHandler Checkpoint(SubscriptionRun run) => ((CheckpointedRun)run).Checkpoint; + + /// + /// Run is passed explicitly, not looked up, so a message that completes after its run ended acknowledges + /// into that (refusing) run, not into whichever replaced it. + /// + protected async ValueTask HandleInternal(SubscriptionRun run, IMessageConsumeContext context) { try { Logger.Current = Log; - var ctx = new AsyncConsumeContext(context, Ack, NackOnAsyncWorker); + + var ctx = new AsyncConsumeContext(context, c => Ack(run, c), (c, e) => NackOnAsyncWorker(run, c, e)); await Handler(ctx).NoContext(); } catch (OperationCanceledException e) when (context.CancellationToken.IsCancellationRequested) { context.LogContext.MessageHandlingFailed(Options.SubscriptionId, context, e); - Dropped(DropReason.Stopped, e); } catch (Exception e) { context.LogContext.MessageHandlingFailed(Options.SubscriptionId, context, e); @@ -68,87 +106,51 @@ protected async ValueTask HandleInternal(IMessageConsumeContext context) { } /// - /// Wraps the Nack callback for the async worker path. When ThrowOnError is true, - /// Nack throws to signal a fatal error. On the async worker thread (AsyncHandlingFilter), - /// that throw would silently kill the channel worker without triggering Dropped/Resubscribe. - /// This wrapper catches the throw and calls Dropped instead. + /// Nack throws under ThrowOnError; on the channel worker that + /// would silently kill the reader, so it's turned into this run's failure instead. /// - ValueTask NackOnAsyncWorker(IMessageConsumeContext context, Exception exception) { + ValueTask NackOnAsyncWorker(SubscriptionRun run, IMessageConsumeContext context, Exception exception) { try { - return Nack(context, exception); + return Nack(run, context, exception); } catch (Exception) { - Dropped(DropReason.SubscriptionError, exception); + run.Fail(DropReason.SubscriptionError, exception); return default; } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - ValueTask Ack(IMessageConsumeContext context) { - // Capture locally — CheckpointCommitHandler can be nulled by Resubscribe/DisposeCommitHandler - // on another thread while the async worker is still completing a message. - var handler = CheckpointCommitHandler; + async ValueTask Ack(SubscriptionRun run, IMessageConsumeContext context) { + var position = GetPositionFromContext(context); - if (handler is null) return default; + // Committed through the dispatching run, never whichever run is current: another run's counter + // could collide with a live sequence or let the checkpoint advance over a message it never handled. + // + // Uncancellable on purpose: a dropped CommitPosition is poison — the handler won't commit past the + // gap it leaves. + var commit = new CommitPosition(position.Position!.Value, context.Sequence, position.Created) { LogContext = context.LogContext }; - var eventPosition = GetPositionFromContext(context); - LastProcessed = eventPosition; + if (!await Checkpoint(run).Commit(commit, CancellationToken.None).NoContext()) { + context.LogContext.MessageFromPreviousRunIgnored(context); - context.LogContext.MessageAcked(context.MessageType, context.GlobalPosition); + return; + } - return handler.Commit( - new(eventPosition.Position!.Value, context.Sequence, eventPosition.Created) { LogContext = context.LogContext }, - context.CancellationToken - ); + context.LogContext.MessageAcked(context.MessageType, context.GlobalPosition); } - ValueTask Nack(IMessageConsumeContext context, Exception exception) { + ValueTask Nack(SubscriptionRun run, IMessageConsumeContext context, Exception exception) { context.LogContext.MessageNacked(context.MessageType, context.GlobalPosition, exception); - return Options.ThrowOnError ? throw exception : Ack(context); + return Options.ThrowOnError ? throw exception : Ack(run, context); } - protected async Task GetCheckpoint(CancellationToken cancellationToken) { - CheckpointCommitHandler ??= new( - options.SubscriptionId, - checkpointStore, - TimeSpan.FromMilliseconds(options.CheckpointCommitDelayMs), - options.CheckpointCommitBatchSize, - LoggerFactory - ); - - if (IsRunning && LastProcessed != null) { return new(Options.SubscriptionId, LastProcessed?.Position); } - + /// + /// Called by Connect once per run, before any dispatch. Always reads the store — correct, since + /// the run being replaced flushed before it ended. + /// + protected async Task GetCheckpoint(SubscriptionRun run) { Logger.Current = Log; - var checkpoint = await CheckpointStore.GetLastCheckpoint(Options.SubscriptionId, cancellationToken).NoContext(); - - LastProcessed = new EventPosition(checkpoint.Position, DateTime.Now); - - return checkpoint; - } - - protected override async Task Resubscribe(TimeSpan delay, CancellationToken cancellationToken) { - // Reset checkpoint state so the new run reads from the committed checkpoint, - // not from LastProcessed (which may be ahead of the failed event). - LastProcessed = null; - Sequence = 0; - - await DisposeCommitHandler(); - - await base.Resubscribe(delay, cancellationToken); - } - - protected override async ValueTask Finalize(CancellationToken cancellationToken) => await DisposeCommitHandler(); - - async ValueTask DisposeCommitHandler() { - // Swap to null first so the concurrent path (Resubscribe vs Finalize) sees null. The read and - // the write aren't atomic, so both paths can still come away with the same handler — that stays - // safe because the commit worker's dispose is idempotent, and the second caller awaits the first - // one's shutdown rather than re-entering it and cancelling an already-disposed CTS (AI-1699). - var handler = CheckpointCommitHandler; - CheckpointCommitHandler = null; - - if (handler != null) await handler.DisposeAsync().NoContext(); + return await CheckpointStore.GetLastCheckpoint(Options.SubscriptionId, run.Token).NoContext(); } } diff --git a/src/Core/src/Eventuous.Subscriptions/Filters/AsyncHandlingFilter.cs b/src/Core/src/Eventuous.Subscriptions/Filters/AsyncHandlingFilter.cs index 9c0baa212..1c45b254c 100644 --- a/src/Core/src/Eventuous.Subscriptions/Filters/AsyncHandlingFilter.cs +++ b/src/Core/src/Eventuous.Subscriptions/Filters/AsyncHandlingFilter.cs @@ -58,17 +58,23 @@ static async ValueTask DelayedConsume(WorkerTask workerTask, CancellationToken c var exception = ctx.HandlingResults.GetException(); switch (exception) { - case TaskCanceledException: - case OperationCanceledException: break; + // Stopping, not failing: the message was never decided, so don't ack it — the next + // run redelivers it from the checkpoint, or the broker once its lock lapses. + case OperationCanceledException when ctx.CancellationToken.IsCancellationRequested: + return; + case null: throw new ApplicationException("Event handler failed"); - default: throw exception; + + // Anything else — including a self-inflicted cancellation such as an HTTP timeout + // (TaskCanceledException) — is an ordinary failure and goes to Nack; what Nack does + // with it is the subscription's policy, not this filter's. + default: throw exception; } } if (!ctx.HandlingResults.IsPending()) await ctx.Acknowledge().NoContext(); - } catch (TaskCanceledException) { - return; - } catch (OperationCanceledException) { + } catch (OperationCanceledException) when (ctx.CancellationToken.IsCancellationRequested) { + // Same rule: don't acknowledge. return; } catch (Exception e) { ctx.LogContext.MessageHandlingFailed(nameof(AsyncHandlingFilter), workerTask.Context, e); @@ -83,10 +89,15 @@ static async ValueTask DelayedConsume(WorkerTask workerTask, CancellationToken c } } - protected override ValueTask Send(AsyncConsumeContext context, LinkedListNode? next) - => next == null - ? throw new InvalidOperationException("Concurrent context must have a next filer") - : _worker.Write(new(context, next), context.CancellationToken); + protected override async ValueTask Send(AsyncConsumeContext context, LinkedListNode? next) { + if (next == null) throw new InvalidOperationException("Concurrent context must have a next filer"); + + // Refused means the worker is stopping — logged so a message vanishing between the pipe and a + // handler isn't silent. + if (!await _worker.Write(new(context, next), context.CancellationToken).NoContext()) { + context.LogContext.MessageNotQueued(context); + } + } readonly record struct WorkerTask(AsyncConsumeContext Context, LinkedListNode Filter); diff --git a/src/Core/src/Eventuous.Subscriptions/IMessageSubscription.cs b/src/Core/src/Eventuous.Subscriptions/IMessageSubscription.cs index 190c189db..7ddd90980 100644 --- a/src/Core/src/Eventuous.Subscriptions/IMessageSubscription.cs +++ b/src/Core/src/Eventuous.Subscriptions/IMessageSubscription.cs @@ -14,6 +14,14 @@ namespace Eventuous.Subscriptions; public interface IMessageSubscription { string SubscriptionId { get; } + /// + /// Starts the subscription, returning once it is up. One run per instance — calling it again before + /// completes throws . A subscription + /// that failed to come up isn't running, so it can be started again. + /// + /// Called each time the subscription comes up, including after a resubscribe. + /// Called each time it goes down. + /// Cancelling it stops the subscription. ValueTask Subscribe(OnSubscribed onSubscribed, OnDropped onDropped, CancellationToken cancellationToken); ValueTask Unsubscribe(OnUnsubscribed onUnsubscribed, CancellationToken cancellationToken); diff --git a/src/Core/src/Eventuous.Subscriptions/Logging/SubscriptionLogging.cs b/src/Core/src/Eventuous.Subscriptions/Logging/SubscriptionLogging.cs index a46f141fb..db93eab56 100644 --- a/src/Core/src/Eventuous.Subscriptions/Logging/SubscriptionLogging.cs +++ b/src/Core/src/Eventuous.Subscriptions/Logging/SubscriptionLogging.cs @@ -76,6 +76,30 @@ public void FailedToHandleMessageWithRetry(string handlerType, string messageTyp public void MessageAcked(string messageType, ulong position) => log.TraceLog?.Log("Message {Type} acknowledged at {Position}", messageType, position); + /// + /// A message from a dropped run that finished after its replacement started; unacked, so the new + /// run redelivers it from the checkpoint. + /// + public void MessageFromPreviousRunIgnored(IBaseConsumeContext context) + => log.DebugLog?.Log( + "Message {MessageType} from {Stream}:{Position} belongs to a previous run and was not acknowledged", + context.MessageType, + context.Stream, + context.GlobalPosition + ); + + /// + /// The handling worker is stopping and never took the message; left unacknowledged, so it comes + /// back on the next run. + /// + public void MessageNotQueued(IBaseConsumeContext context) + => log.WarnLog?.Log( + "Message {MessageType} from {Stream}:{Position} was not queued for handling because the subscription is stopping", + context.MessageType, + context.Stream, + context.GlobalPosition + ); + public void MessageNacked(string messageType, ulong position, Exception exception) => log.WarnLog?.Log(exception, "Message {Type} not acknowledged at {Position}", messageType, position); @@ -86,9 +110,39 @@ public void SubscriptionDropped(DropReason reason, Exception? exception) => log.WarnLog?.Log(exception, "Dropped: {Reason}", reason); public void SubscriptionWillResubscribe(TimeSpan delay) => log.WarnLog?.Log($"Will resubscribe after {delay}"); + + /// + /// Configured retry delay can't be waited on; fell back to the default. Otherwise this + /// misconfiguration would only show up as a subscription retrying flat out. + /// + public void SubscriptionRetryDelayInvalid(TimeSpan configured, TimeSpan used) + => log.WarnLog?.Log($"Retry delay {configured} cannot be waited on, using {used} instead"); + + /// + /// Configured teardown timeout can't be waited on; fell back to the default. Otherwise this + /// misconfiguration would only show up as a teardown that never gives up. + /// + public void SubscriptionTeardownTimeoutInvalid(TimeSpan configured, TimeSpan used) + => log.WarnLog?.Log($"Teardown timeout {configured} cannot be waited on, using {used} instead"); + + /// + /// The caller stopped waiting for the subscription to finish stopping. Not a failed stop — teardown + /// runs on its own budget regardless. + /// + public void SubscriptionStopTimedOut() => log.WarnLog?.Log("Gave up waiting for the subscription to stop"); + public void SubscriptionResubscribing() => log.WarnLog?.Log("Resubscribing"); public void SubscriptionResubscribed() => log.InfoLog?.Log("Resubscribed"); - public void SubscriptionResubscribeFailed(Exception e) => log.ErrorLog?.Log(e, "Failed to resubscribe"); + + /// + /// The supervisor itself failed, leaving the subscription down for good — anything else it sees is + /// reported as a drop and retried. + /// + public void SubscriptionSuperviseFailed(Exception e) => log.ErrorLog?.Log(e, "Subscription supervisor failed"); + + public void SubscriptionDisconnectFailed(Exception e) => log.WarnLog?.Log(e, "Failed to release the subscription"); + + public void SubscriptionCallbackFailed(Exception e) => log.WarnLog?.Log(e, "Subscription callback failed"); } public static void MessageTypeNotFound(this ILogger? log) diff --git a/src/Core/src/Eventuous.Subscriptions/SubscriptionOptions.cs b/src/Core/src/Eventuous.Subscriptions/SubscriptionOptions.cs index d7f2bdfa1..6b1ec9504 100644 --- a/src/Core/src/Eventuous.Subscriptions/SubscriptionOptions.cs +++ b/src/Core/src/Eventuous.Subscriptions/SubscriptionOptions.cs @@ -14,6 +14,36 @@ public abstract record SubscriptionOptions { /// Set to true if you want the subscription to fail and stop if anything goes wrong. /// public bool ThrowOnError { get; set; } + + /// + /// How long the subscription waits before replacing a dropped connection. Default is two seconds. + /// + /// + /// Sets the load an unreachable broker sees from a fleet of retrying instances, and how long a recovered + /// one takes to be noticed. + /// + public TimeSpan RetryDelay { get; set; } = DefaultRetryDelay; + + /// + /// Default for , and its fallback when set to a delay that can't be waited on. + /// + public static readonly TimeSpan DefaultRetryDelay = TimeSpan.FromSeconds(2); + + /// + /// How long a resubscribe lets the transport take over releasing its connection, joining its message loop + /// and closing out the run it is replacing, before it is asked to stop being graceful about it. Default is + /// five seconds. + /// + /// + /// Not a deadline: teardown waits for every release regardless. Once it elapses releases are asked to drop + /// what's optional, but essential work — the final checkpoint flush above all — still completes. + /// + public TimeSpan TeardownTimeout { get; set; } = DefaultTeardownTimeout; + + /// + /// Default for , and its fallback when set to a value that can't be waited on. + /// + public static readonly TimeSpan DefaultTeardownTimeout = TimeSpan.FromSeconds(5); } public abstract record SubscriptionWithCheckpointOptions : SubscriptionOptions { diff --git a/src/Core/src/Eventuous.Subscriptions/SubscriptionRun.cs b/src/Core/src/Eventuous.Subscriptions/SubscriptionRun.cs new file mode 100644 index 000000000..c8ff25ec6 --- /dev/null +++ b/src/Core/src/Eventuous.Subscriptions/SubscriptionRun.cs @@ -0,0 +1,135 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +namespace Eventuous.Subscriptions; + +using Logging; + +/// +/// Why a run ended. A reference type so can make +/// "first reason wins" a single atomic operation instead of a lock. +/// +sealed record Failure(DropReason Reason, Exception? Exception); + +/// +/// One attempt at being subscribed. State scoped to that attempt — token, sequence, failure — lives here so +/// a late signal names the run it belongs to, not whichever run happens to be current. +/// +/// +/// Derivable, so a subscription can hang per-attempt state off the run and release it via +/// instead of a field the next run would overwrite. owns its +/// own teardown order. +/// +public class SubscriptionRun { + readonly CancellationTokenSource _cts; + + // Load-bearing, not hygiene: without it, Fail from the channel worker resumes the supervisor inline and + // runs the whole teardown on the thread that owns the message reader. + readonly TaskCompletionSource _ended = new(TaskCreationOptions.RunContinuationsAsynchronously); + + Failure? _failure; + ulong _sequence; + List>? _releases; + + // protected: a transport can derive its own run. internal: the supervisor builds the plain one, which + // isn't itself a SubscriptionRun. + protected internal SubscriptionRun(CancellationToken lifetime) { + _cts = CancellationTokenSource.CreateLinkedTokenSource(lifetime); + Token = _cts.Token; // Copied while the source is alive, so it stays readable after disposal. + + // Also completes Ended, so shutdown and failure arrive through one arm. + _cts.Token.Register(static s => ((TaskCompletionSource)s!).TrySetResult(), _ended); + } + + /// + /// Cancelled when this run ends. The token a transport gives its I/O. + /// + public CancellationToken Token { get; } + + /// + /// Completed when this run is over, for any reason — covers both a failure and a shutdown. + /// + public Task Ended => _ended.Task; + + /// + /// Returns the value from before the increment, like the Sequence++ it replaces. Belongs to the + /// run, so a replacement starts from zero by construction. + /// + public ulong NextSequence() => Interlocked.Increment(ref _sequence) - 1; + + /// + /// Ends this run. Safe from any thread, any run age, inside a catch or finally. Touches nothing disposable, + /// logs and invokes nothing — the supervisor reports the drop, once per run, on its own stack. + /// + public void Fail(DropReason reason, Exception? exception) { + if (Interlocked.CompareExchange(ref _failure, new(reason, exception), null) is not null) return; + + _ended.TrySetResult(); + } + + internal Failure? Failure => Volatile.Read(ref _failure); + + /// + /// Stops this run: production stops, the run ends, then registered handles release in reverse + /// registration order — first registered releases last, since acks in flight must land before the + /// handles they need are gone. + /// + /// + /// Returns only once every release has finished. tells them to hurry, it + /// does not cut them off — see . Never throws; runs once per run. + /// + internal async ValueTask Stop(CancellationToken graceful, LogContext log) { + // Also completes Ended (see ctor). Guarded because it also runs whatever the transport registered + // on this token. + try { + await _cts.CancelAsync().NoContext(); + } catch (Exception e) { + log.SubscriptionDisconnectFailed(e); + } + + try { + await Disconnect(graceful, log).NoContext(); + } finally { + // Must run even if Disconnect throws (it doesn't today): unregisters this run from the + // subscription's lifetime token, which outlives it. + _cts.Dispose(); + } + } + + /// + /// Registers a handle and its release. Call from Connect as each handle is acquired, so a Connect that + /// throws part-way still has everything taken so far released by teardown. Releases run in reverse + /// acquisition order, after the token cancels. + /// + /// + /// Called only from Connect, on the supervisor's stack — the same stack every release runs on later — so + /// no lock is needed. + /// + public void OnDisconnect(Func release) { + ArgumentNullException.ThrowIfNull(release); + (_releases ??= []).Add(release); + } + + /// + /// Releases every handle registered through , in reverse order, each guarded so + /// one failure can't strand the rest. Never throws; clears registrations so a second call is a no-op. + /// + /// + /// means "stop being graceful and finish quickly", never "stop": every release + /// is awaited, or the next run could read a checkpoint the previous one hadn't finished writing. + /// + async ValueTask Disconnect(CancellationToken graceful, LogContext log) { + if (_releases is not { Count: > 0 } releases) return; + + for (var i = releases.Count - 1; i >= 0; i--) { + // Invocation inside the try: synchronous release bodies throw before returning a ValueTask. + try { + await releases[i](graceful).NoContext(); + } catch (Exception e) { + log.SubscriptionDisconnectFailed(e); + } + } + + releases.Clear(); + } +} diff --git a/src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/StoreFixtureBase.cs b/src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/StoreFixtureBase.cs index 27cc6833b..b4da34654 100644 --- a/src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/StoreFixtureBase.cs +++ b/src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/StoreFixtureBase.cs @@ -22,7 +22,12 @@ public abstract class StoreFixtureBase { public abstract partial class StoreFixtureBase(LogLevel logLevel) : StoreFixtureBase, IStartableFixture where TContainer : DockerContainer { public virtual async Task InitializeAsync() { - Container = CreateContainer(); + // Initialising twice is a restart, and tests do it — the previous round's container and provider + // must be released before these properties are overwritten, or they're abandoned. + if (_initialized) await Teardown(); + + _initialized = true; + Container = CreateContainer(); await Container.StartAsync(); var services = new ServiceCollection(); @@ -56,15 +61,33 @@ public virtual async ValueTask DisposeAsync() { if (_disposed) return; _disposed = true; - var inits = Provider.GetServices(); + await Teardown(); + GC.SuppressFinalize(this); + } - foreach (var hostedService in inits) { - await hostedService.StopAsync(CancellationToken.None); + /// + /// Releases one round's container and provider. Tolerant of a half-built fixture — a failed + /// may need to release a container without ever having built a provider. + /// + async ValueTask Teardown() { + var provider = Provider; + var container = Container; + + // Cleared before releasing, so a partial failure here doesn't find round one's disposed state again. + Provider = null!; + Container = null!; + + try { + if (provider is not null) { + foreach (var hostedService in provider.GetServices()) { + await hostedService.StopAsync(CancellationToken.None); + } + + await provider.DisposeAsync(); + } + } finally { + if (container is not null) await container.DisposeAsync(); } - - await Provider.DisposeAsync(); - await Container.DisposeAsync(); - GC.SuppressFinalize(this); } protected abstract void SetupServices(IServiceCollection services); @@ -78,6 +101,7 @@ protected virtual void GetDependencies(IServiceProvider provider) { } public IEventSerializer Serializer { get; private set; } = null!; bool _disposed; + bool _initialized; protected static string GetSchemaName() => NormaliseRegex().Replace(new Faker().Internet.UserName(), "").ToLower(); diff --git a/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/SubscriptionFixtureBase.cs b/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/SubscriptionFixtureBase.cs index dad909875..bf9a3a7ec 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/SubscriptionFixtureBase.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/SubscriptionFixtureBase.cs @@ -38,9 +38,12 @@ protected SubscriptionFixtureBase(bool autoStart = true, LogLevel logLevel = Log protected internal SubscriptionHealthCheck Health { get; } = new(); /// - /// True when the subscription has detected a drop and is trying to resubscribe. + /// True between a drop and the next resubscription, tracked from the callbacks below rather than asked + /// of the subscription. /// - public bool IsDropped => ((EventSubscription)Subscription).IsDropped; + public bool IsDropped => _dropped; + + volatile bool _dropped; /// /// Returns the subscription's end-of-stream measure delegate (requires an ). @@ -52,10 +55,12 @@ protected SubscriptionFixtureBase(bool autoStart = true, LogLevel logLevel = Log protected internal ValueTask StartSubscription() => Subscription.Subscribe( id => { + _dropped = false; Health.ReportHealthy(id); Log.LogInformation("{Subscription} subscribed", id); }, (id, reason, ex) => { + _dropped = true; Health.ReportUnhealthy(id, ex); Log.LogWarning(ex, "{Subscription} dropped {Reason}", id, reason); }, @@ -103,7 +108,14 @@ public override async Task InitializeAsync() { } public override async ValueTask DisposeAsync() { - if (_autoStart) await StopSubscription(); - await base.DisposeAsync(); + // Guarded so an initialisation failure before GetDependencies (e.g. a container never ready) still + // reaches the base's teardown, which releases it. + try { + if (_autoStart) await StopSubscription(); + } catch (Exception) { + // Must not cost us the container the base holds. + } finally { + await base.DisposeAsync(); + } } } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/TestEventHandler.cs b/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/TestEventHandler.cs index 38202ac5b..30cfe5d0a 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/TestEventHandler.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/TestEventHandler.cs @@ -34,8 +34,19 @@ public TestEventHandler() : this(null) { } public On AssertThat() => Hypothesis.On(_observer); + /// + /// Expects exactly to be handled within . Takes + /// the whole deadline: proving "n and no more" means watching the window out, so pick one that suits the + /// transport rather than one padded for the worst case. + /// + /// + /// An empty expectation matches everything instead, since Contains on an empty collection rejects + /// every message and would leave nothing for Exactly(0) to count — an assertion that cannot fail. + /// public Hypothesis AssertCollection(TimeSpan deadline, List collection) - => Hypothesis.On(_observer).Timebox(deadline).Exactly(collection.Count).Match(collection.Contains); + => collection.Count == 0 + ? Hypothesis.On(_observer).Timebox(deadline).AtMost(0).Match(_ => true) + : Hypothesis.On(_observer).Timebox(deadline).Exactly(collection.Count).Match(collection.Contains); /// /// Messages handled so far. Backed by a concurrent queue so tests can poll it while the subscription @@ -59,3 +70,4 @@ public void Reset() { } public record TestEventHandlerOptions(TimeSpan? Delay = null); + diff --git a/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs b/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs index 3970a3819..ca226ec27 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs @@ -9,10 +9,8 @@ namespace Eventuous.Tests.Subscriptions.Base; /// -/// The two properties the resubscribe path relies on from every transport. Since a drop now stops the -/// previous run before starting the next, teardown runs on a connection that will be used again, and a -/// transport whose resources are single-use has to rebuild them rather than restart them. Stated here so -/// each provider suite can assert it against real infrastructure. +/// Asserts against real infrastructure the two properties the resubscribe path relies on from every +/// transport: teardown may run more than once, and a stopped subscription must reconnect cleanly. /// public abstract class SubscriptionRestartBase( SubscriptionFixtureBase fixture @@ -26,9 +24,8 @@ SubscriptionFixtureBase - /// Unsubscribing twice must not throw. The framework only calls transport teardown for a live run, but - /// a provider may still be asked to release resources it has already released — through an explicit - /// stop, or by a drop landing while shutdown is in flight. + /// Unsubscribing twice must not throw, even though a provider may be asked to release resources it has + /// already released. /// protected async Task ShouldTolerateRepeatedUnsubscribe() { await fixture.StartSubscription(); @@ -37,7 +34,7 @@ protected async Task ShouldTolerateRepeatedUnsubscribe() { } /// - /// Subscribing again after a full stop must consume newly produced events. Asserts by event identity, + /// Subscribing again after a full stop must consume newly produced events, asserted by event identity /// so a replay of the first batch can't pass for the second. /// protected async Task ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/AssemblyInfo.cs b/src/Core/test/Eventuous.Tests.Subscriptions/AssemblyInfo.cs new file mode 100644 index 000000000..352c42e8d --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/AssemblyInfo.cs @@ -0,0 +1,7 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +// Backstop, not the budget, so a hung wait fails the test instead of hanging CI forever. Tests with a +// tighter bound still carry their own [Timeout]; this just has to clear the slowest legitimate test (the +// 100-cycle resubscribe ones, at a 60s budget). +[assembly: Timeout(120_000)] diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/CancelledMessageTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/CancelledMessageTests.cs new file mode 100644 index 000000000..97698b75b --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/CancelledMessageTests.cs @@ -0,0 +1,218 @@ +using System.Collections.Concurrent; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Eventuous.Tools; +using Shouldly; +using LoggingExtensions = Eventuous.TestHelpers.TUnit.Logging.LoggingExtensions; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// Pins down the fix in .DelayedConsume: whether a cancelled handler +/// gets acknowledged must turn on whose token was cancelled, not on the exception type. Before the fix, any +/// was acknowledged regardless of cause, silently skipping the +/// event in flight when a run's own teardown cancelled a parked handler. +/// +public class CancelledMessageTests { + /// + /// A handler cancelled because the run is ending was never given a verdict, so it must not be + /// acknowledged — only redelivered once the successor run comes up. + /// + [Test] + public async Task Handler_cancelled_by_shutdown_is_not_acknowledged_and_is_redelivered(CancellationToken ct) { + var loggerFactory = LoggingExtensions.GetLoggerFactory(); + var checkpointStore = new NoOpCheckpointStore(); + var committed = new ConcurrentQueue(); + checkpointStore.CheckpointStored += (_, cp) => committed.Enqueue(cp.Position); + + var handler = new ParkOnFirstDeliveryHandler(); + var pipe = new ConsumePipe().AddDefaultConsumer(handler); + + var options = new TestOptions { + SubscriptionId = "cancelled-not-acked", + ThrowOnError = true, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }; + + // The default retry delay is 2s; a short one keeps the test deterministic and fast. + options.RetryDelay = TimeSpan.FromMilliseconds(20); + + var subscription = new SingleEventSubscription(options, checkpointStore, pipe, loggerFactory); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await Wait.Until(() => handler.Parked.IsCompleted, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the handler should have received the first delivery and parked on it"); + + committed.ShouldBeEmpty("nothing should commit while the only delivery so far is still parked, undecided"); + + subscription.FailCurrentRun(); + + // The handler blocks again on redelivery, so the test can inspect the checkpoint before it's allowed to succeed. + (await Wait.Until(() => handler.RedeliveryStarted.IsCompleted, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the successor run should redeliver the event the cancelled handler never finished"); + + // If the fix regresses, DelayedConsume acknowledges the cancelled delivery and this fires. + committed.ShouldBeEmpty("the checkpoint must never move past an event whose only delivery was cancelled by shutdown, not decided"); + + handler.LetRedeliverySucceed(); + + (await Wait.Until(() => committed.Contains((ulong?)0), TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the checkpoint should reach position 0 once the redelivered event is actually handled"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// An from the handler's own token (e.g. an HttpClient timeout) + /// is an ordinary failure, not a shutdown — it must be skipped like any other exception, not left + /// unacknowledged. Guards against fixing the test above by matching on exception type instead of on + /// which token fired. + /// + [Test] + public async Task Handler_self_cancellation_is_an_ordinary_failure_and_is_skipped(CancellationToken ct) { + var loggerFactory = LoggingExtensions.GetLoggerFactory(); + var checkpointStore = new NoOpCheckpointStore(); + var committed = new ConcurrentQueue(); + checkpointStore.CheckpointStored += (_, cp) => committed.Enqueue(cp.Position); + + var handler = new SelfCancellingHandler(); + var pipe = new ConsumePipe().AddDefaultConsumer(handler); + + var options = new TestOptions { + SubscriptionId = "self-cancel-is-ordinary-failure", + ThrowOnError = false, + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }; + + var subscription = new SingleEventSubscription(options, checkpointStore, pipe, loggerFactory); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await Wait.Until(() => committed.Contains((ulong?)0), TimeSpan.FromSeconds(5))) + .ShouldBeTrue("a handler-local cancellation should be skipped and the checkpoint should advance past it"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + + + record TestOptions : SubscriptionWithCheckpointOptions; + + /// + /// Delivers one synthetic event at position 0, once per run, then parks until the run ends. + /// + sealed class SingleEventSubscription( + TestOptions options, + ICheckpointStore checkpointStore, + ConsumePipe pipe, + ILoggerFactory? loggerFactory + ) + : EventSubscriptionWithCheckpoint( + options, + checkpointStore, + pipe, + 1, + SubscriptionKind.All, + loggerFactory, + null, + null + ) { + SubscriptionRun? _run; + + /// + /// Fails the current run, standing in for a transport drop or any other reason the supervisor tears a run down. + /// + public void FailCurrentRun() + => Volatile.Read(ref _run)?.Fail(DropReason.SubscriptionError, new InvalidOperationException("Simulated drop while a handler is parked")); + + protected override async ValueTask Connect(SubscriptionRun run) { + Volatile.Write(ref _run, run); + + await GetCheckpoint(run).NoContext(); + + // Started on a task of its own so it never runs inline on the supervisor's stack during Connect. + var pumping = Task.Run(() => RunDeliverOnce(run), CancellationToken.None); + + // No handle of its own to release: registered purely to join the loop before the next Connect. + run.OnDisconnect(_ => new(pumping)); + } + + /// + /// Runs and reports its own death, the same contract a real transport keeps. + /// + Task RunDeliverOnce(SubscriptionRun run) + => TransportPump.Run(run, () => DeliverOnce(run), "SingleEventSubscription pump ended while the connection was up"); + + async Task DeliverOnce(SubscriptionRun run) { + var context = new MessageConsumeContext( + Guid.NewGuid().ToString(), + "TestEvent", + "application/json", + "test-stream", + 0, + 0, + 0, + run.NextSequence(), + DateTime.UtcNow, + new { EventNumber = 0 }, + new(), + Options.SubscriptionId, + run.Token + ) { LogContext = Log }; + + await HandleInternal(run, context).NoContext(); + + // Parked rather than returned: a pump ending while its connection is up is read as a drop. + await run.Ended.NoContext(); + } + } + + /// + /// Parks on the first delivery until cancelled, then parks again on redelivery so the test can inspect + /// the checkpoint before the second attempt succeeds. + /// + sealed class ParkOnFirstDeliveryHandler : BaseEventHandler { + readonly TaskCompletionSource _neverCompletes = new(); + readonly TaskCompletionSource _parked = new(TaskCreationOptions.RunContinuationsAsynchronously); + readonly TaskCompletionSource _redeliveryStarted = new(TaskCreationOptions.RunContinuationsAsynchronously); + readonly TaskCompletionSource _proceedWithSuccess = new(TaskCreationOptions.RunContinuationsAsynchronously); + + int _deliveries; + + public Task Parked => _parked.Task; + public Task RedeliveryStarted => _redeliveryStarted.Task; + + public void LetRedeliverySucceed() => _proceedWithSuccess.TrySetResult(); + + public override async ValueTask HandleEvent(IMessageConsumeContext context) { + var attempt = Interlocked.Increment(ref _deliveries); + + switch (attempt) { + case 1: + _parked.TrySetResult(); + await _neverCompletes.Task.WaitAsync(context.CancellationToken).NoContext(); + break; + case 2: + _redeliveryStarted.TrySetResult(); + await _proceedWithSuccess.Task.NoContext(); + break; + } + + return EventHandlingStatus.Success; + } + } + + /// + /// Fails the way an HttpClient call does on a timeout: an + /// whose token belongs to the failing operation, not to anything the subscription owns. + /// + sealed class SelfCancellingHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) + => throw new TaskCanceledException("Simulated HttpClient timeout"); + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/CapturingLoggerFactory.cs b/src/Core/test/Eventuous.Tests.Subscriptions/CapturingLoggerFactory.cs new file mode 100644 index 000000000..cb6165867 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/CapturingLoggerFactory.cs @@ -0,0 +1,47 @@ +using System.Collections.Concurrent; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// Captures log lines so a test can assert on what the production code reported. Defaults to warnings and +/// above — the level most assertions want — but takes anything down to for the +/// tests that assert on the supervisor's own debug narration ("Resubscribing", "belongs to a previous run"). +/// +/// +/// The exception is appended to the captured text: ILogger's formatter only renders the state, so +/// text that exists solely in the exception message (e.g. "CancellationTokenSource has been disposed") +/// would otherwise never match. +/// +sealed class CapturingLoggerFactory(LogLevel minimum = LogLevel.Warning) : ILoggerFactory { + readonly ConcurrentQueue _lines = []; + + /// + /// Polls rather than waiting out the full timeout, checking once more after the deadline for a + /// boundary-line match. + /// + public Task WaitForWarning(string contains, TimeSpan timeout) => Wait.Until(() => Contains(contains), timeout); + + public bool Contains(string text) => _lines.Any(line => line.Contains(text)); + + public int Count(string text) => _lines.Count(line => line.Contains(text)); + + public ILogger CreateLogger(string categoryName) => new CapturingLogger(_lines, minimum); + + public void AddProvider(ILoggerProvider provider) { } + + public void Dispose() { } + + sealed class CapturingLogger(ConcurrentQueue lines, LogLevel minimum) : ILogger { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { + if (logLevel < minimum) return; + + var line = formatter(state, exception); + + lines.Enqueue(exception is null ? line : $"{line} {exception}"); + } + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs index 206234303..0671db38c 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs @@ -1,4 +1,3 @@ -using System.Threading.Channels; using Eventuous.Subscriptions.Checkpoints; using Shouldly; @@ -110,19 +109,14 @@ public async Task Dispose_releases_a_backpressured_commit_and_drains_without_han await Task.Delay(200, cancellationToken); overflow.IsCompleted.ShouldBeFalse("The overflow Commit should be backpressured before Dispose begins"); - // Dispose while the store is stalled and a writer is parked on the full channel. Dispose - // completes the channel writer, which releases the parked write rather than leaving it - // hanging forever: the pending WriteAsync faults with ChannelClosedException (observed - // behaviour, pinned here). The position is lost, but only because the handler is shutting - // down — the caller is unblocked, not stalled. The outcome is captured first, then - // asserted, so a surprise here still flows through the finally-side cleanup. + // The parked caller must come back rather than hang, and be told its position never made it — + // silence here would let the acknowledgement path treat a dropped position as committed. disposeTask = handler.DisposeAsync().AsTask(); - var overflowOutcome = await overflow.AsTask() - .WaitAsync(TimeSpan.FromSeconds(5), cancellationToken) - .ContinueWith(t => t.Exception?.GetBaseException(), TaskContinuationOptions.ExecuteSynchronously); + // Generous rather than tight: the release is immediate, but happens on disposal's thread while the store is stalled. + var accepted = await overflow.AsTask().WaitAsync(TimeSpan.FromSeconds(20), cancellationToken); - overflowOutcome.ShouldBeOfType("Completing the writer should release the parked Commit with ChannelClosedException"); + accepted.ShouldBeFalse("a Commit released by disposal never queued its position, and has to say so"); // Let the store recover so dispose can drain the queued positions and run its final // force-commit within its own internal bounds. diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerLifecycleTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerLifecycleTests.cs new file mode 100644 index 000000000..26a8fd66a --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerLifecycleTests.cs @@ -0,0 +1,149 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using Eventuous.Subscriptions.Checkpoints; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// A handler belongs to one run and lasts exactly as long as it does — it owns whether it still accepts +/// positions, so replacing the run underneath a dispatched commit can't redirect it elsewhere. +/// +public class CheckpointCommitHandlerLifecycleTests { + [Test] + public async Task An_open_handler_commits() { + var (handler, committed) = Build(); + + var accepted = await handler.Commit(Position(7, sequence: 0), CancellationToken.None); + + accepted.ShouldBeTrue(); + + // Commits are batched onto the handler's own worker, so the store sees them a beat later. + var stored = await Wait.Until(() => committed().Contains(7UL), TimeSpan.FromSeconds(5)); + + stored.ShouldBeTrue("an accepted commit never reached the store"); + + await handler.DisposeAsync(); + } + + /// + /// A commit that claimed success after the handler stopped would acknowledge a message nothing is + /// going to store. + /// + [Test] + public async Task A_stopped_handler_refuses() { + var (handler, committed) = Build(); + + await handler.Commit(Position(7, sequence: 0), CancellationToken.None); + (await Wait.Until(() => committed().Contains(7UL), TimeSpan.FromSeconds(5))).ShouldBeTrue(); + + await handler.DisposeAsync(); + + var accepted = await handler.Commit(Position(8, sequence: 1), CancellationToken.None); + + accepted.ShouldBeFalse("a commit into a stopped handler was reported as accepted"); + + committed().ShouldNotContain(8UL); + } + + /// + /// A commit parked on backpressure when disposal completes the channel must come back refused — its + /// position was never queued, so reporting success would drop it from the checkpoint. + /// + /// + /// Disposal itself still blocks here, retrying its final checkpoint flush without a token by design, so + /// this asserts on the parked commit's result, not on disposal finishing. + /// + [Test] + [Timeout(60_000)] + public async Task A_commit_parked_at_disposal_is_refused(CancellationToken cancellationToken) { + var inStore = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // Blocking the store blocks the worker, filling the channel and parking a commit inside the handler. + var handler = new CheckpointCommitHandler( + "dispose-refuses", + async (checkpoint, _, ct) => { + inStore.TrySetResult(); + await release.Task.WaitAsync(ct); + + return checkpoint; + }, + TimeSpan.FromMilliseconds(10), + batchSize: 1 + ); + + Task? disposing = null; + + try { + await handler.Commit(Position(0, sequence: 0), cancellationToken); + await inStore.Task.WaitAsync(TimeSpan.FromSeconds(5), cancellationToken); + + for (var sequence = 1UL; sequence <= 1000; sequence++) await handler.Commit(Position(sequence, sequence), cancellationToken); + + var parked = handler.Commit(Position(1001, sequence: 1001), cancellationToken).AsTask(); + await Task.Delay(200, cancellationToken); + parked.IsCompleted.ShouldBeFalse("the commit should be parked inside the handler"); + + disposing = handler.DisposeAsync().AsTask(); + + (await parked.WaitAsync(TimeSpan.FromSeconds(10), cancellationToken)) + .ShouldBeFalse("a commit released by the channel closing never queued its position, and has to say so"); + } finally { + release.TrySetResult(); + + if (disposing != null) await disposing.WaitAsync(TimeSpan.FromSeconds(30), CancellationToken.None); + } + } + + /// + /// Acknowledgements reach the handler from any thread, unserialised; a lost one leaves a gap it never + /// commits past. + /// + [Test] + public async Task Concurrent_commits_all_land() { + var (handler, committed) = Build(); + + var accepted = await Task.WhenAll( + Enumerable.Range(0, 200) + .Select(i => Task.Run(async () => await handler.Commit(Position((ulong)i, (ulong)i), CancellationToken.None))) + ); + + accepted.ShouldAllBe(x => x, "an open handler refused an acknowledgement"); + + // Commits up to the first gap, so seeing the last position means every one before it arrived too. + var arrived = await Wait.Until(() => committed().Contains(199UL), TimeSpan.FromSeconds(5)); + + arrived.ShouldBeTrue($"the handler lost an acknowledgement; it committed up to {committed().LastOrDefault()}"); + + await handler.DisposeAsync(); + } + + /// + /// Returns a snapshot delegate, not the live list — the commit worker appends from its own thread. + /// + static (CheckpointCommitHandler Handler, Func> Committed) Build() { + var committed = new List(); + + var handler = new CheckpointCommitHandler( + "commit-handler-lifecycle-tests", + (checkpoint, _, _) => { + lock (committed) committed.Add(checkpoint.Position!.Value); + + return new(checkpoint); + }, + TimeSpan.FromMilliseconds(10), + batchSize: 1 + ); + + return (handler, Snapshot); + + List Snapshot() { + lock (committed) return [..committed]; + } + } + + static CommitPosition Position(ulong position, ulong sequence) => new(position, sequence, DateTime.UtcNow); + +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs index 775cc267f..494197ef8 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs @@ -100,9 +100,7 @@ record TestOptions : SubscriptionOptions; class TestSub(TestOptions options, ConsumePipe consumePipe) : EventSubscription(options, consumePipe, NullLoggerFactory.Instance, null) { - protected override ValueTask Subscribe(CancellationToken cancellationToken) => default; - - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => default; + protected override ValueTask Connect(SubscriptionRun run) => default; } public class TestDependency { diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/RegistrationTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/RegistrationTests.cs index bcab8703d..6195d54ec 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/RegistrationTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/RegistrationTests.cs @@ -154,9 +154,7 @@ record TestOptions : SubscriptionOptions { class TestSub(TestOptions options, ConsumePipe consumePipe) : EventSubscription(options, consumePipe, NullLoggerFactory.Instance, null), IMeasuredSubscription { - protected override ValueTask Subscribe(CancellationToken cancellationToken) => default; - - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => default; + protected override ValueTask Connect(SubscriptionRun run) => default; public GetSubscriptionEndOfStream GetMeasure() => _ => new(new EndOfStream(SubscriptionId, 0, DateTime.UtcNow)); } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs index b8ece0979..1c4fe41bf 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs @@ -4,25 +4,21 @@ using Eventuous.Subscriptions.Context; using Eventuous.Subscriptions.Filters; using Eventuous.Tools; -using Microsoft.Extensions.Logging; using Shouldly; namespace Eventuous.Tests.Subscriptions; /// -/// One failure has to cost one resubscribe. A transport drop reaches Dropped once for the pump's read -/// and once for every in-flight message on the same dead connection; a handler deferring by throwing -/// under ThrowOnError produces the same burst. Either way: one drop cycle, one live transport, no -/// accumulation, and a checkpoint that keeps moving afterwards. +/// One failure — a transport drop or a burst of handler nacks — must cost exactly one resubscribe, +/// with no accumulation and a checkpoint that keeps moving. /// public class ResubscribeConcurrencyTests { /// - /// A whole page of messages fails while the pump keeps delivering, so Dropped is called once per - /// failing message, all inside one drop window. + /// A whole page of messages fails at once, so Dropped is called once per message, all in one drop window. /// [Test] public async Task Burst_of_nacks_produces_a_single_resubscribe(CancellationToken ct) { - var logs = new RecordingLoggerFactory(); + var logs = new CapturingLoggerFactory(LogLevel.Trace); const int messageCount = 8; @@ -39,25 +35,23 @@ public async Task Burst_of_nacks_produces_a_single_resubscribe(CancellationToken new ConsumePipe().AddDefaultConsumer(handler), logs, concurrencyLimit: 4, - // Only the first run delivers: a replacement redelivering the same failing messages would - // legitimately open a second drop window. - pump: async (sub, transport, start, token) => { + // Only the first run delivers — a replacement redelivering the same messages would open a second drop window. + pump: async (sub, transport, start, run) => { if (transport.Index == 0) { - for (var i = 0; i < messageCount; i++) await sub.Deliver(start + (ulong)i, token).NoContext(); + for (var i = 0; i < messageCount; i++) await sub.Deliver(run, start + (ulong)i).NoContext(); } - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(500) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - // All of them have to fail before the resubscribe fires, or this proves nothing about - // concurrent drops. - (await WaitUntil(() => handler.HandledCount >= messageCount, TimeSpan.FromSeconds(5))) + // All of them have to fail before the resubscribe fires, or this proves nothing about concurrent drops. + (await Wait.Until(() => handler.HandledCount >= messageCount, TimeSpan.FromSeconds(5))) .ShouldBeTrue($"all {messageCount} messages should have been handled and nacked, got {handler.HandledCount}"); - (await WaitUntil(() => subscription.SubscribeCalls > 1, TimeSpan.FromSeconds(5))).ShouldBeTrue("the subscription should have resubscribed"); + (await Wait.Until(() => subscription.SubscribeCalls > 1, TimeSpan.FromSeconds(5))).ShouldBeTrue("the subscription should have resubscribed"); // Give any extra resubscribes scheduled by the other nacks time to show up before asserting. await Task.Delay(TimeSpan.FromSeconds(1), ct); @@ -78,7 +72,7 @@ public async Task Transport_failure_produces_a_single_resubscribe(CancellationTo const int inFlight = 4; const ulong last = 7; - var logs = new RecordingLoggerFactory(); + var logs = new CapturingLoggerFactory(LogLevel.Trace); var store = new NoOpCheckpointStore(); var handler = new ConnectionBoundHandler(t => t == 0); @@ -93,35 +87,33 @@ public async Task Transport_failure_produces_a_single_resubscribe(CancellationTo new ConsumePipe().AddDefaultConsumer(handler), logs, concurrencyLimit: inFlight, - pump: async (sub, transport, start, token) => { + pump: async (sub, transport, start, run) => { if (transport.Index > 0) { - // The store is back. Nothing about this run is unusual. - for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + for (var i = start; i <= last && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); return; } - for (var i = 0; i < inFlight; i++) await sub.Deliver((ulong)i, token, transport.Index).NoContext(); + for (var i = 0; i < inFlight; i++) await sub.Deliver(run, (ulong)i, transport.Index).NoContext(); // Wait until they're in flight, so the failure hits all of them at once. - await WaitUntil(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); + await Wait.Until(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); // HTTP/2 INTERNAL_ERROR: the connection is gone, and everything using it fails together. handler.KillConnection(transport.Index); // What AllStreamSubscription.PumpMessages does when MoveNextAsync throws. - sub.Drop(new IOException("The HTTP/2 server closed the connection")); + run.Fail(DropReason.SubscriptionError, new IOException("The HTTP/2 server closed the connection")); - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(300) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - // (c) The checkpoint has to keep moving afterwards. - var recovered = await WaitUntil( + var recovered = await Wait.Until( async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, TimeSpan.FromSeconds(15) ); @@ -132,12 +124,10 @@ public async Task Transport_failure_produces_a_single_resubscribe(CancellationTo recovered.ShouldBeTrue("the subscription must commit again after the connection comes back"); - // (a) One transport failure, one drop cycle, one resubscribe. subscription.SubscribeCalls.ShouldBe(2, "one initial subscribe plus exactly one resubscribe for the whole connection failure"); logs.Count("Resubscribing").ShouldBe(1, "the pump's drop and the in-flight nacks are one drop cycle between them"); logs.Count("Dropped:").ShouldBe(1, "the drop is reported once per cycle, not once per failed operation"); - // (b) The run that owned the dead connection is gone. var transports = subscription.Transports.ToArray(); transports[0].IsDisposed.ShouldBeTrue("the dead connection should have been disposed"); transports[0].PumpExited.ShouldBeTrue("the pump reading the dead connection should have exited"); @@ -150,7 +140,7 @@ public async Task Transport_failure_produces_a_single_resubscribe(CancellationTo /// [Test] public async Task Repeated_transport_failures_do_not_accumulate(CancellationToken ct) { - const int cycles = 25; + const int cycles = 15; const int inFlight = 4; const ulong last = 7; @@ -166,31 +156,32 @@ public async Task Repeated_transport_failures_do_not_accumulate(CancellationToke }, store, new ConsumePipe().AddDefaultConsumer(handler), - new RecordingLoggerFactory(), + new CapturingLoggerFactory(LogLevel.Trace), concurrencyLimit: inFlight, - pump: async (sub, transport, start, token) => { + pump: async (sub, transport, start, run) => { if (transport.Index >= cycles) { // The store settles down and the subscription catches up. - for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + for (var i = start; i <= last && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); return; } - for (var i = start; i < start + inFlight && !token.IsCancellationRequested; i++) await sub.Deliver(i, token, transport.Index).NoContext(); + for (var i = start; i < start + inFlight && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); - await WaitUntil(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); + await Wait.Until(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); handler.KillConnection(transport.Index); - sub.Drop(new IOException("The HTTP/2 server closed the connection")); - await Task.Delay(Timeout.Infinite, token).NoContext(); + run.Fail(DropReason.SubscriptionError, new IOException("The HTTP/2 server closed the connection")); + + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(10) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - var recovered = await WaitUntil( + var recovered = await Wait.Until( async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, TimeSpan.FromSeconds(60) ); @@ -221,28 +212,31 @@ public async Task Resubscribe_stops_the_previous_transport_and_pump(Cancellation }, new NoOpCheckpointStore(), new ConsumePipe().AddDefaultConsumer(new CountingHandler()), - new RecordingLoggerFactory(), + new CapturingLoggerFactory(LogLevel.Trace), concurrencyLimit: 1, - pump: async (sub, transport, start, token) => { + pump: async (sub, transport, start, run) => { var position = start; - while (!token.IsCancellationRequested) { + while (!run.Token.IsCancellationRequested) { deliveries.Enqueue((transport.Index, position)); - await sub.Deliver(position++, token).NoContext(); - await Task.Delay(5, token).NoContext(); + await sub.Deliver(run, position++).NoContext(); + await Task.Delay(5, run.Token).NoContext(); } } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(100) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - (await WaitUntil(() => deliveries.Count > 3, TimeSpan.FromSeconds(5))).ShouldBeTrue("the first pump should be delivering"); + (await Wait.Until(() => deliveries.Count > 3, TimeSpan.FromSeconds(5))).ShouldBeTrue("the first pump should be delivering"); subscription.Drop(new InvalidOperationException("Simulated transport drop")); - (await WaitUntil(() => deliveries.Any(d => d.Transport == 1), TimeSpan.FromSeconds(5))).ShouldBeTrue("the replacement pump should be delivering"); + (await Wait.Until(() => deliveries.Any(d => d.Transport == 1), TimeSpan.FromSeconds(5))).ShouldBeTrue("the replacement pump should be delivering"); + + // Let the replacement get well clear of the switch, so the old pump has every chance to interleave. + (await Wait.Until(() => deliveries.Count(d => d.Transport == 1) >= 3, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the replacement pump should have kept delivering"); - await Task.Delay(TimeSpan.FromMilliseconds(300), ct); await subscription.Unsubscribe(_ => { }, ct); var transports = subscription.Transports.ToArray(); @@ -261,11 +255,127 @@ public async Task Resubscribe_stops_the_previous_transport_and_pump(Cancellation } /// - /// Two contexts sharing a sequence number collapse into one entry in the commit handler's position - /// set, which then refuses to commit past the resulting hole. + /// A transport that can't reconnect — the broker is still down — must keep being retried, not give up. + /// + [Test] + public async Task A_resubscribe_that_fails_to_connect_keeps_retrying(CancellationToken ct) { + var logs = new CapturingLoggerFactory(LogLevel.Trace); + + const int failUntil = 4; + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "resubscribe-retries", + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new CountingHandler()), + logs, + concurrencyLimit: 1, + pump: (_, _, _, run) => Task.Delay(Timeout.Infinite, run.Token) + ) { + ResubscribeDelay = TimeSpan.FromMilliseconds(50), + // Attempt 0 is the original run coming up; the broker stays down through attempt failUntil. + FailStart = attempt => attempt is > 0 and < failUntil ? new InvalidOperationException($"Connection refused on attempt {attempt}") : null + }; + + var transitions = new Transitions(); + + await subscription.Subscribe(_ => transitions.Subscribed(), (_, _, _) => transitions.Dropped(), ct); + + subscription.Drop(new InvalidOperationException("Simulated transport drop")); + + // Wait on the attempt count, not a transition: a loop that gives up leaves no cycle open either. + var retried = await Wait.Until(() => subscription.SubscribeCalls > failUntil, TimeSpan.FromSeconds(15)); + + (await Wait.Until(() => transitions.Up, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the subscription should have come back up once the broker stopped refusing"); + + // Ten retry delays: long enough that a loop still running would have spent another attempt, which the + // exact count below would catch. + await Task.Delay(TimeSpan.FromMilliseconds(500), ct); + + var attempts = subscription.SubscribeCalls; + var up = transitions.Up; + + await subscription.Unsubscribe(_ => { }, ct); + + retried.ShouldBeTrue($"the subscription stopped retrying after {attempts} attempts and never came back up"); + attempts.ShouldBe(failUntil + 1, "the loop kept resubscribing after the replacement run was up"); + up.ShouldBeTrue("the drop cycle outlived the run that recovered from it"); + } + + /// + /// A drop landing while the replacement run is still coming up must not cost an extra attempt of its own. + /// + /// + /// Not a retry-delay test: the supervisor is inside Connect for the whole hold, so the delay is + /// irrelevant here. That's covered by SupervisorTests.The_retry_delay_elapses_between_connect_attempts. + /// + [Test] + public async Task Drop_while_a_resubscribe_is_in_flight_does_not_spin(CancellationToken ct) { + var logs = new CapturingLoggerFactory(LogLevel.Trace); + + var reached = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var starting = 0; + + var subscription = new PumpingSubscription( + new() { + SubscriptionId = "drop-during-resubscribe", + CheckpointCommitBatchSize = 1, + CheckpointCommitDelayMs = 10 + }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new CountingHandler()), + logs, + concurrencyLimit: 1, + pump: (_, _, _, run) => Task.Delay(Timeout.Infinite, run.Token) + ) { + ResubscribeDelay = TimeSpan.FromMilliseconds(50), + // Only the replacement run is held; the first has to come up for there to be one to drop. + WhileStarting = async () => { + if (Interlocked.Increment(ref starting) != 2) return; + + reached.TrySetResult(); + await release.Task; + } + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + subscription.Drop(new InvalidOperationException("Simulated transport drop")); + + (await Wait.Until(() => reached.Task.IsCompleted, TimeSpan.FromSeconds(5))).ShouldBeTrue("the resubscribe should have reached the replacement run"); + + // Opens a second drop cycle while the first resubscribe still holds the lifecycle. + subscription.Drop(new InvalidOperationException("Simulated drop on the replacement")); + + // The supervisor is parked inside Connect, so no further attempt can start however many drops land. + logs.Count("Resubscribing").ShouldBe(1, "a drop landing mid-connect must not start an attempt alongside the one in flight"); + + release.SetResult(); + + // The held attempt finishes, and only then is the queued drop handled — one further cycle, not a spin. + (await Wait.Until(() => Volatile.Read(ref starting) >= 3, TimeSpan.FromSeconds(10))) + .ShouldBeTrue($"the drop taken during the hold should have been served after it, only saw {Volatile.Read(ref starting)} connect(s)"); + + await subscription.Unsubscribe(_ => { }, ct); + + logs.Count("Resubscribing").ShouldBe(2, "the drop that landed mid-connect costs exactly one further cycle; spinning produces thousands"); + } + + /// + /// Two contexts sharing a sequence number collapse into one entry in the commit handler's position set, + /// which then refuses to commit past the resulting hole. /// + /// + /// A contract guard, not a race detector: the counter is an unconditional Interlocked.Increment, so this + /// can't find a bad interleaving — non-atomicity would have to be caught by review, not by running this. + /// [Test] - public async Task Concurrent_context_creation_yields_unique_sequences() { + public async Task Concurrent_context_creation_yields_unique_sequences(CancellationToken ct) { const int threads = 8; const int perThread = 2000; @@ -273,11 +383,15 @@ public async Task Concurrent_context_creation_yields_unique_sequences() { new() { SubscriptionId = "unique-sequences" }, new NoOpCheckpointStore(), new ConsumePipe().AddDefaultConsumer(new CountingHandler()), - new RecordingLoggerFactory(), + new CapturingLoggerFactory(LogLevel.Trace), concurrencyLimit: 1, - pump: (_, _, _, token) => Task.Delay(Timeout.Infinite, token) + pump: (_, _, _, run) => Task.Delay(Timeout.Infinite, run.Token) ); + // The counter belongs to the run, so there has to be one to draw from. + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + var run = subscription.Run.ShouldNotBeNull(); var results = new ulong[threads][]; await Task.WhenAll( @@ -285,7 +399,7 @@ await Task.WhenAll( .Select(t => Task.Run(() => { var mine = new ulong[perThread]; - for (var i = 0; i < perThread; i++) mine[i] = subscription.CreateContext(0).Sequence; + for (var i = 0; i < perThread; i++) mine[i] = run.NextSequence(); results[t] = mine; } @@ -293,6 +407,8 @@ await Task.WhenAll( ) ); + await subscription.Unsubscribe(_ => { }, ct); + var all = results.SelectMany(x => x).ToArray(); all.Length.ShouldBe(threads * perThread); @@ -304,12 +420,14 @@ await Task.WhenAll( } /// - /// The steady state of a deferring handler: the same message fails on every redelivery, the - /// checkpoint holds at the last one before it, and nothing accumulates while that goes on. + /// The steady state of a deferring handler: the same message fails on every redelivery, the checkpoint + /// holds just before it, and nothing accumulates. /// [Test] public async Task Repeated_nacks_hold_a_stable_checkpoint_without_accumulating(CancellationToken ct) { - const int cycles = 100; + // Enough cycles that anything per-cycle would be plainly visible in the bounds below; a hundred cost + // minutes of CI and proved nothing thirty don't. + const int cycles = 30; const ulong failAt = 3; var stored = new ConcurrentQueue(); @@ -327,44 +445,41 @@ public async Task Repeated_nacks_hold_a_stable_checkpoint_without_accumulating(C }, store, new ConsumePipe().AddDefaultConsumer(handler), - new RecordingLoggerFactory(), + new CapturingLoggerFactory(LogLevel.Trace), concurrencyLimit: 1, - pump: async (sub, _, start, token) => { - for (var i = start; i < start + 5 && !token.IsCancellationRequested; i++) await sub.Deliver(i, token).NoContext(); + pump: async (sub, _, start, run) => { + for (var i = start; i < start + 5 && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i).NoContext(); - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(10) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - (await WaitUntil(() => subscription.SubscribeCalls > cycles, TimeSpan.FromSeconds(60))) + (await Wait.Until(() => subscription.SubscribeCalls > cycles, TimeSpan.FromSeconds(60))) .ShouldBeTrue($"the subscription should have gone through {cycles} drop cycles, it did {subscription.SubscribeCalls - 1}"); await subscription.Unsubscribe(_ => { }, ct); - // The failing message is never acked, so the checkpoint parks on the one before it. A corrupted - // sequence shows up as a checkpoint that never commits, or one that jumps past the failure. + // A corrupted sequence would show up as a checkpoint that never commits, or one that jumps past the failure. stored.ShouldNotBeEmpty("the messages before the failing one should have been committed"); stored.ShouldAllBe(p => p < failAt, "the checkpoint must never advance past the message that keeps failing"); stored.Last().ShouldBe(failAt - 1, "the checkpoint should settle on the last message before the failing one"); (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position.ShouldBe(failAt - 1); - // Only the messages ahead of the failure, plus the flush when the first commit handler is - // replaced. More than that means the checkpoint is being rewritten every cycle. + // Plus one for the flush when the first commit handler is replaced. stored.Count.ShouldBeLessThanOrEqualTo((int)failAt + 1, $"the checkpoint should settle, not be rewritten across {cycles} cycles"); handler.HandledCount.ShouldBeGreaterThanOrEqualTo(cycles, "the failing message must be redelivered on every cycle"); - // Nothing accumulates: one transport and one pump at a time, no matter how many cycles. subscription.MaxLivePumps.ShouldBe(1, "message pumps accumulated across drop cycles"); subscription.Transports.Count(t => !t.IsDisposed).ShouldBe(0, "every transport should have been disposed by the time the subscription stops"); subscription.Transports.Count.ShouldBe(subscription.SubscribeCalls, "each subscribe should create exactly one transport"); } /// - /// Nothing commits past a gap in the sequence, so a subscription whose checkpoint stalled needs a - /// clean commit handler from the next subscribe — or it processes forever without ever committing. + /// A subscription whose checkpoint stalled on a gap needs a clean commit handler from the next + /// subscribe, or it processes forever without ever committing. /// [Test] public async Task Subscription_commits_again_after_a_commit_gap(CancellationToken ct) { @@ -386,19 +501,19 @@ public async Task Subscription_commits_again_after_a_commit_gap(CancellationToke }, store, new ConsumePipe().AddDefaultConsumer(handler), - new RecordingLoggerFactory(), + new CapturingLoggerFactory(LogLevel.Trace), concurrencyLimit: 1, - pump: async (sub, _, start, token) => { - for (var i = start; i <= last && !token.IsCancellationRequested; i++) await sub.Deliver(i, token).NoContext(); + pump: async (sub, _, start, run) => { + for (var i = start; i <= last && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i).NoContext(); - await Task.Delay(Timeout.Infinite, token).NoContext(); + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); } ) { ResubscribeDelay = TimeSpan.FromMilliseconds(50) }; await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); // The messages after the failing one ack out of order, leaving a hole nothing commits past. - var stalled = await WaitUntil( + var stalled = await Wait.Until( async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == failAt - 1, TimeSpan.FromSeconds(10) ); @@ -408,7 +523,7 @@ public async Task Subscription_commits_again_after_a_commit_gap(CancellationToke // The precondition resolves. Nothing else changes — no restart, no manual intervention. defer = false; - var recovered = await WaitUntil( + var recovered = await Wait.Until( async () => (await store.GetLastCheckpoint(subscription.SubscriptionId, ct)).Position == last, TimeSpan.FromSeconds(15) ); @@ -418,27 +533,122 @@ public async Task Subscription_commits_again_after_a_commit_gap(CancellationToke recovered.ShouldBeTrue("a subscription whose checkpoint stalled must commit again once the failing message succeeds"); } - static Task WaitUntil(Func condition, TimeSpan timeout) => WaitUntil(() => Task.FromResult(condition()), timeout); - static async Task WaitUntil(Func> condition, TimeSpan timeout) { - var deadline = DateTime.UtcNow + timeout; + /// + /// An acknowledgement must commit through the run that dispatched the message, never through whichever + /// run is current — otherwise it can collide with or paper over a hole in a different run's sequence. + /// + /// + /// Not a contrived window: teardown only joins the transport pump, so a message still inside a handler + /// when its run ends is the ordinary case on every resubscribe. + /// + [Test] + public async Task An_acknowledgement_from_a_dropped_run_is_refused(CancellationToken ct) { + var logs = new CapturingLoggerFactory(LogLevel.Trace); + var handler = new ParkingHandler(); - while (DateTime.UtcNow < deadline) { - if (await condition()) return true; + var subscription = new PumpingSubscription( + new() { SubscriptionId = "ack-belongs-to-its-run", CheckpointCommitBatchSize = 1, CheckpointCommitDelayMs = 10 }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(handler), + logs, + concurrencyLimit: 1, + pump: async (sub, transport, _, run) => { + // Only the first run delivers; the replacement just holds its connection open. + if (transport.Index == 0) await sub.Deliver(run, 0).NoContext(); - await Task.Delay(20); + await run.Ended.NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(50) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + try { + (await handler.Parked.WaitAsync(TimeSpan.FromSeconds(5), ct).ContinueWith(t => t.IsCompletedSuccessfully, ct)) + .ShouldBeTrue("the handler should be holding the first run's message"); + + subscription.Drop(new IOException("the connection died while a handler was mid-flight")); + + (await Wait.Until(() => subscription.SubscribeCalls >= 2, TimeSpan.FromSeconds(10))) + .ShouldBeTrue("the replacement run should have come up while the handler was still parked"); + + handler.Release(); + + (await Wait.Until(() => logs.Count("belongs to a previous run") >= 1, TimeSpan.FromSeconds(10))) + .ShouldBeTrue("the late acknowledgement should have been refused by the run that dispatched it"); + } finally { + handler.Release(); + + // The test's own token, not None — an unbounded wait in a finally turns a failing test into a hanging one. + await subscription.Unsubscribe(_ => { }, ct); } + } + + /// + /// The positive counterpart to the test above: an acknowledgement that lands while its own run is being + /// torn down must still commit. That is what the release ordering buys — the commit handler is registered + /// before Connect, so it releases after every transport handle, and a handler still mid-flight when + /// teardown starts gets its checkpoint written rather than dropped. + /// + /// + /// Reversing that order turns this into silent checkpoint loss: the ack is refused, the subscription + /// still stops cleanly, and the work is replayed on the next start with nothing logged as an error. + /// + [Test] + [Timeout(30_000)] + public async Task An_acknowledgement_in_flight_during_teardown_still_commits(CancellationToken ct) { + var logs = new CapturingLoggerFactory(LogLevel.Trace); + var handler = new ParkingHandler(); + var store = new NoOpCheckpointStore(); - return await condition(); + var stored = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + store.CheckpointStored += (_, checkpoint) => stored.TrySetResult(checkpoint.Position); + + var subscription = new PumpingSubscription( + new() { SubscriptionId = "ack-during-teardown", CheckpointCommitBatchSize = 1, CheckpointCommitDelayMs = 10 }, + store, + new ConsumePipe().AddDefaultConsumer(handler), + logs, + concurrencyLimit: 1, + pump: async (sub, _, _, run) => { + await sub.Deliver(run, 0).NoContext(); + await run.Ended.NoContext(); + } + ) { + ResubscribeDelay = TimeSpan.FromMilliseconds(50), + // Teardown has started and the transport is going away, but the commit handler is still open. + WhileStopping = async () => { + handler.Release(); + + // Bounded, so a refused ack fails the assertion below instead of hanging teardown forever. + await Task.WhenAny(stored.Task, Task.Delay(TimeSpan.FromSeconds(10), CancellationToken.None)); + } + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + try { + await handler.Parked.WaitAsync(TimeSpan.FromSeconds(10), ct); + + // A clean stop is the same teardown a resubscribe runs, so this covers both. + await subscription.Unsubscribe(_ => { }, ct); + } finally { + handler.Release(); + } + + stored.Task.IsCompletedSuccessfully.ShouldBeTrue("an ack raised while the run was tearing down must reach the commit handler that dispatched it"); + (await stored.Task).ShouldBe(0ul, "the position the parked handler acknowledged is the one that must be durable"); + + logs.Count("belongs to a previous run").ShouldBe(0, "the run that dispatched the message was still the one acknowledging it"); } + const string TransportKey = "transport"; record TestOptions : SubscriptionWithCheckpointOptions; /// - /// Stands in for a transport connection, recording only whether the subscription got rid of it and - /// whether the pump reading it has finished. + /// Stands in for a transport connection, tracking disposal and whether its pump has exited. /// sealed class FakeTransport(int index) : IAsyncDisposable { public int Index { get; } = index; @@ -453,35 +663,67 @@ public ValueTask DisposeAsync() { } /// - /// Shaped like the real catch-up subscriptions: Subscribe creates a transport and hands it to a pump - /// on its own task, Unsubscribe drops the transport and waits for the pump. Pump body per test. + /// Shaped like the real catch-up subscriptions: Connect creates a transport, a pump reads it, Disconnect + /// drops it. Pump body is supplied per test. /// sealed class PumpingSubscription( - TestOptions options, - ICheckpointStore checkpointStore, - ConsumePipe pipe, - ILoggerFactory? loggerFactory, - int concurrencyLimit, - Func pump + TestOptions options, + ICheckpointStore checkpointStore, + ConsumePipe pipe, + ILoggerFactory? loggerFactory, + int concurrencyLimit, + Func pump ) : EventSubscriptionWithCheckpoint(options, checkpointStore, pipe, concurrencyLimit, SubscriptionKind.All, loggerFactory, null, null) { readonly ConcurrentQueue _transports = []; - FakeTransport? _transport; - Task? _pumpTask; - int _subscribeCalls; - int _livePumps; - int _maxLivePumps; + SubscriptionRun? _run; + int _subscribeCalls; + int _livePumps; + int _maxLivePumps; public IReadOnlyCollection Transports => _transports; public int SubscribeCalls => Volatile.Read(ref _subscribeCalls); public int MaxLivePumps => Volatile.Read(ref _maxLivePumps); - public TimeSpan ResubscribeDelay { get; init; } = TimeSpan.FromMilliseconds(100); - - public void Drop(Exception exception) => Dropped(DropReason.SubscriptionError, exception); + /// + /// The newest run this subscription was given, for drawing sequence numbers or failing from outside the loop. + /// + public SubscriptionRun? Run => Volatile.Read(ref _run); + + /// + /// Writes through to the option the supervisor reads. The 2s default would put a hundred cycles at + /// three minutes against a 60s budget, so every test here sets its own. + /// + public TimeSpan ResubscribeDelay { + init => Options.RetryDelay = value; + } - public MessageConsumeContext CreateContext(ulong position, int transport = 0) + /// + /// Awaited part-way through bringing a run up, so a test can hold a restart open and see what a + /// drop arriving in that window costs. + /// + public Func? WhileStarting { get; init; } + + /// + /// Awaited inside the transport's own release, so a test can act while teardown is underway but the + /// commit handler — which registers first and so releases last — is still open. That window is where + /// a late acknowledgement has to land. + /// + public Func? WhileStopping { get; init; } + + /// + /// Consulted with the attempt number before a run is brought up, so a test can make a transport + /// refuse to connect and leave recovery entirely to the resubscribe loop. + /// + public Func? FailStart { get; init; } + + /// + /// Fails the current run, standing in for a transport reporting from outside the loop. + /// + public void Drop(Exception exception) => Run?.Fail(DropReason.SubscriptionError, exception); + + public MessageConsumeContext CreateContext(SubscriptionRun run, ulong position, int transport = 0) => new MessageConsumeContext( Guid.NewGuid().ToString(), "TestEvent", @@ -490,7 +732,7 @@ public MessageConsumeContext CreateContext(ulong position, int transport = 0) position, position, position, - Sequence++, + run.NextSequence(), DateTime.UtcNow, new { Position = position }, new(), @@ -499,51 +741,55 @@ public MessageConsumeContext CreateContext(ulong position, int transport = 0) ) { LogContext = Log } .WithItem(TransportKey, transport); - public ValueTask Deliver(ulong position, CancellationToken cancellationToken, int transport = 0) { - var context = CreateContext(position, transport); - context.CancellationToken = cancellationToken; + public ValueTask Deliver(SubscriptionRun run, ulong position, int transport = 0) { + var context = CreateContext(run, position, transport); + context.CancellationToken = run.Token; - return HandleInternal(context); + return HandleInternal(run, context); } - // The production delays are 2s and 10s; a hundred cycles of those would take half an hour. - protected override Task Resubscribe(TimeSpan delay, CancellationToken cancellationToken) => base.Resubscribe(ResubscribeDelay, cancellationToken); + protected override async ValueTask Connect(SubscriptionRun run) { + // First, so a drop arriving mid-connect finds the run it belongs to (see WhileStarting tests). + Volatile.Write(ref _run, run); - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { - var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); - var start = position == null ? 0 : position.Value + 1; + var (_, position) = await GetCheckpoint(run).NoContext(); + var start = position == null ? 0 : position.Value + 1; - var transport = new FakeTransport(Interlocked.Increment(ref _subscribeCalls) - 1); + var attempt = Interlocked.Increment(ref _subscribeCalls) - 1; + + if (FailStart?.Invoke(attempt) is { } failure) throw failure; + + var transport = new FakeTransport(attempt); _transports.Enqueue(transport); - _transport = transport; - - _pumpTask = Task.Run( - async () => { - TrackPumpStarted(); - - try { await pump(this, transport, start, cancellationToken).NoContext(); } catch (Exception) { - // The pump ending is what the tests observe, not how it ended - } finally { - Interlocked.Decrement(ref _livePumps); - transport.PumpExited = true; - } - }, - CancellationToken.None - ); - } - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - var transport = _transport; - var pump = _pumpTask; - _transport = null; - _pumpTask = null; + if (WhileStarting != null) await WhileStarting().NoContext(); - // As in the real subscriptions: cancelling a source shutdown already disposed is benign. - try { Stopping.Cancel(false); } catch (ObjectDisposedException) { } + // Started on a task of its own so it never runs inline on the supervisor's stack during Connect. + var pumping = Task.Run(() => Pump(run, transport, start), CancellationToken.None); - if (transport != null) await transport.DisposeAsync().NoContext(); + // Dispose then join, same order teardown always used, just one release instead of two steps. + run.OnDisconnect(async _ => { + if (WhileStopping != null) await WhileStopping().NoContext(); - if (pump != null) await Task.WhenAny(pump, Task.Delay(TimeSpan.FromSeconds(5), cancellationToken)).NoContext(); + await transport.DisposeAsync().NoContext(); + await pumping.NoContext(); + }); + } + + /// + /// Runs on a task of its own, so is a real measurement of overlap. Reports + /// its own death — a plain return or exception while is still live is a drop + /// nothing else would otherwise notice. + /// + async Task Pump(SubscriptionRun run, FakeTransport transport, ulong start) { + TrackPumpStarted(); + + try { + await TransportPump.Run(run, () => pump(this, transport, start, run), "PumpingSubscription pump ended while the connection was up").NoContext(); + } finally { + Interlocked.Decrement(ref _livePumps); + transport.PumpExited = true; + } } void TrackPumpStarted() { @@ -560,8 +806,7 @@ void TrackPumpStarted() { } /// - /// Defers by throwing when a precondition isn't met, so the message isn't acknowledged and gets - /// redelivered after the resubscribe. + /// Defers by throwing when a precondition isn't met, so the message is redelivered after the resubscribe. /// sealed class DeferringHandler(Func shouldDefer) : BaseEventHandler { int _handled; @@ -578,8 +823,8 @@ public override ValueTask HandleEvent(IMessageConsumeContex } /// - /// Does I/O over the same connection the subscription reads from, so it fails the moment that - /// connection dies — which is how one transport failure becomes a batch of simultaneous nacks. + /// Does I/O over the subscription's own connection, so a connection death turns into a batch of + /// simultaneous nacks. /// sealed class ConnectionBoundHandler(Func holdsUntilConnectionDies) : BaseEventHandler { readonly ConcurrentDictionary _dead = []; @@ -615,24 +860,30 @@ sealed class CountingHandler : BaseEventHandler { public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); } - sealed class RecordingLoggerFactory : ILoggerFactory { - readonly ConcurrentQueue _messages = []; + /// + /// Holds the first message until released, so its acknowledgement lands after the dispatching run is + /// already torn down. + /// + sealed class ParkingHandler : BaseEventHandler { + readonly TaskCompletionSource _parked = new(TaskCreationOptions.RunContinuationsAsynchronously); + readonly TaskCompletionSource _release = new(TaskCreationOptions.RunContinuationsAsynchronously); - public int Count(string contains) => _messages.Count(m => m.Contains(contains)); + int _parkedOnce; - public ILogger CreateLogger(string categoryName) => new RecordingLogger(_messages); + public Task Parked => _parked.Task; - public void AddProvider(ILoggerProvider provider) { } + public void Release() => _release.TrySetResult(); - public void Dispose() { } + public override async ValueTask HandleEvent(IMessageConsumeContext context) { + if (Interlocked.CompareExchange(ref _parkedOnce, 1, 0) != 0) return EventHandlingStatus.Success; - sealed class RecordingLogger(ConcurrentQueue messages) : ILogger { - public IDisposable? BeginScope(TState state) where TState : notnull => null; + _parked.TrySetResult(); - public bool IsEnabled(LogLevel logLevel) => true; + // Deliberately not watching the context token, or this would abandon the message instead of acking it late. + await _release.Task; - public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) - => messages.Enqueue(formatter(state, exception)); + return EventHandlingStatus.Success; } } + } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeOnHandlerFailureTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeOnHandlerFailureTests.cs index fb7b27e7b..5539cc0e5 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeOnHandlerFailureTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeOnHandlerFailureTests.cs @@ -55,17 +55,16 @@ await subscription.Subscribe( // Assert if (completedTask == droppedTcs.Task) { + // Reaching the drop callback at all is the assertion: it is the only report of a drop there is. var (id, _, _) = await droppedTcs.Task; id.ShouldBe("test-handler-failure"); - // Subscription should have been dropped due to error - subscription.IsDropped.ShouldBeTrue("Subscription should be marked as dropped after handler failure"); } else { var handledCount = handler.HandledCount; Assert.Fail( $"Dropped was never called. Handler processed {handledCount} events before failure. " + - $"IsRunning={subscription.IsRunning}, IsDropped={subscription.IsDropped}. " + + $"IsRunning={subscription.IsRunning}, subscribed {subscribedCount} time(s). " + "This confirms the bug: exception in handler causes silent subscription death." ); } @@ -156,98 +155,9 @@ public override ValueTask HandleEvent(IMessageConsumeContex } } - /// - /// Validates that Ack does not throw when CheckpointCommitHandler is concurrently - /// nulled by Resubscribe/DisposeCommitHandler on another thread while the - /// AsyncHandlingFilter worker is still completing a message. - /// - [Test] - [Retry(3)] - public async Task Should_not_throw_nre_when_ack_races_with_resubscribe(CancellationToken ct) { - // Arrange - var loggerFactory = LoggingExtensions.GetLoggerFactory(); - var nreTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var ackStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var proceedToAck = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - - var options = new TestSubscriptionOptions { - SubscriptionId = "test-ack-race", - ThrowOnError = true, - CheckpointCommitBatchSize = 1, - CheckpointCommitDelayMs = 100 - }; - - // A handler that signals when it's about to ack, then waits for the test to - // trigger resubscribe before the ack path runs. - var handler = new SlowAckHandler(ackStarted, proceedToAck); - var pipe = new ConsumePipe().AddDefaultConsumer(handler); - - var checkpointStore = new NoOpCheckpointStore(); - - var subscription = new TestPollingSubscription( - options, - checkpointStore, - pipe, - loggerFactory, - eventCount: 20 - ); - - // Act - await subscription.Subscribe( - _ => { }, - (_, _, ex) => { - if (ex is NullReferenceException nre) nreTcs.TrySetResult(nre); - }, - ct - ); - - // Wait until the handler has processed an event and is about to ack - var started = await Task.WhenAny(ackStarted.Task, Task.Delay(TimeSpan.FromSeconds(10), ct)); - started.ShouldBe(ackStarted.Task, "Handler should have started processing an event"); - - // Now trigger Dropped → Resubscribe, which will null CheckpointCommitHandler - subscription.TriggerDropped(); - - // Give Resubscribe a moment to dispose the commit handler - await Task.Delay(200, ct); - - // Let the handler complete — the AsyncHandlingFilter worker will now call Acknowledge → Ack. - // Without the fix, the commit handler is already null at this point, causing an NRE. - proceedToAck.TrySetResult(); - - // Assert — wait for either the NRE or a timeout - var result = await Task.WhenAny(nreTcs.Task, Task.Delay(TimeSpan.FromSeconds(5), ct)); - - if (result == nreTcs.Task) { - var exception = await nreTcs.Task; - Assert.Fail( - $"NullReferenceException in Ack path during resubscribe race: {exception}. " + - "CheckpointCommitHandler was null when Ack tried to call Commit()." - ); - } - - // Cleanup - await subscription.Unsubscribe(_ => { }, ct); - } - - /// - /// A handler that signals the test when processing is happening, - /// then blocks until the test allows it to complete. This creates the - /// window for the race between Ack and Resubscribe. - /// - class SlowAckHandler(TaskCompletionSource ackStarted, TaskCompletionSource proceedToAck) : BaseEventHandler { - int _signaled; - - public override async ValueTask HandleEvent(IMessageConsumeContext context) { - // Signal only on the first event to avoid double-signaling - if (Interlocked.CompareExchange(ref _signaled, 1, 0) == 0) { - ackStarted.TrySetResult(); - await proceedToAck.Task; - } - - return EventHandlingStatus.Success; - } - } + // A test watching OnDropped for an NRE on an ack/teardown race used to live here, but was unreachable + // (SubscriptionRun.Fail keeps only the first reason). The invariant it meant to cover is now asserted in + // ResubscribeConcurrencyTests.An_acknowledgement_from_a_dropped_run_is_refused. record TestSubscriptionOptions : SubscriptionWithCheckpointOptions; @@ -273,33 +183,28 @@ class TestPollingSubscription( null, null ) { - TaskRunner? _runner; + SubscriptionRun? _run; - /// - /// Exposes the protected Dropped method so the test can trigger a resubscribe. - /// - public void TriggerDropped() - => Dropped(DropReason.SubscriptionError, new InvalidOperationException("Simulated drop for race test")); - - protected override ValueTask Subscribe(CancellationToken cancellationToken) { - _runner = new TaskRunner(PollEvents).Start(); + protected override async ValueTask Connect(SubscriptionRun run) { + Volatile.Write(ref _run, run); - return default; - } + var checkpoint = await GetCheckpoint(run).NoContext(); - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - if (_runner == null) return; + // Started on a task of its own so it never runs inline on the supervisor's stack during Connect. + var pumping = Task.Run(() => RunPollEvents(run, (int)(checkpoint.Position ?? 0)), CancellationToken.None); - await _runner.Stop(cancellationToken); - _runner.Dispose(); - _runner = null; + // No handle of its own to release: registered purely to join the loop before the next Connect. + run.OnDisconnect(_ => new(pumping)); } - async Task PollEvents(CancellationToken cancellationToken) { - var checkpoint = await GetCheckpoint(cancellationToken); - var start = (int)(checkpoint.Position ?? 0); + /// + /// Runs and reports its own death, the same contract a real transport keeps. + /// + Task RunPollEvents(SubscriptionRun run, int start) + => TransportPump.Run(run, () => PollEvents(run, start), "TestPollingSubscription pump ended while the connection was up"); - for (var i = start; i < eventCount && !cancellationToken.IsCancellationRequested; i++) { + async Task PollEvents(SubscriptionRun run, int start) { + for (var i = start; i < eventCount && !run.Token.IsCancellationRequested; i++) { var context = new MessageConsumeContext( Guid.NewGuid().ToString(), "TestEvent", @@ -308,20 +213,23 @@ async Task PollEvents(CancellationToken cancellationToken) { (ulong)i, (ulong)i, (ulong)i, - Sequence++, + run.NextSequence(), DateTime.UtcNow, new { EventNumber = i }, new(), Options.SubscriptionId, - cancellationToken + run.Token ) { LogContext = Log }; - await HandleInternal(context).NoContext(); + await HandleInternal(run, context).NoContext(); - await Task.Delay(50, cancellationToken); + await Task.Delay(50, run.Token).NoContext(); } onCompleted?.Invoke(); + + // Parked rather than returned: a pump ending while its connection is up is read as a drop. + await run.Ended.NoContext(); } } } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionRunTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionRunTests.cs new file mode 100644 index 000000000..9fcd2e9a2 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionRunTests.cs @@ -0,0 +1,179 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using System.Collections.Concurrent; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Logging; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// A single run's teardown contract, tested directly: a fake transport with one handle can't tell a correct +/// release ordering from a reversed one, and that ordering is what checkpoint durability rests on. +/// +public class SubscriptionRunTests { + static LogContext Log => Logger.CreateContext("subscription-run-tests", null); + + /// + /// Registration order is acquisition order, so releasing forwards would close the connection an in-flight + /// ack still needs. It is why the commit handler, registered before Connect, outlives every transport handle. + /// + [Test] + public async Task Releases_run_in_reverse_registration_order() { + var order = new ConcurrentQueue(); + var run = new SubscriptionRun(CancellationToken.None); + + for (var i = 0; i < 3; i++) { + var registered = i; + run.OnDisconnect(_ => { order.Enqueue(registered); return default; }); + } + + await run.Stop(CancellationToken.None, Log); + + order.ToArray().ShouldBe([2, 1, 0], "first registered must release last, or an ack lands after the handle it needs is gone"); + } + + /// + /// One handle that won't let go must not strand the rest — those are the ones holding the connection open. + /// + [Test] + public async Task A_release_that_throws_does_not_strand_the_others() { + var order = new ConcurrentQueue(); + var run = new SubscriptionRun(CancellationToken.None); + + run.OnDisconnect(_ => { order.Enqueue(0); return default; }); + + // Throws synchronously, before returning a ValueTask, which is why Disconnect invokes inside the try. + run.OnDisconnect(_ => throw new InvalidOperationException("cannot let go")); + + run.OnDisconnect(_ => { order.Enqueue(2); return default; }); + + await Should.NotThrowAsync(async () => await run.Stop(CancellationToken.None, Log)); + + order.ToArray().ShouldBe([2, 0], "the release registered before the throwing one still has to run"); + } + + /// + /// The graceful token means "stop being graceful", never "stop": a release that ignores it is still awaited, + /// or the next run reads a checkpoint the previous one hadn't finished writing. + /// + [Test] + [Timeout(10_000)] + public async Task Teardown_waits_for_a_release_that_ignores_the_graceful_token(CancellationToken ct) { + var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var letGo = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var released = false; + + var run = new SubscriptionRun(CancellationToken.None); + + run.OnDisconnect(async _ => { + entered.TrySetResult(); + await letGo.Task; + released = true; + }); + + // Gone before teardown starts: the worst case for a release that ignores it. + using var expired = new CancellationTokenSource(); + await expired.CancelAsync(); + + var stopping = run.Stop(expired.Token, Log).AsTask(); + await entered.Task.WaitAsync(ct); + + stopping.IsCompleted.ShouldBeFalse("an expired budget must not cut a release off part-way"); + + letGo.TrySetResult(); + await stopping; + + released.ShouldBeTrue("every release is awaited to completion regardless of the graceful token"); + } + + /// + /// Teardown clears its registrations, so a second Stop releases nothing again and doesn't throw on the + /// source the first disposed. + /// + [Test] + [Timeout(10_000)] + public async Task A_second_Stop_releases_nothing_again_and_does_not_throw(CancellationToken ct) { + var releases = 0; + var run = new SubscriptionRun(CancellationToken.None); + + run.OnDisconnect(_ => { Interlocked.Increment(ref releases); return default; }); + + await run.Stop(CancellationToken.None, Log); + await Should.NotThrowAsync(async () => await run.Stop(CancellationToken.None, Log)); + + releases.ShouldBe(1, "releasing a handle twice is the double-dispose the registration clear exists to prevent"); + } + + /// + /// First reason wins: a dying transport raises several failures, and only the first is the cause. + /// + [Test] + public void Fail_keeps_the_first_reason_and_ignores_later_ones() { + var run = new SubscriptionRun(CancellationToken.None); + var cause = new InvalidOperationException("the connection went away"); + + run.Fail(DropReason.ServerError, cause); + run.Fail(DropReason.SubscriptionError, new FormatException("a consequence of the above")); + + run.Failure!.Reason.ShouldBe(DropReason.ServerError); + run.Failure!.Exception.ShouldBeSameAs(cause); + } + + /// + /// Failure and shutdown arrive through one arm, so the supervisor awaits one signal rather than racing two. + /// + [Test] + [Timeout(10_000)] + public async Task Ended_completes_on_a_failure_and_on_cancellation_alike() { + var failed = new SubscriptionRun(CancellationToken.None); + failed.Ended.IsCompleted.ShouldBeFalse("a healthy run has not ended"); + + failed.Fail(DropReason.ServerError, new InvalidOperationException("gone")); + await failed.Ended; + + using var lifetime = new CancellationTokenSource(); + var stopped = new SubscriptionRun(lifetime.Token); + + await lifetime.CancelAsync(); + await stopped.Ended; + + stopped.Failure.ShouldBeNull("a shutdown is not a failure, so there is nothing for the supervisor to report"); + } + + /// + /// The sequence belongs to the run, so a replacement starts from zero rather than inheriting a counter a + /// late ack could collide with. + /// + [Test] + public void NextSequence_starts_at_zero_and_each_run_has_its_own() { + var first = new SubscriptionRun(CancellationToken.None); + + first.NextSequence().ShouldBe(0ul); + first.NextSequence().ShouldBe(1ul); + + new SubscriptionRun(CancellationToken.None).NextSequence().ShouldBe(0ul, "a replacement run must not inherit its predecessor's sequence"); + } + + /// + /// Drawn from the channel worker's threads, so the draw has to be atomic. Can't prove the absence of a + /// race, but a lost update shows up as a duplicate. + /// + [Test] + [Timeout(30_000)] + public async Task Concurrent_NextSequence_yields_unique_values(CancellationToken ct) { + const int threads = 8; + const int perThread = 500; + + var run = new SubscriptionRun(CancellationToken.None); + var drawn = new ConcurrentQueue(); + + await Task.WhenAll( + Enumerable.Range(0, threads) + .Select(_ => Task.Run(() => { for (var i = 0; i < perThread; i++) drawn.Enqueue(run.NextSequence()); }, ct)) + ); + + drawn.Distinct().Count().ShouldBe(threads * perThread, "two messages sharing a sequence let the checkpoint advance over one of them"); + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionShutdownTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionShutdownTests.cs index cacf848a0..675da05dd 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionShutdownTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionShutdownTests.cs @@ -1,123 +1,130 @@ -using System.Collections.Concurrent; using Eventuous.Subscriptions; using Eventuous.Subscriptions.Filters; -using Microsoft.Extensions.Logging; using Shouldly; namespace Eventuous.Tests.Subscriptions; /// -/// A dropped subscription schedules the resubscribe on a background task, and that task used to read -/// Stopping.Token long after Unsubscribe got to the source. A drop that races shutdown is -/// benign — there is nothing left to resubscribe to — but it used to cost a spurious warning, an -/// unobserved exception, and a commit-handler dispose racing the one in Finalize (AI-1699). +/// A failure can be reported from any thread, at any time, including on a run shutdown has already ended. +/// That used to cost a spurious warning, an unobserved exception, or a commit-handler dispose racing the +/// one in teardown (AI-1699); now it should cost nothing at all. /// public class SubscriptionShutdownTests { /// - /// The KurrentDB subscriptions cancel Stopping at the top of their Unsubscribe, so a - /// drop during shutdown normally finds the token cancelled. Resubscribing from there is pure waste: - /// EventSubscriptionWithCheckpoint.Resubscribe disposes the commit handler before it ever - /// looks at the token, which is what put a second disposer in the race with Finalize. + /// A failure arriving after the subscription's lifetime is cancelled has nothing to restart, observed + /// by counting connects — a second one would mean the failure bought an unwanted replacement run. /// [Test] - public async Task Drop_after_shutdown_started_does_not_resubscribe(CancellationToken ct) { - var subscription = new TestSubscription(new() { SubscriptionId = "test-drop-when-cancelled" }, new ConsumePipe(), new CapturingLoggerFactory()); + public async Task Failure_after_shutdown_started_does_not_reconnect(CancellationToken ct) { + var subscription = new TestSubscription(new() { SubscriptionId = "test-fail-when-cancelled", RetryDelay = ShortRetry }, new ConsumePipe(), new CapturingLoggerFactory()); - await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + using var host = CancellationTokenSource.CreateLinkedTokenSource(ct); - subscription.DropAfterCancellingStopping(); + await subscription.Subscribe(_ => { }, (_, _, _) => { }, host.Token); - var resubscribed = await subscription.WaitForResubscribe(TimeSpan.FromSeconds(2)); + // Cancelled through the token Subscribe was given — the route shutdown uses — not the subscription's own. + await host.CancelAsync(); + subscription.Fail(); - resubscribed.ShouldBeFalse("a subscription that is already stopping has nothing to resubscribe to"); + (await Wait.Until(() => !subscription.IsRunning, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the cancelled subscription should have finished stopping"); + + // Ten retry delays after it stopped: an unwanted replacement run would have connected by now. + await Task.Delay(TimeSpan.FromMilliseconds(200), ct); + + subscription.Connects.ShouldBe(1, "a subscription that is already stopping has nothing to reconnect to"); + subscription.IsRunning.ShouldBeFalse(); } + /// + /// The losing side of the race: shutdown disposes the run's source while a failure is still being + /// reported against it. Run many times, since the window can't be reconstructed deterministically. + /// [Test] - public async Task Drop_racing_unsubscribe_does_not_report_a_disposed_cts(CancellationToken ct) { + public async Task Failure_racing_unsubscribe_does_not_report_a_disposed_cts(CancellationToken ct) { var logs = new CapturingLoggerFactory(); - var subscription = new TestSubscription(new() { SubscriptionId = "test-drop-race" }, new ConsumePipe(), logs); + for (var i = 0; i < 50; i++) { + var subscription = new TestSubscription(new() { SubscriptionId = $"test-fail-race-{i}", RetryDelay = ShortRetry }, new ConsumePipe(), logs); - await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - // Reproduce the losing side of the race: Unsubscribe has already disposed Stopping while the - // subscription still believes it's running, which is exactly what lets Dropped reach the token. - subscription.DropAfterDisposingStopping(); + var failing = Task.Run( + () => { + for (var fail = 0; fail < 20; fail++) subscription.Fail(); + }, + ct + ); - var reported = await logs.WaitForWarning("CancellationTokenSource has been disposed", TimeSpan.FromSeconds(2)); + await subscription.Unsubscribe(_ => { }, ct); + await failing; + } - reported.ShouldBeFalse("dropping while Unsubscribe disposes Stopping is a benign shutdown race, not an error"); - } + // Matches text that only appears in an ObjectDisposedException's message, not just the log template. + var reported = logs.Contains("CancellationTokenSource has been disposed"); - record TestSubscriptionOptions : SubscriptionOptions; + reported.ShouldBeFalse("failing a run while shutdown tears it down is a benign race, not an error"); + } /// - /// A subscription that does nothing but expose the drop path. Stopping is protected, so both - /// shutdown states can be reproduced without any test-only hooks in the production class. + /// Two shutdown paths racing: whichever caller reads the session before the supervisor retires it + /// performs the stop, but OnUnsubscribed answers "is this subscription stopped", not "did this + /// call stop it" — so both callers are owed an answer, and neither may hang or throw. /// - class TestSubscription(TestSubscriptionOptions options, ConsumePipe pipe, ILoggerFactory loggerFactory) - : EventSubscription(options, pipe, loggerFactory, null) { - readonly TaskCompletionSource _resubscribed = new(TaskCreationOptions.RunContinuationsAsynchronously); + /// + /// Does not reach the window Unsubscribe's ObjectDisposedException clause guards — 150 + /// attempts never landed it. That clause stands on AI-1699, not on this test. + /// + [Test] + public async Task Concurrent_unsubscribes_do_not_report_a_disposed_cts(CancellationToken ct) { + var logs = new CapturingLoggerFactory(); - public void DropAfterCancellingStopping() { - Stopping.Cancel(false); - Drop(); - } + for (var i = 0; i < 50; i++) { + var subscription = new TestSubscription(new() { SubscriptionId = $"test-stop-race-{i}", RetryDelay = ShortRetry }, new ConsumePipe(), logs); - public void DropAfterDisposingStopping() { - Stopping.Dispose(); - Drop(); - } + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); - public async Task WaitForResubscribe(TimeSpan timeout) - => await Task.WhenAny(_resubscribed.Task, Task.Delay(timeout)) == _resubscribed.Task; + var stops = 0; + var second = Task.Run(async () => await subscription.Unsubscribe(_ => Interlocked.Increment(ref stops), ct), ct); - protected override Task Resubscribe(TimeSpan delay, CancellationToken cancellationToken) { - _resubscribed.TrySetResult(); + await subscription.Unsubscribe(_ => Interlocked.Increment(ref stops), ct); + await second; - return Task.CompletedTask; + stops.ShouldBe(2, "both callers asked to be told the subscription stopped, and for both of them it is"); + subscription.IsRunning.ShouldBeFalse("both callers returned, so the subscription is down either way"); } - protected override ValueTask Subscribe(CancellationToken cancellationToken) => default; + var reported = logs.Contains("CancellationTokenSource has been disposed"); - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => default; - - void Drop() => Dropped(DropReason.SubscriptionError, new InvalidOperationException("Simulated drop during shutdown")); + reported.ShouldBeFalse("a supervisor that finished before the second caller reached it is not a disconnect failure"); } - sealed class CapturingLoggerFactory : ILoggerFactory { - readonly ConcurrentQueue _warnings = []; - - /// - /// Polls rather than waiting out the full timeout, so the failing case reports in milliseconds. - /// The resubscribe runs on a fire-and-forget task, so there is nothing to await on. - /// - public async Task WaitForWarning(string contains, TimeSpan timeout) { - var deadline = DateTime.UtcNow + timeout; - - while (DateTime.UtcNow < deadline) { - if (_warnings.Any(w => w.Contains(contains))) return true; - - await Task.Delay(20); - } - - return false; - } + /// + /// Short, so the races below churn through connects and teardowns rather than sitting in the retry delay. + /// + static readonly TimeSpan ShortRetry = TimeSpan.FromMilliseconds(20); - public ILogger CreateLogger(string categoryName) => new CapturingLogger(_warnings); + record TestSubscriptionOptions : SubscriptionOptions; - public void AddProvider(ILoggerProvider provider) { } + /// + /// Counts its connects and hands out the run to fail; the run is kept past its own teardown on purpose, + /// since failing a retired run is exactly what these tests are about. + /// + class TestSubscription(TestSubscriptionOptions options, ConsumePipe pipe, ILoggerFactory loggerFactory) + : EventSubscription(options, pipe, loggerFactory, null) { + int _connects; + SubscriptionRun? _run; - public void Dispose() { } + public int Connects => Volatile.Read(ref _connects); - sealed class CapturingLogger(ConcurrentQueue warnings) : ILogger { - public IDisposable? BeginScope(TState state) where TState : notnull => null; + public void Fail() => Volatile.Read(ref _run)?.Fail(DropReason.SubscriptionError, new InvalidOperationException("Simulated failure during shutdown")); - public bool IsEnabled(LogLevel logLevel) => true; + protected override ValueTask Connect(SubscriptionRun run) { + Volatile.Write(ref _run, run); + Interlocked.Increment(ref _connects); - public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { - if (logLevel >= LogLevel.Warning) warnings.Enqueue(formatter(state, exception)); - } + return default; } } + } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorSettingsTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorSettingsTests.cs new file mode 100644 index 000000000..a9aea4989 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorSettingsTests.cs @@ -0,0 +1,82 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Logging; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// A delay that can't be waited on has to fall back, loudly: left alone, a negative retry delay throws inside +/// the supervisor's Task.Delay and a negative teardown timeout never expires. +/// +public class SupervisorSettingsTests { + // -1ms, which Task.Delay and CancellationTokenSource read as "never": the one negative that must survive. + const long InfiniteMs = -1; + + [Test] + [Arguments(-5_000, 2_000, true)] + [Arguments(InfiniteMs, InfiniteMs, false)] + [Arguments(0, 0, false)] + [Arguments(5_000, 5_000, false)] + public void Retry_delay_is_replaced_only_when_it_cannot_be_waited_on(long configuredMs, long expectedMs, bool warns) { + var logs = new CapturingLoggerFactory(); + + var settings = SupervisorSettings.From( + new TestOptions { SubscriptionId = "settings", RetryDelay = TimeSpan.FromMilliseconds(configuredMs) }, + Logger.CreateContext("settings", logs) + ); + + settings.RetryDelay.ShouldBe(TimeSpan.FromMilliseconds(expectedMs)); + settings.TeardownTimeout.ShouldBe(SubscriptionOptions.DefaultTeardownTimeout, "a valid teardown timeout must not be disturbed by the retry delay"); + + logs.Contains("Retry delay").ShouldBe(warns, "a silent fallback leaves an operator reading a value the subscription isn't using"); + logs.Contains("Teardown timeout").ShouldBeFalse(); + } + + [Test] + [Arguments(-5_000, 5_000, true)] + [Arguments(InfiniteMs, InfiniteMs, false)] + [Arguments(0, 0, false)] + [Arguments(7_000, 7_000, false)] + public void Teardown_timeout_is_replaced_only_when_it_cannot_be_waited_on(long configuredMs, long expectedMs, bool warns) { + var logs = new CapturingLoggerFactory(); + + var settings = SupervisorSettings.From( + new TestOptions { SubscriptionId = "settings", TeardownTimeout = TimeSpan.FromMilliseconds(configuredMs) }, + Logger.CreateContext("settings", logs) + ); + + settings.TeardownTimeout.ShouldBe(TimeSpan.FromMilliseconds(expectedMs)); + settings.RetryDelay.ShouldBe(SubscriptionOptions.DefaultRetryDelay, "a valid retry delay must not be disturbed by the teardown timeout"); + + logs.Contains("Teardown timeout").ShouldBe(warns); + logs.Contains("Retry delay").ShouldBeFalse(); + } + + /// + /// Both bad at once must both be reported, or fixing one means restarting to hear about the other. + /// + [Test] + public void Two_unusable_settings_are_both_reported_and_both_fall_back() { + var logs = new CapturingLoggerFactory(); + + var settings = SupervisorSettings.From( + new TestOptions { + SubscriptionId = "settings", + RetryDelay = TimeSpan.FromSeconds(-3), + TeardownTimeout = TimeSpan.FromSeconds(-4) + }, + Logger.CreateContext("settings", logs) + ); + + settings.RetryDelay.ShouldBe(SubscriptionOptions.DefaultRetryDelay); + settings.TeardownTimeout.ShouldBe(SubscriptionOptions.DefaultTeardownTimeout); + + logs.Contains("Retry delay -00:00:03 cannot be waited on").ShouldBeTrue(); + logs.Contains("Teardown timeout -00:00:04 cannot be waited on").ShouldBeTrue(); + } + + record TestOptions : SubscriptionOptions; +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorTests.cs new file mode 100644 index 000000000..d5ef38f9e --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/SupervisorTests.cs @@ -0,0 +1,598 @@ +using System.Collections.Concurrent; +using System.Diagnostics; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// Guarantees of the supervisor in EventSubscription.Supervise that no other test file exercises directly. +/// Each one regressed silently once already during the resubscribe rewrite. +/// +public class SupervisorTests { + /// + /// A subscription whose transport is never reachable must fail Subscribe outright, not retry forever + /// inside it, and a first connect failure must not poison the instance for a later attempt. + /// + [Test] + public async Task A_first_connect_that_throws_propagates_and_is_not_retried(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(50)) { FailConnect = attempt => attempt == 0 ? new InvalidOperationException("no broker") : null }; + + await Should.ThrowAsync(() => subscription.Subscribe(_ => { }, (_, _, _) => { }, ct).AsTask()); + + subscription.IsRunning.ShouldBeFalse("a first connect that gave up must not be considered running"); + + // Long enough that a retry would have landed if the loop kept going after the return. + await Task.Delay(subscription.RetryDelayForTests + TimeSpan.FromMilliseconds(200), ct); + subscription.Connects.ShouldBe(1, "a failed first connect must not be retried"); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + subscription.IsRunning.ShouldBeTrue("a subscription that gave up once must still be able to start later"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// Once a subscription has been up at least once, a connect failure is just another drop cycle, not + /// terminal like the first. + /// + [Test] + public async Task A_connect_that_throws_after_the_subscription_is_up_is_retried(CancellationToken ct) { + var transitions = new Transitions(); + + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(20)) { + FailConnect = attempt => attempt == 1 ? new InvalidOperationException("blip") : null + }; + + await subscription.Subscribe(_ => transitions.Subscribed(), (_, _, _) => transitions.Dropped(), ct); + + subscription.Run!.Fail(DropReason.SubscriptionError, new InvalidOperationException("connection lost")); + + (await Wait.Until(() => subscription.Connects >= 3, TimeSpan.FromSeconds(5))) + .ShouldBeTrue($"the failing reconnect should have been retried, only saw {subscription.Connects} connects"); + + (await Wait.Until(() => subscription.IsRunning && transitions.Up, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the subscription should have come back up after the transient connect failure"); + + await subscription.Unsubscribe(_ => { }, ct); + + transitions.Drops.ShouldBeGreaterThanOrEqualTo(1, "the failing reconnect attempt should also have been reported as a drop"); + } + + /// + /// A throwing OnSubscribed must not read as a dropped connection — it's a caller callback, not part of the handshake. + /// + [Test] + public async Task A_throwing_OnSubscribed_does_not_end_the_run(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(50)); + + await subscription.Subscribe(_ => throw new InvalidOperationException("boom from OnSubscribed"), (_, _, _) => { }, ct); + + subscription.IsRunning.ShouldBeTrue("Subscribe should still have completed the handshake"); + + await Task.Delay(subscription.RetryDelayForTests + TimeSpan.FromMilliseconds(200), ct); + + subscription.Connects.ShouldBe(1, "a throwing OnSubscribed must not cost a reconnect"); + subscription.Run!.Ended.IsCompleted.ShouldBeFalse("the run announced to the caller must still be the live one"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// A throwing OnDropped must not unwind the retry loop it's reported from (see ReportDrop) — the + /// replacement run still has to come up. + /// + [Test] + public async Task A_throwing_OnDropped_does_not_stop_the_retry_loop(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(20)); + + await subscription.Subscribe(_ => { }, (_, _, _) => throw new InvalidOperationException("boom from OnDropped"), ct); + + subscription.Run!.Fail(DropReason.SubscriptionError, new InvalidOperationException("connection lost")); + + (await Wait.Until(() => subscription.Connects >= 2, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("a throwing OnDropped must not prevent the replacement run from connecting"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// OnUnsubscribed answers "is this subscription stopped", not "did this call stop it" — so it must fire + /// even for a subscription that never started. + /// + [Test] + [Timeout(10_000)] + public async Task Unsubscribe_reports_even_when_the_subscription_never_started(CancellationToken ct) { + var subscription = new FakeSubscription(); + + var called = false; + await subscription.Unsubscribe(_ => called = true, ct); + + called.ShouldBeTrue("a caller that asked to be told the subscription stopped is owed an answer either way"); + } + + /// + /// A first connect that gave up already retired its session, so Unsubscribe has nothing left to stop — + /// it must still report, and return promptly. + /// + [Test] + [Timeout(10_000)] + public async Task Unsubscribe_reports_when_the_first_connect_gave_up(CancellationToken ct) { + var subscription = new FakeSubscription { FailConnect = _ => new InvalidOperationException("no broker") }; + + await Should.ThrowAsync(() => subscription.Subscribe(_ => { }, (_, _, _) => { }, ct).AsTask()); + + var called = false; + await subscription.Unsubscribe(_ => called = true, ct); + + called.ShouldBeTrue("the subscription is stopped, which is what the callback reports"); + subscription.IsRunning.ShouldBeFalse(); + } + + /// + /// A stop that outlasts the caller's token is logged, not thrown — this is + /// on the host's shutdown token, where a throw would abort every service queued behind it. The session is + /// discarded either way, so the warning is the only signal that teardown was still running. + /// + [Test] + [Timeout(10_000)] + public async Task An_Unsubscribe_that_outlasts_its_token_still_reports_and_does_not_throw(CancellationToken ct) { + var logs = new CapturingLoggerFactory(); + + // Longer than the caller's patience below; observes its token so teardown still finishes. + var subscription = new FakeSubscription(loggerFactory: logs) { OnDisconnectAsync = token => Task.Delay(TimeSpan.FromSeconds(1), token) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + using var impatient = CancellationTokenSource.CreateLinkedTokenSource(ct); + impatient.CancelAfter(TimeSpan.FromMilliseconds(100)); + + var called = false; + await subscription.Unsubscribe(_ => called = true, impatient.Token); + + called.ShouldBeTrue("a best-effort stop still reports, so a caller isn't left waiting on a teardown it can't see"); + + (await logs.WaitForWarning("Gave up waiting for the subscription to stop", TimeSpan.FromSeconds(1))) + .ShouldBeTrue("giving up on a stop in progress is worth a line of its own, since nothing throws to say it"); + + // A teardown that outlived the caller must not leave the subscription refusing to start again. + subscription.IsRunning.ShouldBeFalse("a stop that timed out still gives up ownership of the session"); + await Should.NotThrowAsync(() => subscription.Subscribe(_ => { }, (_, _, _) => { }, ct).AsTask()); + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// Unsubscribe is idempotent (see TransportTeardownTests for the transport side): a repeated call must + /// still report, without hanging or throwing. + /// + [Test] + [Timeout(10_000)] + public async Task A_repeated_Unsubscribe_reports_again_and_does_not_throw(CancellationToken ct) { + var subscription = new FakeSubscription(); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + var calls = 0; + await subscription.Unsubscribe(_ => Interlocked.Increment(ref calls), ct); + await subscription.Unsubscribe(_ => Interlocked.Increment(ref calls), ct); + + calls.ShouldBe(2, "both callers asked to be told the subscription stopped, and for both of them it is"); + } + + /// + /// A clean stop reports no drop, even when the transport fails the run on its own cancellation — that + /// Fail runs inside CancelAsync and wins the first-wins race, so without a lifetime check a clean stop + /// would be reported as an unhealthy drop. + /// + [Test] + [Timeout(10_000)] + public async Task A_clean_stop_reports_no_drop_when_the_transport_fails_the_run_on_the_way_out(CancellationToken ct) { + var subscription = new FakeSubscription { FailRunOnCancellation = true }; + + var drops = 0; + await subscription.Subscribe(_ => { }, (_, _, _) => Interlocked.Increment(ref drops), ct); + await subscription.Unsubscribe(_ => { }, ct); + + drops.ShouldBe(0, "a drop raised by our own cancellation describes the shutdown, not a failure to report"); + } + + /// + /// A Disconnect that throws on every run — a broker already unreachable — must cost a log line, not the + /// retry loop, or the subscription that most needs to reconnect is the one that silently stops trying. + /// + [Test] + [Timeout(30_000)] + public async Task A_Disconnect_that_throws_does_not_stop_the_subscription_retrying(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(20)) { + Pump = _ => Task.FromException(new InvalidOperationException("connection lost")), + OnDisconnect = _ => throw new InvalidOperationException("cannot let go") + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await Wait.Until(() => subscription.Connects >= 3, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("a failed release ends the run, not the subscription"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// Disposal must stop the subscription before releasing the pipe, or a running supervisor keeps + /// reconnecting and dispatching into disposed filters. + /// + [Test] + [Timeout(10_000)] + public async Task Disposing_a_running_subscription_stops_it_before_releasing_the_pipe(CancellationToken ct) { + var filter = new CountingDisposableFilter(); + var disconnects = 0; + + var subscription = new FakeSubscription(pipe: new ConsumePipe().AddFilterFirst(filter).AddDefaultConsumer(new NoOpHandler())) { + OnDisconnect = _ => Interlocked.Increment(ref disconnects) + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + subscription.IsRunning.ShouldBeTrue(); + + await subscription.DisposeAsync(); + + subscription.IsRunning.ShouldBeFalse("a disposed subscription that keeps its supervisor is one that keeps reconnecting into a disposed pipe"); + disconnects.ShouldBe(1, "the transport is released first, which is the whole point of stopping before disposing"); + filter.Disposals.ShouldBe(1); + + await subscription.DisposeAsync(); + filter.Disposals.ShouldBe(1, "disposal is once, however many times it is asked for"); + } + + /// + /// Concurrent disposals must release the pipe once — a check-then-set guard letting both through would + /// dispose every filter in it twice. + /// + [Test] + [Timeout(10_000)] + public async Task Concurrent_disposals_release_the_pipe_once(CancellationToken ct) { + var filter = new CountingDisposableFilter(); + var subscription = new FakeSubscription(pipe: new ConsumePipe().AddFilterFirst(filter).AddDefaultConsumer(new NoOpHandler())); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(_ => Task.Run(async () => await subscription.DisposeAsync(), ct))); + + filter.Disposals.ShouldBe(1, "two disposals reaching the pipe means every filter in it is disposed twice"); + } + + /// + /// Disconnect must get a token of its own, not the run token teardown just cancelled — an already-cancelled + /// token turns release logic into an instant no-op and leaks the connection. + /// + [Test] + public async Task Disconnect_receives_a_token_that_is_not_cancelled(CancellationToken ct) { + bool? teardownTokenCancelled = null; + bool? runTokenCancelledByThen = null; + SubscriptionRun? capturedRun = null; + + var subscription = new FakeSubscription { + OnDisconnect = token => { + teardownTokenCancelled = token.IsCancellationRequested; + runTokenCancelledByThen = capturedRun?.Token.IsCancellationRequested; + } + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + capturedRun = subscription.Run; + + await subscription.Unsubscribe(_ => { }, ct); + + runTokenCancelledByThen.ShouldBe(true, "the run token must already be cancelled by the time Disconnect runs"); + teardownTokenCancelled.ShouldBe(false, "Disconnect must get its own budget, not the run token that was just cancelled"); + } + + /// + /// A drop landing right before shutdown must not turn Unsubscribe into a wait for the retry delay to elapse, + /// nor spend another connect attempt. + /// + [Test] + public async Task Cancelling_during_the_retry_delay_ends_the_subscription_without_another_connect(CancellationToken ct) { + var transitions = new Transitions(); + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromSeconds(10)); + + await subscription.Subscribe(_ => transitions.Subscribed(), (_, _, _) => transitions.Dropped(), ct); + + subscription.Run!.Fail(DropReason.SubscriptionError, new InvalidOperationException("connection lost")); + + (await Wait.Until(() => transitions.Drops >= 1, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the drop should have been reported before the retry delay starts"); + + var stopwatch = Stopwatch.StartNew(); + await subscription.Unsubscribe(_ => { }, ct); + stopwatch.Stop(); + + stopwatch.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(5), "Unsubscribe must not wait out the retry delay it interrupted"); + subscription.Connects.ShouldBe(1, "cancelling during the retry delay must not spend another connect attempt"); + } + + /// + /// A caller told its first connect failed must be able to retry immediately — if the session were still + /// published during teardown, the retry would hit "already running" and be turned away by a dying session. + /// + /// + /// The slow Disconnect observes its token so this fails on a bad ordering instead of hanging. + /// + [Test] + public async Task A_retry_after_a_failed_first_connect_actually_starts(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(50)) { + FailConnect = attempt => attempt == 0 ? new InvalidOperationException("no broker") : null, + OnDisconnectAsync = token => Task.Delay(300, token) + }; + + await Should.ThrowAsync(() => subscription.Subscribe(_ => { }, (_, _, _) => { }, ct).AsTask()); + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + subscription.Connects.ShouldBe(2, "the retry must start a run of its own rather than returning against the dying session"); + subscription.IsRunning.ShouldBeTrue("the retry must leave the subscription up"); + + await subscription.Unsubscribe(_ => { }, ct); + } + + /// + /// One run per instance: a second Subscribe must be refused before its callbacks are wired in, or the + /// live run starts reporting to a caller that was told nothing. + /// + [Test] + public async Task A_second_subscribe_while_running_is_refused_and_keeps_the_live_callbacks(CancellationToken ct) { + var firstDrops = new ConcurrentQueue(); + var secondDrops = new ConcurrentQueue(); + + // Parked until the refusal has landed. + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(20)) { Pump = _ => release.Task }; + + await subscription.Subscribe(_ => { }, (_, reason, _) => firstDrops.Enqueue(reason), ct); + + // Check the message, not just the type — a connect failure is an InvalidOperationException too. + var refused = await Should.ThrowAsync( + () => subscription.Subscribe(_ => { }, (_, reason, _) => secondDrops.Enqueue(reason), ct).AsTask() + ); + + refused.Message.ShouldContain("already running"); + + subscription.IsRunning.ShouldBeTrue("a refused caller must leave the running subscription alone"); + + release.TrySetResult(); + + (await Wait.Until(() => !firstDrops.IsEmpty, TimeSpan.FromSeconds(5))) + .ShouldBeTrue("the drop belongs to the caller that started the run"); + + await subscription.Unsubscribe(_ => { }, ct); + + secondDrops.ShouldBeEmpty("a caller that was refused must not be wired into the live run"); + } + + /// + /// Whatever a transport hands is what the caller hears: same reason, + /// same exception instance, once per run. The supervisor neither classifies nor substitutes. + /// + /// + /// The cancellation case matters most: a cancelled task has no , so reading + /// only that field would lose it to a synthetic "processing ended". + /// + [Test] + [Timeout(10_000)] + [MethodDataSource(nameof(Failures))] + public async Task A_failed_run_reports_its_reason_and_exception_verbatim(DropReason reason, Exception failure, CancellationToken ct) { + var drops = new ConcurrentQueue<(DropReason Reason, Exception? Exception)>(); + + // Long enough that the failed run is the only one, so the queue holds exactly what it reported. + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromSeconds(30)); + + await subscription.Subscribe(_ => { }, (_, r, e) => drops.Enqueue((r, e)), ct); + + subscription.Run!.Fail(reason, failure); + + (await Wait.Until(() => !drops.IsEmpty, TimeSpan.FromSeconds(5))).ShouldBeTrue("a failed run is a drop to report"); + + await subscription.Unsubscribe(_ => { }, ct); + + drops.Count.ShouldBe(1, "one failure is one drop — health flapping is what a double report looks like"); + + var reported = drops.Single(); + reported.Reason.ShouldBe(reason); + reported.Exception.ShouldBeSameAs(failure, "the cause must arrive as it was raised, not re-wrapped or replaced"); + } + + public static IEnumerable> Failures() { + yield return () => (DropReason.ServerError, new InvalidOperationException("the connection went away")); + yield return () => (DropReason.SubscriptionError, new FormatException("the pump broke")); + yield return () => (DropReason.ServerError, new OperationCanceledException("the pump hit its own deadline")); + } + + /// + /// The retry delay has to actually elapse between attempts — a dead connection spinning into hundreds of + /// reconnects a second would still pass a test that only asserts a resubscribe happens. + /// + [Test] + public async Task The_retry_delay_elapses_between_connect_attempts(CancellationToken ct) { + var delay = TimeSpan.FromMilliseconds(300); + + // Every attempt after the first fails, so every gap between connects is a retry delay. + var subscription = new FakeSubscription(retryDelay: delay) { FailConnect = attempt => attempt == 0 ? null : new InvalidOperationException("still down") }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + subscription.Run!.Fail(DropReason.SubscriptionError, new InvalidOperationException("connection lost")); + + (await Wait.Until(() => subscription.ConnectTicks.Count >= 4, TimeSpan.FromSeconds(10))) + .ShouldBeTrue($"expected the loop to keep retrying, only saw {subscription.ConnectTicks.Count} connects"); + + await subscription.Unsubscribe(_ => { }, ct); + + var ticks = subscription.ConnectTicks.ToArray(); + + // Margin under the configured delay since the timer may fire a little early. + var floor = delay - TimeSpan.FromMilliseconds(50); + + for (var i = 1; i < ticks.Length; i++) { + Stopwatch.GetElapsedTime(ticks[i - 1], ticks[i]) + .ShouldBeGreaterThan(floor, $"connect {i} followed connect {i - 1} without waiting out the retry delay"); + } + } + + /// + /// The previous run's pump must be joined before the next connect, or two runs end up delivering into the + /// same consume pipe at once. + /// + [Test] + public async Task A_pump_that_lingers_after_cancellation_is_joined_before_the_next_connect(CancellationToken ct) { + var subscription = new FakeSubscription(retryDelay: TimeSpan.FromMilliseconds(20)) { + Pump = async run => { + try { await Task.Delay(Timeout.Infinite, run.Token); } catch (OperationCanceledException) { } + + // The overrun: keeps running well past cancellation, like a pump parked in a handler or a slow read. + await Task.Delay(500, CancellationToken.None); + } + }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + subscription.Run!.Fail(DropReason.SubscriptionError, new InvalidOperationException("connection lost")); + + (await Wait.Until(() => subscription.Connects >= 2, TimeSpan.FromSeconds(10))) + .ShouldBeTrue("the replacement run should have connected"); + + await subscription.Unsubscribe(_ => { }, ct); + + subscription.MaxLivePumpsAtConnect.ShouldBe(0, "a run started while its predecessor's pump was still running"); + } + + + + record TestOptions : SubscriptionOptions; + + /// + /// A minimal transport whose only behaviour is what a test configures: which connects fail, the retry + /// delay, and what Disconnect is handed. + /// + sealed class FakeSubscription(TimeSpan? retryDelay = null, ILoggerFactory? loggerFactory = null, ConsumePipe? pipe = null) + : EventSubscription( + new() { SubscriptionId = $"supervisor-{Guid.NewGuid()}", RetryDelay = retryDelay ?? TimeSpan.FromMilliseconds(20) }, + pipe ?? new ConsumePipe().AddDefaultConsumer(new NoOpHandler()), + loggerFactory, + null + ) { + int _connects; + int _livePumps; + int _maxLivePumpsAtConnect; + SubscriptionRun? _run; + + public int Connects => Volatile.Read(ref _connects); + + /// + /// Pumps seen running at the moment a connect started. Above zero means a run began alongside its + /// predecessor's pump, which the bounded join exists to prevent. + /// + public int MaxLivePumpsAtConnect => Volatile.Read(ref _maxLivePumpsAtConnect); + + /// + /// When each connect started, so a test can assert on the gaps between them. + /// + public ConcurrentQueue ConnectTicks { get; } = new(); + + /// + /// The run from the most recent Connect, kept past its own teardown since some tests fail it after the fact. + /// + public SubscriptionRun? Run => Volatile.Read(ref _run); + + public TimeSpan RetryDelayForTests => Options.RetryDelay; + + /// + /// Consulted with the zero-based attempt number before each connect; a returned exception fails that attempt. + /// + public Func? FailConnect { get; init; } + + public Action? OnDisconnect { get; init; } + + /// + /// Held for as long as this returns, so a test can widen the teardown window a caller races. + /// + public Func? OnDisconnectAsync { get; init; } + + /// + /// Started on the run when set; counted while it runs so the join can be asserted on. + /// + public Func? Pump { get; init; } + + /// + /// Fails the run from a registration on its own token, simulating a client that reports a drop the + /// instant the run token is cancelled, unaware the shutdown is ours. + /// + public bool FailRunOnCancellation { get; init; } + + protected override ValueTask Connect(SubscriptionRun run) { + var attempt = Interlocked.Increment(ref _connects) - 1; + Volatile.Write(ref _run, run); + ConnectTicks.Enqueue(Stopwatch.GetTimestamp()); + + // Assigned below, but the callback awaiting it is registered here, unconditionally, so teardown + // reaches this fake's release even when Connect is about to fail outright. + Task? pumping = null; + + run.OnDisconnect(async ct => { + OnDisconnect?.Invoke(ct); + + if (OnDisconnectAsync is not null) await OnDisconnectAsync(ct).ConfigureAwait(false); + + // Joining last keeps MaxLivePumpsAtConnect honest: the next Connect must never see this one still running. + if (pumping is not null) await pumping.ConfigureAwait(false); + }); + + if (FailRunOnCancellation) run.Token.Register(() => run.Fail(DropReason.ServerError, new("connection closed"))); + + var live = Volatile.Read(ref _livePumps); + + // Racy by design: any reading above zero is a predecessor still pumping. + if (live > MaxLivePumpsAtConnect) Volatile.Write(ref _maxLivePumpsAtConnect, live); + + if (FailConnect?.Invoke(attempt) is { } failure) throw failure; + + // Started on a task of its own so it never runs inline on the supervisor's stack during Connect. + if (Pump is { } pump) pumping = Task.Run(() => Counted(pump, run), CancellationToken.None); + + return default; + } + + /// + /// Runs the configured pump and reports its own death, the same contract a real transport keeps. + /// + async Task Counted(Func pump, SubscriptionRun run) { + Interlocked.Increment(ref _livePumps); + + try { + await TransportPump.Run(run, () => pump(run), "Processing ended while the connection was up").ConfigureAwait(false); + } finally { Interlocked.Decrement(ref _livePumps); } + } + } + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } + + /// + /// A pass-through filter that counts how many times the pipe disposed it. + /// + sealed class CountingDisposableFilter : ConsumeFilter, IAsyncDisposable { + int _disposals; + + public int Disposals => Volatile.Read(ref _disposals); + + protected override ValueTask Send(IMessageConsumeContext context, LinkedListNode? next) + => next?.Value.Send(context, next.Next) ?? default; + + public ValueTask DisposeAsync() { + Interlocked.Increment(ref _disposals); + + return default; + } + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/Transitions.cs b/src/Core/test/Eventuous.Tests.Subscriptions/Transitions.cs new file mode 100644 index 000000000..5c9550d66 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/Transitions.cs @@ -0,0 +1,27 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +namespace Eventuous.Tests.Subscriptions; + +/// +/// Drops and resubscriptions as the subscription's own callbacks report them. Written by the supervisor, +/// read by the test — hence the volatile and the interlocked. +/// +sealed class Transitions { + volatile bool _up; + int _drops; + + /// + /// True when the last transition was coming up rather than a drop. + /// + public bool Up => _up; + + public int Drops => Volatile.Read(ref _drops); + + public void Subscribed() => _up = true; + + public void Dropped() { + _up = false; + Interlocked.Increment(ref _drops); + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/TransportPump.cs b/src/Core/test/Eventuous.Tests.Subscriptions/TransportPump.cs new file mode 100644 index 000000000..7fba51a16 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/TransportPump.cs @@ -0,0 +1,31 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using Eventuous.Subscriptions; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// The contract every transport's reading loop keeps: report its death as this run's failure unless the run's +/// own token ended it. +/// +/// +/// Test infrastructure, deliberately. The supervisor has no pump classification of its own — each transport +/// decides (see SqlSubscriptionBase.Connect) — so keeping the fakes' copy here stops a test asserting +/// on it and calling that production behaviour. +/// +static class TransportPump { + public static async Task Run(SubscriptionRun run, Func loop, string endedWhileHealthy) { + try { + await loop().ConfigureAwait(false); + + if (!run.Token.IsCancellationRequested) run.Fail(DropReason.ServerError, new InvalidOperationException(endedWhileHealthy)); + } catch (OperationCanceledException e) when (!run.Token.IsCancellationRequested) { + run.Fail(DropReason.ServerError, e); + } catch (OperationCanceledException) { + // The run's own token asked for this: graceful, not a drop. + } catch (Exception e) { + run.Fail(DropReason.ServerError, e); + } + } +} diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs b/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs index ac6c2f9d4..d4476fe9d 100644 --- a/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs +++ b/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs @@ -6,10 +6,9 @@ namespace Eventuous.Tests.Subscriptions; /// -/// A transport's Unsubscribe releases whatever its Subscribe built, and some of what it releases is -/// single-use: a Google Pub/Sub SubscriberClient cannot be started twice, and an Azure Service Bus -/// processor is replaced rather than reused. So teardown has to run once per run, and it is the -/// framework's job to make sure of it — otherwise every transport has to be defensively idempotent. +/// Teardown must run exactly once per run — some transport resources are single-use (a Google Pub/Sub +/// SubscriberClient can't be started twice), and it's the framework's job, not every transport's, to +/// guarantee that. /// public class TransportTeardownTests { [Test] @@ -35,8 +34,7 @@ public async Task Teardown_does_not_run_when_the_subscription_never_started(Canc record TestOptions : SubscriptionOptions; /// - /// Counts what the framework asks of a transport. It holds no resources, because the point is the - /// call pattern rather than what a real transport would do with it. + /// Counts what the framework asks of a transport, without holding any real resources of its own. /// sealed class CountingSubscription() : EventSubscription( @@ -45,20 +43,12 @@ sealed class CountingSubscription() null, null ) { - int _starts; int _stops; - public int Starts => Volatile.Read(ref _starts); - public int Stops => Volatile.Read(ref _stops); + public int Stops => Volatile.Read(ref _stops); - protected override ValueTask Subscribe(CancellationToken cancellationToken) { - Interlocked.Increment(ref _starts); - - return default; - } - - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) { - Interlocked.Increment(ref _stops); + protected override ValueTask Connect(SubscriptionRun run) { + run.OnDisconnect(_ => { Interlocked.Increment(ref _stops); return default; }); return default; } diff --git a/src/Core/test/Eventuous.Tests.Subscriptions/Wait.cs b/src/Core/test/Eventuous.Tests.Subscriptions/Wait.cs new file mode 100644 index 000000000..2ab1fd942 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/Wait.cs @@ -0,0 +1,25 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +namespace Eventuous.Tests.Subscriptions; + +/// +/// Polls for a condition the subscription reaches on its own schedule, so the timeout is the failure bound +/// rather than the test's running time. +/// +static class Wait { + public static Task Until(Func condition, TimeSpan timeout) => Until(() => Task.FromResult(condition()), timeout); + + /// Checks once more after the deadline, so a boundary case isn't reported as a timeout. + public static async Task Until(Func> condition, TimeSpan timeout) { + var deadline = DateTime.UtcNow + timeout; + + while (DateTime.UtcNow < deadline) { + if (await condition()) return true; + + await Task.Delay(20); + } + + return await condition(); + } +} diff --git a/src/Gateway/test/Eventuous.Tests.Gateway/RegistrationTests.cs b/src/Gateway/test/Eventuous.Tests.Gateway/RegistrationTests.cs index 12ac6cf0b..95345ff8a 100644 --- a/src/Gateway/test/Eventuous.Tests.Gateway/RegistrationTests.cs +++ b/src/Gateway/test/Eventuous.Tests.Gateway/RegistrationTests.cs @@ -47,9 +47,7 @@ class TestTransform : IGatewayTransform { record TestOptions : SubscriptionOptions; class TestSub(TestOptions options, ConsumePipe consumePipe) : EventSubscription(options, consumePipe, NullLoggerFactory.Instance, null) { - protected override ValueTask Subscribe(CancellationToken cancellationToken) => default; - - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => default; + protected override ValueTask Connect(SubscriptionRun run) => default; } class TestProducer : BaseProducer { diff --git a/src/GooglePubSub/src/Eventuous.GooglePubSub.CloudRun/CloudRunPubSubSubscription.cs b/src/GooglePubSub/src/Eventuous.GooglePubSub.CloudRun/CloudRunPubSubSubscription.cs index a05aade23..4fe1bf6de 100644 --- a/src/GooglePubSub/src/Eventuous.GooglePubSub.CloudRun/CloudRunPubSubSubscription.cs +++ b/src/GooglePubSub/src/Eventuous.GooglePubSub.CloudRun/CloudRunPubSubSubscription.cs @@ -13,9 +13,18 @@ namespace Eventuous.GooglePubSub.CloudRun; public class CloudRunPubSubSubscription(CloudRunPubSubSubscriptionOptions options, ConsumePipe consumePipe, ILoggerFactory? loggerFactory, IEventSerializer? eventSerializer = null) : EventSubscription(options, consumePipe, loggerFactory, eventSerializer) { - protected override ValueTask Subscribe(CancellationToken cancellationToken) => ValueTask.CompletedTask; + // A push subscription has no connection to establish and no pump to run, so nothing can end its run + // short of shutdown: it has exactly one run for its whole life. + protected override ValueTask Connect(SubscriptionRun run) => default; - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) => ValueTask.CompletedTask; + ulong _sequence; + + /// + /// The sequence for the next pushed message. Counted here rather than on the run: with one run for the + /// whole life of the subscription the two are the same number, and the endpoint is reachable whether or + /// not anything ever started this subscription — it is mapped on the app, not on the run. + /// + ulong NextSequence() => Interlocked.Increment(ref _sequence) - 1; const string DefaultContentType = "application/json"; @@ -71,7 +80,7 @@ public static void MapSubscription(WebApplication app, string path = "/") { 0, 0, 0, - subscription.Sequence++, + subscription.NextSequence(), envelope.Message.PublishTime, message, null, diff --git a/src/GooglePubSub/src/Eventuous.GooglePubSub/Subscriptions/GooglePubSubSubscription.cs b/src/GooglePubSub/src/Eventuous.GooglePubSub/Subscriptions/GooglePubSubSubscription.cs index 5686a6065..fe22ad2c7 100644 --- a/src/GooglePubSub/src/Eventuous.GooglePubSub/Subscriptions/GooglePubSubSubscription.cs +++ b/src/GooglePubSub/src/Eventuous.GooglePubSub/Subscriptions/GooglePubSubSubscription.cs @@ -24,8 +24,6 @@ public class GooglePubSubSubscription : EventSubscription /// Creates a Google PubSub subscription service /// @@ -78,22 +76,52 @@ public GooglePubSubSubscription( if (options is { FailureHandler: not null, ThrowOnError: false }) Log.ThrowOnErrorIncompatible(); } - Task _subscriberTask = null!; - Task _monitorTask = null!; - - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { + protected override async ValueTask Connect(SubscriptionRun run) { var builder = new SubscriberClientBuilder { Logger = Log.Logger }; Options.ConfigureClientBuilder?.Invoke(builder); builder.SubscriptionName = _subscriptionName; if (Options.CreateSubscription) { - await CreateSubscription(_subscriptionName, _topicName, builder.EmulatorDetection, Options.ConfigureSubscription, cancellationToken).NoContext(); + await CreateSubscription(_subscriptionName, _topicName, builder.EmulatorDetection, Options.ConfigureSubscription, run.Token).NoContext(); + } + + var client = await builder.BuildAsync(run.Token).NoContext(); + + Task pumping; + + try { + // Started inline, not on its own task: StartAsync must have run before teardown can call StopAsync, + // which otherwise throws on a client that never started. + pumping = client.StartAsync(Handle); + } catch { + // Nothing is registered to release the client yet, and its StopAsync would throw, so dispose it here. + await client.DisposeAsync().NoContext(); + + throw; } - _client = await builder.BuildAsync(cancellationToken).NoContext(); + // Nothing else observes this task, so wire its end to Fail explicitly: any end other than a clean + // StopAsync-driven stop is a drop. + var reporting = pumping.ContinueWith( + t => { + if (t.IsCompletedSuccessfully && run.Token.IsCancellationRequested) return; - _subscriberTask = _client.StartAsync(Handle); - _monitorTask = MonitorSubscriberTask(_subscriberTask, cancellationToken); + run.Fail( + DropReason.ServerError, + t.Exception?.GetBaseException() ?? new InvalidOperationException("Google Pub/Sub client task ended before it was stopped") + ); + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default + ); + + // StopAsync first, then join: the client's task only ends once StopAsync has run, so joining first + // would deadlock until the graceful budget expires. + run.OnDisconnect(async ct => { + await client.StopAsync(ct).NoContext(); + await reporting.NoContext(); + }); return; @@ -113,7 +141,7 @@ async Task Handle(PubsubMessage msg, CancellationToken ct) { 0, 0, 0, - Sequence++, + run.NextSequence(), msg.PublishTime.ToDateTime(), evt, AsMeta(msg.Attributes), @@ -125,34 +153,12 @@ async Task Handle(PubsubMessage msg, CancellationToken ct) { await Handler(ctx).NoContext(); return Reply.Ack; - } catch (Exception ex) { return await _failureHandler(_client, msg, ex).NoContext(); } + } catch (Exception ex) { return await _failureHandler(client, msg, ex).NoContext(); } } Metadata AsMeta(MapField attributes) => new(attributes.ToDictionary(x => x.Key, object (x) => x.Value)!); } - async Task MonitorSubscriberTask(Task subscriberTask, CancellationToken cancellationToken) { - try { - await subscriberTask.NoContext(); - - // If the task completes without cancellation, the subscription was dropped - if (!cancellationToken.IsCancellationRequested) { - Dropped(DropReason.Stopped, null); - } - } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - // Expected when shutting down - } catch (Exception ex) { - // Subscriber task failed with an unrecoverable error - Dropped(DropReason.ServerError, ex); - } - } - - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - if (_client != null) await _client.StopAsync(cancellationToken).NoContext(); - await _subscriberTask.NoContext(); - await _monitorTask.NoContext(); - } - public async Task CreateSubscription( SubscriptionName subscriptionName, TopicName topicName, diff --git a/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/PubSubTests.cs b/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/PubSubTests.cs index 873bc2248..5725b83cd 100644 --- a/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/PubSubTests.cs +++ b/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/PubSubTests.cs @@ -63,7 +63,27 @@ public async Task SubscribeAndProduceMany(CancellationToken cancellationToken) { var testEvents = TestEvent.CreateMany(count); await _producer.Produce(_pubsubTopic, testEvents, null, cancellationToken: cancellationToken); - await _handler.AssertCollection(TimeSpan.FromSeconds(40), [..testEvents]).Validate(cancellationToken); + + // The expectation watches the whole window, so this is the test's runtime, not just its bound. The + // emulator delivers all 10k in well under a second; the rest is headroom for a slower machine. + await _handler.AssertCollection(TimeSpan.FromSeconds(15), [..testEvents]).Validate(cancellationToken); + } + + [Test] + [Retry(3)] + public async Task StopsAndStartsAgain(CancellationToken cancellationToken) { + // A SubscriberClient can be started and stopped once, so the second run has to build its own. That is + // why the client is released through the run rather than held on the subscription, and why it is only + // registered once StartAsync succeeded. Delivery after the restart is what shows the replacement + // client is the one receiving. + await _subscription.UnsubscribeWithLog(_log, cancellationToken); + await _subscription.SubscribeWithLog(_log, cancellationToken); + + var testEvent = TestEvent.Create(); + + await _producer.Produce(_pubsubTopic, testEvent, null, cancellationToken: cancellationToken); + + await _handler.AssertThat().Timebox(TimeSpan.FromSeconds(10)).Any().Match(x => x as TestEvent == testEvent).Validate(cancellationToken); } [Before(Test)] diff --git a/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs b/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs deleted file mode 100644 index 45d97643e..000000000 --- a/src/GooglePubSub/test/Eventuous.Tests.GooglePubSub/TeardownTests.cs +++ /dev/null @@ -1,41 +0,0 @@ -using Eventuous.GooglePubSub.Subscriptions; -using Eventuous.Subscriptions; -using Eventuous.Subscriptions.Context; -using Eventuous.Subscriptions.Filters; - -namespace Eventuous.Tests.GooglePubSub; - -/// -/// A SubscriberClient cannot be started twice — the SDK documents StartAsync as callable once per -/// instance — so the subscription builds a new one on every Subscribe. That only holds together if -/// teardown releases the one it stopped and never touches it again. No emulator is involved: the -/// constructor only resolves resource names, so teardown can be exercised on its own. -/// -/// Both tests assert by completing. An exception out of teardown fails them. -/// -public class TeardownTests { - [Test] - public async Task Teardown_without_a_subscribe_completes(CancellationToken ct) { - var subscription = Create(); - - // Reachable in production, not only from a test: Subscribe records the run before awaiting the - // transport, so a Subscribe that throws part-way leaves shutdown tearing down a run that never - // got as far as its subscriber task. - await subscription.Unsubscribe(_ => { }, ct); - } - - [Test] - public async Task Teardown_twice_completes(CancellationToken ct) { - var subscription = Create(); - - await subscription.Unsubscribe(_ => { }, ct); - await subscription.Unsubscribe(_ => { }, ct); - } - - static GooglePubSubSubscription Create() - => new("test-project", "test-topic", "test-subscription", new ConsumePipe().AddDefaultConsumer(new NoOpHandler())); - - sealed class NoOpHandler : BaseEventHandler { - public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); - } -} diff --git a/src/Kafka/src/Eventuous.Kafka/Subscriptions/KafkaBasicSubscription.cs b/src/Kafka/src/Eventuous.Kafka/Subscriptions/KafkaBasicSubscription.cs index b14f1162e..d53c3fcaf 100644 --- a/src/Kafka/src/Eventuous.Kafka/Subscriptions/KafkaBasicSubscription.cs +++ b/src/Kafka/src/Eventuous.Kafka/Subscriptions/KafkaBasicSubscription.cs @@ -9,9 +9,6 @@ namespace Eventuous.Kafka.Subscriptions; public class KafkaBasicSubscription(KafkaSubscriptionOptions options, ConsumePipe consumePipe, ILoggerFactory? loggerFactory, IEventSerializer? eventSerializer) : EventSubscription(options, consumePipe, loggerFactory, eventSerializer) { - protected override ValueTask Subscribe(CancellationToken cancellationToken) - => throw new NotImplementedException(); - - protected override ValueTask Unsubscribe(CancellationToken cancellationToken) + protected override ValueTask Connect(SubscriptionRun run) => throw new NotImplementedException(); } diff --git a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs index e4a68cb40..8106abd9c 100644 --- a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs +++ b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs @@ -76,35 +76,24 @@ public AllStreamSubscription( /// internal const string CheckpointReachedMessageType = "$checkpoint-reached"; - KurrentDBClient.StreamSubscriptionResult? _subscription; - Task? _messagePump; - - // The highest $all position known to be scanned by the server in the current run: seeded from the - // stored checkpoint on (re)subscribe, advanced by every received event and checkpoint message. The - // caught-up commit must never go below it — the commit machinery is gated by sequence, not by - // position, so an older position submitted later would regress the stored checkpoint. - ulong? _lastScannedPosition; - /// /// Starts the subscription /// - /// - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { + protected override async ValueTask Connect(SubscriptionRun run) { var filterOptions = new SubscriptionFilterOptions(Options.EventFilter ?? EventTypeFilter.ExcludeSystemEvents(), Options.CheckpointInterval); - var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); + var (_, position) = await GetCheckpoint(run).NoContext(); // The $all head, read before subscribing: by the time the server reports the subscription as // caught up, everything at or below this position has provably been scanned, so it can be // committed even if no event or checkpoint message ever surfaced it (small stores never cross // the checkpoint interval, idle tails park up to one interval below the head). - var headPosition = await GetAllStreamHead(cancellationToken).NoContext(); - _lastScannedPosition = position; + var head = await GetAllStreamHead(run.Token).NoContext(); var fromAll = GetPosition(); - var subscription = Client.SubscribeToAll(fromAll, Options.ResolveLinkTos, filterOptions, Options.Credentials, cancellationToken); - var messages = subscription.Messages.GetAsyncEnumerator(cancellationToken); + var subscription = Client.SubscribeToAll(fromAll, Options.ResolveLinkTos, filterOptions, Options.Credentials, run.Token); + var messages = subscription.Messages.GetAsyncEnumerator(run.Token); try { if (!await messages.MoveNextAsync().NoContext() || messages.Current is not StreamMessage.SubscriptionConfirmation) { @@ -117,8 +106,28 @@ protected override async ValueTask Subscribe(CancellationToken cancellationToken throw; } - _subscription = subscription; - _messagePump = Task.Run(() => PumpMessages(subscription, messages, headPosition, cancellationToken), CancellationToken.None); + // Runs on its own task so Connect never blocks; classified here because nothing else observes this task. + var pumping = Task.Run( + async () => { + try { + await Consume(run, messages, head, position).NoContext(); + } catch (Exception) when (run.Token.IsCancellationRequested) { + // Normal shutdown: the token cancelled the read, which is what teardown waits on. + } catch (Exception e) { + run.Fail(DropReason.ServerError, e); + } + }, + CancellationToken.None + ); + + // Join the pump first, then the enumerator, then the subscription: the enumerator's generated + // iterator shares one value-task source with an in-flight read, so disposing it before the pump + // has stopped reading would re-enter that source and fault on a pool thread. + run.OnDisconnect(async _ => { + await pumping.NoContext(); + await messages.DisposeAsync().NoContext(); + await subscription.DisposeAsync().NoContext(); + }); return; @@ -134,79 +143,65 @@ protected override async ValueTask Subscribe(CancellationToken cancellationToken /// handlers as before, plus the caught-up notification, which the callback-based client API /// silently discards. The message-based API is used precisely to observe that notification. /// - async Task PumpMessages( - KurrentDBClient.StreamSubscriptionResult subscription, - IAsyncEnumerator messages, - ulong? headPosition, - CancellationToken cancellationToken + /// + /// Returns only after reporting a consumer error; every other exit throws, leaving drop-vs-shutdown + /// classification to the caller. + /// + async Task Consume( + SubscriptionRun run, + IAsyncEnumerator messages, + ulong? headPosition, + ulong? lastScannedPosition ) { - try { - while (await messages.MoveNextAsync().NoContext()) { - // Falling behind re-enters catch-up mode, making the current head the new caught-up - // commit candidate: every match at or below it is delivered before the next caught-up - // notification, exactly like the pre-subscribe head on the initial catch-up. Reading the - // head on the caught-up message instead would be unsafe — matches between the server's - // live transition and the read could still be in flight, and committing past them skips - // them on restart. Handled outside the inner try because the read is a server call: its - // failures are transport failures and must reach the outer catch, not get labelled as - // consumer errors. - if (messages.Current is StreamMessage.FellBehind) { - headPosition = await GetAllStreamHead(cancellationToken).NoContext(); - - continue; - } - - try { - switch (messages.Current) { - case StreamMessage.Event(var resolvedEvent): - _lastScannedPosition = GetContextPosition(resolvedEvent); - await HandleInternal(CreateContext(resolvedEvent, cancellationToken)).NoContext(); - - break; - case StreamMessage.AllStreamCheckpointReached(var checkpointPosition): - _lastScannedPosition = checkpointPosition.CommitPosition; - await HandleCheckpointReached(checkpointPosition, cancellationToken).NoContext(); - - break; - case StreamMessage.CaughtUp: - // The server reached the live edge, so the commit candidate — the head read - // before (re-)entering catch-up mode — has been scanned even though no - // checkpoint message reported it. The client's caught-up message carries no - // position, so that read is the best provably scanned position available; - // skip it once something newer is already known. - if (headPosition is { } head && (_lastScannedPosition is not { } lastScanned || head > lastScanned)) { - _lastScannedPosition = head; - await HandleCheckpointReached(new(head, head), cancellationToken).NoContext(); - } - - break; - } - } catch (Exception ex) when (!cancellationToken.IsCancellationRequested) { - // Handling a message failed: the transport is fine, the consumer is not — same - // classification the callback-based API gave to errors thrown by its callbacks. - // DeserializeData rethrows the raw serializer exception, so matching on exception - // types here would misattribute malformed payloads to the server. - Dropped(DropReason.SubscriptionError, ex); - - return; - } + while (await messages.MoveNextAsync().NoContext()) { + // Falling behind re-enters catch-up mode: re-read the head as the new commit candidate, since + // reading it later from the caught-up message could race matches still in flight. Kept outside + // the inner try because a failure here is a transport failure, not a consumer error. + if (messages.Current is StreamMessage.FellBehind) { + headPosition = await GetAllStreamHead(run.Token).NoContext(); + + continue; } - // The server ended the message stream without an error and without being asked to stop: - // treat it as a drop, so the subscription resubscribes instead of staying silently dead - if (!cancellationToken.IsCancellationRequested) { - Dropped(DropReason.ServerError, new InvalidOperationException($"Subscription {Options.SubscriptionId} message stream ended unexpectedly")); + try { + switch (messages.Current) { + case StreamMessage.Event(var resolvedEvent): + lastScannedPosition = GetContextPosition(resolvedEvent); + await HandleInternal(run, CreateContext(run, resolvedEvent, run.Token)).NoContext(); + + break; + case StreamMessage.AllStreamCheckpointReached(var checkpointPosition): + lastScannedPosition = checkpointPosition.CommitPosition; + await HandleCheckpointReached(run, checkpointPosition, run.Token).NoContext(); + + break; + case StreamMessage.CaughtUp: + // The server reached the live edge, so the commit candidate — the head read + // before (re-)entering catch-up mode — has been scanned even though no + // checkpoint message reported it. The client's caught-up message carries no + // position, so that read is the best provably scanned position available; + // skip it once something newer is already known. + if (headPosition is { } head && (lastScannedPosition is not { } lastScanned || head > lastScanned)) { + lastScannedPosition = head; + await HandleCheckpointReached(run, new(head, head), run.Token).NoContext(); + } + + break; + } + } catch (Exception ex) when (!run.Token.IsCancellationRequested) { + // Handling a message failed: the transport is fine, the consumer is not — same + // classification the callback-based API gave to errors thrown by its callbacks. + // DeserializeData rethrows the raw serializer exception, so matching on exception + // types here would misattribute malformed payloads to the server. + run.Fail(DropReason.SubscriptionError, ex); + + return; } - } catch (Exception) when (cancellationToken.IsCancellationRequested) { - // Normal shutdown: the subscription got disposed or the token got cancelled mid-read - } catch (Exception ex) { - Dropped(DropReason.ServerError, ex); - } finally { - // Double disposal on the unsubscribe path is fine; on the dropped path this is the only - // cleanup of the underlying call before Resubscribe replaces the subscription. - await messages.DisposeAsync().NoContext(); - await subscription.DisposeAsync().NoContext(); } + + // Server closed the stream; thrown rather than reported, so shutdown-vs-drop is classified by the + // same filter that covers a read failing mid-shutdown. + throw new InvalidOperationException($"Subscription {Options.SubscriptionId} to $all: message stream ended unexpectedly"); } async Task GetAllStreamHead(CancellationToken cancellationToken) { @@ -218,27 +213,6 @@ CancellationToken cancellationToken return lastEvent.Length == 0 ? null : lastEvent[0].Event.Position.CommitPosition; } - /// - /// Stops the subscription - /// - /// - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - try { - Stopping.Cancel(false); - - if (_subscription != null) - await _subscription.DisposeAsync().NoContext(); - _subscription = null; - - if (_messagePump is { } pump) { - await Task.WhenAny(pump, Task.Delay(100, cancellationToken)).NoContext(); - _messagePump = null; - } - } catch (Exception) { - // Nothing to see here - } - } - /// /// The delivered record's own position in $all — the link's position for a resolved link event, /// never the resolved target's. The target can be arbitrarily older than the subscription cursor @@ -248,7 +222,7 @@ protected override async ValueTask Unsubscribe(CancellationToken cancellationTok /// static ulong GetContextPosition(ResolvedEvent re) => (re.OriginalPosition ?? re.OriginalEvent.Position).CommitPosition; - MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancellationToken) { + MessageConsumeContext CreateContext(SubscriptionRun run, ResolvedEvent re, CancellationToken cancellationToken) { var evt = DeserializeData( re.Event.ContentType, re.Event.EventType, @@ -265,7 +239,7 @@ MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancella re.Event.EventNumber, re.OriginalEventNumber, GetContextPosition(re), - Sequence++, + run.NextSequence(), re.Event.Created, evt, MetadataSerializer.DeserializeMeta(Options, re.Event.Metadata, re.Event.EventStreamId), @@ -283,7 +257,7 @@ MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancella /// everything since then, and consumers comparing the checkpoint to the $all head see a phantom, /// never-closing lag. /// - Task HandleCheckpointReached(global::KurrentDB.Client.Position position, CancellationToken cancellationToken) { + Task HandleCheckpointReached(SubscriptionRun run, global::KurrentDB.Client.Position position, CancellationToken cancellationToken) { var context = new MessageConsumeContext( position.CommitPosition.ToString(), CheckpointReachedMessageType, @@ -292,7 +266,7 @@ Task HandleCheckpointReached(global::KurrentDB.Client.Position position, Cancell position.CommitPosition, position.CommitPosition, position.CommitPosition, - Sequence++, + run.NextSequence(), DateTime.UtcNow, null, null, @@ -300,7 +274,7 @@ Task HandleCheckpointReached(global::KurrentDB.Client.Position position, Cancell cancellationToken ); - return HandleInternal(context).AsTask(); + return HandleInternal(run, context).AsTask(); } /// diff --git a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBCatchUpSubscriptionBase.cs b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBCatchUpSubscriptionBase.cs index c345c8399..e45d6978e 100644 --- a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBCatchUpSubscriptionBase.cs +++ b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBCatchUpSubscriptionBase.cs @@ -40,23 +40,4 @@ protected KurrentDBCatchUpSubscriptionBase( /// EventStoreDB client instance /// protected KurrentDBClient Client { get; } - - /// - /// Stops the subscription - /// - /// - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - try { - Stopping.Cancel(false); - Subscription?.Dispose(); - await Task.Delay(100, cancellationToken); - } catch (Exception) { - // Nothing to see here - } - } - - /// - /// Underlying EventStoreDB subscription - /// - protected global::KurrentDB.Client.StreamSubscription? Subscription { get; set; } } diff --git a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBMappings.cs b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBMappings.cs index 6a7248cc4..462ba4fc8 100644 --- a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBMappings.cs +++ b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/KurrentDBMappings.cs @@ -6,7 +6,7 @@ namespace Eventuous.KurrentDB.Subscriptions; static class KurrentDBMappings { public static DropReason AsDropReason(SubscriptionDroppedReason reason) => reason switch { - SubscriptionDroppedReason.Disposed => DropReason.Stopped, + SubscriptionDroppedReason.Disposed => DropReason.ServerError, SubscriptionDroppedReason.ServerError => DropReason.ServerError, SubscriptionDroppedReason.SubscriberError => DropReason.SubscriptionError, _ => throw new ArgumentOutOfRangeException(nameof(reason), reason, null) diff --git a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/PersistentSubscriptionBase.cs b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/PersistentSubscriptionBase.cs index 303d1efb5..60503a460 100644 --- a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/PersistentSubscriptionBase.cs +++ b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/PersistentSubscriptionBase.cs @@ -42,8 +42,6 @@ public abstract class PersistentSubscriptionBase : EventSubscription where readonly HandleEventProcessingFailure _handleEventProcessingFailure; - PersistentSubscription? _subscription; - /// /// EventStoreDB persistent subscription base class constructor /// @@ -115,27 +113,31 @@ protected PersistentSubscriptionBase( /// /// Subscribe to a persistent subscription /// - /// - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { + protected override async ValueTask Connect(SubscriptionRun run) { var settings = Options.SubscriptionSettings ?? new PersistentSubscriptionSettings(Options.ResolveLinkTos); + PersistentSubscription connected; + try { - _subscription = await LocalSubscribe(HandleEvent, HandleDrop, cancellationToken).NoContext(); + connected = await LocalSubscribe(HandleEvent, HandleDrop, run.Token).NoContext(); } catch (PersistentSubscriptionNotFoundException) { - await CreatePersistentSubscription(settings, cancellationToken); + await CreatePersistentSubscription(settings, run.Token); - _subscription = await LocalSubscribe(HandleEvent, HandleDrop, cancellationToken).NoContext(); + connected = await LocalSubscribe(HandleEvent, HandleDrop, run.Token).NoContext(); } + // No settling delay needed: the supervisor now ignores failures raised while shutting down. + run.OnDisconnect(_ => { connected.Dispose(); return default; }); + return; void HandleDrop(PersistentSubscription __, SubscriptionDroppedReason reason, Exception? exception) - => Dropped(KurrentDBMappings.AsDropReason(reason), exception); + => run.Fail(KurrentDBMappings.AsDropReason(reason), exception); async Task HandleEvent(PersistentSubscription subscription, ResolvedEvent re, int? retryCount, CancellationToken ct) { Logger.Configure(Options.SubscriptionId, LoggerFactory); - var context = CreateContext(re, ct) + var context = CreateContext(run, re, ct) .WithItem(ResolvedEventKey, re) .WithItem(SubscriptionKey, subscription); @@ -143,8 +145,8 @@ async Task HandleEvent(PersistentSubscription subscription, ResolvedEvent re, in await Handler(context).NoContext(); LastProcessed = EventPosition.FromContext(context); await Ack(context).NoContext(); - } catch (OperationCanceledException e) when (ct.IsCancellationRequested) { - Dropped(DropReason.Stopped, e); + } catch (OperationCanceledException) when (ct.IsCancellationRequested) { + // Its own token was cancelled: the supervisor already knows the run is over. } catch (Exception e) { await Nack(context, e).NoContext(); } @@ -194,7 +196,7 @@ async ValueTask Nack(MessageConsumeContext ctx, Exception exception) { await _handleEventProcessingFailure(Client, subscription, re, exception).NoContext(); } - MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancellationToken) { + MessageConsumeContext CreateContext(SubscriptionRun run, ResolvedEvent re, CancellationToken cancellationToken) { var evt = DeserializeData( re.Event.ContentType, re.Event.EventType, @@ -211,7 +213,7 @@ MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancella re.Event.EventNumber, GetContextStreamPosition(re), re.Event.Position.CommitPosition, - Sequence++, + run.NextSequence(), re.Event.Created, evt, MetadataSerializer.DeserializeMeta(Options, re.Event.Metadata, re.Event.EventStreamId, re.Event.EventNumber), @@ -227,20 +229,6 @@ MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancella /// protected abstract ulong GetContextStreamPosition(ResolvedEvent re); - /// - /// Unsubscribe from a persistent subscription - /// - /// - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - try { - _subscription?.Dispose(); - Stopping.Cancel(false); - await Task.Delay(100, cancellationToken); - } catch (Exception) { - // It might throw - } - } - static Task DefaultEventProcessingFailureHandler( KurrentDBClient client, PersistentSubscription subscription, diff --git a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/StreamSubscription.cs b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/StreamSubscription.cs index 9254e5e4b..d32e18b2d 100644 --- a/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/StreamSubscription.cs +++ b/src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/StreamSubscription.cs @@ -85,24 +85,26 @@ public StreamSubscription( /// /// Starts a catch-up subscription /// - /// - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { - var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); + protected override async ValueTask Connect(SubscriptionRun run) { + var (_, position) = await GetCheckpoint(run).NoContext(); var fromStream = GetStreamPosition(); - Subscription = await Client.SubscribeToStreamAsync( + var subscription = await Client.SubscribeToStreamAsync( Options.StreamName, fromStream, (_, @event, ct) => HandleEvent(@event, ct), Options.ResolveLinkTos, HandleDrop, Options.Credentials, - cancellationToken + run.Token ) .NoContext(); Log.InfoLog?.Log("Subscribed to stream {Stream}", Options.StreamName); + // No settling delay needed: the supervisor now ignores failures raised while shutting down. + run.OnDisconnect(_ => { subscription.Dispose(); return default; }); + return; FromStream GetStreamPosition() => position switch { @@ -119,14 +121,14 @@ async Task HandleEvent(ResolvedEvent re, CancellationToken ct) { if (Options.IgnoreSystemEvents && re.Event.EventType.Length > 0 && re.Event.EventType[0] == '$') return; - await HandleInternal(CreateContext(re, ct)).NoContext(); + await HandleInternal(run, CreateContext(run, re, ct)).NoContext(); } void HandleDrop(global::KurrentDB.Client.StreamSubscription _, SubscriptionDroppedReason reason, Exception? ex) - => Dropped(KurrentDBMappings.AsDropReason(reason), ex); + => run.Fail(KurrentDBMappings.AsDropReason(reason), ex); } - MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancellationToken) { + MessageConsumeContext CreateContext(SubscriptionRun run, ResolvedEvent re, CancellationToken cancellationToken) { var evt = DeserializeData( re.Event.ContentType, re.Event.EventType, @@ -150,7 +152,7 @@ MessageConsumeContext CreateContext(ResolvedEvent re, CancellationToken cancella re.Event.EventNumber, re.OriginalEventNumber.ToUInt64(), re.Event.Position.CommitPosition, - Sequence++, + run.NextSequence(), re.Event.Created, evt, meta, diff --git a/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/Fixtures/PersistentSubscriptionFixture.cs b/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/Fixtures/PersistentSubscriptionFixture.cs index 6a3482452..d833465bc 100644 --- a/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/Fixtures/PersistentSubscriptionFixture.cs +++ b/src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/Fixtures/PersistentSubscriptionFixture.cs @@ -46,7 +46,19 @@ public async ValueTask InitializeAsync() { } public async ValueTask DisposeAsync() { - if (autoStart) await Stop(); - _listener.Dispose(); + // Guarded, and the fixture released in the finally: both statements below touch fields that stay null + // until late in InitializeAsync, so an initialisation that failed earlier than that — a container that + // never became ready being the realistic case — would otherwise throw past the release. + try { + if (autoStart) await Stop(); + _listener.Dispose(); + } catch (Exception) { + // Whatever went wrong starting up, it must not cost us the container below. + } finally { + // The inner fixture owns the container this one started, so it has to go back here: nothing else + // holds a reference to it, and with it left running every use of this fixture costs the machine + // another KurrentDB instance until something reaps it. + await Fixture.DisposeAsync(); + } } } diff --git a/src/RabbitMq/src/Eventuous.RabbitMq/Subscriptions/RabbitMqSubscription.cs b/src/RabbitMq/src/Eventuous.RabbitMq/Subscriptions/RabbitMqSubscription.cs index de9f4e7ef..779edd1f4 100644 --- a/src/RabbitMq/src/Eventuous.RabbitMq/Subscriptions/RabbitMqSubscription.cs +++ b/src/RabbitMq/src/Eventuous.RabbitMq/Subscriptions/RabbitMqSubscription.cs @@ -7,6 +7,7 @@ using Eventuous.Subscriptions.Logging; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using RabbitMQ.Client.Exceptions; namespace Eventuous.RabbitMq.Subscriptions; @@ -20,9 +21,6 @@ public class RabbitMqSubscription : EventSubscription /// Creates RabbitMQ subscription service instance /// @@ -90,12 +88,17 @@ public RabbitMqSubscription( eventSerializer ) { } - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { - _connection = await _connectionFactory.CreateConnectionAsync(cancellationToken).NoContext(); - _channel = await _connection.CreateChannelAsync(cancellationToken: cancellationToken).NoContext(); + protected override async ValueTask Connect(SubscriptionRun run) { + // Registered as each handle opens, so a partial Connect still leaves teardown able to close what + // it opened, channel before connection. + var connection = await _connectionFactory.CreateConnectionAsync(run.Token).NoContext(); + run.OnDisconnect(CloseConnection); + + var channel = await connection.CreateChannelAsync(cancellationToken: run.Token).NoContext(); + run.OnDisconnect(CloseChannel); var prefetch = Options.PrefetchCount > 0 ? Options.PrefetchCount : Options.ConcurrencyLimit * 2; - await _channel.BasicQosAsync(0, (ushort)prefetch, false, cancellationToken).NoContext(); + await channel.BasicQosAsync(0, (ushort)prefetch, false, run.Token).NoContext(); var exchange = Ensure.NotEmptyString(Options.Exchange); @@ -105,73 +108,108 @@ protected override async ValueTask Subscribe(CancellationToken cancellationToken Log.WarnLog?.Log("Fan-out exchange doesn't support routing keys"); } - await _channel.ExchangeDeclareAsync( + await channel.ExchangeDeclareAsync( exchange, Options.ExchangeOptions.Type, Options.ExchangeOptions.Durable, Options.ExchangeOptions.AutoDelete, Options.ExchangeOptions.Arguments, - cancellationToken: cancellationToken + cancellationToken: run.Token ) .NoContext(); var queue = Options.QueueOptions.Queue ?? Options.SubscriptionId; Log.InfoLog?.Log("Ensuring queue {Queue}", queue); - await _channel.QueueDeclareAsync( + await channel.QueueDeclareAsync( queue, Options.QueueOptions.Durable, Options.QueueOptions.Exclusive, Options.QueueOptions.AutoDelete, Options.QueueOptions.Arguments, - cancellationToken: cancellationToken + cancellationToken: run.Token ) .NoContext(); Log.InfoLog?.Log("Binding exchange {Exchange} to queue {Queue}", exchange, queue); - await _channel.QueueBindAsync( + await channel.QueueBindAsync( queue, exchange, Options.BindingOptions.RoutingKey, Options.BindingOptions.Arguments, - cancellationToken: cancellationToken + cancellationToken: run.Token ) .NoContext(); - var consumer = new AsyncEventingBasicConsumer(_channel); - consumer.ReceivedAsync += HandleReceived; + // Channel captured as a local rather than looked up at ack time, since a delivery tag only means + // something on the channel it came from, and a resubscribe would have moved on to a different one. + var consumer = new AsyncEventingBasicConsumer(channel); + consumer.ReceivedAsync += (_, received) => HandleReceived(channel, received, run.Token); + + await channel.BasicConsumeAsync(queue, false, consumer, run.Token).NoContext(); + + return; + + // Each disposal is in its own finally: closes tend to fail exactly when the broker is unhealthy, + // which is when a leak costs most, and a throwing channel close must not skip the connection close. + async ValueTask CloseChannel(CancellationToken cancellationToken) { + try { + await channel.CloseAsync(cancellationToken).NoContext(); + } finally { + channel.Dispose(); + } + } - await _channel.BasicConsumeAsync(queue, false, consumer, cancellationToken).NoContext(); + async ValueTask CloseConnection(CancellationToken cancellationToken) { + try { + await connection.CloseAsync(cancellationToken: cancellationToken).NoContext(); + } finally { + connection.Dispose(); + } + } } const string ReceivedMessageKey = "receivedMessage"; - async Task HandleReceived(object sender, BasicDeliverEventArgs received) { + async Task HandleReceived(IChannel channel, BasicDeliverEventArgs received, CancellationToken cancellationToken) { Logger.Current = Log; try { - var ctx = CreateContext(sender, received).WithItem(ReceivedMessageKey, received); + var ctx = CreateContext(received, cancellationToken).WithItem(ReceivedMessageKey, received); await Handler(new AsyncConsumeContext(ctx, Ack, Nack)).NoContext(); } catch (Exception) { // This won't stop the subscription, but the reader will be gone. Not sure how to solve this one. if (Options.ThrowOnError) throw; } - } - async ValueTask Ack(IMessageConsumeContext ctx) { - var received = ctx.Items.GetItem(ReceivedMessageKey)!; - await _channel!.BasicAckAsync(received.DeliveryTag, false).NoContext(); - } + return; + + async ValueTask Ack(IMessageConsumeContext _) { + try { + await channel.BasicAckAsync(received.DeliveryTag, false).NoContext(); + } catch (Exception e) when (IsChannelGone(e)) { LogDeliveryUndecided(e); } + } - async ValueTask Nack(IMessageConsumeContext ctx, Exception exception) { - if (Options.ThrowOnError) throw exception; + async ValueTask Nack(IMessageConsumeContext _, Exception exception) { + if (Options.ThrowOnError) throw exception; - var received = ctx.Items.GetItem(ReceivedMessageKey)!; - await _failureHandler(_channel!, received, exception).NoContext(); + try { + await _failureHandler(channel, received, exception).NoContext(); + } catch (Exception e) when (IsChannelGone(e)) { LogDeliveryUndecided(e); } + } + + void LogDeliveryUndecided(Exception e) + => Log.WarnLog?.Log(e, "Delivery {DeliveryTag} left undecided, its channel is already closed", received.DeliveryTag); } - MessageConsumeContext CreateContext(object sender, BasicDeliverEventArgs received) { + /// + /// Whether a failed ack/nack means the channel is gone (a buffered handler finishing after teardown + /// closed it) rather than a broker refusal. Safe to swallow: an unacked delivery just gets redelivered. + /// + static bool IsChannelGone(Exception exception) => exception is AlreadyClosedException or ObjectDisposedException; + + MessageConsumeContext CreateContext(BasicDeliverEventArgs received, CancellationToken cancellationToken) { var evt = DeserializeData(received.BasicProperties.ContentType!, received.BasicProperties.Type!, received.Body, received.Exchange); var meta = received.BasicProperties.Headers != null @@ -191,24 +229,10 @@ MessageConsumeContext CreateContext(object sender, BasicDeliverEventArgs receive evt, meta, SubscriptionId, - default + cancellationToken ); } - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - if (_channel != null) { - await _channel.CloseAsync(cancellationToken).NoContext(); - _channel.Dispose(); - _channel = null; - } - - if (_connection != null) { - await _connection.CloseAsync(cancellationToken: cancellationToken).NoContext(); - _connection.Dispose(); - _connection = null; - } - } - async ValueTask DefaultEventFailureHandler(IChannel channel, BasicDeliverEventArgs message, Exception? exception) { Log.WarnLog?.Log("Error in the consumer, will redeliver", exception?.ToString() ?? "Unknown error"); await channel.BasicRejectAsync(message.DeliveryTag, true).NoContext(); diff --git a/src/Redis/src/Eventuous.Redis/Subscriptions/RedisSubscriptionBase.cs b/src/Redis/src/Eventuous.Redis/Subscriptions/RedisSubscriptionBase.cs index 53861ac27..2d306aa94 100644 --- a/src/Redis/src/Eventuous.Redis/Subscriptions/RedisSubscriptionBase.cs +++ b/src/Redis/src/Eventuous.Redis/Subscriptions/RedisSubscriptionBase.cs @@ -36,54 +36,62 @@ public abstract class RedisSubscriptionBase( protected GetRedisDatabase GetDatabase { get; } = Ensure.NotNull(getDatabase, "Connection factory"); - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { - await BeforeSubscribe(cancellationToken).NoContext(); + protected override async ValueTask Connect(SubscriptionRun run) { + await BeforeSubscribe(run.Token).NoContext(); - var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); + var checkpoint = await GetCheckpoint(run).NoContext(); - _runner = new TaskRunner(token => PollingQuery(position + 1, token)).Start(); - } + // Resolved before the pump starts: an unsupported StartFrom is a config error and must throw from + // Connect, not surface as a drop the supervisor retries forever from the pump. + // Local rather than a field so a later Connect can't move it under a loop still winding down. + var start = checkpoint.Position is { } position + ? (long)(position + 1) + : Options.StartFrom == InitialPosition.Earliest + ? 0 + : throw new NotSupportedException("Redis subscription does not support latest position"); - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - if (_runner == null) return; + // Runs on its own task so Connect never blocks; classified here because nothing else observes this task. + var pumping = Task.Run( + async () => { + try { + await Poll(run, start, run.Token).NoContext(); + } catch (Exception) when (run.Token.IsCancellationRequested) { + // This run's own token asked for it: graceful, not a drop. + } catch (Exception e) { + // Any other cancellation (an inner deadline, a WaitAsync timeout) is a real drop cause. + run.Fail(DropReason.ServerError, e); + } + }, + CancellationToken.None + ); - await _runner.Stop(cancellationToken); - _runner.Dispose(); - _runner = null; + // No handle of its own to release: registered purely to join the loop before the next Connect starts. + run.OnDisconnect(_ => new(pumping)); } const string ContentType = "application/json"; - TaskRunner? _runner; - - async Task PollingQuery(ulong? position, CancellationToken cancellationToken) { - var start = position.HasValue - ? (long)position - : Options.StartFrom == InitialPosition.Earliest - ? 0 - : throw new NotSupportedException("Redis subscription does not support latest position"); - + /// + /// The polling loop. Its only clean exit is the token; every other exit is a fault the pump in + /// reads as a drop. + /// + async Task Poll(SubscriptionRun run, long start, CancellationToken cancellationToken) { while (!cancellationToken.IsCancellationRequested) { try { var persistentEvents = await ReadEvents(GetDatabase(), start).NoContext(); foreach (var persistentEvent in persistentEvents) { - await HandleInternal(ToConsumeContext(persistentEvent, cancellationToken)).NoContext(); + await HandleInternal(run, ToConsumeContext(run, persistentEvent, cancellationToken)).NoContext(); start = persistentEvent.StreamPosition + 1; } } catch (InvalidOperationException e) when (e.Message.Contains("Reading is not allowed after reader was completed") || cancellationToken.IsCancellationRequested) { throw new OperationCanceledException("Redis read operation terminated", e, cancellationToken); - } catch (Exception e) { - IsDropped = true; - Log.WarnLog?.Log(e, "Subscription dropped"); - - throw; } } } - MessageConsumeContext ToConsumeContext(ReceivedEvent evt, CancellationToken cancellationToken) { + MessageConsumeContext ToConsumeContext(SubscriptionRun run, ReceivedEvent evt, CancellationToken cancellationToken) { Logger.Current = Log; var data = DeserializeData( @@ -96,10 +104,10 @@ MessageConsumeContext ToConsumeContext(ReceivedEvent evt, CancellationToken canc var meta = (evt.JsonMetadata == null) ? new() : _metaSerializer.Deserialize(Encoding.UTF8.GetBytes(evt.JsonMetadata)); - return AsContext(evt, data, meta, cancellationToken); + return AsContext(run, evt, data, meta, cancellationToken); } - MessageConsumeContext AsContext(ReceivedEvent evt, object? e, Metadata? meta, CancellationToken cancellationToken) + MessageConsumeContext AsContext(SubscriptionRun run, ReceivedEvent evt, object? e, Metadata? meta, CancellationToken cancellationToken) => new( evt.MessageId.ToString(), evt.MessageType, @@ -108,7 +116,7 @@ MessageConsumeContext AsContext(ReceivedEvent evt, object? e, Metadata? meta, Ca (ulong)evt.StreamPosition, (ulong)evt.StreamPosition, (ulong)evt.GlobalPosition, - Sequence++, + run.NextSequence(), evt.Created, e, meta, diff --git a/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs b/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs index 1981b40ef..c2981e6d1 100644 --- a/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs +++ b/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs @@ -1,4 +1,3 @@ -using Eventuous.Redis; using Eventuous.Redis.Subscriptions; using Eventuous.Subscriptions; using Eventuous.Subscriptions.Checkpoints; @@ -10,8 +9,7 @@ namespace Eventuous.Tests.Redis.Subscriptions; /// -/// The polling loop is the whole subscription: nothing else reads Redis. These tests need no server — -/// ReadEvents is the seam, and a failure there is the same shape as a failure from the driver. +/// The polling loop is the whole subscription, so these tests fail at the ReadEvents seam and need no server. /// public class PollFailureTests { /// diff --git a/src/Relational/src/Eventuous.Sql.Base/Subscriptions/SqlSubscriptionBase.cs b/src/Relational/src/Eventuous.Sql.Base/Subscriptions/SqlSubscriptionBase.cs index 00b968baa..d4655befd 100644 --- a/src/Relational/src/Eventuous.Sql.Base/Subscriptions/SqlSubscriptionBase.cs +++ b/src/Relational/src/Eventuous.Sql.Base/Subscriptions/SqlSubscriptionBase.cs @@ -76,9 +76,11 @@ public abstract class SqlSubscriptionBase( private record DetectedGap(long Position, DateTime FirstSeen); - async Task PollingQuery(ulong? position, CancellationToken cancellationToken) { - var start = position.HasValue ? (long)position : -1; - + /// + /// The polling loop. Its only clean exit is a stop request; every other exit is a fault the pump in + /// reads as a drop. + /// + async Task Poll(SubscriptionRun run, long start, CancellationToken cancellationToken) { DetectedGap? gap = null; var retryCount = 0; @@ -92,7 +94,7 @@ async Task PollingQuery(ulong? position, CancellationToken cancellationToken) { return; - async Task Poll() { + async Task PollOnce() { try { await using var connection = await OpenConnection(cancellationToken).NoContext(); await using var cmd = PrepareCommand(connection, start); @@ -114,7 +116,7 @@ async Task Poll() { } if (!ShouldSkipEvent(persistedEvent)) { - await HandleInternal(ToConsumeContext(persistedEvent, cancellationToken)).NoContext(); + await HandleInternal(run, ToConsumeContext(run, persistedEvent, cancellationToken)).NoContext(); } start = MoveStart(persistedEvent); @@ -133,27 +135,24 @@ async Task Poll() { return new(true, gap != null, received); } catch (Exception e) { - if (IsStopping(e)) { - IsDropped = true; - - return new(false, false, 0); - } + // IsStopping alone isn't enough: providers can report unrelated aborts (e.g. SQL Server's + // "Operation cancelled by user.") with the same shape, so only trust it once the token agrees. + if (IsStopping(e) && cancellationToken.IsCancellationRequested) return new(false, false, 0); if (IsTransient(e)) { return new(true, true, 0); } - Dropped(DropReason.ServerError, e); - - return new(false, false, 0); + // Let it propagate instead of reporting here too — a faulted pump is already a drop. + throw; } } async Task ExecutePollCycle() { while (!cancellationToken.IsCancellationRequested) { - var result = await Poll().NoContext(); + var result = await PollOnce().NoContext(); - if (!result.Continue) break; + if (!result.Continue) return; if (result.Retry) { await Task.Delay(Options.Retry.InitialDelayMs * retryCount++, cancellationToken).NoContext(); @@ -201,33 +200,42 @@ async Task ExecutePollCycle() { /// /// Starts the subscription /// - /// - protected override async ValueTask Subscribe(CancellationToken cancellationToken) { - await BeforeSubscribe(cancellationToken).NoContext(); - var (_, position) = await GetCheckpoint(cancellationToken).NoContext(); + /// The run being connected; reassigned every call, since a run is repeatable on this instance. + protected override async ValueTask Connect(SubscriptionRun run) { + await BeforeSubscribe(run.Token).NoContext(); + var checkpoint = await GetCheckpoint(run).NoContext(); + var position = checkpoint.Position; if (position == null && Options.StartFrom == InitialPosition.Latest) { - var endOfStream = await GetSubscriptionEndOfStream(cancellationToken).NoContext(); + var endOfStream = await GetSubscriptionEndOfStream(run.Token).NoContext(); if (endOfStream == EndOfStream.Invalid) { throw new InvalidOperationException($"Could not get the end of the stream for subscription {SubscriptionId}"); } - await CheckpointStore.StoreCheckpoint(new(SubscriptionId, endOfStream.Position), true, cancellationToken).NoContext(); + await CheckpointStore.StoreCheckpoint(new(SubscriptionId, endOfStream.Position), true, run.Token).NoContext(); position = endOfStream.Position; } - _runner = new TaskRunner(token => PollingQuery(position, token)).Start(); - } + // Local rather than a field: a later Connect on this instance must not move the position under a + // loop that is still winding down. + var start = position.HasValue ? (long)position : -1; - /// - /// Stops the subscription. - /// - /// - protected override async ValueTask Unsubscribe(CancellationToken cancellationToken) { - if (_runner == null) return; + // Runs on its own task so Connect never blocks; classified here because nothing else observes this task. + var pumping = Task.Run( + async () => { + try { + await Poll(run, start, run.Token).NoContext(); + } catch (Exception) when (run.Token.IsCancellationRequested) { + // This run's own token asked for it: graceful, not a drop. + } catch (Exception e) { + // Any other cancellation (an inner deadline, a WaitAsync timeout) is a real drop cause. + run.Fail(DropReason.ServerError, e); + } + }, + CancellationToken.None + ); - await _runner.Stop(cancellationToken); - _runner.Dispose(); - _runner = null; + // No handle of its own to release: registered purely to join the loop before the next Connect starts. + run.OnDisconnect(_ => new(pumping)); } /// @@ -242,17 +250,17 @@ protected override async ValueTask Unsubscribe(CancellationToken cancellationTok SubscriptionKind.Stream => evt.StreamPosition }; - MessageConsumeContext ToConsumeContext(PersistedEvent evt, CancellationToken cancellationToken) { + MessageConsumeContext ToConsumeContext(SubscriptionRun run, PersistedEvent evt, CancellationToken cancellationToken) { Logger.Current = Log; var data = DeserializeData(ContentType, evt.MessageType, Encoding.UTF8.GetBytes(evt.JsonData), evt.StreamName!, (ulong)evt.StreamPosition); var meta = evt.JsonMetadata == null ? new() : _metaSerializer.Deserialize(Encoding.UTF8.GetBytes(evt.JsonMetadata!)); - return AsContext(evt, data, meta, cancellationToken); + return AsContext(run, evt, data, meta, cancellationToken); } - MessageConsumeContext AsContext(PersistedEvent evt, object? e, Metadata? meta, CancellationToken cancellationToken) + MessageConsumeContext AsContext(SubscriptionRun run, PersistedEvent evt, object? e, Metadata? meta, CancellationToken cancellationToken) => Kind switch { SubscriptionKind.Stream => new( evt.MessageId.ToString(), @@ -262,7 +270,7 @@ MessageConsumeContext AsContext(PersistedEvent evt, object? e, Metadata? meta, C (ulong)evt.StreamPosition, (ulong)evt.StreamPosition, (ulong)evt.GlobalPosition, - Sequence++, + run.NextSequence(), evt.Created, e, meta, @@ -277,7 +285,7 @@ MessageConsumeContext AsContext(PersistedEvent evt, object? e, Metadata? meta, C (ulong)evt.StreamPosition, (ulong)evt.StreamPosition, (ulong)evt.GlobalPosition, - Sequence++, + run.NextSequence(), evt.Created, e, meta, @@ -286,8 +294,6 @@ MessageConsumeContext AsContext(PersistedEvent evt, object? e, Metadata? meta, C ) }; - TaskRunner? _runner; - const string ContentType = "application/json"; /// diff --git a/src/SqlServer/test/Eventuous.Tests.SqlServer/ExcludeOnMacOs.cs b/src/SqlServer/test/Eventuous.Tests.SqlServer/ExcludeOnMacOs.cs new file mode 100644 index 000000000..cecea2c9e --- /dev/null +++ b/src/SqlServer/test/Eventuous.Tests.SqlServer/ExcludeOnMacOs.cs @@ -0,0 +1,7 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using TUnit.Core.Enums; + +// SQL Server for Linux has no arm64 build; its amd64 image segfaults on startup under a Mac container runtime. +[assembly: ExcludeOn(OS.MacOs)] diff --git a/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/PollFailureTests.cs b/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/PollFailureTests.cs new file mode 100644 index 000000000..20389178d --- /dev/null +++ b/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/PollFailureTests.cs @@ -0,0 +1,153 @@ +// Copyright (C) Eventuous HQ OÜ. All rights reserved +// Licensed under the Apache License, Version 2.0. + +using Eventuous.Sqlite.Subscriptions; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Microsoft.Data.Sqlite; +using Shouldly; + +namespace Eventuous.Tests.Sqlite.Subscriptions; + +/// +/// How SqlSubscriptionBase.PollOnce classifies a failed poll — stop, retry, or drop. Sqlite shares that +/// base with Postgres and SQL Server but needs no container, so it is the cheapest place to pin the +/// classification, and the only one that runs where SQL Server's suite is excluded. +/// +/// Every test fails at the OpenConnection seam, so no database is involved. +public class PollFailureTests { + /// + /// A transient failure is the loop's own business. Escalating it would turn a blip into a full reconnect. + /// + [Test] + [Timeout(30_000)] + public async Task A_transient_failure_is_retried_without_reporting_a_drop(CancellationToken ct) { + var subscription = new PollingSubscription("sqlite-transient", (_, _) => throw new TimeoutException("the database was busy"), transient: true); + + var drops = 0; + await subscription.Subscribe(_ => { }, (_, _, _) => Interlocked.Increment(ref drops), ct); + + var retried = await WaitUntil(() => subscription.Polls > 3, TimeSpan.FromSeconds(10)); + + await subscription.Unsubscribe(_ => { }, ct); + + retried.ShouldBeTrue($"a transient failure should be retried inside the poll loop, it polled {subscription.Polls} time(s)"); + drops.ShouldBe(0, "a retry the loop handles itself must not be reported as a dropped subscription"); + } + + /// + /// Anything not transient propagates out of the loop, which the pump reads as this run's failure. Without + /// it the subscription stops consuming while still reporting healthy. + /// + [Test] + [Timeout(30_000)] + public async Task A_fatal_poll_failure_is_reported_as_a_drop(CancellationToken ct) { + var failure = new InvalidOperationException("no such table: eventuous.messages"); + var subscription = new PollingSubscription("sqlite-fatal", (_, _) => throw failure); + + DropReason? reason = null; + Exception? reported = null; + + await subscription.Subscribe(_ => { }, (_, r, e) => { reason = r; reported = e; }, ct); + + var dropped = await WaitUntil(() => reason != null, TimeSpan.FromSeconds(10)); + + await subscription.Unsubscribe(_ => { }, ct); + + dropped.ShouldBeTrue("a poll failure the provider can't retry has to reach the dropped callback, so health checks see it"); + reason.ShouldBe(DropReason.ServerError); + reported.ShouldBeSameAs(failure, "the cause must survive the trip out of the poll loop"); + } + + /// + /// The regression the token check guards: providers report unrelated aborts with a cancellation shape + /// (SQL Server's "Operation cancelled by user."), so one raised while nobody asked to stop is a real fault. + /// + /// + /// Trusting the shape alone ends the loop silently — the pump returns, nothing is recorded, and the + /// subscription reports healthy while consuming nothing. + /// + [Test] + [Timeout(30_000)] + public async Task A_cancellation_shaped_failure_with_no_stop_requested_is_still_a_drop(CancellationToken ct) { + var subscription = new PollingSubscription("sqlite-foreign-cancel", (_, _) => throw new OperationCanceledException("Operation cancelled by user.")); + + DropReason? reason = null; + await subscription.Subscribe(_ => { }, (_, r, _) => reason = r, ct); + + var dropped = await WaitUntil(() => reason != null, TimeSpan.FromSeconds(10)); + + await subscription.Unsubscribe(_ => { }, ct); + + dropped.ShouldBeTrue("a cancellation nobody asked for is a fault, and swallowing it leaves a silently dead subscription"); + reason.ShouldBe(DropReason.ServerError); + } + + /// + /// The other side of the check: once the run's token is cancelled, the poll's cancellation is our own + /// shutdown, not a drop. + /// + [Test] + [Timeout(30_000)] + public async Task A_cancellation_during_shutdown_is_not_reported_as_a_drop(CancellationToken ct) { + // Parks in the poll until the run token cancels, which is what a real read does during a clean stop. + var subscription = new PollingSubscription("sqlite-clean-stop", async (_, token) => await Task.Delay(Timeout.Infinite, token)); + + var drops = 0; + await subscription.Subscribe(_ => { }, (_, _, _) => Interlocked.Increment(ref drops), ct); + + (await WaitUntil(() => subscription.Polls >= 1, TimeSpan.FromSeconds(10))).ShouldBeTrue("the poll loop should have started"); + + await subscription.Unsubscribe(_ => { }, ct); + + drops.ShouldBe(0, "stopping is not dropping — a cancellation we asked for describes the shutdown"); + } + + static async Task WaitUntil(Func condition, TimeSpan timeout) { + var deadline = DateTime.UtcNow + timeout; + + while (DateTime.UtcNow < deadline) { + if (condition()) return true; + + await Task.Delay(20); + } + + return condition(); + } + + /// + /// Replaces the connection with whatever the test wants a poll to do. The connection string is required by + /// the base constructor but never used. + /// + sealed class PollingSubscription(string id, Func onPoll, bool transient = false) + : SqliteAllStreamSubscription( + new() { + SubscriptionId = id, + ConnectionString = "Data Source=:memory:", + // Short, so a retrying loop churns through attempts rather than sitting in backoff. + Retry = new() { InitialDelayMs = 1 }, + Polling = new() { MinIntervalMs = 1, MaxIntervalMs = 5 }, + RetryDelay = TimeSpan.FromMilliseconds(50) + }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new NoOpHandler()) + ) { + int _polls; + + public int Polls => Volatile.Read(ref _polls); + + protected override async ValueTask OpenConnection(CancellationToken cancellationToken) { + await onPoll(Interlocked.Increment(ref _polls), cancellationToken).ConfigureAwait(false); + + throw new InvalidOperationException("a poll that was meant to fail returned instead"); + } + + protected override bool IsTransient(Exception exception) => transient; + } + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } +} diff --git a/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/SubscriptionRestartTests.cs b/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/SubscriptionRestartTests.cs new file mode 100644 index 000000000..7cc73c376 --- /dev/null +++ b/src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/SubscriptionRestartTests.cs @@ -0,0 +1,104 @@ +using Eventuous.Sqlite.Subscriptions; +using Eventuous.Sut.App; +using Eventuous.Tests.Persistence.Base.Fixtures; +using Eventuous.Tests.Subscriptions.Base; +using static Eventuous.Sut.App.Commands; +using static Eventuous.Sut.Domain.BookingEvents; + +namespace Eventuous.Tests.Sqlite.Subscriptions; + +/// +/// The two properties every transport has to hold now that a drop stops the previous run before starting the +/// next: teardown runs on a connection that will be used again, and a transport whose resources are single-use +/// has to rebuild them in Connect rather than restart them. +/// +/// +/// Sqlite shares SqlSubscriptionBase with Postgres and SQL Server, which is where the largest transport +/// change in this rewrite landed — but it is the only one of the three that needs no container and runs on every +/// target framework, so it is both the cheapest place to catch a regression and the only one that would catch a +/// framework-specific one. It cannot reuse SubscriptionRestartBase, which is bound to a Docker container. +/// +[NotInParallel] +public class SubscriptionRestart() : SubscriptionTestBase(Fixture) { + static readonly SubscriptionFixture Fixture + = new(_ => { }, false); + + /// + /// Unsubscribing twice must not throw. The framework only tears a live run down, but a provider can still be + /// asked to release what it has already released — through an explicit stop, or a drop landing while + /// shutdown is in flight. + /// + [Test] + public async Task Sqlite_ShouldTolerateRepeatedUnsubscribe() { + await Fixture.StartSubscription(); + await Fixture.StopSubscription(); + await Fixture.StopSubscription(); + } + + /// + /// Subscribing again on the same instance after a full stop must consume newly produced events. Asserted by + /// event identity, so a replay of the first batch cannot pass for the second — which is what a + /// Connect that reused a spent reader would produce. + /// + [Test] + public async Task Sqlite_ShouldConsumeAfterResubscribe(CancellationToken cancellationToken) { + const int batch = 5; + + var started = false; + + try { + var first = (await GenerateAndHandleCommands(batch)).Select(ToEvent).ToList(); + await Fixture.StartSubscription(); + started = true; + await Assert.That(await WaitForEvents(first, cancellationToken)).IsTrue(); + + await Fixture.StopSubscription(); + started = false; + + await Fixture.StartSubscription(); + started = true; + + // Produced after the restart, so nothing the first run consumed can pass for them. + var second = (await GenerateAndHandleCommands(batch)).Select(ToEvent).ToList(); + var consumed = await WaitForEvents(second, cancellationToken); + + await Fixture.StopSubscription(); + started = false; + + await Assert.That(consumed).IsTrue(); + } finally { + if (started) { + try { await Fixture.StopSubscription(); } catch (Exception) { /* cleanup only */ } + } + } + } + + static async Task WaitForEvents(List expected, CancellationToken cancellationToken) { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(TimeSpan.FromSeconds(30)); + + try { + while (true) { + var handled = Fixture.Handler.Handled; + + if (expected.All(handled.Contains)) return true; + + await Task.Delay(200, cts.Token); + } + } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { return false; } + } + + static BookingImported ToEvent(ImportBooking cmd) => new(cmd.RoomId, cmd.Price, cmd.CheckIn, cmd.CheckOut); + + static async Task> GenerateAndHandleCommands(int count) { + var commands = Enumerable.Range(0, count).Select(_ => DomainFixture.CreateImportBooking()).ToList(); + var service = new BookingService(Fixture.EventStore); + + foreach (var cmd in commands) { + var result = await service.Handle(cmd, default); + result.ThrowIfError(); + } + + return commands; + } +}