Skip to content

Commit 73a6002

Browse files
JusterZhuclaude
andcommitted
fix: resolve CI failures and address Copilot review comments
- Fix CoreTest/DownloadModelsTests: remove CVP property references, update for PackageType + FallbackFull* fields - Fix CoreTest/ConfigurationModelsTests: same CVP→PackageType migration - Fix CoreTest/DownloadPlanBuilderTests: replace CVP tests with chain/full switch test, update helper signatures - Fix ClientTest/Program.cs: remove CVP field references (Already fixed in fix/chain-fallback-skip-covered-versions) - Fix HttpDownloadSource.MapVersionEntry: restore MinClientVersion mapping (Copilot: prevents premature update to unsupported versions) - Fix DownloadPlanBuilder: treat PackageType=0 (Unspecified) as Chain for backward compatibility (Copilot: prevents empty plan from old servers) - Fix DownloadPlanBuilder: limit chain size comparison to same AppType as bestFull (Copilot: prevents incorrect cross-AppType switching) - Fix AbstractStrategy: wrap fallback execution in try/catch so a failed fallback doesn't abort the entire update loop (Copilot: graceful degradation) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent fbc7a16 commit 73a6002

6 files changed

Lines changed: 65 additions & 62 deletions

File tree

src/c#/GeneralUpdate.Core/Download/DownloadPlanBuilder.cs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,12 @@ public static DownloadPlan Build(
136136

137137
if (candidates.Count == 0) return DownloadPlan.Empty;
138138

139-
// Separate chain vs full packages
139+
// Separate chain vs full packages.
140+
// Treat Unspecified (0) as Chain for backward compatibility with older
141+
// servers that do not set PackageType yet.
140142
var chainCandidates = candidates
141-
.Where(a => a.PackageType == (int)Configuration.PackageType.Chain)
143+
.Where(a => a.PackageType == (int)Configuration.PackageType.Chain
144+
|| a.PackageType == (int)Configuration.PackageType.Unspecified)
142145
.ToList();
143146

144147
var fullCandidates = candidates
@@ -155,7 +158,11 @@ public static DownloadPlan Build(
155158
.OrderByDescending(a => { Semver.TryParse(a.Version, out var sv); return sv; })
156159
.First();
157160

158-
long chainTotal = chainCandidates.Sum(a => a.Size);
161+
// Only compare against chain packages of the same AppType as bestFull.
162+
// Mixing Client and Upgrade sizes together could trigger incorrect switching.
163+
long chainTotal = chainCandidates
164+
.Where(a => a.AppType == bestFull.AppType)
165+
.Sum(a => a.Size);
159166
var threshold = (long)(bestFull.Size * 0.8);
160167

161168
if (chainTotal >= threshold)

src/c#/GeneralUpdate.Core/Download/Sources/HttpDownloadSource.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ private static DownloadAsset MapVersionEntry(VersionEntry v)
176176
SHA256: v.Hash,
177177
Version: v.Version ?? "0.0.0",
178178
PackageType: v.PackageType,
179+
MinClientVersion: v.MinClientVersion,
179180
IsForcibly: v.IsForcibly == true,
180181
IsFreeze: v.IsFreeze == true,
181182
RecordId: v.RecordId,

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -209,15 +209,26 @@ public virtual async Task ExecuteAsync()
209209
fallbackContext.Add("DiffPipeline", DiffPipeline);
210210

211211
var fallbackBuilder = BuildPipeline(fallbackContext);
212-
await fallbackBuilder.Build();
213-
status = ReportType.Success;
214-
// Record the fallback full package version so subsequent
215-
// chain packages ≤ this version are skipped.
216-
if (!string.IsNullOrEmpty(version.FallbackFullVersion)
217-
&& Semver.TryParse(version.FallbackFullVersion, out var ffv)
218-
&& (fallbackEffectiveVersion == null || ffv > fallbackEffectiveVersion))
212+
try
213+
{
214+
await fallbackBuilder.Build();
215+
status = ReportType.Success;
216+
// Record the fallback full package version so subsequent
217+
// chain packages ≤ this version are skipped.
218+
if (!string.IsNullOrEmpty(version.FallbackFullVersion)
219+
&& Semver.TryParse(version.FallbackFullVersion, out var ffv)
220+
&& (fallbackEffectiveVersion == null || ffv > fallbackEffectiveVersion))
221+
{
222+
fallbackEffectiveVersion = ffv;
223+
}
224+
}
225+
catch (Exception fallbackEx)
219226
{
220-
fallbackEffectiveVersion = ffv;
227+
// Fallback itself failed (e.g. missing full zip, decompression error).
228+
// Downgrade to normal failure handling so the loop can continue
229+
// processing remaining versions.
230+
GeneralTracer.Error($"AbstractStrategy.ExecuteAsync: fallback full package also failed for {version.Version}. Error: {fallbackEx.Message}");
231+
status = ReportType.Failure;
221232
}
222233
}
223234
catch (Exception e)

tests/CoreTest/Configuration/ConfigurationModelsTests.cs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -88,30 +88,33 @@ public void DownloadAsset_AllFields_ConstructsCorrectly()
8888
Assert.Equal("sha256:abc123def456", asset.SHA256);
8989
Assert.Equal(104857600, asset.Size);
9090
Assert.Equal(DownloadPriority.Normal, asset.Priority);
91-
Assert.False(asset.IsCrossVersion);
91+
Assert.Equal(0, asset.PackageType);
9292
Assert.False(asset.IsForcibly);
9393
Assert.False(asset.IsFreeze);
9494
}
9595

9696
[Fact]
97-
public void DownloadAsset_CrossVersion_IsForcibly()
97+
public void DownloadAsset_ChainWithFallback_PropertiesSet()
9898
{
9999
var asset = new DownloadAsset(
100-
Name: "critical-patch.zip",
101-
Url: "https://cdn.example.com/security/hotfix.zip",
100+
Name: "chain-patch.zip",
101+
Url: "https://cdn.example.com/chain/patch.zip",
102102
Size: 5L * 1024 * 1024,
103103
SHA256: null,
104-
Version: "2.0.1-hotfix",
104+
Version: "2.0.1",
105105
Priority: DownloadPriority.High,
106-
IsForcibly: true,
107-
IsCrossVersion: true,
108-
FromVersion: "2.0.0"
106+
PackageType: (int)PackageType.Chain,
107+
FallbackFullName: "full-v2.0.1",
108+
FallbackFullUrl: "https://cdn.example.com/full/v2.0.1.zip",
109+
FallbackFullHash: "abc123def456",
110+
IsForcibly: true
109111
);
110112

111113
Assert.Equal(DownloadPriority.High, asset.Priority);
112114
Assert.True(asset.IsForcibly);
113-
Assert.True(asset.IsCrossVersion);
114-
Assert.Equal("2.0.0", asset.FromVersion);
115+
Assert.Equal((int)PackageType.Chain, asset.PackageType);
116+
Assert.Equal("full-v2.0.1", asset.FallbackFullName);
117+
Assert.Equal("abc123def456", asset.FallbackFullHash);
115118
}
116119

117120
#endregion

tests/CoreTest/Download/DownloadModelsTests.cs

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@ public void DownloadAsset_Defaults_AreSensible()
2222
Assert.Null(asset.SHA256);
2323
Assert.Equal("1.0.0", asset.Version);
2424
Assert.Equal(DownloadPriority.Normal, asset.Priority);
25-
Assert.False(asset.IsCrossVersion);
26-
Assert.Null(asset.FromVersion);
25+
Assert.Equal(0, asset.PackageType);
2726
Assert.Null(asset.MinClientVersion);
2827
Assert.False(asset.IsForcibly);
2928
Assert.False(asset.IsFreeze);
@@ -34,8 +33,12 @@ public void DownloadAsset_FullySpecified_AllPropertiesSet()
3433
{
3534
var asset = new DownloadAsset(
3635
"package.zip", "https://cdn/pkg.zip", 1024, "abc123", "3.0.0",
37-
DownloadPriority.High, true, "1.0.0", "2.0.0",
38-
"srcHash", "tgtHash", true, false
36+
DownloadPriority.High, 0,
37+
MinClientVersion: "2.0.0",
38+
FallbackFullName: "full-pkg",
39+
FallbackFullUrl: "https://cdn/full.zip",
40+
FallbackFullHash: "fullhash",
41+
IsForcibly: true, IsFreeze: false
3942
);
4043

4144
Assert.Equal("package.zip", asset.Name);
@@ -44,11 +47,10 @@ public void DownloadAsset_FullySpecified_AllPropertiesSet()
4447
Assert.Equal("abc123", asset.SHA256);
4548
Assert.Equal("3.0.0", asset.Version);
4649
Assert.Equal(DownloadPriority.High, asset.Priority);
47-
Assert.True(asset.IsCrossVersion);
48-
Assert.Equal("1.0.0", asset.FromVersion);
4950
Assert.Equal("2.0.0", asset.MinClientVersion);
50-
Assert.Equal("srcHash", asset.SourceArchiveHash);
51-
Assert.Equal("tgtHash", asset.TargetArchiveHash);
51+
Assert.Equal("full-pkg", asset.FallbackFullName);
52+
Assert.Equal("https://cdn/full.zip", asset.FallbackFullUrl);
53+
Assert.Equal("fullhash", asset.FallbackFullHash);
5254
Assert.True(asset.IsForcibly);
5355
Assert.False(asset.IsFreeze);
5456
}

tests/CoreTest/Download/DownloadPlanBuilderTests.cs

Lines changed: 12 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,10 @@ public class DownloadPlanBuilderTests
88
{
99
private static DownloadAsset Asset(string name = "a", string version = "2.0.0", string url = "http://u",
1010
long size = 100, string hash = null, bool isFreeze = false, bool isForcibly = false,
11-
bool isCrossVersion = false, string fromVersion = null, string minClientVersion = null)
11+
int packageType = (int)PackageType.Chain, string minClientVersion = null)
1212
=> new(name, url, size, hash, version,
1313
IsFreeze: isFreeze, IsForcibly: isForcibly,
14-
IsCrossVersion: isCrossVersion, FromVersion: fromVersion,
15-
MinClientVersion: minClientVersion);
14+
PackageType: packageType, MinClientVersion: minClientVersion);
1615

1716
[Fact]
1817
public void Build_AssetsNull_ReturnsEmpty()
@@ -82,40 +81,21 @@ public void Build_NoAssetIsForcibly_IsForciblyFalse()
8281
}
8382

8483
[Fact]
85-
public void Build_CrossVersionIncluded_CvpFirst_ReturnsCvpOnly()
84+
public void Build_FullPackageSelected_WhenChainExceedsThreshold()
8685
{
87-
// When a matching CVP exists, the plan selects the CVP and drops
88-
// same-AppType chain packages (CVP covers the full range).
86+
// Small chain + small full → chain_total >= 80% full → Full selected
8987
var assets = new[]
9088
{
91-
Asset("cross", "5.0.0", isCrossVersion: true, fromVersion: "1.0.0"),
92-
Asset("inc", "2.0.0"), Asset("inc2", "3.0.0")
89+
Asset("chain1", "1.1.0", size: 100),
90+
Asset("chain2", "2.0.0", size: 100),
91+
Asset("full", "2.0.0", packageType: (int)PackageType.Full),
9392
};
93+
// chain_total=200, 80% of full(100) = 80 → 200 >= 80 → full selected
9494
var result = DownloadPlanBuilder.Build(assets, "1.0.0");
9595
Assert.True(result.HasAssets);
9696
Assert.Single(result.Assets);
97-
Assert.True(result.Assets[0].IsCrossVersion);
98-
Assert.Equal("5.0.0", result.Assets[0].Version);
99-
}
100-
101-
[Fact]
102-
public void Build_CvpWithMixedAppTypes_KeepsChainForOtherTypes()
103-
{
104-
// CVP covers Client (AppType=1). Upgrade chain packages (AppType=2)
105-
// should still be included since the CVP doesn't cover that AppType.
106-
var assets = new[]
107-
{
108-
AssetWithType("cvp", "5.0.0", appType: 1, isCrossVersion: true, fromVersion: "1.0.0"),
109-
AssetWithType("upgrade1", "2.0.0", appType: 2),
110-
AssetWithType("upgrade2", "3.0.0", appType: 2),
111-
};
112-
var result = DownloadPlanBuilder.Build(assets, "1.0.0");
113-
Assert.True(result.HasAssets);
114-
Assert.Equal(3, result.Assets.Count);
115-
Assert.Equal("5.0.0", result.Assets[0].Version); // CVP first
116-
Assert.True(result.Assets[0].IsCrossVersion);
117-
Assert.Equal("2.0.0", result.Assets[1].Version); // chain for other AppType
118-
Assert.Equal("3.0.0", result.Assets[2].Version);
97+
Assert.Equal((int)PackageType.Full, result.Assets[0].PackageType);
98+
Assert.Equal("2.0.0", result.Assets[0].Version);
11999
}
120100

121101
[Fact]
@@ -174,12 +154,11 @@ public void Build_MixedFrozenAndActive_FiltersFrozen()
174154
private static DownloadAsset AssetWithType(string name = "a", string version = "2.0.0",
175155
string url = "http://u", long size = 100, string? hash = null,
176156
bool isFreeze = false, bool isForcibly = false,
177-
bool isCrossVersion = false, string? fromVersion = null,
157+
int packageType = (int)PackageType.Chain,
178158
string? minClientVersion = null, int? appType = null)
179159
=> new(name, url, size, hash, version,
180160
IsFreeze: isFreeze, IsForcibly: isForcibly,
181-
IsCrossVersion: isCrossVersion, FromVersion: fromVersion,
182-
MinClientVersion: minClientVersion, AppType: appType);
161+
PackageType: packageType, MinClientVersion: minClientVersion, AppType: appType);
183162

184163
[Fact]
185164
public void HasUpdate_EmptyAssets_ReturnsFalse()

0 commit comments

Comments
 (0)