From 85db6628f2f79fb454749923e2e9ebed2e29c4d4 Mon Sep 17 00:00:00 2001 From: Shane Loeffler Date: Sat, 25 Apr 2026 17:23:12 -0500 Subject: [PATCH 1/2] propagate aborted reads instead of returning missing data, use standard AbortSignal errors --- src/reader/repository.ts | 14 ++--- src/reader/session.ts | 94 ++++++++++-------------------- src/storage/http-storage.ts | 22 ++----- src/storage/storage.ts | 18 +++--- src/store.ts | 24 ++++++-- tests/reader/repository.test.ts | 11 ++++ tests/reader/session.test.ts | 26 +++++++-- tests/storage/http-storage.test.ts | 16 ++--- tests/store.test.ts | 63 ++++++++++++++------ 9 files changed, 155 insertions(+), 133 deletions(-) diff --git a/src/reader/repository.ts b/src/reader/repository.ts index b923604..78d39bf 100644 --- a/src/reader/repository.ts +++ b/src/reader/repository.ts @@ -3,7 +3,7 @@ */ import type { Storage, RequestOptions } from "../storage/storage.js"; -import { NotFoundError, AbortError } from "../storage/storage.js"; +import { NotFoundError, isAbortError } from "../storage/storage.js"; import { getBranchRefDirPath, getBranchRefPath, @@ -63,7 +63,7 @@ export class Repository { * @param options - Optional request options (signal for cancellation) * @returns ParsedRepo if v2 format, null if v1 format * @throws Error if repo file exists but fails to parse - * @throws AbortError if the operation was aborted + * @throws An error named "AbortError" if the operation was aborted */ private async loadRepoInfo( options?: RequestOptions, @@ -83,7 +83,7 @@ export class Repository { return this.repoInfo; } catch (error) { // Propagate abort errors - if (error instanceof AbortError) { + if (isAbortError(error)) { this.repoInfoAttempted = false; // Allow retry throw error; } @@ -167,7 +167,7 @@ export class Repository { } } catch (error) { // Propagate abort errors - if (error instanceof AbortError) { + if (isAbortError(error)) { throw error; } // Listing not supported - try legacy path only @@ -208,7 +208,7 @@ export class Repository { } } catch (error) { // Propagate abort errors - if (error instanceof AbortError) { + if (isAbortError(error)) { throw error; } // Listing not supported - return legacy path without checking exists. @@ -265,7 +265,7 @@ export class Repository { } } } catch (error) { - if (error instanceof AbortError) { + if (isAbortError(error)) { throw error; } throw new Error("Cannot list branches: storage does not support listing"); @@ -322,7 +322,7 @@ export class Repository { } } } catch (error) { - if (error instanceof AbortError) { + if (isAbortError(error)) { throw error; } throw new Error("Cannot list tags: storage does not support listing"); diff --git a/src/reader/session.ts b/src/reader/session.ts index 3b1a6a3..3636488 100644 --- a/src/reader/session.ts +++ b/src/reader/session.ts @@ -3,7 +3,6 @@ */ import type { Storage, ByteRange, RequestOptions } from "../storage/storage.js"; -import { AbortError } from "../storage/storage.js"; import { decompress } from "fzstd"; import { LRUCache } from "../cache/lru.js"; import { @@ -307,8 +306,7 @@ export class ReadSession { coords: number[], options?: RequestOptions, ): Promise { - // Early abort check - if (options?.signal?.aborted) return null; + options?.signal?.throwIfAborted(); const node = this.getNode(path); if (!node || node.nodeData.type !== "array") { @@ -318,28 +316,22 @@ export class ReadSession { // Find the manifest that contains this chunk const arrayData = node.nodeData; - try { - for (const manifestRef of arrayData.manifests) { - // Check if this manifest covers the requested coordinates - if (!this.coordsInExtents(coords, manifestRef.extents)) { - continue; - } + for (const manifestRef of arrayData.manifests) { + // Check if this manifest covers the requested coordinates + if (!this.coordsInExtents(coords, manifestRef.extents)) { + continue; + } - // Load the manifest with signal - const manifest = await this.loadManifest(manifestRef.objectId, options); + // Load the manifest with signal + const manifest = await this.loadManifest(manifestRef.objectId, options); - // Find the chunk reference - const chunkRef = findChunkRef(manifest, node.id, coords); - if (!chunkRef) continue; + // Find the chunk reference + const chunkRef = findChunkRef(manifest, node.id, coords); + if (!chunkRef) continue; - // Fetch the chunk data based on payload type with signal - const payload = getChunkPayload(chunkRef); - return this.fetchChunkPayload(payload, options); - } - } catch (error) { - // Mid-flight abort → return null - if (error instanceof AbortError) return null; - throw error; + // Fetch the chunk data based on payload type with signal + const payload = getChunkPayload(chunkRef); + return this.fetchChunkPayload(payload, options); } return null; @@ -364,7 +356,7 @@ export class ReadSession { range: { offset: number; length: number } | { suffixLength: number }, options?: RequestOptions, ): Promise { - if (options?.signal?.aborted) return null; + options?.signal?.throwIfAborted(); const node = this.getNode(path); if (!node || node.nodeData.type !== "array") { @@ -373,22 +365,17 @@ export class ReadSession { const arrayData = node.nodeData; - try { - for (const manifestRef of arrayData.manifests) { - if (!this.coordsInExtents(coords, manifestRef.extents)) { - continue; - } + for (const manifestRef of arrayData.manifests) { + if (!this.coordsInExtents(coords, manifestRef.extents)) { + continue; + } - const manifest = await this.loadManifest(manifestRef.objectId, options); - const chunkRef = findChunkRef(manifest, node.id, coords); - if (!chunkRef) continue; + const manifest = await this.loadManifest(manifestRef.objectId, options); + const chunkRef = findChunkRef(manifest, node.id, coords); + if (!chunkRef) continue; - const payload = getChunkPayload(chunkRef); - return this.fetchChunkPayloadRange(payload, range, options); - } - } catch (error) { - if (error instanceof AbortError) return null; - throw error; + const payload = getChunkPayload(chunkRef); + return this.fetchChunkPayloadRange(payload, range, options); } return null; @@ -469,19 +456,10 @@ export class ReadSession { signal: options?.signal, }; - let response: Response; - try { - const client = options?.fetchClient; - response = client - ? await client.fetch(httpUrl, fetchInit) - : await fetch(httpUrl, fetchInit); - } catch (error) { - // Translate abort errors to our class (handles DOMException and other implementations) - if (error instanceof Error && error.name === "AbortError") { - throw new AbortError(); - } - throw error; - } + const client = options?.fetchClient; + const response = client + ? await client.fetch(httpUrl, fetchInit) + : await fetch(httpUrl, fetchInit); if (response.status === 412) { throw new Error( @@ -596,18 +574,10 @@ export class ReadSession { signal: options?.signal, }; - let response: Response; - try { - const client = options?.fetchClient; - response = client - ? await client.fetch(httpUrl, fetchInit) - : await fetch(httpUrl, fetchInit); - } catch (error) { - if (error instanceof Error && error.name === "AbortError") { - throw new AbortError(); - } - throw error; - } + const client = options?.fetchClient; + const response = client + ? await client.fetch(httpUrl, fetchInit) + : await fetch(httpUrl, fetchInit); if (response.status === 412) { throw new Error( diff --git a/src/storage/http-storage.ts b/src/storage/http-storage.ts index 0b0243c..81c1bd0 100644 --- a/src/storage/http-storage.ts +++ b/src/storage/http-storage.ts @@ -5,7 +5,7 @@ */ import type { Storage, ByteRange, RequestOptions } from "./storage.js"; -import { NotFoundError, StorageError, AbortError } from "./storage.js"; +import { NotFoundError, StorageError, isAbortError } from "./storage.js"; /** Options for HTTP storage */ export interface HttpStorageOptions { @@ -65,10 +65,7 @@ export class HttpStorage implements Storage { range?: ByteRange, options?: RequestOptions, ): Promise { - // Early abort check - if (options?.signal?.aborted) { - throw new AbortError(); - } + options?.signal?.throwIfAborted(); const url = this.getUrl(path); const headers = this.getHeaders(range); @@ -83,10 +80,7 @@ export class HttpStorage implements Storage { signal: options?.signal, }); } catch (error) { - // Translate abort errors to our class (handles DOMException and other implementations) - if (error instanceof Error && error.name === "AbortError") { - throw new AbortError(); - } + if (isAbortError(error)) throw error; throw new StorageError( `Failed to fetch ${url}: ${error instanceof Error ? error.message : String(error)}`, error instanceof Error ? error : undefined, @@ -109,10 +103,7 @@ export class HttpStorage implements Storage { } async exists(path: string, options?: RequestOptions): Promise { - // Early abort check - if (options?.signal?.aborted) { - throw new AbortError(); - } + options?.signal?.throwIfAborted(); const url = this.getUrl(path); @@ -126,10 +117,7 @@ export class HttpStorage implements Storage { return response.ok; } catch (error) { - // Rethrow abort errors (handles DOMException and other implementations) - if (error instanceof Error && error.name === "AbortError") { - throw new AbortError(); - } + if (isAbortError(error)) throw error; return false; } } diff --git a/src/storage/storage.ts b/src/storage/storage.ts index 37148e6..b580958 100644 --- a/src/storage/storage.ts +++ b/src/storage/storage.ts @@ -82,12 +82,14 @@ export class StorageError extends Error { } } -/** Error thrown when an operation is aborted */ -export class AbortError extends Error { - constructor(message = "Operation aborted") { - super(message); - this.name = "AbortError"; - } +/** True for web-platform abort errors from fetch/AbortSignal. */ +export function isAbortError(error: unknown): boolean { + return ( + typeof error === "object" && + error !== null && + "name" in error && + error.name === "AbortError" + ); } /** @@ -105,7 +107,7 @@ export interface Storage { * @returns Object data as bytes * @throws NotFoundError if the object doesn't exist * @throws StorageError for other errors - * @throws AbortError if the operation was aborted + * @throws An error named "AbortError" if the operation was aborted */ getObject( path: string, @@ -119,7 +121,7 @@ export interface Storage { * @param path - Path to the object * @param options - Optional request options (signal for cancellation) * @returns True if the object exists - * @throws AbortError if the operation was aborted + * @throws An error named "AbortError" if the operation was aborted */ exists(path: string, options?: RequestOptions): Promise; diff --git a/src/store.ts b/src/store.ts index 1d82288..f25ebee 100644 --- a/src/store.ts +++ b/src/store.ts @@ -9,6 +9,7 @@ import { Repository } from "./reader/repository.js"; import { ReadSession } from "./reader/session.js"; import { HttpStorage } from "./storage/http-storage.js"; import type { Storage, FetchClient } from "./storage/storage.js"; +import { NotFoundError } from "./storage/storage.js"; import type { NodeSnapshot } from "./format/flatbuffers/types.js"; /** @@ -219,7 +220,7 @@ export class IcechunkStore implements AsyncReadable { key: AbsolutePath, opts?: { signal?: AbortSignal }, ): Promise { - if (opts?.signal?.aborted) return undefined; + opts?.signal?.throwIfAborted(); const parsed = parseZarrKey(key); const resolvedPath = this.resolvePath(parsed.path); @@ -237,8 +238,15 @@ export class IcechunkStore implements AsyncReadable { azureAccount: this.azureAccount, }); return chunk ?? undefined; - } catch { - return undefined; + } catch (err) { + // Only treat "chunk legitimately absent" as undefined. Propagate + // everything else — swallowing aborts/network errors here causes + // zarrita's Array.getChunk to return a fillValue chunk, which + // downstream consumers cache as valid data (e.g. zarr-layer commits + // it to region.data, producing permanent blank regions even after + // repainting). + if (err instanceof NotFoundError) return undefined; + throw err; } } @@ -259,7 +267,7 @@ export class IcechunkStore implements AsyncReadable { range: RangeQuery, opts?: { signal?: AbortSignal }, ): Promise { - if (opts?.signal?.aborted) return undefined; + opts?.signal?.throwIfAborted(); const parsed = parseZarrKey(key); const resolvedPath = this.resolvePath(parsed.path); @@ -289,8 +297,12 @@ export class IcechunkStore implements AsyncReadable { return data.slice(-range.suffixLength); } return data.slice(range.offset, range.offset + range.length); - } catch { - return undefined; + } catch (err) { + // Same rationale as `get()`: don't swallow aborts or real errors, + // or zarrita will silently fall back to a fillValue chunk and + // consumers will cache that garbage as valid data. + if (err instanceof NotFoundError) return undefined; + throw err; } } diff --git a/tests/reader/repository.test.ts b/tests/reader/repository.test.ts index dbcd2bb..3c27b36 100644 --- a/tests/reader/repository.test.ts +++ b/tests/reader/repository.test.ts @@ -68,6 +68,17 @@ describe("Repository", () => { ); }); + it("should propagate native abort errors while opening", async () => { + const abortError = new DOMException("Aborted", "AbortError"); + const storage = new (class extends MockStorage { + async getObject(): Promise { + throw abortError; + } + })(); + + await expect(Repository.open({ storage })).rejects.toBe(abortError); + }); + it("should open a valid v1 repository with main branch", async () => { const snapshotId = createMockSnapshotId(1); const storage = new MockStorage({ diff --git a/tests/reader/session.test.ts b/tests/reader/session.test.ts index b024788..b144fc6 100644 --- a/tests/reader/session.test.ts +++ b/tests/reader/session.test.ts @@ -936,7 +936,7 @@ describe("ReadSession", () => { }); describe("abort signal handling", () => { - it("should return null when signal is already aborted", async () => { + it("should reject when signal is already aborted", async () => { const session = createMockSession({ nodes: [createArrayNode("/array", { manifests: [] })], }); @@ -944,11 +944,29 @@ describe("ReadSession", () => { const controller = new AbortController(); controller.abort(); - const result = await session.getChunk("/array", [0], { - signal: controller.signal, + await expect( + session.getChunk("/array", [0], { + signal: controller.signal, + }), + ).rejects.toMatchObject({ name: "AbortError" }); + }); + + it("should reject range reads when signal is already aborted", async () => { + const session = createMockSession({ + nodes: [createArrayNode("/array", { manifests: [] })], }); - expect(result).toBeNull(); + const controller = new AbortController(); + controller.abort(); + + await expect( + session.getChunkRange( + "/array", + [0], + { offset: 0, length: 1 }, + { signal: controller.signal }, + ), + ).rejects.toMatchObject({ name: "AbortError" }); }); it("should return null for non-existent array (not throw)", async () => { diff --git a/tests/storage/http-storage.test.ts b/tests/storage/http-storage.test.ts index f044c34..265a085 100644 --- a/tests/storage/http-storage.test.ts +++ b/tests/storage/http-storage.test.ts @@ -1,10 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { HttpStorage } from "../../src/storage/http-storage.js"; -import { - NotFoundError, - StorageError, - AbortError, -} from "../../src/storage/storage.js"; +import { NotFoundError, StorageError } from "../../src/storage/storage.js"; describe("HttpStorage", () => { const originalFetch = global.fetch; @@ -217,7 +213,7 @@ describe("HttpStorage", () => { await expect( storage.getObject("file.txt", undefined, { signal: controller.signal }), - ).rejects.toThrow(AbortError); + ).rejects.toMatchObject({ name: "AbortError" }); // fetch should not be called expect(global.fetch).not.toHaveBeenCalled(); @@ -232,7 +228,7 @@ describe("HttpStorage", () => { await expect( storage.getObject("file.txt", undefined, { signal: controller.signal }), - ).rejects.toThrow(AbortError); + ).rejects.toBe(abortError); }); it("should pass signal to fetch (getObject)", async () => { @@ -262,7 +258,7 @@ describe("HttpStorage", () => { await expect( storage.exists("file.txt", { signal: controller.signal }), - ).rejects.toThrow(AbortError); + ).rejects.toMatchObject({ name: "AbortError" }); expect(global.fetch).not.toHaveBeenCalled(); }); @@ -276,7 +272,7 @@ describe("HttpStorage", () => { await expect( storage.exists("file.txt", { signal: controller.signal }), - ).rejects.toThrow(AbortError); + ).rejects.toBe(abortError); }); it("should handle non-DOMException abort errors", async () => { @@ -290,7 +286,7 @@ describe("HttpStorage", () => { await expect( storage.getObject("file.txt", undefined, { signal: controller.signal }), - ).rejects.toThrow(AbortError); + ).rejects.toBe(abortError); }); }); }); diff --git a/tests/store.test.ts b/tests/store.test.ts index 0ac82c6..aec9f67 100644 --- a/tests/store.test.ts +++ b/tests/store.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { IcechunkStore } from "../src/store.js"; import type { AbsolutePath } from "../src/store.js"; import { MockStorage } from "./fixtures/mock-storage.js"; +import { NotFoundError } from "../src/storage/storage.js"; /** * Helper to create an IcechunkStore with a mock session. @@ -105,15 +106,40 @@ describe("IcechunkStore", () => { }); describe("error handling", () => { - it("should return undefined on error", async () => { + it("should return undefined for NotFoundError (legitimate missing key)", async () => { getRawMetadataSpy.mockImplementation(() => { - throw new Error("Storage error"); + throw new NotFoundError("/array/zarr.json"); }); const result = await store.get("/array/zarr.json" as AbsolutePath); expect(result).toBeUndefined(); }); + + it("should propagate non-NotFoundError failures", async () => { + // Swallowing these would cause zarrita's Array.getChunk to fall back + // to a fillValue chunk silently — downstream consumers then cache + // that garbage as if it were real data. + getRawMetadataSpy.mockImplementation(() => { + throw new Error("Storage error"); + }); + + await expect( + store.get("/array/zarr.json" as AbsolutePath), + ).rejects.toThrow("Storage error"); + }); + + it("should propagate AbortError so consumers don't cache garbage", async () => { + const abortErr = new Error("Operation aborted"); + abortErr.name = "AbortError"; + getRawMetadataSpy.mockImplementation(() => { + throw abortErr; + }); + + await expect( + store.get("/array/zarr.json" as AbsolutePath), + ).rejects.toThrow("Operation aborted"); + }); }); describe("key parsing edge cases", () => { @@ -271,24 +297,22 @@ describe("IcechunkStore", () => { expect(result).toBeUndefined(); }); - it("should return undefined when signal is already aborted", async () => { + it("should reject when signal is already aborted", async () => { const store = createStoreWithMockSession({ - getRawMetadata: vi - .fn() - .mockReturnValue(new Uint8Array([1, 2, 3, 4, 5])), + getRawMetadata: vi.fn(), getChunk: vi.fn(), }); const controller = new AbortController(); controller.abort(); - const result = await store.getRange( - "/zarr.json" as AbsolutePath, - { offset: 0, length: 3 }, - { signal: controller.signal }, - ); - - expect(result).toBeUndefined(); + await expect( + store.getRange( + "/zarr.json" as AbsolutePath, + { offset: 0, length: 3 }, + { signal: controller.signal }, + ), + ).rejects.toMatchObject({ name: "AbortError" }); }); }); @@ -361,7 +385,7 @@ describe("IcechunkStore", () => { }); describe("abort signal handling", () => { - it("should return undefined when signal is already aborted", async () => { + it("should reject when signal is already aborted", async () => { const getChunkSpy = vi.fn(); const store = createStoreWithMockSession({ getRawMetadata: vi.fn(), @@ -371,11 +395,12 @@ describe("IcechunkStore", () => { const controller = new AbortController(); controller.abort(); - const result = await store.get("/array/c/0" as AbsolutePath, { - signal: controller.signal, - }); + await expect( + store.get("/array/c/0" as AbsolutePath, { + signal: controller.signal, + }), + ).rejects.toMatchObject({ name: "AbortError" }); - expect(result).toBeUndefined(); expect(getChunkSpy).not.toHaveBeenCalled(); }); @@ -397,7 +422,7 @@ describe("IcechunkStore", () => { }); }); - it("should return undefined when getChunk returns null due to abort", async () => { + it("should return undefined when getChunk returns null for a missing chunk", async () => { const getChunkSpy = vi.fn().mockResolvedValue(null); const store = createStoreWithMockSession({ getRawMetadata: vi.fn(), From c157bfc04b94ff12f866802f6c4901deaed6bb29 Mon Sep 17 00:00:00 2001 From: Shane Loeffler Date: Sat, 25 Apr 2026 17:33:26 -0500 Subject: [PATCH 2/2] Clarify abort error type guard --- src/storage/storage.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/storage/storage.ts b/src/storage/storage.ts index b580958..0e2bb87 100644 --- a/src/storage/storage.ts +++ b/src/storage/storage.ts @@ -82,14 +82,17 @@ export class StorageError extends Error { } } +type ErrorWithName = { name: unknown }; + +function hasErrorName(error: unknown): error is ErrorWithName { + return typeof error === "object" && error !== null && "name" in error; +} + /** True for web-platform abort errors from fetch/AbortSignal. */ -export function isAbortError(error: unknown): boolean { - return ( - typeof error === "object" && - error !== null && - "name" in error && - error.name === "AbortError" - ); +export function isAbortError( + error: unknown, +): error is ErrorWithName & { name: "AbortError" } { + return hasErrorName(error) && error.name === "AbortError"; } /**