chore(balance): post-merge cleanup of #182-#185 reviews (#187) #195

Merged
maximus merged 7 commits from issue-187-balance-cleanup-post-184-185 into main 2026-05-03 23:42:15 +00:00
2 changed files with 47 additions and 7 deletions
Showing only changes of commit 8c3a64d172 - Show all commits

View file

@ -92,10 +92,12 @@ describe("proposeStarterAccounts", () => {
it("inserts selected starters atomically and returns their ids", async () => { it("inserts selected starters atomically and returns their ids", async () => {
// BEGIN // BEGIN
mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); 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 mockSelect
.mockResolvedValueOnce([{ id: 11 }]) // cash category .mockResolvedValueOnce([{ id: 11 }]) // cash category lookup
.mockResolvedValueOnce([{ id: 13 }]); // rrsp category .mockResolvedValueOnce([{ count: 0 }]) // S3 collision check for cash
.mockResolvedValueOnce([{ id: 13 }]) // rrsp category lookup
.mockResolvedValueOnce([{ count: 0 }]); // S3 collision check for rrsp
mockExecute mockExecute
.mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 100 }) // INSERT cash .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 100 }) // INSERT cash
.mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 101 }) // INSERT rrsp .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); 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 () => { it("rolls back on insert failure", async () => {
mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // BEGIN 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.mockRejectedValueOnce(new Error("disk full"));
mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // ROLLBACK mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // ROLLBACK

View file

@ -509,9 +509,11 @@ export async function getStarterCollisions(): Promise<Set<string>> {
* in BEGIN/COMMIT on any failure ROLLBACK is issued and the original error * in BEGIN/COMMIT on any failure ROLLBACK is issued and the original error
* is re-thrown. Returns the inserted account ids in input order. * is re-thrown. Returns the inserted account ids in input order.
* *
* Callers MUST pre-filter `selectedKeys` against `getStarterCollisions()` so * Callers SHOULD pre-filter `selectedKeys` against `getStarterCollisions()`
* we never INSERT a duplicate (the table has no UNIQUE on (name, category), * to keep the UI honest, but each iteration ALSO re-checks for an existing
* so collisions would silently create dupes if not guarded upstream). * (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( export async function proposeStarterAccounts(
selectedKeys: string[] selectedKeys: string[]
@ -538,6 +540,18 @@ export async function proposeStarterAccounts(
`Seeded category '${starter.categoryKey}' missing — expected v9 schema` `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( const result = await db.execute(
`INSERT INTO balance_accounts (balance_category_id, name, currency, is_active) `INSERT INTO balance_accounts (balance_category_id, name, currency, is_active)
VALUES ($1, $2, 'CAD', 1)`, VALUES ($1, $2, 'CAD', 1)`,