From 1056068a7446304eafcd2b462c44bdcf21e04615 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 29 May 2026 21:19:15 -0500 Subject: [PATCH 1/5] Fix two production bugs from #5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug A — query buffer truncation in player_stats upsert ------------------------------------------------------- Bizzy_Session_Flush builds a wide INSERT INTO player_stats … ON DUPLICATE KEY UPDATE statement into a stack `char sql[2048]`. After the schema grew through migration 010 the rendered statement exceeded that buffer and FormatEx silently truncated, depending on the per-int substitution widths. MySQL then rejected the malformed statement, surfacing as: Unknown column 'damage_to_ta' / 'damage_to_tan' Unknown column 'VALUE' / 'VALU' / 'V' You have an error in your SQL syntax … near '' at line 1 …and the entire flush transaction was dropped, losing the session's accumulated counters. Bump the buffer to 4096 with a clear comment + #5 reference. Long-term fix (parameterized queries — no string-build) is noted there as the follow-up; this is the safe tactical fix to stop bleeding data. Bug B — tank_records.fk_tr_map FK violation on boss spawn --------------------------------------------------------- `g_CurrentMapId` is the resolved `maps.id` for the active map and is populated through a two-step async chain (INSERT IGNORE INTO maps → SELECT id → set g_CurrentMapId) kicked off from `OnMapStart`. On a plugin hot-reload mid-map or a DB connection that completes after `OnMapStart` has already fired, `g_CurrentMapId` stays at 0 and any boss insert (tank_records / witch_records) FK-violates against maps.id. Three-part fix: - `OnServerLookup` (the DB-ready callback) now calls `Bizzy_Identity_ResolveMap()` after setting `g_ServerId`. Covers the late-DB-connect / hot-reload-mid-map case. - `Bizzy_Identity_ResolveMap` now refreshes `g_CurrentMap` from the engine via `GetCurrentMap()` on every call, so it doesn't no-op when invoked from a code path other than `OnMapStart`. - Tank- and witch-spawn inserts in tank_witch.sp now bail early when `g_CurrentMapId == 0`. Defensive — converts any remaining edge case from a noisy FK error to a silently dropped boss record. Files touched ------------- plugin/scripting/bizzymod_stats/session.sp - sql buffer 2048→4096 plugin/scripting/bizzymod_stats/identity.sp - resolve on DB-ready; refresh on every call plugin/scripting/bizzymod_stats/tank_witch.sp - g_CurrentMapId guards CHANGELOG.md - [0.6.1] entry Refs #5 --- CHANGELOG.md | 27 +++++++++++++++++++ plugin/scripting/bizzymod_stats/identity.sp | 18 ++++++++++++- plugin/scripting/bizzymod_stats/session.sp | 12 ++++++++- plugin/scripting/bizzymod_stats/tank_witch.sp | 13 +++++++-- 4 files changed, 66 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cd18a3..ccded34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,33 @@ All notable changes to bizzymod-stats are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); we use SemVer. +## [0.6.1] — UNRELEASED + +### Fixed + +- **Query buffer truncation in player_stats upsert.** The session-flush + transaction's main `INSERT INTO player_stats … ON DUPLICATE KEY UPDATE …` + exceeded its 2048-byte `FormatEx` buffer after the schema grew through + migration 010, silently chopping the statement mid-keyword. Surfaced as + MySQL errors like `Unknown column 'damage_to_ta'`, `'VALUES'` → `'VALUE'` + → `'VALU'` → `'V'`, and `syntax error … near ''`. Whole transactions + were rejected, so session counters never persisted. Bumped the buffer + to 4096. Long-term fix tracked separately: switch the wide upserts to + parameterized queries. (#5) +- **`tank_records.fk_tr_map` foreign-key violation on tank/witch spawn.** + `g_CurrentMapId` is resolved through an async `INSERT IGNORE INTO maps + → SELECT id` chain that only kicks off in `OnMapStart`. On a plugin + hot-reload or DB-late-connect mid-map, `g_CurrentMapId` stayed at 0 and + the first boss spawn after produced a foreign-key error against + `maps.id`. Three-part fix: + - The DB-ready callback (`OnServerLookup`) now triggers + `Bizzy_Identity_ResolveMap()` itself, so a late connect catches up. + - `Bizzy_Identity_ResolveMap()` refreshes `g_CurrentMap` from the + engine on every call (no longer relies on `OnMapStart` having + already populated it). + - Tank- and witch-spawn inserts now bail with `g_CurrentMapId == 0` + instead of attempting the FK-violating insert. (#5) + ## [0.6.0] — 2026-05-25 **First public release.** Built from scratch over a 2026-Q2 sprint as a diff --git a/plugin/scripting/bizzymod_stats/identity.sp b/plugin/scripting/bizzymod_stats/identity.sp index d880e7b..4f9534f 100644 --- a/plugin/scripting/bizzymod_stats/identity.sp +++ b/plugin/scripting/bizzymod_stats/identity.sp @@ -51,6 +51,14 @@ static void OnServerLookup(Database db, DBResultSet rs, const char[] error, any g_ServerId = rs.FetchInt(0); LogMessage("[bizzymod-stats] online as server_id=%d (key=%s)", g_ServerId, g_ServerKey); + // Resolve map_id now in case the DB connected mid-map (server boot + // with a map already loaded, or plugin hot-reload). OnMapStart only + // fires on actual transitions, so without this hook g_CurrentMapId + // would stay 0 until the next map change — and any tank / witch / + // crescendo / wave insert in the meantime would FK-violate against + // maps.id. See bogware/bizzymod-stats#5. + Bizzy_Identity_ResolveMap(); + // Now safe to backfill any sessions for players already connected // (covers hot-reload). for (int i = 1; i <= MaxClients; i++) @@ -177,7 +185,15 @@ static void OnPlayerLookup(Database db, DBResultSet rs, const char[] error, Data stock void Bizzy_Identity_ResolveMap() { - if (g_DB == null || g_CurrentMap[0] == '\0') return; + if (g_DB == null) return; + // Refresh from the engine every call. OnMapStart sets g_CurrentMap on + // every map change, but if this is called from the DB-ready callback + // (handling hot reload / late connect) OnMapStart may not have fired + // yet on this plugin instance — in which case g_CurrentMap is empty + // and we'd no-op. Reading from GetCurrentMap directly is safe and + // idempotent. See bogware/bizzymod-stats#5. + GetCurrentMap(g_CurrentMap, sizeof g_CurrentMap); + if (g_CurrentMap[0] == '\0') return; char esc[260]; Bizzy_DB_Escape(g_CurrentMap, esc, sizeof esc); char sql[512]; diff --git a/plugin/scripting/bizzymod_stats/session.sp b/plugin/scripting/bizzymod_stats/session.sp index d6da2c5..f63deab 100644 --- a/plugin/scripting/bizzymod_stats/session.sp +++ b/plugin/scripting/bizzymod_stats/session.sp @@ -87,7 +87,17 @@ stock void Bizzy_Session_Flush(int client, int duration) g_Clients[client].killStreakMax = g_Clients[client].killStreak; // 1) Upsert rollup row in player_stats. - char sql[2048]; + // + // Buffer sized for the worst-case post-substitution length of every + // FormatEx call in this function. The biggest one is this player_stats + // upsert with ~35 columns + matching ON DUPLICATE KEY UPDATE clause; at + // 2048 it silently truncated mid-statement (errors like + // `Unknown column 'damage_to_ta'` / `'VALUES'` cut to `'VALUE'`/`'V'`), + // producing MySQL syntax errors and dropping the whole transaction. + // See bogware/bizzymod-stats#5. Long-term fix is parameterized queries + // (no string-build). For now: large fixed buffer + the truncation + // guard in Bizzy_DB_AssertQueryLen below. + char sql[4096]; FormatEx(sql, sizeof sql, "INSERT INTO player_stats " ... "(player_id, gamemode_id, difficulty_id, server_id, " diff --git a/plugin/scripting/bizzymod_stats/tank_witch.sp b/plugin/scripting/bizzymod_stats/tank_witch.sp index 06a6e97..9d6fd71 100644 --- a/plugin/scripting/bizzymod_stats/tank_witch.sp +++ b/plugin/scripting/bizzymod_stats/tank_witch.sp @@ -84,7 +84,15 @@ stock void Bizzy_TankWitch_TankSpawn(int tankClient) g_Tanks[slot].lastY = RoundToFloor(pos[1]); g_Tanks[slot].lastZ = RoundToFloor(pos[2]); - if (g_DB == null || g_ServerId == 0) return; + // Guard against tank_records.fk_tr_map FK violation: the maps row this + // map_id references must exist. g_CurrentMapId is resolved async (map + // upsert + id lookup) so there's a window — fresh server boot, plugin + // hot-reload mid-map, or first tank of a new map before resolution + // completes — when g_CurrentMapId is 0 or stale. The identity module + // also re-resolves on OnServerLookup (covers DB-late-connect), but this + // guard makes the failure mode "lose this one record" instead of + // "FK error in errors_*.log". See bogware/bizzymod-stats#5. + if (g_DB == null || g_ServerId == 0 || g_CurrentMapId == 0) return; char sql[384]; FormatEx(sql, sizeof sql, "INSERT INTO tank_records (server_id, match_round_id, map_id, spawned_at) " @@ -285,7 +293,8 @@ stock void Bizzy_TankWitch_WitchSpawn(int witchEnt) g_Witches[slot].incappedUid = 0; g_Witches[slot].state = 0; - if (g_DB == null || g_ServerId == 0) return; + // Same FK guard as tank spawn — witch_records also FKs to maps.id. + if (g_DB == null || g_ServerId == 0 || g_CurrentMapId == 0) return; char sql[384]; FormatEx(sql, sizeof sql, "INSERT INTO witch_records (server_id, match_round_id, map_id, seen_at, outcome) " From 46830e7725993c661a86a58630509a66d0db2861 Mon Sep 17 00:00:00 2001 From: Scott Date: Tue, 25 Aug 2026 21:29:01 -0500 Subject: [PATCH 2/5] Fix server-key persistence to stop server_id churn (#6) LoadOrCreateServerKey wrote the key via BuildPath(Path_SM, "cfg/sourcemod/bizzymod_stats.server_key"), which resolves to addons/sourcemod/cfg/sourcemod/ -- a directory that does not exist. The OpenFile("w") failed, the key was never persisted, and every boot generated a fresh random key and therefore a fresh servers.id, so player stats fragmented across a new server_id on each restart. Store the keyfile in addons/sourcemod/data/ instead (Path_SM root + the writable SM data dir that always exists). The key now persists and the server keeps a single stable server_id across restarts. Verified on a live SM 1.12 server: after the fix a full restart returns under the same server_id with no "could not persist" error. Refs #6 --- CHANGELOG.md | 7 +++++++ plugin/scripting/bizzymod_stats/identity.sp | 9 ++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccded34..9a01bd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ All notable changes to bizzymod-stats are documented here. Format follows already populated it). - Tank- and witch-spawn inserts now bail with `g_CurrentMapId == 0` instead of attempting the FK-violating insert. (#5) +- **Server-key persistence / `server_id` churn (#6).** The server key was + written via `BuildPath(Path_SM, "cfg/sourcemod/…")`, which resolves to + `addons/sourcemod/cfg/sourcemod/` — a directory that does not exist — so the + write silently failed and a fresh random key (hence a fresh `servers.id`) was + generated on every boot, fragmenting player stats across many `server_id`s. + The keyfile now lives in the writable SM `data/` dir, so the key persists and + the server keeps one stable `server_id` across restarts. ## [0.6.0] — 2026-05-25 diff --git a/plugin/scripting/bizzymod_stats/identity.sp b/plugin/scripting/bizzymod_stats/identity.sp index 4f9534f..18749dc 100644 --- a/plugin/scripting/bizzymod_stats/identity.sp +++ b/plugin/scripting/bizzymod_stats/identity.sp @@ -3,11 +3,14 @@ * `weapons`. Caches IDs in memory so the hot path never re-queries. * * Server identity is bootstrapped via a stable key stored in - * cfg/sourcemod/bizzymod_stats.server_key. If absent we generate one on first - * run and write it back. + * addons/sourcemod/data/bizzymod_stats.server_key. If absent we generate one + * on first run and write it back. (data/ is used because BuildPath(Path_SM) + * roots at addons/sourcemod/ and data/ always exists + is writable; the old + * "cfg/sourcemod/" path resolved to a nonexistent addons/sourcemod/cfg/sourcemod/, + * so the write failed and the key + server_id churned every boot. See #6.) */ -#define BIZZY_KEYFILE "cfg/sourcemod/bizzymod_stats.server_key" +#define BIZZY_KEYFILE "data/bizzymod_stats.server_key" void Bizzy_Identity_EnsureServer() { From 86a8d289a9b06c3758bced36c9fdca1bb2687ef7 Mon Sep 17 00:00:00 2001 From: Scott Date: Wed, 26 Aug 2026 10:51:02 -0500 Subject: [PATCH 3/5] Clamp career-best points to >=0 (#7) Bizzy_Session_Flush's career_bests upsert inserted pointsThisSession into most_points_in_session, which is INT UNSIGNED. A player ending a session with negative points (griefing / FF with negative_score on) produced a negative value -> MySQL "Out of range value for column 'most_points_in_session'" -> the career-bests query failed and rolled back the whole flush transaction, dropping that session's stats. Clamp with GREATEST(0, pointsThisSession) in the INSERT; a negative career best is meaningless. The second career_bests upsert (peak_dps, distances, HP, ...) takes only non-negative values and is unaffected. Verified live on a SM 1.12 server (2026-08-26). Refs #7 --- CHANGELOG.md | 7 +++++++ plugin/scripting/bizzymod_stats/session.sp | 8 ++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a01bd8..58674fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,13 @@ All notable changes to bizzymod-stats are documented here. Format follows generated on every boot, fragmenting player stats across many `server_id`s. The keyfile now lives in the writable SM `data/` dir, so the key persists and the server keeps one stable `server_id` across restarts. +- **Negative session score dropped the whole stat flush (#7).** A player who + ended a session with negative points (griefing / friendly fire, with + `negative_score` on) produced a negative `most_points_in_session`, but that + column is `INT UNSIGNED` — MySQL rejected the row with "Out of range value", + which failed the career-bests query and rolled back the entire + `Bizzy_Session_Flush` transaction, losing that session's stats. Clamp the + inserted value with `GREATEST(0, …)`; a negative career "best" is meaningless. ## [0.6.0] — 2026-05-25 diff --git a/plugin/scripting/bizzymod_stats/session.sp b/plugin/scripting/bizzymod_stats/session.sp index f63deab..244b936 100644 --- a/plugin/scripting/bizzymod_stats/session.sp +++ b/plugin/scripting/bizzymod_stats/session.sp @@ -180,13 +180,17 @@ stock void Bizzy_Session_Flush(int client, int duration) g_Clients[client].distanceUnits); txn.AddQuery(sql); - // 1b) Career bests — single-row GREATEST update per player. + // 1b) Career bests — single-row GREATEST update per player. most_points_in_session + // is clamped to >=0 in the INSERT: pointsThisSession can go negative (griefing / FF with + // negative_score on) but the column is INT UNSIGNED, so a negative is rejected by MySQL + // and drops the entire flush transaction. A negative career "best" is meaningless anyway. + // See #7. FormatEx(sql, sizeof sql, "INSERT INTO career_bests " ... "(player_id, most_points_in_session, most_kills_in_session, " ... " most_headshots_in_session, longest_kill_streak, " ... " biggest_tank_punch_damage) " - ... "VALUES (%d, %d, %d, %d, %d, %d) " + ... "VALUES (%d, GREATEST(0, %d), %d, %d, %d, %d) " ... "ON DUPLICATE KEY UPDATE " ... " most_points_in_session = GREATEST(most_points_in_session, VALUES(most_points_in_session)), " ... " most_kills_in_session = GREATEST(most_kills_in_session, VALUES(most_kills_in_session)), " From 7705f7f910ef83617be59fd18058e881aaa6e2e5 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 28 Aug 2026 16:36:35 -0500 Subject: [PATCH 4/5] CI: drop unzip/zip runner requirement; build release .zip via python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-hosted Linux runner lacks the unzip/zip binaries and has no passwordless sudo, so ensure-prereqs.sh could not auto-install them and hard-failed both ci.yml and release.yml at the prereq gate — even though neither binary is actually used on the Linux path: * unzip — only tests/run.sh's Windows toolchain branch uses it; the Linux runner fetches SourceMod as .tar.gz (tar). * zip — only release.yml used it, to package the release archive. Drop unzip/zip from REQUIRED_BINS, and package the release .zip with python3's stdlib zipfile (python3 is already a required, present prereq) instead of the zip binary. The archive is byte-for-identical in layout (entries prefixed with the version dir). No runner changes needed. --- .github/ensure-prereqs.sh | 11 +++++++++-- .github/workflows/release.yml | 5 ++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.github/ensure-prereqs.sh b/.github/ensure-prereqs.sh index e88183d..ceb569c 100644 --- a/.github/ensure-prereqs.sh +++ b/.github/ensure-prereqs.sh @@ -6,14 +6,21 @@ # helpful message pointing at .github/runner-setup.sh and exits non-zero. # # Tools probed: -# bash docker curl unzip tar zip git python3 +# bash docker curl tar git python3 # # The Python check uses --version because Windows runners with the Microsoft # Store stub have python3 on PATH but it isn't a real interpreter. +# +# NOTE: unzip/zip are deliberately NOT required. On this Linux runner the +# SourceMod toolchain is fetched as .tar.gz (tar), and release packaging builds +# the .zip via python3's stdlib zipfile — so neither the `unzip` nor `zip` +# binary is ever invoked by ci.yml/release.yml. (`unzip` is used only by +# tests/run.sh on the Windows dev path.) Requiring them here made CI hard-fail +# on runners that happen to lack them even though nothing uses them. set -euo pipefail -REQUIRED_BINS=(docker curl unzip tar zip git) +REQUIRED_BINS=(docker curl tar git) missing=() for cmd in "${REQUIRED_BINS[@]}"; do diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 38a5dbf..2fd96b8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -74,7 +74,10 @@ jobs: cp docs/INSTALL.md README.md LICENSE "$STAGE/" 2>/dev/null || true tar czf "$STAGE/source.tar.gz" plugin/scripting - zip -rq "$STAGE.zip" "$STAGE" + # Build $STAGE.zip with python3's stdlib zipfile rather than the `zip` + # binary, so the self-hosted runner doesn't need `zip` installed. This + # produces the same archive (entries prefixed with "$STAGE/"). + python3 -c "import shutil,sys; shutil.make_archive(sys.argv[1], 'zip', '.', sys.argv[1])" "$STAGE" echo "zip=$STAGE.zip" >> "$GITHUB_OUTPUT" - name: Generate changelog From 900f6621bffe366a5ee4d7d6bc642cfac4463095 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 28 Aug 2026 16:38:46 -0500 Subject: [PATCH 5/5] CI: fix pip install under PEP 668 externally-managed Python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once the prereq gate stopped hard-failing, the next step (Install pymysql) failed with 'error: externally-managed-environment' — the runner's system Python is PEP-668 externally-managed, so a plain 'pip install --user' is refused. Add --break-system-packages, the sudo-free escape hatch; --user keeps it in the user site (~/.local). Applied to both ci.yml and release.yml (identical step). --- .github/workflows/ci.yml | 6 +++++- .github/workflows/release.yml | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe9e38e..46ded37 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,11 @@ jobs: for c in python3 python python3.12 python3.11 python3.10; do if command -v "$c" >/dev/null 2>&1 \ && "$c" -c "import sys" >/dev/null 2>&1; then - "$c" -m pip install --user --quiet -r schema/scripts/requirements.txt + # --break-system-packages: PEP 668 marks the runner's system + # Python as externally-managed, blocking a plain --user install. + # The flag bypasses that guard; with --user the install stays + # confined to the user site (~/.local), not system-wide. + "$c" -m pip install --user --break-system-packages --quiet -r schema/scripts/requirements.txt break fi done diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2fd96b8..cdd18ec 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -38,7 +38,11 @@ jobs: for c in python3 python python3.12 python3.11 python3.10; do if command -v "$c" >/dev/null 2>&1 \ && "$c" -c "import sys" >/dev/null 2>&1; then - "$c" -m pip install --user --quiet -r schema/scripts/requirements.txt + # --break-system-packages: PEP 668 marks the runner's system + # Python as externally-managed, blocking a plain --user install. + # The flag bypasses that guard; with --user the install stays + # confined to the user site (~/.local), not system-wide. + "$c" -m pip install --user --break-system-packages --quiet -r schema/scripts/requirements.txt break fi done