From a406c4a302dbe464f5ea381ca614b5862380e351 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois-Simon=20Fauteux-Chapleau?=
 <francois-simon.fauteux-chapleau@savoirfairelinux.com>
Date: Wed, 15 Jan 2025 12:08:12 -0500
Subject: [PATCH] upnp_context: unregister all FAILED mappings

This patch fixes a bug where FAILED mappings with the "auto-update"
option enabled are left in the local list, even after a new mapping has
been requested. This can cause the list to grow substantially over time
and was partly responsible for the bug described in the following issue:
https://git.jami.net/savoirfairelinux/jami-client-qt/-/issues/1898

GitLab: #45
Change-Id: I009ff791ae90d10f64d46dede3b2cbfc9c1b7160
---
 src/upnp/upnp_context.cpp | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index f8d44b39..e5558510 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -630,12 +630,15 @@ UPnPContext::enforceAvailableMappingsLimits()
         int pendingCount = 0;
         int inProgressCount = 0;
         int openCount = 0;
+        int unavailableCount = 0;
         {
             std::lock_guard lock(mappingMutex_);
             const auto& mappingList = getMappingList(type);
             for (const auto& [_, mapping] : mappingList) {
-                if (!mapping->isAvailable())
+                if (!mapping->isAvailable()) {
+                    unavailableCount++;
                     continue;
+                }
                 switch (mapping->getState()) {
                     case MappingState::PENDING:
                         pendingCount++;
@@ -652,12 +655,13 @@ UPnPContext::enforceAvailableMappingsLimits()
             }
         }
         int availableCount = openCount + pendingCount + inProgressCount;
-        if (logger_) logger_->debug("Number of 'available' {} mappings in the local list: {} ({} open + {} pending + {} in progress)",
+        if (logger_) logger_->debug("Number of {} mappings in the local list: {} available ({} open + {} pending + {} in progress), {} unavailable",
                                     Mapping::getTypeStr(type),
                                     availableCount,
                                     openCount,
                                     pendingCount,
-                                    inProgressCount);
+                                    inProgressCount,
+                                    unavailableCount);
 
         int minAvailableMappings = getMinAvailableMappings(type);
         if (minAvailableMappings > availableCount) {
@@ -1307,16 +1311,19 @@ UPnPContext::handleFailedMapping(const Mapping::sharedPtr_t& map)
     }
 
     if (isReady()) {
+        // We have a valid IGD, but the mapping request
+        // failed, so try again with a new port number.
         Mapping newMapping(map->getType());
         newMapping.enableAutoUpdate(true);
         newMapping.setNotifyCallback(map->getNotifyCallback());
-        reserveMapping(newMapping);
-        if (logger_) logger_->debug("Mapping {} has auto-update enabled, a new mapping will be requested",
-                                    map->toString());
 
-        // TODO: figure out if this line is actually necessary
-        // (See https://review.jami.net/c/jami-daemon/+/16940)
+        // Remove the old (failed) mapping from the local list
         map->setNotifyCallback(nullptr);
+        unregisterMapping(map);
+
+        if (logger_) logger_->debug("Mapping {} had auto-update enabled, a new mapping will be requested",
+                                    map->toString());
+        reserveMapping(newMapping);
     } else {
         // If there is no valid IGD, the mapping is marked as pending
         // and will be requested when an IGD becomes available.