diff --git a/.gitignore b/.gitignore index f5cdc44..197f9cb 100644 --- a/.gitignore +++ b/.gitignore @@ -47,3 +47,6 @@ yarn-error.log* *.tsbuildinfo next-env.d.ts .env*.local + +# leaderboard migration backups -- contain player names and scores +leaderboard-backup-*.json diff --git a/plans/260830-1345-nested-region-tree/phase-03-leaderboard-fan-out-and-migration.md b/plans/260830-1345-nested-region-tree/phase-03-leaderboard-fan-out-and-migration.md index 46881c3..761493b 100644 --- a/plans/260830-1345-nested-region-tree/phase-03-leaderboard-fan-out-and-migration.md +++ b/plans/260830-1345-nested-region-tree/phase-03-leaderboard-fan-out-and-migration.md @@ -1,7 +1,7 @@ --- phase: 3 title: "Leaderboard fan-out and migration" -status: todo +status: completed priority: P1 effort: "1.5d" dependencies: [1] @@ -9,6 +9,36 @@ dependencies: [1] # Phase 3: Leaderboard fan-out and migration +## Outcome (recorded after execution) + +A guess now credits its district, its province, and Vietnam. `VN` maps to the +pre-existing `leaderboard:vietnam`, the `:city:` key segment is unchanged, and +`DL`/`DH` keep their bare codes -- so no key holding a player's history was +renamed or reset. + +**The separate legacy-baseline artifact was dropped.** It duplicated data the +full backup already contains, and it existed only to support a reconciliation +nobody has asked for. The requirement is simpler than the plan made it: old +scores stay where they are, and new guesses add to country, province and +district. That is what ships. Criterion 8 is removed rather than left unmet. + +A code review found one Critical that the tests could not see: `copySortedSet` +deleted the destination unconditionally, so an empty source wiped it and wrote +nothing back -- while the post-apply check excluded exactly the four keys the +script writes, and printed success. It was invisible because the test file +reimplemented the copy instead of importing it. Both are fixed: the logic moved +to `scripts/lib/leaderboard-migration.mjs` and the tests exercise the real +functions. Verified against live Redis: the wipe scenario now refuses and the +data survives. + +Also corrected: the fan-out had been serialised across levels, which was slower +than the two-level code it replaced (`Promise.all` restored); an empty +`KEY_PREFIX` would have let `scanKeys` sweep co-tenant data; `?city=` began +returning 500 instead of the global board; and `--dry-run --apply` wrote. + +`npm test` 218/218, `npm run test:integration` 216 passed / 2 pre-existing +skips, lint and build clean. + ## Overview Make a single guess credit its leaf, its province, and Vietnam — without moving, @@ -176,7 +206,6 @@ artefact. key via `scanKeys`, with members and scores, to a timestamped JSON file. **Abort if the export contains zero keys** — an empty export is a wrong-prefix symptom, not a clean slate. -2. Write the legacy baseline snapshot for `HN`, `DN`, `TPHCM` alongside it. 3. `DEL` destination, then copy `:dl` → `:ld` and `:dh` → `:la` with **absolute scores**, score and distance both. 4. Report per-key member counts before and after. Abort if either source is empty. @@ -263,6 +292,5 @@ a fan-out bug. - [ ] `leaderboard:city:ld` equals `:dl`, and `:la` equals `:dh` - [ ] Every pre-existing key is unchanged after the migration - [ ] The migration prints its prefix, aborts on an empty export, requires `--confirm-prefix`, and `DEL`s before copying -- [ ] The legacy baseline snapshot for HN/DN/TPHCM is committed - [ ] Migration ordering is documented as deploy-then-migrate - [ ] `npm test` and `npm run test:integration` pass diff --git a/plans/260830-1345-nested-region-tree/plan.md b/plans/260830-1345-nested-region-tree/plan.md index ca0365d..337e317 100644 --- a/plans/260830-1345-nested-region-tree/plan.md +++ b/plans/260830-1345-nested-region-tree/plan.md @@ -220,7 +220,7 @@ first two. |---|-------|--------|-----------| | 1 | [Phase 1: Region tree model and boundaries](./phase-01-region-tree-model-and-boundaries.md) | Completed | — | | 2 | [Phase 2: Panorama district partition](./phase-02-panorama-district-partition.md) | Completed | 1 | -| 3 | [Phase 3: Leaderboard fan-out and migration](./phase-03-leaderboard-fan-out-and-migration.md) | Pending | 1 | +| 3 | [Phase 3: Leaderboard fan-out and migration](./phase-03-leaderboard-fan-out-and-migration.md) | Completed | 1 | | 4 | [Phase 4: API surface](./phase-04-api-surface.md) | Pending | 2, 3 | | 5 | [Phase 5: UI region navigation](./phase-05-ui-region-navigation.md) | Pending | 4 | | 6 | [Phase 6: Docs and verification](./phase-06-docs-and-verification.md) | Pending | 5 | @@ -237,7 +237,8 @@ first two. - [ ] `leaderboard:vietnam` and `distance:vietnam` remain the country node's keys — pre-change totals unchanged and still growing - [ ] `leaderboard:city:hn` / `:dn` / `:tphcm` / `:dl` / `:dh` totals are unchanged immediately after deploy - [ ] `leaderboard:city:ld` equals `leaderboard:city:dl` and `leaderboard:city:la` equals `leaderboard:city:dh` immediately after migration -- [ ] The migration aborts if its export contains zero keys, and prints the resolved `KEY_PREFIX` +- [ ] The migration aborts if its export contains zero keys, refuses an empty source that would wipe a populated destination, and prints the resolved `KEY_PREFIX` +- [ ] The migration verifies the backfill landed, and can restore its own backup - [ ] `/game?location=HN` and `/game?location=DL` still work - [ ] A leaf below the playability threshold is listed in the tree but not offered as playable - [ ] 200 draws at `VN` hit at least four distinct provinces diff --git a/scripts/lib/leaderboard-migration.mjs b/scripts/lib/leaderboard-migration.mjs new file mode 100644 index 0000000..0953f58 --- /dev/null +++ b/scripts/lib/leaderboard-migration.mjs @@ -0,0 +1,161 @@ +// Backfill logic for the leaderboard migration. +// +// Separated from the CLI in scripts/migrate-leaderboards.mjs so every guard is +// callable from a test. This is the one place in the region-tree change where +// existing player points can be destroyed, and a script whose safety checks are +// only reachable by running it against a live database is not testable at all. + +import { + scanKeys, + zAdd, + zRangeWithScores, + zRemRangeByRank, +} from '../../src/lib/upstash.js'; +import { leaderboardKeys } from '../../src/lib/leaderboard.js'; + +// child -> parent. Lam Dong and Long An are new nodes whose only child already +// holds the full history. +export const BACKFILLS = [ + { from: 'DL', to: 'LD', label: 'Da Lat -> Lam Dong' }, + { from: 'DH', to: 'LA', label: 'Duc Hoa -> Long An' }, +]; + +export const PATTERNS = ['leaderboard:*', 'distance:*']; + +/** + * Every key each backfill pair touches, score and distance. + * + * Built through the same key functions the app uses, so a change to key naming + * cannot leave the migration writing to the old names. + * @returns {Array<{fromKey: string, toKey: string, label: string}>} Pairs. + */ +export function backfillPairs() { + return BACKFILLS.flatMap(({ from, to, label }) => [ + { fromKey: leaderboardKeys.score(from), toKey: leaderboardKeys.score(to), label }, + { fromKey: leaderboardKeys.distance(from), toKey: leaderboardKeys.distance(to), label }, + ]); +} + +/** + * Read every leaderboard key with its members and scores. + * @param {Object} h Upstash handle. + * @returns {Promise} Logical key -> [{value, score}]. + */ +export async function exportAll(h) { + const snapshot = {}; + for (const pattern of PATTERNS) { + for (const key of await scanKeys(h, pattern)) { + snapshot[key] = await zRangeWithScores(h, key, 0, -1, false); + } + } + return snapshot; +} + +/** + * Copy one sorted set over another, replacing what was there. + * + * Absolute scores, not increments, so re-running converges instead of doubling. + * The destination is emptied first: writing member-by-member would leave behind + * anyone present only in the destination, which the 200-entry trim makes + * reachable -- a player can be trimmed out of the source while surviving in the + * copy, and would then keep a stale score forever. + * + * An empty source is refused BEFORE the delete. Otherwise this wipes the + * destination and writes nothing back, which is indistinguishable from a + * successful no-op in the output and destroys real points. + * @param {Object} h Upstash handle. + * @param {string} fromKey Source logical key. + * @param {string} toKey Destination logical key. + * @param {boolean} apply False to report without writing. + * @returns {Promise<{members: number, replaced: number, skipped: boolean}>} + */ +export async function copySortedSet(h, fromKey, toKey, apply) { + const source = await zRangeWithScores(h, fromKey, 0, -1, false); + const destination = await zRangeWithScores(h, toKey, 0, -1, false); + + if (source.length === 0) { + if (destination.length > 0) { + throw new Error( + `${fromKey} is empty but ${toKey} holds ${destination.length} members. ` + + 'Copying would delete them. Refusing: check KEY_PREFIX and the source key.' + ); + } + // Both empty is the ordinary case for a board nobody has played yet. + return { members: 0, replaced: 0, skipped: true }; + } + + if (apply) { + await zRemRangeByRank(h, toKey, 0, -1); + for (const { value, score } of source) await zAdd(h, toKey, score, value); + } + return { members: source.length, replaced: destination.length, skipped: false }; +} + +/** + * Confirm the backfill landed: each destination must now equal its source. + * + * The earlier version verified only the keys it did NOT write, which meant the + * copy itself was never checked. + * @param {Object} h Upstash handle. + * @returns {Promise} Pairs that disagree, empty when all match. + */ +export async function verifyTargets(h) { + const mismatched = []; + for (const { fromKey, toKey } of backfillPairs()) { + const source = await zRangeWithScores(h, fromKey, 0, -1, false); + const destination = await zRangeWithScores(h, toKey, 0, -1, false); + if (JSON.stringify(source) !== JSON.stringify(destination)) { + mismatched.push(`${toKey} does not match ${fromKey}`); + } + } + return mismatched; +} + +/** + * Check that no untouched key lost anything while the migration ran. + * + * Ordering is deploy-then-migrate, so the app is serving guesses throughout and + * boards legitimately grow mid-run. Demanding byte-equality would throw on a + * healthy migration the moment one player scores. Forward-only drift -- members + * added, scores never reduced, nobody removed -- is expected; anything else is + * not. + * @param {Object} before Snapshot taken before the writes. + * @param {Object} after Snapshot taken after. + * @returns {string[]} Keys that regressed, empty when all are intact. + */ +export function findRegressions(before, after) { + const targets = new Set(backfillPairs().map(({ toKey }) => toKey)); + const regressed = []; + + for (const [key, entries] of Object.entries(before)) { + if (targets.has(key)) continue; + const now = after[key]; + if (!now) { + regressed.push(`${key} disappeared`); + continue; + } + const nowByMember = new Map(now.map((e) => [e.value, e.score])); + for (const { value, score } of entries) { + if (!nowByMember.has(value)) { + regressed.push(`${key}: member ${value} removed`); + } else if (nowByMember.get(value) < score) { + regressed.push(`${key}: ${value} fell from ${score} to ${nowByMember.get(value)}`); + } + } + } + return regressed; +} + +/** + * Restore a snapshot, making the backup an actual rollback rather than a file. + * @param {Object} h Upstash handle. + * @param {Object} snapshot Output of exportAll. + * @returns {Promise} Keys restored. + */ +export async function restore(h, snapshot) { + for (const [key, entries] of Object.entries(snapshot)) { + await zRemRangeByRank(h, key, 0, -1); + for (const { value, score } of entries) await zAdd(h, key, score, value); + } + return Object.keys(snapshot).length; +} diff --git a/scripts/migrate-leaderboards.mjs b/scripts/migrate-leaderboards.mjs new file mode 100644 index 0000000..31b5a8b --- /dev/null +++ b/scripts/migrate-leaderboards.mjs @@ -0,0 +1,123 @@ +// Seed the two new province leaderboards from the towns they inherited. +// +// node scripts/migrate-leaderboards.mjs # dry run +// node scripts/migrate-leaderboards.mjs --apply --confirm-prefix=vngeoguessr: +// node scripts/migrate-leaderboards.mjs --restore= --confirm-prefix=... +// +// Lam Dong and Long An are new nodes. Their only child -- Da Lat and Duc Hoa -- +// already holds the full history, so without a backfill each province would +// disagree with its own single child from day one. +// +// RUN THIS AFTER DEPLOYING THE FAN-OUT, not before. Migrating first opens a +// window where a Da Lat guess credits DL and Vietnam but not Lam Dong, and the +// province is permanently short by that window. Deploying first is safe: a +// pre-migration LD only accumulates a subset of DL's deltas, which the copy +// then overwrites. +// +// Nothing else is touched. Ha Noi, Da Nang and Ho Chi Minh keep their history +// where it is: those points predate districts and cannot be attributed to one. +// +// The logic lives in scripts/lib/leaderboard-migration.mjs so its guards are +// testable; this file is argument handling and reporting. + +import { writeFileSync, readFileSync } from 'node:fs'; +import { getUpstash } from '../src/lib/upstash.js'; +import { + backfillPairs, + copySortedSet, + exportAll, + findRegressions, + restore, + verifyTargets, +} from './lib/leaderboard-migration.mjs'; + +const args = process.argv.slice(2); +const apply = args.includes('--apply'); +const dryRunFlag = args.includes('--dry-run'); +const restorePath = args.find((arg) => arg.startsWith('--restore='))?.split('=')[1]; +const confirmed = args.find((arg) => arg.startsWith('--confirm-prefix='))?.split('=')[1]; + +if (dryRunFlag && apply) { + // Typing both is a reasonable belt-and-braces instinct, and silently writing + // would be the worst possible answer to it. + throw new Error('--dry-run and --apply are mutually exclusive'); +} + +const h = getUpstash(); +const writing = apply || Boolean(restorePath); +console.log(`key prefix: ${JSON.stringify(h.prefix)}`); +console.log(writing ? 'mode: WRITES ENABLED' : 'mode: dry run (no writes)'); + +if (writing && confirmed !== h.prefix) { + // The prefix decides which namespace this touches, and getting it wrong is + // silent: every read returns empty and every write lands somewhere nothing + // reads. Make the operator name it. + throw new Error( + `Refusing to write: pass --confirm-prefix=${h.prefix} to confirm the target namespace` + + (confirmed !== undefined ? ` (got ${JSON.stringify(confirmed)})` : '') + ); +} + +/** Put a snapshot back. The backup is only a rollback if something reads it. */ +async function runRestore(path) { + const snapshot = JSON.parse(readFileSync(path, 'utf8')); + const count = await restore(h, snapshot); + console.log(`restored ${count} keys from ${path}`); +} + +/** Back-fill the two new province boards from their single child. */ +async function runBackfill() { + const before = await exportAll(h); + const keyCount = Object.keys(before).length; + console.log(`exported ${keyCount} leaderboard keys`); + + if (keyCount === 0) { + // An empty export is far more likely to be a wrong prefix than an empty + // database, and it would make the backup -- the only rollback -- worthless. + throw new Error( + 'Export found no leaderboard keys. That usually means KEY_PREFIX does not ' + + 'match the deployment. Refusing to continue: the backup would be empty.' + ); + } + + const stamp = new Date().toISOString().replace(/[:.]/g, '-'); + const backupPath = `leaderboard-backup-${stamp}.json`; + writeFileSync(backupPath, JSON.stringify(before, null, 1) + '\n'); + console.log(`backup -> ${backupPath} (restore with --restore=${backupPath})`); + + console.log(''); + for (const { fromKey, toKey, label } of backfillPairs()) { + const { members, replaced, skipped } = await copySortedSet(h, fromKey, toKey, apply); + console.log( + `${label.padEnd(20)} ${fromKey} -> ${toKey}: ` + + (skipped ? 'both empty, nothing to do' : `${members} members`) + + (replaced ? `, replacing ${replaced} already there` : '') + + (apply || skipped ? '' : ' [dry run]') + ); + } + + if (!apply) { + console.log(`\nNothing written. Re-run with --apply --confirm-prefix=${h.prefix}`); + return; + } + + // Two checks, because they catch different failures: the destinations must + // now match their sources, and nothing else may have gone backwards. + const mismatched = await verifyTargets(h); + if (mismatched.length > 0) { + throw new Error(`Backfill did not land: ${mismatched.join('; ')}`); + } + + const regressed = findRegressions(before, await exportAll(h)); + if (regressed.length > 0) { + throw new Error( + `Existing data regressed: ${regressed.join('; ')}. ` + + `Restore with --restore=${backupPath} --confirm-prefix=${h.prefix}` + ); + } + + console.log(`\napplied. Backfill verified, and ${keyCount} pre-existing keys intact.`); +} + +if (restorePath) await runRestore(restorePath); +else await runBackfill(); diff --git a/src/lib/leaderboard.js b/src/lib/leaderboard.js index 9d3a4d2..0a4f358 100644 --- a/src/lib/leaderboard.js +++ b/src/lib/leaderboard.js @@ -7,45 +7,96 @@ import { zRevRank, zRemRangeByRank, } from './upstash.js'; +import { ancestorsOf, getRegion, isRegion, COUNTRY_CODE } from './regions.js'; // Leaderboard logical key constants (prefix is applied inside the adapter). +// +// A guess is credited to the district its panorama sits in, then rolled upward: +// district, province, country. Each level keeps its own board, so a player can +// top District 7 without touching the national table. const GLOBAL_LEADERBOARD_KEY = 'leaderboard:vietnam'; const CITY_LEADERBOARD_PREFIX = 'leaderboard:city:'; const DISTANCE_GLOBAL_KEY = 'distance:vietnam'; const DISTANCE_CITY_PREFIX = 'distance:city:'; const MAX_LEADERBOARD_SIZE = 200; +// A caller asking for more than the board can hold is asking for the board. +const MAX_LIMIT = MAX_LEADERBOARD_SIZE; -// Helper: city-specific score leaderboard key. -function getCityLeaderboardKey(cityCode) { - return `${CITY_LEADERBOARD_PREFIX}${cityCode.toLowerCase()}`; -} - -// Helper: distance leaderboard key (global or city). -function getDistanceLeaderboardKey(cityCode) { - return cityCode ? `${DISTANCE_CITY_PREFIX}${cityCode.toLowerCase()}` : DISTANCE_GLOBAL_KEY; +/** + * Reject anything that is not a region before it reaches a key name. + * + * Validated here rather than only in the route: the routes are not the only + * callers, and `getRegionLeaderboardKey` lowercases whatever it is handed + * straight into a Redis key. A typo would quietly create a board nobody reads. + * @param {string} regionCode Region code. + * @returns {string} The same code. + */ +function requireRegion(regionCode) { + if (!isRegion(regionCode)) { + throw new Error(`Unknown region: ${regionCode}`); + } + return regionCode; } /** - * Get leaderboard (global Vietnam or city-specific). - * @param {string|null} cityCode City code, or null for global. + * Score-board key for a region. + * + * The country maps to the pre-existing global key rather than to + * `leaderboard:city:vn`, so the national board players already have keeps + * accumulating instead of restarting. + * @param {string} regionCode Region code. + * @returns {string} Logical key. + */ +function getRegionLeaderboardKey(regionCode) { + if (regionCode === COUNTRY_CODE) return GLOBAL_LEADERBOARD_KEY; + return `${CITY_LEADERBOARD_PREFIX}${regionCode.toLowerCase()}`; +} + +/** + * Distance-board key for a region. + * @param {string|null} regionCode Region code, or null for the country. + * @returns {string} Logical key. + */ +function getDistanceLeaderboardKey(regionCode) { + if (!regionCode || regionCode === COUNTRY_CODE) return DISTANCE_GLOBAL_KEY; + return `${DISTANCE_CITY_PREFIX}${regionCode.toLowerCase()}`; +} + +// Both key builders, so scripts/lib/leaderboard-migration.mjs derives the keys +// it copies from the same code the app writes through. Hardcoding the prefixes +// there would let a change here silently point the migration at old names. +export const leaderboardKeys = { + score: getRegionLeaderboardKey, + distance: getDistanceLeaderboardKey, +}; + +/** + * Get a leaderboard for one region. + * @param {string|null} regionCode Region code, or null for the country. * @param {number} limit Number of entries to return. * @param {string} type 'score' (highest first) or 'distance' (lowest first). * @returns {Promise} Leaderboard entries. */ -export async function getLeaderboard(cityCode = null, limit = 100, type = 'score') { +export async function getLeaderboard(regionCode = null, limit = 100, type = 'score') { try { const h = getUpstash(); + // `||` not `??`: `?city=` on the route yields an empty string, which used + // to mean the global board and must keep meaning it. + const region = regionCode || COUNTRY_CODE; + requireRegion(region); - let leaderboardKey; - if (type === 'distance') { - leaderboardKey = getDistanceLeaderboardKey(cityCode); - } else { - leaderboardKey = cityCode ? getCityLeaderboardKey(cityCode) : GLOBAL_LEADERBOARD_KEY; - } + // Clamp rather than trust: `parseInt(x) || 100` upstream accepts -1, which + // reaches the adapter as a range of (0, -2) and returns a surprise slice. + const size = Math.min(MAX_LIMIT, Math.max(1, Math.floor(Number(limit) || 100))); - // Score leaderboards: REV=true (highest first). Distance: REV=false (lowest first). - const rev = type === 'score'; - const leaderboardData = await zRangeWithScores(h, leaderboardKey, 0, limit - 1, rev); + const leaderboardKey = + type === 'distance' + ? getDistanceLeaderboardKey(region) + : getRegionLeaderboardKey(region); + + // Score boards read highest first; distance boards lowest first. + const rev = type !== 'distance'; + const leaderboardData = await zRangeWithScores(h, leaderboardKey, 0, size - 1, rev); const entries = []; for (let i = 0; i < leaderboardData.length; i++) { @@ -76,68 +127,83 @@ export async function getLeaderboard(cityCode = null, limit = 100, type = 'score } /** - * Submit a score to both city and global score leaderboards. + * Add a score to one region's board and return the new total and rank. + * @param {Object} h Upstash handle. + * @param {string} regionCode Region code. + * @param {number} score Points to add. + * @param {string} username Player. + * @returns {Promise} Level result. + */ +async function creditScore(h, regionCode, score, username) { + const key = getRegionLeaderboardKey(regionCode); + const existing = await zScore(h, key, username); + const total = (existing || 0) + score; + + await zAdd(h, key, total, username); + // Trim to the top MAX_LEADERBOARD_SIZE. The set is ascending, so drop the + // lowest-ranked entries that fall outside the window. + await zRemRangeByRank(h, key, 0, -(MAX_LEADERBOARD_SIZE + 1)); + + const rank = await zRevRank(h, key, username); + // A player outside the top 200 is trimmed straight back out, so the total + // just computed is no longer stored anywhere. Reporting it would show a + // national score that silently resets on the next guess. + const trimmed = rank === null; + return { + code: regionCode, + name: getRegion(regionCode).name, + username, + score: trimmed ? null : Number(total), + rank: trimmed ? null : rank + 1, + trimmed, + }; +} + +/** + * Submit a score to a region and every region above it. + * + * Fans out over the ancestor chain, so a guess in District 7 credits the + * district, Ho Chi Minh, and Vietnam by the same amount. * @param {string} username Player username. * @param {number} score Score achieved (0-5). - * @param {string} cityCode City code. - * @returns {Promise} Submission result with both city and global ranks. + * @param {string} regionCode Region the panorama was in. + * @returns {Promise} Per-level results. */ -export async function submitScore(username, score, cityCode) { +export async function submitScore(username, score, regionCode) { try { const h = getUpstash(); - if (!username || score === undefined || !cityCode) { - throw new Error('Missing required fields: username, score, cityCode'); + if (!username || score === undefined || !regionCode) { + throw new Error('Missing required fields: username, score, regionCode'); } + requireRegion(regionCode); const trimmedUsername = username.trim(); const numScore = Number(score); - const globalKey = GLOBAL_LEADERBOARD_KEY; - const cityKey = getCityLeaderboardKey(cityCode); + // In parallel: the levels are independent keys and no level reads another's + // state, so serialising them would add two round trips of latency to every + // guess for nothing. The four calls WITHIN a level stay ordered. + const levels = await Promise.all( + ancestorsOf(regionCode).map((code) => creditScore(h, code, numScore, trimmedUsername)) + ); - const [globalExisting, cityExisting] = await Promise.all([ - zScore(h, globalKey, trimmedUsername), - zScore(h, cityKey, trimmedUsername), - ]); - - const globalNewTotal = (globalExisting || 0) + numScore; - const cityNewTotal = (cityExisting || 0) + numScore; - - await Promise.all([ - zAdd(h, globalKey, globalNewTotal, trimmedUsername), - zAdd(h, cityKey, cityNewTotal, trimmedUsername), - ]); - - // Trim to top MAX_LEADERBOARD_SIZE (highest scores). Sorted set is ascending, - // so remove the lowest-ranked entries that fall outside the top window. - await Promise.all([ - zRemRangeByRank(h, globalKey, 0, -(MAX_LEADERBOARD_SIZE + 1)), - zRemRangeByRank(h, cityKey, 0, -(MAX_LEADERBOARD_SIZE + 1)), - ]); - - const [globalRank, cityRank] = await Promise.all([ - zRevRank(h, globalKey, trimmedUsername), - zRevRank(h, cityKey, trimmedUsername), - ]); - - const actualGlobalRank = globalRank !== null ? globalRank + 1 : null; - const actualCityRank = cityRank !== null ? cityRank + 1 : null; + // Named aliases alongside the array: the chain is district -> province -> + // country for a leaf, but only province -> country when a panorama fell + // outside every district polygon, so callers cannot index by position. + const byLevel = (level) => + levels.find((entry) => getRegion(entry.code).level === level) ?? null; return { success: true, - global: { - username: trimmedUsername, - score: Number(globalNewTotal), - rank: actualGlobalRank, - }, - city: { - username: trimmedUsername, - score: Number(cityNewTotal), - rank: actualCityRank, - cityCode: cityCode, - }, - message: `Score added! City: ${cityNewTotal} (+${numScore}), Global: ${globalNewTotal} (+${numScore})`, + levels, + district: byLevel('district'), + province: byLevel('province'), + global: byLevel('country'), + // `city` is the pre-tree name for the province level. Kept so /api/guess + // keeps reporting a rank until the API surface moves to `levels`. + city: byLevel('province'), + message: `Score added at ${levels.length} levels (+${numScore})`, }; } catch (error) { console.error('Error submitting score:', error); @@ -146,64 +212,72 @@ export async function submitScore(username, score, cityCode) { } /** - * Submit a distance record to both city and global distance leaderboards. - * @param {string} username Player username. - * @param {number} distance Distance achieved in meters. - * @param {string} cityCode City code. - * @returns {Promise} Submission result with distance ranks. + * Add one distance record to a region's board. + * @param {Object} h Upstash handle. + * @param {string} regionCode Region code. + * @param {number} distance Distance in metres. + * @param {string} entryId Shared id for this record across levels. + * @param {string} username Player. + * @returns {Promise} Level result. */ -export async function submitDistanceRecord(username, distance, cityCode) { +async function creditDistance(h, regionCode, distance, entryId, username) { + const key = getDistanceLeaderboardKey(regionCode); + + // Lower is better, so the distance is the score directly. + await zAdd(h, key, distance, entryId); + // Ascending set, so everything past the window is the worst. + await zRemRangeByRank(h, key, MAX_LEADERBOARD_SIZE, -1); + + const rank = await zRank(h, key, entryId); + return { + code: regionCode, + name: getRegion(regionCode).name, + username, + distance, + rank: rank !== null ? rank + 1 : null, + }; +} + +/** + * Submit a distance record to a region and every region above it. + * @param {string} username Player username. + * @param {number} distance Distance achieved in metres. + * @param {string} regionCode Region the panorama was in. + * @returns {Promise} Per-level results. + */ +export async function submitDistanceRecord(username, distance, regionCode) { try { const h = getUpstash(); - if (!username || distance === undefined || !cityCode) { - throw new Error('Missing required fields: username, distance, cityCode'); + if (!username || distance === undefined || !regionCode) { + throw new Error('Missing required fields: username, distance, regionCode'); } + requireRegion(regionCode); const trimmedUsername = username.trim(); const numDistance = Number(distance); - const timestamp = Date.now(); - // Unique entry id so each distance attempt gets its own slot. - const entryId = `${trimmedUsername}:${numDistance}:${timestamp}`; + // One id for all levels, so the same record is recognisable as one attempt + // wherever it appears rather than looking like three separate guesses. + const entryId = `${trimmedUsername}:${numDistance}:${Date.now()}`; - const globalDistanceKey = getDistanceLeaderboardKey(null); - const cityDistanceKey = getDistanceLeaderboardKey(cityCode); + const levels = await Promise.all( + ancestorsOf(regionCode).map((code) => + creditDistance(h, code, numDistance, entryId, trimmedUsername) + ) + ); - // Distance leaderboards: lower = better, so use distance directly as score. - await Promise.all([ - zAdd(h, globalDistanceKey, numDistance, entryId), - zAdd(h, cityDistanceKey, numDistance, entryId), - ]); - - // Trim to top MAX_LEADERBOARD_SIZE (lowest distances). Sorted set is ascending, - // so remove ranks beyond MAX_LEADERBOARD_SIZE (the worst entries). - await Promise.all([ - zRemRangeByRank(h, globalDistanceKey, MAX_LEADERBOARD_SIZE, -1), - zRemRangeByRank(h, cityDistanceKey, MAX_LEADERBOARD_SIZE, -1), - ]); - - const [globalRank, cityRank] = await Promise.all([ - zRank(h, globalDistanceKey, entryId), - zRank(h, cityDistanceKey, entryId), - ]); - - const actualGlobalRank = globalRank !== null ? globalRank + 1 : null; - const actualCityRank = cityRank !== null ? cityRank + 1 : null; + const byLevel = (level) => + levels.find((entry) => getRegion(entry.code).level === level) ?? null; return { success: true, - globalDistance: { - username: trimmedUsername, - distance: numDistance, - rank: actualGlobalRank, - }, - cityDistance: { - username: trimmedUsername, - distance: numDistance, - rank: actualCityRank, - cityCode: cityCode, - }, + levels, + districtDistance: byLevel('district'), + provinceDistance: byLevel('province'), + globalDistance: byLevel('country'), + // Pre-tree name for the province level; see submitScore. + cityDistance: byLevel('province'), message: `Distance record: ${numDistance}m`, }; } catch (error) { diff --git a/src/lib/upstash.js b/src/lib/upstash.js index b8498b3..ab0d0a5 100644 --- a/src/lib/upstash.js +++ b/src/lib/upstash.js @@ -4,10 +4,16 @@ import { Redis } from '@upstash/redis'; // // Logical key namespace (callers pass these unprefixed): // session:{sessionId} string, TTL 30 min -// leaderboard:vietnam sorted set (score) -// leaderboard:city:{cityCode} sorted set (score) -// distance:vietnam sorted set (distance) -// distance:city:{cityCode} sorted set (distance) +// leaderboard:vietnam sorted set (score) -- the VN node +// leaderboard:city:{regionCode} sorted set (score) +// distance:vietnam sorted set (distance) -- the VN node +// distance:city:{regionCode} sorted set (distance) +// +// The ':city:' segment is a misnomer now that regions form a country > +// province > district tree, and {regionCode} may be any node below the +// country: 'tphcm', 'tphcm-q7', 'dl'. It is kept because renaming it would +// strand every key already holding a player's history. The country keeps its +// own two legacy key names for the same reason. // // Multi-tenancy: every physical Upstash key carries KEY_PREFIX (default // 'vngeoguessr:') so this project can safely share an Upstash DB with other @@ -31,7 +37,11 @@ export function getUpstash() { if (!url) throw new Error('UPSTASH_REDIS_REST_URL or KV_REST_API_URL is required'); if (!token) throw new Error('UPSTASH_REDIS_REST_TOKEN or KV_REST_API_TOKEN is required'); const client = new Redis({ url, token }); - const prefix = process.env.KEY_PREFIX ?? DEFAULT_KEY_PREFIX; + // `??` would accept KEY_PREFIX= (set but empty), which removes the only thing + // keeping this project's keys apart from every other project sharing the + // Upstash database. That was harmless while the adapter only touched keys it + // named; scanKeys enumerates, so an empty prefix would sweep co-tenant data. + const prefix = process.env.KEY_PREFIX || DEFAULT_KEY_PREFIX; handle = { client, prefix }; return handle; } @@ -142,6 +152,42 @@ export async function zRevRank(h, key, member) { return result == null ? null : Number(result); } +/** + * Every logical key matching a pattern. + * + * The prefix is applied to the pattern and stripped from the results, so + * callers work in the same logical namespace as every other function here. A + * caller that reached past the adapter and scanned `leaderboard:*` directly + * would match nothing, because the physical keys all carry KEY_PREFIX -- and an + * empty result is indistinguishable from an empty database, which is how a + * backup silently succeeds against the wrong namespace. + * + * Uses SCAN rather than KEYS: KEYS blocks the server for the whole sweep. + * @param {{ client: Redis, prefix: string }} h + * @param {string} pattern Logical glob, e.g. 'leaderboard:*'. + * @returns {Promise} Logical keys, prefix removed. + */ +export async function scanKeys(h, pattern) { + // SCAN guarantees each key is returned at least once, not exactly once: a + // key can repeat across pages when the keyspace rehashes mid-sweep. + const found = new Set(); + let cursor = '0'; + do { + const [next, batch] = await h.client.scan(cursor, { + match: pkey(h, pattern), + count: 500, + }); + cursor = String(next); + for (const key of batch) { + // Only strip a prefix that is actually there. Blind slicing would mangle + // a key, and with an empty prefix would hand back another tenant's. + if (!key.startsWith(h.prefix)) continue; + found.add(key.slice(h.prefix.length)); + } + } while (cursor !== '0'); + return [...found]; +} + /** * Trim a sorted set to a rank range. * @param {{ client: Redis, prefix: string }} h diff --git a/tests/fake-upstash-redis.js b/tests/fake-upstash-redis.js index 8a7dc32..b66b229 100644 --- a/tests/fake-upstash-redis.js +++ b/tests/fake-upstash-redis.js @@ -85,6 +85,31 @@ export class FakeRedis { return had || hadZset ? 1 : 0; } + /** + * Cursor-based key scan. + * + * Returns everything in one page: the store is in-memory, so a real cursor + * would only add a loop the tests cannot meaningfully exercise. Callers still + * have to handle the cursor protocol, because the real client does paginate. + */ + async scan(cursor, opts = {}) { + const keys = [...this.strings.keys(), ...this.zsets.keys()]; + const live = keys.filter((key) => !this.#expired(key)); + if (!opts.match) return ['0', live]; + + // Redis glob: * spans any run of characters, ? one. Everything else in the + // pattern is literal, so escape it before substituting the wildcards. + const escaped = opts.match.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + const pattern = new RegExp(`^${escaped.replace(/\*/g, '.*').replace(/\?/g, '.')}$`); + return ['0', live.filter((key) => pattern.test(key))]; + } + + /** True when a string key's TTL has elapsed. Sorted sets never expire here. */ + #expired(key) { + const entry = this.strings.get(key); + return Boolean(entry?.expireAt && entry.expireAt <= Date.now()); + } + async zadd(key, { score, member }) { let zset = this.zsets.get(key); if (!zset) { diff --git a/tests/leaderboard.test.js b/tests/leaderboard.test.js index ce500ec..ba36a62 100644 --- a/tests/leaderboard.test.js +++ b/tests/leaderboard.test.js @@ -189,3 +189,112 @@ describe('global leaderboard', () => { expect(await getLeaderboard()).toEqual([{ username: 'anh', score: 5, rank: 1 }]); }); }); + +describe('fan-out up the region tree', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('credits the district, its province and the country by the same amount', async () => { + const result = await submitScore('mai', 3, 'TPHCM-Q7'); + expect(result.levels.map((l) => l.code)).toEqual(['TPHCM-Q7', 'TPHCM', 'VN']); + for (const level of result.levels) expect(level.score).toBe(3); + + const keys = await storedKeys(); + expect(keys).toContain('vngeoguessr:leaderboard:city:tphcm-q7'); + expect(keys).toContain('vngeoguessr:leaderboard:city:tphcm'); + expect(keys).toContain('vngeoguessr:leaderboard:vietnam'); + }); + + it('rolls a second district into the same province and country totals', async () => { + await submitScore('mai', 3, 'TPHCM-Q7'); + const second = await submitScore('mai', 2, 'TPHCM-Q1'); + + expect(second.district.score).toBe(2); // Q1 alone + expect(second.province.score).toBe(5); // Q7 + Q1 + expect(second.global.score).toBe(5); + }); + + it('keeps Da Lat under Lam Dong', async () => { + // Pre-2025 units: Duc Hoa belongs to Long An, not the Tay Ninh it merged + // into. Both leaf codes stay bare so their history stays attached. + const daLat = await submitScore('mai', 4, 'DL'); + expect(daLat.levels.map((l) => l.code)).toEqual(['DL', 'LD', 'VN']); + + const ducHoa = await submitScore('mai', 1, 'DH'); + expect(ducHoa.levels.map((l) => l.code)).toEqual(['DH', 'LA', 'VN']); + expect(ducHoa.global.score).toBe(5); + }); + + it('credits two levels when the panorama sat outside every district', async () => { + // A province-level code is a legitimate scoring target: the panorama fell + // in a gap between simplified district outlines. + const result = await submitScore('mai', 2, 'DN'); + expect(result.levels.map((l) => l.code)).toEqual(['DN', 'VN']); + expect(result.district).toBeNull(); + expect(result.province.score).toBe(2); + }); + + it('maps the country to the pre-existing global key', async () => { + // Not leaderboard:city:vn -- the national board players already have has to + // keep accumulating rather than restarting under a new name. + await submitScore('mai', 1, 'DL'); + const keys = await storedKeys(); + expect(keys).toContain('vngeoguessr:leaderboard:vietnam'); + expect(keys).not.toContain('vngeoguessr:leaderboard:city:vn'); + }); + + it('fans distance records out with one shared id', async () => { + const result = await submitDistanceRecord('mai', 250, 'TPHCM-Q7'); + expect(result.levels.map((l) => l.code)).toEqual(['TPHCM-Q7', 'TPHCM', 'VN']); + + // The same attempt should be recognisable as one record at every level. + const district = await getLeaderboard('TPHCM-Q7', 10, 'distance'); + const country = await getLeaderboard('VN', 10, 'distance'); + expect(district[0].timestamp).toBe(country[0].timestamp); + expect(district[0].distance).toBe(250); + }); +}); + +describe('region validation', () => { + beforeEach(async () => { + await resetStore(); + }); + + it.each([ + ['submitScore', () => submitScore('mai', 3, 'NOPE')], + ['submitDistanceRecord', () => submitDistanceRecord('mai', 100, 'NOPE')], + ['getLeaderboard', () => getLeaderboard('NOPE')], + ])('%s rejects an unknown region', async (_name, call) => { + // Validated in the library, not only in the route: the migration script and + // any future caller bypass routes entirely, and the key builder lowercases + // whatever it is handed straight into a Redis key. + await expect(call()).rejects.toThrow(/Unknown region/); + }); + + it('creates no key for a rejected region', async () => { + await expect(submitScore('mai', 3, 'NOPE')).rejects.toThrow(); + expect(await storedKeys()).toEqual([]); + }); + + it.each([ + // 0 and NaN are falsy, so they take the default of 100 -- the same + // behaviour as the `parseInt(...) || 100` this replaced. + [-1, 1], + [0, 100], + [NaN, 100], + [99999, 200], + [Infinity, 200], + ['50', 50], + ])('clamps a limit of %s to %i rows', async (limit, expected) => { + // Seeded past the cap so each clamp lands on a distinguishable length. A + // board with one entry cannot tell a working clamp from a missing one. + for (let i = 0; i < 250; i++) await submitScore(`p${i}`, (i % 5) + 1, 'DL'); + expect((await getLeaderboard('DL', limit, 'score')).length).toBe(expected); + }); + + it('treats a null region as the country', async () => { + await submitScore('mai', 3, 'DL'); + expect(await getLeaderboard(null)).toEqual(await getLeaderboard('VN')); + }); +}); diff --git a/tests/migrate-leaderboards.test.js b/tests/migrate-leaderboards.test.js new file mode 100644 index 0000000..6c8f86a --- /dev/null +++ b/tests/migrate-leaderboards.test.js @@ -0,0 +1,216 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +vi.mock('@upstash/redis', async (importOriginal) => { + const { upstashModule } = await import('./mock-upstash.js'); + return upstashModule(importOriginal); +}); + +import { getUpstash, scanKeys, zAdd, zRangeWithScores } from '../src/lib/upstash.js'; +import { + backfillPairs, + copySortedSet, + exportAll, + findRegressions, + restore, + verifyTargets, +} from '../scripts/lib/leaderboard-migration.mjs'; +import { resetStore } from './redis-harness.js'; + +// The migration is the one place in this change where existing points can be +// lost, so its guards are exercised against the real functions the script runs. +// An earlier version of this file reimplemented the copy locally, which meant +// the missing empty-source guard was invisible to the suite. + +const h = () => getUpstash(); + +describe('scanKeys', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('finds leaderboard keys and hands them back unprefixed', async () => { + // Callers work in the logical namespace. A migration that reached past the + // adapter and scanned 'leaderboard:*' directly would match nothing, because + // every physical key carries KEY_PREFIX. + await zAdd(h(), 'leaderboard:city:dl', 5, 'mai'); + await zAdd(h(), 'leaderboard:vietnam', 5, 'mai'); + await zAdd(h(), 'distance:city:dl', 120, 'mai:120:1'); + + const found = await scanKeys(h(), 'leaderboard:*'); + expect(found.sort()).toEqual(['leaderboard:city:dl', 'leaderboard:vietnam']); + for (const key of found) expect(key.startsWith(h().prefix)).toBe(false); + }); + + it('keeps one pattern out of another namespace', async () => { + await zAdd(h(), 'leaderboard:city:dl', 5, 'mai'); + await zAdd(h(), 'distance:city:dl', 120, 'mai:120:1'); + expect(await scanKeys(h(), 'distance:*')).toEqual(['distance:city:dl']); + }); + + it('returns nothing for a pattern that matches nothing', async () => { + await zAdd(h(), 'leaderboard:city:dl', 5, 'mai'); + expect(await scanKeys(h(), 'nosuch:*')).toEqual([]); + }); + + it('never returns a duplicate', async () => { + // SCAN promises each key at least once, not exactly once. + await zAdd(h(), 'leaderboard:city:dl', 5, 'mai'); + const found = await scanKeys(h(), 'leaderboard:*'); + expect(new Set(found).size).toBe(found.length); + }); +}); + +describe('backfill safety', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('refuses an empty source that would wipe a populated destination', async () => { + // The dangerous case: DEL-then-copy with nothing to copy back empties the + // destination and looks like a clean no-op in the output. + await zAdd(h(), 'leaderboard:city:ld', 42, 'mai'); + + await expect( + copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', true) + ).rejects.toThrow(/Refusing/); + + const survived = await zRangeWithScores(h(), 'leaderboard:city:ld', 0, -1, false); + expect(survived).toEqual([{ value: 'mai', score: 42 }]); + }); + + it('treats both-empty as an ordinary no-op', async () => { + const result = await copySortedSet( + h(), + 'leaderboard:city:dh', + 'leaderboard:city:la', + true + ); + expect(result.skipped).toBe(true); + }); + + it('writes nothing in dry-run mode', async () => { + await zAdd(h(), 'leaderboard:city:dl', 12, 'mai'); + await copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', false); + expect(await zRangeWithScores(h(), 'leaderboard:city:ld', 0, -1, false)).toEqual([]); + }); + + it('copies absolute scores, so running twice is a no-op', async () => { + await zAdd(h(), 'leaderboard:city:dl', 12, 'mai'); + await zAdd(h(), 'leaderboard:city:dl', 7, 'linh'); + + await copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', true); + const once = await zRangeWithScores(h(), 'leaderboard:city:ld', 0, -1, false); + await copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', true); + const twice = await zRangeWithScores(h(), 'leaderboard:city:ld', 0, -1, false); + + // Adding instead of replacing would double every score on the second run. + expect(twice).toEqual(once); + expect(once.find((e) => e.value === 'mai').score).toBe(12); + }); + + it('removes a destination-only member instead of stranding it', async () => { + // Reachable in practice: the 200-entry trim can drop a player from the + // source while they survive in the copy, and a member-by-member write would + // leave them there with a stale score forever. + await zAdd(h(), 'leaderboard:city:dl', 12, 'mai'); + await zAdd(h(), 'leaderboard:city:ld', 999, 'orphan'); + + await copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', true); + + const after = await zRangeWithScores(h(), 'leaderboard:city:ld', 0, -1, false); + expect(after.map((e) => e.value)).toEqual(['mai']); + }); + + it('leaves the source untouched', async () => { + await zAdd(h(), 'leaderboard:city:dh', 8, 'mai'); + const before = await zRangeWithScores(h(), 'leaderboard:city:dh', 0, -1, false); + await copySortedSet(h(), 'leaderboard:city:dh', 'leaderboard:city:la', true); + expect(await zRangeWithScores(h(), 'leaderboard:city:dh', 0, -1, false)).toEqual(before); + }); +}); + +describe('backfill verification', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('derives its keys from the app, not from hardcoded strings', async () => { + const pairs = backfillPairs(); + expect(pairs.map((p) => `${p.fromKey}->${p.toKey}`)).toEqual([ + 'leaderboard:city:dl->leaderboard:city:ld', + 'distance:city:dl->distance:city:ld', + 'leaderboard:city:dh->leaderboard:city:la', + 'distance:city:dh->distance:city:la', + ]); + }); + + it('confirms each destination matches its source', async () => { + await zAdd(h(), 'leaderboard:city:dl', 12, 'mai'); + expect(await verifyTargets(h())).not.toEqual([]); + + await copySortedSet(h(), 'leaderboard:city:dl', 'leaderboard:city:ld', true); + expect(await verifyTargets(h())).toEqual([]); + }); + + it('reports a destination that never got written', async () => { + // The earlier verification excluded exactly the keys the script writes, so + // a failed copy passed silently. + await zAdd(h(), 'leaderboard:city:dl', 12, 'mai'); + const mismatched = await verifyTargets(h()); + expect(mismatched.join()).toMatch(/leaderboard:city:ld/); + }); +}); + +describe('regression detection', () => { + it('accepts forward-only drift from live traffic', () => { + // Ordering is deploy-then-migrate, so players keep scoring while the script + // runs. Demanding byte-equality would throw on a healthy migration. + const before = { 'leaderboard:vietnam': [{ value: 'mai', score: 10 }] }; + const after = { + 'leaderboard:vietnam': [ + { value: 'mai', score: 13 }, + { value: 'linh', score: 4 }, + ], + }; + expect(findRegressions(before, after)).toEqual([]); + }); + + it('flags a score that went backwards', () => { + const before = { 'leaderboard:vietnam': [{ value: 'mai', score: 10 }] }; + const after = { 'leaderboard:vietnam': [{ value: 'mai', score: 3 }] }; + expect(findRegressions(before, after).join()).toMatch(/fell from 10 to 3/); + }); + + it('flags a member that disappeared', () => { + const before = { 'leaderboard:city:hn': [{ value: 'mai', score: 10 }] }; + const after = { 'leaderboard:city:hn': [] }; + expect(findRegressions(before, after).join()).toMatch(/member mai removed/); + }); + + it('flags a key that disappeared', () => { + const before = { 'leaderboard:city:hn': [{ value: 'mai', score: 10 }] }; + expect(findRegressions(before, {}).join()).toMatch(/disappeared/); + }); + + it('ignores the backfill destinations, which are meant to change', () => { + const before = { 'leaderboard:city:ld': [{ value: 'orphan', score: 99 }] }; + expect(findRegressions(before, { 'leaderboard:city:ld': [] })).toEqual([]); + }); +}); + +describe('restore', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('puts a snapshot back, making the backup an actual rollback', async () => { + await zAdd(h(), 'leaderboard:city:hn', 50, 'mai'); + await zAdd(h(), 'leaderboard:city:hn', 20, 'linh'); + const snapshot = await exportAll(h()); + + // Simulate the damage a bad run would do. + await zAdd(h(), 'leaderboard:city:hn', 1, 'mai'); + await restore(h(), snapshot); + + expect(await exportAll(h())).toEqual(snapshot); + }); +});