handle addLocal for down services better

* if we already have a registration in progress, hook the second
  one to the first so they get the same answer at the same time
* if we think the service is down, restart its monitoring to
  avoid getting stale information, and add the inflight hook so we get an
  answer when the new ping returns.

2 files changed, 53 insertions, 5 deletions

diff --git a/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp b/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
index 40f8baa32a9..a69a77d3640 100644
--- a/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
+++ b/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
@@ -240,16 +240,33 @@ TEST_F(LocalRpcMonitorMapTest, local_add_already_up) {
     ASSERT_TRUE(handler_deleted);
 }
 
-TEST_F(LocalRpcMonitorMapTest, local_add_already_down) {
+TEST_F(LocalRpcMonitorMapTest, local_add_unknown_comes_up) {
     std::unique_ptr<OkState> state;
     bool handler_deleted;
     add_mapping(mapping, false);
     map.addLocal(mapping, std::make_unique<MyAddLocalHandler>(state, handler_deleted));
-    monitor_log.expect({});
+    monitor_log.expect({MonitorCall::stop(mapping), MonitorCall::start(mapping, true)});
     map_log.expect({});
+    ASSERT_FALSE(state);
+    map.up(mapping);
     ASSERT_TRUE(state);
     EXPECT_TRUE(state->ok());
     ASSERT_TRUE(handler_deleted);
+    map_log.expect({MapCall::add(mapping)});
+}
+
+TEST_F(LocalRpcMonitorMapTest, local_add_unknown_goes_down) {
+    std::unique_ptr<OkState> state;
+    bool handler_deleted;
+    add_mapping(mapping, false);
+    map.addLocal(mapping, std::make_unique<MyAddLocalHandler>(state, handler_deleted));
+    monitor_log.expect({MonitorCall::stop(mapping), MonitorCall::start(mapping, true)});
+    map_log.expect({});
+    ASSERT_FALSE(state);
+    map.down(mapping);
+    ASSERT_TRUE(state);
+    EXPECT_FALSE(state->ok());
+    ASSERT_TRUE(handler_deleted);
 }
 
 TEST_F(LocalRpcMonitorMapTest, local_add_conflict) {
diff --git a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
index 79f796dc435..47f604a08ec 100644
--- a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
@@ -10,6 +10,26 @@ namespace slobrok {
 
 #pragma GCC diagnostic ignored "-Winline"
 
+namespace {
+
+struct ChainedAddLocalCompletionHandler : LocalRpcMonitorMap::AddLocalCompletionHandler {
+    std::unique_ptr<AddLocalCompletionHandler> first;
+    std::unique_ptr<AddLocalCompletionHandler> second;
+
+    ChainedAddLocalCompletionHandler(std::unique_ptr<AddLocalCompletionHandler> f,
+                                     std::unique_ptr<AddLocalCompletionHandler> s)
+        : first(std::move(f)), second(std::move(s))
+    {}
+
+    void doneHandler(OkState result) override {
+        first->doneHandler(result);
+        second->doneHandler(result);
+    }
+    ~ChainedAddLocalCompletionHandler() override {}
+};
+
+}
+
 void LocalRpcMonitorMap::DelayedTasks::PerformTask() {
     std::vector<Event> todo;
     std::swap(todo, _queue);
@@ -89,12 +109,23 @@ void LocalRpcMonitorMap::addLocal(const ServiceMapping &mapping,
         mapping.name.c_str(), mapping.spec.c_str());
     auto old = _map.find(mapping.name);
     if (old != _map.end()) {
-        const PerService & exists = old->second;
+        PerService & exists = old->second;
         if (exists.spec == mapping.spec) {
             LOG(debug, "added mapping %s->%s was already present",
                 mapping.name.c_str(), mapping.spec.c_str());
-            // clear exists.ignoreFirstOk ?
-            inflight->doneHandler(OkState(0, "already registered"));
+            if (exists.up) {
+                inflight->doneHandler(OkState(0, "already registered"));
+            } else if (exists.inflight) {
+                auto newInflight = std::make_unique<ChainedAddLocalCompletionHandler>(
+                    std::move(exists.inflight),
+                    std::move(inflight));
+                exists.inflight = std::move(newInflight);
+            } else {
+                _mappingMonitor->stop(mapping);
+                exists.inflight = std::move(inflight);
+                exists.ignoreFirstOk = false;
+                _mappingMonitor->start(mapping, true);
+            }
             return;
         }
         LOG(warning, "tried addLocal for mapping %s->%s, but already had conflicting mapping %s->%s",