[dotnet] [bidi] Fix commands cancelling callback

[nvborisenko]

Disposing ctsRegistration too early, preventing command cannot be cancelled via timeout.

💥 What does this PR do?

This pull request makes a minor change to the ExecuteCommandAsync method in Broker.cs, moving the return statement inside the try block to ensure the awaited task result is returned before buffer cleanup in the finally block. This change helps prevent returning a result after resources may have been released.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Fix BiDi command cancellation by moving return before finally block

🐞 Bug fix

Walkthroughs

Description

• Move return statement inside try block before finally cleanup
• Ensures awaited task result returned before buffer resources released
• Prevents command cancellation timeout callback disposal race condition

File Changes

1. dotnet/src/webdriver/BiDi/Broker.cs 🐞 Bug fix +2/-2

Reorder return statement to prevent resource cleanup race

• Moved return statement from after finally block into try block
• Ensures task result is awaited and returned before buffer cleanup occurs
• Prevents premature resource disposal that could interfere with command cancellation via timeout

dotnet/src/webdriver/BiDi/Broker.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0)   📘 Rule violations (1)   📎 Requirement gaps (0)   🎨 UX Issues (0)

📘\ ☼ Reliability (1)

1. Timeout cancellation untested 📘 ☼

Description

This PR changes ExecuteCommandAsync behavior around when the command task is awaited relative to
resource cleanup, which impacts timeout/cancellation behavior. No corresponding test change is
included to prevent regressions in command timeout cancellation behavior.

Code

dotnet/src/webdriver/BiDi/Broker.cs[175]

+            return (TResult)await tcs.Task.ConfigureAwait(false);

Evidence

The checklist requires adding/updating tests when implementing fixes or changing observable
behavior. The diff introduces a functional behavior change by awaiting tcs.Task inside the try
block (affecting timeout/cancellation timing), but the provided PR diff contains only this
production code change and no test additions/updates.

AGENTS.md
dotnet/src/webdriver/BiDi/Broker.cs[139-180]
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
A behavioral change was made to `Broker.ExecuteCommandAsync` that affects timeout/cancellation timing, but no regression test was added/updated to lock in the fix.

## Issue Context
The fix moves `return (TResult)await tcs.Task` inside the `try` block so the `ctsRegistration` remains active until the command completes, which is intended to restore timeout-based cancellation. This is a reliability-sensitive path and should be covered by a focused test to prevent regressions.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[139-180]
- dotnet/test/webdriver/BiDi/BiDiFixture.cs[20-64]

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

---

2. Buffer returned after response ☑ 🐞 ➹

Description

ExecuteCommandAsync now returns the rented sendBuffer only after awaiting the command result, so
each in-flight command retains a pooled buffer for the full response/timeout duration. This can
increase allocations/churn (pool capacity is 16 and RentBuffer allocates when empty), especially
with parallel commands or long/infinite timeouts.

Code

dotnet/src/webdriver/BiDi/Broker.cs[R175-180]

+            return (TResult)await tcs.Task.ConfigureAwait(false);
        }
        finally
        {
            ReturnBuffer(sendBuffer);
        }
-
-        return (TResult)await tcs.Task.ConfigureAwait(false);

Evidence

The PR moves return await tcs.Task inside the try, which means the `finally {
ReturnBuffer(sendBuffer); }` executes only after the awaited task completes; previously the buffer
could be returned before awaiting the response. Since the shared _bufferPool is bounded to 16
items and RentBuffer() allocates a new PooledBufferWriter when the pool is empty (each renting
an 8KB array from ArrayPool), retaining send buffers for the entire command lifetime increases the
likelihood of extra allocations and reduces buffer reuse, also impacting receive-side buffering
which uses the same pool.

dotnet/src/webdriver/BiDi/Broker.cs[117-181]
dotnet/src/webdriver/BiDi/Broker.cs[49-54]
dotnet/src/webdriver/BiDi/Broker.cs[456-468]
dotnet/src/webdriver/BiDi/Broker.cs[365-392]
dotnet/src/webdriver/BiDi/Broker.cs[477-485]
dotnet/src/webdriver/BiDi/CommandOptions.cs[22-25]

Agent prompt

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

### Issue description
`sendBuffer` is only needed for JSON serialization and `_transport.SendAsync(...)`, but the PR now returns it to the pool only after awaiting the command response (`tcs.Task`). This retains pooled buffers for the full command lifetime (potentially long, or even indefinite if `Timeout` is set accordingly), reducing reuse and increasing allocations.

### Issue Context
You still need to keep the cancellation/timeout registration (`cts.Token.Register(...)`) alive until `tcs.Task` completes so timeouts cancel the pending command.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[117-181]

### Suggested approach
Restructure scopes so:
1) `ctsRegistration` lifetime spans the `await tcs.Task`.
2) `sendBuffer` is returned in a `finally` immediately after `SendAsync` completes (success or failure), not after the response is received.

For example (conceptually):
- Declare `using var ctsRegistration = ...;` in a scope that includes the `return await tcs.Task`.
- Use a separate `try/finally` around just the send/serialization portion to `ReturnBuffer(sendBuffer)` right after send.

ⓘ 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

This PR fixes BiDi command timeout/cancellation behavior in the .NET WebDriver by adjusting control flow in Broker.ExecuteCommandAsync so that cancellation registrations remain active while awaiting the command result.

Changes:

• Move the await tcs.Task return into the try block so the method awaits the command completion before leaving the scope that contains the cancellation registration.