pw_bluetooth_proxy: Fix connection cleanup logic on ACL disconnect

Upon receiving a disconnection complete event, we should always
remove connection from active connections list (even if there are
not currently pending packets)

Add test for disconnect cleanup.

Change-Id: Ib80fe92c4b299d121b733294b7fe4b1f2c72bcd0
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/247053
Docs-Not-Needed: Austin Foxley <[email protected]>
Presubmit-Verified: CQ Bot Account <[email protected]>
Reviewed-by: David Rees <[email protected]>
Lint: Lint 🤖 <[email protected]>
Pigweed-Auto-Submit: Austin Foxley <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

pw_bluetooth_proxy/acl_data_channel.cc

@@ -146,17 +146,25 @@ void AclDataChannel::HandleDisconnectionCompleteEvent(
 
   uint16_t conn_handle = dc_event->connection_handle().Read();
   AclConnection* connection_ptr = FindConnection(conn_handle);
-  if (connection_ptr && connection_ptr->num_pending_packets > 0) {
-    emboss::StatusCode status = dc_event->status().Read();
-    if (status == emboss::StatusCode::SUCCESS) {
+  if (!connection_ptr) {
+    credit_allocation_mutex_.unlock();
+    hci_transport_.SendToHost(std::move(h4_packet));
+    return;
+  }
+
+  emboss::StatusCode status = dc_event->status().Read();
+  if (status == emboss::StatusCode::SUCCESS) {
+    if (connection_ptr->num_pending_packets > 0) {
       PW_LOG_WARN(
           "Proxy viewed disconnect (reason: %#.2hhx) for connection %#.4hx "
           "with packets in flight. Releasing associated credits",
           cpp23::to_underlying(dc_event->reason().Read()),
           conn_handle);
       proxy_pending_le_acl_packets_ -= connection_ptr->num_pending_packets;
-      active_connections_.erase(connection_ptr);
-    } else {
+    }
+    active_connections_.erase(connection_ptr);
+  } else {
+    if (connection_ptr->num_pending_packets > 0) {
       PW_LOG_WARN(
           "Proxy viewed failed disconnect (status: %#.2hhx) for connection "
           "%#.4hx with packets in flight. Not releasing associated credits.",

pw_bluetooth_proxy/proxy_host_test.cc

@@ -1521,6 +1521,23 @@ TEST(DisconnectionCompleteTest, DisconnectionReclaimsCredits) {
       SendDisconnectionCompleteEvent(proxy, capture.connection_handle));
   EXPECT_EQ(proxy.GetNumFreeLeAclPackets(), 8);
 
+  // Use 1 credit and reclaim it on a bunch of random channels. Then send
+  // disconnect and ensure it was cleaned up in connections list. The send will
+  // fail if disconnect doesn't cleanup properly.
+  //
+  // We already have an active connection at this point in the test, so loop
+  // over the remaining slots + 1 which would otherwise fail if cleanup wasn't
+  // working right.
+  for (uint16_t i = 0; i < kMaxProxyActiveConnections; ++i) {
+    uint16_t handle = 0x234 + i;
+    EXPECT_TRUE(
+        proxy.SendGattNotify(handle, 1, pw::span(attribute_value)).ok());
+    PW_TEST_EXPECT_OK(SendNumberOfCompletedPackets(
+        proxy, FlatMap<uint16_t, uint16_t, 1>({{{handle, 1}}})));
+    EXPECT_EQ(proxy.GetNumFreeLeAclPackets(), 8);
+    PW_TEST_EXPECT_OK(SendDisconnectionCompleteEvent(proxy, handle));
+  }
+
   // Send Number_of_Completed_Packets event that reports 10 packets, none of
   // which should be reclaimed because this Connection has disconnected. Checks
   // in send_to_host_fn will ensure we have not modified the NOCP event.
@@ -1529,7 +1546,7 @@ TEST(DisconnectionCompleteTest, DisconnectionReclaimsCredits) {
       FlatMap<uint16_t, uint16_t, 1>({{{capture.connection_handle, 10}}})));
   EXPECT_EQ(proxy.GetNumFreeLeAclPackets(), 8);
   // NOCP has credits remaining so will be passed on to host.
-  EXPECT_EQ(capture.sends_called, 3);
+  EXPECT_EQ(capture.sends_called, 13);
 }
 
 TEST(DisconnectionCompleteTest, FailedDisconnectionHasNoEffect) {