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/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 38a5dbf..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 @@ -74,7 +78,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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cd18a3..58674fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,47 @@ 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) +- **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. +- **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 **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..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() { @@ -51,6 +54,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 +188,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..244b936 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, " @@ -170,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)), " 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) "