Skip to content

Commit 32f4789

Browse files
marticlimentmarticliment
authored
Merge pull request #4171 from Janek91/fix/4131-duplicate-unattended-updates
2 parents 6b1843e + 3733638 commit 32f4789

File tree

3 files changed

+222
-3
lines changed

3 files changed

+222
-3
lines changed

‎src/UniGetUI.PackageEngine.Serializable.Tests/TestDuplicateUpdateDetection.cs‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
using NSubstitute;
2+
using UniGetUI.Interface.Enums;
3+
using UniGetUI.PackageEngine.Interfaces;
4+
using UniGetUI.PackageEngine.PackageClasses;
5+
6+
namespace UniGetUI.PackageEngine.Tests;
7+
8+
/// <summary>
9+
/// Tests for duplicate detection in update operations (Issue #4131).
10+
/// These tests verify the logic used in AppOperationHelper.UpdateAll() and UpdateAllForManager()
11+
/// to prevent multiple update operations for the same package from being queued simultaneously.
12+
/// </summary>
13+
public class TestDuplicateUpdateDetection
14+
{
15+
/// <summary>
16+
/// Test that verifies Package.GetHash() returns the same value for identical packages.
17+
/// This is the core mechanism used to detect duplicate operations in the queue.
18+
///
19+
/// Background: Issue #4131 - When unattended updates trigger repeatedly while UAC prompts
20+
/// are pending, UpdateAll() was creating dozens of duplicate operations for the same package.
21+
/// The fix uses GetHash() to check if an operation for a package already exists in the queue.
22+
/// </summary>
23+
[Fact]
24+
public void GetHash_ShouldReturnSameValue_ForIdenticalPackages()
25+
{
26+
// Arrange - Create mock manager and source
27+
// Note: We use the same instances to ensure consistent hashing
28+
var mockManager = Substitute.For<IPackageManager>();
29+
mockManager.Name.Returns("TestManager");
30+
31+
var mockSource = Substitute.For<IManagerSource>();
32+
mockSource.Name.Returns("TestSource");
33+
34+
// Create two package instances with identical identity (same manager, source, and ID)
35+
var package1 = new Package(
36+
name: "TestPackage",
37+
id: "test.package.id",
38+
version: "1.0.0",
39+
source: mockSource,
40+
manager: mockManager
41+
);
42+
43+
var package2 = new Package(
44+
name: "TestPackage",
45+
id: "test.package.id",
46+
version: "1.0.0",
47+
source: mockSource,
48+
manager: mockManager
49+
);
50+
51+
// Act - Get hash for both packages
52+
long hash1 = package1.GetHash();
53+
long hash2 = package2.GetHash();
54+
55+
// Assert - Hashes must be equal for the duplicate detection to work
56+
Assert.Equal(hash1, hash2);
57+
}
58+
59+
/// <summary>
60+
/// Test that verifies Package.GetHash() returns different values for different packages.
61+
/// This ensures the duplicate detection doesn't falsely match unrelated packages.
62+
/// </summary>
63+
[Fact]
64+
public void GetHash_ShouldReturnDifferentValues_ForDifferentPackages()
65+
{
66+
// Arrange
67+
var mockManager = Substitute.For<IPackageManager>();
68+
mockManager.Name.Returns("TestManager");
69+
70+
var mockSource = Substitute.For<IManagerSource>();
71+
mockSource.Name.Returns("TestSource");
72+
73+
// Create packages with different IDs
74+
var package1 = new Package(
75+
name: "TestPackage1",
76+
id: "test.package.one",
77+
version: "1.0.0",
78+
source: mockSource,
79+
manager: mockManager
80+
);
81+
82+
var package2 = new Package(
83+
name: "TestPackage2",
84+
id: "test.package.two",
85+
version: "1.0.0",
86+
source: mockSource,
87+
manager: mockManager
88+
);
89+
90+
// Act
91+
long hash1 = package1.GetHash();
92+
long hash2 = package2.GetHash();
93+
94+
// Assert - Hashes must be different so we don't skip legitimate different packages
95+
Assert.NotEqual(hash1, hash2);
96+
}
97+
98+
/// <summary>
99+
/// Test that verifies packages from different managers have different hashes.
100+
/// The duplicate detection must consider the package manager as part of identity.
101+
/// </summary>
102+
[Fact]
103+
public void GetHash_ShouldDiffer_ForSamePackageFromDifferentManagers()
104+
{
105+
// Arrange - Create two different managers
106+
var mockManager1 = Substitute.For<IPackageManager>();
107+
mockManager1.Name.Returns("WinGet");
108+
109+
var mockManager2 = Substitute.For<IPackageManager>();
110+
mockManager2.Name.Returns("Chocolatey");
111+
112+
var mockSource = Substitute.For<IManagerSource>();
113+
mockSource.Name.Returns("TestSource");
114+
115+
// Same package ID from different managers
116+
var packageFromWinGet = new Package(
117+
name: "TestPackage",
118+
id: "test.package",
119+
version: "1.0.0",
120+
source: mockSource,
121+
manager: mockManager1
122+
);
123+
124+
var packageFromChoco = new Package(
125+
name: "TestPackage",
126+
id: "test.package",
127+
version: "1.0.0",
128+
source: mockSource,
129+
manager: mockManager2
130+
);
131+
132+
// Act
133+
long hashWinGet = packageFromWinGet.GetHash();
134+
long hashChoco = packageFromChoco.GetHash();
135+
136+
// Assert - Must be different because they're from different managers
137+
Assert.NotEqual(hashWinGet, hashChoco);
138+
}
139+
140+
/// <summary>
141+
/// Test simulating the scenario where multiple UpdateAll() calls should detect duplicates.
142+
/// This represents what happens when auto-updates trigger while operations are queued.
143+
/// </summary>
144+
[Fact]
145+
public void SimulateDuplicateDetection_MultipleIdenticalPackages_ShouldHaveSameHash()
146+
{
147+
// Arrange - Simulate receiving same package multiple times from UpgradablePackagesLoader
148+
var mockManager = Substitute.For<IPackageManager>();
149+
mockManager.Name.Returns("WinGet");
150+
151+
var mockSource = Substitute.For<IManagerSource>();
152+
mockSource.Name.Returns("winget");
153+
154+
// Simulate UpdateAll() being called 3 times, each time getting "same" package
155+
var packagesFromFirstCall = new Package(
156+
name: "Git",
157+
id: "Git.Git",
158+
version: "2.40.0",
159+
source: mockSource,
160+
manager: mockManager
161+
);
162+
163+
var packagesFromSecondCall = new Package(
164+
name: "Git",
165+
id: "Git.Git",
166+
version: "2.40.0",
167+
source: mockSource,
168+
manager: mockManager
169+
);
170+
171+
var packagesFromThirdCall = new Package(
172+
name: "Git",
173+
id: "Git.Git",
174+
version: "2.40.0",
175+
source: mockSource,
176+
manager: mockManager
177+
);
178+
179+
// Act - Get hashes that would be used for duplicate detection
180+
var hash1 = packagesFromFirstCall.GetHash();
181+
var hash2 = packagesFromSecondCall.GetHash();
182+
var hash3 = packagesFromThirdCall.GetHash();
183+
184+
// Assert - All three should have the same hash, allowing duplicate detection
185+
Assert.Equal(hash1, hash2);
186+
Assert.Equal(hash2, hash3);
187+
Assert.Equal(hash1, hash3);
188+
189+
// In the actual implementation, the second and third calls would find
190+
// an existing operation with matching hash and skip creating duplicates
191+
}
192+
}
193+

‎src/UniGetUI.PackageEngine.Serializable.Tests/UniGetUI.PackageEngine.Serializable.Tests.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
<ItemGroup>
1010
<PackageReference Include="coverlet.collector" Version="6.0.4"/>
1111
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.14.1" />
12+
<PackageReference Include="NSubstitute" Version="5.3.0" />
1213
<PackageReference Include="xunit" Version="2.9.3"/>
1314
<PackageReference Include="xunit.runner.visualstudio" Version="3.1.4">
1415
<PrivateAssets>all</PrivateAssets>
@@ -22,6 +23,9 @@
2223

2324
<ItemGroup>
2425
<ProjectReference Include="..\UniGetUI.PackageEngine.Serializable\UniGetUI.PackageEngine.Serializable.csproj" />
26+
<ProjectReference Include="..\UniGetUI.PackageEngine.PackageManagerClasses\UniGetUI.PackageEngine.Classes.csproj" />
27+
<ProjectReference Include="..\UniGetUI.PAckageEngine.Interfaces\UniGetUI.PackageEngine.Interfaces.csproj" />
28+
<ProjectReference Include="..\UniGetUI.Interface.Enums\UniGetUI.Interface.Enums.csproj" />
2529
</ItemGroup>
2630

2731
</Project>

‎src/UniGetUI/AppOperationHelper.cs‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -259,17 +259,39 @@ public static void Update(IReadOnlyList<IPackage> packages, bool? elevated = nul
259259
public static async Task UpdateAll()
260260
{
261261
foreach (IPackage package in UpgradablePackagesLoader.Instance.Packages)
262+
{
262263
if (package.Tag is not PackageTag.BeingProcessed and not PackageTag.OnQueue)
263-
await Update(package);
264+
{
265+
// Check for duplicate operations already in the queue (prevents duplicates during unattended updates)
266+
var isDuplicate = _operationList.Any(x => x.Operation is UpdatePackageOperation updateOp
267+
&& updateOp.Package.GetHash() == package.GetHash()
268+
&& (x.Operation.Status is OperationStatus.InQueue or OperationStatus.Running));
269+
270+
if (!isDuplicate)
271+
await Update(package);
272+
else
273+
Logger.Info($"Update operation for package {package.Id} is already queued or running. Skipping duplicate in UpdateAll.");
274+
}
275+
}
264276
}
265277

266278
public static async Task UpdateAllForManager(string managerName)
267279
{
268280
foreach (IPackage package in UpgradablePackagesLoader.Instance.Packages)
269281
{
270282
if (package.Tag is not PackageTag.OnQueue and not PackageTag.BeingProcessed
271-
&& package.Manager.Name == managerName || package.Manager.DisplayName == managerName)
272-
await Update(package);
283+
&& (package.Manager.Name == managerName || package.Manager.DisplayName == managerName))
284+
{
285+
// Check for duplicate operations already in the queue (prevents duplicates during unattended updates)
286+
var isDuplicate = _operationList.Any(x => x.Operation is UpdatePackageOperation updateOp
287+
&& updateOp.Package.GetHash() == package.GetHash()
288+
&& (x.Operation.Status is OperationStatus.InQueue or OperationStatus.Running));
289+
290+
if (!isDuplicate)
291+
await Update(package);
292+
else
293+
Logger.Info($"Update operation for package {package.Id} is already queued or running. Skipping duplicate in UpdateAllForManager.");
294+
}
273295
}
274296
}
275297

0 commit comments

Comments
 (0)