Skip to content

Commit 5c4f3a5

Browse files
JusterZhuclaude
andcommitted
fix(core): address Copilot review comments
- Fix PID parse index in CleanupOldTempDirectories (index 2, not 3) - Wrap all DeleteDirectory enumeration in try/catch for concurrent-safety - Extract directory name in ShouldSkipDirectory to handle both bare names and full paths - Update Option.cs GetOrAdd comment to note factory may run multiple times under contention - Dispose Process handle with 'using' in GracefulExit.CurrentProcessAsync - Guard PreParseVersions against duplicate keys from custom IDownloadSource implementations Co-authored-by: Claude <noreply@anthropic.com>
1 parent ee30c85 commit 5c4f3a5

5 files changed

Lines changed: 59 additions & 29 deletions

File tree

src/c#/GeneralUpdate.Core/Configuration/Option.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,9 @@ protected Option(string name)
6969
/// <exception cref="ArgumentNullException">Thrown when <paramref name="name" /> is <c>null</c>.</exception>
7070
public static Option<T> ValueOf<T>(string name, T defaultValue = default!)
7171
{
72-
// GetOrAdd is lock-free on ConcurrentDictionary; the factory runs at most once per key.
73-
// First registration wins — subsequent calls with the same name return the existing
74-
// singleton instance (preserving the original default value).
72+
// GetOrAdd is lock-free on ConcurrentDictionary. Under contention the
73+
// factory may run multiple times, but only one instance is stored and
74+
// returned by all racing callers. The factory must be side-effect-free.
7575
var raw = _registry.GetOrAdd(name, _ => new Option<T>(name, defaultValue));
7676

7777
// If the existing entry was registered with a different type T, create a new one.

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,15 @@ public static bool HasUpdate(
8686
/// <summary>Pre-parses versions for a list of assets to avoid repeated Semver.TryParse calls.</summary>
8787
private static Dictionary<DownloadAsset, SemVersion?> PreParseVersions(IEnumerable<DownloadAsset> assets)
8888
{
89-
return assets.ToDictionary(
90-
a => a,
91-
a => ParseVersion(a.Version));
89+
var map = new Dictionary<DownloadAsset, SemVersion?>();
90+
foreach (var a in assets)
91+
{
92+
// Custom IDownloadSource implementations could return duplicate records;
93+
// silently accept the first occurrence rather than throwing.
94+
if (!map.ContainsKey(a))
95+
map[a] = ParseVersion(a.Version);
96+
}
97+
return map;
9298
}
9399

94100
/// <summary>

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,16 @@ public bool IsBlacklistedFormat(string extension)
9595
/// Uses case-insensitive substring containment matching (<c>string.IndexOf</c>) for evaluation.
9696
/// The directory is considered skippable if its name contains any string from the <c>Directories</c> list.
9797
/// </remarks>
98-
public bool ShouldSkipDirectory(string directoryName)
99-
=> _config.Directories?.Any(d =>
100-
directoryName.StartsWith(d, StringComparison.OrdinalIgnoreCase)) == true;
98+
public bool ShouldSkipDirectory(string directoryOrPath)
99+
{
100+
// Extract the directory name from a possible full path so both
101+
// bare names ("backup-2026") and full paths (".backups/backup-2026") work.
102+
var dirName = Path.GetFileName(directoryOrPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar));
103+
if (string.IsNullOrEmpty(dirName)) return false;
104+
105+
return _config.Directories?.Any(d =>
106+
dirName.StartsWith(d, StringComparison.OrdinalIgnoreCase)) == true;
107+
}
101108

102109
/// <summary>
103110
/// Matches a file name against a simple Glob pattern.

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

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -248,9 +248,10 @@ public static void CleanupOldTempDirectories(TimeSpan? maxAge = null)
248248
var dirName = dirInfo.Name;
249249

250250
// Parse timestamp from "generalupdate_yyyy-MM-dd-HHmmss-fff_PID_name"
251+
// Splitting by '_': [0]=prefix, [1]=timestamp, [2]=PID, [3..]=name.
251252
// If the PID segment matches the current process, skip it.
252253
var parts = dirName.Split('_');
253-
if (parts.Length >= 4 && int.TryParse(parts[3], out var pid) && pid == currentPid)
254+
if (parts.Length >= 3 && int.TryParse(parts[2], out var pid) && pid == currentPid)
254255
continue;
255256

256257
if (dirInfo.CreationTimeUtc < cutoff)
@@ -346,42 +347,58 @@ public static void DeleteDirectory(string targetDir)
346347
// Enumerate then delete with per-item exception handling.
347348
// Between enumeration and deletion, concurrent processes may add/remove
348349
// files — handle these races gracefully instead of crashing.
349-
foreach (var file in Directory.GetFiles(targetDir))
350+
try
350351
{
352+
foreach (var file in Directory.GetFiles(targetDir))
353+
{
354+
try
355+
{
356+
File.SetAttributes(file, FileAttributes.Normal);
357+
File.Delete(file);
358+
}
359+
catch (FileNotFoundException) { /* raced away — already deleted */ }
360+
catch (DirectoryNotFoundException) { /* raced away */ }
361+
catch (UnauthorizedAccessException ex)
362+
{
363+
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete file '{Path.GetFileName(file)}': {ex.Message}");
364+
}
365+
}
366+
351367
try
352368
{
353-
File.SetAttributes(file, FileAttributes.Normal);
354-
File.Delete(file);
369+
foreach (var dir in Directory.GetDirectories(targetDir))
370+
{
371+
try
372+
{
373+
DeleteDirectory(dir);
374+
}
375+
catch (DirectoryNotFoundException) { /* raced away */ }
376+
catch (UnauthorizedAccessException ex)
377+
{
378+
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete directory '{Path.GetFileName(dir)}': {ex.Message}");
379+
}
380+
}
355381
}
356-
catch (FileNotFoundException) { /* raced away — already deleted */ }
357-
catch (DirectoryNotFoundException) { /* raced away */ }
382+
catch (DirectoryNotFoundException) { /* parent raced away */ }
358383
catch (UnauthorizedAccessException ex)
359384
{
360-
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete file '{Path.GetFileName(file)}': {ex.Message}");
385+
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot enumerate subdirectories: {ex.Message}");
361386
}
362-
}
363387

364-
foreach (var dir in Directory.GetDirectories(targetDir))
365-
{
366388
try
367389
{
368-
DeleteDirectory(dir);
390+
Directory.Delete(targetDir, false);
369391
}
370392
catch (DirectoryNotFoundException) { /* raced away */ }
371393
catch (UnauthorizedAccessException ex)
372394
{
373-
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete directory '{Path.GetFileName(dir)}': {ex.Message}");
395+
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete directory '{Path.GetFileName(targetDir)}': {ex.Message}");
374396
}
375397
}
376-
377-
try
378-
{
379-
Directory.Delete(targetDir, false);
380-
}
381-
catch (DirectoryNotFoundException) { /* raced away */ }
398+
catch (DirectoryNotFoundException) { /* parent raced away before enumeration */ }
382399
catch (UnauthorizedAccessException ex)
383400
{
384-
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot delete directory '{Path.GetFileName(targetDir)}': {ex.Message}");
401+
GeneralTracer.Warn($"StorageManager.DeleteDirectory: cannot enumerate '{targetDir}': {ex.Message}");
385402
}
386403
}
387404

src/c#/GeneralUpdate.Core/GracefulExit.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public static Task CurrentProcessAsync(int timeoutMs = 3000)
6161
// a no-op, but the process will exit when the async call stack unwinds.
6262
if (Environment.OSVersion.Platform == PlatformID.Win32NT)
6363
{
64-
var p = Process.GetCurrentProcess();
64+
using var p = Process.GetCurrentProcess();
6565
if (!p.HasExited)
6666
p.CloseMainWindow();
6767
}

0 commit comments

Comments
 (0)