-
Notifications
You must be signed in to change notification settings - Fork 562
fix(driver-utils): prevent unhandled rejection in getSnapshotTree prefetch #26173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(driver-utils): prevent unhandled rejection in getSnapshotTree prefetch #26173
Conversation
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an unhandled promise rejection bug in PrefetchDocumentStorageService.getSnapshotTree() by adding a .catch() handler to the fire-and-forget prefetch pattern. This is a follow-up to PR #26151, which fixed a similar issue in cachedRead() but missed this code path.
- Adds
.catch()handler to prevent unhandled rejections whengetSnapshotTree()fails - Adds unit test to verify the fix and ensure errors are still properly propagated to callers
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 | Adds .catch() handler to fire-and-forget prefetch pattern in getSnapshotTree() to prevent unhandled rejections while still propagating errors to callers |
| packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts | Adds mock failure flags and test case to verify getSnapshotTree() failures don't cause unhandled rejections |
…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 <[email protected]>
Co-Authored-By: anthony-murphy <[email protected]>
907fe32 to
a3e582d
Compare
packages/loader/driver-utils/src/prefetchDocumentStorageService.ts
Outdated
Show resolved
Hide resolved
Address review comment - void is no longer needed since .catch() handles the promise rejection, so there's no floating promise issue. Co-Authored-By: Claude Opus 4.5 <[email protected]> Co-Authored-By: anthony-murphy <[email protected]>
|
Azure Pipelines commands (copy and post separately, limit of 10 at a time): |
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Summary
PrefetchDocumentStorageServicefire-and-forget callsProblem
This is a follow-up to PR #26151. After that fix was deployed, telemetry showed 349
uncaughtExceptionerrors fromShreddedSummaryDocumentStorageService.readBlobin test-service-load tinylicious tests.Root cause: PR #26151 attached
.catch()to the inner promise incachedRead(), but that only handles ONE promise chain. The fire-and-forget callers create a DIFFERENT chain:void this.cachedRead(blob)-cachedReadis 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.void p.then(...)ingetSnapshotTree()- creates a derived promise that also rejects whenprejects.Solution
Fix all three fire-and-forget patterns:
Test plan
getSnapshotTreefailure scenario🤖 Generated with Claude Code
Co-Authored-By: anthony-murphy [email protected]