refactor: rewrite BrighterAsyncContext for correctness and modernization

[thomhurst]

Summary

Rewrites BrighterAsyncContext and its collaborators in src/Paramore.Brighter/Tasks/ to fix correctness bugs inherited from the AsyncEx-derived original implementation and modernize the code for current .NET conventions. Adds a thorough test suite covering shutdown races, concurrent-Execute, nested Run, and a 1000-iteration stress test.

Correctness fixes

• Outstanding-operations counter leak — Enqueue now balances the counter when TryAdd fails on a closed queue. The previous code left the counter stuck and Run would hang forever.
• Late Post after shutdown — BrighterTaskQueue.TryAdd / CompleteAdding / GetScheduledTasks tolerate both InvalidOperationException (completed-for-adding) and ObjectDisposedException (queue disposed), in the correct subclass-first catch order. Stray async continuations posting back after Run returns are absorbed silently.
• Late Send after shutdown — now throws ObjectDisposedException instead of blocking indefinitely on a task the pump will never execute.
• Exception swallowing in Run(Func<Task>) / Run<T>(Func<Task<T>>) — continuation now does try { WaitAndUnwrapException } finally { OperationCompleted }, so OperationCompleted cannot mask the user's exception.
• Concurrent Execute — enforces the single-thread invariant via Interlocked.CompareExchange, throwing InvalidOperationException on re-entry instead of silently violating it.
• Idempotent Dispose — guarded by Interlocked.Exchange(ref _disposed, 1); double-dispose no longer throws ObjectDisposedException from the underlying BlockingCollection.
• GetScheduledTasks — takes a ToArray() snapshot rather than iterating the live BlockingCollection (which is undocumented against concurrent Add/Take), and absorbs ObjectDisposedException.
• TaskExtensions.WaitAndUnwrapException(CancellationToken) — uses Task.WaitAsync(ct) on net6+; on netstandard2.0 unwraps via ex.InnerException (the previous code captured the wrapping AggregateException instead, double-wrapping the thrown exception).

Modernization

• BrighterAsyncContext and BrighterSynchronizationContext are now sealed.
• Enqueue, OperationStarted, OperationCompleted, GetScheduledTasks on BrighterAsyncContext moved from public to internal.
• Tuple<Task, bool> replaced with (Task, bool) ValueTuple on the hot path.
• Static lambdas with state params eliminate per-Enqueue closure allocations.
• ArgumentNullException.ThrowIfNull under #if NET with netstandard2.0 fallbacks.
• XML docs rewritten for accuracy; MIT attribution block added to TaskExtensions.
• Removed dead code: ExecuteImmediately, TryExecuteNewThread, BrighterSynchronizationContext.Timeout, OutstandingOperations setter.

⚠️ Breaking changes (major bump)

• BrighterAsyncContext and BrighterSynchronizationContext are now sealed.
• Enqueue, OperationStarted, OperationCompleted, GetScheduledTasks went from public to internal.
• ExecuteImmediately removed.
• BrighterSynchronizationContext.Timeout property removed.
• OutstandingOperations setter removed (getter kept and now returns a correct Volatile.Read of the internal counter; before, the setter was a dead auto-prop that always returned 0).

No in-repo consumers use any of the removed/tightened surfaces beyond Run(...) and Current.

Test coverage

Added (6 cases):

• Post_AfterContextDisposed_DoesNotThrow
• Send_AfterContextDisposed_Throws
• Send_AfterShutdown_StressIterations_NeverHangsAndOnlyThrowsObjectDisposed — 1000 iterations, 5s bounded wait per iteration
• Dispose_CalledTwice_DoesNotThrow
• Run_NestedInsideOuterRun_DoesNotDeadlock
• Execute_CalledConcurrently_ThrowsInvalidOperationException

Test plan

• Build clean across all TFMs: netstandard2.0, net8.0, net9.0, net10.0.
• BrighterSynchronizationContextsTests — 31/31 passed in ~1s.
• Proactor regression smoke — 38/38 passed in ~30s.

Commits

1. build: bump OpenTelemetry packages to 1.15.3 for GHSA-g94r-2vxg-569j — CVE fix required for master to restore. Happy to split into a separate PR if preferred.
2. refactor: rewrite BrighterAsyncContext for correctness and modernization — the rewrite itself.

Review process

This PR was developed with multiple review passes (code-reuse, code-quality, efficiency, and three thorough correctness passes). Each pass surfaced specific findings that were addressed before the next. Review trail visible in the commit history if useful.

[iancooper]

Thanks @thomhurst I'll take a peek.

[iancooper]

Thanks @thomhurst. I was a aware of a couple of cases where we seemed to block the thread, so its great that you managed to track them down. The additional operation is an interesting solution to one of the problems

[thomhurst]

Code review

Found 1 issue:

1. BrighterAsyncContext.Enqueue attaches the OperationCompleted continuation before calling _taskQueue.TryAdd. If TryAdd fails, the code calls OperationCompleted() manually to balance the counter, but the ContinueWith is still wired to the task. The stranded task can later be run via BrighterTaskScheduler.TryExecuteTaskInline (e.g. a caller doing task.Wait() while BrighterAsyncContext.Current == _asyncContext), which fires the continuation a second time and decrements _outstandingOperations below zero. The next OperationCompleted() that hits zero in normal flow will spuriously call CompleteAdding() while operations are still in flight, closing the queue prematurely.

   TryExecuteTaskInline only checks BrighterAsyncContext.Current == _asyncContext; it does not require the task to be in the queue:

   Brighter/src/Paramore.Brighter/Tasks/BrighterTaskScheduler.cs

   Lines 58 to 67 in 4a2523e

   	protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued)
   	{
   	#if DEBUG_CONTEXT
   	Debug.IndentLevel = 1;
   	Debug.WriteLine($"BrighterTaskScheduler: TryExecuteTaskInline on thread {Thread.CurrentThread.ManagedThreadId}");
   	Debug.IndentLevel = 0;
   	#endif
   	return BrighterAsyncContext.Current == _asyncContext && TryExecuteTask(task);
   	}

   Enqueue ordering (continuation attached before TryAdd, manual decrement on failure):

   Brighter/src/Paramore.Brighter/Tasks/BrighterAsyncContext.cs

   Lines 109 to 146 in 4a2523e

   	/// </summary>
   	public static BrighterAsyncContext? Current =>
   	(SynchronizationContext.Current as BrighterSynchronizationContext)?.AsyncContext;
   	/// <summary>
   	/// Enqueues a task for execution by this context.
   	/// </summary>
   	/// <param name="task">The task to enqueue.</param>
   	/// <param name="propagateExceptions">Whether to propagate exceptions back to <see cref="Execute"/>.</param>
   	internal void Enqueue(Task task, bool propagateExceptions)
   	{
   	#if DEBUG_CONTEXT
   	Debug.IndentLevel = 1;
   	Debug.WriteLine($"BrighterAsyncContext: Enqueueing task {task.Id} on thread {Thread.CurrentThread.ManagedThreadId} for context {Id}");
   	Debug.IndentLevel = 0;
   	#endif
   	OperationStarted();
   	// Attach completion continuation before queueing. The task cannot start until the
   	// scheduler pulls it from the queue, so no completion race is possible here.
   	task.ContinueWith(
   	static (_, state) => ((BrighterAsyncContext)state!).OperationCompleted(),
   	this,
   	CancellationToken.None,
   	TaskContinuationOptions.ExecuteSynchronously,
   	_taskScheduler);
   	if (!_taskQueue.TryAdd(task, propagateExceptions))
   	{
   	// Queue is already completed for adding: the task will never be pulled, so the
   	// continuation will never fire. Balance the outstanding-operations counter
   	// manually so callers of Execute do not hang. Record shutdown so Send can
   	// distinguish "pump will process this task" from "task will never run".
   	Volatile.Write(ref _shutdown, 1);
   	OperationCompleted();
   	}
   	}

   Suggested fix: detach/cancel the continuation on TryAdd failure, or attach it only after a successful TryAdd (with the task observed for completion separately).

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[codescene-delta-analysis]

Code Health Improved (1 files improve in Code Health)

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

View Improvements

File	Code Health Impact	Categories Improved
BrighterAsyncContext.cs	9.10 → 9.39	Code Duplication

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.