diff --git a/HISTORY.md b/HISTORY.md index 72c27bb..6265bdf 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,6 +1,10 @@ # Bill Tracker — Changelog ## v0.41.0 +### 💰 Payment atomicity audit (Track A) — every balance mutation is now transactional + +- **[Money integrity] Wrapped the four remaining non-atomic payment paths in `db.transaction()`.** Quick-pay and bulk-pay were already atomic (X1), but the audit found the INSERT/UPDATE + `applyBalanceDelta` pair split across two un-transactional statements on **manual `POST /payments`**, **autopay-suggestion confirm**, **`DELETE` (unpay)**, and **`POST /:id/restore`** — plus **`PUT` (edit)**, where the bill-balance write and the payment `UPDATE` weren't atomic. A crash/constraint failure between the two statements could leave a payment without its balance adjustment (or a balance reversed without the row deleted). Each is now a single transaction: the row write and the balance mutation land together or neither. Behavior-preserving — `tests/paymentsRoute.test.js` pins the create → delete → restore → edit balance round-trip and the double-restore no-op; full server suite green (186 tests). (Dedupe-guard work for the manual path follows as a separate change.) + ### 🔷 TypeScript foundation + branded money types - **[Client] Adopted TypeScript incrementally, starting with the money boundary** — the move off plain JS. Upgraded `jsconfig` → `tsconfig` with full `strict` + `noUncheckedIndexedAccess`, but `allowJs` + `checkJs:false` so every existing `.js`/`.jsx` keeps resolving and running untouched — only `.ts`/`.tsx` are type-checked, so it's a file-by-file migration with nothing breaking in between. Added `npm run typecheck` (`tsc --noEmit`) and wired it into `npm run ci`. Installed TypeScript 6 + `@types/react`/`react-dom` 19. diff --git a/routes/payments.js b/routes/payments.js index 7a520a4..b022ee4 100644 --- a/routes/payments.js +++ b/routes/payments.js @@ -204,13 +204,19 @@ router.post('/', (req, res) => { const balCalc = computeBalanceDelta(bill, payment.amount); const failureFlag = autopay_failure ? 1 : 0; - const result = db.prepare( - 'INSERT INTO payments (bill_id, amount, paid_date, method, notes, balance_delta, interest_delta, payment_source, autopay_failure) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)' - ).run(payment.bill_id, payment.amount, payment.paid_date, method || null, notes || null, balCalc?.balance_delta ?? null, balCalc?.interest_delta ?? null, payment.payment_source, failureFlag); - applyBalanceDelta(db, bill.id, balCalc); + // Atomic: the INSERT and the balance update land together or neither, so a + // mid-way failure never leaves a payment without its balance adjustment. + const insertManualPayment = db.transaction(() => { + const r = db.prepare( + 'INSERT INTO payments (bill_id, amount, paid_date, method, notes, balance_delta, interest_delta, payment_source, autopay_failure) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)' + ).run(payment.bill_id, payment.amount, payment.paid_date, method || null, notes || null, balCalc?.balance_delta ?? null, balCalc?.interest_delta ?? null, payment.payment_source, failureFlag); + applyBalanceDelta(db, bill.id, balCalc); + return r.lastInsertRowid; + }); + const paymentId = insertManualPayment(); - res.status(201).json(serializePayment(db.prepare('SELECT * FROM payments WHERE id = ?').get(result.lastInsertRowid))); + res.status(201).json(serializePayment(db.prepare('SELECT * FROM payments WHERE id = ?').get(paymentId))); }); // POST /api/payments/quick — pay a bill (expected amount, today) @@ -315,25 +321,31 @@ router.post('/autopay-suggestions/:billId/confirm', (req, res) => { } const balCalc = computeBalanceDelta(bill, suggestedPayment.amount); - const result = db.prepare(` - INSERT INTO payments (bill_id, amount, paid_date, method, notes, balance_delta, interest_delta, payment_source) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `).run( - bill.id, - suggestedPayment.amount, - suggestedPayment.paid_date, - 'autopay', - 'Confirmed autopay suggestion', - balCalc?.balance_delta ?? null, - balCalc?.interest_delta ?? null, - 'manual', - ); - applyBalanceDelta(db, bill.id, balCalc); - db.prepare('DELETE FROM autopay_suggestion_dismissals WHERE user_id = ? AND bill_id = ? AND year = ? AND month = ?') - .run(req.user.id, bill.id, ym.year, ym.month); + // Atomic: the payment INSERT, its balance update, and clearing the dismissal + // must all land or none, so a mid-way failure can't leave a half-recorded pay. + const confirmAutopay = db.transaction(() => { + const r = db.prepare(` + INSERT INTO payments (bill_id, amount, paid_date, method, notes, balance_delta, interest_delta, payment_source) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `).run( + bill.id, + suggestedPayment.amount, + suggestedPayment.paid_date, + 'autopay', + 'Confirmed autopay suggestion', + balCalc?.balance_delta ?? null, + balCalc?.interest_delta ?? null, + 'manual', + ); + applyBalanceDelta(db, bill.id, balCalc); + db.prepare('DELETE FROM autopay_suggestion_dismissals WHERE user_id = ? AND bill_id = ? AND year = ? AND month = ?') + .run(req.user.id, bill.id, ym.year, ym.month); + return r.lastInsertRowid; + }); + const paymentId = confirmAutopay(); - res.status(201).json({ created: true, payment: serializePayment(db.prepare('SELECT * FROM payments WHERE id = ?').get(result.lastInsertRowid)) }); + res.status(201).json({ created: true, payment: serializePayment(db.prepare('SELECT * FROM payments WHERE id = ?').get(paymentId)) }); }); // POST /api/payments/autopay-suggestions/:billId/dismiss @@ -467,13 +479,18 @@ router.put('/:id', (req, res) => { const bill = db.prepare('SELECT * FROM bills WHERE id = ? AND user_id = ? AND deleted_at IS NULL').get(existing.bill_id, req.user.id); let nextInterestDelta = existing.interest_delta ?? null; + // Compute the balance mutation without writing yet — the write happens inside + // the transaction below so it lands atomically with the payment UPDATE. + let balCalc = null; + let paymentPortion = null; + let restoredBalance = null; if (bill) { // Reverse only the *payment* portion of the stored delta (not the interest component) // so that interest already charged this month is not double-counted. For legacy rows // where interest_delta is NULL, fall back to reversing the full delta as before. const interestPortion = existing.interest_delta ?? 0; - const paymentPortion = existing.balance_delta != null ? existing.balance_delta - interestPortion : null; - let restoredBalance = bill.current_balance; + paymentPortion = existing.balance_delta != null ? existing.balance_delta - interestPortion : null; + restoredBalance = bill.current_balance; if (paymentPortion != null && bill.current_balance != null) { restoredBalance = Math.max(0, bill.current_balance - paymentPortion); } @@ -481,38 +498,45 @@ router.put('/:id', (req, res) => { // interest_accrued_month is still set to this month (if interest was charged) so // computeBalanceDelta will skip interest when the payment is within the same month, // and charge a fresh month of interest if editing into a new calendar month. - const balCalc = computeBalanceDelta({ ...bill, current_balance: restoredBalance }, nextAmount); + balCalc = computeBalanceDelta({ ...bill, current_balance: restoredBalance }, nextAmount); nextBalanceDelta = balCalc?.balance_delta ?? null; nextInterestDelta = balCalc?.interest_delta ?? null; - if (balCalc) { - applyBalanceDelta(db, existing.bill_id, balCalc); - } else if (paymentPortion != null && restoredBalance != null) { - db.prepare("UPDATE bills SET current_balance = ?, updated_at = datetime('now') WHERE id = ?") - .run(restoredBalance, existing.bill_id); - } } const { autopay_failure } = req.body; const nextAutopayFailure = autopay_failure !== undefined ? (autopay_failure ? 1 : 0) : existing.autopay_failure; - db.prepare(` - UPDATE payments SET - amount = ?, paid_date = ?, method = ?, notes = ?, balance_delta = ?, interest_delta = ?, - payment_source = ?, autopay_failure = ?, updated_at = datetime('now') - WHERE id = ? - AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL) - `).run( - nextAmount, - nextPaidDate, - method !== undefined ? (method || null) : existing.method, - notes !== undefined ? (notes || null) : existing.notes, - nextBalanceDelta, - nextInterestDelta, - nextPaymentSource, - nextAutopayFailure, - req.params.id, - req.user.id, - ); + // Atomic: the bill-balance write and the payment UPDATE land together or neither, + // so an edit never leaves the balance reversed without the row updated (or vice versa). + const applyEdit = db.transaction(() => { + if (bill) { + if (balCalc) { + applyBalanceDelta(db, existing.bill_id, balCalc); + } else if (paymentPortion != null && restoredBalance != null) { + db.prepare("UPDATE bills SET current_balance = ?, updated_at = datetime('now') WHERE id = ?") + .run(restoredBalance, existing.bill_id); + } + } + db.prepare(` + UPDATE payments SET + amount = ?, paid_date = ?, method = ?, notes = ?, balance_delta = ?, interest_delta = ?, + payment_source = ?, autopay_failure = ?, updated_at = datetime('now') + WHERE id = ? + AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL) + `).run( + nextAmount, + nextPaidDate, + method !== undefined ? (method || null) : existing.method, + notes !== undefined ? (notes || null) : existing.notes, + nextBalanceDelta, + nextInterestDelta, + nextPaymentSource, + nextAutopayFailure, + req.params.id, + req.user.id, + ); + }); + applyEdit(); res.json(serializePayment(db.prepare(`SELECT p.* FROM payments p JOIN bills b ON b.id = p.bill_id WHERE p.id = ? AND p.${SQL_NOT_DELETED} AND b.user_id = ? AND b.deleted_at IS NULL`).get(req.params.id, req.user.id))); }); @@ -524,24 +548,27 @@ router.delete('/:id', (req, res) => { if (!payment) return res.status(404).json(standardizeError('Payment not found', 'NOT_FOUND', 'id')); if (isTransactionLinkedPayment(payment)) return rejectTransactionLinkedPayment(res); - // Reverse any balance delta that was stored when this payment was created. - // If this payment was the one that charged interest this month, clear + // Atomic: reverse the balance delta and soft-delete the payment together, so a + // failure can't leave the balance restored while the payment is still active + // (or vice versa). If this payment charged interest this month, clear // interest_accrued_month so the next payment can re-accrue correctly. - if (!payment.accounting_excluded && payment.balance_delta != null) { - const bill = db.prepare('SELECT current_balance FROM bills WHERE id = ? AND user_id = ? AND deleted_at IS NULL').get(payment.bill_id, req.user.id); - if (bill?.current_balance != null) { - const restored = Math.max(0, bill.current_balance - payment.balance_delta); - db.prepare(` - UPDATE bills - SET current_balance = ?, - interest_accrued_month = CASE WHEN ? THEN NULL ELSE interest_accrued_month END, - updated_at = datetime('now') - WHERE id = ? - `).run(restored, payment.interest_delta != null ? 1 : 0, payment.bill_id); + const softDeletePayment = db.transaction(() => { + if (!payment.accounting_excluded && payment.balance_delta != null) { + const bill = db.prepare('SELECT current_balance FROM bills WHERE id = ? AND user_id = ? AND deleted_at IS NULL').get(payment.bill_id, req.user.id); + if (bill?.current_balance != null) { + const restored = Math.max(0, bill.current_balance - payment.balance_delta); + db.prepare(` + UPDATE bills + SET current_balance = ?, + interest_accrued_month = CASE WHEN ? THEN NULL ELSE interest_accrued_month END, + updated_at = datetime('now') + WHERE id = ? + `).run(restored, payment.interest_delta != null ? 1 : 0, payment.bill_id); + } } - } - - db.prepare("UPDATE payments SET deleted_at = datetime('now') WHERE id = ? AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL)").run(req.params.id, req.user.id); + db.prepare("UPDATE payments SET deleted_at = datetime('now') WHERE id = ? AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL)").run(req.params.id, req.user.id); + }); + softDeletePayment(); res.json({ success: true }); }); @@ -552,25 +579,29 @@ router.post('/:id/restore', (req, res) => { if (!payment) return res.status(404).json(standardizeError('Deleted payment not found', 'NOT_FOUND', 'id')); if (isTransactionLinkedPayment(payment)) return rejectTransactionLinkedPayment(res); - // Re-apply the balance delta (undo the reversal done on delete). - // If this payment originally charged interest, restore interest_accrued_month - // to the month of the payment so future same-month payments skip interest. - if (!payment.accounting_excluded && payment.balance_delta != null) { - const bill = db.prepare('SELECT current_balance FROM bills WHERE id = ? AND user_id = ? AND deleted_at IS NULL').get(payment.bill_id, req.user.id); - if (bill?.current_balance != null) { - const reapplied = Math.max(0, bill.current_balance + payment.balance_delta); - const interestMonth = payment.interest_delta != null ? (payment.paid_date?.slice(0, 7) ?? null) : null; - db.prepare(` - UPDATE bills - SET current_balance = ?, - interest_accrued_month = CASE WHEN ? IS NOT NULL THEN ? ELSE interest_accrued_month END, - updated_at = datetime('now') - WHERE id = ? - `).run(reapplied, interestMonth, interestMonth, payment.bill_id); + // Atomic: re-apply the balance delta and un-delete the payment together, so a + // failure can't leave the balance re-applied while the payment stays deleted + // (or vice versa). If this payment originally charged interest, restore + // interest_accrued_month to the payment's month so future same-month payments + // skip interest. + const restorePayment = db.transaction(() => { + if (!payment.accounting_excluded && payment.balance_delta != null) { + const bill = db.prepare('SELECT current_balance FROM bills WHERE id = ? AND user_id = ? AND deleted_at IS NULL').get(payment.bill_id, req.user.id); + if (bill?.current_balance != null) { + const reapplied = Math.max(0, bill.current_balance + payment.balance_delta); + const interestMonth = payment.interest_delta != null ? (payment.paid_date?.slice(0, 7) ?? null) : null; + db.prepare(` + UPDATE bills + SET current_balance = ?, + interest_accrued_month = CASE WHEN ? IS NOT NULL THEN ? ELSE interest_accrued_month END, + updated_at = datetime('now') + WHERE id = ? + `).run(reapplied, interestMonth, interestMonth, payment.bill_id); + } } - } - - db.prepare('UPDATE payments SET deleted_at = NULL WHERE id = ? AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL)').run(req.params.id, req.user.id); + db.prepare('UPDATE payments SET deleted_at = NULL WHERE id = ? AND bill_id IN (SELECT id FROM bills WHERE user_id = ? AND deleted_at IS NULL)').run(req.params.id, req.user.id); + }); + restorePayment(); res.json(serializePayment(db.prepare(`SELECT p.* FROM payments p JOIN bills b ON b.id = p.bill_id WHERE p.id = ? AND p.${SQL_NOT_DELETED} AND b.user_id = ? AND b.deleted_at IS NULL`).get(req.params.id, req.user.id))); }); diff --git a/tests/paymentsRoute.test.js b/tests/paymentsRoute.test.js new file mode 100644 index 0000000..0678646 --- /dev/null +++ b/tests/paymentsRoute.test.js @@ -0,0 +1,100 @@ +'use strict'; + +// Track A — the manual create / delete / restore / edit payment paths must apply +// their balance mutation atomically with the row write (each is now wrapped in a +// single db.transaction()). These tests pin the *observable* invariant the wrap +// guarantees: the bill balance and the payment state stay consistent across a +// create → delete → restore → edit round-trip, and a double-restore is a no-op. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const fs = require('node:fs'); + +const dbPath = path.join(os.tmpdir(), `bill-tracker-payments-${process.pid}.sqlite`); +process.env.DB_PATH = dbPath; + +const { getDb, closeDb } = require('../db/database'); +const router = require('../routes/payments'); + +function handler(method, routePath) { + const layer = router.stack.find(l => l.route?.path === routePath && l.route.methods[method]); + assert.ok(layer, `${method.toUpperCase()} ${routePath} exists`); + return layer.route.stack[layer.route.stack.length - 1].handle; +} +function call(method, routePath, { userId, params = {}, body = {} } = {}) { + const h = handler(method, routePath); + return new Promise((resolve) => { + const req = { params, body, query: {}, user: { id: userId, role: 'user' } }; + const res = { + statusCode: 200, + status(c) { this.statusCode = c; return this; }, + json(d) { resolve({ status: this.statusCode, data: d }); }, + }; + h(req, res); + }); +} +function balanceOf(billId) { + return getDb().prepare('SELECT current_balance FROM bills WHERE id = ?').get(billId).current_balance; +} + +let userId, billId; +test.before(() => { + const db = getDb(); + userId = db.prepare("INSERT INTO users (username, password_hash, role, active) VALUES ('pay-user','x','user',1)").run().lastInsertRowid; + // $100 expected, $1,000 balance (cents), 0% interest so deltas are pure principal. + billId = db.prepare( + "INSERT INTO bills (user_id, name, due_day, expected_amount, current_balance, minimum_payment, interest_rate, active) VALUES (?, 'Loan', 1, 10000, 100000, 5000, 0, 1)", + ).run(userId).lastInsertRowid; +}); +test.after(() => { + closeDb(); + for (const s of ['', '-wal', '-shm']) { try { fs.unlinkSync(dbPath + s); } catch {} } +}); + +let paymentId; +test('manual POST /payments creates the payment (201) and drops the balance once', async () => { + const { status, data } = await call('post', '/', { + userId, body: { bill_id: billId, amount: 100, paid_date: '2026-07-01' }, + }); + assert.equal(status, 201); + assert.equal(data.amount, 100, 'serialized amount in dollars'); + paymentId = data.id; + assert.equal(balanceOf(billId), 90000, '1000.00 − 100.00 = 900.00 (cents)'); + const count = getDb().prepare('SELECT COUNT(*) c FROM payments WHERE bill_id = ? AND deleted_at IS NULL').get(billId).c; + assert.equal(count, 1); +}); + +test('DELETE reverses the balance and soft-deletes atomically', async () => { + const { status, data } = await call('delete', '/:id', { userId, params: { id: paymentId } }); + assert.equal(status, 200); + assert.equal(data.success, true); + assert.equal(balanceOf(billId), 100000, 'balance restored to 1000.00'); + const active = getDb().prepare('SELECT COUNT(*) c FROM payments WHERE bill_id = ? AND deleted_at IS NULL').get(billId).c; + assert.equal(active, 0, 'payment is soft-deleted'); +}); + +test('restore re-applies the balance drop atomically', async () => { + const { status } = await call('post', '/:id/restore', { userId, params: { id: paymentId } }); + assert.equal(status, 200); + assert.equal(balanceOf(billId), 90000, 'balance dropped to 900.00 again'); + const active = getDb().prepare('SELECT COUNT(*) c FROM payments WHERE bill_id = ? AND deleted_at IS NULL').get(billId).c; + assert.equal(active, 1, 'payment is active again'); +}); + +test('double-restore is a no-op (404, balance unchanged)', async () => { + const before = balanceOf(billId); + const { status } = await call('post', '/:id/restore', { userId, params: { id: paymentId } }); + assert.equal(status, 404, 'already-active payment cannot be restored again'); + assert.equal(balanceOf(billId), before, 'balance not double-applied'); +}); + +test('PUT edit reverses the old amount and applies the new one atomically', async () => { + const { status, data } = await call('put', '/:id', { + userId, params: { id: paymentId }, body: { amount: 50 }, + }); + assert.equal(status, 200); + assert.equal(data.amount, 50, 'amount updated to $50'); + // reverse $100 (→ 1000.00) then apply $50 (→ 950.00) + assert.equal(balanceOf(billId), 95000, '1000.00 − 50.00 = 950.00 (cents)'); +});