From 30ff7ae3578c23cf009ebad5efeb799612bb0368 Mon Sep 17 00:00:00 2001 From: Oleh Omelchenko Date: Sun, 7 Jun 2026 20:03:47 +0300 Subject: [PATCH] =?UTF-8?q?Roll=20back=20import=20atomically=20on=20storag?= =?UTF-8?q?e-quota=20failure=20(M6,=20=C2=A708)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/architecture/02-persistence.md | 21 +++++ src/app/services/transfer.test.ts | 126 ++++++++++++++++++++++++++++ src/app/services/transfer.ts | 70 ++++++++++++++-- 3 files changed, 211 insertions(+), 6 deletions(-) diff --git a/docs/architecture/02-persistence.md b/docs/architecture/02-persistence.md index cc09233..6bc5494 100644 --- a/docs/architecture/02-persistence.md +++ b/docs/architecture/02-persistence.md @@ -426,6 +426,27 @@ user-fixable (storage full → next step, no diagnostic) from not (blocked stora explanation **+** a reportable `detail`); and a blocked store at startup **warns and runs in memory** rather than rejecting into the void. +### Multi-record writes (import): atomicity at the service boundary + +`db.ts` exposes only per-store request/transaction helpers — there is **no wrapper for a +single transaction spanning many records across stores**. So a bulk operation like **import** +(`services/transfer.ts`) cannot be truly atomic at the IDB layer; it achieves atomicity at +the **service boundary** instead: write the new records to IndexedDB first, tracking what +succeeded, and only on full success commit to the Zustand stores. On any write failure +(typically `QuotaExceededError`) it **rolls back best-effort** — deletes the records written +so far (`Promise.allSettled`) and removes any datasets already added to the store — so the +spec §08 "no partial import is committed" contract holds and the user gets an actionable +"storage full, delete and retry" message. + +> **Rule:** for any operation that persists multiple records, write-then-commit and roll +> back on failure — never mutate the in-memory stores before the writes are known to have +> landed (a half-merged workspace is worse than a failed import). +> **Limit:** rollback is best-effort, not transactional; if the rollback deletes themselves +> fail, orphan records can remain (invisible to the app — never added to a store — and +> cleaned up on the next successful write). True cross-record atomicity would require +> exposing a raw multi-store transaction from `db.ts`; defer that until a second multi-record +> writer needs it. + --- ## 7. Checklist for Adding a New Persisted Entity diff --git a/src/app/services/transfer.test.ts b/src/app/services/transfer.test.ts index 4c6448f..1d990f0 100644 --- a/src/app/services/transfer.test.ts +++ b/src/app/services/transfer.test.ts @@ -7,9 +7,23 @@ vi.mock('../infrastructure/file-transfer', () => ({ readTextFile: vi.fn(), })); +// The IDB adapter is mocked so tests are deterministic and don't rely on +// happy-dom's IndexedDB. saveSnippet and deleteSnippet are replaced with +// in-memory no-ops by default; individual tests override them to simulate +// quota failures. +vi.mock('../infrastructure/snippet-store', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + saveSnippet: vi.fn().mockResolvedValue(undefined), + deleteSnippet: vi.fn().mockResolvedValue(undefined), + }; +}); + import { createDataset } from '@core/dataset'; import { createSnippet } from '@core/snippet'; import { downloadJson, readTextFile } from '../infrastructure/file-transfer'; +import { deleteSnippet, saveSnippet, StorageQuotaError } from '../infrastructure/snippet-store'; import { useDatasetStore } from '../stores/DatasetStore'; import { useNotificationStore } from '../stores/NotificationStore'; import { useSnippetStore } from '../stores/SnippetStore'; @@ -17,6 +31,8 @@ import { exportWorkspace, importWorkspace } from './transfer'; const mockedDownload = vi.mocked(downloadJson); const mockedRead = vi.mocked(readTextFile); +const mockedSave = vi.mocked(saveSnippet); +const mockedDelete = vi.mocked(deleteSnippet); /** The most recent notification raised. */ function lastNote() { @@ -153,4 +169,114 @@ describe('importWorkspace', () => { expect(imported.id).not.toBe('s1'); expect(snippets.find((s) => s.name === 'Existing')!.id).toBe('s1'); }); + + describe('atomic rollback on storage-quota failure', () => { + it('leaves the snippet store and IDB unchanged when a quota error fires on the first write', async () => { + // Seed one pre-existing snippet. + useSnippetStore.getState().hydrate([createSnippet({ id: 'pre1', name: 'Pre-existing' })]); + // Every saveSnippet call throws a quota error. + mockedSave.mockRejectedValueOnce(new StorageQuotaError()); + + await importJson( + JSON.stringify({ + version: '1.0', + snippets: [ + { + id: 'imp1', + created: '2026-01-01T00:00:00.000Z', + name: 'Incoming', + spec: '{"mark":"bar"}', + }, + ], + }), + ); + + // The Zustand store must be unchanged — no partial import visible. + const snippets = useSnippetStore.getState().snippets; + expect(snippets).toHaveLength(1); + expect(snippets[0].id).toBe('pre1'); + + // deleteSnippet must NOT have been called (nothing was written before the + // first save threw, so there is nothing to clean up). + expect(mockedDelete).not.toHaveBeenCalled(); + }); + + it('rolls back IDB records already written before the failing call', async () => { + // saveSnippet succeeds for the first snippet, then throws on the second. + mockedSave.mockResolvedValueOnce(undefined); + mockedSave.mockRejectedValueOnce(new StorageQuotaError()); + + await importJson( + JSON.stringify({ + version: '1.0', + snippets: [ + { id: 'a', created: '2026-01-01T00:00:00.000Z', name: 'A', spec: '{"mark":"bar"}' }, + { id: 'b', created: '2026-01-01T00:00:00.000Z', name: 'B', spec: '{"mark":"point"}' }, + ], + }), + ); + + // The store must still be empty — addSnippets was never called. + expect(useSnippetStore.getState().snippets).toHaveLength(0); + + // deleteSnippet must be called once to clean up the record that was written + // before the second save failed (the first snippet's id). + expect(mockedDelete).toHaveBeenCalledTimes(1); + }); + + it('rolls back datasets added to the store before the snippet write failed', async () => { + // The import contains both a dataset and a snippet; the snippet write fails. + mockedSave.mockRejectedValueOnce(new StorageQuotaError()); + + await importJson( + JSON.stringify({ + version: '1.0', + snippets: [ + { id: 's1', created: '2026-01-01T00:00:00.000Z', name: 'S', spec: '{"mark":"bar"}' }, + ], + datasets: [{ id: 1, name: 'DS', data: [{ x: 1 }], format: 'json', source: 'inline' }], + }), + ); + + // Neither snippets nor datasets should persist in the store. + expect(useSnippetStore.getState().snippets).toHaveLength(0); + expect(useDatasetStore.getState().datasets).toHaveLength(0); + }); + + it('surfaces a quota error as a clear actionable notification without a detail field', async () => { + mockedSave.mockRejectedValueOnce(new StorageQuotaError()); + + await importJson( + JSON.stringify({ + version: '1.0', + snippets: [{ id: 'x', created: '2026-01-01T00:00:00.000Z', name: 'X', spec: '{}' }], + }), + ); + + const note = lastNote(); + expect(note.kind).toBe('error'); + expect(note.title).toContain('storage full'); + // Plain-language copy: names the problem and the fix, no error codes. + expect(note.message.toLowerCase()).toContain('delete'); + expect(note.message.toLowerCase()).toContain('try'); + // User-fixable failure: no diagnostic detail (nothing to report). + expect(note.detail).toBeUndefined(); + }); + + it('surfaces a non-quota storage error with a diagnostic detail', async () => { + mockedSave.mockRejectedValueOnce(new DOMException('disk I/O error', 'UnknownError')); + + await importJson( + JSON.stringify({ + version: '1.0', + snippets: [{ id: 'x', created: '2026-01-01T00:00:00.000Z', name: 'X', spec: '{}' }], + }), + ); + + const note = lastNote(); + expect(note.kind).toBe('error'); + expect(note.detail).toBeDefined(); + expect(note.detail).toContain('UnknownError'); + }); + }); }); diff --git a/src/app/services/transfer.ts b/src/app/services/transfer.ts index 59e43fa..d2b09b7 100644 --- a/src/app/services/transfer.ts +++ b/src/app/services/transfer.ts @@ -8,6 +8,14 @@ * Order matters on import (spec §08 → Merge): datasets are committed **before** * snippets so a snippet's by-name reference resolves against the just-added * dataset (whose name may have been auto-suffixed to avoid a clash). + * + * Atomicity (spec §08 → Storage limit handling / spec §10 → Non-destructive + * import): snippet writes are performed directly to IDB before touching the + * Zustand store. If any write fails the already-written IDB records are deleted + * and the just-added datasets are rolled back from the store, so neither the + * store nor IDB retains a partial import. The write-through subscriber sees + * store changes only after all IDB writes succeed; because `put` is idempotent + * the second (subscriber) write of each record is a harmless no-op. */ import { buildExportEnvelope, exportFilename, exportSummaryMessage } from '@core/export-envelope'; @@ -20,6 +28,7 @@ import { } from '@core/import-normalize'; import { snippetSizeBytes } from '@core/snippet'; import { downloadJson, readTextFile } from '../infrastructure/file-transfer'; +import { deleteSnippet, saveSnippet, StorageQuotaError } from '../infrastructure/snippet-store'; import { notify } from '../stores/NotificationStore'; import { useDatasetStore } from '../stores/DatasetStore'; import { useSnippetStore } from '../stores/SnippetStore'; @@ -122,6 +131,9 @@ export async function importWorkspace(file: File): Promise { const finalSnippets = reassignCollidingSnippetIds(existingSnippetIds, renamedSnippets); // Commit datasets BEFORE snippets so by-name references resolve (spec §08). + // Record which dataset ids existed before the add so we can roll them back if + // the subsequent snippet writes fail. + const datasetIdsBefore = new Set(useDatasetStore.getState().datasets.map((d) => d.id)); useDatasetStore.getState().addDatasets(dedupedDatasets); // Storage budget pre-check (spec §08 → Storage limit handling): warn on overage @@ -132,12 +144,58 @@ export async function importWorkspace(file: File): Promise { const incomingBytes = finalSnippets.reduce((n, s) => n + snippetSizeBytes(s), 0); const overage = existingBytes + incomingBytes - SNIPPET_BUDGET_BYTES; - // TODO (spec §08 → Storage limit handling): a *hard* quota failure on save - // should commit no partial snippet import. The write-through subscriber - // currently surfaces such a failure as a per-record error toast, but the - // in-memory store keeps the records (they vanish on reload). True atomic - // rollback needs a write-before-store import path — a persistence-architecture - // change deferred until the M6 storage monitor (web.dev quota estimate) lands. + // Atomic snippet write (spec §08 → Storage limit handling / spec §10 → + // Non-destructive import): write every snippet to IDB before updating the + // Zustand store. A quota failure mid-write is caught here, the already-written + // records are deleted, the just-added datasets are removed from the store, and + // we surface a clear actionable error — leaving the workspace exactly as before. + // After all writes succeed, `addSnippets` makes them visible in the store; the + // write-through subscriber re-runs `saveSnippet` for each, but `put` is + // idempotent so those second writes are harmless. + const writtenSnippetIds: string[] = []; + try { + for (const snippet of finalSnippets) { + await saveSnippet(snippet); + writtenSnippetIds.push(snippet.id); + } + } catch (err) { + // Roll back: delete every IDB record we already wrote. + await Promise.allSettled(writtenSnippetIds.map((id) => deleteSnippet(id))); + + // Roll back: remove the datasets we just added to the store so neither + // store nor IDB retains any trace of this import. + const addedDatasets = useDatasetStore + .getState() + .datasets.filter((d) => !datasetIdsBefore.has(d.id)); + for (const d of addedDatasets) { + useDatasetStore.getState().remove(d.id); + } + + // Surface a clear, actionable error (spec §08 "Quota failure"; NN/g #9 / + // GOV.UK plain language — no codes, tell the user what to do next). + if (err instanceof StorageQuotaError) { + notify({ + kind: 'error', + title: 'Import failed — storage full', + message: + 'The import could not be saved because snippet storage is full. ' + + 'Delete snippets you no longer need to free up space, then try importing again.', + }); + } else { + notify({ + kind: 'error', + title: 'Import failed', + message: + 'A storage error prevented the import from completing. The workspace was not changed. ' + + 'If this keeps happening, your browser may be blocking local storage.', + detail: + err instanceof Error ? `Snippet save failed: ${err.name}: ${err.message}` : String(err), + }); + } + return; + } + + // All IDB writes succeeded — now make the snippets visible in the store. useSnippetStore.getState().addSnippets(finalSnippets); // Feedback (spec §08 → Feedback): one summary toast; a warning when datasets were