[dotnet] [bidi] Revert... Wait until events are dispatched

[nvborisenko]

This reverts commit 2f71272.

Blocking events processing by users' handlers appeared risky. It might logically blocks.

🔗 Related Issues

#16574

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Revert BiDi event dispatch synchronization to fix deadlocks

🐞 Bug fix

Walkthroughs

Description

• Reverts event dispatching synchronization that caused logical deadlocks
• Simplifies event registration and unsubscription flow
• Removes drain mechanism that waited for pending events
• Refactors event processing to use record struct instead of class

File Changes

1. dotnet/src/webdriver/BiDi/EventDispatcher.cs 🐞 Bug fix +23/-131

Revert event synchronization and simplify event handling

• Reverts commit 2f7127244336d9a585874ff6bc4a8fa336c64414 that added event drain synchronization
• Removes DrainAsync() method and related sequence tracking (_enqueueSeq, _processedSeq,
 _drainWaiters)
• Simplifies SubscribeAsync() by removing try-catch error handling for handler addition
• Simplifies UnsubscribeAsync() by removing drain wait and directly removing handlers
• Changes EventItem record to PendingEvent struct and stores TypeInfo directly instead of
 EventRegistration
• Removes handler snapshot locking mechanism and replaces with simple ToArray() copy
• Removes individual handler exception catching, keeping only top-level event processing error
 handling
• Renames _processEventsTask to _eventEmitterTask and uses TaskFactory for task creation

dotnet/src/webdriver/BiDi/EventDispatcher.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (2)

1. Handler added after subscribe 📎 Requirement gap ⛯ Reliability

Description

SubscribeAsync registers the remote subscription before adding the local handler, and
UnsubscribeAsync removes the handler immediately without draining queued events. This can race
with event delivery during setup/teardown and cause intermittent missed console logs unless delayed.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R58-70]

+        var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false);

-        try
-        {
-            var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false);
+        registration.Handlers.Add(eventHandler);

-            return new Subscription(subscribeResult.Subscription, this, eventHandler);
-        }
-        catch
-        {
-            registration.RemoveHandler(eventHandler);
-            throw;
-        }
+        return new Subscription(subscribeResult.Subscription, this, eventHandler);
    }

    public async ValueTask UnsubscribeAsync(Subscription subscription, CancellationToken cancellationToken)
    {
        if (_events.TryGetValue(subscription.EventHandler.EventName, out var registration))
        {
            await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false);
-
-            // Wait until all pending events for this method are dispatched
-            try
-            {
-                await registration.DrainAsync(cancellationToken).ConfigureAwait(false);
-            }
-            finally
-            {
-                registration.RemoveHandler(subscription.EventHandler);
-            }
+            registration.Handlers.Remove(subscription.EventHandler);

Evidence

Compliance requires deterministic delivery of console log events without time-based delays and
avoiding loss during WebSocket shutdown; adding handlers only after subscribing and removing them
immediately on unsubscribe creates a window where events can be received but not dispatched to user
handlers.

.NET BiDi log subscription reliably receives console logs without time-based delays
BiDi WebSocket shutdown does not drop pending log events in .NET
dotnet/src/webdriver/BiDi/EventDispatcher.cs[58-62]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[65-70]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SubscribeAsync` adds the local handler only after awaiting the remote `SubscribeAsync`, and `UnsubscribeAsync` removes the handler immediately without draining pending events. This creates a race where events can arrive during setup/teardown but are not dispatched, causing missed log entries unless `Task.Delay` is used.

## Issue Context
This PR is a revert related to BiDi event dispatch timing. The compliance requirement is that console logs should be delivered deterministically to `OnEntryAddedAsync` handlers even when the session/connection is torn down quickly.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[58-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Handlers list not thread-safe 📘 Rule violation ⛯ Reliability

Description

EventRegistration.Handlers is now a plain List<T> exposed publicly and is mutated/read from
different code paths without synchronization. This can cause races/exceptions and inconsistent
dispatch during concurrent subscribe/unsubscribe and event emission.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[135]

+        public List<EventHandler> Handlers { get; } = [];

Evidence

The compliance rule requires atomic/thread-safe tracking of async operations/state; using an
unsynchronized List for handler registration while iterating and modifying across async code paths
is not thread-safe even if callers attempt to copy via ToArray().

dotnet/src/webdriver/BiDi/EventDispatcher.cs[135-135]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[60-60]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[70-70]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[104-107]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`EventRegistration.Handlers` is an unsynchronized `List<EventHandler>` that is mutated and enumerated from async code paths. This violates the requirement for atomic/thread-safe state updates and risks race conditions or runtime exceptions.

## Issue Context
Event dispatching and subscription changes can occur concurrently with processing pending events, especially during shutdown. The code currently relies on `ToArray()` during iteration but does not make `Add`/`Remove` operations safe.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[60-70]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[104-107]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[135-135]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Unsubscribe cleanup not guaranteed 🐞 Bug ⛯ Reliability

Description

If the remote UnsubscribeAsync throws or is canceled, the local handler is not removed because
removal happens after the await with no finally. This can leak handlers and keep invoking user
callbacks after unsubscribe fails.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R65-71]

    public async ValueTask UnsubscribeAsync(Subscription subscription, CancellationToken cancellationToken)
    {
        if (_events.TryGetValue(subscription.EventHandler.EventName, out var registration))
        {
            await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false);
-
-            // Wait until all pending events for this method are dispatched
-            try
-            {
-                await registration.DrainAsync(cancellationToken).ConfigureAwait(false);
-            }
-            finally
-            {
-                registration.RemoveHandler(subscription.EventHandler);
-            }
+            registration.Handlers.Remove(subscription.EventHandler);
        }

Evidence

Local removal is after awaiting the remote call, so any exception prevents cleanup. Since
Subscription.DisposeAsync calls UnsubscribeAsync, this can also leak during shutdown/cleanup paths.

dotnet/src/webdriver/BiDi/EventDispatcher.cs[65-71]
dotnet/src/webdriver/BiDi/Subscription.cs[37-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`UnsubscribeAsync` does not guarantee local handler removal when the remote unsubscribe fails.

### Issue Context
Unsubscribe is invoked directly by users and indirectly by `Subscription.DisposeAsync`, so failure paths must still clean up local state.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[65-71]
- dotnet/src/webdriver/BiDi/Subscription.cs[37-46]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Enqueue failure not logged 📎 Requirement gap ✧ Quality

Description

TryWrite to the pending-events channel is not checked and failures are silently ignored. During
shutdown (writer completed) this can drop events without any diagnostic logging for users.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R76-78]

+        if (_events.TryGetValue(method, out var registration) && registration.TypeInfo is not null)
        {
-            if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))
-            {
-                registration.IncrementEnqueued();
-            }
-            else
-            {
-                if (_logger.IsEnabled(LogEventLevel.Warn))
-                {
-                    _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");
-                }
-            }
+            _pendingEvents.Writer.TryWrite(new PendingEvent(method, jsonUtf8Bytes, bidi, registration.TypeInfo));

Evidence

The compliance rule asks for user-helpful logging where insight is needed; this change removes the
prior warning path and makes event loss during teardown harder to diagnose, which is especially
relevant given the requirement to not drop pending log events.

BiDi WebSocket shutdown does not drop pending log events in .NET
AGENTS.md
dotnet/src/webdriver/BiDi/EventDispatcher.cs[76-78]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_pendingEvents.Writer.TryWrite(...)` is called without checking the return value, so enqueue failures (e.g., after `Complete()` during teardown) silently drop events and provide no diagnostics.

## Issue Context
This area is user-impacting because dropped BiDi log events are the core symptom under investigation, and lack of logging makes it difficult to understand why logs are missed.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[76-78]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Logs raw exception string 📘 Rule violation ⛨ Security

Description

The error log interpolates the full exception ({ex}), which can include sensitive/nullable message
content and lacks stable identifiers (e.g., method/request id). This reduces resilience and may leak
details in user logs.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R112-115]

                    if (_logger.IsEnabled(LogEventLevel.Error))
                    {
-                        _logger.Error($"Unhandled error deserializing BiDi event: {ex}");
+                        _logger.Error($"Unhandled error processing BiDi event handler: {ex}");
                    }

Evidence

The compliance rule requires resilient logging that avoids leaking raw exception messages and
prefers stable identifiers like exception type and relevant correlation data; the new log line
directly embeds the exception object in the message.

dotnet/src/webdriver/BiDi/EventDispatcher.cs[112-115]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The error path logs `ex` via string interpolation, which can leak exception messages and does not include stable identifiers (like exception type + event method) in a structured/resilient way.

## Issue Context
These errors can occur while processing BiDi events; logs should help diagnose issues without exposing sensitive or unstable exception message content.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[112-115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. One handler breaks others 🐞 Bug ⛯ Reliability

Description

A single handler exception aborts dispatch to remaining handlers for the same event because the
try/catch wraps the entire foreach loop. This is a regression in robustness when multiple handlers
are registered for one event name.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R96-107]

                try
                {
-                    var eventArgs = (EventArgs)JsonSerializer.Deserialize(evt.JsonUtf8Bytes.Span, evt.Registration.TypeInfo)!;
-                    eventArgs.BiDi = evt.BiDi;
-
-                    foreach (var handler in evt.Registration.GetHandlersSnapshot())
+                    if (_events.TryGetValue(result.Method, out var registration))
                    {
-                        try
+                        // Deserialize on background thread instead of network thread (single parse)
+                        var eventArgs = (EventArgs)JsonSerializer.Deserialize(result.JsonUtf8Bytes.Span, result.TypeInfo)!;
+                        eventArgs.BiDi = result.BiDi;
+
+                        foreach (var handler in registration.Handlers.ToArray()) // copy handlers avoiding modified collection while iterating
                        {
                            await handler.InvokeAsync(eventArgs).ConfigureAwait(false);
                        }
-                        catch (Exception ex)
-                        {
-                            if (_logger.IsEnabled(LogEventLevel.Error))
-                            {
-                                _logger.Error($"Unhandled error processing BiDi event handler: {ex}");
-                            }
-                        }

Evidence

ProcessEventsAwaiterAsync catches exceptions outside the foreach, so a throw from one InvokeAsync
prevents subsequent handlers from running. Multiple handlers are expected because each call to
Module.SubscribeAsync creates and registers a new EventHandler instance for the same event name.

dotnet/src/webdriver/BiDi/EventDispatcher.cs[96-116]
dotnet/src/webdriver/BiDi/Module.cs[36-48]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[56-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Exceptions thrown by a single user handler currently prevent other handlers from running for the same event.

### Issue Context
Multiple independent subscriptions to the same event name are supported; a single handler should not break others.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[89-116]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

Reverts the prior change that waited for pending BiDi events to be dispatched during unsubscription, due to the risk of user event handlers blocking event processing and causing hangs.

Changes:

• Replaces per-registration “drain” tracking with a simpler pending-event channel carrying method/type info.
• Updates subscription/unsubscription bookkeeping to directly add/remove handlers without draining.
• Adjusts the background event processing loop and its error handling/logging.