[browser][coreCLR] Enable download retry by default and improve retry sequencing#127559
[browser][coreCLR] Enable download retry by default and improve retry sequencing#127559pavelsavara wants to merge 1 commit intodotnet:mainfrom
Conversation
pavelsavara
added
arch-wasm
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Pull request overview
This PR enables download retry by default for the CoreCLR WASM loader and adjusts retry sequencing so second-attempt downloads don’t begin until the initial set of downloads has been queued, reducing the risk of retry storms during startup.
Changes:
- Enable
enableDownloadRetryby default in loader configuration. - Gate retry attempts on a “all first-attempt downloads queued” signal, and add diagnostic retry logging.
- Re-enable and expand WASM build tests to validate download-retry recovery and progress completion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/native/libs/Common/JavaScript/loader/run.ts | Signals when initial asset download work has been queued, to gate retry attempts. |
| src/native/libs/Common/JavaScript/loader/config.ts | Defaults enableDownloadRetry to true when not explicitly configured. |
| src/native/libs/Common/JavaScript/loader/assets.ts | Adds gating for retry attempts and wraps resource-load exceptions so they aren’t silently suppressed. |
| src/mono/wasm/Wasm.Build.Tests/ModuleConfigTests.cs | Re-enables retry-related coverage and adds a dedicated retry recovery test. |
| let response: Response; | ||
| try { | ||
| response = await loadResource(asset); | ||
| } catch (err: any) { | ||
| // Strip .silent flag from download errors so they are properly reported via exit listeners | ||
| throw new Error(`Failed to load resource '${asset.name}' from '${asset.resolvedUrl}': ${err.message || err}`); | ||
| } |
There was a problem hiding this comment.
In fetchBytes, the catch block wraps and rethrows a new Error but drops the original exception details/stack (and doesn’t set cause). Since this file already uses new Error(..., { cause: err }) elsewhere (e.g., callLibraryInitializerOnRuntimeConfigLoaded), consider using the same pattern here while still stripping .silent. Also, err.message || err can yield [object Object]; prefer normalizing via err instanceof Error ? err.message : String(err) (or similar) for a stable message.
| let response: Response; | ||
| try { | ||
| response = await loadResource(asset); | ||
| } catch (err: any) { | ||
| // Strip .silent flag from download errors so they are properly reported via exit listeners | ||
| throw new Error(`Failed to load resource '${asset.name}' from '${asset.resolvedUrl}': ${err.message || err}`); | ||
| } |
There was a problem hiding this comment.
Same concern as fetchBytes: fetchText wraps and rethrows a new Error without preserving the original error as a cause, which makes debugging harder and loses the original stack. Consider adding cause: err (while still removing .silent) and normalizing the error message via err instanceof Error ? err.message : String(err) instead of err.message || err.
| // Signal that all first-attempt downloads have been queued. | ||
| // Retry logic waits on this before attempting second downloads. |
There was a problem hiding this comment.
The comment says this signals that “all first-attempt downloads have been queued”, but modulesAfterRuntimeReady JS module loads are initiated after this point when downloadOnly is false. Either adjust the comment to match what’s actually being gated (e.g., asset fetches queued via forEachResource/fetch*), or move the signal to after any additional first-attempt download work you intend to include.
| // Signal that all first-attempt downloads have been queued. | |
| // Retry logic waits on this before attempting second downloads. | |
| // Signal that all first-attempt asset fetches queued above via forEachResource/fetch* have been queued. | |
| // Retry logic waits on this before attempting second downloads for those asset fetches. |
Summary
Enables
enableDownloadRetryby default in the new coreCLR WASM loader and improves the retry strategy so that retries only begin after all first-attempt downloads have been queued. This prevents retry storms from starving initial download slots during startup.Fixes #124946
Changes
Retry sequencing (
assets.ts)allDownloadsQueuedPCSpromise completion source that gates retry attempts — no retry fires until every first-attempt download has been kicked off.resolveAllDownloadsQueued()is called fromcreateRuntime()inrun.tsafter allforEachResource/ fetch calls are queued.dotnetLogger.debugmessages (Retrying download/Retrying download (2)) so retries are observable in diagnostic tracing.Error re-wrapping (
assets.ts)fetchBytesandfetchTextnow catch errors thrown byloadResourceand re-throw a plainErrorwith the asset name and URL. This strips the.silentflag thatloadBootResourcecallbacks may attach, ensuring the failure surfaces through the normal exit/error listeners instead of being silently swallowed.Default config (
config.ts)enableDownloadRetrytotruewhen not explicitly configured, matching the legacy loader's default.Tests (
ModuleConfigTests.cs)[InlineData(true)]case forDownloadProgressFinishes(was disabled via #124946).DownloadRetryRecoversFromFailure— a dedicated[Fact]that publishes in Release, triggers simulated assembly download failures, and asserts that: