Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .planning/STATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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 <changed files>` green.
- Integration test green in CI (testcontainers Postgres; not runnable locally).
- No OpenAPI contract change.
Original file line number Diff line number Diff line change
@@ -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 <changed files>` green.
- [x] No OpenAPI contract change.
- [ ] CI integration run green (runs in pipeline; not runnable locally here).
7 changes: 7 additions & 0 deletions src/modules/statistics/repository/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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
`;
Expand Down
66 changes: 66 additions & 0 deletions src/modules/statistics/repository/tests/postgres.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down