Add Blob openStream() so missing-blob-file errors are catchable before the response commits - #2138
Add Blob openStream() so missing-blob-file errors are catchable before the response commits#2138harper-joseph wants to merge 2 commits into
Conversation
…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>
There was a problem hiding this comment.
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.
| 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); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
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
- Performance is a feature here. Do flag hot-path allocations, unnecessary async/await layers, and per-request work that could be hoisted. (link)
There was a problem hiding this comment.
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)
| await assert.rejects(blob.openStream(), (error) => { | ||
| assert.equal(error.statusCode, 404); | ||
| assert.equal(error.code, 'ENOENT'); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
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
- Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
- Use the bare
node:assertmodule instead ofnode:assert/strictfor test assertions to comply with linting rules, while still utilizing strict assertion methods likeassert.strictEqualfrom the bare module.
There was a problem hiding this comment.
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>
|
A few thoughts:
|
Summary
Adds
openStream(): Promise<ReadableStream>toFileBackedBlob, an async variant ofstream()that resolves only once the backing blob file has been opened and its header validated — so the pre-streaming failure class is catchable withtry/catchbefore an HTTP caller commits a response status: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, andawait 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 existingstream(): 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 aReadableStreamthat re-emits that first chunk and forwards subsequent reads andcancel(). In-memory (contentBuffer) blobs returnstream()directly since they cannot fail to open.stream()itself is untouched (changing its signature would break the webBlobcontract and internal callers likesaveBlob).Note: if a write is in flight,
openStream()waits up tostorage.blobReadTimeout(default 20s) before resolving or rejecting 503 — same semantics as the existing read paths.Where to look
resources/blob.ts— cancellation propagates viareader.cancel(), mid-stream errors still surface through the returned stream andon('error')listeners. Zero-length blobs close on the first read (donehandled instart).server/http.tsbody handling) use this so every client gets a proper 404/503 for missing blobs with no app-code change.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 (withcode: 'ENOENT'), write-in-flight → 503, self-consistent truncation → 500. Full blob suite passes (68 tests).GEMINI_API_KEYwas available in the agent environment. Flagging so human review knows it's the first independent pass.🤖 Generated with Claude Code (Claude Fable 5)