From b8837967c7eed6fc0a8270b427deb83b629b539a Mon Sep 17 00:00:00 2001 From: 0xward <0xward.dev@gmail.com> Date: Sun, 2 Aug 2026 18:08:59 +0000 Subject: [PATCH] Every kill since the season board shipped was thrown away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OWNER, looking at the live Season 2 board: *"aku test login pakai email, tapi di leaderboard tidak tercatat jumlah kills nya??"* — his row read 1,095 XP and 0 kills. So did every other row. One line in the wrong place. Firestore rejects a transaction that reads after it writes: "Firestore transactions require all reads to be executed before all writes." recordRunKills gained a season mirror and the mirror's `tx.get` went in BELOW the career `tx.set`. Every call threw, the catch turned it into a console line nobody was watching, and because Firestore buffers a transaction's writes until it commits, NOTHING landed — not the season kills the change was for, and not the career kills that had worked for months. XP was fine on the same rows, which is exactly why nobody caught it: recordRunProgress does one read and two writes and was never affected. A column that reads 0 looks like a column nobody has earned yet. The fix is to hoist both reads above both writes. The value is in the test. scripts/test-leaderboard-writes.js runs the real functions against a stub that enforces Firestore's constraint AND buffers writes the way a real transaction does — a stub that applied writes as they were made would have shown the career column surviving and understated the damage. Reverted against the old code it fails four assertions, including both totals reading empty. It also asserts the RULE rather than this instance: no transaction anywhere in the module may read after it writes. The next person to add a mirror gets a red test instead of a silently empty column. The delta logic the throw was hiding is now covered too — a Revive keeps p.kills climbing, so 23 then 30 must add 7; a fresh run reports 5 and must add 5, not subtract 25. Verified: 8/8 test:kills, tsc clean, lint unchanged. NOT included, because they need the owner's decision rather than a guess: the food drop rate and the props he reported near a doorway. Measurements for both are in the PR. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017A764RdnwpyWnG7uCNhMiQ --- lib/leaderboardService.ts | 21 +++- package.json | 3 +- scripts/test-leaderboard-writes.js | 176 +++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 5 deletions(-) create mode 100644 scripts/test-leaderboard-writes.js diff --git a/lib/leaderboardService.ts b/lib/leaderboardService.ts index 85c3930..fabc0eb 100644 --- a/lib/leaderboardService.ts +++ b/lib/leaderboardService.ts @@ -162,8 +162,25 @@ export async function recordRunKills( const normalizedAddr = walletAddress.toLowerCase() const ref = doc(db, 'leaderboard', normalizedAddr) await runTransaction(db, async (tx) => { + // ── BOTH READS FIRST. THIS ORDER IS NOT A STYLE CHOICE ───────────────── + // + // Firestore rejects a transaction that reads after it writes: "Firestore + // transactions require all reads to be executed before all writes." The + // season mirror was added below the career write, so every call threw, + // the catch swallowed it into a console line nobody was watching, and + // NOTHING was recorded — not the season kills it was added for, and not + // the career kills that had worked for months. + // + // Owner, from the live board: *"aku test login pakai email, tapi di + // leaderboard tidak tercatat jumlah kills nya?"* XP was fine on the same + // row, which is the tell — recordRunProgress does one read and two + // writes, so it was never affected. + const seasonId = seasonKey() + const seasonRef = seasonPlayerRef(seasonId, normalizedAddr) const snap = await tx.get(ref) + const seasonSnap = await tx.get(seasonRef) const data = (snap.exists() ? snap.data() : {}) as Partial + const seasonPrev = (seasonSnap.exists() ? seasonSnap.data() : {}) as { kills?: number } const prevTotal = data.totalKills ?? data.kills ?? 0 const lastRecorded = data.lastRecordedKills ?? 0 const delta = @@ -182,10 +199,6 @@ export async function recordRunKills( // Season kills ride the SAME delta — the hard part (not double-counting a // Revive that keeps p.kills climbing) is already solved above, and // solving it twice is how the two totals would drift apart. - const seasonId = seasonKey() - const seasonRef = seasonPlayerRef(seasonId, normalizedAddr) - const seasonSnap = await tx.get(seasonRef) - const seasonPrev = (seasonSnap.exists() ? seasonSnap.data() : {}) as { kills?: number } tx.set( seasonRef, { diff --git a/package.json b/package.json index 1c77167..dbc73a6 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,8 @@ "test:pass-cards": "node scripts/test-season-pass-cards.js --serve", "test:seasonxp": "node scripts/test-season-xp.js", "test:exit-streak": "node scripts/test-exit-streak.js", - "test:privy": "node scripts/test-privy-gate.js --serve" + "test:privy": "node scripts/test-privy-gate.js --serve", + "test:kills": "node scripts/test-leaderboard-writes.js" }, "overrides": { "permissionless": { diff --git a/scripts/test-leaderboard-writes.js b/scripts/test-leaderboard-writes.js new file mode 100644 index 0000000..f45180f --- /dev/null +++ b/scripts/test-leaderboard-writes.js @@ -0,0 +1,176 @@ +#!/usr/bin/env node +/** + * test-leaderboard-writes.js — the transaction rule that ate every kill. + * + * OWNER, looking at the live Season 2 board: *"aku test login pakai email, tapi + * di leaderboard tidak tercatat jumlah kills nya??"* — his row read 1,095 XP + * and 0 kills, and so did every other row on the board. + * + * The cause was one line in the wrong place. Firestore rejects a transaction + * that reads after it writes: + * + * "Firestore transactions require all reads to be executed before all + * writes." + * + * recordRunKills gained a season mirror, and the mirror's `tx.get` was written + * below the career `tx.set`. So the transaction threw on every call, the catch + * turned it into a console line nobody was watching, and NOTHING was recorded — + * not the season kills the change was for, and not the career kills that had + * been working for months. XP was untouched on the same rows, which is exactly + * why it went unnoticed: recordRunProgress does one read and two writes. + * + * This runs the real functions against a stub that ENFORCES the rule, so the + * next person who adds a read to one of these transactions gets a failing test + * instead of a silently empty column. + * + * node scripts/test-leaderboard-writes.js + */ +const { execFileSync } = require('child_process') +const path = require('path'), os = require('os') + +let fails = 0 +const ok = (l, c, d) => { console.log((c ? ' ✓ ' : ' ✗ FAIL: ') + l + (d !== undefined ? ' (' + d + ')' : '')); if (!c) fails++ } + +// ── the stub: Firestore's actual constraint, enforced ──────────────────────── +const store = new Map() +let wroteInThisTx = false + +const key = (r) => r.__path +const mkRef = (p) => ({ __path: p }) + +// Writes are BUFFERED and only committed when the whole transaction resolves, +// which is what Firestore actually does — and it is the reason this bug lost +// the career kills too, not just the season mirror it was introduced for. A +// stub that applied writes as they were made would have shown the career +// column surviving and quietly understated the damage. +function makeTx(pending) { + return { + async get(ref) { + if (wroteInThisTx) { + // The real SDK's message, so a failure here reads like production. + throw new Error('Firestore transactions require all reads to be executed before all writes.') + } + const v = store.get(key(ref)) + return { exists: () => v !== undefined, data: () => v } + }, + set(ref, value) { + wroteInThisTx = true + pending.push([key(ref), value]) + }, + } +} + +const errors = [] +const stubFirestore = ` + const store = ${'globalThis.__store'}; +` + +// The module imports firebase/firestore. Rather than stand up the SDK, bundle +// it with that import aliased to this stub — same code path, no network. +const stubPath = path.join(os.tmpdir(), 'ns-fs-stub-' + process.pid + '.js') +require('fs').writeFileSync(stubPath, ` + export const doc = (db, col, id) => ({ __path: col + '/' + id }) + export const collection = (db, ...p) => ({ __path: p.join('/') }) + export const getDoc = async () => ({ exists: () => false, data: () => ({}) }) + export const getDocs = async () => ({ docs: [] }) + export const setDoc = async () => {} + export const updateDoc = async () => {} + export const query = (...a) => a + export const orderBy = (...a) => a + export const limit = (...a) => a + export const where = (...a) => a + export const serverTimestamp = () => Date.now() + export const increment = (n) => n + export const runTransaction = async (db, fn) => globalThis.__runTx(fn) + export const writeBatch = () => ({ set(){}, update(){}, async commit(){} }) + export const deleteDoc = async () => {} + export const startAfter = (...a) => a + export const documentId = () => '__name__' + export const Timestamp = { now: () => ({ toMillis: () => Date.now() }) } + export const getFirestore = () => ({}) +`) +// firebase/app is only reached through lib/firebase.ts, which runs at import +// time. Stub it so importing the module under test does not try to stand up a +// real Firebase app from environment variables that are not here. +const appStub = path.join(os.tmpdir(), 'ns-app-stub-' + process.pid + '.js') +require('fs').writeFileSync(appStub, ` + export const initializeApp = () => ({}) + export const getApps = () => [] +`) + +const out = path.join(os.tmpdir(), 'ns-lb-' + process.pid + '.cjs') +execFileSync('npx', ['esbuild', 'lib/leaderboardService.ts', '--bundle', '--platform=node', + '--format=cjs', '--log-level=error', + '--alias:firebase/firestore=' + stubPath, + '--alias:firebase/app=' + appStub, + '--alias:@=' + path.resolve('.'), + '--outfile=' + out], + { stdio: ['ignore', 'ignore', 'inherit'] }) + +globalThis.__runTx = async (fn) => { + wroteInThisTx = false + const pending = [] + const result = await fn(makeTx(pending)) // a throw here commits nothing + for (const [k, v] of pending) store.set(k, Object.assign({}, store.get(k) || {}, v)) + return result +} + +const L = require(out) + +const W = '0x' + '77'.repeat(20) +const orig = console.error +console.error = (...a) => errors.push(a.join(' ')) + +;(async () => { + // ── the bug, in the shape the owner saw it ──────────────────────────────── + await L.recordRunKills(W, 23) + console.error = orig + + ok('the transaction does not throw on the read-after-write rule', + !errors.some((e) => /reads to be executed before all writes/.test(e)), + errors[0] || 'no errors') + + const career = store.get('leaderboard/' + W) + ok('career kills are recorded', career && career.totalKills === 23, + JSON.stringify(career)) + ok('and the raw counter is remembered, so a Revive cannot double-count', + career && career.lastRecordedKills === 23) + + const seasonPath = [...store.keys()].find((k) => /^seasonLeaderboard\//.test(k)) + const season = seasonPath ? store.get(seasonPath) : null + ok('season kills are recorded too — the column that read 0 on the live board', + season && season.kills === 23, JSON.stringify(season)) + + // ── the delta logic, which the throw was hiding ─────────────────────────── + // p.kills keeps climbing across a Revive within one session, so the same + // life reporting 23 then 30 must add 7, not 30. + await L.recordRunKills(W, 30) + ok('a second report from the same life adds only the difference', + store.get('leaderboard/' + W).totalKills === 30, + String(store.get('leaderboard/' + W).totalKills)) + ok('and the season total tracks it exactly', store.get(seasonPath).kills === 30, + String(store.get(seasonPath).kills)) + + // A fresh run starts p.kills at 0 again; 5 must count in full, not as -25. + await L.recordRunKills(W, 5) + ok('a fresh life counts in full rather than going backwards', + store.get('leaderboard/' + W).totalKills === 35, + String(store.get('leaderboard/' + W).totalKills)) + + // ── the guard, stated as a rule rather than an instance ─────────────────── + // Every transaction in the module, not just the one that broke. + const src = require('fs').readFileSync('lib/leaderboardService.ts', 'utf8') + const bodies = src.split('runTransaction(db, async (tx) => {').slice(1) + let offenders = 0 + for (const b of bodies) { + const body = b.slice(0, b.indexOf('\n })')) + const firstSet = body.indexOf('tx.set') + const lastGet = body.lastIndexOf('tx.get') + if (firstSet >= 0 && lastGet > firstSet) offenders++ + } + ok('no transaction in the module reads after it writes', offenders === 0, + offenders + ' of ' + bodies.length) + + console.log(fails ? ` ${fails} GAGAL` : ' semua lolos') + process.exit(fails ? 1 : 0) +})().catch((e) => { console.error = orig; console.error('ERROR', e.message); process.exit(2) })