Skip to content

Commit 2c92781

Browse files
JusterZhuclaude
andauthored
fix: resolve 14 code-review issues across Core/Differential/Bowl/Drivelution/Extension (#513)
* fix: resolve 14 code-review issues across Core/Differential/Bowl/Drivelution/Extension ## Bug fixes - **Brotli patch detection**: add BrotliFormatVersion (0x02) to format detection and decompression switch in BsdiffDiffer (#512) - **ReadFileWithBudget**: trim buffer to actual bytes read to prevent zero-padded data corrupting patch computation (#512) - **BsdiffDiffer.Search**: guard against sentinel -1 values from Split() that would cause IndexOutOfRangeException (#512) - **Bowl EnsureDirectory**: stop deleting the fail directory on every Prepare() call — preserves crash diagnostics from prior sessions (#512) - **FileNode.Equals**: return false for incompatible types instead of throwing ArgumentException, conforming to the IEquatable contract (#512) - **EventManager.Dispatch**: snapshot the delegate invocation list before enumerating to avoid race with concurrent AddListener/RemoveListener (#512) - **DiffPipeline**: add 'using' to both SemaphoreSlim instances to fix kernel handle leaks (#512) - **FileTree.Compare**: guard all 'node0' access paths with null-conditional operators to prevent NullReferenceException (#512) - **ClientStrategy.CheckFail**: use Version.TryParse instead of bare new Version() to avoid crashing on malformed version strings (#512) - **GeneralTracer**: wrap InitializeFileListener with lock to prevent duplicate TextTraceListener creation on concurrent day-rollover (#512) - **ProcessRunner**: cancel the Task.Delay timer via CancellationTokenSource when the process exits first, preventing timer leak (#512) - **OssStrategy**: resolve upgrade app name and search pattern based on RuntimeInformation.IsOSPlatform instead of hardcoding .exe (#512) - **VersionComparer**: use long instead of int for major/minor/patch to prevent overflow on extremely large version numbers (#512) - **ExtensionGeneralHost**: move name extraction before lifecycle hook calls and use extensionName (not file path) as Id in ExtensionMetadata (#512) ## Test coverage - BsdiffDifferTests: 6 new edge-case tests (single-byte, two-byte, repeating patterns, single-byte-change round-trips) - VersionComparerTests: 3 new large-number overflow tests - FileTreeTests: 6 new tests for null node0, imbalanced trees, matching trees - FileNodeTests: update Equals_NonFileNodeType test to match new behaviour All 1864 tests pass across 5 test suites (0 failures). Co-Authored-By: Claude <noreply@anthropic.com> * fix: address 5 Copilot review suggestions 1. BsdiffDiffer: recognize Brotli marker (0x02) as extended-header format even on netstandard2.0 (fail-fast) instead of mis-detecting as legacy BZip2 2. VersionComparer.ParseVersion: use TryParseNumeric with overflow guard instead of bare long.Parse (can throw on > Int64 values) 3. VersionComparer.ComparePrerelease: add IsNumericString fallback for purely-numeric identifiers that exceed long.MaxValue (length+ordinal) 4. OssStrategy: limit diagnostic file listing to 20 entries to prevent unbounded log noise on non-Windows 5. VersionComparerTests: fix doc comment (long.MaxValue -> int.MaxValue) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4bc591a commit 2c92781

18 files changed

Lines changed: 397 additions & 72 deletions

File tree

src/c#/GeneralUpdate.Bowl/Strategies/LinuxBowlStrategy.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -275,8 +275,10 @@ private static string DetectDistro()
275275

276276
private static void EnsureDirectory(string path)
277277
{
278-
if (Directory.Exists(path))
279-
Directory.Delete(path, recursive: true);
280-
Directory.CreateDirectory(path);
278+
// Create the fail directory if it does not yet exist.
279+
// Do NOT delete an existing directory — that would destroy
280+
// crash diagnostics from previous surveillance sessions.
281+
if (!Directory.Exists(path))
282+
Directory.CreateDirectory(path);
281283
}
282284
}

src/c#/GeneralUpdate.Bowl/Strategies/ProcessRunner.cs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public static async Task<ProcessExitResult> RunAsync(
2828
var outputLines = new List<string>();
2929

3030
using var process = new Process { StartInfo = startInfo };
31+
using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
3132
var tcs = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
3233

3334
process.OutputDataReceived += (_, e) =>
@@ -61,14 +62,17 @@ public static async Task<ProcessExitResult> RunAsync(
6162
process.BeginOutputReadLine();
6263
process.BeginErrorReadLine();
6364

64-
// Wait for exit or timeout/cancellation
65-
var completedTask = await Task.WhenAny(
66-
tcs.Task,
67-
Task.Delay(timeoutMs, ct)
68-
);
65+
// Wait for exit or timeout/cancellation.
66+
// Cancel the delay when the process exits first to avoid a timer leak.
67+
var delayTask = Task.Delay(timeoutMs, timeoutCts.Token);
68+
var completedTask = await Task.WhenAny(tcs.Task, delayTask);
69+
70+
// Cancel the opposing task so timers/resources are reclaimed promptly.
71+
timeoutCts.Cancel();
6972

7073
if (completedTask == tcs.Task)
7174
{
75+
try { await delayTask; } catch (OperationCanceledException) { /* cancelled — expected */ }
7276
var exitCode = await tcs.Task;
7377
GeneralTracer.Info($"ProcessRunner.RunAsync: process exited, ExitCode={exitCode}");
7478
// Snapshot output under lock to avoid race with in-flight handlers

src/c#/GeneralUpdate.Bowl/Strategies/WindowsBowlStrategy.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,10 @@ public Task PostProcessAsync(in BowlContext context,
6363

6464
private static void EnsureDirectory(string path)
6565
{
66-
if (Directory.Exists(path))
67-
StorageHelper.DeleteDirectory(path);
68-
Directory.CreateDirectory(path);
66+
// Create the fail directory if it does not yet exist.
67+
// Do NOT delete an existing directory — that would destroy
68+
// crash diagnostics from previous surveillance sessions.
69+
if (!Directory.Exists(path))
70+
Directory.CreateDirectory(path);
6971
}
7072
}

src/c#/GeneralUpdate.Core/Event/EventManager.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,15 @@ public void Dispatch<TEventArgs>(object sender, TEventArgs eventArgs) where TEve
119119
var type = typeof(Action<object, TEventArgs>);
120120
if (_dicDelegates.TryGetValue(type, out var existingDelegate))
121121
{
122+
// Snapshot the delegate invocation list to avoid a race with
123+
// concurrent AddListener / RemoveListener mutating the delegate
124+
// while we enumerate it. The ConcurrentDictionary protects the
125+
// dictionary structure but not the Delegate value itself.
126+
var invocationList = existingDelegate.GetInvocationList();
127+
122128
// Invoke each handler individually so one handler's exception
123129
// doesn't prevent others from being called.
124-
foreach (var handler in existingDelegate.GetInvocationList())
130+
foreach (var handler in invocationList)
125131
{
126132
try
127133
{

src/c#/GeneralUpdate.Core/FileSystem/FileNode.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,8 +232,7 @@ public FileNode SearchParent(long id)
232232
public override bool Equals(object obj)
233233
{
234234
if (obj == null) return false;
235-
var tempNode = obj as FileNode;
236-
if (tempNode == null) throw new ArgumentException(nameof(tempNode));
235+
if (obj is not FileNode tempNode) return false;
237236
return string.Equals(Hash, tempNode.Hash, StringComparison.OrdinalIgnoreCase) &&
238237
string.Equals(Name, tempNode.Name, StringComparison.OrdinalIgnoreCase);
239238
}

src/c#/GeneralUpdate.Core/FileSystem/FileTree.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,23 +209,23 @@ public void Compare(FileNode node, FileNode node0, ref List<FileNode> nodes)
209209
if (node != null && node.Left != null)
210210
{
211211
if (!node.Equals(node0) && node0 != null) nodes.Add(node0);
212-
Compare(node.Left, node0.Left, ref nodes);
212+
Compare(node.Left, node0?.Left, ref nodes);
213213
}
214214
else if (node0 != null && node0.Left != null)
215215
{
216216
nodes.Add(node0);
217-
Compare(node.Left, node0.Left, ref nodes);
217+
Compare(node?.Left, node0.Left, ref nodes);
218218
}
219219

220220
if (node != null && node.Right != null)
221221
{
222222
if (!node.Equals(node0) && node0 != null) nodes.Add(node0);
223-
Compare(node.Right, node0 == null ? null : node0.Right, ref nodes);
223+
Compare(node.Right, node0?.Right, ref nodes);
224224
}
225225
else if (node0 != null && node0.Right != null)
226226
{
227227
nodes.Add(node0);
228-
Compare(node == null ? null : node.Right, node0.Right, ref nodes);
228+
Compare(node?.Right, node0.Right, ref nodes);
229229
}
230230
else if (node0 != null)
231231
{

src/c#/GeneralUpdate.Core/Pipeline/DiffPipeline.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ public async Task CleanAsync(
222222
}
223223

224224
int completed = 0;
225-
var semaphore = new SemaphoreSlim(_options.MaxDegreeOfParallelism);
225+
using var semaphore = new SemaphoreSlim(_options.MaxDegreeOfParallelism);
226226

227227
var tasks = differentFiles.Select(file => Task.Run(async () =>
228228
{
@@ -342,7 +342,7 @@ public async Task DirtyAsync(
342342
}
343343

344344
int completed = 0;
345-
var semaphore = new SemaphoreSlim(_options.MaxDegreeOfParallelism);
345+
using var semaphore = new SemaphoreSlim(_options.MaxDegreeOfParallelism);
346346

347347
var matchedPairs = new List<(FileInfo OldFile, FileInfo PatchFile)>();
348348
foreach (var oldFile in oldFiles)

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -903,7 +903,10 @@ private bool CheckFail(string version)
903903
var fail = Environments.GetEnvironmentVariable("UpgradeFail");
904904
if (string.IsNullOrEmpty(fail) || string.IsNullOrEmpty(version))
905905
return false;
906-
return new Version(fail) >= new Version(version);
906+
if (!Version.TryParse(fail, out var failVersion) ||
907+
!Version.TryParse(version, out var versionParsed))
908+
return false;
909+
return failVersion >= versionParsed;
907910
}
908911

909912
/// <summary>

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using System.Diagnostics;
44
using System.IO;
55
using System.Linq;
6+
using System.Runtime.InteropServices;
67
using System.Text;
78
using System.Text.Json;
89
using System.Threading.Tasks;
@@ -258,15 +259,25 @@ private async Task ExecuteClientAsync()
258259
: installPath;
259260
var upgradeAppName = !string.IsNullOrWhiteSpace(_configInfo.UpdateAppName)
260261
? _configInfo.UpdateAppName
261-
: "GeneralUpdate.Upgrade.exe";
262+
: RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
263+
? "GeneralUpdate.Upgrade.exe"
264+
: "GeneralUpdate.Upgrade";
262265
var appPath = Path.Combine(upgradeDir, upgradeAppName);
263266
GeneralTracer.Info($"[OssClient] Resolved upgrade path: {appPath}");
264267

265-
// List exe files in the directory to help diagnose missing file issues
268+
// List executables in the directory to help diagnose missing file issues
266269
try
267270
{
268-
var dirFiles = Directory.GetFiles(upgradeDir, "*.exe").Select(f => Path.GetFileName(f));
269-
GeneralTracer.Info($"[OssClient] *.exe files in {upgradeDir}: [{string.Join(", ", dirFiles)}]");
271+
var searchPattern = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "*.exe" : "*";
272+
const int maxDisplay = 20;
273+
var dirFiles = Directory.EnumerateFiles(upgradeDir, searchPattern)
274+
.Take(maxDisplay + 1)
275+
.Select(f => Path.GetFileName(f))
276+
.ToList();
277+
var summary = dirFiles.Count > maxDisplay
278+
? $"[{string.Join(", ", dirFiles.Take(maxDisplay))}, ... and {dirFiles.Count - maxDisplay} more]"
279+
: $"[{string.Join(", ", dirFiles)}]";
280+
GeneralTracer.Info($"[OssClient] Files in {upgradeDir}: {summary}");
270281
}
271282
catch (Exception ex)
272283
{

src/c#/GeneralUpdate.Core/Tracer/GeneralTracer.cs

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -34,27 +34,30 @@ static GeneralTracer()
3434

3535
private static void InitializeFileListener()
3636
{
37-
//Ensure that log files are rotated on a daily basis
38-
var today = DateTime.Now.ToString("yyyy-MM-dd");
39-
if (today == _currentLogDate && _fileListener != null)
40-
return;
41-
42-
if (_fileListener != null)
37+
lock (_lockObj)
4338
{
44-
Trace.Listeners.Remove(_fileListener);
45-
_fileListener.Flush();
46-
_fileListener.Close();
47-
_fileListener.Dispose();
48-
}
39+
// Ensure that log files are rotated on a daily basis
40+
var today = DateTime.Now.ToString("yyyy-MM-dd");
41+
if (today == _currentLogDate && _fileListener != null)
42+
return;
4943

50-
var logDir = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Logs");
51-
Directory.CreateDirectory(logDir);
44+
if (_fileListener != null)
45+
{
46+
Trace.Listeners.Remove(_fileListener);
47+
_fileListener.Flush();
48+
_fileListener.Close();
49+
_fileListener.Dispose();
50+
}
51+
52+
var logDir = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Logs");
53+
Directory.CreateDirectory(logDir);
5254

53-
var logFileName = Path.Combine(logDir, $"generalupdate-trace {today}.log");
54-
_fileListener = new TextTraceListener(logFileName);
55-
56-
Trace.Listeners.Add(_fileListener);
57-
_currentLogDate = today;
55+
var logFileName = Path.Combine(logDir, $"generalupdate-trace {today}.log");
56+
_fileListener = new TextTraceListener(logFileName);
57+
58+
Trace.Listeners.Add(_fileListener);
59+
_currentLogDate = today;
60+
}
5861
}
5962

6063
public static void Debug(string message) => WriteTraceMessage(TraceLevel.Verbose, message);

0 commit comments

Comments
 (0)