Fix player profile runtime PostgreSQL grouping

2026-06-09 11:23:39 +02:00
parent 5a52889cfb
commit 455a712913
3 changed files with 246 additions and 3 deletions
--- a/ai/tasks/done/TASK-200-fix-player-profile-runtime-postgres-grouping.md
+++ b/ai/tasks/done/TASK-200-fix-player-profile-runtime-postgres-grouping.md
@@ -0,0 +1,176 @@
+---
+id: TASK-200-fix-player-profile-runtime-postgres-grouping
+title: Fix player profile runtime PostgreSQL grouping
+status: done
+type: backend
+team: Backend Senior
+supporting_teams:
+  - Arquitecto de Base de Datos
+  - Arquitecto Python
+roadmap_item: foundation
+priority: high
+---
+
+# TASK-200 - Fix player profile runtime PostgreSQL grouping
+
+## Goal
+
+Corregir el `500` de `/api/stats/players/{player_id}?server_id={server}` cuando el jugador no tiene fila en `player_period_stats` para el timeframe solicitado y la ruta cae al fallback runtime sobre PostgreSQL.
+
+## Context
+
+`TASK-199` introdujo `player_period_stats` y el endpoint público ya usa correctamente el read model cuando existe una fila para el jugador/scope/timeframe solicitado. El problema aparece cuando el read model no tiene fila para ese jugador en el timeframe pedido y la lectura cae a `_get_rcon_materialized_player_stats_runtime(...)`.
+
+La causa raíz validada en producción es una query de `_fetch_player_stats(...)` en `backend/app/rcon_historical_player_stats.py` que usa una expresión equivalente a `COALESCE(MAX(stats.player_name), stats.player_id)` sin agregar ni agrupar `stats.player_id`. SQLite lo tolera, pero PostgreSQL lanza `psycopg.errors.GroupingError`, provocando un `500` para casos como `server_id=comunidad-hispana-01` cuando no hay fila `weekly` o `monthly` en `player_period_stats`.
+
+Preserve the current product identity: Spanish-speaking HLL Vietnam community, military/Vietnam/tactical/sober visual direction and controlled repository evolution.
+
+## Steps
+
+1. Inspect the listed files first.
+2. Fix the runtime fallback query used by `_fetch_player_stats(...)` so it is valid in PostgreSQL.
+3. Preserve SQLite/local compatibility when the explicit local path is used.
+4. Keep `player_period_stats` as the preferred source whenever a row exists.
+5. Keep runtime fallback when the read model is missing, incomplete or unavailable.
+6. Ensure the no-read-model-row case returns the existing controlled contract instead of a `500`.
+7. Validate the affected player-profile, player-search and ranking snapshot paths.
+8. Document the root cause, affected SQL, new behavior and production validation path.
+
+## Files to Read First
+
+- `AGENTS.md`
+- `ai/repo-context.md`
+- `ai/architecture-index.md`
+- `backend/app/rcon_historical_player_stats.py`
+- `backend/app/payloads.py`
+- `backend/app/routes.py`
+- `backend/app/postgres_rcon_storage.py`
+- `scripts/run-stats-validation.ps1`
+- `docs/player-period-stats-read-model-plan.md`
+- `ai/tasks/done/TASK-199-add-player-period-stats-read-model.md`
+
+## Expected Files to Modify
+
+- `backend/app/rcon_historical_player_stats.py`
+- `scripts/run-stats-validation.ps1`
+- `ai/tasks/in-progress/TASK-200-fix-player-profile-runtime-postgres-grouping.md`
+
+## Constraints
+
+- Keep the change minimal and backend-only.
+- Do not execute `ai-platform run`.
+- Do not modify frontend.
+- Do not change design.
+- Do not touch images or assets.
+- Do not reactivate Elo/MMR.
+- Do not reintroduce Comunidad Hispana `#03`.
+- Do not mix automation changes into this task.
+- Do not change public contracts except to avoid the runtime `500`.
+- Keep `player_period_stats` as the preferred source when data exists.
+- Keep runtime fallback active when the read model has no row for the requested scope/window.
+- If runtime also has no rows for that scope/window, return the existing controlled empty/zero contract instead of throwing.
+- Do not break:
+  - `/api/stats/players/search`
+  - `/api/stats/players/{player_id}`
+  - ranking snapshots
+  - `refresh-player-search-index`
+  - `refresh-player-period-stats`
+
+## Validation
+
+Before completing the task ensure:
+
+- `python -m py_compile backend/app/rcon_historical_player_stats.py`
+- `powershell -ExecutionPolicy Bypass -File scripts/run-stats-validation.ps1`
+- `powershell -ExecutionPolicy Bypass -File scripts/run-integration-tests.ps1`
+- local validation proves `_fetch_player_stats(...)` no longer emits PostgreSQL-invalid aggregate SQL
+- local validation proves the runtime fallback path does not throw when a server-specific read-model row is missing
+- local validation proves `player_period_stats` is still used when a row exists
+- local validation proves `player_search_index` still works
+- local validation proves ranking snapshots are not broken
+- validate these profile cases:
+  - `/api/stats/players/76561197978442431`
+  - `/api/stats/players/76561197978442431?server_id=all`
+  - `/api/stats/players/76561197978442431?server_id=all-servers`
+  - `/api/stats/players/76561197978442431?server_id=comunidad-hispana-02`
+  - `/api/stats/players/76561197978442431?server_id=comunidad-hispana-01`
+- the last case must not return `500`
+- `git diff --name-only` matches the expected scope
+
+## Outcome
+
+Implemented:
+
+- `backend/app/rcon_historical_player_stats.py`
+  - fixed `_fetch_player_stats(...)` runtime aggregation SQL
+  - removed the PostgreSQL-invalid expression `COALESCE(MAX(stats.player_name), stats.player_id)`
+  - kept the player-name fallback in Python with `row["player_name"] or player_id`
+- `scripts/run-stats-validation.ps1`
+  - added a SQL-shape regression check for `_fetch_player_stats(...)`
+  - added a fixture with a `yearly-only-player` on `comunidad-hispana-01`
+  - validated the server-specific fallback path when `player_period_stats` has no weekly/monthly row and runtime also has no rows in the requested window
+
+Root cause of the `500`:
+
+- when `/api/stats/players/{player_id}` could not serve `player_period_stats`, it fell back to `_get_rcon_materialized_player_stats_runtime(...)`
+- `_fetch_player_stats(...)` executed a grouped aggregate query that selected:
+  - `COALESCE(MAX(stats.player_name), stats.player_id) AS player_name`
+- PostgreSQL rejects `stats.player_id` in that expression because it is neither aggregated nor part of `GROUP BY`
+- SQLite/local compatibility masked the issue because SQLite tolerates that aggregate shape
+
+Concrete affected SQL shape:
+
+- previous:
+  - `SELECT COALESCE(MAX(stats.player_name), stats.player_id) AS player_name, ...`
+- new:
+  - `SELECT MAX(stats.player_name) AS player_name, ...`
+- public fallback behavior stays intact because Python already normalizes the result with `str(row["player_name"] or player_id)`
+
+Previous behavior:
+
+- read-model hits returned `read_model=player-period-stats` and `fallback_used=false`
+- if the requested scope/timeframe had no player row and runtime fallback executed on PostgreSQL, the endpoint could raise `psycopg.errors.GroupingError`
+- the request then returned HTTP `500`
+
+New behavior:
+
+- `player_period_stats` remains the preferred source when a row exists
+- runtime fallback remains active when the read model is empty, incomplete or unavailable
+- PostgreSQL fallback SQL is now valid
+- if the requested scope/window has no runtime rows, the endpoint returns the existing controlled zero-value profile contract instead of throwing
+- no frontend, design, asset, Elo/MMR or Comunidad Hispana `#03` changes were made
+
+Validations executed:
+
+- `python -m py_compile backend/app/rcon_historical_player_stats.py`
+- `powershell -ExecutionPolicy Bypass -File scripts/run-stats-validation.ps1`
+- `powershell -ExecutionPolicy Bypass -File scripts/run-integration-tests.ps1`
+
+Validation notes:
+
+- local import-based validation confirmed `_fetch_player_stats(...)` no longer emits the PostgreSQL-invalid aggregate shape
+- local fixture validation confirmed the server-specific missing-read-model fallback does not throw and returns a controlled response
+- `player_period_stats` still serves the profile path when rows exist
+- `player_search_index` and ranking snapshot validations still pass through `scripts/run-stats-validation.ps1`
+- live HTTP validation at `http://127.0.0.1:8000` could not run in this environment because the backend was not up; the script reported that explicitly and route-contract checks still passed via local Python imports
+- `git diff --name-only` included unrelated pre-existing changes in `ai/system-metrics.md` and `frontend/assets/img/weapons/black/*`; those files were not modified by this task
+
+How to validate in production:
+
+- call:
+  - `/api/stats/players/76561197978442431`
+  - `/api/stats/players/76561197978442431?server_id=all`
+  - `/api/stats/players/76561197978442431?server_id=all-servers`
+  - `/api/stats/players/76561197978442431?server_id=comunidad-hispana-02`
+  - `/api/stats/players/76561197978442431?server_id=comunidad-hispana-01`
+- verify the first four still report:
+  - `source.read_model = "player-period-stats"`
+  - `source.fallback_used = false`
+- verify the last case no longer returns HTTP `500`
+- verify the last case returns a controlled stats payload with fallback metadata instead of a PostgreSQL grouping exception
+
+## Change Budget
+
+- Prefer fewer than 5 modified files.
+- Prefer changes under 200 lines when feasible.
+- Split the work into follow-up tasks if limits are exceeded.
--- a/backend/app/rcon_historical_player_stats.py
+++ b/backend/app/rcon_historical_player_stats.py
@@ -803,7 +803,7 @@ def _fetch_player_stats(
    row = connection.execute(
        f"""
        SELECT
-            COALESCE(MAX(stats.player_name), stats.player_id) AS player_name,
+            MAX(stats.player_name) AS player_name,
            COUNT(DISTINCT stats.match_key) AS matches_considered,
            SUM(COALESCE(stats.kills, 0)) AS kills,
            SUM(COALESCE(stats.deaths, 0)) AS deaths,
--- a/scripts/run-stats-validation.ps1
+++ b/scripts/run-stats-validation.ps1
@@ -935,6 +935,7 @@ def build_player_period_stats_fixture():
                ('comunidad-hispana-02', 'comunidad-hispana-02', 's2w1', 'hurtgen', 'Hurtgen Forest', 'warfare', 201, 202, '2026-06-08T20:00:00Z', '2026-06-08T21:00:00Z', 5, 0, 'allied', 'exact', ?),
                ('comunidad-hispana-02', 'comunidad-hispana-02', 's2w2', 'kharkov', 'Kharkov', 'warfare', 203, 204, '2026-06-09T20:00:00Z', '2026-06-09T21:00:00Z', 3, 2, 'allied', 'exact', ?),
                ('comunidad-hispana-02', 'comunidad-hispana-02', 's2w3', 'driel', 'Driel', 'warfare', 205, 206, '2026-06-10T20:00:00Z', '2026-06-10T21:00:00Z', 2, 1, 'allied', 'exact', ?),
+                ('comunidad-hispana-01', 'comunidad-hispana-01', 's1y1', 'utah', 'Utah Beach', 'warfare', 401, 402, '2026-01-15T18:00:00Z', '2026-01-15T19:00:00Z', 5, 1, 'allied', 'exact', ?),
                ('comunidad-hispana-01', 'comunidad-hispana-01', 'old-year', 'omaha', 'Omaha Beach', 'warfare', 301, 302, '2025-12-15T18:00:00Z', '2025-12-15T19:00:00Z', 1, 0, 'allied', 'exact', ?)
            """,
            (
@@ -945,6 +946,7 @@ def build_player_period_stats_fixture():
                MATCH_RESULT_SOURCE,
                MATCH_RESULT_SOURCE,
                MATCH_RESULT_SOURCE,
+                MATCH_RESULT_SOURCE,
            ),
        )
        connection.execute(
@@ -961,6 +963,7 @@ def build_player_period_stats_fixture():
                ('comunidad-hispana-02', 's2w1', 'regression-player', 'Regression Hero', 'allied', 4, 2, 0, 0, '{}', '{}', '{}', '{}', 201, 202),
                ('comunidad-hispana-02', 's2w2', 'regression-player', 'Regression Hero', 'allied', 5, 4, 0, 0, '{}', '{}', '{}', '{}', 203, 204),
                ('comunidad-hispana-02', 's2w3', 'regression-player', 'Regression Hero', 'allied', 3, 2, 0, 0, '{}', '{}', '{}', '{}', 205, 206),
+                ('comunidad-hispana-01', 's1y1', 'yearly-only-player', 'Yearly Only', 'allied', 8, 3, 0, 0, '{}', '{}', '{}', '{}', 401, 402),
                ('comunidad-hispana-01', 'old-year', 'regression-player', 'Historic Regression', 'allied', 99, 1, 0, 0, '{}', '{}', '{}', '{}', 301, 302),
                ('comunidad-hispana-01', 's1w1', 'other-player', 'Other Player', 'axis', 5, 7, 0, 0, '{}', '{}', '{}', '{}', 101, 102),
                ('comunidad-hispana-01', 's1w2', 'other-player', 'Other Player', 'allied', 3, 6, 0, 0, '{}', '{}', '{}', '{}', 103, 104),
@@ -974,6 +977,42 @@ def build_player_period_stats_fixture():
    return db_path


+def validate_fetch_player_stats_sql_contract():
+    captured = {}
+
+    class FakeResult:
+        def fetchone(self):
+            return {
+                "player_name": None,
+                "matches_considered": 0,
+                "kills": 0,
+                "deaths": 0,
+                "teamkills": 0,
+            }
+
+    class FakeConnection:
+        def execute(self, sql, params):
+            captured["sql"] = sql
+            captured["params"] = list(params)
+            return FakeResult()
+
+    result = player_search_stats._fetch_player_stats(
+        connection=FakeConnection(),
+        player_id="regression-player",
+        server_id="comunidad-hispana-01",
+        window={
+            "start": datetime(2026, 6, 8, 0, 0, 0, tzinfo=timezone.utc),
+            "end": datetime(2026, 6, 10, 23, 59, 59, tzinfo=timezone.utc),
+            "kind": "current-week",
+        },
+    )
+
+    sql = captured.get("sql") or ""
+    require("COALESCE(MAX(stats.player_name), stats.player_id)" not in sql, "Runtime player stats SQL must not reference non-aggregated stats.player_id inside COALESCE(MAX(...)).")
+    require("MAX(stats.player_name) AS player_name" in sql, "Runtime player stats SQL should aggregate player_name directly.")
+    require(result.get("player_name") == "regression-player", "Runtime player stats should still fall back to player_id in Python when SQL player_name is null.")
+
+
 def validate_player_period_stats_refresh_and_read_model():
    db_path = build_player_period_stats_fixture()
    anchor = datetime(2026, 6, 10, 22, 0, 0, tzinfo=timezone.utc)
@@ -1115,8 +1154,35 @@ def validate_player_period_stats_fallbacks():
            "Player profile should fall back when one required player period row is missing.",
        )
        require(
-            (missing_row_result.get("source") or {}).get("fallback_reason") == "player-period-stats-player-missing",
-            "Player profile should expose an explicit missing-player-row fallback reason.",
+            (missing_row_result.get("source") or {}).get("fallback_reason") in {"player-period-stats-empty", "player-period-stats-player-missing"},
+            "Player profile should expose a controlled fallback reason when the player period read model is incomplete.",
+        )
+
+        server_specific_missing_read_model_result = get_rcon_materialized_player_stats(
+            player_id="yearly-only-player",
+            server_id="comunidad-hispana-01",
+            timeframe="weekly",
+            db_path=db_path,
+        )
+        require(
+            (server_specific_missing_read_model_result.get("source") or {}).get("fallback_used") is True,
+            "Player profile should fall back when a server-specific weekly row is missing from player_period_stats.",
+        )
+        require(
+            (server_specific_missing_read_model_result.get("source") or {}).get("fallback_reason") in {"player-period-stats-empty", "player-period-stats-player-missing"},
+            "Server-specific missing player-period row should expose a controlled fallback reason.",
+        )
+        require(
+            server_specific_missing_read_model_result.get("matches_considered") == 0,
+            "Server-specific fallback without runtime rows in the requested window should return zero matches instead of throwing.",
+        )
+        require(
+            server_specific_missing_read_model_result.get("kills") == 0,
+            "Server-specific fallback without runtime rows should return zero kills instead of throwing.",
+        )
+        require(
+            server_specific_missing_read_model_result.get("player_name") == "yearly-only-player",
+            "Server-specific fallback without runtime rows should keep the controlled player_name fallback.",
        )

        original_read_model = player_search_stats._get_player_period_stats_read_model
@@ -1202,6 +1268,7 @@ validate_player_search_index_refresh_and_search()
 validate_player_search_index_fallbacks()
 validate_postgres_player_period_stats_schema_path()
 validate_player_period_stats_cli_defaults()
+validate_fetch_player_stats_sql_contract()
 validate_player_period_stats_refresh_and_read_model()
 validate_player_period_stats_fallbacks()