fix: handle dangling promise rejection in external ingest upload (fixes #317056)#317064
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
Conversation
#317056) The p.finally() call creates a new promise whose rejection is never caught. When a 404 rejects the upload promise p, the .finally() return also rejects as an unhandled rejection hitting telemetry. The actual error is properly handled via Promise.all(uploading) — this just prevents the duplicate unhandled rejection from the .finally() chain. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
🔧 Error Fix
Summary
Error:
Ingest not found (404) for document— an unhandled promise rejection from the external ingest upload pool in the Copilot extension's workspace chunk search.Root Cause: In
externalIngestClient.ts, when a document upload returns HTTP 404, the code throws at line 425. The upload promisepis correctly tracked in theuploadingset and handled viaPromise.all(uploading). However,p.finally(...)(line 432) creates a new promise whose rejection is never caught. Whenprejects, the.finally()return promise also rejects — and since no handler is attached to it, Node.js fires anunhandledRejectionevent that reaches error telemetry.Fixes #317056
Recommended reviewer:
@mjbvzCulprit Commit
5137aa4525bcdef5f337d85e9f0078a26ae62585@mjbvzthrowon 404 at line 425, which rejects promisep. Combined with the pre-existingp.finally(...)pattern that discards its return value, this created a dangling rejected promise.Code Flow
sequenceDiagram participant PI as performIngestion participant IIFE as uploadIIFE participant FIN as finallyReturn participant PA as PromiseAll participant TEL as Telemetry PI->>IIFE: start upload IIFE->>IIFE: makeRequest returns 404 IIFE->>IIFE: throw ExternalIngestRequestError line 425 IIFE-->>FIN: finally callback runs rejected promise IIFE-->>PA: rejection propagates via uploading set FIN-->>TEL: unhandled rejection no catch on finally returnAffected Files
extensions/copilot/src/platform/workspaceChunkSearch/node/codeSearch/externalIngestClient.ts.finally()Repro Steps
pp.finally(...)propagates the rejection to its return promise.finally()return is never caught, an unhandled rejection firesHow the Fix Works
Chosen approach (
externalIngestClient.ts): Added.catch(() => {})to the end of thep.finally(...)chain. This prevents the dangling promise from firing an unhandled rejection. The actual error is still correctly propagated viaPromise.all(uploading)through the originalpreference in theuploadingset — the fix is at the data producer of the unhandled rejection (the uncaught.finally()return), not at the crash site.Recommended Owner
@mjbvz— author of the culprit commit and primary maintainer of the external ingest client.