fix(ingest): backfill and derive replay_timestamp from source_replay_id - #16
Merged
Merged
Conversation
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>
…tion 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>
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>
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>
Afgan0r
marked this pull request as ready for review
June 14, 2026 15:29
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
1 participant
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.
Problem
replays.replay_timestampis NULL for ~1101 early replays (sg 187, mace 657, ~257 untyped) — they predate the primary (non-epoch) date source later replays carry. The F11 all-time-scope guard (and r.replay_timestamp is not null, #15) correctly excludes NULL-timestamp replays, so every player silently loses the games/kills/deaths in those replays. Proven on staging: player "Zero" roster = 894 sg replays (831 timed + 63 NULL) → all-timeplayer_stats.games= exactly 831.Fix
The date is recoverable:
source_replay_idis the sg.zone Unix-epoch suffix (sg-zone-1624129684→ 2021-06-19). Recovered two ways — a fallback, never a replacement (NULL-only):0011— one-time, idempotent, forward-only backfill ofreplay_timestamp = to_timestamp(<trailing \d{9,} epoch>)for NULL-only rows.deriveReplayTimestampFromSourceId/resolveReplayTimestamphelper; the promotion service resolves the effective timestamp beforecreateReplay(keep primary, else fall back to the source-id epoch). Placed in the service persolidstats-server-ts-conventions§A (derivation = minimal business logic; repository stays data-access only).The F11 guard is untouched (stays as the safety net). The migration regex and the TS helper agree (
\d{9,}trailing epoch) so backfilled and newly-promoted replays are consistent. 9 digits is the smallest width excluding an incidental short numeric suffix (≥ 1e8 s = 1973).ops:stats:recalculateMUST follow deploy to fold the recovered replays into the all-time buckets.Tests
replay-timestamp.test.ts): epoch parse — valid (sg-zone-…/mace-zone-…/bare/9-digit boundary), non-numeric → null, empty → null, 8-digit → null, digits-not-at-end → null;resolveReplayTimestampkeep/fallback/null.service.test.ts): promotion derives fromsource_replay_idwhen staging timestamp is null; keeps a present staging timestamp.repository/tests/postgres.test.ts): real service + repo + Postgres derivation on promotion; migration backfill UPDATE fills the epoch-suffixed NULL row and leaves a non-numeric id NULL.Validation
pnpm lint/typecheck/formatgreen;pnpm test(unit) 635 passed. Integration tests not run locally — no Docker in this environment; they run in CI (testcontainers). No OpenAPI contract change.🤖 Generated with Claude Code