Track deleted runtime objects to avoid erroneous recreation

lib/base/dictionary.cpp

@@ -155,16 +155,17 @@ Dictionary::Iterator Dictionary::End()
  * Removes the item specified by the iterator from the dictionary.
  *
  * @param it The iterator.
+ * @return an iterator to the element after the removed element.
  */
-void Dictionary::Remove(Dictionary::Iterator it)
+Dictionary::Iterator Dictionary::Remove(Dictionary::Iterator it)
 {
 	ASSERT(OwnsLock());
 	std::unique_lock<std::shared_timed_mutex> lock (m_DataMutex);
 
 	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Dictionary must not be modified."));
 
-	m_Data.erase(it);
+	return m_Data.erase(it);
 }
 
 /**

lib/base/dictionary.hpp

@@ -56,7 +56,7 @@ class Dictionary final : public Object
 
 	void Remove(const String& key);
 
-	void Remove(Iterator it);
+	Iterator Remove(Iterator it);
 
 	void Clear();
 

lib/remote/apilistener-configsync.cpp

@@ -98,6 +98,13 @@ Value ApiListener::ConfigUpdateObjectAPIHandler(const MessageOrigin::Ptr& origin
 	/* update the object */
 	double objVersion = params->Get("version");
 
+	if (listener->GetRuntimeObjectDeletionTs(objType, objName) >= objVersion) {
+		Log(LogInformation, "ApiListener")
+			<< "Ignoring config update for deleted object '" << objName << "' of type '" << objType << "' from '"
+			<< identity << "' (endpoint: '" << endpoint->GetName() << "', zone: '" << endpointZone->GetName() << "')";
+		return Empty;
+	}
+
 	Type::Ptr ptype = Type::GetByName(objType);
 	auto *ctype = dynamic_cast<ConfigType *>(ptype.get());
 
@@ -274,6 +281,24 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin
 
 	ConfigObject::Ptr object = ctype->GetObject(objName);
 
+	// Check if the deletion is for an older object version
+	double objVersion = params->Get("version");
+	if (object && objVersion < object->GetVersion()) {
+		Log(LogNotice, "ApiListener")
+			<< "Discarding 'config delete object' message"
+			<< " from '" << identity << "' (endpoint: '" << endpoint->GetName() << "', zone: '" << endpointZone->GetName() << "')"
+			<< " for object '" << object->GetName()
+			<< "': Object version " << std::fixed << object->GetVersion()
+			<< " is more recent than the deleted version " << std::fixed << objVersion << ".";
+
+		return Empty;
+	}
+
+	/* We need to store the deletion timestamp even if no object exists, to protect against
+	 * endpoints that try to sync back objects that would have been deleted by this message.
+	 */
+	listener->UpdateRuntimeObjectDeletionTs(objType, objName, objVersion);
+
 	if (!object) {
 		Log(LogNotice, "ApiListener")
 			<< "Could not delete non-existent object '" << objName << "' with type '" << params->Get("type") << "'.";
@@ -437,6 +462,24 @@ void ApiListener::DeleteConfigObject(const ConfigObject::Ptr& object, const Mess
 	if (object->GetPackage() != "_api")
 		return;
 
+	auto typeName = object->GetReflectionType()->GetName();
+	auto objName = object->GetName();
+
+	/* Update the deletion timestamp only if the deletion didn't originate from another
+	 * endpoint, because when this deletion was triggered via a JSON-RPC message, the
+	 * stored timestamp is the one that was set in `ApiListener::ConfigDeleteObjectAPIHandler`
+	 * and  already reflects the time the original deletion was triggered.
+	 * Also set if no timestamp was stored yet, as a safeguard. There shouldn't be any
+	 * situation where this happens, but that relies on the correct execution order of
+	 * many other functions, like cascading deletes always propagating in the correct
+	 * order through JSON-RPC, so the children are always deleted first.
+	 */
+	double deletionTime = GetRuntimeObjectDeletionTs(typeName, objName);
+	if (!origin || origin->IsLocal() || deletionTime == 0) {
+		deletionTime = Utility::GetTime();
+		UpdateRuntimeObjectDeletionTs(typeName, objName, deletionTime);
+	}
+
 	/* only send objects to zones which have access to the object */
 	if (client) {
 		Zone::Ptr target_zone = client->GetEndpoint()->GetZone();
@@ -460,7 +503,7 @@ void ApiListener::DeleteConfigObject(const ConfigObject::Ptr& object, const Mess
 
 	params->Set("name", object->GetName());
 	params->Set("type", object->GetReflectionType()->GetName());
-	params->Set("version", object->GetVersion());
+	params->Set("version", deletionTime);
 
 
 #ifdef I2_DEBUG
@@ -508,3 +551,79 @@ void ApiListener::SendRuntimeConfigObjects(const JsonRpcConnection::Ptr& aclient
 	Log(LogInformation, "ApiListener")
 		<< "Finished syncing runtime objects to endpoint '" << endpoint->GetName() << "'.";
 }
+
+/**
+ * Prunes the list of deleted runtime objects.
+ *
+ * This takes into account the longest log duration of all the endpoints in the cluster and
+ * then deletes the entries for objects that are older than that.
+ *
+ * The reasoning is that once a deletion is older than log duration of an endpoint it is
+ * unlikely that we could still get any delayed updates to the object versions that were deleted.
+ * At that point, either all endpoints are up-to-date, or the problematic messages have been
+ * dropped from the replay log anyway.
+ */
+void ApiListener::PruneDeletedRuntimeObjects()
+{
+	auto deletedRuntimeObjects = GetDeletedRuntimeObjects();
+
+	double maxLogDuration = 0;
+	for (const auto &endpoint : ConfigType::GetObjectsByType<Endpoint>()) {
+		maxLogDuration = std::max(maxLogDuration, endpoint->GetLogDuration());
+	}
+	double cutoff = Utility::GetTime() - maxLogDuration;
+
+	ObjectLock lock(deletedRuntimeObjects);
+
+	for (auto it = deletedRuntimeObjects->Begin(); it != deletedRuntimeObjects->End();) {
+		if (it->second < cutoff) {
+			it = deletedRuntimeObjects->Remove(it);
+		} else {
+			it++;
+		};
+	}
+}
+
+static String MakeDeletionTimestampKey(const String& typeName, const String& objName)
+{
+	return typeName + ":" + objName;
+}
+
+/**
+ * Get the timestamp at which the object has been most recently deleted.
+ *
+ * @param typeName The name of the tyope of the object
+ * @param objName The name of the object
+ * @return the timestamp of the object, or 0 if there was no entry.
+ */
+double ApiListener::GetRuntimeObjectDeletionTs(const String& typeName, const String& objName)
+{
+	auto dict = GetDeletedRuntimeObjects();
+	auto ts = dict->Get(MakeDeletionTimestampKey(typeName, objName));
+	if (ts.IsNumber()) {
+		return ts.Get<double>();
+	}
+	return 0;
+}
+
+/**
+ * Updates the deletion timestamp for the object.
+ *
+ * The timestamp will not be changed in case a newer timestamp has already been stored
+ * for the object.
+ *
+ * @param typeName The name of the tyope of the object
+ * @param objName The name of the object
+ * @param ts the timestamp
+ */
+bool ApiListener::UpdateRuntimeObjectDeletionTs(const String& typeName, const String& objName, double ts)
+{
+	auto dict = GetDeletedRuntimeObjects();
+	ObjectLock lock(dict);
+	auto current = GetRuntimeObjectDeletionTs(typeName, objName);
+	if (current > ts) {
+		return false;
+	}
+	dict->Set(MakeDeletionTimestampKey(typeName, objName), ts);
+	return true;
+}

lib/remote/apilistener.cpp

@@ -285,6 +285,12 @@ void ApiListener::Start(bool runtimeCreated)
 	m_Timer->Start();
 	m_Timer->Reschedule(0);
 
+	m_DeletedRuntimeObjectsTimer = Timer::Create();
+	m_DeletedRuntimeObjectsTimer->OnTimerExpired.connect([this](const Timer* const&) { PruneDeletedRuntimeObjects(); });
+	m_DeletedRuntimeObjectsTimer->SetInterval(15 * 60);
+	m_DeletedRuntimeObjectsTimer->Start();
+	m_DeletedRuntimeObjectsTimer->Reschedule(0);
+
 	m_ReconnectTimer = Timer::Create();
 	m_ReconnectTimer->OnTimerExpired.connect([this](const Timer * const&) { ApiReconnectTimerHandler(); });
 	m_ReconnectTimer->SetInterval(10);
@@ -367,6 +373,7 @@ void ApiListener::Stop(bool runtimeDeleted)
 	m_CleanupCertificateRequestsTimer->Stop(true);
 	m_AuthorityTimer->Stop(true);
 	m_ReconnectTimer->Stop(true);
+	m_DeletedRuntimeObjectsTimer->Stop(true);
 	m_Timer->Stop(true);
 	m_RenewOwnCertTimer->Stop(true);
 

lib/remote/apilistener.hpp

@@ -183,6 +183,7 @@ class ApiListener final : public ObjectImpl<ApiListener>
 	std::set<HttpServerConnection::Ptr> m_HttpClients;
 
 	Timer::Ptr m_Timer;
+	Timer::Ptr m_DeletedRuntimeObjectsTimer;
 	Timer::Ptr m_ReconnectTimer;
 	Timer::Ptr m_AuthorityTimer;
 	Timer::Ptr m_CleanupCertificateRequestsTimer;
@@ -204,6 +205,10 @@ class ApiListener final : public ObjectImpl<ApiListener>
 	void CleanupCertificateRequestsTimerHandler();
 	void CheckApiPackageIntegrity();
 
+	void PruneDeletedRuntimeObjects();
+	double GetRuntimeObjectDeletionTs(const String& typeName, const String& objName);
+	bool UpdateRuntimeObjectDeletionTs(const String& typeName, const String& objName, double ts);
+
 	bool AddListener(const String& node, const String& service);
 	void StopListener();
 	void AddConnection(const Endpoint::Ptr& endpoint);

lib/remote/apilistener.ti

@@ -63,6 +63,10 @@ class ApiListener : ConfigObject
 	[no_user_modify] String identity;
 
 	[state, no_user_modify] Dictionary::Ptr last_failed_zones_stage_validation;
+
+	[state, no_user_modify] Dictionary::Ptr deleted_runtime_objects {
+		default {{{ return new Dictionary(); }}}
+	};
 };
 
 }