[Rushaway]

Summary

• Server lost VIP on a failed DB connect (vip/Database.sp): a failed connect called SetFailState, which unloaded VIP-Core until a manual reload (the retry was commented out). It now logs the error and retries on a timer with a backoff (5 s, 10 s, 20 s ... capped at 2 min), and clears IS_LOADING so DB_Connect() can run again.
• Charset (vip/Database.sp): SET NAMES + SET CHARSET were queued and SetCharset() called after the CREATE TABLE. SetCharset() now runs once, in the connect callback, before any query, so it survives the driver's auto-reconnect. SET NAMES is kept only as a fallback, and the redundant SET CHARSET is gone.
• Stale VIP cache after edits (vip/AdminMenu.sp, vip/API.sp, vip/UTIL.sp, vip/Clients.sp): group/time changes from the admin menu, VIP_SetClientVIPGroup / VIP_SetClientAccessTime, and sm_addvip / sm_setvip updated vip_users without touching g_hVIPCache. The follow-up reload then read the old group or expiry from the cache. The admin-menu group change also reloaded the player before the UPDATE had even run.
  • Each success callback now drops the account from the cache and then reloads the player.
  • The group change now reloads from a new SQL_Callback_ChangeGroup.
  • A cache miss in Clients_LoadClient now stores the row it reads (bUpdateCache = true), so the cache fills back in straight away.
  • The time-change callback finds the target by account id rather than through the admin's menu data, which may have moved to another player.
• Expiry cleanup N+1 (vip/Database.sp): every expired/outdated VIP was deleted with its own DELETE. This ran on every map start, round restart and DB connect. Now there is one Transaction per cleanup query, and logging and cache removal still happen per row. Added non-unique KEY expires / KEY lastvisit so the cleanup SELECTs don't scan the whole table:
  • in CREATE TABLE
  • as an async ALTER TABLE ... ADD INDEX for existing MySQL installs, ignoring error 1061
  • as CREATE INDEX IF NOT EXISTS on SQLite
• DataPack leaks / skipped cache update (vip/Database.sp):
  • SQL_Callback_SelectRemoveClient leaked its pack when the row was not found.
  • SQL_Callback_RemoveClient returned early (leaking the pack) when the admin had left (iAdmin == -1), skipping both the log line and DB_RemoveVIPFromCache. It now logs and updates the cache and only skips the reply.
• Null result set use (vip/CMD.sp, vip/API.sp): SQL_Callback_OnSelectRemoveClient and SQL_Callback_ChangeClientSettings logged the error and then went on to call methods on a null hResult. Both now return.
• Unescaped search (vip/adminmenu/List.sp): the admin VIP-list name search built LIKE '%...%' with FormatEx straight from chat input, so a ' broke the query. It now uses g_hDatabase.Format (literal % kept as %%).
• Small fixes:
  • DB_UpdateClient passed DBPrio_Low as the data argument; it now uses Query(cb, q, _, DBPrio_Low).
  • DB_LoadAllVIPsToCache no longer fetches expired rows, which the callback discarded anyway.
  • The group UPDATEs in AdminMenu.sp and API.sp now use g_hDatabase.Format.
  • Added g_hDatabase null checks before the queries touched here.
• Version bumped to 3.1.6 R.

Not included

• SQLite_UseDatabase("vip_core") (synchronous; used only when databases.cfg has no vip_core entry): Database.Connect with a missing entry falls back to the default conf, not to a SQLite file. Making it async would mean either asking admins to add a vip_core SQLite entry, or moving existing SQLite users onto another database file. Both change the setup, so it is left as is.
• Cache filter without USE_MORE_SERVERS: DB_LoadAllVIPsToCache only adds the sid filter when USE_MORE_SERVERS is 1 (the default). Builds compiled with 0 would cache other servers' VIPs too. This is pre-existing and not touched here.

Test plan

• Compiles with spcomp 1.12.0.7259 (same deps as CI), no warnings
• CI build passes
• Wrong DB credentials: the plugin stays loaded, logs the error, retries with growing delays, and connects once the credentials are fixed
• Existing MySQL install: on first start SHOW INDEX FROM vip_users lists expires and lastvisit, and later restarts log no error (1061 ignored)
• Admin menu: change a connected VIP's group or time. The player is reloaded with the new values (check with sm_vip_dump_cache and the VIP menu)
• sm_setvip / sm_addvip on an existing VIP: the new group/expiry applies at once, not only after the next map
• sm_delvip on an unknown id: "not found" reply, no error. Delete a VIP while the admin disconnects: the log line is written and the cache entry removed
• With sm_vip_delete_expired set and several expired rows: one transaction deletes them all, and every row gets its log line
• Admin VIP-list search with a name containing ': the query works

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[WareZ57]

I read this against master (6f08232, merges cleanly) and compiled VIP_Core.sp with spcomp 1.12.0.7259: builds clean. I also ran the schema bits on a throwaway MySQL 8.4. The new CREATE TABLE with KEY expires / KEY lastvisit works, and the follow-up ALTER TABLE ... ADD INDEX then fails with ERROR 1061: Duplicate key name 'expires', which SQL_Callback_AddIndex matches.

Things I checked that look good:

• Reconnect: DB_Connect() is only called from OnPluginStart and Timer_DB_Reconnect, and a timer is only created on a failed connect, so there is a single retry chain with a capped backoff and nothing stacks across maps. The timer dies with the plugin. IS_LOADING is cleared on failure, so the next try isn't blocked.
• Cache invalidation: every write path to vip_users now drops the account from g_hVIPCache in its success callback before reloading: admin menu group/time, the two natives, sm_addvip/sm_setvip through SQL_Callback_OnVIPClientAdded, and deletes. SM runs all threaded queries on one worker, in order, so a full DB_LoadAllVIPsToCache racing with an UPDATE always delivers its callback before the UPDATE's invalidation.
• Expiry transaction: one DataPack per query, freed on both success (DB_OnClientRemoved) and failure. GET_CID(REASON_EXPIRED) stays negative, so no reply goes out. The pack layout matches DB_RemoveClientFromID.
• DataPack leak fixes, null-result returns in CMD.sp/API.sp, DB_UpdateClient priority argument, g_hDatabase.Format on the group UPDATEs and the name search: all correct.

Two things I'd still change:

1. The group change can reload the wrong player (AdminMenu.sp:574-584 → SQL_Callback_ChangeGroup :604-634)

The PR fixed this for the time change, which now resolves the target by account id because the admin's menu data "may have moved to another player". The group change still writes DATA_KEY_TargetUID into the pack and prefers it in the callback. That key can be stale: in the shared list handler, picking a DB entry (adminmenu/List.sp:118) only sets DATA_KEY_TargetID and never clears the TargetUID left behind by an earlier online pick (List.sp:97). Only InitiateDataMap clears it, and that runs only when entering from the top admin menu.

Scenario:

• the admin opens the VIP list and selects online VIP A;
• goes back, picks "show all", and selects VIP B from the DB list;
• changes B's group.

The UPDATE hits B's row, but SQL_Callback_ChangeGroup resolves iTarget = CID(A's userid). So it runs ResetClient(A), fires VIP_OnVIPClientRemoved(A, "VIP-Group Changed") and reloads A. If B is online, B keeps the old group until reconnect. The same bug exists on master, but this code is being rewritten here anyway.

Fix: resolve the target only by account id, as SQL_Callback_ChangeTime now does. Drop the TargetUID cell, or at least check it:

int iTarget = CID(iTargetUID);
if (!iTarget || GetSteamAccountID(iTarget) != iTargetID)
{
	iTarget = IsClientOnline(iTargetID);
}

2. Without SetFailState, the DB-less period can now be reached, and some paths still assume a handle

Before, a failed connect unloaded the core, so nothing could run without a database. Now the plugin stays up with g_hDatabase == null until a retry succeeds. The paths this PR touched are guarded, but these still call methods on the null handle and throw "Invalid database Handle":

• sm_addvip / sm_setvip and VIP_GiveClientVIP with add-to-DB: UTIL_ADD_VIP_PLAYER / UTIL_SET_VIP_PLAYER (UTIL.sp:315/354 and :416/455). These also leak the DataPack created just before the Format.
• sm_delvip (CMD.sp:221).
• ShowTargetInfo from the online VIP list (adminmenu/List.sp:330).

It only happens while the DB is down at startup, so it's low impact. But a if (!g_hDatabase) { reply "database not connected"; return; } at the top of those functions (before the new DataPack() in the UTIL ones) would make the "plugin stays loaded" behaviour clean.

CI is green and master hasn't moved. Once 1 is fixed I'm happy with this.