From c47644ea46d2a132ffe6fce8d0166f5617b0920c Mon Sep 17 00:00:00 2001
From: JJ <3617814+joligario@users.noreply.github.com>
Date: Sun, 20 Aug 2023 20:21:51 -0400
Subject: [PATCH] [Instances] Honor reserved instances (#3563)

* [Instances] Honor reserved instances

Logic to select next available instance id was incorrect.
The correct logic selected the max id + 1 or max reserved + 1, but then it would overwrite if we enabled recycling ids.
Additionally, it was incrementing the reserved ids and assigning ids to the max reserved id vice the next available.
Finally, it was running the logic twice.

* Fix updated SQL to use fmt
---
 common/database_instances.cpp | 73 ++++++++++++-----------------------
 1 file changed, 24 insertions(+), 49 deletions(-)

diff --git a/common/database_instances.cpp b/common/database_instances.cpp
index 4b2992303..25cbaa7f3 100644
--- a/common/database_instances.cpp
+++ b/common/database_instances.cpp
@@ -131,85 +131,60 @@ bool Database::CreateInstance(uint16 instance_id, uint32 zone_id, uint32 version
 bool Database::GetUnusedInstanceID(uint16 &instance_id)
 {
 	uint32 max_reserved_instance_id = RuleI(Instances, ReservedInstances);
-	uint32 max                      = 32000;
+	uint32 max_instance_id          = 32000;
 
+	// sanity check reserved
+	if (max_reserved_instance_id >= max_instance_id) {
+		instance_id = 0;
+		return false;
+	}
+	
+	// initial query - get max unused id above reserved
 	auto query = fmt::format(
 		"SELECT IFNULL(MAX(id), {}) + 1 FROM instance_list WHERE id > {}",
 		max_reserved_instance_id,
 		max_reserved_instance_id
 	);
 
+	// recycle instances - change query to get first unused id above reserved
 	if (RuleB(Instances, RecycleInstanceIds)) {
-		query = (
+		query = fmt::format(
 			SQL(
-				SELECT i.id + 1 AS next_available
+				SELECT MIN(i.id + 1) AS next_available
 				FROM instance_list i
-				LEFT JOIN instance_list i2 ON i2.id = i.id + 1
-				WHERE i2.id IS NULL
-				ORDER BY i.id
-				LIMIT 0, 1;
-
-			)
+				LEFT JOIN instance_list i2 ON i.id + 1 = i2.id
+				WHERE i.id > {}
+				AND i2.id IS NULL;
+			),
+			RuleI(Instances, ReservedInstances)
 		);
 	}
 
 	auto results = QueryDatabase(query);
 
+	// could not successfully query - bail out
 	if (!results.Success()) {
 		instance_id = 0;
 		return false;
 	}
 
+	// no instances running - assign first id above reserved
 	if (results.RowCount() == 0) {
-		instance_id = max_reserved_instance_id;
+		instance_id = max_reserved_instance_id + 1;
 		return true;
 	}
 
 	auto row = results.begin();
 
-	if (Strings::ToInt(row[0]) <= max) {
+	// check that id is within limits
+	if (Strings::ToInt(row[0]) <= max_instance_id) {
 		instance_id = Strings::ToInt(row[0]);
-
 		return true;
 	}
 
-	if (instance_id < max_reserved_instance_id) {
-		instance_id = max_reserved_instance_id;
-		return true;
-	}
-
-	query   = fmt::format("SELECT id FROM instance_list where id > {} ORDER BY id", max_reserved_instance_id);
-	results = QueryDatabase(query);
-
-	if (!results.Success()) {
-		instance_id = 0;
-		return false;
-	}
-
-	if (results.RowCount() == 0) {
-		instance_id = 0;
-		return false;
-	}
-
-	max_reserved_instance_id++;
-
-	for (auto row : results) {
-		if (max_reserved_instance_id < Strings::ToUnsignedInt(row[0])) {
-			instance_id = max_reserved_instance_id;
-			return true;
-		}
-
-		if (max_reserved_instance_id > max) {
-			instance_id = 0;
-			return false;
-		}
-
-		max_reserved_instance_id++;
-	}
-
-	instance_id = max_reserved_instance_id;
-
-	return true;
+	// unhandled situation - should not reach here
+	instance_id = 0;
+	return false;
 }
 
 bool Database::IsGlobalInstance(uint16 instance_id)