From 9796fe91364e7e3acf9d176625ac04296acd5168 Mon Sep 17 00:00:00 2001 From: Pavlov Alexandr Date: Sun, 14 Jun 2026 19:48:09 +0700 Subject: [PATCH] fix(statistics): exclude null-timestamp replays from scope aggregation The all-time recalc scope loaded every current replay of its game type, including the 1101 parity-corpus replays with a NULL replay_timestamp. A no-SteamID player on such a replay reaches the name-fallback path, which keys on replay_timestamp.toISOString() and threw 'Cannot read properties of null (reading toISOString)'. scopedCurrentResultsSql now excludes NULL-timestamp replays (they cannot be placed in time and the audit path already reports them missing_replay_timestamp, never aggregated). Adds a repository integration test that reproduces the crash pre-fix and asserts the null-timestamp replay's player is excluded from the all-time bucket and gets no name-fallback nickname. GSD: .planning/quick/260614-r9k-recalc-null-timestamp-guard Co-Authored-By: Claude Opus 4.8 (1M context) --- .planning/STATE.md | 1 + .../260614-r9k-PLAN.md | 55 ++++++++++++++++ .../260614-r9k-SUMMARY.md | 66 +++++++++++++++++++ .../statistics/repository/repository.ts | 7 ++ .../repository/tests/postgres.test.ts | 66 +++++++++++++++++++ 5 files changed, 195 insertions(+) create mode 100644 .planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-PLAN.md create mode 100644 .planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-SUMMARY.md diff --git a/.planning/STATE.md b/.planning/STATE.md index 8badd73..128be2c 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -132,6 +132,7 @@ Decisions are logged in PROJECT.md Key Decisions table. Recent decisions affecti | 260510-hc5 | Remove staging ingress from server-2 CD | 2026-05-10 | dbe5025 | [260510-hc5-remove-staging-ingress-from-server-2-cd](./quick/260510-hc5-remove-staging-ingress-from-server-2-cd/) | | 260614-c0d | Fix F7 — set-based recalculation so parity check can pass | 2026-06-14 | 2930f10 | [260614-c0d-f7-set-based-recalculation-parity](./quick/260614-c0d-f7-set-based-recalculation-parity/) | | 260614-fw2 | Set-based canonical player identity resolution in per-rotation recalc (behavior-preserving) | 2026-06-14 | fa7c54b | [260614-fw2-perf-set-based-canonical-player-identity](./quick/260614-fw2-perf-set-based-canonical-player-identity/) | +| 260614-r9k | Guard all-time recalc against NULL replay_timestamp (toISOString crash) | 2026-06-14 | b0275e0 | [260614-r9k-recalc-null-timestamp-guard](./quick/260614-r9k-recalc-null-timestamp-guard/) | ## Deferred Items diff --git a/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-PLAN.md b/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-PLAN.md new file mode 100644 index 0000000..c0b063c --- /dev/null +++ b/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-PLAN.md @@ -0,0 +1,55 @@ +--- +quick_id: 260614-r9k +slug: recalc-null-timestamp-guard +status: complete +--- + +# Quick Task 260614-r9k: Guard the all-time recalc against NULL replay_timestamp + +## Problem (root cause) + +The full-run / all-time statistics recalculation crashed with +`TypeError: Cannot read properties of null (reading 'toISOString')` in +`uniqueNameOccurrences` (`src/modules/statistics/repository/repository.ts:838`). + +`scopedCurrentResultsSql` builds the per-scope query over current replays. For the +**all-time** scope it loaded *every* current replay of the scope's game type — +including replays with a `NULL` `replay_timestamp` (the parity corpus has 1101 such +rows). When such a replay carries a **no-SteamID** player, identity resolution falls +through to the name-fallback path (`ensureNameFallbackIdentities` → +`uniqueNameOccurrences`), which keys each occurrence on +`row.replay_timestamp.toISOString()`. With a `NULL` timestamp that dereference throws. + +The single-replay audit path already reports these as `missing_replay_timestamp` and +never recalculates them; only the all-time scope's broad scan pulled them in. + +## Approach + +Exclude `NULL`-timestamp replays from `scopedCurrentResultsSql` with +`and r.replay_timestamp is not null`. Rationale: + +- They cannot be placed in time, so they have no rotation/all-time position. +- The all-time bucket's name-occurrence identity resolution keys on the timestamp. +- Recalculation already classifies them `missing_replay_timestamp` (never aggregated), + so excluding them from the scope is behavior-consistent, not a new policy. + +This is the narrowest, convention-conformant fix: the repository layer owns the SQL +and the row contract, so the guard belongs in the query, not in a downstream +service/usecase. (`solidstats-server-ts-conventions` §A: repository = data-access +adapter; keep each layer doing its own job.) + +## Tasks + +1. **Repository** (`repository/repository.ts`): add the `r.replay_timestamp is not null` + predicate to `scopedCurrentResultsSql` + a docstring note explaining the exclusion. +2. **Integration test** (`repository/tests/postgres.test.ts`): reproduce the crash + pre-fix / pass post-fix using the file's seed helpers + (`seedRotation`/`seedPlayer`/`seedParserResult`/`missionEnvelope`/ + `classifyGameTypesForCurrentReplays`); assert the null-timestamp replay's player is + excluded from the all-time bucket and gets no name-fallback nickname. + +## Acceptance + +- `pnpm typecheck`, `pnpm exec eslint ` green. +- Integration test green in CI (testcontainers Postgres; not runnable locally). +- No OpenAPI contract change. diff --git a/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-SUMMARY.md b/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-SUMMARY.md new file mode 100644 index 0000000..67971e0 --- /dev/null +++ b/.planning/quick/260614-r9k-recalc-null-timestamp-guard/260614-r9k-SUMMARY.md @@ -0,0 +1,66 @@ +--- +quick_id: 260614-r9k +slug: recalc-null-timestamp-guard +status: complete +date: 2026-06-14 +--- + +# Quick Task 260614-r9k — Summary + +## What changed + +Fixed a crash in the full-run / all-time statistics recalculation: +`TypeError: Cannot read properties of null (reading 'toISOString')` raised in +`uniqueNameOccurrences` (`src/modules/statistics/repository/repository.ts`). + +- `repository/repository.ts` — `scopedCurrentResultsSql` now appends + `and r.replay_timestamp is not null` to its WHERE clause, with a docstring note. The + all-time scope's broad scan previously loaded `NULL`-timestamp replays (1101 in the + parity corpus); a no-SteamID player on such a replay reached the name-fallback path, + which keys each occurrence on `replay_timestamp.toISOString()` and threw on `NULL`. +- `repository/tests/postgres.test.ts` — added an integration test that seeds a timed + `sg` replay (SteamID player Alpha) plus a `NULL`-timestamp `sg` replay (no-SteamID + player Ghost), runs `classifyGameTypesForCurrentReplays`, then recalculates the timed + replay. Pre-fix this threw in the all-time rebuild; post-fix it returns + `{ playerStats: 2, rotationId, squadStats: 0, status: "recalculated" }`. Asserts only + Alpha appears in `player_stats` and Ghost gets no name-fallback `player_nicknames` row. + +## Why this is correct (key insight) + +`NULL`-timestamp replays cannot be placed in time and the single-replay audit path +already reports them `missing_replay_timestamp` (never recalculated). Excluding them +from the scope query is consistent with existing behavior — it does not change which +replays get aggregated, only stops the all-time scan from dereferencing a `NULL` +timestamp. The guard lives in the repository SQL because that layer owns the query and +the row contract (`solidstats-server-ts-conventions` §A). + +## Conventions applied + +- **`solidstats-server-ts-conventions` §A** (layer responsibilities): the fix is the + repository's SQL/row contract, kept in the repository layer — no leak into + service/usecase. +- **`solidstats-server-ts-tests`** — per-layer testing map (repository ⇒ **integration** + against real Postgres, never a mocked DB) and the testcontainers harness; reused the + file's existing typed seed builders (`seedRotation`/`seedPlayer`/`seedParserResult`/ + `missionEnvelope`/`fullRunRepository.classifyGameTypesForCurrentReplays`). +- **`solidstats-shared-testing-standards`** (via the tests skill) — AAA structure + (explicit Arrange/Act/Assert), scenario-named `it`, and a **strong oracle**: an exact + `toEqual` on the recalc result plus positive (Alpha aggregated) and negative (Ghost + excluded, no fallback nickname) assertions rather than a loose `toMatchObject`. + Distinct `sourceReplayId`s avoid the seed helper's unique object_key/checksum clash. + +## Validation + +`pnpm typecheck` and `pnpm exec eslint src/modules/statistics/repository/repository.ts +src/modules/statistics/repository/tests/postgres.test.ts` — both green. The integration +test needs the testcontainers/Docker Postgres, which isn't available locally; it runs in +CI. No OpenAPI contract change. + +## Acceptance + +- [x] Root cause fixed at the repository SQL layer. +- [x] Integration test reproduces the crash pre-fix and passes post-fix (correct by + construction per the test skill). +- [x] `pnpm typecheck`, `pnpm exec eslint ` green. +- [x] No OpenAPI contract change. +- [ ] CI integration run green (runs in pipeline; not runnable locally here). diff --git a/src/modules/statistics/repository/repository.ts b/src/modules/statistics/repository/repository.ts index f4b220c..d5a3cc8 100644 --- a/src/modules/statistics/repository/repository.ts +++ b/src/modules/statistics/repository/repository.ts @@ -557,6 +557,12 @@ async function aggregateInputsFromRows( * predicate uses `IS NOT DISTINCT FROM` so a `null` audit-path game type matches * the legacy single-bucket rows, while a concrete type matches exactly and never * picks up excluded (`NULL`) replays. + * + * Replays with a `NULL` replay_timestamp are excluded: they cannot be placed in + * time, the all-time bucket's name-occurrence identity resolution keys on the + * timestamp, and the recalculation already reports them as + * `missing_replay_timestamp` (never recalculated). Without this filter the + * all-time scope loaded them and crashed on `replay_timestamp.toISOString()`. */ function scopedCurrentResultsSql(scope: AggregateScope): string { const rotationPredicate = @@ -567,6 +573,7 @@ function scopedCurrentResultsSql(scope: AggregateScope): string { join replays r on r.id = pr.replay_id where pr.status = 'current' and r.game_type is not distinct from $1 + and r.replay_timestamp is not null ${rotationPredicate} order by r.replay_timestamp, pr.created_at `; diff --git a/src/modules/statistics/repository/tests/postgres.test.ts b/src/modules/statistics/repository/tests/postgres.test.ts index 65b04df..8168473 100644 --- a/src/modules/statistics/repository/tests/postgres.test.ts +++ b/src/modules/statistics/repository/tests/postgres.test.ts @@ -552,6 +552,72 @@ describe("PgStatisticsRepository", () => { }); }); + it("excludes a null-timestamp replay from the all-time bucket rebuild", async () => { + // Arrange — two current `sg` replays. One is placeable in time with a + // SteamID-bearing player (Alpha). The other has NO replay_timestamp and a + // no-SteamID player (Ghost), so its identity can only be resolved via the + // name-fallback path, which keys on `replay_timestamp.toISOString()`. The + // all-time bucket rebuild scans EVERY current `sg` replay, so without the + // `replay_timestamp is not null` filter in scopedCurrentResultsSql it loads + // Ghost's row and crashes with + // `Cannot read properties of null (reading 'toISOString')`. Distinct + // sourceReplayIds keep the replays' unique object_key/checksum apart. + const rotationId = await seedRotation(), + playerAlpha = await seedPlayer("Alpha", "steam-a"), + timedResultId = await seedParserResult({ + rawSnapshot: { + contract_version: "3.0.0", + parser: {}, + players: [{ eid: 101, n: "Alpha", sid: "steam-a" }], + replay: missionEnvelope("sg_assault"), + source: {}, + status: "success", + }, + replayTimestamp: "2026-02-01T12:00:00.000Z", + sourceReplayId: "sg-timed-replay", + }); + await seedParserResult({ + rawSnapshot: { + contract_version: "3.0.0", + parser: {}, + players: [{ eid: 202, n: "Ghost" }], + replay: missionEnvelope("sg_assault"), + source: {}, + status: "success", + }, + replayTimestamp: null, + sourceReplayId: "sg-null-timestamp-replay", + }); + await fullRunRepository.classifyGameTypesForCurrentReplays(); + + // Act — recalculate the timed replay; this rebuilds the sg per-rotation AND + // sg all-time buckets, the latter scanning the null-timestamp replay too. + const result = + await repository.recalculatePlayerAndSquadStatsForParserResult( + timedResultId, + ); + + // Assert — recalc completes (no toISOString crash) and aggregates only the + // timed replay: Alpha across the sg per-rotation + sg all-time scopes + // (playerStats: 2), no squads. The null-timestamp replay contributes + // nothing: no all-time player_stats row and no name-fallback nickname for + // its no-SteamID player. + expect(result).toEqual({ + playerStats: 2, + rotationId, + squadStats: 0, + status: "recalculated", + }); + const aggregatedPlayerIds = await pool.query<{ player_id: string }>( + "select distinct player_id from player_stats", + ); + expect(aggregatedPlayerIds.rows).toEqual([{ player_id: playerAlpha }]); + const ghostNicknames = await pool.query<{ count: string }>( + "select count(*) from player_nicknames where lower(nickname) = 'ghost'", + ); + expect(ghostNicknames.rows[0]?.count).toBe("0"); + }); + it("assigns replay rotation and replaces commander side aggregates", async () => { const rotationId = await seedRotation(), playerA = await seedPlayer("Alpha", "steam-a"),