fix: resolve P0 pipeline bugs and improve robustness - #436
Merged
JusterZhu merged 2 commits intoMay 26, 2026
Conversation
….Core
**P0 - MacStrategy Pipeline order mismatched Windows/Linux**
MacStrategy.BuildPipeline registered middleware in wrong LIFO order
(Hash→Compress→Patch), causing actual execution Patch→Compress→Hash.
Fixed to match Windows/Linux: Patch→Compress→Hash (LIFO execution:
Hash→Compress→Patch). Also added missing try/catch/finally with
GracefulExit to StartApp.
**P0 - PatchMiddleware unable to receive injected IBinaryDiffer**
PipelineBuilder uses new() to create middleware, so the parametrized
constructor was never invoked by the pipeline system. Differ was always
null, making differential patching non-functional regardless of
Bootstrap.BinaryDiffer<T>() configuration.
Fix: differ now flows through PipelineContext ("BinaryDiffer" key),
set by AbstractStrategy.CreatePipelineContext from its new Differ
property. Bootstrap injects differ into strategies after resolving
extensions.
**P1 - OSSUpgrade exception path lacked process exit**
ExecuteUpgradeAsync re-threw exceptions without terminating the
process. Added finally block with GracefulExit.CurrentProcessAsync(),
and replaced throw with EventManager dispatch for consistent
error handling.
**P1 - Download semaphore had no timeout**
SemaphoreSlim.WaitAsync in DefaultDownloadOrchestrator could block
indefinitely if a hung download held the slot. Added 5-minute timeout.
**P2 - Configinfo.Validate() was never called**
SetConfig now calls configInfo.Validate() before mapping to catch
invalid configuration early with clear error messages.
**P2 - Rollback/restore not wired into update failure path**
Added TryRollback() to AbstractStrategy, called when pipeline
execution fails for a version. Restores from BackupDirectory if
it exists.
**P2 - GetTempDirectory timestamp only day-level precision**
Added millisecond precision and ProcessId to prevent collisions
between concurrent update processes.
**Also fixed:**
- OSSUpdateStrategy zipName construction ("MyAppzip.zip" double-zip)
- GlobalConfigInfo orphaned DriverDirectory XML docs
- Updated PatchMiddlewareTests for new PipelineContext approach
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets robustness and correctness issues in GeneralUpdate.Core’s update pipeline, focusing on middleware ordering (macOS), differential patch injection, failure/exit behavior, and a handful of operational hardening fixes.
Changes:
- Align macOS pipeline middleware registration order with Windows/Linux and improve updater shutdown behavior after
StartApp(). - Rework
PatchMiddlewareto resolveIBinaryDifferfromPipelineContext, update bootstrap wiring, and adjust tests accordingly. - Add additional robustness: config validation on
SetConfig, rollback attempt on pipeline failure, download semaphore timeout, and more collision-resistant temp directory names.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreTest/Pipeline/PatchMiddlewareTests.cs | Updates tests to reflect differ resolution via PipelineContext instead of constructor injection. |
| src/c#/GeneralUpdate.Core/Strategy/UpgradeUpdateStrategy.cs | Adds SetDiffer() to push an injected differ down to the OS strategy. |
| src/c#/GeneralUpdate.Core/Strategy/OSSUpdateStrategy.cs | Ensures OSS upgrade path reports errors and exits the process via GracefulExit in finally. |
| src/c#/GeneralUpdate.Core/Strategy/MacStrategy.cs | Fixes middleware registration order and aligns StartApp() shutdown pattern with other OS strategies. |
| src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs | Adds SetDiffer() to push an injected differ down to the OS strategy. |
| src/c#/GeneralUpdate.Core/Strategy/AbstractStrategy.cs | Adds Differ property, injects it into PipelineContext, and attempts rollback on per-version pipeline failure. |
| src/c#/GeneralUpdate.Core/Pipeline/PatchMiddleware.cs | Resolves IBinaryDiffer from PipelineContext (key BinaryDiffer) and skips patching if absent. |
| src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs | Improves temp directory uniqueness by adding time + PID to the generated name. |
| src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs | Adds a timeout to SemaphoreSlim.WaitAsync to avoid indefinite blocking. |
| src/c#/GeneralUpdate.Core/Configuration/GlobalConfigInfo.cs | Removes orphaned/duplicated XML documentation. |
| src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs | Calls Configinfo.Validate() and attempts to inject an IBinaryDiffer extension into strategies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+70
to
+73
| if (!await sem.WaitAsync(TimeSpan.FromMinutes(5), token).ConfigureAwait(false)) | ||
| { | ||
| GeneralTracer.Warn("DefaultDownloadOrchestrator: semaphore wait timed out, proceeding anyway."); | ||
| } |
Comment on lines
132
to
143
| // Inject binary differ into OS-level strategy for differential patching | ||
| var differ = ResolveExtension<IBinaryDiffer>(); | ||
| if (differ != null) | ||
| { | ||
| if (roleStrategy is ClientUpdateStrategy cs2) | ||
| cs2.SetDiffer(differ); | ||
| else if (roleStrategy is UpgradeUpdateStrategy us2) | ||
| us2.SetDiffer(differ); | ||
| } | ||
|
|
||
| roleStrategy.Create(_configInfo); | ||
|
|
Comment on lines
+76
to
+81
| /// <summary>Sets the binary differ on the underlying OS-level strategy for differential patch updates.</summary> | ||
| public void SetDiffer(Differential.IBinaryDiffer? differ) | ||
| { | ||
| if (_osStrategy is AbstractStrategy abs) | ||
| abs.Differ = differ; | ||
| } |
Comment on lines
+93
to
+98
| /// <summary>Sets the binary differ on the underlying OS-level strategy for differential patch updates.</summary> | ||
| public void SetDiffer(Differential.IBinaryDiffer? differ) | ||
| { | ||
| if (_osStrategy is AbstractStrategy abs) | ||
| abs.Differ = differ; | ||
| } |
| /// The <see cref="IBinaryDiffer"/> implementation is injected via | ||
| /// The <see cref="IBinaryDiffer"/> implementation is resolved from | ||
| /// <see cref="PipelineContext"/> (key "BinaryDiffer"), set by | ||
| /// <see cref="Strategy.AbstractStrategy"/> when the differ is injected via |
…ing, XML doc cref - Semaphore: only Release() when WaitAsync succeeded; skip asset on timeout instead of bypassing concurrency limit (prevent SemaphoreFullException). - SetDiffer: move injection after Create() so _osStrategy exists; add _pendingDiffer caching in ClientUpdateStrategy/UpgradeUpdateStrategy so SetDiffer is safe to call at any point. - XML doc cref: use fully-qualified type name in PatchMiddleware. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 task
JusterZhu
added a commit
that referenced
this pull request
May 26, 2026
…ImmutableQueue) The LIFO stack caused middleware execution order to be the reverse of registration order, which was the root cause of the MacStrategy pipeline bug fixed in #436. With FIFO (ImmutableQueue), registration order now equals execution order for intuitive API semantics. - PipelineBuilder: ImmutableStack → ImmutableQueue, Push → Enqueue - All OS strategies: register Hash → Compress → Patch (executes in same order) - MacStrategy: also changed to UseMiddlewareIf<PatchMiddleware> for consistency Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.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.
Summary
This PR fixes 2 P0 blocking bugs and several P1/P2 robustness issues discovered during a code review of GeneralUpdate.Core.
P0 - MacStrategy Pipeline middleware execution order was wrong
MacStrategy.BuildPipeline registered middleware in a different order than Windows/Linux, causing the LIFO pipeline to execute Patch→Compress→Hash instead of Hash→Compress→Patch. This made macOS updates completely non-functional.
P0 - PatchMiddleware could never receive an injected IBinaryDiffer
PipelineBuilder creates middleware via
new(), so the parameterized constructor was never invoked by the pipeline system. The differ was always null. Now it flows through PipelineContext.P1 - OSSUpgrade exception path lacked process exit
ExecuteUpgradeAsync re-threw exceptions without terminating the process. Added finally block with GracefulExit.
P1 - Download semaphore had no timeout
SemaphoreSlim.WaitAsync in DefaultDownloadOrchestrator could block indefinitely.
P2 - Configinfo.Validate() was never called
Added to SetConfig to catch invalid configuration early.
P2 - Rollback/restore not wired into update failure path
Added TryRollback() to AbstractStrategy, called when pipeline fails for a version.
P2 - GetTempDirectory only had day-level precision
Added millisecond precision and ProcessId to prevent collisions.
Files changed
Strategy/MacStrategy.csStrategy/AbstractStrategy.csPipeline/PatchMiddleware.csBootstrap/GeneralUpdateBootstrap.csStrategy/ClientUpdateStrategy.csStrategy/UpgradeUpdateStrategy.csStrategy/OSSUpdateStrategy.csDownload/Orchestrators/DefaultDownloadOrchestrator.csFileSystem/StorageManager.csConfiguration/GlobalConfigInfo.cstests/.../PatchMiddlewareTests.csTest plan
🤖 Generated with Claude Code