fix(driver-utils): prevent unhandled rejections in prefetch by anthony-murphy-agent · Pull Request #26151 · microsoft/FluidFramework

anthony-murphy-agent · 2026-01-07T23:12:29Z

Summary

Fix unhandled promise rejections in PrefetchDocumentStorageService when parallel prefetch requests fail
Add unit tests for prefetch error handling and caching behavior

Problem

The PrefetchDocumentStorageService fires prefetch requests using fire-and-forget pattern (void this.cachedRead(blob)). When these requests failed, the .catch() handler was re-throwing errors, creating rejected promises stored in cache that no one awaited - causing uncaughtException errors.

Evidence from telemetry showed 98 uncaughtException errors from a single process within ~70ms, all originating from readBlob via prefetch.

Solution

Store the original promise in the cache and attach .catch() for side effects only (cache cleanup on retryable errors). Do not re-throw - callers who await the cached promise still receive the rejection properly.

Also added a race-condition fix to avoid clearing a newer cached promise when an older one fails.

Test plan

New unit tests for PrefetchDocumentStorageService (4 tests covering error propagation, retry behavior, prefetch caching, and fire-and-forget failure handling)
driver-utils tests pass (60 tests)
Lint passes

🤖 Generated with Claude Code

Co-Authored-By: anthony-murphy anthony.murphy@microsoft.com

fixes AB#47951

The PrefetchDocumentStorageService was storing a promise that re-threw errors in the cache. When prefetch fired requests with fire-and-forget pattern (void this.cachedRead()), failures created rejected promises that no one awaited, causing unhandled rejection errors. The fix stores the original promise in the cache and attaches a .catch() handler for side effects only (cache cleanup on retryable errors). Callers who await the cached promise still receive the rejection properly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

Add unit tests verifying: - Errors propagate to callers who await readBlob - Cache is cleared on retryable errors, allowing retry - Prefetch successfully caches blobs for later reads 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

Replace `assert.ok(result)` with `assert.strictEqual(result.byteLength, 3)` to avoid using an object in a boolean context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

- Fix race condition in catch handler by checking if cached promise is still the same before deleting (prevents concurrent request issues) - Replace flaky setTimeout with polling-based waitForCondition helper - Add test for fire-and-forget prefetch failure scenario - Improve assertion messages for clarity 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

Use Promise.resolve() to flush the microtask queue for catch handlers, as setImmediate schedules a macrotask which is semantically incorrect for this purpose. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

anthony-murphy-agent · 2026-01-07T23:17:05Z

/azp run

azure-pipelines · 2026-01-07T23:17:11Z

Commenter does not have sufficient privileges for PR 26151 in repo microsoft/FluidFramework

Copilot

Pull request overview

This PR fixes unhandled promise rejections in PrefetchDocumentStorageService that occurred when fire-and-forget prefetch requests failed. The fix changes how error handlers are attached to cached promises to prevent rejected promises from causing uncaughtException errors.

Key changes:

Modified the error handling in cachedRead() to attach .catch() for side effects only, without re-throwing errors
Added race condition protection to prevent clearing newer cached promises when older ones fail
Added comprehensive unit tests covering error propagation, retry behavior, prefetch caching, and fire-and-forget failure scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`packages/loader/driver-utils/src/prefetchDocumentStorageService.ts`	Fixed unhandled rejection issue by storing original promise in cache and attaching non-throwing catch handler for cleanup; added race condition protection
`packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts`	Added comprehensive test suite with 4 tests covering error propagation, cache clearing on retryable errors, successful prefetch caching, and fire-and-forget failure handling

anthony-murphy · 2026-01-07T23:21:10Z

/azp run

azure-pipelines · 2026-01-07T23:21:16Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

anthony-murphy · 2026-01-07T23:22:09Z

/azp run Build - client packages

azure-pipelines · 2026-01-07T23:22:24Z

Azure Pipelines successfully started running 1 pipeline(s).

anthony-murphy · 2026-01-07T23:23:18Z

/azp run Build - client packages, Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools, repo-policy-check, server-routerlicious, server-gitrest, server-historian, server-gitssh

azure-pipelines · 2026-01-07T23:23:24Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

anthony-murphy · 2026-01-07T23:23:57Z

/azp run Build - client packages, Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid

azure-pipelines · 2026-01-07T23:24:13Z

Azure Pipelines successfully started running 1 pipeline(s).

anthony-murphy · 2026-01-07T23:24:19Z

/azp run Build - protocol-definitions, Build - test-tools, repo-policy-check, server-routerlicious, server-gitrest, server-historian, server-gitssh

azure-pipelines · 2026-01-07T23:24:31Z

Azure Pipelines successfully started running 1 pipeline(s).

anthony-murphy · 2026-01-07T23:52:00Z

fixes AB#47951

jason-ha · 2026-01-08T18:47:54Z

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

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


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.

jason-ha · 2026-01-08T19:04:09Z

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

+		// 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",
+		);


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.

markfields · 2026-01-08T19:36:18Z

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

@@ -57,15 +57,20 @@ export class PrefetchDocumentStorageService extends DocumentStorageServiceProxy
 				return prefetchedBlobP;
 			}
 			const prefetchedBlobPFromStorage = this.internalStorageService.readBlob(blobId);


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.

## Summary - Adds explicit return type to `policies` getter in test mock to satisfy `@typescript-eslint/explicit-function-return-type` rule ## Root cause Race condition between two PRs: | Event | Time (UTC) | |-------|------------| | PR #26151 created (prefetch fix) | Jan 7, 23:12 | | PR #26149 merged (ESLint rule promotion) | Jan 8, 01:45 | | PR #26151 merged | Jan 8, 18:38 | PR #26151's CI ran before the ESLint rule was promoted in #26149. By the time #26151 was merged, the stricter linting rule was already in place, but CI wasn't re-run against the updated main branch. ## Test plan - [x] ESLint passes locally 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: anthony-murphy-agent <253562292+anthony-murphy-agent@users.noreply.github.com> Co-authored-by: anthony-murphy <anthony.murphy@microsoft.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

…fetch When getSnapshotTree() fails (e.g., network timeout), the fire-and-forget prefetch pattern `void p.then(...)` creates a derived promise that also rejects. Without a .catch() handler, this causes uncaughtException errors. This is a follow-up to PR microsoft#26151 which fixed the same issue in cachedRead() but missed this code path in getSnapshotTree(). The fix adds .catch() to the derived promise chain to prevent unhandled rejections. The original promise `p` is still returned to callers, so they receive the error properly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>

…orget 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 microsoft#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>

…fetch (#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>

- Replace `any` with proper error types in tests using GenericNetworkError and NonRetryableError from network.ts - Add test for non-retryable errors to verify cache behavior (errors without canRetry=true should remain cached, not cleared) - Skip prefetch calls if blobId is already cached to avoid unnecessary async overhead Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

@jason-ha

## Summary - Replace `any` with proper error types in tests using `GenericNetworkError` and `NonRetryableError` - Add test for non-retryable errors to verify cache behavior - Skip prefetch calls if blobId is already cached to avoid unnecessary async overhead This is a follow-up PR to address review comments from #26151: - @jason-ha: Avoid `any` and `as any` - errors should use proper types - @jason-ha: Add test for non-retryable errors - @markfields: Skip `readBlob` call if blobId is already in `prefetchCache` ## Test plan - [x] New unit test for non-retryable error caching behavior - [x] All existing prefetch tests pass (6 total) - [x] Lint passes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> --------- Co-authored-by: anthony-murphy-agent <anthony-murphy-agent@users.noreply.github.com> Co-authored-by: anthony-murphy <anthony.murphy@microsoft.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

…t#26151) ## Summary - Fix unhandled promise rejections in `PrefetchDocumentStorageService` when parallel prefetch requests fail - Add unit tests for prefetch error handling and caching behavior ## Problem The `PrefetchDocumentStorageService` fires prefetch requests using fire-and-forget pattern (`void this.cachedRead(blob)`). When these requests failed, the `.catch()` handler was re-throwing errors, creating rejected promises stored in cache that no one awaited - causing `uncaughtException` errors. Evidence from telemetry showed 98 `uncaughtException` errors from a single process within ~70ms, all originating from `readBlob` via prefetch. ## Solution Store the original promise in the cache and attach `.catch()` for side effects only (cache cleanup on retryable errors). Do not re-throw - callers who await the cached promise still receive the rejection properly. Also added a race-condition fix to avoid clearing a newer cached promise when an older one fails. ## Test plan - [x] New unit tests for `PrefetchDocumentStorageService` (4 tests covering error propagation, retry behavior, prefetch caching, and fire-and-forget failure handling) - [x] driver-utils tests pass (60 tests) - [x] Lint passes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> fixes [AB#47951](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/47951) --------- Co-authored-by: anthony-murphy-agent <253562292+anthony-murphy-agent@users.noreply.github.com> Co-authored-by: anthony-murphy <anthony.murphy@microsoft.com>

…oft#26161) ## Summary - Adds explicit return type to `policies` getter in test mock to satisfy `@typescript-eslint/explicit-function-return-type` rule ## Root cause Race condition between two PRs: | Event | Time (UTC) | |-------|------------| | PR microsoft#26151 created (prefetch fix) | Jan 7, 23:12 | | PR microsoft#26149 merged (ESLint rule promotion) | Jan 8, 01:45 | | PR microsoft#26151 merged | Jan 8, 18:38 | PR microsoft#26151's CI ran before the ESLint rule was promoted in microsoft#26149. By the time microsoft#26151 was merged, the stricter linting rule was already in place, but CI wasn't re-run against the updated main branch. ## Test plan - [x] ESLint passes locally 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: anthony-murphy-agent <253562292+anthony-murphy-agent@users.noreply.github.com> Co-authored-by: anthony-murphy <anthony.murphy@microsoft.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

…fetch (microsoft#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 microsoft#26151. After that fix was deployed, telemetry showed 349 `uncaughtException` errors from `ShreddedSummaryDocumentStorageService.readBlob` in test-service-load tinylicious tests. **Root cause:** PR microsoft#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>

anthony-murphy-agent and others added 6 commits January 7, 2026 10:27

Merge branch 'main' into fix/prefetch-unhandled-rejections

366276d

anthony-murphy requested a review from Copilot January 7, 2026 23:13

anthony-murphy closed this Jan 7, 2026

anthony-murphy reopened this Jan 7, 2026

Copilot started reviewing on behalf of anthony-murphy January 7, 2026 23:14 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

anthony-murphy approved these changes Jan 8, 2026

View reviewed changes

jzaffiro approved these changes Jan 8, 2026

View reviewed changes

anthony-murphy merged commit 06d309d into microsoft:main Jan 8, 2026
42 checks passed

anthony-murphy deleted the fix/prefetch-unhandled-rejections branch January 8, 2026 18:38

jason-ha reviewed Jan 8, 2026

View reviewed changes

anthony-murphy-agent mentioned this pull request Jan 8, 2026

fix(driver-utils): add missing return type to policies getter #26161

Merged

1 task

markfields reviewed Jan 8, 2026

View reviewed changes

anthony-murphy-agent mentioned this pull request Jan 9, 2026

fix(driver-utils): prevent unhandled rejection in getSnapshotTree prefetch #26173

Merged

3 tasks

anthony-murphy-agent mentioned this pull request Jan 13, 2026

fix(driver-utils): address PR #26151 review comments #26204

Merged

3 tasks

Conversation

anthony-murphy-agent commented Jan 7, 2026 • edited by azure-boards bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Test plan

Uh oh!

anthony-murphy-agent commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

anthony-murphy commented Jan 7, 2026

Uh oh!

Uh oh!

jason-ha Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jason-ha Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

markfields Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anthony-murphy-agent commented Jan 7, 2026 •

edited by azure-boards bot

Loading