some fixups for mirror of remote slobrok map:

* require symmetrical register/unregister calls
* modernize a bit
* add unit test for ServiceMapMirror
* use common mock listener in unit tests
* add an explicit shutdown() when removing a RemoteSlobrok instance,
  and perform clear() after canceling all RPC requests and tasks

12 files changed, 216 insertions, 83 deletions

diff --git a/slobrok/CMakeLists.txt b/slobrok/CMakeLists.txt
index 153a029e74d..6acbf5d5134 100644
--- a/slobrok/CMakeLists.txt
+++ b/slobrok/CMakeLists.txt
@@ -22,6 +22,7 @@ vespa_define_module(
     src/tests/mirrorapi
     src/tests/registerapi
     src/tests/service_map_history
+    src/tests/service_map_mirror
     src/tests/standalone
     src/tests/startsome
     src/tests/startup
diff --git a/slobrok/src/tests/service_map_mirror/CMakeLists.txt b/slobrok/src/tests/service_map_mirror/CMakeLists.txt
new file mode 100644
index 00000000000..2af34702068
--- /dev/null
+++ b/slobrok/src/tests/service_map_mirror/CMakeLists.txt
@@ -0,0 +1,9 @@
+# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+vespa_add_executable(slobrok_service_map_mirror_test_app TEST
+    SOURCES
+    service_map_mirror_test.cpp
+    DEPENDS
+    slobrok_slobrokserver
+    GTest::GTest
+)
+vespa_add_test(NAME slobrok_service_map_mirror_test_app COMMAND slobrok_service_map_mirror_test_app)
diff --git a/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp
new file mode 100644
index 00000000000..b1d6c3fb1c1
--- /dev/null
+++ b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp
@@ -0,0 +1,101 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include <vespa/slobrok/server/mock_map_listener.h>
+#include <vespa/slobrok/server/service_map_mirror.h>
+#include <vespa/vespalib/gtest/gtest.h>
+#include <vespa/vespalib/util/stringfmt.h>
+#include <map>
+
+using namespace vespalib;
+using namespace slobrok;
+using vespalib::make_string_short::fmt;
+
+using Map = std::map<vespalib::string, vespalib::string>;
+
+Map dump(const ServiceMapMirror &history) {
+    Map result;
+    for (const auto & entry : history.allMappings()) {
+        result[entry.name] = entry.spec;
+    }
+    return result;
+}
+
+
+void addTo(ServiceMapMirror &target, const ServiceMapping &mapping) {
+    auto cur = target.currentGeneration();
+    std::vector<vespalib::string> removes = {};
+    ServiceMappingList updates = { mapping };
+    auto nxt = cur;
+    nxt.add();
+    MapDiff diff{cur, removes, updates, nxt};
+    target.apply(diff);
+}
+
+void removeFrom(ServiceMapMirror &target, const ServiceMapping &mapping) {
+    auto cur = target.currentGeneration();
+    std::vector<vespalib::string> removes = { mapping.name };
+    ServiceMappingList updates = { };
+    auto nxt = cur;
+    nxt.add();
+    MapDiff diff{cur, removes, updates, nxt};
+    target.apply(diff);
+}
+
+TEST(ServiceMapMirrorTest, empty_inspection) {
+    ServiceMapMirror mirror;
+    auto bar = dump(mirror);
+    EXPECT_TRUE(bar.empty());
+
+    MockMapListener observer;
+    mirror.registerListener(observer);
+    mirror.unregisterListener(observer);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
+}
+
+TEST(ServiceMapMirrorTest, full_inspection) {
+    ServiceMapMirror mirror;
+    MockMapListener observer;
+    mirror.registerListener(observer);
+    for (int i = 0; i < 1984; ++i) {
+        EXPECT_EQ(mirror.currentGeneration(), GenCnt(i));
+        auto name = fmt("key/%d/name", i);
+        auto spec = fmt("tcp/host%d.domain.tld:19099", 10000+i);
+        ServiceMapping toAdd{name, spec};
+        addTo(mirror, toAdd);
+        EXPECT_EQ(observer.last_event, MockEvent::ADD);
+        EXPECT_EQ(observer.last_add, toAdd);
+    }
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1984));
+    ServiceMapping toRemove{"key/666/name", "tcp/host10666.domain.tld:19099"};
+    removeFrom(mirror, toRemove);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
+    EXPECT_EQ(observer.last_remove, toRemove);
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1985));
+
+    ServiceMapping oldMapping{"key/1969/name", "tcp/host11969.domain.tld:19099"};
+    ServiceMapping toUpdate{"key/1969/name", "tcp/woodstock:19069"};
+    addTo(mirror, toUpdate);
+    EXPECT_EQ(observer.last_event, MockEvent::UPDATE);
+    EXPECT_EQ(observer.last_remove, oldMapping);
+    EXPECT_EQ(observer.last_add, toUpdate);
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1986));
+
+    auto map = dump(mirror);
+    EXPECT_FALSE(map.contains("foo"));
+    EXPECT_TRUE(map.contains("key/0/name"));
+    EXPECT_FALSE(map.contains("key/666/name"));
+    EXPECT_TRUE(map.contains("key/1983/name"));
+    EXPECT_FALSE(map.contains("key/1984/name"));
+    EXPECT_TRUE(map.contains("key/1969/name"));
+    EXPECT_EQ(map["key/0/name"], "tcp/host10000.domain.tld:19099");
+    EXPECT_EQ(map["key/123/name"], "tcp/host10123.domain.tld:19099");
+    EXPECT_EQ(map["key/1983/name"], "tcp/host11983.domain.tld:19099");
+    EXPECT_EQ(map["key/1969/name"], "tcp/woodstock:19069");
+    EXPECT_EQ(map.size(), 1983ul);
+
+    mirror.unregisterListener(observer);
+}
+
+
+GTEST_MAIN_RUN_ALL_TESTS()
+
diff --git a/slobrok/src/tests/union_service_map/union_service_map_test.cpp b/slobrok/src/tests/union_service_map/union_service_map_test.cpp
index 5f1f70fb9fb..30db95324bb 100644
--- a/slobrok/src/tests/union_service_map/union_service_map_test.cpp
+++ b/slobrok/src/tests/union_service_map/union_service_map_test.cpp
@@ -1,5 +1,6 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
+#include <vespa/slobrok/server/mock_map_listener.h>
 #include <vespa/slobrok/server/union_service_map.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/util/stringfmt.h>
@@ -8,70 +9,32 @@ using namespace vespalib;
 using namespace slobrok;
 using vespalib::make_string_short::fmt;
 
-enum class Event { NONE, ADD, REMOVE, UPDATE };
-
-struct MapObserver : public MapListener {
-    MapObserver();
-    virtual ~MapObserver();
-    void add(const ServiceMapping &mapping) override;
-    void remove(const ServiceMapping &mapping) override;
-    void update(const ServiceMapping &old_mapping,
-                const ServiceMapping &new_mapping) override;
-
-    Event last_event = Event::NONE;
-    ServiceMapping last_add = {{}, {}};
-    ServiceMapping last_remove = {{}, {}};
-
-    void clear() { last_event = Event::NONE; }
-};
-
-MapObserver::MapObserver() = default;
-MapObserver::~MapObserver() = default;
-
-void MapObserver::add(const ServiceMapping &mapping) {
-    last_event = Event::ADD;
-    last_add = mapping;
-}
-
-void MapObserver::remove(const ServiceMapping &mapping) {
-    last_event = Event::REMOVE;
-    last_remove = mapping;
-}
-
-void MapObserver::update(const ServiceMapping &old_mapping,
-                         const ServiceMapping &new_mapping)
-{
-    last_event = Event::UPDATE;
-    last_remove = old_mapping;
-    last_add = new_mapping;
-}
-
 TEST(UnionServiceMapTest, forwards_simple_requests) {
     ProxyMapSource source;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping one{"foo/1", "bar/1"};
     source.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     ServiceMapping two{"foo/2", "bar/2"};
     source.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
 
     source.remove(one);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 
     ServiceMapping two_q{"foo/2", "qux/2"};
     source.update(two, two_q);
     // update implemented ass remove+add:
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_remove, two);
     EXPECT_EQ(observer.last_add, two_q);
 }
@@ -81,47 +44,47 @@ TEST(UnionServiceMapTest, handles_refcount) {
     ProxyMapSource source2;
     ProxyMapSource source3;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source1.registerListener(unionizer);
     source2.registerListener(unionizer);
     source3.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping one{"foo/1", "bar/1"};
     source1.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source3.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping two{"foo/2", "bar/2"};
     source1.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.add(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     source1.remove(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     source1.remove(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(two);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, two);
 
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source3.remove(one);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 }
 
@@ -130,48 +93,48 @@ TEST(UnionServiceMapTest, handles_conflicts) {
     ProxyMapSource source2;
     ProxyMapSource source3;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source1.registerListener(unionizer);
     source2.registerListener(unionizer);
     source3.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping one{"foo/1", "bar/1"};
     source1.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     observer.clear();
     source2.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping two{"foo/2", "bar/2"};
     source1.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
     observer.clear();
     source2.add(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping one_q{"foo/1", "qux/1"};
     source3.add(one_q);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 
     ServiceMapping two_q{"foo/2", "qux/2"};
     source3.add(two_q);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, two);
 
     source3.remove(one_q);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
 
     observer.clear();
     source1.remove(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two_q);
 }
 
diff --git a/slobrok/src/vespa/slobrok/server/CMakeLists.txt b/slobrok/src/vespa/slobrok/server/CMakeLists.txt
index 1492676cd3a..20f5bda4278 100644
--- a/slobrok/src/vespa/slobrok/server/CMakeLists.txt
+++ b/slobrok/src/vespa/slobrok/server/CMakeLists.txt
@@ -11,8 +11,8 @@ vespa_add_library(slobrok_slobrokserver
     map_diff.cpp
     map_listener.cpp
     map_source.cpp
-    service_map_mirror.cpp
     metrics_producer.cpp
+    mock_map_listener.cpp
     monitor.cpp
     named_service.cpp
     proxy_map_source.cpp
@@ -26,6 +26,7 @@ vespa_add_library(slobrok_slobrokserver
     rpcmirror.cpp
     sbenv.cpp
     service_map_history.cpp
+    service_map_mirror.cpp
     service_mapping.cpp
     slobrokserver.cpp
     union_service_map.cpp
diff --git a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp
index a608c4b3cba..632c823a2c1 100644
--- a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp
+++ b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp
@@ -25,8 +25,7 @@ ExchangeManager::~ExchangeManager() = default;
 OkState
 ExchangeManager::addPartner(const std::string & name, const std::string & spec)
 {
-    RemoteSlobrok *oldremote = lookupPartner(name);
-    if (oldremote != nullptr) {
+    if (RemoteSlobrok *oldremote = lookupPartner(name)) {
         // already a partner, should be OK
         if (spec != oldremote->getSpec()) {
             return OkState(FRTE_RPC_METHOD_FAILED, "name already partner with different spec");
@@ -37,11 +36,9 @@ ExchangeManager::addPartner(const std::string & name, const std::string & spec)
         }
         return OkState();
     }
-
-    LOG_ASSERT(_partners.find(name) == _partners.end());
-    auto newPartner = std::make_unique<RemoteSlobrok>(name, spec, *this);
-    RemoteSlobrok & partner = *newPartner;
-    _partners.emplace(name, std::move(newPartner));
+    auto [ it, wasNew ] = _partners.emplace(name, std::make_unique<RemoteSlobrok>(name, spec, *this));
+    LOG_ASSERT(wasNew);
+    RemoteSlobrok & partner = *it->second;
     partner.tryConnect();
     return OkState();
 }
@@ -53,6 +50,7 @@ ExchangeManager::removePartner(const std::string & name)
     auto oldremote = std::move(_partners[name]);
     LOG_ASSERT(oldremote);
     _partners.erase(name);
+    oldremote->shutdown();
 }
 
 std::vector<std::string>
diff --git a/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp b/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp
new file mode 100644
index 00000000000..9f4b670ddf4
--- /dev/null
+++ b/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp
@@ -0,0 +1,28 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "mock_map_listener.h"
+
+namespace slobrok {
+
+MockMapListener::MockMapListener() = default;
+MockMapListener::~MockMapListener() = default;
+
+void MockMapListener::add(const ServiceMapping &mapping) {
+    last_event = MockEvent::ADD;
+    last_add = mapping;
+}
+
+void MockMapListener::remove(const ServiceMapping &mapping) {
+    last_event = MockEvent::REMOVE;
+    last_remove = mapping;
+}
+
+void MockMapListener::update(const ServiceMapping &old_mapping,
+                             const ServiceMapping &new_mapping)
+{
+    last_event = MockEvent::UPDATE;
+    last_remove = old_mapping;
+    last_add = new_mapping;
+}
+
+}
diff --git a/slobrok/src/vespa/slobrok/server/mock_map_listener.h b/slobrok/src/vespa/slobrok/server/mock_map_listener.h
new file mode 100644
index 00000000000..0c2b51cca2c
--- /dev/null
+++ b/slobrok/src/vespa/slobrok/server/mock_map_listener.h
@@ -0,0 +1,26 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include "map_listener.h"
+
+namespace slobrok {
+
+enum class MockEvent { NONE, ADD, REMOVE, UPDATE };
+
+struct MockMapListener : public MapListener {
+    MockMapListener();
+    virtual ~MockMapListener();
+    void add(const ServiceMapping &mapping) override;
+    void remove(const ServiceMapping &mapping) override;
+    void update(const ServiceMapping &old_mapping,
+                const ServiceMapping &new_mapping) override;
+
+    MockEvent last_event = MockEvent::NONE;
+    ServiceMapping last_add = {{}, {}};
+    ServiceMapping last_remove = {{}, {}};
+
+    void clear() { last_event = MockEvent::NONE; }
+};
+
+}
diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
index 0b9872c03e2..78ca25d4723 100644
--- a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
+++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
@@ -33,8 +33,7 @@ RemoteSlobrok::RemoteSlobrok(const std::string &name, const std::string &spec,
     _rpcserver.healthCheck();
 }
 
-RemoteSlobrok::~RemoteSlobrok()
-{
+void RemoteSlobrok::shutdown() {
     _reconnecter.disable();
 
     _pending.clear();
@@ -59,9 +58,13 @@ RemoteSlobrok::~RemoteSlobrok()
     if (_remRemReq != nullptr) {
         _remRemReq->Abort();
     }
-    // _rpcserver destructor called automatically
+    _serviceMapMirror.clear();
 }
 
+RemoteSlobrok::~RemoteSlobrok() {
+    shutdown();
+    // _rpcserver destructor called automatically
+}
 
 void
 RemoteSlobrok::doPending()
diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.h b/slobrok/src/vespa/slobrok/server/remote_slobrok.h
index 9e3fb1df55f..e463ac6be21 100644
--- a/slobrok/src/vespa/slobrok/server/remote_slobrok.h
+++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.h
@@ -76,6 +76,7 @@ public:
     const std::string & getName() const { return _rpcserver.getName(); }
     const std::string & getSpec() const { return _rpcserver.getSpec(); }
     ServiceMapMirror &remoteMap() { return _serviceMapMirror; }
+    void shutdown();
 
     // interfaces implemented:
     void notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::string errmsg) override;
diff --git a/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp b/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp
index 8b9a40efed7..51db84e24a4 100644
--- a/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp
+++ b/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp
@@ -13,7 +13,7 @@ ServiceMapMirror::ServiceMapMirror()
 {}
 
 ServiceMapMirror::~ServiceMapMirror() {
-    clear();
+    LOG_ASSERT(_listeners.size() == 0);
 }
 
 void ServiceMapMirror::apply(const MapDiff &diff) {
@@ -87,4 +87,3 @@ void ServiceMapMirror::unregisterListener(MapListener &listener) {
 
 
 } // namespace slobrok
-
diff --git a/slobrok/src/vespa/slobrok/server/union_service_map.cpp b/slobrok/src/vespa/slobrok/server/union_service_map.cpp
index f3d63f2a087..4a8ca51d409 100644
--- a/slobrok/src/vespa/slobrok/server/union_service_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/union_service_map.cpp
@@ -17,10 +17,12 @@ void UnionServiceMap::add(const ServiceMapping &mapping)
     if (iter == _mappings.end()) {
         _mappings[key].emplace_back(mapping.spec, 1u);
         _proxy.add(mapping);
+        LOG(debug, "add new %s->%s", mapping.name.c_str(), mapping.spec.c_str());
     } else {
         Mappings &values = iter->second;
         for (CountedSpec &old : values) {
             if (old.spec == mapping.spec) {
+                LOG(debug, "add ref to existing %s->%s", mapping.name.c_str(), mapping.spec.c_str());
                 ++old.count;
                 return;
             }
@@ -41,6 +43,7 @@ void UnionServiceMap::remove(const ServiceMapping &mapping)
         LOG(error, "Broken invariant: did not find %s in mappings", key.c_str());
         return;
     }
+    LOG(debug, "remove ref from %s->%s", mapping.name.c_str(), mapping.spec.c_str());
     Mappings &values = iter->second;
     bool found = false;
     for (CountedSpec &old : values) {