[C#] Use event delegate for thread-safe, insertion-ordered event handler dispatch

[Copilot]

_eventHandlers was a HashSet<SessionEventHandler>, which is not thread-safe. Concurrent calls to On(...) or disposal of subscriptions while DispatchEvent was iterating could corrupt the set. The .ToArray() snapshot in DispatchEvent partially mitigated iteration safety but didn't protect the HashSet itself from concurrent mutation, and it didn't guarantee invocation order.

Changes

Replace HashSet<SessionEventHandler> with a private event SessionEventHandler? (multicast delegate):

• Thread-safe add/remove: The compiler-generated +=/-= accessors use a lock-free Interlocked.CompareExchange CAS loop.
• Insertion-ordered dispatch: Delegate.Combine appends to the invocation list, so handlers are always called in registration order. This eliminates a class of bugs where handler-ordering assumptions (e.g., an event logger running before a TaskCompletionSource-based waiter) depend on the collection's enumeration order.
• Inherent snapshot semantics: Delegates are immutable — DispatchEvent reads the field once and invokes it, so handlers added/removed during dispatch don't affect the current iteration. No .ToArray() allocation needed.
• DisposeAsync: .Clear() replaced with = null

[Copilot]

Pull request overview

This PR improves the thread-safety of .NET session event handler registration/dispatch in CopilotSession by replacing a non-thread-safe HashSet with a ConcurrentDictionary used as a set, reducing the risk of collection corruption under concurrent subscribe/unsubscribe while events are being dispatched.

Changes:

• Replaced _eventHandlers from HashSet<SessionEventHandler> to ConcurrentDictionary<SessionEventHandler, bool>.
• Updated On(...) to use TryAdd/TryRemove for thread-safe subscription management.
• Updated DispatchEvent(...) to iterate the concurrent collection without the prior .ToArray() snapshot.

Comments suppressed due to low confidence (2)

dotnet/src/Session.cs:265

• ConcurrentDictionary enumeration is thread-safe but is not guaranteed to be a stable snapshot; concurrent On(...) / disposal during dispatch can cause handlers to be skipped or newly-added handlers to be invoked mid-dispatch. If dispatch semantics should be consistent per event, prefer enumerating a snapshot (e.g., _eventHandlers.Keys snapshot) or explicitly snapshotting keys before invoking handlers.

    internal void DispatchEvent(SessionEvent sessionEvent)
    {
        foreach (var (handler, _) in _eventHandlers)
        {
            // We allow handler exceptions to propagate so they are not lost
            handler(sessionEvent);

dotnet/src/Session.cs:250

• This change addresses a concurrency bug, but there’s no regression test exercising concurrent subscribe/unsubscribe while DispatchEvent runs. Adding a targeted test (even a bounded stress test) would help catch future regressions and validate the thread-safety guarantee of On(...) + disposal during event dispatch.

    public IDisposable On(SessionEventHandler handler)
    {
        _eventHandlers.TryAdd(handler, true);
        return new OnDisposeCall(() => _eventHandlers.TryRemove(handler, out _));

[github-actions]

Cross-SDK Consistency Review: Thread Safety

This PR correctly addresses a thread-safety issue in the .NET SDK by using ConcurrentDictionary for event handler storage. I've reviewed the equivalent code across all four SDK implementations:

Current Thread Safety Status

SDK	Thread-Safe?	Implementation
.NET	✅ Yes (after this PR)	ConcurrentDictionary(SessionEventHandler, bool) with TryAdd/TryRemove
Go	✅ Yes	sync.RWMutex protecting slice operations in On() and dispatchEvent()
Python	✅ Yes	threading.Lock protecting set operations in on() and _dispatch_event()
Node.js/TypeScript	❌ No	Plain Set(SessionEventHandler) with no synchronization

Consistency Gap

The Node.js/TypeScript SDK (nodejs/src/session.ts) has the same thread-safety vulnerability that this PR fixes for .NET:

• Line 59: private eventHandlers: Set(SessionEventHandler) = new Set();
• Lines 279-281: Direct add() and delete() operations on the Set
• Line 305: Iteration over the Set during dispatch

While JavaScript's single-threaded event loop makes this less likely to cause issues in typical use cases, the SDK could still experience race conditions if:

• Event handlers are added/removed during async operations
• Multiple async tasks manipulate handlers concurrently
• The SDK is used in worker threads or other concurrent contexts

Recommendation

Consider applying a similar thread-safety improvement to the Node.js SDK. Options include:

1. Taking a snapshot before iteration (similar to the old .NET code's .ToArray())
2. Using a more sophisticated synchronization approach if concurrent handler modification is expected
3. Documenting that handler registration/unregistration should not occur during event dispatch

This would ensure consistent thread-safety guarantees across all SDK implementations.

---

Note: This PR itself is excellent and correctly fixes the .NET SDK. The Node.js SDK issue is pre-existing but worth addressing for cross-SDK consistency.

AI generated by SDK Consistency Review Agent

[SteveSandersonMS]

@stephentoub I'm curious about whether you think Session should be thread-safe in general. From this change I'm guessing currently it isn't. But what would be the use cases for which we should change it to be thread-safe?

[github-actions]

Cross-SDK Consistency Review ✅

I've reviewed this PR for consistency across all SDK implementations (Node.js/TypeScript, Python, Go, and .NET).

Summary

This PR appropriately addresses a thread-safety issue specific to .NET. The change from HashSet to ConcurrentDictionary is the correct fix for .NET, and no changes are needed in the other SDKs because they already handle concurrency correctly for their respective runtime environments.

Analysis by SDK

✅ .NET (this PR):

• Before: HashSet with .ToArray() snapshot - not fully thread-safe
• After: ConcurrentDictionary(SessionEventHandler, bool) - fully thread-safe
• Status: Fixed correctly ✓

✅ Go:

• Current implementation: []sessionHandler slice protected by sync.RWMutex
• In dispatchEvent() (line 459-465), uses RLock() to create a snapshot before iterating
• Status: Already thread-safe, appropriate for Go ✓

✅ Python:

• Current implementation: set protected by threading.Lock
• In _dispatch_event() (line 235-250), acquires lock and creates list() snapshot before iterating
• Status: Already thread-safe, appropriate for Python ✓

✅ Node.js/TypeScript:

• Current implementation: Set(SessionEventHandler) with no locks
• JavaScript is single-threaded with an event loop - concurrent modification during iteration is not possible in the same way as multi-threaded environments
• Status: Appropriate for JavaScript's concurrency model ✓

Conclusion

Each SDK uses thread-safety mechanisms appropriate to its language and runtime:

• .NET: Now uses ConcurrentDictionary (this PR) ✓
• Go: Uses sync.RWMutex with read locks and snapshots ✓
• Python: Uses threading.Lock with snapshots ✓
• Node.js: Relies on single-threaded event loop ✓

No cross-SDK consistency issues detected. The changes maintain appropriate patterns for each language's concurrency model.

AI generated by SDK Consistency Review Agent

[stephentoub]

@stephentoub I'm curious about whether you think Session should be thread-safe in general. From this change I'm guessing currently it isn't. But what would be the use cases for which we should change it to be thread-safe?

I think we should be clear about what is safe and what isn't. Right now, if someone does this:

session.SendAsync(...);
session.On(anotherhandler);

that's not safe even though they're only using the instance themselves serially. That's an example of the case this PR is trying to address.

If we don't think that's valid use, that's fine, but we should then document what's safe and what's not, e.g. you can't use session.On or dispose of an IDisposable returned from On while a session is in-use, only when it's idle.

[SteveSandersonMS]

@stephentoub Totally makes sense - thanks! Yes, the developer can't control when events fire, and likely can't stop them from happening on a different thread, so anything that could conflict with event dispatch does need to be thread-safe.