Skip to content

EventSubscription.Unsubscribe is not idempotent #564

Description

@alexeyzimarev

Spotted while fixing the shutdown dispose race in #562 / #563. Not known to be causing failures today, but it's the same family and the current safety depends on an exception being swallowed.

Describe the bug

EventSubscription.Unsubscribe disposes Stopping as its last act (EventSubscription.cs:76):

public async ValueTask Unsubscribe(OnUnsubscribed onUnsubscribed, CancellationToken cancellationToken) {
    IsRunning = false;
    await Unsubscribe(cancellationToken).NoContext();
    Log.SubscriptionStopped();
    onUnsubscribed(Options.SubscriptionId);
    await Finalize(cancellationToken);
    Sequence = 0;
    Stopping.Dispose();
}

A second call re-enters the concrete Unsubscribe, and the KurrentDB subscriptions cancel Stopping as their first statement — on a disposed source, which throws ObjectDisposedException:

Each of those bodies is wrapped in catch (Exception) { }, so the throw is currently invisible. Two consequences:

  1. The throw short-circuits the rest of the concrete teardown (_subscription?.Dispose(), draining the message pump). Harmless on a second call where that work is already done, but it means the blanket catch is load-bearing rather than incidental.
  2. onUnsubscribed is invoked again, so anything counting unsubscribes (health reporting) sees it twice.

Expected behaviour

A second Unsubscribe should be a no-op, the way DisposeAsync already is via its _disposed flag (EventSubscription.cs:229).

To reproduce

await subscription.Unsubscribe(_ => { }, ct) twice on any KurrentDB subscription. Nothing surfaces because of the blanket catch; remove it and the second call throws ObjectDisposedException: The CancellationTokenSource has been disposed.

Notes

Unsubscribe is public API (IMessageSubscription), so callers other than SubscriptionHostedService.StopAsync can reach this. Guarding on IsRunning isn't sufficient on its own — the flag is set to false before the async teardown runs, so two overlapping calls would both get past it. A dedicated flag interlocked like the one in ChannelWorkerBase.DisposeAsync would be consistent with how #562 solved the equivalent problem one layer down.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions