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 new file mode 100644 index 000000000..ca226ec27 --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionRestartBase.cs @@ -0,0 +1,99 @@ +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; + +/// +/// 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 + ) : 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, even though a provider may be asked to release resources it has + /// already released. + /// + protected async Task ShouldTolerateRepeatedUnsubscribe() { + await fixture.StartSubscription(); + await fixture.StopSubscription(); + await fixture.StopSubscription(); + } + + /// + /// 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) { + 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/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 new file mode 100644 index 000000000..1c4fe41bf --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeConcurrencyTests.cs @@ -0,0 +1,889 @@ +using System.Collections.Concurrent; +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Checkpoints; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Eventuous.Tools; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// 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 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 CapturingLoggerFactory(LogLevel.Trace); + + 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 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(run, start + (ulong)i).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 Wait.Until(() => handler.HandledCount >= messageCount, TimeSpan.FromSeconds(5))) + .ShouldBeTrue($"all {messageCount} messages should have been handled and nacked, got {handler.HandledCount}"); + + (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); + + 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 CapturingLoggerFactory(LogLevel.Trace); + 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, run) => { + if (transport.Index > 0) { + for (var i = start; i <= last && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); + + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); + + return; + } + + 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 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. + run.Fail(DropReason.SubscriptionError, new IOException("The HTTP/2 server closed the connection")); + + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(300) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + var recovered = await Wait.Until( + 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"); + + 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"); + + 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 = 15; + 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 CapturingLoggerFactory(LogLevel.Trace), + concurrencyLimit: inFlight, + 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 && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); + + await Task.Delay(Timeout.Infinite, run.Token).NoContext(); + + return; + } + + for (var i = start; i < start + inFlight && !run.Token.IsCancellationRequested; i++) await sub.Deliver(run, i, transport.Index).NoContext(); + + await Wait.Until(() => handler.InFlight == inFlight, TimeSpan.FromSeconds(5)).NoContext(); + handler.KillConnection(transport.Index); + + 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 Wait.Until( + 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 CapturingLoggerFactory(LogLevel.Trace), + concurrencyLimit: 1, + pump: async (sub, transport, start, run) => { + var position = start; + + while (!run.Token.IsCancellationRequested) { + deliveries.Enqueue((transport.Index, position)); + await sub.Deliver(run, position++).NoContext(); + await Task.Delay(5, run.Token).NoContext(); + } + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(100) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (await Wait.Until(() => deliveries.Count > 3, TimeSpan.FromSeconds(5))).ShouldBeTrue("the first pump should be delivering"); + + subscription.Drop(new InvalidOperationException("Simulated transport drop")); + + (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 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"); + } + + /// + /// 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(CancellationToken ct) { + const int threads = 8; + const int perThread = 2000; + + var subscription = new PumpingSubscription( + new() { SubscriptionId = "unique-sequences" }, + new NoOpCheckpointStore(), + new ConsumePipe().AddDefaultConsumer(new CountingHandler()), + new CapturingLoggerFactory(LogLevel.Trace), + concurrencyLimit: 1, + 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( + Enumerable.Range(0, threads) + .Select(t => Task.Run(() => { + var mine = new ulong[perThread]; + + for (var i = 0; i < perThread; i++) mine[i] = run.NextSequence(); + + results[t] = mine; + } + ) + ) + ); + + await subscription.Unsubscribe(_ => { }, ct); + + 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 just before it, and nothing accumulates. + /// + [Test] + public async Task Repeated_nacks_hold_a_stable_checkpoint_without_accumulating(CancellationToken ct) { + // 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(); + 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 CapturingLoggerFactory(LogLevel.Trace), + concurrencyLimit: 1, + 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, run.Token).NoContext(); + } + ) { ResubscribeDelay = TimeSpan.FromMilliseconds(10) }; + + await subscription.Subscribe(_ => { }, (_, _, _) => { }, ct); + + (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); + + // 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); + + // 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"); + + 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"); + } + + /// + /// 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) { + 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 CapturingLoggerFactory(LogLevel.Trace), + concurrencyLimit: 1, + 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, 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 Wait.Until( + 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 Wait.Until( + 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"); + } + + + /// + /// 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(); + + 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 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(); + + 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, tracking disposal and whether its pump has exited. + /// + 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: 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 + ) + : EventSubscriptionWithCheckpoint(options, checkpointStore, pipe, concurrencyLimit, SubscriptionKind.All, loggerFactory, null, null) { + readonly ConcurrentQueue _transports = []; + + 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); + + /// + /// 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; + } + + /// + /// 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", + "application/json", + "test-stream", + position, + position, + position, + run.NextSequence(), + DateTime.UtcNow, + new { Position = position }, + new(), + Options.SubscriptionId, + CancellationToken.None + ) { LogContext = Log } + .WithItem(TransportKey, transport); + + public ValueTask Deliver(SubscriptionRun run, ulong position, int transport = 0) { + var context = CreateContext(run, position, transport); + context.CancellationToken = run.Token; + + return HandleInternal(run, context); + } + + 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); + + var (_, position) = await GetCheckpoint(run).NoContext(); + var start = position == null ? 0 : position.Value + 1; + + var attempt = Interlocked.Increment(ref _subscribeCalls) - 1; + + if (FailStart?.Invoke(attempt) is { } failure) throw failure; + + var transport = new FakeTransport(attempt); + _transports.Enqueue(transport); + + if (WhileStarting != null) await WhileStarting().NoContext(); + + // 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); + + // Dispose then join, same order teardown always used, just one release instead of two steps. + run.OnDisconnect(async _ => { + if (WhileStopping != null) await WhileStopping().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() { + 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 is 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 subscription's own connection, so a connection death turns into 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); + } + + /// + /// 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); + + int _parkedOnce; + + public Task Parked => _parked.Task; + + public void Release() => _release.TrySetResult(); + + public override async ValueTask HandleEvent(IMessageConsumeContext context) { + if (Interlocked.CompareExchange(ref _parkedOnce, 1, 0) != 0) return EventHandlingStatus.Success; + + _parked.TrySetResult(); + + // Deliberately not watching the context token, or this would abandon the message instead of acking it late. + await _release.Task; + + 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 new file mode 100644 index 000000000..d4476fe9d --- /dev/null +++ b/src/Core/test/Eventuous.Tests.Subscriptions/TransportTeardownTests.cs @@ -0,0 +1,60 @@ +using Eventuous.Subscriptions; +using Eventuous.Subscriptions.Context; +using Eventuous.Subscriptions.Filters; +using Shouldly; + +namespace Eventuous.Tests.Subscriptions; + +/// +/// 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] + 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, without holding any real resources of its own. + /// + sealed class CountingSubscription() + : EventSubscription( + new() { SubscriptionId = "transport-teardown" }, + new ConsumePipe().AddDefaultConsumer(new NoOpHandler()), + null, + null + ) { + int _stops; + + public int Stops => Volatile.Read(ref _stops); + + protected override ValueTask Connect(SubscriptionRun run) { + run.OnDisconnect(_ => { Interlocked.Increment(ref _stops); return default; }); + + return default; + } + } + + sealed class NoOpHandler : BaseEventHandler { + public override ValueTask HandleEvent(IMessageConsumeContext context) => new(EventHandlingStatus.Success); + } +} 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/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/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/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 new file mode 100644 index 000000000..c2981e6d1 --- /dev/null +++ b/src/Redis/test/Eventuous.Tests.Redis/Subscriptions/PollFailureTests.cs @@ -0,0 +1,92 @@ +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, so these tests fail at the ReadEvents seam and need no server. +/// +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/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/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); + } +} 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; + } +}