From 4bbb72ee147bbb680b2778a0679653d98aba344f Mon Sep 17 00:00:00 2001 From: Rushaway Date: Wed, 28 Oct 2026 15:42:15 +0100 Subject: [PATCH] fix(sql): fix stale VIP cache, leaks and connection handling - Retry failed connects with a backoff instead of SetFailState - Set the charset once per connection, drop SET CHARSET - Invalidate the VIP cache after group/time/add updates, reload after the UPDATE completes - Delete expired/outdated VIPs in one transaction, index expires/lastvisit - Fix DataPack leaks and the skipped cache update when the admin left - Return on query errors before using a null result set - Escape the admin list name search, pass DBPrio_Low as priority Co-Authored-By: Claude Opus 5.5 --- addons/sourcemod/scripting/VIP_Core.sp | 2 +- addons/sourcemod/scripting/vip/API.sp | 39 ++++-- addons/sourcemod/scripting/vip/AdminMenu.sp | 94 ++++++++----- addons/sourcemod/scripting/vip/CMD.sp | 1 + addons/sourcemod/scripting/vip/Clients.sp | 4 +- addons/sourcemod/scripting/vip/Database.sp | 130 +++++++++++++++--- addons/sourcemod/scripting/vip/UTIL.sp | 3 + .../sourcemod/scripting/vip/adminmenu/List.sp | 7 +- 8 files changed, 211 insertions(+), 69 deletions(-) diff --git a/addons/sourcemod/scripting/VIP_Core.sp b/addons/sourcemod/scripting/VIP_Core.sp index f219e6cf..36d541fe 100644 --- a/addons/sourcemod/scripting/VIP_Core.sp +++ b/addons/sourcemod/scripting/VIP_Core.sp @@ -7,7 +7,7 @@ #include #if !defined VIP_VERSION -#define VIP_VERSION "3.1.5 R" +#define VIP_VERSION "3.1.6 R" #endif diff --git a/addons/sourcemod/scripting/vip/API.sp b/addons/sourcemod/scripting/vip/API.sp index ab74334c..91a5cdad 100644 --- a/addons/sourcemod/scripting/vip/API.sp +++ b/addons/sourcemod/scripting/vip/API.sp @@ -419,12 +419,16 @@ public int Native_SetClientVIPGroup(Handle hPlugin, int iNumParams) if (view_as(GetNativeCell(3))) { int iClientID; - if (g_hFeatures[iClient].GetValue(KEY_CID, iClientID) && iClientID != -1) + if (g_hFeatures[iClient].GetValue(KEY_CID, iClientID) && iClientID != -1 && g_hDatabase) { char szQuery[256]; - FormatEx(SZF(szQuery), "UPDATE `vip_users` SET `group` = '%s' WHERE `account_id` = %d%s;", szGroup, iClientID, g_szSID); + g_hDatabase.Format(SZF(szQuery), "UPDATE `vip_users` SET `group` = '%s' WHERE `account_id` = %d%s;", szGroup, iClientID, g_szSID); DBG_SQL_Query(szQuery); - g_hDatabase.Query(SQL_Callback_ChangeClientSettings, szQuery, UID(iClient)); + + DataPack hPack = new DataPack(); + hPack.WriteCell(UID(iClient)); + hPack.WriteCell(iClientID); + g_hDatabase.Query(SQL_Callback_ChangeClientSettings, szQuery, hPack); if (g_CVAR_bLogsEnable) { @@ -483,12 +487,16 @@ public int Native_SetClientAccessTime(Handle hPlugin, int iNumParams) if (view_as(GetNativeCell(3))) { int iClientID; - if (g_hFeatures[iClient].GetValue(KEY_CID, iClientID) && iClientID != -1) + if (g_hFeatures[iClient].GetValue(KEY_CID, iClientID) && iClientID != -1 && g_hDatabase) { char szQuery[256]; FormatEx(SZF(szQuery), "UPDATE `vip_users` SET `expires` = %d WHERE `account_id` = %d%s;", iTime, iClientID, g_szSID); DBG_SQL_Query(szQuery); - g_hDatabase.Query(SQL_Callback_ChangeClientSettings, szQuery, UID(iClient)); + + DataPack hPack = new DataPack(); + hPack.WriteCell(UID(iClient)); + hPack.WriteCell(iClientID); + g_hDatabase.Query(SQL_Callback_ChangeClientSettings, szQuery, hPack); } } @@ -499,19 +507,32 @@ public int Native_SetClientAccessTime(Handle hPlugin, int iNumParams) return false; } -public void SQL_Callback_ChangeClientSettings(Database hOwner, DBResultSet hResult, const char[] szError, any iClient) +public void SQL_Callback_ChangeClientSettings(Database hOwner, DBResultSet hResult, const char[] szError, DataPack hPack) { DBG_SQL_Response("SQL_Callback_SelectVipClientInfo"); - if (szError[0]) + + hPack.Reset(); + int iClient = CID(hPack.ReadCell()); + int iClientID = hPack.ReadCell(); + delete hPack; + + if (hResult == null || szError[0]) { LogError("SQL_Callback_ChangeClientSettings: %s", szError); + return; } DBG_SQL_Response("hResult.AffectedRows = %d", hResult.AffectedRows); - if ((iClient = CID(iClient)) && hResult.AffectedRows) + if (hResult.AffectedRows) { - Clients_CheckVipAccess(iClient, false); + // The row changed: drop the cached copy so the reload reads it from the database + DB_RemoveVIPFromCache(iClientID); + + if (iClient) + { + Clients_CheckVipAccess(iClient, false); + } } } diff --git a/addons/sourcemod/scripting/vip/AdminMenu.sp b/addons/sourcemod/scripting/vip/AdminMenu.sp index 926125cf..7e88ef17 100644 --- a/addons/sourcemod/scripting/vip/AdminMenu.sp +++ b/addons/sourcemod/scripting/vip/AdminMenu.sp @@ -456,10 +456,13 @@ public int MenuHandler_TimeMenu(Menu hMenu, MenuAction action, int iClient, int g_hClientData[iClient].GetValue(DATA_KEY_TargetID, iTarget); g_hClientData[iClient].SetValue(DATA_KEY_Time, iExpires); - char szQuery[512]; - FormatEx(SZF(szQuery), "UPDATE `vip_users` SET `expires` = '%d' WHERE `account_id` = '%d'%s;", iExpires, iTarget, g_szSID); - DBG_SQL_Query(szQuery); - g_hDatabase.Query(SQL_Callback_ChangeTime, szQuery, UID(iClient)); + if (g_hDatabase) + { + char szQuery[512]; + FormatEx(SZF(szQuery), "UPDATE `vip_users` SET `expires` = '%d' WHERE `account_id` = '%d'%s;", iExpires, iTarget, g_szSID); + DBG_SQL_Query(szQuery); + g_hDatabase.Query(SQL_Callback_ChangeTime, szQuery, iTarget); + } ShowTargetInfoMenu(iClient); } @@ -561,25 +564,24 @@ public int MenuHandler_GroupsList(Menu hMenu, MenuAction action, int iClient, in g_hClientData[iClient].GetString(DATA_KEY_Name, SZF(szName)); g_hClientData[iClient].GetString(DATA_KEY_Group, SZF(szOldGroup)); - FormatEx(SZF(szQuery), "UPDATE `vip_users` SET `group` = '%s' WHERE `account_id` = %d%s;", szGroup, iTargetID, g_szSID); + if (!g_hDatabase) + { + return 0; + } - DBG_SQL_Query(szQuery); - g_hDatabase.Query(SQL_Callback_ErrorCheck, szQuery); + g_hDatabase.Format(SZF(szQuery), "UPDATE `vip_users` SET `group` = '%s' WHERE `account_id` = %d%s;", szGroup, iTargetID, g_szSID); int iTarget = 0; g_hClientData[iClient].GetValue(DATA_KEY_TargetUID, iTarget); - iTarget = CID(iTarget); - if (!iTarget) - { - iTarget = IsClientOnline(iTargetID); - } - if (iTarget) - { - ResetClient(iTarget); - CreateForward_OnVIPClientRemoved(iTarget, "VIP-Group Changed", iClient); - Clients_CheckVipAccess(iTarget, false); - } + // Reload the target once the UPDATE is done, not before + DataPack hPack = new DataPack(); + hPack.WriteCell(UID(iClient)); + hPack.WriteCell(iTarget); + hPack.WriteCell(iTargetID); + + DBG_SQL_Query(szQuery); + g_hDatabase.Query(SQL_Callback_ChangeGroup, szQuery, hPack); VIP_PrintToChatClient(iClient, "%t", "ADMIN_SET_GROUP", szName, szGroup); if (g_CVAR_bLogsEnable) @@ -599,7 +601,39 @@ public int MenuHandler_GroupsList(Menu hMenu, MenuAction action, int iClient, in return 0; } -public void SQL_Callback_ChangeTime(Database hOwner, DBResultSet hResult, const char[] szError, any UserID) +public void SQL_Callback_ChangeGroup(Database hOwner, DBResultSet hResult, const char[] szError, DataPack hPack) +{ + DBG_SQL_Response("SQL_Callback_ChangeGroup") + + hPack.Reset(); + int iClient = CID(hPack.ReadCell()); + int iTarget = CID(hPack.ReadCell()); + int iTargetID = hPack.ReadCell(); + delete hPack; + + if (szError[0]) + { + LogError("SQL_Callback_ChangeGroup: %s", szError); + return; + } + + // The row changed: drop the cached copy so the reload reads it from the database + DB_RemoveVIPFromCache(iTargetID); + + if (!iTarget) + { + iTarget = IsClientOnline(iTargetID); + } + + if (iTarget) + { + ResetClient(iTarget); + CreateForward_OnVIPClientRemoved(iTarget, "VIP-Group Changed", iClient); + Clients_CheckVipAccess(iTarget, false); + } +} + +public void SQL_Callback_ChangeTime(Database hOwner, DBResultSet hResult, const char[] szError, any iTargetID) { DBG_SQL_Response("SQL_Callback_ChangeTime") if (szError[0]) @@ -612,22 +646,14 @@ public void SQL_Callback_ChangeTime(Database hOwner, DBResultSet hResult, const if (hResult.AffectedRows) { - int iClient = CID(UserID); - if (iClient) - { - int iTarget; - g_hClientData[iClient].GetValue(DATA_KEY_TargetUID, iTarget); - iTarget = CID(iTarget); - if(!iTarget) - { - g_hClientData[iClient].GetValue(DATA_KEY_TargetID, iTarget); - iTarget = IsClientOnline(iTarget); - } + // The row changed: drop the cached copy so the reload reads it from the database + DB_RemoveVIPFromCache(iTargetID); - if (iTarget) - { - Clients_CheckVipAccess(iTarget, true); - } + // Resolve the target by account id: the admin may have left or opened another player since + int iTarget = IsClientOnline(iTargetID); + if (iTarget) + { + Clients_CheckVipAccess(iTarget, true); } } } diff --git a/addons/sourcemod/scripting/vip/CMD.sp b/addons/sourcemod/scripting/vip/CMD.sp index dd42a4de..4e2a9312 100644 --- a/addons/sourcemod/scripting/vip/CMD.sp +++ b/addons/sourcemod/scripting/vip/CMD.sp @@ -230,6 +230,7 @@ public void SQL_Callback_OnSelectRemoveClient(Database hOwner, DBResultSet hResu if (hResult == null || szError[0]) { LogError("SQL_Callback_OnSelectRemoveClient: %s", szError); + return; } if (iClient) diff --git a/addons/sourcemod/scripting/vip/Clients.sp b/addons/sourcemod/scripting/vip/Clients.sp index f793eb90..afdde489 100644 --- a/addons/sourcemod/scripting/vip/Clients.sp +++ b/addons/sourcemod/scripting/vip/Clients.sp @@ -75,8 +75,8 @@ void Clients_LoadClient(int iClient, bool bNotify) return; } - // If not in cache, query database - Clients_LoadClientFromDB(iClient, iAccountID, bNotify, false); + // If not in cache, query database (and cache the row it returns) + Clients_LoadClientFromDB(iClient, iAccountID, bNotify, true); } void Clients_LoadClientFromDB(int iClient, int iAccountID, bool bNotify, bool bUpdateCache) diff --git a/addons/sourcemod/scripting/vip/Database.sp b/addons/sourcemod/scripting/vip/Database.sp index f1c0696c..7983ba22 100644 --- a/addons/sourcemod/scripting/vip/Database.sp +++ b/addons/sourcemod/scripting/vip/Database.sp @@ -4,6 +4,8 @@ //#define CHARSET "utf8" //#define COLLATION "utf8_unicode_ci" +int g_iDBConnectRetries; + void DB_OnPluginStart() { DB_Connect(); @@ -43,12 +45,25 @@ public void OnDBConnect(Database hDatabase, const char[] szError, any data) { if (hDatabase == null || szError[0]) { - SetFailState("OnDBConnect %s", szError); + delete hDatabase; + g_hDatabase = null; UNSET_BIT(GLOBAL_INFO, IS_MySQL); - // CreateTimer(5.0, Timer_DB_Reconnect); + UNSET_BIT(GLOBAL_INFO, IS_LOADING); + + // Retry with a backoff: 5s, 10s, 20s, ... up to 2 minutes + float fDelay = 5.0 * float(1 << (g_iDBConnectRetries < 5 ? g_iDBConnectRetries : 5)); + if (fDelay > 120.0) + { + fDelay = 120.0; + } + g_iDBConnectRetries++; + + LogError("OnDBConnect: %s (retrying in %.0f seconds)", szError, fDelay); + CreateTimer(fDelay, Timer_DB_Reconnect); return; } + g_iDBConnectRetries = 0; g_hDatabase = hDatabase; if (data == 1) @@ -63,6 +78,12 @@ public void OnDBConnect(Database hDatabase, const char[] szError, any data) if (strcmp(szDriver, "mysql", false) == 0) { SET_BIT(GLOBAL_INFO, IS_MySQL); + + // Once per connection, before any query: the driver keeps it across auto-reconnects + if (!g_hDatabase.SetCharset(CHARSET)) + { + g_hDatabase.Query(SQL_Callback_ErrorCheck, "SET NAMES '" ... CHARSET ... "'", _, DBPrio_High); + } } else { @@ -75,6 +96,12 @@ public void OnDBConnect(Database hDatabase, const char[] szError, any data) CreateTables(); } +public Action Timer_DB_Reconnect(Handle hTimer) +{ + DB_Connect(); + return Plugin_Stop; +} + void CreateTables() { DBG_Database("CreateTables"); @@ -93,8 +120,14 @@ void CreateTables() `sid` INT UNSIGNED NOT NULL, \ `group` VARCHAR(64) NOT NULL, \ `expires` INT UNSIGNED NOT NULL default 0, \ - CONSTRAINT pk_PlayerID PRIMARY KEY (`account_id`, `sid`) \ + CONSTRAINT pk_PlayerID PRIMARY KEY (`account_id`, `sid`), \ + KEY `expires` (`expires`), \ + KEY `lastvisit` (`lastvisit`) \ ) DEFAULT CHARSET=" ... CHARSET ... ";", _, DBPrio_High); + + // Existing installs: add the indexes used by the expired/outdated cleanup (error 1061 = already there) + g_hDatabase.Query(SQL_Callback_AddIndex, "ALTER TABLE `vip_users` ADD INDEX `expires` (`expires`);", _, DBPrio_High); + g_hDatabase.Query(SQL_Callback_AddIndex, "ALTER TABLE `vip_users` ADD INDEX `lastvisit` (`lastvisit`);", _, DBPrio_High); } else { @@ -105,6 +138,18 @@ void CreateTables() `lastvisit` INTEGER UNSIGNED NOT NULL default 0, \ `group` VARCHAR(64) NOT NULL, \ `expires` INTEGER UNSIGNED NOT NULL default 0);", _, DBPrio_High); + + g_hDatabase.Query(SQL_Callback_AddIndex, "CREATE INDEX IF NOT EXISTS `vip_users_expires` ON `vip_users` (`expires`);", _, DBPrio_High); + g_hDatabase.Query(SQL_Callback_AddIndex, "CREATE INDEX IF NOT EXISTS `vip_users_lastvisit` ON `vip_users` (`lastvisit`);", _, DBPrio_High); + } +} + +public void SQL_Callback_AddIndex(Database hOwner, DBResultSet hResult, const char[] szError, any data) +{ + // 1061: Duplicate key name, the index already exists + if (szError[0] && StrContains(szError, "Duplicate key name", false) == -1) + { + LogError("SQL_Callback_AddIndex: %s", szError); } } @@ -118,14 +163,6 @@ public void SQL_Callback_TableCreate(Database hOwner, DBResultSet hResult, const return; } - if (GLOBAL_INFO & IS_MySQL) - { - g_hDatabase.Query(SQL_Callback_ErrorCheck, "SET NAMES '" ... CHARSET ... "'", _, DBPrio_High); - g_hDatabase.Query(SQL_Callback_ErrorCheck, "SET CHARSET '" ... CHARSET ... "'", _, DBPrio_High); - - g_hDatabase.SetCharset(CHARSET); - } - UNSET_BIT(GLOBAL_INFO, IS_LOADING); OnReadyToStart(); @@ -147,6 +184,11 @@ public void SQL_Callback_TableCreate(Database hOwner, DBResultSet hResult, const void RemoveExpAndOutPlayers() { + if (!g_hDatabase) + { + return; + } + if (g_CVAR_iDeleteExpired >= 0) { char szQuery[256]; @@ -195,7 +237,7 @@ void DB_UpdateClient(int iClient, const char[] szDbName = NULL_STRING) } DBG_SQL_Query(szQuery); - g_hDatabase.Query(SQL_Callback_ErrorCheck, szQuery, DBPrio_Low); + g_hDatabase.Query(SQL_Callback_ErrorCheck, szQuery, _, DBPrio_Low); } void DB_RemoveClientFromID(int iAdmin = 0, @@ -310,7 +352,10 @@ public void SQL_Callback_SelectRemoveClient(Database hOwner, DBResultSet hResult hPack.WriteString(szName); DB_RemoveClient(hPack, iClientID); + return; } + + delete hPack; } void DB_RemoveClient(DataPack hDataPack, int iClientID) @@ -333,9 +378,14 @@ public void SQL_Callback_RemoveClient(Database hOwner, DBResultSet hResult, cons return; } - DBG_SQL_Response("hResult.AffectedRows = %d", hResult.AffectedRows); + DB_OnClientRemoved(hPack, hResult.AffectedRows); +} + +void DB_OnClientRemoved(DataPack hPack, int iAffectedRows) +{ + DBG_SQL_Response("hResult.AffectedRows = %d", iAffectedRows); - if (hResult.AffectedRows) + if (iAffectedRows) { hPack.Reset(); @@ -347,11 +397,7 @@ public void SQL_Callback_RemoveClient(Database hOwner, DBResultSet hResult, cons hPack.ReadString(SZF(szName)); hPack.ReadString(SZF(szGroup)); - if(iAdmin == -1) - { - return; - } - + // iAdmin is -1 when the admin left: still log and update the cache, only skip the reply if (g_CVAR_bLogsEnable) { LogToFile(g_szLogFile, "%T", "LOG_VIP_DELETED", LANG_SERVER, szName, iClientID, szGroup, szAdmin); @@ -383,8 +429,12 @@ public void SQL_Callback_SelectExpiredAndOutdated(Database hOwner, DBResultSet h if (hResult.RowCount) { + // Delete every expired/outdated row in a single transaction instead of one query per row int iClientID; - char szName[MNL*2], szGroup[64]; + char szName[MNL*2], szGroup[64], szAdmin[PMP], szQuery[256]; + FormatEx(SZF(szAdmin), "%T", iReason == REASON_EXPIRED ? "REASON_EXPIRED":"REASON_INACTIVITY", LANG_SERVER); + + Transaction hTxn = new Transaction(); while (hResult.FetchRow()) { DBG_SQL_Response("hResult.FetchRow()"); @@ -394,8 +444,43 @@ public void SQL_Callback_SelectExpiredAndOutdated(Database hOwner, DBResultSet h DBG_SQL_Response("hResult.FetchString(1) = '%s'", szName); hResult.FetchString(2, SZF(szGroup)); DBG_SQL_Response("hResult.FetchString(2) = '%s'", szGroup); - DB_RemoveClientFromID(iReason, _, iClientID, false, szName, szGroup); + + // Same layout as the DataPack built by DB_RemoveClientFromID + DataPack hPack = new DataPack(); + hPack.WriteCell(iClientID); + hPack.WriteCell(iReason); + hPack.WriteCell(false); + hPack.WriteString(szAdmin); + hPack.WriteString(szName); + hPack.WriteString(szGroup); + + FormatEx(SZF(szQuery), "DELETE FROM `vip_users` WHERE `account_id` = %d%s;", iClientID, g_szSID); + DBG_SQL_Query(szQuery); + hTxn.AddQuery(szQuery, hPack); } + + g_hDatabase.Execute(hTxn, SQL_TxnCallback_RemoveClients, SQL_TxnCallback_RemoveClientsFailure, _, DBPrio_Low); + } +} + +public void SQL_TxnCallback_RemoveClients(Database hOwner, any data, int iNumQueries, DBResultSet[] hResults, any[] hQueryData) +{ + DBG_SQL_Response("SQL_TxnCallback_RemoveClients"); + + for (int i = 0; i < iNumQueries; ++i) + { + DB_OnClientRemoved(view_as(hQueryData[i]), hResults[i].AffectedRows); + } +} + +public void SQL_TxnCallback_RemoveClientsFailure(Database hOwner, any data, int iNumQueries, const char[] szError, int iFailIndex, any[] hQueryData) +{ + LogError("SQL_TxnCallback_RemoveClientsFailure: %s", szError); + + for (int i = 0; i < iNumQueries; ++i) + { + DataPack hPack = view_as(hQueryData[i]); + delete hPack; } } @@ -427,7 +512,8 @@ void DB_LoadAllVIPsToCache() #endif } - FormatEx(SZF(szQuery), "SELECT `account_id`, `expires`, `group`, `name` FROM `vip_users` WHERE 1=1%s;", szWhere); + // Expired VIPs are not cached anyway, so do not fetch them + FormatEx(SZF(szQuery), "SELECT `account_id`, `expires`, `group`, `name` FROM `vip_users` WHERE (`expires` = 0 OR `expires` > %d)%s;", GetTime(), szWhere); DBG_SQL_Query(szQuery); g_hDatabase.Query(SQL_Callback_LoadAllVIPsToCache, szQuery, _, DBPrio_High); diff --git a/addons/sourcemod/scripting/vip/UTIL.sp b/addons/sourcemod/scripting/vip/UTIL.sp index 54a4e959..59f436b4 100644 --- a/addons/sourcemod/scripting/vip/UTIL.sp +++ b/addons/sourcemod/scripting/vip/UTIL.sp @@ -522,6 +522,9 @@ public void SQL_Callback_OnVIPClientAdded(Database hOwner, DBResultSet hResult, return; } + // The row changed: drop the cached copy so the reload reads it from the database + DB_RemoveVIPFromCache(iAccountID); + if (iTarget > 0) { Clients_CheckVipAccess(iTarget, true); diff --git a/addons/sourcemod/scripting/vip/adminmenu/List.sp b/addons/sourcemod/scripting/vip/adminmenu/List.sp index b1b60a30..1548507a 100644 --- a/addons/sourcemod/scripting/vip/adminmenu/List.sp +++ b/addons/sourcemod/scripting/vip/adminmenu/List.sp @@ -193,6 +193,11 @@ public int MenuHandler_SearchPlayersListMenu(Menu hMenu, MenuAction action, int void ShowVipPlayersFromDBMenu(int iClient, int iOffset = 0) { LogMessage("ShowVipPlayersFromDBMenu"); + if (!g_hDatabase) + { + return; + } + g_hClientData[iClient].SetValue(DATA_KEY_MenuListType, MENU_TYPE_DB_LIST); g_hClientData[iClient].SetValue(DATA_KEY_Offset, iOffset); @@ -208,7 +213,7 @@ void ShowVipPlayersFromDBMenu(int iClient, int iOffset = 0) } else { - FormatEx(SZF(szWhere), " AND `name` LIKE '%%%s%%'", szSearch); + g_hDatabase.Format(SZF(szWhere), " AND `name` LIKE '%%%s%%'", szSearch); } }