diff --git a/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-PLAN.md b/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-PLAN.md new file mode 100644 index 0000000..378b9b4 --- /dev/null +++ b/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-PLAN.md @@ -0,0 +1,92 @@ +--- +quick_id: 260614-f14 +slug: f13a-deaths-fold-undercount +status: complete +date: 2026-06-14 +--- + +# Quick Task 260614-f14: Fix the F13a deaths-fold undercount + +## Problem (root cause) + +After F12 + F13a + a full recalc, `sg` `deaths.total` runs ~5% **below** legacy, +while games/kills are within ~1.5%. The decisive evidence is per-player: the +recalc's `deaths.total` is 3-6% lower than the parser's own death-game signal +(replays where the player's `player_counter.deaths_total > 0`, i.e. parser +`player.d > 0`), which itself matches legacy. So the parser is right; the loss is +in server-2's death **aggregation** (`calculatePlayerAndSquadAggregates`, +`src/modules/statistics/service/service.ts`). + +The leak is the interaction between the F13a per-replay cap and the **pre-F13a +`counterDeathReferences` suppression**, which F13a left in place: + +- `counterDeathReferences` (was `service.ts` ~88-95) is a `Set` of + `event.observedPlayerRef` for **every** `player_counter` whose + `counterDeaths(payload) !== undefined`. Note `counterDeaths` returns a DEFINED + `{total:0,…}` even for an explicit-zero counter (`deaths_total:0`); it is + `undefined` only when both `deaths_total` and `deaths_by_teamkills` are absent. +- `applyVictimDeath` / `applySquadEvent` (victim branch) SUPPRESSED the victim + kill-row death when `counterDeathReferences.has(String(victim_entity_id))`. +- `applyCounterEvent` resolves `playersByEntity.get(event.observedPlayerRef)` and + RETURNS EARLY (drops the death) when the player is unresolved. + +**The leak:** when an entity has a `player_counter` in `counterDeathReferences` +(so the victim path is suppressed for that entity) but the counter path records +nothing for the resolved victim — because the counter reports an explicit-zero +`deaths_total` (`incrementDeathsByCounter` adds 0), or because `observedPlayerRef` +does not resolve in `playersByEntity` — the real victim kill-row death is lost +from **both** paths. The two paths also key differently (counter on +`observedPlayerRef`, victim on `String(victim_entity_id)`), so any resolution +mismatch between them leaks a death. + +Reproduced as a failing unit test before the fix: a replay with a +`player_counter` `deaths_total:0` for entity 202 plus a `kill` row whose victim is +202 (resolving to player-b) yields `deaths.total: 0`, where the target invariant +requires `1`. + +## Target invariant + +Per canonical player, all-time `deaths.total` MUST equal the number of distinct +replays in which that player died at least once (one-life: capped at 1/replay) — +equivalently the parser's `d>0` replay count. + +## Approach + +With F13a's per-replay cap (`foldCappedDeaths` caps to <=1 `total` and <=1 +`by_teamkills` per replay) already in place, the `counterDeathReferences` +suppression is **redundant and harmful**: double-counting between the counter path +and the victim path now just collapses to 1 per replay anyway. The minimal fix is +to **stop suppressing the victim path** and let both paths tally into the +per-replay scratch maps; whichever path resolves the entity records the (capped) +death. + +- Remove `counterDeathReferences` (construction, the `ReplayEventContext` field, + and both `.has(...)` suppression checks in `applyVictimDeath` and + `applySquadEvent`). +- Both the counter path and the victim kill-row path now write into + `replayPlayerDeaths` / `replaySquadDeaths`; `foldCappedDeaths` collapses any + double-count to a single death and keeps `by_teamkills <= total`. + +This stays entirely in the **service** layer (the aggregation owner) — no parser, +schema, repository SQL, or API-contract change +(`solidstats-server-ts-conventions` §A: keep each layer doing its own job). F13a's +cap is preserved: a player with `d=3` in one replay still counts 1. + +## Tasks + +1. **Tests first** (`service/tests/aggregates.test.ts`): add the failing leak repro + (zero counter + victim kill row → `total:1`), a teamkill variant asserting + `by_teamkills <= total`, and the N-distinct-replays invariant (died in 3 + replays → `total:3`). Hoist a shared `counterAndKillReplay` fixture builder to + module scope. +2. **Service** (`service/service.ts`): delete `counterDeathReferences` and the two + victim-path suppression checks; document the no-suppression rationale on the + per-replay scratch maps. + +## Acceptance + +- New leak/invariant tests pass; F13a cap tests still pass. +- `pnpm lint`, `pnpm typecheck`, `pnpm test` (unit) green. +- 100% coverage on the changed `service.ts` (unit-only run; full `test:coverage` + needs Docker for the integration files). +- No OpenAPI / schema / parser-contract change. diff --git a/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-SUMMARY.md b/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-SUMMARY.md new file mode 100644 index 0000000..9296e43 --- /dev/null +++ b/.planning/quick/260614-f14-f13a-deaths-fold-undercount/260614-f14-SUMMARY.md @@ -0,0 +1,90 @@ +--- +quick_id: 260614-f14 +slug: f13a-deaths-fold-undercount +status: complete +date: 2026-06-14 +--- + +# Quick Task 260614-f14 — Summary + +## What changed + +Fixed the ~5% deaths undercount introduced by the F13a per-game cap (PR #17). +Server-2 aggregation only — the Rust parser and its `d`/`td` artifact are +untouched. + +- `src/modules/statistics/service/service.ts` — + `calculatePlayerAndSquadAggregates` no longer builds or consults + `counterDeathReferences`. The pre-F13a suppression of the victim kill-row death + path (`applyVictimDeath` and the `applySquadEvent` victim branch) is removed. + Both the counter path (`player_counter` → `deaths_total`/`by_teamkills`) and the + victim kill-row path now tally into the per-replay scratch maps + (`replayPlayerDeaths`, `replaySquadDeaths`); `foldCappedDeaths` still collapses + each replay's contribution to <=1 `total` and <=1 `by_teamkills`, so removing the + suppression cannot inflate counts — it only stops the leak. +- `src/modules/statistics/service/tests/aggregates.test.ts` — added a hoisted + `counterAndKillReplay` fixture builder and three tests: the leak repro (zero + counter + victim kill row → `total:1`), a teamkill variant asserting + `by_teamkills <= total`, and the N-distinct-replays invariant (died in 3 replays + → `total:3`). + +## Root cause (confirmed) + +`counterDeathReferences` held `observedPlayerRef` for **every** `player_counter` +with a DEFINED `counterDeaths(payload)` — including explicit-zero counters +(`deaths_total:0` still yields `{total:0,…}`, which is defined). The victim path +was suppressed whenever `counterDeathReferences.has(String(victim_entity_id))`, +while the counter path (`applyCounterEvent`) dropped the death when its +`observedPlayerRef` did not resolve in `playersByEntity` or contributed 0 for an +explicit-zero counter. So for an entity with a present-but-non-contributing +counter plus a real victim kill row, the death was lost from **both** paths. The +two paths also key differently (counter: `observedPlayerRef`; victim: +`String(victim_entity_id)`), so any resolution mismatch leaked a death. With +F13a's per-replay cap already in place, the suppression was redundant — its only +remaining effect was this leak. + +## The fix + +Stop suppressing the victim path; let whichever path resolves the entity record +the death into the per-replay scratch map. The F13a cap (`foldCappedDeaths`) +guarantees <=1 death and <=1 teamkill death per replay, so double-counting between +the two paths collapses to exactly 1 and `by_teamkills` never exceeds `total`. The +all-time `deaths.total` therefore equals the number of distinct replays the player +died in — the target invariant and the legacy "games-died-in" semantics. + +## Conventions applied + +- **`solidstats-server-ts-conventions` §A** (layer responsibilities): the fix is + business logic in the **service** layer (the aggregation owner) — no leak into + the parser, repository SQL, schema, or API contract. +- **`solidstats-server-ts-tests`** — service logic tested as **unit** (per-layer + map), using the file's in-line `AggregateReplayInput` fixtures, a hoisted shared + builder to satisfy `unicorn/consistent-function-scoping`, and no non-null + assertions. +- **`solidstats-shared-testing-standards`** — strong oracles (exact `toEqual` on + the `deaths` object), one behavior per `it`, TDD (failing repro written first). + +## Validation + +- `pnpm lint` — green (full repo). +- `pnpm typecheck` — green. +- `pnpm format` — green. +- `pnpm test` (unit) — 657 passed (78 files); `aggregates.test.ts` 14/14. +- Coverage: `service.ts` is **100%** (statements/branches/functions/lines) from the + unit suite (scoped V8 run). The full CI gate `pnpm test:coverage` runs the + integration files (`*/tests/postgres.test.ts`, `src/test/integration/**`) which + require Docker/testcontainers; Docker is unavailable in this environment, so the + repo-wide 100% gate (which depends on those integration files for the + repository/route/infra layers) could not be exercised here. The changed file is + fully covered by unit tests. +- No OpenAPI / schema / parser-contract change. + +## Acceptance + +- [x] Failing repro written first (zero counter + victim kill row → `total:1`). +- [x] Victim-path suppression removed; both paths tally into the capped fold. +- [x] One-life cap preserved (d=3 in one replay still counts 1). +- [x] `by_teamkills <= total` (cap consistency) asserted and holds. +- [x] N-distinct-replays invariant: died in 3 replays → `total:3`. +- [x] `pnpm lint`, `pnpm typecheck`, `pnpm test` green; `service.ts` 100% coverage. +- [x] No parser, schema, or API-contract change. diff --git a/src/modules/statistics/service/service.ts b/src/modules/statistics/service/service.ts index 0f3ef74..0ead0db 100644 --- a/src/modules/statistics/service/service.ts +++ b/src/modules/statistics/service/service.ts @@ -85,23 +85,22 @@ export function calculatePlayerAndSquadAggregates( const playersByEntity = new Map( replay.players.map((player) => [player.entityRef, player]), ), - counterDeathReferences = new Set( - replay.events.flatMap((event) => - event.eventType === "player_counter" && - counterDeaths(event.payload) !== undefined - ? [event.observedPlayerRef] - : [], - ), - ), // Per-replay death tallies. Solid Games are one-life: a player can die at // most once per game, so death evidence is summed within the replay here // and then folded into the cross-replay aggregate as a capped (<=1) // contribution. This keeps incap/revive artifacts and duplicate kill rows // from inflating a player's death count beyond games-died-in semantics. + // + // Both the counter path (player_counter -> deaths_total/by_teamkills) and + // the victim kill-row path tally into these maps without cross-path + // suppression: the per-replay cap in `foldCappedDeaths` collapses any + // double-count to a single death, so letting whichever path resolves the + // entity record the death avoids the F13a leak where a present-but- + // non-contributing counter (e.g. an explicit-zero deaths_total) suppressed + // a real victim death that the counter path then failed to credit. replayPlayerDeaths = new Map(), replaySquadDeaths = new Map(), eventContext = { - counterDeathReferences, playersByEntity, replayPlayerDeaths, replaySquadDeaths, @@ -177,7 +176,6 @@ interface MutableSquadAggregate extends MutablePlayerAggregate { } interface ReplayEventContext { - counterDeathReferences: Set; playersByEntity: Map; // Per-replay, uncapped death tallies keyed by playerId / squadId. Folded into // the cross-replay aggregate as a capped (<=1) contribution after the replay's @@ -263,9 +261,6 @@ function applyVictimDeath( if (typeof victimEntityId !== "number") { return; } - if (context.counterDeathReferences.has(String(victimEntityId))) { - return; - } const victim = context.playersByEntity.get(String(victimEntityId)); if (victim === undefined) { return; @@ -304,9 +299,6 @@ function applySquadEvent( if (typeof victimEntityId !== "number") { return; } - if (context.counterDeathReferences.has(String(victimEntityId))) { - return; - } const victim = context.playersByEntity.get(String(victimEntityId)); if (victim?.squadId !== undefined) { incrementDeaths( diff --git a/src/modules/statistics/service/tests/aggregates.test.ts b/src/modules/statistics/service/tests/aggregates.test.ts index 51b82ec..84c9ff6 100644 --- a/src/modules/statistics/service/tests/aggregates.test.ts +++ b/src/modules/statistics/service/tests/aggregates.test.ts @@ -30,6 +30,30 @@ function twoDeathEventsReplay(replayId: string): AggregateReplayInput { }; } +function counterAndKillReplay(replayId: string): AggregateReplayInput { + return { + events: [ + { + eventType: "player_counter", + observedPlayerRef: "202", + payload: { deaths_total: 1 }, + sourceRef: {}, + }, + { + eventType: "kill", + observedPlayerRef: "101", + payload: { victim_entity_id: 202 }, + sourceRef: {}, + }, + ], + players: [ + { entityRef: "101", playerId: "player-a" }, + { entityRef: "202", playerId: "player-b" }, + ], + replayId, + }; +} + describe("calculatePlayerAndSquadAggregates", () => { it("calculates player and squad kills, teamkills, deaths, and replay counts", () => { const result = calculatePlayerAndSquadAggregates([ @@ -574,4 +598,101 @@ describe("calculatePlayerAndSquadAggregates", () => { .deaths, ).toEqual({ by_teamkills: 1, total: 1 }); }); + + it("counts the death from the victim kill row when a present-but-non-contributing counter exists for the victim entity", () => { + // F13a leak repro. Entity 202 has a player_counter that reports an + // explicit-zero deaths_total (counterDeaths returns a DEFINED {total:0}, so + // the counter path adds nothing). A kill row proves 202 died and resolves to + // player-b. Under the buggy victim-path suppression the death was lost from + // BOTH paths and player-b ended with total:0 despite a real death event. + const result = calculatePlayerAndSquadAggregates([ + { + events: [ + { + eventType: "player_counter", + observedPlayerRef: "202", + payload: { deaths_total: 0 }, + sourceRef: {}, + }, + { + eventType: "kill", + observedPlayerRef: "101", + payload: { victim_entity_id: 202 }, + sourceRef: {}, + }, + ], + players: [ + { entityRef: "101", playerId: "player-a" }, + { entityRef: "202", playerId: "player-b", squadId: "squad-b" }, + ], + replayId: "replay-1", + }, + ]); + + expect( + result.playerStats.find((player) => player.playerId === "player-b")?.stats + .deaths, + ).toEqual({ by_teamkills: 0, total: 1 }); + expect( + result.squadStats.find((squad) => squad.squadId === "squad-b")?.stats + .deaths, + ).toEqual({ by_teamkills: 0, total: 1 }); + }); + + it("counts the teamkill death from the victim kill row when a zero counter exists for the victim entity (byTeamkills <= total)", () => { + // Same leak as above, but the kill row is a teamkill: byTeamkills must equal + // total (both 1), never exceed it. The zero counter must not suppress it. + const result = calculatePlayerAndSquadAggregates([ + { + events: [ + { + eventType: "player_counter", + observedPlayerRef: "202", + payload: { deaths_by_teamkills: 0, deaths_total: 0 }, + sourceRef: {}, + }, + { + eventType: "teamkill", + observedPlayerRef: "101", + payload: { victim_entity_id: 202 }, + sourceRef: {}, + }, + ], + players: [ + { entityRef: "101", playerId: "player-a" }, + { entityRef: "202", playerId: "player-b", squadId: "squad-b" }, + ], + replayId: "replay-1", + }, + ]); + + const playerDeaths = result.playerStats.find( + (player) => player.playerId === "player-b", + )?.stats.deaths; + expect(playerDeaths).toEqual({ by_teamkills: 1, total: 1 }); + expect(playerDeaths?.by_teamkills ?? 0).toBeLessThanOrEqual( + playerDeaths?.total ?? 0, + ); + expect( + result.squadStats.find((squad) => squad.squadId === "squad-b")?.stats + .deaths, + ).toEqual({ by_teamkills: 1, total: 1 }); + }); + + it("counts deaths.total equal to the number of distinct replays a player died in (no loss under the counter+victim paths)", () => { + // The invariant: deaths.total === count of replays where the player died at + // least once. Each replay below has a resolving counter AND a victim kill row + // for the same entity; the player must be credited exactly one death per + // replay, three across three replays. + const result = calculatePlayerAndSquadAggregates([ + counterAndKillReplay("replay-1"), + counterAndKillReplay("replay-2"), + counterAndKillReplay("replay-3"), + ]); + + expect( + result.playerStats.find((player) => player.playerId === "player-b")?.stats + .deaths, + ).toEqual({ by_teamkills: 0, total: 3 }); + }); });