[dotnet] [bidi] Unregister cancelled commands

[nvborisenko]

Refines the command execution logic in the Broker class to improve resource management and prevent memory leaks when commands are canceled. The main change ensures that canceled commands are properly removed from the _pendingCommands dictionary.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Unregister canceled commands from pending dictionary to prevent memory leaks

• Register cancellation handler to clean up command state on timeout

• Reorder serialization to occur before command registration

---

File Walkthrough

	Relevant files
Bug fix	Broker.cs Clean up pending commands on cancellation                                dotnet/src/webdriver/BiDi/Broker.cs Modified cancellation token registration to remove canceled commands from _pendingCommands dictionary Wrapped registration in using statement for proper resource disposal Reordered command serialization to occur before command registration in pending dictionary +6/-2

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Cancellation race leak: The cancellation callback can run before _pendingCommands[command.Id] is populated, causing TryRemove to be a no-op and leaving a canceled command registered in _pendingCommands. Referred Code using var ctsRegistration = cts.Token.Register(() => { tcs.TrySetCanceled(cts.Token); _pendingCommands.TryRemove(command.Id, out _); }); var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo); _pendingCommands[command.Id] = commandInfo; Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Fix race condition causing memory leak Suggestion Impact: The commit reordered the code so serialization and adding the CommandInfo to _pendingCommands happen before registering the cancellation callback, addressing the race window. It did not include the suggested defensive TryRemove check around cancellation. code diff: + var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); + var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo); + _pendingCommands[command.Id] = commandInfo; + using var ctsRegistration = cts.Token.Register(() => { tcs.TrySetCanceled(cts.Token); _pendingCommands.TryRemove(command.Id, out _); }); Fix a race condition by adding the command to _pendingCommands before registering the cancellation callback to prevent a potential memory leak. dotnet/src/webdriver/BiDi/Broker.cs [150-157] -using var ctsRegistration = cts.Token.Register(() => -{ - tcs.TrySetCanceled(cts.Token); - _pendingCommands.TryRemove(command.Id, out _); -}); -var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo); _pendingCommands[command.Id] = commandInfo; +using var ctsRegistration = cts.Token.Register(() => +{ + if (_pendingCommands.TryRemove(command.Id, out _)) + { + tcs.TrySetCanceled(cts.Token); + } +}); +var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); + [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition that could lead to a memory leak and proposes a valid fix by reordering operations. The improved code also adds a defensive check, making the cancellation logic more robust.	Medium
	✅ Remove pending entry on send failure Suggestion Impact: Wrapped _transport.SendAsync in a try/catch and on exception removed the command id from _pendingCommands before rethrowing, preventing stale pending entries when send fails. code diff: + var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); + var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo); + _pendingCommands[command.Id] = commandInfo; + using var ctsRegistration = cts.Token.Register(() => { tcs.TrySetCanceled(cts.Token); _pendingCommands.TryRemove(command.Id, out _); }); - var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo); - var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo); - _pendingCommands[command.Id] = commandInfo; - - await _transport.SendAsync(data, cts.Token).ConfigureAwait(false); + + try + { + await _transport.SendAsync(data, cts.Token).ConfigureAwait(false); + } + catch + { + _pendingCommands.TryRemove(command.Id, out _); + throw; + } Add a try/catch block around the _transport.SendAsync call to ensure the _pendingCommands entry is cleaned up if the send operation fails. dotnet/src/webdriver/BiDi/Broker.cs [159] -await _transport.SendAsync(data, cts.Token).ConfigureAwait(false); +try +{ + await _transport.SendAsync(data, cts.Token).ConfigureAwait(false); +} +catch +{ + _pendingCommands.TryRemove(command.Id, out _); + throw; +} [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a resource leak path where an exception during SendAsync would leave a stale entry in _pendingCommands, and the proposed try/catch block is the correct way to fix it.	Medium
Update

[Copilot]

Pull request overview

Refines .NET BiDi Broker.ExecuteCommandAsync cancellation handling to avoid retaining canceled commands in the broker’s pending-command tracking, helping prevent memory growth over time during frequent cancellations/timeouts.

Changes:

• Dispose the cancellation token registration and add cleanup logic to remove canceled commands from _pendingCommands.
• Reorder command serialization relative to _pendingCommands registration.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.