[Bug] Outstanding message check: a count that never returns freezes the limit check, and the static semaphore spreads it to other mediators #4584

Describe the bug

@iancooper, this is the issue I said I'd open on PR #4552. PR #4564 and PR #4569 have merged, and PR #4583, the last one for issue #4554, is open. PR #4569 fixed the checks piling up in a burst, so this only covers what is left of the points you raised there: the static semaphore and the missing timeout and cancellation on the count. You said it might be worth its own issue, as it may bite at scale.

OutboxProducerMediator runs the count of outstanding messages inside a Task.Run, where it waits on s_checkOutstandingSemaphoreToken, a static SemaphoreSlim(1, 1), with no timeout. The mediator is generic, so that semaphore is shared by every mediator with the same message and transaction types. The mediator doesn't bound the count either: IAmAnOutboxSync.GetOutstandingMessageCount takes no token, the async count is called without one, and the async branch blocks on it with GetAwaiter().GetResult().

A count that never returns is not far-fetched. RelationDatabaseOutbox counts through OutstandingMessages, which builds its query with a timeout of -1, and CreateCommand turns that into a CommandTimeout of 0, which ADO.NET treats as no limit. So the count waits as long as the database does, for example behind a lock. The Spanner outbox overrides this with 60 seconds.

If a count never returns, that mediator never counts again. Its in-flight flag from PR #4569 stays claimed until the check ends, and before PR #4569 every later check would have blocked on the semaphore anyway. The outstanding count keeps its last value, so the limit is checked against a stale number: if it was over the limit, every post throws OutboxLimitReachedException from then on, and if it was under, posts never throw however much the outbox grows.

Any other mediator with the same types and a limit set then blocks a pool thread on the semaphore at its next check, and stops counting too. With a single AddProducers there is one mediator per pair of types, so this part needs more than one in the process, like several hosts or command processors built by hand.

To Reproduce

The PR #4569 test When_a_burst_of_posts_arrives_before_the_first_check_starts_should_queue_one_outstanding_count already relies on this: it holds a count open in one mediator, and while that count is held the other mediator can't start its first count.

To see it on its own, on master (cbd2d11), build two command processors whose mediators have the same message and transaction types, each with its own outbox, a limit set and a check interval of zero, which is what AddProducers uses. The first outbox's count blocks until it is released.

1. Post through the first one, so its count starts and blocks.
2. Post through the second one.

The second count doesn't start while the first one is blocked. I checked it with the CountingOutbox test double, holding the first count: the second outbox got no count call in 3 seconds, and its count started once I released the first one.

Possible fixes

If you want me to take this, I'd like to know which of these you want first. They fix different parts, so more than one can go together.

Dropping the semaphore, or making it per mediator, is the smallest change. Since PR #4569 a mediator never runs two counts at once, so the semaphore only serialises counts across mediators, and without it a hung count would only stop its own mediator. The two PR #4569 tests that hold a count with a second mediator rely on the shared semaphore, so they would need another way to keep a check waiting. PR #4242 made the clear semaphore per mediator for a similar problem.

Bounding the count is what gets a mediator with a hung count checking again. Every outbox that ships with Brighter also implements the sync interface, so with any of them the mediator counts through the sync path, and there the only bound is the one the outbox sets itself. The DynamoDB outbox already sets one: its count gives up after its configured timeout, 500 ms by default, and lands in the mediator's catch, which sets the count to 0. The relational outboxes could give the count's query a real CommandTimeout in the same way. Passing a token with a timeout to the async count would only help an outbox that is async only. Since a count that times out is read as 0, we'd also need to decide whether it should keep the last value instead. On issue #4573 you chose to detect a hung sweep rather than cancel it, so if you'd rather not bound the count either, dropping the semaphore at least keeps a hang inside one mediator.

Making the check async, the first option you gave on PR #4552, would free the pool thread that a waiting check holds. With one check per mediator that is at most one thread each, and it doesn't help with a hang.

This is also where my question on issue #4554 fits: is it intentional that the semaphore is shared across mediators?

Exceptions (if any)

None. The check catches and logs exceptions from the count, but a call that never returns never gets there.

Further technical details

• Brighter: master at cbd2d11. The static semaphore and the count with no timeout are the same on 10.8.0.
• I confirmed it in the code and checked it as above, on .NET SDK 10.0.401, Windows 11.