Repository navigation
fix(statistics): exclude null-timestamp replays from scope aggregation - #15
Merged
Merged
Conversation
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) <noreply@anthropic.com>
Afgan0r
force-pushed
the
fix/recalc-null-timestamp-guard
branch
from
June 14, 2026 12:57
619d772 to
9796fe9
Compare
Afgan0r
added a commit
that referenced
this pull request
Jun 14, 2026
…id (#16) * fix(ingest): backfill and derive replay_timestamp from source_replay_id Early replays (~1101: sg 187, mace 657, ~257 untyped) carry a NULL replay_timestamp because they predate the primary (non-epoch) date source later replays have. The F11 all-time-scope guard (PR #15) correctly excludes NULL-timestamp replays, so every player silently loses the games/kills/deaths recorded in them. The date is recoverable: source_replay_id is the sg.zone Unix-epoch suffix (e.g. sg-zone-1624129684 -> 2021-06-19). Recover it two ways: - Migration 0011: one-time, idempotent, forward-only backfill of replay_timestamp from to_timestamp(<trailing \d{9,} epoch>) for NULL-only rows. A primary timestamp is never overwritten. - Ingest path: a pure deriveReplayTimestampFromSourceId / resolveReplayTimestamp helper; the promotion service resolves the effective timestamp before createReplay (keep primary, else fall back to the source-id epoch). The F11 guard stays in place as the safety net. A full ops:stats:recalculate must follow deploy to fold the recovered replays into the all-time buckets. Tests: unit for the epoch parse (valid / non-numeric / short / non-trailing) and the service fallback; integration for the promotion path and the migration backfill UPDATE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ingest): bound F12 replay-timestamp epoch and exercise real migration Address PR #16 review findings on the F12 source-id timestamp backfill. - CORRECTNESS: the migration SQL and the TS helper diverged on long trailing digit runs (TS guarded with Number.isSafeInteger; SQL had no bound, so 14-18 digits gave far-future dates and >=19 digits overflowed int8 and aborted). Bound the accepted epoch to a plausible Unix-seconds range [1e9 (2001-09) .. 2e9 (2033-05)] on BOTH paths: the TS helper rejects out-of-range values, and 0011 matches a trailing run of exactly 10 digits ((\D|^)\d{10}$), casts only the captured group, and re-checks the value BETWEEN 1000000000 AND 2000000000 so an over-long run is skipped before the ::bigint cast instead of overflowing it. Backfilled and newly-promoted rows now agree exactly. - TESTS: the integration test inlined a hand-copied UPDATE; rewrite it to read and execute the real 0011 migration file against seeded NULL rows (idempotent, NULL-only), and assert in-range, non-numeric, 13-digit, and 19-digit-overflow rows so drift in the .sql file is caught. Add boundary assertions to the TS unit test on both the accept and reject sides. - DOCS: the 0011 down-comment referenced a promotion_evidence marker that is never written; replace it with a reversal that actually identifies backfilled rows by re-deriving the epoch from source_replay_id. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ingest): align F12 epoch regex with migration 0011 SQL semantics The TS helper used a greedy `\d{9,}$`, so a zero-padded all-numeric id longer than 10 digits (e.g. `00000000001500000000`) was captured and Number()-stripped to an in-range epoch (1500000000) and ACCEPTED, while migration 0011's `(\D|^)\d{10}$` requires exactly 10 trailing digits preceded by a non-digit or string start and leaves the same row NULL. This broke the "TS and SQL accept/reject the same inputs" property. Change the trailing-epoch pattern to `/(?:^|\D)(\d{10})$/`, capturing exactly 10 trailing digits, then apply the unchanged [1e9..2e9] bound. Both paths now accept `sg-zone-1624129684` and a bare `1624129684`, and reject the zero-padded over-long run, 9-digit, 11+-digit, and out-of-bound runs identically. Verified TS output matches an emulated SQL regex+bound across an adversarial corpus. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ingest): drop unreachable NaN guard in replay-timestamp derivation The range check bounds epochSeconds to [1e9, 2e9], so epochSeconds * 1000 lands in [1e12, 2e12] ms -- always far inside JS Date's valid range (+/-8.64e15 ms). new Date() can therefore never yield an Invalid Date, so `Number.isNaN(date.getTime())` is provably always false and its return-null branch is dead code. That dead branch was the single uncovered branch (line 51) failing the repo's 100% coverage gate in CI's Verify. Remove the guard rather than suppress it (per conventions: delete provably-dead code instead of adding a v8-ignore or an unreachable-case test) and update the JSDoc to drop the now-impossible "or when it does not yield a valid date" clause. The exact-10-digit regex, the [1e9, 2e9] bound, and the migration-0011 SQL agreement are untouched. replay-timestamp.ts now reports 100% statements/branches/functions/lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All-time bucket rebuild crashed on NULL replay_timestamp (toISOString on null) for the 1101 timestamp-less replays in the parity corpus. Filter them in scopedCurrentResultsSql (full-run + audit paths). Real-pg regression test added. Found while running the F8 parity recalc.