diff --git a/docs/REMEDIATION_PLAN.md b/docs/REMEDIATION_PLAN.md index 79b895d..75c4704 100644 --- a/docs/REMEDIATION_PLAN.md +++ b/docs/REMEDIATION_PLAN.md @@ -28,7 +28,7 @@ Key: `S` ≤ half day · `M` ~1–3 days · `L` ~1 week+ · 🔴 launch-blocker - [x] **1.1 M — `src/money/atomic.js` BigInt module.** ✅ Exact integer money math (`toBig`/`toSafe`/`sum`/`add`/`mulByDecimal`/`format`). `calculatePayout` now computes `base * multiplier` via BigInt (half-up rounding, narrows to number only when exact). `walletRPCService` batch total + fee sum use `money.sum` instead of float `reduce`. Property tests (`moneyAtomic.test.js`) prove exactness on amounts above 2^53 where the old float path drifted. - [x] **1.2 S — Snapshot payout config at game start.** ✅ Migration 016 adds `payout_escape_amount`/`payout_treasure_amount` (BIGINT) + multipliers to `games`. Both start paths stamp the resolved payout amounts (via `_computePayoutSnapshot`); `completeGame` pays from the snapshot (falls back to live calc only for pre-snapshot games). New `payoutSnapshot.test.js` proves a mid-game config change can't alter an in-flight payout. - [x] **1.3 S — Close payout uniqueness holes.** ✅ `processing` covered in the unique index (migration 015) and in the in-code existing-payout guard. Migration 017 adds idempotent `NOT VALID` CHECK constraints on `payments.status`, `payments.payment_type`, and `payouts.status` so a typo'd status can't silently orphan a money row (enforces new writes, won't choke on legacy rows). _(games.status/game_mode CHECKs deferred — transient states, lower money-impact.)_ -- [ ] **1.4 S — Fix retry stat guard + non-atomic tx_hash write** (`src/payments/payoutRetryService.js:121,174`). +- [x] **1.4 S — Fix retry stat guard + non-atomic tx_hash write.** ✅ The "already on-chain" path used a `NOT EXISTS` guard against the row it had just marked completed (always skipped → never counted); replaced with a conditional `UPDATE … WHERE status <> 'completed' RETURNING` so stats count exactly once. Successful retries now store `tx_hash` + mark completed + count stats in ONE transaction (no stranded-tx_hash window). New `payoutRetry.test.js` covers count-once, no-double-count, and atomic completion. - [ ] **1.5 S — Remove dead refund send-path** (`src/index.js:795`). - [ ] **1.6 S — `credits` → BIGINT; make migration 010 idempotent** (`src/migrations/010_security_constraints.sql`); per-migration transactions in runner (`src/db/databaseManager.js:146`). diff --git a/src/payments/payoutRetryService.js b/src/payments/payoutRetryService.js index 1bf95a4..e7a3db4 100644 --- a/src/payments/payoutRetryService.js +++ b/src/payments/payoutRetryService.js @@ -119,22 +119,25 @@ class PayoutRetryService { if (txStatus.exists) { // Transaction is in blockchain/mempool - mark as completed, don't retry! await this.db.withTransaction(async (client) => { - await client.query(` + // Transition to completed ONLY if not already completed. The RETURNING + // tells us whether THIS call did the transition, so we count user stats + // exactly once (the previous guard checked the same row it had just + // updated, so it never counted at all). + const transitioned = await client.query(` UPDATE payouts SET status = 'completed', processed_at = NOW() - WHERE id = $1 + WHERE id = $1 AND status <> 'completed' + RETURNING id `, [id]); - // Update user stats (may have been missed if original DB update failed) - await client.query(` - UPDATE users - SET total_amount_won = COALESCE(total_amount_won, 0) + $1, - total_payouts_received = COALESCE(total_payouts_received, 0) + 1 - WHERE id = $2 AND NOT EXISTS ( - SELECT 1 FROM payouts - WHERE id = $3 AND status = 'completed' AND processed_at IS NOT NULL - ) - `, [amount, user_id, id]); + if (transitioned.rows.length > 0) { + await client.query(` + UPDATE users + SET total_amount_won = COALESCE(total_amount_won, 0) + $1, + total_payouts_received = COALESCE(total_payouts_received, 0) + 1 + WHERE id = $2 + `, [amount, user_id]); + } }); console.log(`✅ Payout ${id} already in blockchain (${txStatus.confirmations} confirmations), marked completed`); return; @@ -169,31 +172,35 @@ class PayoutRetryService { description: `Retry payout ${id} for game ${game_id}` }); - // IMMEDIATELY store tx_hash (same pattern as gameModeManager) - if (payoutResult.success && payoutResult.txHash) { - await this.db.query(` - UPDATE payouts SET tx_hash = $1, fee = $2 WHERE id = $3 - `, [payoutResult.txHash, payoutResult.fee, id]); + if (!payoutResult.success) { + throw new AppError(`Payout ${id} retry did not succeed`); } - // Update payout record with success + // Store tx_hash, mark completed, and count stats ATOMICALLY in one transaction, + // so the process can't die between writing tx_hash and marking completed (which + // previously left a tx_hash on a non-completed row). The conditional transition + // (status <> 'completed') makes stat-counting exactly-once. await this.db.withTransaction(async (client) => { - await client.query(` + const transitioned = await client.query(` UPDATE payouts - SET status = 'completed', + SET tx_hash = $1, + fee = $2, + status = 'completed', processed_at = NOW(), retry_count = COALESCE(retry_count, 0) + 1, last_retry_at = NOW() - WHERE id = $1 - `, [id]); + WHERE id = $3 AND status <> 'completed' + RETURNING id + `, [payoutResult.txHash || null, payoutResult.fee || null, id]); - // Update user stats - await client.query(` - UPDATE users - SET total_amount_won = COALESCE(total_amount_won, 0) + $1, - total_payouts_received = COALESCE(total_payouts_received, 0) + 1 - WHERE id = $2 - `, [amount, user_id]); + if (transitioned.rows.length > 0) { + await client.query(` + UPDATE users + SET total_amount_won = COALESCE(total_amount_won, 0) + $1, + total_payouts_received = COALESCE(total_payouts_received, 0) + 1 + WHERE id = $2 + `, [amount, user_id]); + } }); console.log(`✅ Payout ${id} retry succeeded: ${payoutResult.txHash}`); diff --git a/test/payoutRetry.test.js b/test/payoutRetry.test.js new file mode 100644 index 0000000..726c740 --- /dev/null +++ b/test/payoutRetry.test.js @@ -0,0 +1,87 @@ +/** + * Payout retry service tests (Phase 1.4). + * + * Guards two previously-broken behaviours: + * - the "already on-chain" path counts user stats exactly once (the old NOT EXISTS guard + * checked the same row it had just marked completed, so it never counted), and + * - a successful retry stores tx_hash + marks completed + counts stats in ONE transaction + * (no non-atomic tx_hash write that could strand a tx_hash on a non-completed row). + */ + +const PayoutRetryService = require('../src/payments/payoutRetryService'); + +function makeHarness() { + const client = { query: jest.fn().mockResolvedValue({ rows: [] }) }; + const db = { + query: jest.fn().mockResolvedValue({ rows: [] }), + withTransaction: jest.fn().mockImplementation(async (cb) => cb(client)) + }; + return { client, db }; +} + +function svcWith(db, wallet) { + return new PayoutRetryService({ + db, walletService: wallet, debugManager: { CONSOLE_LOGGING: false }, maxRetries: 3 + }); +} + +const findCall = (mockFn, re) => mockFn.mock.calls.find(c => re.test(c[0])); + +describe('PayoutRetryService.retryPayout', () => { + test('already-on-chain: marks completed and counts stats exactly once', async () => { + const { client, db } = makeHarness(); + client.query + .mockResolvedValueOnce({ rows: [{ id: 5 }] }) // payouts transition (did transition) + .mockResolvedValue({ rows: [] }); // users stat update + const wallet = { + checkTransactionStatus: jest.fn().mockResolvedValue({ exists: true, confirmations: 3 }), + processPayout: jest.fn() + }; + + await svcWith(db, wallet).retryPayout({ id: 5, user_id: 1, amount: '100', tx_hash: 'tx5', status: 'failed', retry_count: 0 }); + + // Transition guarded by `status <> 'completed'` (so it counts once). + expect(findCall(client.query, /UPDATE payouts[\s\S]*status <> 'completed'/i)).toBeDefined(); + // Stats were counted. + expect(findCall(client.query, /UPDATE users[\s\S]*total_amount_won/i)).toBeDefined(); + // Did NOT re-send funds (already on chain). + expect(wallet.processPayout).not.toHaveBeenCalled(); + }); + + test('already-on-chain but already completed: does NOT double-count stats', async () => { + const { client, db } = makeHarness(); + client.query + .mockResolvedValueOnce({ rows: [] }) // payouts transition affected 0 rows (already completed) + .mockResolvedValue({ rows: [] }); + const wallet = { + checkTransactionStatus: jest.fn().mockResolvedValue({ exists: true, confirmations: 9 }), + processPayout: jest.fn() + }; + + await svcWith(db, wallet).retryPayout({ id: 5, user_id: 1, amount: '100', tx_hash: 'tx5', status: 'failed', retry_count: 0 }); + + expect(findCall(client.query, /UPDATE users[\s\S]*total_amount_won/i)).toBeUndefined(); + }); + + test('successful retry: stores tx_hash, completes, counts stats in one transaction', async () => { + const { client, db } = makeHarness(); + client.query + .mockResolvedValueOnce({ rows: [{ id: 7 }] }) // completion transition + .mockResolvedValue({ rows: [] }); + const wallet = { + checkTransactionStatus: jest.fn(), + processPayout: jest.fn().mockResolvedValue({ success: true, txHash: 'txNEW', fee: 10 }) + }; + + await svcWith(db, wallet).retryPayout({ id: 7, user_id: 1, amount: '100', payout_address: 'wow1addr', status: 'failed', retry_count: 0 }); + + // tx_hash + completion happen in the SAME statement (atomic). + const completion = findCall(client.query, /UPDATE payouts[\s\S]*tx_hash[\s\S]*status = 'completed'/i); + expect(completion).toBeDefined(); + expect(completion[1]).toContain('txNEW'); + // No standalone pre-transaction tx_hash write on db.query. + expect(findCall(db.query, /UPDATE payouts SET tx_hash = \$1, fee = \$2 WHERE id/i)).toBeUndefined(); + // Stats counted once. + expect(findCall(client.query, /UPDATE users[\s\S]*total_amount_won/i)).toBeDefined(); + }); +});