Fix unhandled ObjectDisposedException from the upload progress callback - #155
Merged
isourabh merged 1 commit intoAug 20, 2026
Conversation
The Azure blob upload progress handler read fileStream.Length on every tick. Progress<T> dispatches its handlers on the thread pool, so a queued callback could run after UploadFileAsync had returned or thrown and the `using` had already disposed the stream. get_Length() then threw ObjectDisposedException on a thread-pool thread, outside the caller's try/catch in IStorePackagedAPIExtensions, taking the whole process down with exit code 1. Capture the length once before the upload so the callback never touches the stream, and guard the handler body so a late callback can never terminate the process. The guard also covers the progress consumer itself: the CLI passes a Spectre.Console ProgressTask, which can throw once the progress display has been torn down. Fixes #154 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f872603-9e85-44d2-a627-788a2806492a
Alexandre Zollinger Chohfi (azchohfi)
requested a balanced review from Copilot
August 18, 2026 23:28
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 18, 2026 23:28
View session
There was a problem hiding this comment.
Pull request overview
Prevents late Azure upload progress callbacks from crashing the NativeAOT CLI after the file stream is disposed.
Changes:
- Captures file length before upload and safely handles progress callbacks.
- Adds regression tests for disposal, percentages, empty files, and reporting exceptions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
MSStore.CLI/Services/AzureBlobManager.cs |
Makes upload progress callbacks independent of stream lifetime. |
MSStore.CLI.UnitTests/AzureBlobManagerUnitTests.cs |
Tests progress callback safety and calculations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
isourabh
approved these changes
Aug 20, 2026
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.
Fixes #154
Problem
AzureBlobManager.UploadFileAsyncbuilt its Azure upload progress handler as a lambda that readfileStream.Lengthon every tick:Progress<T>dispatches its handlers asynchronously onto the thread pool (the CLI has noSynchronizationContext). WhenUploadAsyncreturns — or throws — theusingdisposes the stream while callbacks can still be queued. A queued callback then callsget_Length()on a closed stream and throwsObjectDisposedExceptionon a thread-pool thread, outside the caller'stry/catchinIStorePackagedAPIExtensions, so it is unhandled and kills the process:The crash is at 0%, after the caller's
catchprinted — so the ordering is:UploadAsyncfails fast →usingdisposes →catchhandles the upload failure → a still-queued callback takes the process down. Becausepublishhas already runDeleting existing Submissionby that point, this is disruptive in CI.The race is pre-existing, but #133's move to .NET 10 with native AOT / single-file changed thread-pool scheduling and disposal timing enough to make it deterministic in v0.4.0.
Fix
Capture the length once, before the upload, so the callback never touches the stream, and guard the handler body so a late callback can never escape onto the thread pool.
The guard is not redundant:
progressis a Spectre.ConsoleProgressTask, and reporting into it after the progress display has been torn down can also throw from a thread-pool thread. ThetotalBytes > 0check keepsNaN/Infinityout of the display for a 0-byte package.Upload behaviour itself is unchanged — a failed upload still surfaces through the existing
catchand returns-1.Scope
AzureBlobManageris the only site with this pattern.FileDownloader.DownloadAsyncandPWABuilderClient.GenerateZipAsyncreport progress synchronously on the awaiting thread and do not capture a disposed stream.Verification
UploadFileAsync(sameAzure.Storage.Blobs12.29.1) pointed at a local blob endpoint that accepts part of the body then aborts: the reportedObjectDisposedExceptionfromProgress<T>.InvokeHandlersfires on every run, 13–18 callbacks escaping after thecatchhad already completed.MSStore.CLI.UnitTests/AzureBlobManagerUnitTests.cslock in the invariant that the callback does not touch the file stream, plus percentage correctness, the 0-byte case, and the consumer-throws case.net10.0andnet10.0-windows10.0.17763.0(the CI TFM); remaining failures are pre-existing and were confirmed against a clean baseline.Side note for anyone reproducing this: on .NET 10 CoreCLR an unhandled thread-pool exception no longer terminates the process by default. The shipped CLI is NativeAOT (
PublishAot=true), which fail-fasts — which is why the reporter sees exit code 1.