Reject incomplete OPFS transfers in the synchronous store

CHANGELOG.md

@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 - **Security — opened notebooks:** an opened `.ipynb` file can no longer change the look of DataLab-Web, cover it or contact a remote server. Its markdown cells and its HTML and SVG outputs are now displayed in their own isolated block: their styles no longer apply to the rest of the interface, and content positioned over the whole window stays inside the block, so it cannot hide commands or imitate a DataLab-Web dialog. Form fields are removed, and nothing is loaded from the network (remote images, style sheets, fonts or backgrounds), which prevents a notebook from tracking its opening. Images embedded in the notebook and the tables and styling of Sigima results are displayed as before.
 - **Unsaved changes:** several edits did not mark the workspace as modified, so closing or reloading the page discarded them without warning: editing signal or image values in the _Data_ array editor, duplicating objects, moving an object up or down, pasting or importing a region of interest, changing the resampling method of an image, plotting analysis results, committing an interactive fit, importing nodes from an HDF5 file and running a plugin menu action. They now mark the workspace as modified, like the other edits.
 - **On-disk storage — failed writes:** when the browser refused to write an object to disk (storage quota exceeded, write error), the object lost its data: it had already been removed from memory. The object now stays in memory with its data, and the error is reported. An interrupted write no longer replaces the previous on-disk copy either.
+- **On-disk storage — incomplete transfers:** an object whose data the browser wrote to disk only partly was considered stored and removed from memory, so that it was later read back with missing values; a partial read also gave missing values without error. Incomplete writes now fail like other write errors, so that the object keeps its data in memory, and incomplete reads are reported as errors.
 - **On-disk storage — several tabs:** with DataLab-Web open in two tabs in on-disk storage mode, switching one tab back to memory storage or replacing its workspace deleted the data stored on disk by the other tab, whose objects could then no longer be displayed or processed. Each tab now keeps its data in its own storage area; the areas of closed tabs are reclaimed when the on-disk mode is next enabled.
 - **HDF5 import — complex and integer data:** single-precision complex signals (`complex64`) lost their imaginary part, and unsigned 32-bit images were clipped to 2 147 483 647. Complex data is now imported as complex objects and integer data keeps its values exactly (images with large values become 64-bit floating-point images); a warning now reports integers beyond 2⁵³, which cannot be represented exactly. Previews show the real part of complex data.
 - **HDF5 browser — broken links:** files containing a link to a missing object or to a missing file (e.g. synchrotron master files without their detector files) could not be opened at all. Broken links are now skipped and the rest of the file can be browsed and imported.

src/storage/opfsSyncObjectStore.ts

@@ -99,13 +99,28 @@ export class OpfsSyncObjectStore implements ObjectByteStore {
     return fh.createSyncAccessHandle();
   }
 
-  /** Persist *bytes* for *oid*, overwriting any previous payload in place. */
+  /**
+   * Persist *bytes* for *oid*, overwriting any previous payload in place.
+   *
+   * A write may transfer fewer bytes than requested: the remaining bytes are
+   * written in turn, and a write that transfers nothing throws, so that the
+   * caller never takes an incomplete copy for a stored payload.
+   */
   async put(oid: string, bytes: Uint8Array): Promise<void> {
     const h = await this.handle(oid);
     try {
       // In-place write, then shrink the file to the exact payload length so
       // a smaller re-spill never leaves stale trailing bytes.
-      h.write(bytes, { at: 0 });
+      let written = 0;
+      while (written < bytes.byteLength) {
+        const count = h.write(bytes.subarray(written), { at: written });
+        if (!(count > 0)) {
+          throw new Error(
+            `Incomplete OPFS write for ${oid}: ${written} of ${bytes.byteLength} bytes`,
+          );
+        }
+        written += count;
+      }
       h.truncate(bytes.byteLength);
       h.flush();
     } finally {
@@ -114,13 +129,26 @@ export class OpfsSyncObjectStore implements ObjectByteStore {
     this.sizes.set(oid, bytes.byteLength);
   }
 
-  /** Read back the payload for *oid*. Throws if it is absent. */
+  /**
+   * Read back the payload for *oid*. Throws if it is absent, or if fewer
+   * bytes than the file size can be read (a read may transfer fewer bytes
+   * than requested: the remaining bytes are read in turn).
+   */
   async get(oid: string): Promise<Uint8Array> {
     const h = await this.handle(oid);
     try {
       const size = h.getSize();
       const out = new Uint8Array(size);
-      h.read(out, { at: 0 });
+      let read = 0;
+      while (read < size) {
+        const count = h.read(out.subarray(read), { at: read });
+        if (!(count > 0)) {
+          throw new Error(
+            `Incomplete OPFS read for ${oid}: ${read} of ${size} bytes`,
+          );
+        }
+        read += count;
+      }
       return out;
     } finally {
       h.close();

tests/ts/runtime/diskSpillRollback.test.ts

@@ -103,6 +103,25 @@ describe("disk spill — a failed OPFS write never loses the data", () => {
     expect(store.delete).toHaveBeenCalledWith("A");
   });
 
+  it("attaches the bytes back when only part of them could be written", async () => {
+    // The synchronous OPFS store rejects a write transferring fewer bytes than
+    // requested, instead of reporting the object as stored.
+    const incomplete = async () => {
+      throw new Error("Incomplete OPFS write for A: 2 of 4 bytes");
+    };
+    const { runtime, py, attachCalls } = makeRuntime(incomplete);
+
+    await expect(runtime.spillDirect("A")).rejects.toThrow(
+      "Incomplete OPFS write",
+    );
+
+    expect(attachCalls()).toEqual([
+      { oid: "A", payload: PAYLOAD, on_disk: false },
+    ]);
+    expect(py.spilled).toEqual(new Set());
+    expect(runtime.spilledOids).toEqual(new Set());
+  });
+
   it("attaches the changed data back when its rewrite cannot be written", async () => {
     const { runtime, py, attachCalls } = makeRuntime(quotaExceeded);
 

tests/ts/storage/opfsSyncObjectStore.test.ts

@@ -7,7 +7,9 @@
  * ``Uint8Array`` buffers) and a fake synchronous access handle. This lets us
  * validate the store's behaviour deterministically: the in-place write +
  * ``truncate`` (no stale trailing bytes on a smaller re-spill), the sized
- * read-back, and the bookkeeping (``totalBytes`` / ``count`` / ``delete``).
+ * read-back, short transfers (the File System standard lets ``read`` and
+ * ``write`` transfer fewer bytes than requested), and the bookkeeping
+ * (``totalBytes`` / ``count`` / ``delete``).
  */
 import { afterEach, beforeEach, describe, expect, it } from "vitest";
 
@@ -18,11 +20,28 @@ class FakeFile {
   bytes = new Uint8Array(0);
 }
 
+/** Transfer limits applied by every fake handle (reset before each test). */
+const transfer = {
+  /** Largest number of bytes a single ``read``/``write`` call transfers. */
+  maxBytes: Infinity,
+  /** Number of ``write`` calls after which writes throw. */
+  failWritesAfter: Infinity,
+  /** Number of leading bytes of a file that ``read`` can reach. */
+  readable: Infinity,
+  writeCalls: 0,
+  readCalls: 0,
+};
+
 /** Fake ``FileSystemSyncAccessHandle`` over a {@link FakeFile}. */
 class FakeSyncHandle {
   constructor(private readonly file: FakeFile) {}
   write(buf: ArrayBufferView, { at = 0 }: { at?: number } = {}): number {
-    const src = new Uint8Array(buf.buffer, buf.byteOffset, buf.byteLength);
+    transfer.writeCalls += 1;
+    if (transfer.writeCalls > transfer.failWritesAfter) {
+      throw new DOMException("write failed", "InvalidStateError");
+    }
+    const length = Math.min(buf.byteLength, transfer.maxBytes);
+    const src = new Uint8Array(buf.buffer, buf.byteOffset, length);
     const end = at + src.byteLength;
     if (end > this.file.bytes.byteLength) {
       const grown = new Uint8Array(end);
@@ -33,8 +52,11 @@ class FakeSyncHandle {
     return src.byteLength;
   }
   read(buf: ArrayBufferView, { at = 0 }: { at?: number } = {}): number {
-    const dst = new Uint8Array(buf.buffer, buf.byteOffset, buf.byteLength);
-    const slice = this.file.bytes.subarray(at, at + dst.byteLength);
+    transfer.readCalls += 1;
+    const length = Math.min(buf.byteLength, transfer.maxBytes);
+    const dst = new Uint8Array(buf.buffer, buf.byteOffset, length);
+    const end = Math.min(at + dst.byteLength, transfer.readable);
+    const slice = this.file.bytes.subarray(at, Math.max(at, end));
     dst.set(slice);
     return slice.byteLength;
   }
@@ -99,6 +121,13 @@ const ORIGINAL = {
 let opfsRoot: FakeDirHandle;
 
 beforeEach(() => {
+  Object.assign(transfer, {
+    maxBytes: Infinity,
+    failWritesAfter: Infinity,
+    readable: Infinity,
+    writeCalls: 0,
+    readCalls: 0,
+  });
   opfsRoot = new FakeDirHandle();
   Object.defineProperty(navigator, "storage", {
     configurable: true,
@@ -179,6 +208,54 @@ describe("OpfsSyncObjectStore", () => {
     expect(await store.has("b")).toBe(true);
   });
 
+  it("writes and reads a payload transferred in several fragments", async () => {
+    const store = new OpfsSyncObjectStore();
+    await store.init();
+    const payload = new Uint8Array([1, 2, 3, 4, 5, 6, 7]);
+    transfer.maxBytes = 3;
+    await store.put("obj-1", payload);
+    expect(transfer.writeCalls).toBe(3);
+    expect(store.totalBytes()).toBe(7);
+    const read = await store.get("obj-1");
+    expect(transfer.readCalls).toBe(3);
+    expect(Array.from(read)).toEqual(Array.from(payload));
+  });
+
+  it("fails a write that makes no progress", async () => {
+    // Regression: a short write was taken for a complete one, so that the
+    // object was released from memory while its file held partial data.
+    const store = new OpfsSyncObjectStore();
+    await store.init();
+    transfer.maxBytes = 0;
+    await expect(
+      store.put("obj-1", new Uint8Array([1, 2, 3, 4])),
+    ).rejects.toThrow("Incomplete OPFS write for obj-1: 0 of 4 bytes");
+    expect(store.count()).toBe(0);
+    expect(store.totalBytes()).toBe(0);
+  });
+
+  it("fails a write interrupted after a first fragment", async () => {
+    const store = new OpfsSyncObjectStore();
+    await store.init();
+    transfer.maxBytes = 2;
+    transfer.failWritesAfter = 1;
+    await expect(
+      store.put("obj-1", new Uint8Array([1, 2, 3, 4])),
+    ).rejects.toThrow("write failed");
+    expect(store.count()).toBe(0);
+  });
+
+  it("fails a read that cannot reach the end of the file", async () => {
+    // Regression: a short read returned a buffer partly filled with zeros.
+    const store = new OpfsSyncObjectStore();
+    await store.init();
+    await store.put("obj-1", new Uint8Array([1, 2, 3, 4]));
+    transfer.readable = 2;
+    await expect(store.get("obj-1")).rejects.toThrow(
+      "Incomplete OPFS read for obj-1: 2 of 4 bytes",
+    );
+  });
+
   it("clears every stored payload", async () => {
     const store = new OpfsSyncObjectStore();
     await store.init();