From 50b119121f33df69312039dcfb685063843eab04 Mon Sep 17 00:00:00 2001 From: le king fu Date: Fri, 1 May 2026 07:33:44 -0400 Subject: [PATCH] fix(balance): atomic snapshot save with BEGIN/COMMIT + cleanup migration useSnapshotEditor.save now validates all simple/priced lines in-memory before any DB write, then delegates to a new saveSnapshotAtomic helper that wraps INSERT snapshot + INSERT lines in an explicit BEGIN/COMMIT transaction (ROLLBACK on catch). Pattern matches categorizationService. Migration v11 cleans existing orphan snapshots in profiles that hit the old race; new orphans are no longer possible thanks to the transaction. Resolves #176 --- CHANGELOG.fr.md | 1 + CHANGELOG.md | 1 + src-tauri/src/lib.rs | 120 ++++++++++++++++++++++ src/hooks/useSnapshotEditor.ts | 55 +++++----- src/services/balance.service.test.ts | 148 +++++++++++++++++++++++++++ src/services/balance.service.ts | 118 +++++++++++++++++++++ 6 files changed, 417 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 949a5bd..0043bd4 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -5,6 +5,7 @@ ### Corrigé - Bilan : correction de l'erreur SQLite « misuse of aggregate function MIN() » au chargement de /balance avec des snapshots existants ; remplacement du pattern aggregate-in-WHERE par une window function ROW_NUMBER() dans getAccountsPeriodAnchor (#175). +- Bilan : la sauvegarde d'un snapshot utilise désormais une transaction atomique BEGIN/COMMIT et valide toutes les lignes avant toute écriture en BDD, empêchant les snapshots orphelins lorsque la validation échoue. La migration v11 nettoie les orphelins existants (#176). ## [0.9.0] - 2026-04-29 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0302bee..e7c0c29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Bilan: fix SQLite "misuse of aggregate function MIN()" error when loading /balance with existing snapshots; replaced aggregate-in-WHERE pattern with ROW_NUMBER() window function in getAccountsPeriodAnchor (#175). +- Bilan: snapshot save now uses atomic BEGIN/COMMIT and validates all lines before any DB write, preventing orphan snapshot rows when validation fails. Migration v11 cleans existing orphans (#176). ## [0.9.0] - 2026-04-29 diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 2466b1e..42c809e 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -127,6 +127,22 @@ pub fn run() { WHERE key = 'crypto' AND is_seed = 1;", kind: MigrationKind::Up, }, + // Migration v11 — cleanup orphan balance snapshots (#176). Before + // useSnapshotEditor.save was made atomic via BEGIN/COMMIT, a + // priced-line validation failure could leave the snapshot row + // inserted but with no lines, blocking subsequent saves at that + // date through the snapshot_date UNIQUE constraint. This deletes + // any such orphan rows from existing profiles. New orphans are + // no longer possible thanks to saveSnapshotAtomic. + Migration { + version: 11, + description: "cleanup orphan balance snapshots", + sql: "DELETE FROM balance_snapshots \ + WHERE NOT EXISTS ( \ + SELECT 1 FROM balance_snapshot_lines \ + WHERE snapshot_id = balance_snapshots.id);", + kind: MigrationKind::Up, + }, ]; tauri::Builder::default() @@ -1176,5 +1192,109 @@ mod tests { "CHECK should reject asset_type values outside ('stock','crypto')" ); } + + // ========================================================================= + // Migration v11 — cleanup orphan balance snapshots (#176) + // ------------------------------------------------------------------------- + // Validates that the v11 SQL deletes snapshot rows that have no associated + // lines (left behind by the pre-#176 race) while preserving rows that have + // at least one line. Statement-equivalent to the production migration. + // ========================================================================= + + /// Production v11 SQL — kept in sync with the Migration { version: 11 } + /// entry above. + const V11_SQL: &str = "DELETE FROM balance_snapshots \ + WHERE NOT EXISTS ( \ + SELECT 1 FROM balance_snapshot_lines \ + WHERE snapshot_id = balance_snapshots.id);"; + + #[test] + fn migration_v11_deletes_orphan_snapshots() { + let conn = fresh_db(); + conn.execute_batch(V10_SQL).expect("apply v10"); + + // Seed an orphan: snapshot with NO lines. + conn.execute( + "INSERT INTO balance_snapshots (snapshot_date) VALUES ('2026-01-15')", + [], + ) + .unwrap(); + let orphan_id: i64 = conn + .query_row("SELECT last_insert_rowid()", [], |r| r.get(0)) + .unwrap(); + + // Seed a healthy snapshot with one line — needs an account first. + // Use the seeded `cash` simple category from v9. + let cash_cat_id: i64 = conn + .query_row( + "SELECT id FROM balance_categories WHERE key = 'cash'", + [], + |r| r.get(0), + ) + .unwrap(); + conn.execute( + "INSERT INTO balance_accounts (balance_category_id, name) VALUES (?1, 'Test')", + [cash_cat_id], + ) + .unwrap(); + let acc_id: i64 = conn + .query_row("SELECT last_insert_rowid()", [], |r| r.get(0)) + .unwrap(); + conn.execute( + "INSERT INTO balance_snapshots (snapshot_date) VALUES ('2026-02-15')", + [], + ) + .unwrap(); + let healthy_id: i64 = conn + .query_row("SELECT last_insert_rowid()", [], |r| r.get(0)) + .unwrap(); + conn.execute( + "INSERT INTO balance_snapshot_lines \ + (snapshot_id, account_id, value, price_source) \ + VALUES (?1, ?2, 100.0, 'manual')", + [healthy_id, acc_id], + ) + .unwrap(); + + // Pre-conditions. + let pre_count: i64 = conn + .query_row("SELECT COUNT(*) FROM balance_snapshots", [], |r| r.get(0)) + .unwrap(); + assert_eq!(pre_count, 2); + + // Apply v11. + conn.execute_batch(V11_SQL).expect("apply v11"); + + // Orphan gone, healthy preserved. + let post_count: i64 = conn + .query_row("SELECT COUNT(*) FROM balance_snapshots", [], |r| r.get(0)) + .unwrap(); + assert_eq!(post_count, 1, "v11 should delete only the orphan"); + let surviving_id: i64 = conn + .query_row("SELECT id FROM balance_snapshots", [], |r| r.get(0)) + .unwrap(); + assert_eq!(surviving_id, healthy_id); + // And ensure the orphan id is gone. + let still_orphan: i64 = conn + .query_row( + "SELECT COUNT(*) FROM balance_snapshots WHERE id = ?1", + [orphan_id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(still_orphan, 0); + } + + #[test] + fn migration_v11_is_idempotent_on_clean_db() { + let conn = fresh_db(); + conn.execute_batch(V10_SQL).expect("apply v10"); + // Empty balance_snapshots — running v11 should be a no-op. + conn.execute_batch(V11_SQL).expect("apply v11"); + let count: i64 = conn + .query_row("SELECT COUNT(*) FROM balance_snapshots", [], |r| r.get(0)) + .unwrap(); + assert_eq!(count, 0); + } } diff --git a/src/hooks/useSnapshotEditor.ts b/src/hooks/useSnapshotEditor.ts index adbf801..a4c36c5 100644 --- a/src/hooks/useSnapshotEditor.ts +++ b/src/hooks/useSnapshotEditor.ts @@ -28,10 +28,9 @@ import { listBalanceAccounts, listBalanceCategories, getSnapshotByDate, - createSnapshot, deleteSnapshot, listLinesBySnapshot, - upsertSnapshotLines, + saveSnapshotAtomic, getPreviousSnapshot, BalanceServiceError, } from "../services/balance.service"; @@ -412,35 +411,32 @@ export function useSnapshotEditor(options: Options = {}) { }, [state.previousLines, state.accounts]); /** - * Persist the editor state to the database. - * - 'new' mode: create the snapshot row (UNIQUE per date), then upsert - * its lines. If creation fails because a snapshot was created at this - * same date concurrently (snapshot_date_taken), the page is expected - * to redirect to edit mode. - * - 'edit' mode: upsert lines on the existing snapshot. + * Persist the editor state to the database (#176 — atomic). + * + * Order of operations: + * 1. Build & validate `simpleLines` and `pricedLines` arrays from + * editor state. Any input parsing error throws BEFORE any DB + * mutation happens, so an invalid form never produces an orphan + * snapshot row. + * 2. Call `saveSnapshotAtomic` which wraps `INSERT INTO + * balance_snapshots` (new mode) and the line rewrite in a single + * `BEGIN/COMMIT/ROLLBACK` transaction. + * + * Modes: + * - 'new' mode: atomic helper inserts the snapshot row and its lines. + * - 'edit' mode: only the lines get rewritten on the existing snapshot. * * Only accounts with a non-empty value (after trim) are persisted; empty * fields mean "no entry for this account at this date" — they're cleared - * by the rewrite-all strategy in `upsertSnapshotLines`. + * by the rewrite-all strategy in `saveSnapshotAtomic`. */ const save = useCallback(async (): Promise<{ snapshotId: number }> => { dispatch({ type: "SET_SAVING", payload: true }); dispatch({ type: "SET_ERROR", payload: { message: null, code: null } }); try { - let snapshotId: number; - if (state.mode === "edit" && state.snapshot) { - snapshotId = state.snapshot.id; - } else { - snapshotId = await createSnapshot({ - snapshot_date: state.snapshotDate, - }); - } - // Index account kinds for line classification at save time. - const kindByAccountId = new Map(); - for (const acc of state.accounts) { - kindByAccountId.set(acc.id, acc.category_kind); - } - // Simple-kind lines: drop empty fields, accept any finite number. + // Step 1 — build & validate every line in memory. THROW HERE means + // no DB mutation has happened yet, so no orphan snapshot can be + // left behind by a validation failure (#176). const simpleLines = Object.entries(state.values) .filter(([, v]) => v !== undefined && String(v).trim().length > 0) .map(([accountIdStr, raw]) => { @@ -459,7 +455,6 @@ export function useSnapshotEditor(options: Options = {}) { account_kind: "simple" as const, }; }); - // Priced-kind lines: both qty + price required, value computed. const pricedLines = Object.entries(state.pricedValues) .filter( ([, entry]) => @@ -495,7 +490,16 @@ export function useSnapshotEditor(options: Options = {}) { value: qty * price, }; }); - await upsertSnapshotLines(snapshotId, [...simpleLines, ...pricedLines]); + + // Step 2 — atomic write. BEGIN / INSERT snapshot (if 'new') / + // INSERT lines / COMMIT, with ROLLBACK on any failure. + const existingSnapshotId = + state.mode === "edit" && state.snapshot ? state.snapshot.id : null; + const { snapshotId } = await saveSnapshotAtomic({ + existingSnapshotId, + snapshot_date: state.snapshotDate, + lines: [...simpleLines, ...pricedLines], + }); dispatch({ type: "CLEAR_DIRTY" }); // Reload so 'new' mode flips to 'edit' and the snapshot row is in state. await loadForDate(state.snapshotDate); @@ -512,7 +516,6 @@ export function useSnapshotEditor(options: Options = {}) { state.snapshotDate, state.values, state.pricedValues, - state.accounts, loadForDate, ]); diff --git a/src/services/balance.service.test.ts b/src/services/balance.service.test.ts index d9a78d2..5b0fd83 100644 --- a/src/services/balance.service.test.ts +++ b/src/services/balance.service.test.ts @@ -32,6 +32,7 @@ import { deleteSnapshot, listLinesBySnapshot, upsertSnapshotLines, + saveSnapshotAtomic, getPreviousSnapshot, validateLineKindInvariants, PRICED_VALUE_TOLERANCE, @@ -908,6 +909,153 @@ describe("upsertSnapshotLines — priced kind", () => { }); }); +// ----------------------------------------------------------------------------- +// saveSnapshotAtomic (#176) — atomic BEGIN/COMMIT/ROLLBACK orchestration +// ----------------------------------------------------------------------------- + +describe("saveSnapshotAtomic — new mode", () => { + it("issues BEGIN before any write and COMMIT once everything succeeds", async () => { + // Order: SELECT dup-check → INSERT snapshot → DELETE lines → INSERT line → UPDATE → COMMIT + mockSelect.mockResolvedValueOnce([]); // no duplicate + mockExecute + .mockResolvedValueOnce({ rowsAffected: 0 }) // BEGIN + .mockResolvedValueOnce({ lastInsertId: 42, rowsAffected: 1 }) // INSERT snapshot + .mockResolvedValueOnce({ rowsAffected: 0 }) // DELETE lines + .mockResolvedValueOnce({ lastInsertId: 100, rowsAffected: 1 }) // INSERT line + .mockResolvedValueOnce({ rowsAffected: 1 }) // UPDATE updated_at + .mockResolvedValueOnce({ rowsAffected: 0 }); // COMMIT + + const res = await saveSnapshotAtomic({ + existingSnapshotId: null, + snapshot_date: "2026-04-30", + lines: [{ account_id: 1, value: 1000 }], + }); + + expect(res.snapshotId).toBe(42); + // First execute is BEGIN + expect(mockExecute.mock.calls[0][0]).toBe("BEGIN"); + // INSERT snapshot is second + expect(mockExecute.mock.calls[1][0]).toContain( + "INSERT INTO balance_snapshots" + ); + // DELETE lines, INSERT line, UPDATE updated_at all happen between BEGIN and COMMIT + expect(mockExecute.mock.calls[2][0]).toContain( + "DELETE FROM balance_snapshot_lines" + ); + expect(mockExecute.mock.calls[3][0]).toContain( + "INSERT INTO balance_snapshot_lines" + ); + expect(mockExecute.mock.calls[4][0]).toContain("UPDATE balance_snapshots"); + // Last execute is COMMIT + expect(mockExecute.mock.calls[mockExecute.mock.calls.length - 1][0]).toBe( + "COMMIT" + ); + // No ROLLBACK on success + expect( + mockExecute.mock.calls.some((c: unknown[]) => c[0] === "ROLLBACK") + ).toBe(false); + }); + + it("rejects when a snapshot already exists at this date (snapshot_date_taken) and ROLLBACKs", async () => { + mockSelect.mockResolvedValueOnce([{ id: 7 }]); // duplicate found + mockExecute + .mockResolvedValueOnce({ rowsAffected: 0 }) // BEGIN + .mockResolvedValueOnce({ rowsAffected: 0 }); // ROLLBACK + + await expect( + saveSnapshotAtomic({ + existingSnapshotId: null, + snapshot_date: "2026-04-30", + lines: [{ account_id: 1, value: 1000 }], + }) + ).rejects.toMatchObject({ code: "snapshot_date_taken" }); + + // BEGIN ran, then ROLLBACK because the duplicate threw mid-transaction. + expect(mockExecute.mock.calls[0][0]).toBe("BEGIN"); + expect(mockExecute.mock.calls[1][0]).toBe("ROLLBACK"); + // No INSERT INTO balance_snapshots happened. + expect( + mockExecute.mock.calls.some((c: unknown[]) => + String(c[0]).includes("INSERT INTO balance_snapshots") + ) + ).toBe(false); + }); + + it("ROLLBACKs and re-throws when a line INSERT fails (no orphan snapshot persists)", async () => { + mockSelect.mockResolvedValueOnce([]); // no duplicate + mockExecute + .mockResolvedValueOnce({ rowsAffected: 0 }) // BEGIN + .mockResolvedValueOnce({ lastInsertId: 42, rowsAffected: 1 }) // INSERT snapshot + .mockResolvedValueOnce({ rowsAffected: 0 }) // DELETE lines + .mockRejectedValueOnce(new Error("simulated FK violation")) // INSERT line fails + .mockResolvedValueOnce({ rowsAffected: 0 }); // ROLLBACK + + await expect( + saveSnapshotAtomic({ + existingSnapshotId: null, + snapshot_date: "2026-04-30", + lines: [{ account_id: 999, value: 1000 }], + }) + ).rejects.toThrow("simulated FK violation"); + + // BEGIN happened, ROLLBACK was the last call — no COMMIT. + expect(mockExecute.mock.calls[0][0]).toBe("BEGIN"); + expect( + mockExecute.mock.calls[mockExecute.mock.calls.length - 1][0] + ).toBe("ROLLBACK"); + expect( + mockExecute.mock.calls.some((c: unknown[]) => c[0] === "COMMIT") + ).toBe(false); + }); + + it("rejects validation failures BEFORE BEGIN — no transaction is opened", async () => { + await expect( + saveSnapshotAtomic({ + existingSnapshotId: null, + snapshot_date: "2026-04-30", + // Priced line missing quantity should fail validation before any DB write. + lines: [ + { account_id: 1, value: 100, account_kind: "priced", unit_price: 10 }, + ], + }) + ).rejects.toMatchObject({ code: "snapshot_priced_quantity_required" }); + // Pre-DB validation: no BEGIN, no SELECT, no execute at all. + expect(mockExecute).not.toHaveBeenCalled(); + expect(mockSelect).not.toHaveBeenCalled(); + }); +}); + +describe("saveSnapshotAtomic — edit mode", () => { + it("skips INSERT INTO balance_snapshots when existingSnapshotId is provided", async () => { + mockExecute + .mockResolvedValueOnce({ rowsAffected: 0 }) // BEGIN + .mockResolvedValueOnce({ rowsAffected: 0 }) // DELETE lines + .mockResolvedValueOnce({ lastInsertId: 100, rowsAffected: 1 }) // INSERT line + .mockResolvedValueOnce({ rowsAffected: 1 }) // UPDATE updated_at + .mockResolvedValueOnce({ rowsAffected: 0 }); // COMMIT + + const res = await saveSnapshotAtomic({ + existingSnapshotId: 5, + snapshot_date: "2026-04-30", + lines: [{ account_id: 1, value: 1000 }], + }); + + expect(res.snapshotId).toBe(5); + // No SELECT (no duplicate check in edit mode), no INSERT INTO balance_snapshots. + expect(mockSelect).not.toHaveBeenCalled(); + expect( + mockExecute.mock.calls.some((c: unknown[]) => + String(c[0]).includes("INSERT INTO balance_snapshots") + ) + ).toBe(false); + // BEGIN / DELETE / INSERT line / UPDATE / COMMIT + expect(mockExecute.mock.calls[0][0]).toBe("BEGIN"); + expect(mockExecute.mock.calls[mockExecute.mock.calls.length - 1][0]).toBe( + "COMMIT" + ); + }); +}); + // ----------------------------------------------------------------------------- // Time-series aggregators (Issue #141 / Bilan #3) // ----------------------------------------------------------------------------- diff --git a/src/services/balance.service.ts b/src/services/balance.service.ts index 8052481..e63bd85 100644 --- a/src/services/balance.service.ts +++ b/src/services/balance.service.ts @@ -768,6 +768,124 @@ export async function upsertSnapshotLines( ); } +/** + * Atomic snapshot save (#176). Wraps `INSERT INTO balance_snapshots` and + * the line writes in a single explicit BEGIN/COMMIT transaction so a + * failure during line validation or insertion never leaves an orphan + * snapshot row behind (which used to wedge subsequent saves at the same + * date through the `snapshot_date_taken` UNIQUE constraint). + * + * Caller contract: + * - All `lines` MUST already be validated by the caller — this function + * does NOT translate string inputs to numbers; it expects the same + * `SnapshotLineInput` shape that `upsertSnapshotLines` accepts. + * - The caller passes `existingSnapshotId` for edit-mode (no INSERT + * happens, only the line rewrite). For new-mode pass `null` and a + * `snapshot_date`; this function handles both cases inside the same + * transaction. + * + * On any error, ROLLBACK is issued and the original error is re-thrown. + * If ROLLBACK itself fails (e.g. transaction never opened), that error is + * swallowed and the original is preserved — the caller never sees a + * misleading rollback error. + */ +export async function saveSnapshotAtomic(input: { + existingSnapshotId: number | null; + snapshot_date: string; + notes?: string | null; + lines: SnapshotLineInput[]; +}): Promise<{ snapshotId: number }> { + // Validate every line ahead of time so the transaction never opens for + // a doomed save. Mirrors `upsertSnapshotLines` invariants. + for (const line of input.lines) { + validateLineKindInvariants(line); + } + + const db = await getDb(); + let inTxn = false; + try { + await db.execute("BEGIN"); + inTxn = true; + + let snapshotId: number; + if (input.existingSnapshotId !== null) { + snapshotId = input.existingSnapshotId; + } else { + const date = normalizeSnapshotDate(input.snapshot_date); + // Date collision check inside the transaction so a concurrent + // insert can't sneak between the SELECT and the INSERT. + const dup = await db.select>( + `SELECT id FROM balance_snapshots WHERE snapshot_date = $1`, + [date] + ); + if (dup.length > 0) { + throw new BalanceServiceError( + "snapshot_date_taken", + `A snapshot already exists at ${date}` + ); + } + const insRes = await db.execute( + `INSERT INTO balance_snapshots (snapshot_date, notes) + VALUES ($1, $2)`, + [date, input.notes ? input.notes.trim() || null : null] + ); + snapshotId = insRes.lastInsertId as number; + } + + // Rewrite-all strategy (matches `upsertSnapshotLines`): clear + // existing lines, then re-insert every line. Cheap because snapshot + // line counts are small. + await db.execute( + "DELETE FROM balance_snapshot_lines WHERE snapshot_id = $1", + [snapshotId] + ); + for (const line of input.lines) { + const kind = line.account_kind ?? "simple"; + if (kind === "simple") { + await db.execute( + `INSERT INTO balance_snapshot_lines + (snapshot_id, account_id, quantity, unit_price, value, price_source) + VALUES ($1, $2, NULL, NULL, $3, 'manual')`, + [snapshotId, line.account_id, line.value] + ); + } else { + await db.execute( + `INSERT INTO balance_snapshot_lines + (snapshot_id, account_id, quantity, unit_price, value, price_source) + VALUES ($1, $2, $3, $4, $5, 'manual')`, + [ + snapshotId, + line.account_id, + line.quantity, + line.unit_price, + line.value, + ] + ); + } + } + await db.execute( + `UPDATE balance_snapshots + SET updated_at = CURRENT_TIMESTAMP + WHERE id = $1`, + [snapshotId] + ); + + await db.execute("COMMIT"); + inTxn = false; + return { snapshotId }; + } catch (e) { + if (inTxn) { + try { + await db.execute("ROLLBACK"); + } catch { + // Defensive: if ROLLBACK fails we still want the caller to see + // the original error, not the rollback error. + } + } + throw e; + } +} + /** * Convenience helper used by the "Prefill from previous snapshot" button. * Returns the snapshot whose `snapshot_date` is strictly earlier than