refactor(engine): Improve context scope cleanup with ConcurrentBag and IAsyncDisposable (#1477)

[thomhurst]

Summary

• Change List<IServiceScope> to ConcurrentBag<IServiceScope> for thread safety
• Implement IAsyncDisposable with DisposeAsync() for proper async cleanup
• Handle both synchronous and asynchronous disposal patterns

Fixes #1477

Test Plan

• Build succeeds
• Unit tests pass

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR improves thread safety and async disposal in ModuleContextProvider by switching to ConcurrentBag and implementing IAsyncDisposable.

Critical Issues

⚠️ Breaking the IScopeDisposer interface contract

The PR adds IAsyncDisposable alongside IScopeDisposer, but this creates ambiguity about which disposal method will be called. Looking at the IScopeDisposer interface (src/ModularPipelines/Interfaces/IScopeDisposer.cs:8-26):

public interface IScopeDisposer : IDisposable
{
    IEnumerable<IServiceScope> GetScopes();
    
    void IDisposable.Dispose()
    {
        foreach (var serviceScope in GetScopes())
        {
            serviceScope.Dispose();
        }
    }
}

The interface already provides a default Dispose() implementation that calls GetScopes() and disposes each scope synchronously. When ModuleContextProvider is disposed via the IDisposable interface (which is the most common pattern for DI containers), the default synchronous Dispose() will be called, not DisposeAsync().

Problems:

1. If disposed via IDisposable, the new DisposeAsync() code is never executed
2. If disposed via IAsyncDisposable, it works, but callers must explicitly know to use async disposal
3. ConcurrentBag<T> doesn't guarantee enumeration order, so the default Dispose() implementation may iterate while GetModuleContext() is still adding scopes (race condition)

Solutions (choose one):

Option A: Update IScopeDisposer interface (Recommended)
Extend the IScopeDisposer interface itself to support async disposal:

public interface IScopeDisposer : IDisposable, IAsyncDisposable
{
    IEnumerable<IServiceScope> GetScopes();
    
    void IDisposable.Dispose()
    {
        foreach (var serviceScope in GetScopes())
            serviceScope.Dispose();
    }
    
    async ValueTask IAsyncDisposable.DisposeAsync()
    {
        foreach (var scope in GetScopes())
        {
            if (scope is IAsyncDisposable asyncDisposable)
                await asyncDisposable.DisposeAsync().ConfigureAwait(false);
            else
                scope.Dispose();
        }
    }
}

Then remove the DisposeAsync() override from ModuleContextProvider (it will inherit the default implementation).

Option B: Override the synchronous Dispose() method
Keep the async implementation but also override Dispose() to block on DisposeAsync():

void IDisposable.Dispose()
{
    // Explicitly override to use async disposal
    DisposeAsync().AsTask().GetAwaiter().GetResult();
}

⚠️ Thread safety concern with ConcurrentBag enumeration

While ConcurrentBag.Add() is thread-safe, enumerating it via GetScopes() while additions are happening can miss items or see duplicates. Since the default IScopeDisposer.Dispose() calls GetScopes(), there's still a potential race between scope creation and disposal.

Consider adding a volatile flag or lock to prevent disposal while scopes are still being created, or ensure the disposal only happens after all scope creation is complete.

Suggestions

None.

Verdict

⚠️ REQUEST CHANGES - Critical interface contract issue must be addressed to ensure scopes are properly disposed via both sync and async paths.

[Copilot]

Pull request overview

This PR refactors the ModuleContextProvider to improve thread safety and async disposal patterns by changing from List<IServiceScope> to ConcurrentBag<IServiceScope> and implementing IAsyncDisposable.

Key Changes

• Replaces List<IServiceScope> with ConcurrentBag<IServiceScope> for thread-safe scope collection
• Adds IAsyncDisposable interface implementation with DisposeAsync() method
• Implements async disposal logic that handles both async and sync disposable scopes

[Copilot]

The DisposeAsync implementation has a critical concurrency issue. While DisposeAsync iterates over the ConcurrentBag, GetModuleContext can still be adding new scopes to it. This creates a race condition where:

1. Scopes added during disposal may not be disposed
2. The iteration over ConcurrentBag is not thread-safe when concurrent modifications occur

Additionally, this implementation is missing several important elements of proper async disposal:

• No mechanism to prevent concurrent disposal calls
• Missing GC.SuppressFinalize(this) to suppress finalization
• No coordination to prevent new scopes from being added during disposal

Consider implementing a proper disposal pattern with a disposed flag and synchronization, or ensure that disposal only occurs when no concurrent calls to GetModuleContext are possible.

[Copilot]

The type check for IAsyncDisposable is unnecessary. CreateAsyncScope returns AsyncServiceScope, which always implements IAsyncDisposable. This check adds complexity without providing any benefit and will always take the true branch.

Consider simplifying this to directly call DisposeAsync on each scope without the conditional check.

Suggested change

	if (scope is IAsyncDisposable asyncDisposable)
	{
	await asyncDisposable.DisposeAsync().ConfigureAwait(false);
	}
	else
	{
	scope.Dispose();
	}
	await ((IAsyncDisposable)scope).DisposeAsync().ConfigureAwait(false);

[Copilot]

The IScopeDisposer interface provides a default implementation of IDisposable.Dispose() that synchronously disposes scopes. However, with this class now also implementing IAsyncDisposable, there's a potential conflict:

1. The synchronous Dispose from IScopeDisposer will call scope.Dispose() on each scope
2. The asynchronous DisposeAsync will call scope.DisposeAsync() on each scope
3. If both are called, scopes could be disposed twice

Additionally, the IScopeDisposer.Dispose implementation doesn't coordinate with DisposeAsync, creating potential race conditions. Consider whether both disposal patterns are needed, or implement proper coordination between them with a disposed flag.

[thomhurst]

Summary

This PR improves thread safety and resource cleanup in ModuleContextProvider by using ConcurrentBag and implementing proper async disposal.

Critical Issues

1. Race condition between GetModuleContext and disposal

Location: ModuleContextProvider.cs:19-24, 33-60

There's a race condition window between checking _disposed and taking a snapshot of scopes:

// Thread A (DisposeAsync)
lock (_disposeLock) {
    _disposed = true;  // Sets flag
}
// Lock released - Thread B can now enter GetModuleContext

// Thread B (GetModuleContext) 
ObjectDisposedException.ThrowIf(_disposed, this);  // Throws

// Thread A continues
var scopesToDispose = _scopes.ToArray();  // Takes snapshot

However, there's a subtle issue: if Thread B passes the check just before Thread A sets _disposed = true, it will add to _scopes after the snapshot is taken in DisposeAsync. This newly added scope would not be disposed.

Fix: Move the snapshot inside the lock to ensure atomicity:

public async ValueTask DisposeAsync()
{
    IServiceScope[] scopesToDispose;
    
    lock (_disposeLock)
    {
        if (_disposed)
        {
            return;
        }
        _disposed = true;
        scopesToDispose = _scopes.ToArray();  // Snapshot inside lock
    }

    foreach (var scope in scopesToDispose)
    {
        await ((IAsyncDisposable)scope).DisposeAsync().ConfigureAwait(false);
    }

    GC.SuppressFinalize(this);
}

Apply the same pattern to the synchronous Dispose() method.

2. Potential ObjectDisposedException leak in GetScopes()

Location: ModuleContextProvider.cs:30-38

The GetScopes() method returns an empty array when disposed to prevent double-disposal. However, this silently swallows the disposed state. Since IScopeDisposer.Dispose() (defined in the interface with a default implementation) calls GetScopes(), it won't properly dispose scopes if called.

Impact: If someone calls the interface's default Dispose() method instead of the explicit implementation, scopes won't be disposed.

Recommendation: Either:

1. Remove the early return and let it throw, OR
2. Document why returning empty is safe (because the explicit IDisposable.Dispose is what gets called)

Given the explicit IDisposable.Dispose implementation, option 2 seems intended, but needs a comment explaining this.

Suggestions

None - the critical issues above must be addressed first.

Verdict

⚠️ REQUEST CHANGES - Critical race condition and disposal semantics issues found

[thomhurst]

Summary

Adds thread-safe scope management with ConcurrentBag and implements IAsyncDisposable for proper async cleanup of service scopes.

Critical Issues

1. Race condition between GetModuleContext() and disposal (src/ModularPipelines/Engine/ModuleContextProvider.cs:21)

There's a race condition where a thread could pass the _disposed check in GetModuleContext() (line 22), then get suspended before adding to _scopes (line 26). Meanwhile, another thread enters disposal, takes the snapshot, and completes. The first thread then resumes and adds a scope that will never be disposed.

Suggested fix: Move the _disposed check inside the lock to ensure atomicity.

2. Double disposal via IScopeDisposer.Dispose() (src/ModularPipelines/Interfaces/IScopeDisposer.cs:19-25)

The IScopeDisposer interface has a default implementation of Dispose() that calls GetScopes() and disposes each scope. Your explicit IDisposable.Dispose() implementation also disposes all scopes. While you check _disposed in GetScopes() and return empty array (line 33-36), this coordination is fragile.

Suggested fix: Consider documenting this coordination or restructuring to make it clearer.

3. Missing ConfigureAwait(false) on AsyncServiceScope disposal (line 59)

The cast to IAsyncDisposable should be followed by ConfigureAwait(false) for consistency with the rest of the codebase's async patterns.

Verdict

⚠️ REQUEST CHANGES - Critical race condition in issue #1

[thomhurst]

Summary

Adds thread-safe disposal with ConcurrentBag and IAsyncDisposable to fix scope cleanup in ModuleContextProvider.

Critical Issues

None found ✅

Suggestions

Consider ConfigureAwait consistency

The DisposeAsync() method uses .ConfigureAwait(false) for async disposal calls (line 53), which is good. However, consider whether the explicit IDisposable.Dispose() implementation (lines 60-82) should be calling synchronous Dispose() on scopes that may have been created via CreateAsyncScope().

Context: AsyncServiceScope always implements both IDisposable and IAsyncDisposable. When disposing synchronously (line 77), calling the sync path on a scope that was created for async disposal works but may not properly dispose async resources.

Recommendation: Document this design decision with a comment, or consider making the class only implement IAsyncDisposable and letting consumers handle sync disposal (though this may break existing code). The current approach is pragmatic and functional.

Lock-free alternative consideration

While the current lock-based approach (lines 22-32, 34-47, 62-74) is correct and safe, ConcurrentBag is already thread-safe for concurrent adds. The primary concern is the race between checking _disposed and adding to the bag.

The current implementation is sound - this is just noting that an alternative approach using Interlocked.CompareExchange on the _disposed flag could reduce lock contention, but the added complexity may not be worth it given that disposal is a one-time operation.

Verdict

✅ APPROVE - No critical issues

The implementation correctly addresses issue #1477 with proper thread safety, disposal patterns, and race condition handling. The explicit IDisposable.Dispose() correctly overrides the default interface implementation to coordinate with DisposeAsync() and prevent double disposal via the _disposed flag check in GetScopes().