Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions packages/loader/driver-utils/src/prefetchDocumentStorageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,20 @@ export class PrefetchDocumentStorageService extends DocumentStorageServiceProxy
return prefetchedBlobP;
}
const prefetchedBlobPFromStorage = this.internalStorageService.readBlob(blobId);
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to skip the readBlob call if the blobId is already in prefetchCache?

Side note - you might recall that back in the day I wrote a utility for this (that correctly chains catch as you're doing in this PR) called PromiseCache. Could be useful to avoid these kinds of bugs.

this.prefetchCache.set(
blobId,
prefetchedBlobPFromStorage.catch((error) => {
if (canRetryOnError(error)) {
// Attach error handler for side effects only:
// 1. Clear cache on retryable errors so next read retries
// 2. Prevent unhandled rejection warning for fire-and-forget prefetch
// Note: Callers who await the cached promise will still see the rejection
prefetchedBlobPFromStorage.catch((error) => {
if (canRetryOnError(error)) {
// Only clear cache if our promise is still the cached one
// (avoids race condition with concurrent requests)
if (this.prefetchCache.get(blobId) === prefetchedBlobPFromStorage) {
this.prefetchCache.delete(blobId);
}
throw error;
}),
);
}
});
this.prefetchCache.set(blobId, prefetchedBlobPFromStorage);
return prefetchedBlobPFromStorage;
}
return this.internalStorageService.readBlob(blobId);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "assert";

import type {
IDocumentStorageService,
ISnapshotTree,
} from "@fluidframework/driver-definitions/internal";

import { PrefetchDocumentStorageService } from "../prefetchDocumentStorageService.js";

/**
* Helper to wait for a condition with timeout
*/
async function waitForCondition(
condition: () => boolean,
timeoutMs: number = 1000,
intervalMs: number = 5,
): Promise<void> {
const startTime = Date.now();
while (!condition()) {
if (Date.now() - startTime > timeoutMs) {
throw new Error("Condition not met within timeout");
}
await new Promise((resolve) => setTimeout(resolve, intervalMs));
}
}

/**
* Mock storage service for testing
*/
class MockStorageService implements Partial<IDocumentStorageService> {
public readBlobCalls: string[] = [];
public shouldFail = false;
public failureError = new Error("Mock read failure");

public async readBlob(blobId: string): Promise<ArrayBufferLike> {
this.readBlobCalls.push(blobId);
if (this.shouldFail) {
throw this.failureError;
}
return new Uint8Array([1, 2, 3]).buffer;
}

public async getSnapshotTree(): Promise<ISnapshotTree | null> {
return {
blobs: {
".metadata": "blob1",
"header": "blob2",
"quorumMembers": "blob3",
"other": "blob4",
},
trees: {},
};
}

public get policies() {
return undefined;
}
}

describe("PrefetchDocumentStorageService", () => {
let mockStorage: MockStorageService;
let prefetchService: PrefetchDocumentStorageService;

beforeEach(() => {
mockStorage = new MockStorageService();
prefetchService = new PrefetchDocumentStorageService(
mockStorage as unknown as IDocumentStorageService,
);
});

afterEach(() => {
prefetchService.stopPrefetch();
});

it("should propagate errors to callers who await readBlob", async () => {
mockStorage.shouldFail = true;
const testError = new Error("Network failure");
mockStorage.failureError = testError;

// Direct readBlob call should receive the error
await assert.rejects(
async () => prefetchService.readBlob("someBlob"),
(error: Error) => error.message === "Network failure",
);
});

it("should clear cache on retryable errors allowing retry", async () => {
const retryableError = new Error("Retryable error");
(retryableError as any).canRetry = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should avoid any and especially as any. This suggests that Errors used here do not match errors that will actually be used. Can be a maintenance issue for the future.

mockStorage.failureError = retryableError;
mockStorage.shouldFail = true;

// First call fails
await assert.rejects(async () => prefetchService.readBlob("blob1"));

// Reset mock to succeed
mockStorage.shouldFail = false;
mockStorage.readBlobCalls = [];

// Second call should retry (not use cached error)
const result = await prefetchService.readBlob("blob1");
assert.strictEqual(result.byteLength, 3, "Should return blob data after retry");
assert.strictEqual(
mockStorage.readBlobCalls.length,
1,
"Should perform exactly one new underlying read after cache is cleared",
);
});

it("should successfully prefetch blobs", async () => {
// Trigger prefetch
await prefetchService.getSnapshotTree();

// Wait for prefetch to complete using polling instead of fixed timeout
await waitForCondition(() => mockStorage.readBlobCalls.length > 0);

// Verify blobs were prefetched
assert.ok(
mockStorage.readBlobCalls.length > 0,
"Prefetch should have triggered blob reads",
);

// Clear call tracking
mockStorage.readBlobCalls = [];

// Reading a prefetched blob should use cache (no new call)
await prefetchService.readBlob("blob1");
assert.strictEqual(mockStorage.readBlobCalls.length, 0, "Should use cached prefetch");
});

it("should not cause unhandled rejections on fire-and-forget prefetch failures", async () => {
// Set up to fail all blob reads
const prefetchError = new Error("Prefetch network failure");
(prefetchError as any).canRetry = true;
mockStorage.failureError = prefetchError;
mockStorage.shouldFail = true;

// Trigger prefetch via getSnapshotTree (fire-and-forget pattern)
// The prefetch will fail, but should NOT cause unhandled rejection
await prefetchService.getSnapshotTree();

// Wait for prefetch attempts to occur
await waitForCondition(() => mockStorage.readBlobCalls.length > 0);

// Allow microtask queue to flush (for catch handlers to execute)
await Promise.resolve();

// If we reach here without unhandled rejection, the test passes
// Now verify that explicit readBlob calls still receive the error properly
mockStorage.readBlobCalls = [];
await assert.rejects(
async () => prefetchService.readBlob("blob1"),
(error: Error) => error.message === "Prefetch network failure",
"Explicit readBlob should still receive the error",
);
Comment on lines +154 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

This portion of the testing is a little sketchy - at the very least unclear and can easily be misinterpreted.
mockStorage.readBlobCalls = []; does nothing. There is nothing that examines mockStorage.readBlobCalls after it is changed.
In this place, there should be nothing cached. The cached item should have been cleared. So here it would be fine to change storage to respond differently. There isn't a test to cover non-retriable errors. That should be added. A better test here could be to make this error non-retriable and to check that the error is the same error.

});
});
Loading