Skip to content

Commit 6f218b3

Browse files
anthony-murphy-agentclaudeanthony-murphy
authored
fix(driver-utils): prevent unhandled rejection in getSnapshotTree prefetch (#26173)
## Summary - Fix unhandled promise rejections in `PrefetchDocumentStorageService` fire-and-forget calls - Add unit test for getSnapshotTree failure scenario ## Problem This is a follow-up to PR #26151. After that fix was deployed, telemetry showed 349 `uncaughtException` errors from `ShreddedSummaryDocumentStorageService.readBlob` in test-service-load tinylicious tests. **Root cause:** PR #26151 attached `.catch()` to the inner promise in `cachedRead()`, but that only handles ONE promise chain. The fire-and-forget callers create a DIFFERENT chain: 1. **`void this.cachedRead(blob)`** - `cachedRead` is async, so it returns a NEW promise wrapping the inner promise. The `.catch()` on the inner promise doesn't prevent unhandled rejection on the async function's returned promise. 2. **`void p.then(...)`** in `getSnapshotTree()` - creates a derived promise that also rejects when `p` rejects. ## Solution Fix all three fire-and-forget patterns: ```typescript // Before (inner promise handled, but async wrapper not): void this.cachedRead(blob); // After (handles the async function's returned promise): this.cachedRead(blob).catch(() => {}); ``` ```typescript // Before (derived promise not handled): void p.then((tree) => this.prefetchTree(tree)); // After (handles the derived promise): void p.then(...).catch(() => {}); ``` ## Test plan - [x] New unit test for `getSnapshotTree` failure scenario - [x] All 5 PrefetchDocumentStorageService tests pass - [x] Build passes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> --------- Co-authored-by: anthony-murphy-agent <253562292+anthony-murphy-agent@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: anthony-murphy <anthony.murphy@microsoft.com>
1 parent 6b203f0 commit 6f218b3

File tree

2 files changed

+41
-6
lines changed

2 files changed

+41
-6
lines changed

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,17 @@ 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) => {
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+
p.then((tree: ISnapshotTree | null | undefined) => {
3639
if (tree === null || tree === undefined) {
3740
return;
3841
}
3942
this.prefetchTree(tree);
43+
}).catch(() => {
44+
// Intentionally empty - error will be handled by caller awaiting p
4045
});
4146
}
4247
return p;
@@ -81,17 +86,19 @@ export class PrefetchDocumentStorageService extends DocumentStorageServiceProxy
8186
this.prefetchTreeCore(tree, secondary);
8287

8388
for (const blob of secondary) {
84-
// We don't care if the prefetch succeeds
85-
void this.cachedRead(blob);
89+
// Fire-and-forget prefetch. The .catch() prevents unhandled rejection
90+
// since cachedRead is async and returns a separate promise chain.
91+
this.cachedRead(blob).catch(() => {});
8692
}
8793
}
8894

8995
private prefetchTreeCore(tree: ISnapshotTree, secondary: string[]): void {
9096
for (const [blobKey, blob] of Object.entries(tree.blobs)) {
9197
if (blobKey.startsWith(".") || blobKey === "header" || blobKey.startsWith("quorum")) {
9298
if (blob !== null) {
93-
// We don't care if the prefetch succeeds
94-
void this.cachedRead(blob);
99+
// Fire-and-forget prefetch. The .catch() prevents unhandled rejection
100+
// since cachedRead is async and returns a separate promise chain.
101+
this.cachedRead(blob).catch(() => {});
95102
}
96103
} else if (!blobKey.startsWith("deltas")) {
97104
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)