fix(driver-utils): prevent unhandled rejections in prefetch#26142
Closed
anthony-murphy-agent wants to merge 6 commits intomicrosoft:mainfrom
Closed
fix(driver-utils): prevent unhandled rejections in prefetch#26142anthony-murphy-agent wants to merge 6 commits intomicrosoft:mainfrom
anthony-murphy-agent wants to merge 6 commits intomicrosoft:mainfrom
Conversation
Contributor
Author
|
@microsoft-github-policy-service agree company="Microsoft" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in PrefetchDocumentStorageService where fire-and-forget prefetch requests created unhandled promise rejections when failing. The fix restructures error handling to prevent unhandled rejections while still properly propagating errors to awaiting callers.
Key changes:
- Modified error handling in
cachedRead()to attach.catch()handlers for side effects only (cache cleanup) without re-throwing - Added race condition protection when clearing cache entries
- Created comprehensive unit test suite with 4 new tests covering error propagation, retry behavior, caching, and fire-and-forget failures
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/loader/driver-utils/src/prefetchDocumentStorageService.ts |
Fixed unhandled rejection bug by restructuring promise error handling - stores original promise in cache and attaches non-throwing .catch() for cleanup |
packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts |
Added comprehensive test suite with mock storage service and 4 test cases validating the fix and existing behavior |
CLAUDE.md |
Added comprehensive project documentation covering repository structure, build system, testing, and driver architecture |
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>
anthony-murphy-agent
force-pushed
the
fix/prefetch-unhandled-rejections
branch
from
12ebbcc to
f9b4fe9
Compare
- 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>
anthony-murphy-agent
force-pushed
the
fix/prefetch-unhandled-rejections
branch
from
f9b4fe9 to
107f096
Compare
packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts
Outdated
Show resolved
Hide resolved
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
force-pushed
the
fix/prefetch-unhandled-rejections
branch
from
0b2b707 to
854610b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PrefetchDocumentStorageServicewhen parallel prefetch requests failProblem
The
PrefetchDocumentStorageServicefires 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 - causinguncaughtExceptionerrors.Evidence from telemetry showed 98
uncaughtExceptionerrors from a single process within ~70ms, all originating fromreadBlobvia 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
PrefetchDocumentStorageService(4 tests covering error propagation, retry behavior, prefetch caching, and fire-and-forget failure handling)🤖 Generated with Claude Code