From 8c3a64d17257c5b8913381e72f04f21298bfcfff Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:27:16 -0400 Subject: [PATCH] fix(balance): re-check collisions in-transaction in proposeStarterAccounts (S3) Adds defense-in-depth: each iteration runs a SELECT COUNT(*) WHERE name=? AND balance_category_id=? AND archived_at IS NULL inside the BEGIN/COMMIT block, immediately before its INSERT. On a hit, the iteration skips the INSERT silently and the returned ids array excludes the skipped starter. Rationale: balance_accounts has no UNIQUE constraint on (name, category) and the upstream pre-filter (getStarterCollisions) is best-effort. If a race or a bypass slips a duplicate through, the in-txn check catches it without surfacing a confusing error to the user. Existing two tests in StarterAccountsModal.test.tsx updated to mock the new SELECT call sequence; new test "skips silently when in-txn collision check finds an existing account" added. Suggestion S3 from PR #185 review (#187). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../balance/StarterAccountsModal.test.tsx | 34 ++++++++++++++++--- src/services/balance.service.ts | 20 +++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/components/balance/StarterAccountsModal.test.tsx b/src/components/balance/StarterAccountsModal.test.tsx index e0482dc..d56239c 100644 --- a/src/components/balance/StarterAccountsModal.test.tsx +++ b/src/components/balance/StarterAccountsModal.test.tsx @@ -92,10 +92,12 @@ describe("proposeStarterAccounts", () => { it("inserts selected starters atomically and returns their ids", async () => { // BEGIN mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); - // For each starter: SELECT id FROM balance_categories + INSERT + // For each starter: SELECT category id, SELECT in-txn collision check, INSERT mockSelect - .mockResolvedValueOnce([{ id: 11 }]) // cash category - .mockResolvedValueOnce([{ id: 13 }]); // rrsp category + .mockResolvedValueOnce([{ id: 11 }]) // cash category lookup + .mockResolvedValueOnce([{ count: 0 }]) // S3 collision check for cash + .mockResolvedValueOnce([{ id: 13 }]) // rrsp category lookup + .mockResolvedValueOnce([{ count: 0 }]); // S3 collision check for rrsp mockExecute .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 100 }) // INSERT cash .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 101 }) // INSERT rrsp @@ -110,9 +112,33 @@ describe("proposeStarterAccounts", () => { expect(sqls.filter((s) => /INSERT INTO balance_accounts/.test(s))).toHaveLength(2); }); + it("skips silently when in-txn collision check finds an existing account (S3)", async () => { + // BEGIN + mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); + // First starter "cash": category lookup succeeds, collision check returns count=1 → skip + mockSelect + .mockResolvedValueOnce([{ id: 11 }]) // cash category lookup + .mockResolvedValueOnce([{ count: 1 }]) // S3 collision: cash already exists + // Second starter "rrsp": category lookup + clean collision check + .mockResolvedValueOnce([{ id: 13 }]) // rrsp category lookup + .mockResolvedValueOnce([{ count: 0 }]); // rrsp clean + mockExecute + .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 101 }) // INSERT rrsp + .mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // COMMIT + + const result = await proposeStarterAccounts(["cash", "rrsp"]); + expect(result).toEqual([101]); // only rrsp inserted, cash skipped silently + + const sqls = mockExecute.mock.calls.map((c) => c[0]); + expect(sqls.filter((s) => /INSERT INTO balance_accounts/.test(s))).toHaveLength(1); + expect(sqls).toContain("COMMIT"); // no rollback — skip is normal flow + }); + it("rolls back on insert failure", async () => { mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // BEGIN - mockSelect.mockResolvedValueOnce([{ id: 11 }]); + mockSelect + .mockResolvedValueOnce([{ id: 11 }]) // cash category + .mockResolvedValueOnce([{ count: 0 }]); // S3 collision check clean mockExecute.mockRejectedValueOnce(new Error("disk full")); mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // ROLLBACK diff --git a/src/services/balance.service.ts b/src/services/balance.service.ts index 52c083a..6401ef0 100644 --- a/src/services/balance.service.ts +++ b/src/services/balance.service.ts @@ -509,9 +509,11 @@ export async function getStarterCollisions(): Promise> { * in BEGIN/COMMIT — on any failure ROLLBACK is issued and the original error * is re-thrown. Returns the inserted account ids in input order. * - * Callers MUST pre-filter `selectedKeys` against `getStarterCollisions()` so - * we never INSERT a duplicate (the table has no UNIQUE on (name, category), - * so collisions would silently create dupes if not guarded upstream). + * Callers SHOULD pre-filter `selectedKeys` against `getStarterCollisions()` + * to keep the UI honest, but each iteration ALSO re-checks for an existing + * (name, category) account inside the transaction and skips silently on a + * hit — a defense-in-depth guard since the table has no UNIQUE constraint + * on (name, balance_category_id). Returned ids exclude any skipped starter. */ export async function proposeStarterAccounts( selectedKeys: string[] @@ -538,6 +540,18 @@ export async function proposeStarterAccounts( `Seeded category '${starter.categoryKey}' missing — expected v9 schema` ); } + // Defense-in-depth: re-check collision in-txn before INSERT so we + // never create a silent duplicate even if the upstream pre-filter + // raced or was bypassed (S3 from PR #185 review). + const existing = await db.select<{ count: number }[]>( + `SELECT COUNT(*) AS count FROM balance_accounts + WHERE name = $1 AND balance_category_id = $2 + AND archived_at IS NULL`, + [starter.name, catRows[0].id] + ); + if ((existing[0]?.count ?? 0) > 0) { + continue; + } const result = await db.execute( `INSERT INTO balance_accounts (balance_category_id, name, currency, is_active) VALUES ($1, $2, 'CAD', 1)`,