fix(storage): resolve a write when it commits, not when the request succeeds (#253)

`run()` resolved on `request.onsuccess`. In IndexedDB a request succeeding
and its transaction committing are separate events, in that order, so a
transaction that aborts at commit time -- quota exhaustion being the
realistic cause -- fired `onabort` after the promise had already settled,
where `reject()` is a no-op. The comment above it claimed to handle exactly
the case it did not.

Every write to this database goes through `run()`: `putDocument`, `putFile`,
`putBlob`, `deleteDocument`, `deleteFile`, `deleteBlob`. So a quota-aborted
save of a preset reported success and lost the data, and because `readJson`
in `app-storage.ts` swallows read errors into a fallback, the loss surfaced
later as a missing preset rather than as an error at save time.

Resolving on `transaction.oncomplete` is the durable signal, which is what
`updateDocument` already does. The result now has to be captured in
`onsuccess` and handed over at commit, since `request.result` is only valid
inside its own handler. Reads share `run()`, so they settle a tick later
too; every action it issues is a single request, so no caller is left
holding a transaction that has since committed.

#249 makes this worth fixing now rather than later: it added a RAW
conversion cache of up to 2 GB to the same origin, and it writes through
`putBlob`, so it both raises quota pressure and is subject to the same
false success.

Tests cover the abort paths, which had none.

Closes #250

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: adulbrich

src/lib/storage/kv-durable-writes.test.ts

@@ -0,0 +1,90 @@
+import "fake-indexeddb/auto";
+import { afterEach, describe, expect, it } from "@jest/globals";
+import { getDocument, putDocument, updateDocument } from "./kv";
+
+// What aborts a transaction at commit time in the field is quota exhaustion,
+// and there is no way to exhaust fake-indexeddb's quota. Aborting from a
+// listener on the successful request reproduces the ordering that matters:
+// the request succeeds, and the transaction goes away afterwards.
+const realPut = IDBObjectStore.prototype.put;
+
+// Aborting while the request is still pending is the other order: the abort
+// settles every in-flight request with an AbortError before the transaction
+// itself gives up, so the request never succeeds at all.
+function abortDuringNextPut(): void {
+  IDBObjectStore.prototype.put = function put(
+    this: IDBObjectStore,
+    ...args: Parameters<IDBObjectStore["put"]>
+  ) {
+    IDBObjectStore.prototype.put = realPut;
+    const request = realPut.apply(this, args);
+    this.transaction.abort();
+    return request;
+  };
+}
+
+function abortAfterNextPut(): void {
+  IDBObjectStore.prototype.put = function put(
+    this: IDBObjectStore,
+    ...args: Parameters<IDBObjectStore["put"]>
+  ) {
+    IDBObjectStore.prototype.put = realPut;
+    const request = realPut.apply(this, args);
+    request.addEventListener("success", () => {
+      request.transaction?.abort();
+    });
+    return request;
+  };
+}
+
+afterEach(() => {
+  IDBObjectStore.prototype.put = realPut;
+});
+
+describe("a write whose transaction aborts after the request succeeds", () => {
+  it("rejects rather than reporting the write as durable", async () => {
+    abortAfterNextPut();
+
+    await expect(
+      putDocument("aborted-preset", { name: "one" })
+    ).rejects.toThrow();
+  });
+
+  it("leaves nothing behind to read", async () => {
+    abortAfterNextPut();
+
+    await putDocument("rolled-back-preset", { name: "two" }).catch(
+      () => undefined
+    );
+
+    expect(await getDocument("rolled-back-preset")).toBeUndefined();
+  });
+
+  // `run`'s `onerror` path cannot be isolated from its `onabort` path through
+  // this API: an unprevented request error aborts its own transaction anyway,
+  // so both handlers fire and either one alone would reject. Removing
+  // `request.onerror` was checked, and this still passes. What it does pin is
+  // that a request which never succeeds settles the promise rather than
+  // leaving it pending forever -- the failure mode that resolving on
+  // `oncomplete` would introduce if the abort rejection went missing.
+  it("rejects rather than hanging when the abort lands mid-flight", async () => {
+    abortDuringNextPut();
+
+    await expect(
+      putDocument("abandoned-preset", { name: "three" })
+    ).rejects.toThrow();
+  });
+});
+
+describe("updateDocument", () => {
+  it("rejects when the transaction aborts after its put succeeds", async () => {
+    abortAfterNextPut();
+
+    await expect(
+      updateDocument<number[]>("aborted-counter", (current) => [
+        ...(current ?? []),
+        1,
+      ])
+    ).rejects.toThrow();
+  });
+});

src/lib/storage/kv.ts

@@ -161,12 +161,23 @@ function run<T>(
       new Promise<T>((resolve, reject) => {
         const transaction = database.transaction(store, mode);
         const request = action(transaction.objectStore(store));
-        request.onsuccess = () => resolve(request.result);
-        // Both are needed: a request can fail on its own, and a transaction
-        // can abort underneath a request that already succeeded (quota, for
-        // one), which would otherwise resolve a write that never landed.
+        let value: T;
+        request.onsuccess = () => {
+          value = request.result;
+        };
         request.onerror = () =>
           reject(request.error ?? new Error(`${store}: request failed`));
+        // Resolved on the transaction, not the request: a request succeeding
+        // and its transaction committing are separate events, in that order,
+        // and a quota abort lands between them. Resolving on the request would
+        // report a write that never landed, and the later `onabort` would be a
+        // no-op on an already-settled promise. `updateDocument` does the same.
+        //
+        // Reads go through here too, and that is deliberate rather than
+        // incidental: a readonly transaction commits straight after its last
+        // request, so waiting for it costs a tick and keeps one code path
+        // instead of two that differ in when they are allowed to settle.
+        transaction.oncomplete = () => resolve(value);
         transaction.onabort = () =>
           reject(
             transaction.error ?? new Error(`${store}: transaction aborted`)