Skip to content

Commit a3e582d

Browse files
anthony-murphy-agentclaudeanthony-murphy
committed
fix(driver-utils): prevent unhandled rejection in prefetch fire-and-forget calls
Fix three fire-and-forget patterns that cause unhandled rejections: 1. getSnapshotTree(): `void p.then(...)` creates a derived promise that rejects when p rejects. Added `.catch()` to the promise chain. 2. prefetchTree()/prefetchTreeCore(): `void this.cachedRead(blob)` discards the async function's returned promise. Since cachedRead is async, it returns a separate promise chain from the inner prefetchedBlobPFromStorage. The existing `.catch()` inside cachedRead only handles the inner promise, not the async function's returned promise. Changed to `this.cachedRead(blob).catch(() => {})`. The root cause: attaching `.catch()` to a promise P only prevents unhandled rejection for P's chain. When an async function returns P, the function's returned promise is a DIFFERENT chain that also rejects - and needs its own `.catch()` handler. This is a follow-up to PR #26151 which only fixed the inner promise in cachedRead but didn't fix the callers' fire-and-forget patterns. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>
1 parent 6b203f0 commit a3e582d

File tree

2 files changed

+48
-11
lines changed

2 files changed

+48
-11
lines changed

packages/loader/driver-utils/src/prefetchDocumentStorageService.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,20 @@ export class PrefetchDocumentStorageService extends DocumentStorageServiceProxy
3131
public async getSnapshotTree(version?: IVersion): Promise<ISnapshotTree | null> {
3232
const p = this.internalStorageService.getSnapshotTree(version);
3333
if (this.prefetchEnabled) {
34-
// We don't care if the prefetch succeeds
35-
void p.then((tree: ISnapshotTree | null | undefined) => {
36-
if (tree === null || tree === undefined) {
37-
return;
38-
}
39-
this.prefetchTree(tree);
40-
});
34+
// Fire-and-forget prefetch - we don't care if it succeeds.
35+
// The .catch() prevents unhandled rejection when p rejects, since
36+
// p.then() creates a derived promise that also rejects if p rejects.
37+
// Callers awaiting the returned p will still receive the error.
38+
void p
39+
.then((tree: ISnapshotTree | null | undefined) => {
40+
if (tree === null || tree === undefined) {
41+
return;
42+
}
43+
this.prefetchTree(tree);
44+
})
45+
.catch(() => {
46+
// Intentionally empty - error will be handled by caller awaiting p
47+
});
4148
}
4249
return p;
4350
}
@@ -81,17 +88,19 @@ export class PrefetchDocumentStorageService extends DocumentStorageServiceProxy
8188
this.prefetchTreeCore(tree, secondary);
8289

8390
for (const blob of secondary) {
84-
// We don't care if the prefetch succeeds
85-
void this.cachedRead(blob);
91+
// Fire-and-forget prefetch. The .catch() prevents unhandled rejection
92+
// since cachedRead is async and returns a separate promise chain.
93+
this.cachedRead(blob).catch(() => {});
8694
}
8795
}
8896

8997
private prefetchTreeCore(tree: ISnapshotTree, secondary: string[]): void {
9098
for (const [blobKey, blob] of Object.entries(tree.blobs)) {
9199
if (blobKey.startsWith(".") || blobKey === "header" || blobKey.startsWith("quorum")) {
92100
if (blob !== null) {
93-
// We don't care if the prefetch succeeds
94-
void this.cachedRead(blob);
101+
// Fire-and-forget prefetch. The .catch() prevents unhandled rejection
102+
// since cachedRead is async and returns a separate promise chain.
103+
this.cachedRead(blob).catch(() => {});
95104
}
96105
} else if (!blobKey.startsWith("deltas")) {
97106
if (blob !== null) {

packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ class MockStorageService implements Partial<IDocumentStorageService> {
3737
public readBlobCalls: string[] = [];
3838
public shouldFail = false;
3939
public failureError = new Error("Mock read failure");
40+
public shouldGetSnapshotTreeFail = false;
41+
public getSnapshotTreeError = new Error("Mock getSnapshotTree failure");
4042

4143
public async readBlob(blobId: string): Promise<ArrayBufferLike> {
4244
this.readBlobCalls.push(blobId);
@@ -47,6 +49,9 @@ class MockStorageService implements Partial<IDocumentStorageService> {
4749
}
4850

4951
public async getSnapshotTree(): Promise<ISnapshotTree | null> {
52+
if (this.shouldGetSnapshotTreeFail) {
53+
throw this.getSnapshotTreeError;
54+
}
5055
return {
5156
blobs: {
5257
".metadata": "blob1",
@@ -160,4 +165,27 @@ describe("PrefetchDocumentStorageService", () => {
160165
"Explicit readBlob should still receive the error",
161166
);
162167
});
168+
169+
it("should not cause unhandled rejections when getSnapshotTree fails", async () => {
170+
// Set up getSnapshotTree to fail (e.g., network timeout)
171+
const networkError = new Error("Socket timeout");
172+
mockStorage.shouldGetSnapshotTreeFail = true;
173+
mockStorage.getSnapshotTreeError = networkError;
174+
175+
// getSnapshotTree internally does: void p.then(...).catch(...)
176+
// Without the .catch(), if p rejects, the derived promise from .then() also
177+
// rejects and causes an unhandled rejection. This test verifies the fix.
178+
await assert.rejects(
179+
async () => prefetchService.getSnapshotTree(),
180+
(error: Error) => error.message === "Socket timeout",
181+
"Caller should receive the error",
182+
);
183+
184+
// Allow microtask queue to flush - if there's an unhandled rejection,
185+
// it would surface here or cause the test to fail
186+
await Promise.resolve();
187+
await new Promise((resolve) => setTimeout(resolve, 10));
188+
189+
// If we reach here without unhandled rejection, the test passes
190+
});
163191
});

0 commit comments

Comments
 (0)