Skip to content

Add Blob openStream() so missing-blob-file errors are catchable before the response commits - #2138

Draft
harper-joseph wants to merge 2 commits into
mainfrom
feat/blob-open-stream
Draft

Add Blob openStream() so missing-blob-file errors are catchable before the response commits#2138
harper-joseph wants to merge 2 commits into
mainfrom
feat/blob-open-stream

Conversation

@harper-joseph

Copy link
Copy Markdown
Contributor

Summary

Adds openStream(): Promise<ReadableStream> to FileBackedBlob, an async variant of stream() that resolves only once the backing blob file has been opened and its header validated — so the pre-streaming failure class is catchable with try/catch before an HTTP caller commits a response status:

try {
	const stream = await blob.openStream();
	// safe to send headers now
} catch (e) {
	// e.statusCode: 404 (file cleanly gone), 503 (write/replication in flight), 500 (corrupt)
}

Purpose

When a record exists but its blob file is missing (e.g. the replicated blob never landed — the client-facing symptom in #2134), the current options are bad: blob.on('error') fires only after the client already received a 200, and await blob.bytes() buffers the whole (often very large) blob in memory. In production, ~99% of blob read errors are detectable at open/header time; this makes that class catchable up front while still streaming the body with at most one chunk held in memory.

Implementation

openStream() wraps the existing stream(): it takes a reader, awaits the first read — which drives the file open (with its existing retry/timeout classification from #1423/#1454) and the first-pull header checks (error stubs, pending-replication stubs, #1424 descriptor/size cross-check) — then returns a ReadableStream that re-emits that first chunk and forwards subsequent reads and cancel(). In-memory (contentBuffer) blobs return stream() directly since they cannot fail to open. stream() itself is untouched (changing its signature would break the web Blob contract and internal callers like saveBlob).

Note: if a write is in flight, openStream() waits up to storage.blobReadTimeout (default 20s) before resolving or rejecting 503 — same semantics as the existing read paths.

Where to look

  • The reader-wrapper stream in resources/blob.ts — cancellation propagates via reader.cancel(), mid-stream errors still surface through the returned stream and on('error') listeners. Zero-length blobs close on the first read (done handled in start).
  • Not included (possible follow-up): having Harper's own HTTP path (server/http.ts body handling) use this so every client gets a proper 404/503 for missing blobs with no app-code change.
  • External docs: the Blob API docs in the documentation repo should gain a section on openStream() once this lands.

Testing

Five new unit tests in unitTests/resources/blob.test.js: healthy disk-backed blob + slice, small in-memory blob, missing file → 404 (with code: 'ENOENT'), write-in-flight → 503, self-consistent truncation → 500. Full blob suite passes (68 tests).

⚠️ Cross-model reviews (Codex/Gemini) could not be run: Codex's auth token needs an interactive re-login and no GEMINI_API_KEY was available in the agent environment. Flagging so human review knows it's the first independent pass.


🤖 Generated with Claude Code (Claude Fable 5)

…le before the response commits

stream() must return synchronously (the web Blob contract), so a missing or
corrupt backing file only surfaces through the stream itself — by then an HTTP
caller has typically already sent a 200 for a body that will never arrive.
openStream() awaits the underlying file open and first read (where the header
is validated), so missing files (404), in-flight writes/half-replicated blobs
(503), and error stubs/truncation (500) reject as catchable BlobReadErrors
while still streaming the content without buffering more than one chunk.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an openStream() method to the FileBackedBlob class, allowing blobs to be opened for streaming only after verifying that the backing content is readable. It also adds comprehensive unit tests covering various success and failure scenarios. The review feedback suggests optimizing openStream by replacing async/await with promise chains to reduce overhead on hot paths, and recommends using assert.strictEqual instead of assert.equal in tests to align with the repository's strict assertion style guide.

Comment thread resources/blob.ts
Comment on lines +830 to +852
async openStream(): Promise<ReadableStream> {
const storageInfo = storageInfoForBlob.get(this);
// in-memory content cannot fail to open, use the buffer-backed stream directly
if (storageInfo?.contentBuffer) return this.stream();
const reader = this.stream().getReader();
// the first read drives the file open (with its retry/timeout classification) and the header
// checks in the first pull, so everything wrong before content flows rejects here
const firstChunk = await reader.read();
return new ReadableStream({
start(controller) {
if (firstChunk.done) controller.close();
else controller.enqueue(firstChunk.value);
},
async pull(controller) {
const { done, value } = await reader.read();
if (done) controller.close();
else controller.enqueue(value);
},
cancel(reason) {
return reader.cancel(reason);
},
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The openStream method and its inner pull handler can be optimized to avoid async/await overhead. In high-performance hot paths like stream chunk pulling, avoiding async/await state machines and extra promise allocations by returning .then() chains directly improves performance and reduces garbage collection pressure, aligning with the repository's performance-first style guide.

	openStream(): Promise<ReadableStream> {
		const storageInfo = storageInfoForBlob.get(this);
		// in-memory content cannot fail to open, use the buffer-backed stream directly
		if (storageInfo?.contentBuffer) return Promise.resolve(this.stream());
		const reader = this.stream().getReader();
		// the first read drives the file open (with its retry/timeout classification) and the header
		// checks in the first pull, so everything wrong before content flows rejects here
		return reader.read().then((firstChunk) => {
			return new ReadableStream({
				start(controller) {
					if (firstChunk.done) controller.close();
					else controller.enqueue(firstChunk.value);
				},
				pull(controller) {
					return reader.read().then(({ done, value }) => {
						if (done) controller.close();
						else controller.enqueue(value);
					});
				},
				cancel(reason) {
					return reader.cancel(reason);
				},
			});
		});
	}
References
  1. Performance is a feature here. Do flag hot-path allocations, unnecessary async/await layers, and per-request work that could be hoisted. (link)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 0e993c6 for the per-chunk pull() handler, which is the hot part. Kept openStream() itself async: it runs once per open (not per chunk), and the async wrapper guarantees a synchronous throw from stream()/getReader() (e.g. no store for the file path) surfaces as a rejection rather than a sync exception, keeping blob.openStream().catch(...) uniform for callers. (reply generated by AI)

Comment on lines +1222 to +1226
await assert.rejects(blob.openStream(), (error) => {
assert.equal(error.statusCode, 404);
assert.equal(error.code, 'ENOENT');
return true;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use assert.strictEqual instead of assert.equal to adhere to the repository's strict assertion style guide. This applies to lines 1223, 1224, 1236, and 1248. Note that we should use strict assertion methods like assert.strictEqual from the bare node:assert module rather than importing node:assert/strict to comply with repository standards.

		await assert.rejects(blob.openStream(), (error) => {
			assert.strictEqual(error.statusCode, 404);
			assert.strictEqual(error.code, 'ENOENT');
			return true;
		});
References
  1. Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
  2. Use the bare node:assert module instead of node:assert/strict for test assertions to comply with linting rules, while still utilizing strict assertion methods like assert.strictEqual from the bare module.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 0e993c6 — all four assertions switched to assert.strictEqual from the bare node:assert module. (reply generated by AI)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp

kriszyp commented Aug 11, 2026

Copy link
Copy Markdown
Member

A few thoughts:

  • If this is preferable default behavior, I wonder if the blob streams should be marked with flag indicating that the first chunk should be checked, and http.ts should delay sending the status code, and catch errors on streams (to alter status code)
  • Still concerned the underlying cause should be addressed; not sure what it is here.
  • openStream doesn't seem very descriptive to me. The key goals is verification of a valid blob; maybe a blob.verify() (async) method. If we really need an explicit API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants