Skip to content

Commit ee06190

Browse files
JusterZhuclaude
andcommitted
fix: check download result and abort IPC/launch on pipeline failure
Two related fixes that prevent a cyclic update loop when download or patch application fails: 1. ClientStrategy.ExecuteDownloadAsync: return Task<DownloadReport> instead of Task so the caller can inspect per-asset results. DownloadAndApplyAsync now checks FailedCount and throws on failure, giving the CVP fallback a chance to retry. 2. Both scenario in ClientStrategy: check AllPackagesSucceeded after ApplyUpgradePackagesAsync. If upgrade packages failed to apply, do NOT send ProcessContract via IPC or launch the upgrade process — doing so would forward a stale/missing TempPath and cause undefined behavior. 3. UpdateStrategy: gate client launch on AllPackagesSucceeded. When MainApp packages fail, skip StartAppAsync to avoid restarting the old client, which would immediately re-detect the update and loop. Co-authored-by: Claude <noreply@anthropic.com>
1 parent d44944c commit ee06190

2 files changed

Lines changed: 35 additions & 19 deletions

File tree

src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -601,13 +601,13 @@ async Task DownloadAndApplyAsync(Download.Models.DownloadPlan plan, UpdateScenar
601601
downloadReport.Results.Where(r => !r.Success)
602602
.Select(r => $"{r.Asset.Name}: {r.ErrorMessage}"));
603603
GeneralTracer.Error($"ClientStrategy: {downloadReport.FailedCount} download(s) failed: {failDetails}");
604-
EventManager.Instance.Dispatch(this,
605-
new ExceptionEventArgs(new InvalidOperationException(
606-
$"{downloadReport.FailedCount} download(s) failed. Aborting apply phase."),
607-
"Download failures detected."));
608-
// Throw so CVP fallback can retry with chain packages.
609-
throw new InvalidOperationException(
604+
// Single exception instance for both event dispatch and throw — no
605+
// allocations, consistent correlation in logs and event subscribers.
606+
var ex = new InvalidOperationException(
610607
$"{downloadReport.FailedCount} download(s) failed. Aborting apply phase.");
608+
EventManager.Instance.Dispatch(this, new ExceptionEventArgs(ex, "Download failures detected."));
609+
// Throw so CVP fallback can retry with chain packages.
610+
throw ex;
611611
}
612612

613613
await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);
@@ -635,16 +635,18 @@ async Task DownloadAndApplyAsync(Download.Models.DownloadPlan plan, UpdateScenar
635635
{
636636
case UpdateScenario.UpgradeOnly:
637637
await ApplyUpgradePackagesAsync(uVersions).ConfigureAwait(false);
638-
if ((_osStrategy as AbstractStrategy)?.AllPackagesSucceeded == true)
638+
if (UpgradePackagesSucceeded())
639639
{
640640
await SafeOnAfterUpdateAsync(hooksCtx).ConfigureAwait(false);
641641
await SafeReportUpdateAppliedAsync(hooksCtx, _upgradeRecordId).ConfigureAwait(false);
642642
GeneralTracer.Info("ClientStrategy: Upgrade-only update applied, client continues running.");
643643
}
644644
else
645645
{
646-
await SafeReportUpdateFailedAsync(hooksCtx,
647-
new InvalidOperationException("Upgrade packages failed to apply.")).ConfigureAwait(false);
646+
var failEx = new InvalidOperationException("Upgrade packages failed to apply.");
647+
await SafeOnUpdateErrorAsync(hooksCtx, failEx).ConfigureAwait(false);
648+
await SafeReportUpdateFailedAsync(hooksCtx, failEx).ConfigureAwait(false);
649+
EventManager.Instance.Dispatch(this, new ExceptionEventArgs(failEx, failEx.Message));
648650
GeneralTracer.Error("ClientStrategy: Upgrade-only update failed, client continues running.");
649651
}
650652
break;
@@ -667,12 +669,14 @@ await SafeReportUpdateFailedAsync(hooksCtx,
667669
// may be in an inconsistent state. Do NOT proceed to send IPC or
668670
// launch the upgrade process — doing so would silently fail or
669671
// cause undefined behavior in the upgrade process.
670-
if ((_osStrategy as AbstractStrategy)?.AllPackagesSucceeded != true)
672+
if (!UpgradePackagesSucceeded())
671673
{
674+
var failEx = new InvalidOperationException("Upgrade packages failed to apply.");
675+
await SafeOnUpdateErrorAsync(hooksCtx, failEx).ConfigureAwait(false);
676+
await SafeReportUpdateFailedAsync(hooksCtx, failEx).ConfigureAwait(false);
677+
EventManager.Instance.Dispatch(this, new ExceptionEventArgs(failEx, failEx.Message));
672678
GeneralTracer.Error(
673679
"ClientStrategy: upgrade packages failed to apply, aborting MainApp update and upgrade launch.");
674-
await SafeReportUpdateFailedAsync(hooksCtx,
675-
new InvalidOperationException("Upgrade packages failed to apply.")).ConfigureAwait(false);
676680
break;
677681
}
678682

@@ -1238,6 +1242,17 @@ await Reporter
12381242
return (plan, sc);
12391243
}
12401244

1245+
/// <summary>
1246+
/// Returns <c>true</c> when the OS strategy reports that all upgrade packages
1247+
/// were applied successfully. For custom <see cref="IStrategy"/> implementations
1248+
/// that do not expose <see cref="AbstractStrategy.AllPackagesSucceeded"/>,
1249+
/// returns <c>true</c> (assume success since no failure was signalled).
1250+
/// </summary>
1251+
private bool UpgradePackagesSucceeded()
1252+
{
1253+
return (_osStrategy as AbstractStrategy)?.AllPackagesSucceeded ?? true;
1254+
}
1255+
12411256
/// <summary>
12421257
/// Updates <see cref="_mainRecordId"/> and <see cref="_upgradeRecordId"/> from the
12431258
/// current download plan so status reports use correct record identifiers after fallback.

src/c#/GeneralUpdate.Core/Strategy/UpdateStrategy.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,10 @@ public async Task ExecuteAsync()
122122
// successfully. AbstractStrategy catches per-package failures and
123123
// continues the loop, so ExecuteAsync() completing is not a
124124
// reliable success signal on its own.
125-
pipelineSucceeded = (_osStrategy as AbstractStrategy)?.AllPackagesSucceeded == true;
125+
// For custom IStrategy implementations that don't expose
126+
// AllPackagesSucceeded, assume success (coalesce to true)
127+
// since no failure was signalled via an exception.
128+
pipelineSucceeded = (_osStrategy as AbstractStrategy)?.AllPackagesSucceeded ?? true;
126129
if (pipelineSucceeded)
127130
{
128131
WriteBackClientVersion();
@@ -142,12 +145,10 @@ public async Task ExecuteAsync()
142145
// restart it with old files, causing it to re-detect the update and loop.
143146
if (!pipelineSucceeded)
144147
{
145-
await SafeOnUpdateErrorAsync(ctx,
146-
new InvalidOperationException("MainApp pipeline did not complete successfully."))
147-
.ConfigureAwait(false);
148-
await SafeReportUpdateFailedAsync(ctx,
149-
new InvalidOperationException("MainApp pipeline did not complete successfully."))
150-
.ConfigureAwait(false);
148+
var failEx = new InvalidOperationException("MainApp pipeline did not complete successfully.");
149+
await SafeOnUpdateErrorAsync(ctx, failEx).ConfigureAwait(false);
150+
await SafeReportUpdateFailedAsync(ctx, failEx).ConfigureAwait(false);
151+
EventManager.Instance.Dispatch(this, new ExceptionEventArgs(failEx, failEx.Message));
151152
return;
152153
}
153154

0 commit comments

Comments
 (0)