Fixed: Launcher freezing on startup when fetching mod update metadata#919
Open
drewmcelhany wants to merge 1 commit into
Open
Fixed: Launcher freezing on startup when fetching mod update metadata#919drewmcelhany wants to merge 1 commit into
drewmcelhany wants to merge 1 commit into
Conversation
WalkthroughThe update summary now provides asynchronous metadata retrieval with cached results and a synchronous compatibility wrapper. The launcher fetches this metadata before opening the update dialog. A new dialog view model constructor accepts the precomputed updates and initializes summary, size, selection, and download state fields. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
source/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs (1)
70-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
ConfigureAwait(false)for library-internal awaits.This is reusable library code (
Reloaded.Mod.Loader.Update) whose async method may be awaited directly (Update.cs) or blocked-on via the sync wrapper. Per general async guidance, In library code, you can never use the context, so you should always use ConfigureAwait(false) on every await to avoid needless continuation marshaling back to a captured context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs` around lines 70 - 92, Update every await in the affected ModUpdateSummary async flow, including GetDownloadFileSizeAsync, GetReleaseMetadataAsync, and DownloadNuspecReaderAsync, to use ConfigureAwait(false). Apply this consistently to all library-internal awaits in the containing method without changing exception handling or result-processing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/Dialog/ModUpdateDialogViewModel.cs`:
- Around line 44-56: Update the public ModUpdateDialogViewModel constructor to
handle an empty updateInfo array before accessing UpdateInfo[0]. Preserve the
existing initialization for non-empty arrays and establish an appropriate
no-selection state when no updates are provided.
In `@source/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs`:
- Around line 64-95: Guard the NuGet changelog fallback in the update-info
computation with its own try/catch, covering resolver settings access,
repository creation, nuspec download, and release-notes extraction so failures
leave changelog unset and do not abort processing other mods. Also replace the
direct IPackageResolverDownloadSize cast with a safe capability check and only
fetch download size when the resolver implements that interface, preserving the
existing default size when unsupported or failing.
---
Nitpick comments:
In `@source/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs`:
- Around line 70-92: Update every await in the affected ModUpdateSummary async
flow, including GetDownloadFileSizeAsync, GetReleaseMetadataAsync, and
DownloadNuspecReaderAsync, to use ConfigureAwait(false). Apply this consistently
to all library-internal awaits in the containing method without changing
exception handling or result-processing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b69d7c-05c6-4f7c-87a6-c4195c701234
📒 Files selected for processing (3)
source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/Dialog/ModUpdateDialogViewModel.cssource/Reloaded.Mod.Launcher.Lib/Update.cssource/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs
Comment on lines
+44
to
56
| /// <summary> | ||
| /// Creates the ViewModel using precomputed update info. | ||
| /// Prefer this overload; it performs no network I/O and is safe on the UI thread. | ||
| /// </summary> | ||
| public ModUpdateDialogViewModel(Updater updater, ModUpdateSummary summary, ModUpdate[] updateInfo) | ||
| { | ||
| Updater = updater; | ||
| Summary = summary; | ||
| UpdateInfo = Summary.GetUpdateInfo(); | ||
| UpdateInfo = updateInfo; | ||
| TotalSize = UpdateInfo.Sum(x => x.UpdateSize); | ||
| SelectedUpdate = UpdateInfo[0]; | ||
| CanDownload = true; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against an empty updateInfo array.
UpdateInfo[0] will throw if updateInfo is empty. Currently safe because the only caller (Update.cs) checks HasUpdates() first, but this is a public constructor and that invariant isn't enforced here.
🛡️ Proposed guard
public ModUpdateDialogViewModel(Updater updater, ModUpdateSummary summary, ModUpdate[] updateInfo)
{
+ if (updateInfo.Length == 0)
+ throw new ArgumentException("Update info must contain at least one entry.", nameof(updateInfo));
+
Updater = updater;
Summary = summary;
UpdateInfo = updateInfo;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <summary> | |
| /// Creates the ViewModel using precomputed update info. | |
| /// Prefer this overload; it performs no network I/O and is safe on the UI thread. | |
| /// </summary> | |
| public ModUpdateDialogViewModel(Updater updater, ModUpdateSummary summary, ModUpdate[] updateInfo) | |
| { | |
| Updater = updater; | |
| Summary = summary; | |
| UpdateInfo = Summary.GetUpdateInfo(); | |
| UpdateInfo = updateInfo; | |
| TotalSize = UpdateInfo.Sum(x => x.UpdateSize); | |
| SelectedUpdate = UpdateInfo[0]; | |
| CanDownload = true; | |
| } | |
| /// <summary> | |
| /// Creates the ViewModel using precomputed update info. | |
| /// Prefer this overload; it performs no network I/O and is safe on the UI thread. | |
| /// </summary> | |
| public ModUpdateDialogViewModel(Updater updater, ModUpdateSummary summary, ModUpdate[] updateInfo) | |
| { | |
| if (updateInfo.Length == 0) | |
| throw new ArgumentException("Update info must contain at least one entry.", nameof(updateInfo)); | |
| Updater = updater; | |
| Summary = summary; | |
| UpdateInfo = updateInfo; | |
| TotalSize = UpdateInfo.Sum(x => x.UpdateSize); | |
| SelectedUpdate = UpdateInfo[0]; | |
| CanDownload = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/Dialog/ModUpdateDialogViewModel.cs`
around lines 44 - 56, Update the public ModUpdateDialogViewModel constructor to
handle an empty updateInfo array before accessing UpdateInfo[0]. Preserve the
existing initialization for non-empty arrays and establish an appropriate
no-selection state when no updates are provided.
Comment on lines
+64
to
95
| var resolver = ((IPackageResolverDownloadSize)resultPairs.Manager.Resolver); | ||
| var updateSize = (long)0; | ||
| string? changelog = null; | ||
|
|
||
| try | ||
| { | ||
| var resultPairs = ManagerModResultPairs[x]; | ||
| var modName = resultPairs.ModTuple.Config.ModName; | ||
| var modId = resultPairs.ModTuple.Config.ModId; | ||
| var oldVersion = resultPairs.ModTuple.Config.ModVersion; | ||
| var newVersion = resultPairs.Result.LastVersion; | ||
| var resolver = ((IPackageResolverDownloadSize)resultPairs.Manager.Resolver); | ||
| var updateSize = (long)0; | ||
| string? changelog = null; | ||
| updateSize = await resolver.GetDownloadFileSizeAsync(newVersion!, resultPairs.ModTuple.GetVerificationInfo()); | ||
| } | ||
| catch (Exception) { /* Ignored */ } | ||
|
|
||
| // Get changelog from supported resolver. | ||
| if (resolver is IPackageResolverGetLatestReleaseMetadata getMetadata) | ||
| { | ||
| try | ||
| { | ||
| updateSize = await resolver.GetDownloadFileSizeAsync(newVersion!, resultPairs.ModTuple.GetVerificationInfo()); | ||
| var releaseMetadata = await getMetadata.GetReleaseMetadataAsync(default); | ||
| var extraData = releaseMetadata?.GetExtraData<ReleaseMetadataExtraData>(); | ||
| if (extraData != null) | ||
| changelog = extraData.Changelog; | ||
| } | ||
| catch (Exception) { /* Ignored */ } | ||
| } | ||
|
|
||
| // Get changelog from supported resolver. | ||
| if (resolver is IPackageResolverGetLatestReleaseMetadata getMetadata) | ||
| { | ||
| try | ||
| { | ||
| var releaseMetadata = await getMetadata.GetReleaseMetadataAsync(default); | ||
| var extraData = releaseMetadata?.GetExtraData<ReleaseMetadataExtraData>(); | ||
| if (extraData != null) | ||
| changelog = extraData.Changelog; | ||
| } | ||
| catch (Exception) { /* Ignored */ } | ||
| } | ||
|
|
||
| // NuGet has special case, since it doesn't support release metadata but supports changelogs in nuspec. | ||
| if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver) | ||
| { | ||
| var copiedSettings = nugetResolver.GetResolverSettings(); | ||
| var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl); | ||
| var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!)); | ||
| if (reader != null) | ||
| changelog = reader?.GetReleaseNotes(); | ||
| } | ||
|
|
||
| _updates[x] = new ModUpdate(modId, NuGetVersion.Parse(oldVersion), newVersion!, updateSize, changelog, modName); | ||
| // NuGet has special case, since it doesn't support release metadata but supports changelogs in nuspec. | ||
| if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver) | ||
| { | ||
| var copiedSettings = nugetResolver.GetResolverSettings(); | ||
| var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl); | ||
| var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!)); | ||
| if (reader != null) | ||
| changelog = reader?.GetReleaseNotes(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded external calls can abort the whole update-info computation for every mod.
Unlike the two neighboring paths (download-size fetch, release-metadata fetch), the NuGet nuspec fallback (lines 88-95) has no try/catch. A single mod's nuspec download failure (network hiccup, missing nuspec, etc.) throws out of the loop, propagates through GetUpdateInfoAsync, and is caught by the outer try/catch in Update.cs#CheckForModUpdatesAsync, replacing the update dialog with a generic error message — even though every other mod's update info was already computed successfully. The direct cast to IPackageResolverDownloadSize at line 64 (also unguarded) carries the same risk if any resolver implementation doesn't support that interface.
🛡️ Proposed fix to wrap the nuspec fallback consistently with the other paths
if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver)
{
- var copiedSettings = nugetResolver.GetResolverSettings();
- var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl);
- var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!));
- if (reader != null)
- changelog = reader?.GetReleaseNotes();
+ try
+ {
+ var copiedSettings = nugetResolver.GetResolverSettings();
+ var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl);
+ var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!));
+ if (reader != null)
+ changelog = reader?.GetReleaseNotes();
+ }
+ catch (Exception) { /* Ignored */ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var resolver = ((IPackageResolverDownloadSize)resultPairs.Manager.Resolver); | |
| var updateSize = (long)0; | |
| string? changelog = null; | |
| try | |
| { | |
| var resultPairs = ManagerModResultPairs[x]; | |
| var modName = resultPairs.ModTuple.Config.ModName; | |
| var modId = resultPairs.ModTuple.Config.ModId; | |
| var oldVersion = resultPairs.ModTuple.Config.ModVersion; | |
| var newVersion = resultPairs.Result.LastVersion; | |
| var resolver = ((IPackageResolverDownloadSize)resultPairs.Manager.Resolver); | |
| var updateSize = (long)0; | |
| string? changelog = null; | |
| updateSize = await resolver.GetDownloadFileSizeAsync(newVersion!, resultPairs.ModTuple.GetVerificationInfo()); | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| // Get changelog from supported resolver. | |
| if (resolver is IPackageResolverGetLatestReleaseMetadata getMetadata) | |
| { | |
| try | |
| { | |
| updateSize = await resolver.GetDownloadFileSizeAsync(newVersion!, resultPairs.ModTuple.GetVerificationInfo()); | |
| var releaseMetadata = await getMetadata.GetReleaseMetadataAsync(default); | |
| var extraData = releaseMetadata?.GetExtraData<ReleaseMetadataExtraData>(); | |
| if (extraData != null) | |
| changelog = extraData.Changelog; | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| } | |
| // Get changelog from supported resolver. | |
| if (resolver is IPackageResolverGetLatestReleaseMetadata getMetadata) | |
| { | |
| try | |
| { | |
| var releaseMetadata = await getMetadata.GetReleaseMetadataAsync(default); | |
| var extraData = releaseMetadata?.GetExtraData<ReleaseMetadataExtraData>(); | |
| if (extraData != null) | |
| changelog = extraData.Changelog; | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| } | |
| // NuGet has special case, since it doesn't support release metadata but supports changelogs in nuspec. | |
| if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver) | |
| { | |
| var copiedSettings = nugetResolver.GetResolverSettings(); | |
| var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl); | |
| var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!)); | |
| if (reader != null) | |
| changelog = reader?.GetReleaseNotes(); | |
| } | |
| _updates[x] = new ModUpdate(modId, NuGetVersion.Parse(oldVersion), newVersion!, updateSize, changelog, modName); | |
| // NuGet has special case, since it doesn't support release metadata but supports changelogs in nuspec. | |
| if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver) | |
| { | |
| var copiedSettings = nugetResolver.GetResolverSettings(); | |
| var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl); | |
| var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!)); | |
| if (reader != null) | |
| changelog = reader?.GetReleaseNotes(); | |
| } | |
| var resolver = ((IPackageResolverDownloadSize)resultPairs.Manager.Resolver); | |
| var updateSize = (long)0; | |
| string? changelog = null; | |
| try | |
| { | |
| updateSize = await resolver.GetDownloadFileSizeAsync(newVersion!, resultPairs.ModTuple.GetVerificationInfo()); | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| // Get changelog from supported resolver. | |
| if (resolver is IPackageResolverGetLatestReleaseMetadata getMetadata) | |
| { | |
| try | |
| { | |
| var releaseMetadata = await getMetadata.GetReleaseMetadataAsync(default); | |
| var extraData = releaseMetadata?.GetExtraData<ReleaseMetadataExtraData>(); | |
| if (extraData != null) | |
| changelog = extraData.Changelog; | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| } | |
| // NuGet has special case, since it doesn't support release metadata but supports changelogs in nuspec. | |
| if (string.IsNullOrEmpty(changelog) && resolver is NuGetUpdateResolver nugetResolver) | |
| { | |
| try | |
| { | |
| var copiedSettings = nugetResolver.GetResolverSettings(); | |
| var repository = NugetRepository.FromSourceUrl(copiedSettings.NugetRepository!.SourceUrl); | |
| var reader = await repository.DownloadNuspecReaderAsync(new PackageIdentity(copiedSettings.PackageId, newVersion!)); | |
| if (reader != null) | |
| changelog = reader?.GetReleaseNotes(); | |
| } | |
| catch (Exception) { /* Ignored */ } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/Reloaded.Mod.Loader.Update/Structures/ModUpdateSummary.cs` around
lines 64 - 95, Guard the NuGet changelog fallback in the update-info computation
with its own try/catch, covering resolver settings access, repository creation,
nuspec download, and release-notes extraction so failures leave changelog unset
and do not abort processing other mods. Also replace the direct
IPackageResolverDownloadSize cast with a safe capability check and only fetch
download size when the resolver implements that interface, preserving the
existing default size when unsupported or failing.
Sewer56
reviewed
Jul 23, 2026
| /// <summary> | ||
| /// Legacy constructor. Performs blocking network I/O; do not call from the UI thread. | ||
| /// </summary> | ||
| public ModUpdateDialogViewModel(Updater updater, ModUpdateSummary summary) |
Member
There was a problem hiding this comment.
This is internal code, so no need for a backwards compatible overload.
Member
|
Aight, I see the LLM shenanigans going on here. Though it remains a curious case why an update check deadlocked in the first place. |
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.
Fixes #910
Root cause:
ModUpdateSummary.GetUpdateInfo()performs per-mod network requests(download sizes, changelogs, nuspec fetches) with no timeouts, wrapped in
Task.Run(...).Wait(). It was called from theModUpdateDialogViewModelconstructor,which runs on the UI thread inside
SynchronizationContext.Send— so any slow orunresponsive package endpoint froze the launcher indefinitely on startup.
Full WinDbg analysis of a hang dump is in #910.
Change:
ModUpdateSummary: addedGetUpdateInfoAsync(); the syncGetUpdateInfo()remainsas a wrapper for compatibility.
Update.CheckForModUpdatesAsync: awaits the metadata fetch on the background threadbefore marshaling to the UI thread.
ModUpdateDialogViewModel: new overload accepts precomputed update info; the dialogconstructor now does no I/O.
Verified: Full solution builds (VS Community, v142 toolset for the bootstrapper).
Launched the built Reloaded-II.exe on the previously-freezing setup (Win11 26100,
mods with pending updates): startup completes and the update dialog appears normally.
All build warnings are pre-existing on master.
Note: the underlying requests still lack timeouts — a hung endpoint now stalls the
background check silently rather than the UI. Left out of this PR to keep it minimal;
happy to add a cancellation ceiling as a follow-up if wanted.