Batch 4: Download subsystem integration — replace old DownloadManager#368
Merged
Conversation
- Create DownloadPlanBuilder: cross-version selection, version chain, frozen package filtering, MinClientVersion compatibility check - Create HttpDownloadSource: bridges VersionService → IDownloadSource, validates both client and upgrade versions in one call - Refactor DefaultDownloadOrchestrator: accept DownloadPlan instead of raw URLs, integrate SHA256 verification via DefaultDownloadPipeline - Update IDownloadOrchestrator interface to accept DownloadPlan - Refactor ClientUpdateStrategy: replace old DownloadManager/DownloadTask with HttpDownloadSource + DownloadPlanBuilder + DefaultDownloadOrchestrator - Mark DownloadManager as [Obsolete] — still used by OSS and Silent (to be removed in later batches) Closes #367
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a “Batch 4” refactor of the C# download/update path in GeneralUpdate.Core, moving ClientUpdateStrategy off the legacy DownloadManager/DownloadTask approach and onto the newer layered download abstractions (IDownloadSource → DownloadPlan → IDownloadOrchestrator), with added SHA256 verification in the orchestrator and a new DownloadPlanBuilder intended to support cross-version package selection.
Changes:
- Refactors
ClientUpdateStrategyto useHttpDownloadSource,DownloadPlanBuilder, andDefaultDownloadOrchestrator(and removes the legacyDownloadAsync()method). - Introduces
HttpDownloadSource(new file) andDownloadPlanBuilder(new file). - Updates
IDownloadOrchestrator+DefaultDownloadOrchestratorto execute aDownloadPlanand perform SHA256 verification.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs | Switches client workflow to new download abstractions and removes legacy DownloadAsync() path. |
| src/c#/GeneralUpdate.Core/Download/Sources/HttpDownloadSource.cs | Adds an HTTP-backed IDownloadSource that calls the version validation API and maps to DownloadAsset. |
| src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs | Updates orchestrator to download from a DownloadPlan and verify SHA256 via DefaultDownloadPipeline. |
| src/c#/GeneralUpdate.Core/Download/DownloadPlanBuilder.cs | Adds logic to build an ordered download plan (cross-version selection + version chain). |
| src/c#/GeneralUpdate.Core/Download/DownloadManager.cs | Marks legacy DownloadManager as [Obsolete]. |
| src/c#/GeneralUpdate.Core/Download/Abstractions/IDownloadOrchestrator.cs | Changes orchestrator contract to accept DownloadPlan instead of URL lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+134
to
136
| // Dispatch update info event | ||
| var updateInfoArgs = new UpdateInfoEventArgs(null); | ||
| EventManager.Instance.Dispatch(this, updateInfoArgs); |
Comment on lines
+176
to
180
| // Build process info for the upgrade process | ||
| _configInfo.ProcessInfo = JsonSerializer.Serialize( | ||
| ConfigurationMapper.MapToProcessInfo( | ||
| _configInfo, new List<VersionInfo>(), | ||
| BlackListManager.Instance.BlackFormats.ToList(), |
Comment on lines
+190
to
+200
| // Download via orchestrator (replaces old DownloadManager) | ||
| GeneralTracer.Info($"ClientUpdateStrategy: downloading {downloadPlan.Assets.Count} asset(s)."); | ||
| var httpClient = new System.Net.Http.HttpClient(); | ||
| try | ||
| { | ||
| case true when _configInfo.IsMainUpdate: | ||
| GeneralTracer.Info("ClientUpdateStrategy: both upgrade+main -- downloading and executing."); | ||
| await DownloadAsync(); | ||
| await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false); | ||
| await _osStrategy.ExecuteAsync(); | ||
| await SafeOnBeforeStartAppAsync(hooksCtx).ConfigureAwait(false); | ||
| _osStrategy.StartApp(); | ||
| break; | ||
| case true when !_configInfo.IsMainUpdate: | ||
| GeneralTracer.Info("ClientUpdateStrategy: upgrade-only -- downloading and executing."); | ||
| await DownloadAsync(); | ||
| await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false); | ||
| await _osStrategy.ExecuteAsync(); | ||
| break; | ||
| case false when _configInfo.IsMainUpdate: | ||
| GeneralTracer.Info("ClientUpdateStrategy: main-only -- starting updater."); | ||
| await SafeOnBeforeStartAppAsync(hooksCtx).ConfigureAwait(false); | ||
| _osStrategy.StartApp(); | ||
| break; | ||
| var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(httpClient); | ||
| await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false); | ||
| } | ||
| finally | ||
| { | ||
| httpClient.Dispose(); |
Comment on lines
+76
to
+85
| private static DownloadAsset MapVersionInfo(VersionInfo v) | ||
| { | ||
| return new DownloadAsset( | ||
| Name: v.Name ?? v.Version ?? "unknown", | ||
| Url: v.Url ?? string.Empty, | ||
| Size: v.Size ?? 0, | ||
| SHA256: v.Hash, | ||
| Version: v.Version ?? "0.0.0", | ||
| IsForcibly: v.IsForcibly == true | ||
| ); |
Comment on lines
43
to
46
| var sw = Stopwatch.StartNew(); | ||
| var results = new List<DownloadResult>(); | ||
| var sem = new SemaphoreSlim(maxConcurrency); | ||
| long totalBytes = 0; |
Comment on lines
+60
to
+70
| private static List<DownloadAsset> BuildVersionChain(IEnumerable<DownloadAsset> assets, string currentVersion) | ||
| { | ||
| var current = ParseVersion(currentVersion); | ||
|
|
||
| return assets | ||
| .Where(a => | ||
| { | ||
| var pv = ParseVersion(a.Version); | ||
| if (pv == null) return false; | ||
| return pv > current; | ||
| }) |
This was referenced May 24, 2026
JusterZhu
added a commit
that referenced
this pull request
May 24, 2026
Fixes from PR #368 (Batch 4): 1. UpdateInfoEventArgs: make VersionRespDTO nullable to avoid null ref 2. ClientUpdateStrategy: build VersionInfo list from DownloadAssets instead of passing empty list to ProcessInfo constructor 3. ClientUpdateStrategy: use injected _orchestrator if available, fall back to creating one with HttpClient 5. DefaultDownloadOrchestrator: ensure destDir exists (Directory.CreateDirectory) and dispose SemaphoreSlim (using) 6. DownloadPlanBuilder: guard against invalid currentVersion string Fixes from PR #380 (Batch 10): 8. OSSUpdateStrategy: set zip filename to match Decompress() expectations 9. OSSUpdateStrategy: throw clear exception when URL is null/empty 10. OSSUpdateStrategy: set HttpClient.Timeout to 60s to match old behavior Closes #386
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.
Summary
Core download refactoring batch. Replaces the old DownloadManager/DownloadTask pattern in ClientUpdateStrategy with the new layered download abstractions, and creates the cross-version package selection logic.
New Files
DownloadPlanBuilder
HttpDownloadSource
Refactored
DefaultDownloadOrchestrator
IDownloadOrchestrator
ClientUpdateStrategy
DownloadManager
Build
Closes #367