Skip to content

Commit d44944c

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 2d7568d commit d44944c

2 files changed

Lines changed: 68 additions & 8 deletions

File tree

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

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -573,27 +573,42 @@ private async Task ExecuteStandardWorkflowAsync()
573573
// ════════════════════════════════════════════════════════════════════
574574
var orchOptions = Download.Models.DownloadOrchestratorOptions.From(_configInfo);
575575

576-
async Task ExecuteDownloadAsync(Download.Models.DownloadPlan plan)
576+
async Task<Download.Abstractions.DownloadReport> ExecuteDownloadAsync(Download.Models.DownloadPlan plan)
577577
{
578578
if (_orchestrator != null)
579579
{
580-
await _orchestrator.ExecuteAsync(plan, _configInfo.TempPath).ConfigureAwait(false);
580+
return await _orchestrator.ExecuteAsync(plan, _configInfo.TempPath).ConfigureAwait(false);
581581
}
582582
else
583583
{
584584
var httpClient = GeneralUpdate.Core.Network.HttpClientProvider.Shared;
585585
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(
586586
httpClient, orchOptions, _customDownloadPolicy,
587587
_customDownloadExecutor, _customDownloadPipelineFactory);
588-
await orchestrator.ExecuteAsync(plan, _configInfo.TempPath).ConfigureAwait(false);
588+
return await orchestrator.ExecuteAsync(plan, _configInfo.TempPath).ConfigureAwait(false);
589589
}
590590
}
591591

592592
// Download + report + build version lists + scenario dispatch
593593
async Task DownloadAndApplyAsync(Download.Models.DownloadPlan plan, UpdateScenario sc)
594594
{
595595
GeneralTracer.Info($"ClientStrategy: downloading {plan.Assets.Count} asset(s).");
596-
await ExecuteDownloadAsync(plan).ConfigureAwait(false);
596+
var downloadReport = await ExecuteDownloadAsync(plan).ConfigureAwait(false);
597+
598+
if (downloadReport.FailedCount > 0)
599+
{
600+
var failDetails = string.Join(", ",
601+
downloadReport.Results.Where(r => !r.Success)
602+
.Select(r => $"{r.Asset.Name}: {r.ErrorMessage}"));
603+
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(
610+
$"{downloadReport.FailedCount} download(s) failed. Aborting apply phase.");
611+
}
597612

598613
await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);
599614
await SafeOnDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);
@@ -620,9 +635,18 @@ async Task DownloadAndApplyAsync(Download.Models.DownloadPlan plan, UpdateScenar
620635
{
621636
case UpdateScenario.UpgradeOnly:
622637
await ApplyUpgradePackagesAsync(uVersions).ConfigureAwait(false);
623-
await SafeOnAfterUpdateAsync(hooksCtx).ConfigureAwait(false);
624-
await SafeReportUpdateAppliedAsync(hooksCtx, _upgradeRecordId).ConfigureAwait(false);
625-
GeneralTracer.Info("ClientStrategy: Upgrade-only update applied, client continues running.");
638+
if ((_osStrategy as AbstractStrategy)?.AllPackagesSucceeded == true)
639+
{
640+
await SafeOnAfterUpdateAsync(hooksCtx).ConfigureAwait(false);
641+
await SafeReportUpdateAppliedAsync(hooksCtx, _upgradeRecordId).ConfigureAwait(false);
642+
GeneralTracer.Info("ClientStrategy: Upgrade-only update applied, client continues running.");
643+
}
644+
else
645+
{
646+
await SafeReportUpdateFailedAsync(hooksCtx,
647+
new InvalidOperationException("Upgrade packages failed to apply.")).ConfigureAwait(false);
648+
GeneralTracer.Error("ClientStrategy: Upgrade-only update failed, client continues running.");
649+
}
626650
break;
627651

628652
case UpdateScenario.MainOnly:
@@ -638,6 +662,20 @@ async Task DownloadAndApplyAsync(Download.Models.DownloadPlan plan, UpdateScenar
638662

639663
case UpdateScenario.Both:
640664
await ApplyUpgradePackagesAsync(uVersions).ConfigureAwait(false);
665+
666+
// If upgrade packages failed to apply, the upgrade process binary
667+
// may be in an inconsistent state. Do NOT proceed to send IPC or
668+
// launch the upgrade process — doing so would silently fail or
669+
// cause undefined behavior in the upgrade process.
670+
if ((_osStrategy as AbstractStrategy)?.AllPackagesSucceeded != true)
671+
{
672+
GeneralTracer.Error(
673+
"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);
676+
break;
677+
}
678+
641679
await SafeOnAfterUpdateAsync(hooksCtx).ConfigureAwait(false);
642680
await SafeReportUpdateAppliedAsync(hooksCtx, _upgradeRecordId).ConfigureAwait(false);
643681
SendProcessIpc(cVersions);

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ public async Task ExecuteAsync()
111111
_osStrategy!.Create(_configInfo);
112112

113113
// Apply MainApp updates -- Client already applied Upgrade packages, IPC only has MainApp versions
114+
var pipelineSucceeded = true;
114115
if (_configInfo.UpdateVersions?.Count > 0)
115116
{
116117
GeneralTracer.Info("UpdateStrategy: applying " + _configInfo.UpdateVersions.Count +
@@ -121,14 +122,35 @@ public async Task ExecuteAsync()
121122
// successfully. AbstractStrategy catches per-package failures and
122123
// continues the loop, so ExecuteAsync() completing is not a
123124
// reliable success signal on its own.
124-
if ((_osStrategy as AbstractStrategy)?.AllPackagesSucceeded == true)
125+
pipelineSucceeded = (_osStrategy as AbstractStrategy)?.AllPackagesSucceeded == true;
126+
if (pipelineSucceeded)
127+
{
125128
WriteBackClientVersion();
129+
}
130+
else
131+
{
132+
GeneralTracer.Warn("UpdateStrategy: one or more MainApp packages failed, " +
133+
"skipping manifest write and app launch.");
134+
}
126135
}
127136
else
128137
{
129138
GeneralTracer.Info("UpdateStrategy: no updates to apply, starting application directly.");
130139
}
131140

141+
// When main app updates failed, do NOT launch the client — doing so would
142+
// restart it with old files, causing it to re-detect the update and loop.
143+
if (!pipelineSucceeded)
144+
{
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);
151+
return;
152+
}
153+
132154
// Hooks: after all updates applied
133155
await SafeOnAfterUpdateAsync(ctx).ConfigureAwait(false);
134156

0 commit comments

Comments
 (0)