Skip to content

Commit e9a1080

Browse files
FrankFrank
authored andcommitted
fix: second-pass audit — robustness, logging, null safety
High: - AppLog.Write: wrap in try/catch to prevent recursive crash in global exception handlers when disk is full or log path is inaccessible - AppLog: add log rotation (5 MB cap, rotate to .bak) Medium: - VsrepoService: kill elevated process tree on cancellation (RunVsrepoElevatedAsync) - ProbeAsync: use VSRGUI_PROBE marker to extract JSON from noisy stdout - LoadDefinitions: null-guard Packages and Releases after deserialization - GetInstalledAsync: document assumed vsrepo column layout Low: - RestoreAppState: add _isRestoringState flag to prevent redundant FilterChanged/TargetComboBox_SelectionChanged during state restoration - EnsureEnvironmentAsync: clear path text boxes on probe failure - RunBusyOperationAsync: show status message when operation is skipped - NuGet.config: remove non-portable VS offline package source
1 parent 0af7ba0 commit e9a1080

4 files changed

Lines changed: 91 additions & 15 deletions

File tree

‎MainWindow.xaml.cs‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ public partial class MainWindow : WpfFluentWindow
4848
private bool _isBusy;
4949
private bool _isNavigationExpanded;
5050
private bool _isUpdatingThemeSelection;
51+
private bool _isRestoringState;
5152

5253
private sealed record PackageCommand(string Operation, bool Force);
5354

@@ -228,6 +229,10 @@ private async Task<bool> EnsureEnvironmentAsync(bool reprobe)
228229
}
229230

230231
AppendLog($"Probe failed: {_lastProbe.Message}");
232+
_lastPaths = null;
233+
DefinitionsPathTextBox.Text = string.Empty;
234+
BinariesPathTextBox.Text = string.Empty;
235+
ScriptsPathTextBox.Text = string.Empty;
231236
return false;
232237
}
233238

@@ -482,6 +487,19 @@ private void RestoreSelection(string? identifier)
482487
}
483488

484489
private void RestoreAppState()
490+
{
491+
_isRestoringState = true;
492+
try
493+
{
494+
RestoreAppStateCore();
495+
}
496+
finally
497+
{
498+
_isRestoringState = false;
499+
}
500+
}
501+
502+
private void RestoreAppStateCore()
485503
{
486504
_appState = _appStateService.Load();
487505
ApplyThemeSelection();
@@ -683,6 +701,7 @@ private async Task RunBusyOperationAsync(Func<Task> action, string errorContext)
683701
{
684702
if (_isBusy)
685703
{
704+
StatusTextBlock.Text = "Operation in progress, please wait...";
686705
return;
687706
}
688707

@@ -929,7 +948,7 @@ private void SearchTextBox_TextChanged(object sender, TextChangedEventArgs e)
929948
private void FilterChanged(object sender, SelectionChangedEventArgs e)
930949
{
931950
UpdateStatusSummarySelection();
932-
if (IsLoaded)
951+
if (IsLoaded && !_isRestoringState)
933952
{
934953
ApplyFilters(_selectedPackage?.Identifier);
935954
SaveAppState();
@@ -951,7 +970,7 @@ private void StatusSummaryButton_Click(object sender, RoutedEventArgs e)
951970

952971
private async void TargetComboBox_SelectionChanged(object sender, SelectionChangedEventArgs e)
953972
{
954-
if (IsLoaded && !string.IsNullOrWhiteSpace(PythonPathTextBox.Text) && !_isBusy)
973+
if (IsLoaded && !_isRestoringState && !string.IsNullOrWhiteSpace(PythonPathTextBox.Text) && !_isBusy)
955974
{
956975
_lastPaths = null;
957976
await RefreshPackagesAsync(updateDefinitions: false, reprobe: false, reloadDefinitions: false);
@@ -968,6 +987,7 @@ private async void PackagesGrid_MouseDoubleClick(object sender, MouseButtonEvent
968987
{
969988
if (_isBusy || PackagesGrid.SelectedItem is not PackageItem item)
970989
{
990+
if (_isBusy) StatusTextBlock.Text = "Operation in progress, please wait...";
971991
return;
972992
}
973993

‎NuGet.config‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,5 @@
33
<packageSources>
44
<clear />
55
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
6-
<add key="Microsoft Visual Studio Offline Packages" value="C:\Program Files (x86)\Microsoft SDKs\NuGetPackages\" />
76
</packageSources>
87
</configuration>

‎Services/AppLog.cs‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
using System.Diagnostics;
1+
using System.Diagnostics;
22
using System.IO;
33

44
namespace VSRepo_Gui.Services;
@@ -8,22 +8,51 @@ public static class AppLog
88
private static readonly object SyncRoot = new();
99
private static readonly string LogDirectory = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "VSRepo_Gui", "logs");
1010
private static readonly string LogPath = Path.Combine(LogDirectory, "latest.log");
11+
private const long MaxLogSizeBytes = 5 * 1024 * 1024; // 5 MB
1112

1213
public static string CurrentLogPath => LogPath;
1314

1415
public static void Write(string message)
1516
{
16-
lock (SyncRoot)
17+
try
1718
{
18-
Directory.CreateDirectory(LogDirectory);
19-
File.AppendAllText(LogPath, $"[{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff}] {message}{Environment.NewLine}");
19+
lock (SyncRoot)
20+
{
21+
Directory.CreateDirectory(LogDirectory);
22+
RotateIfNeeded();
23+
File.AppendAllText(LogPath, $"[{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff}] {message}{Environment.NewLine}");
24+
}
25+
}
26+
catch
27+
{
28+
// Must never throw — this is called from global exception handlers.
29+
// If logging fails (disk full, permissions), silently degrade.
30+
Debug.WriteLine($"AppLog.Write failed: {message}");
2031
}
2132
}
2233

2334
public static void Write(Exception exception, string context)
2435
{
2536
Write($"{context}: {exception}");
2637
}
27-
}
28-
29-
38+
39+
private static void RotateIfNeeded()
40+
{
41+
try
42+
{
43+
var info = new FileInfo(LogPath);
44+
if (info.Exists && info.Length > MaxLogSizeBytes)
45+
{
46+
var bakPath = LogPath + ".bak";
47+
File.Delete(bakPath);
48+
File.Move(LogPath, bakPath, overwrite: true);
49+
}
50+
}
51+
catch
52+
{
53+
// Best effort — don't let rotation failure block logging.
54+
}
55+
}
56+
}
57+
58+

‎Services/VsrepoService.cs‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public async Task<ProbeResult> ProbeAsync(string pythonExe, CancellationToken ca
118118
"spec = importlib.util.find_spec('vsrepo.vsrepo')",
119119
"result['has_vsrepo'] = spec is not None",
120120
"result['vsrepo_path'] = None if spec is None else spec.origin",
121-
"print(json.dumps(result))",
121+
"print('VSRGUI_PROBE:' + json.dumps(result))",
122122
});
123123

124124
var command = await RunProcessAsync(pythonExe, ["-c", code], cancellationToken);
@@ -127,7 +127,13 @@ public async Task<ProbeResult> ProbeAsync(string pythonExe, CancellationToken ca
127127
return new ProbeResult(false, command.CombinedOutput, pythonExe, false, false, null, null, null);
128128
}
129129

130-
using var document = JsonDocument.Parse(command.StdOut);
130+
// Extract the marker line — Python libraries may print warnings to stdout during import.
131+
var jsonLine = command.StdOut.Split(new[] { "\r\n", "\n" }, StringSplitOptions.RemoveEmptyEntries)
132+
.LastOrDefault(l => l.StartsWith("VSRGUI_PROBE:"))
133+
?? throw new InvalidOperationException($"Probe script produced no valid output:{Environment.NewLine}{command.StdOut}");
134+
var jsonText = jsonLine["VSRGUI_PROBE:".Length..];
135+
136+
using var document = JsonDocument.Parse(jsonText);
131137
var root = document.RootElement;
132138
var hasVs = root.GetProperty("has_vapoursynth").GetBoolean();
133139
var hasVsrepo = root.GetProperty("has_vsrepo").GetBoolean();
@@ -194,7 +200,17 @@ public async Task<CommandResult> RunVsrepoElevatedAsync(string pythonExe, string
194200
};
195201

196202
process.Start();
197-
await process.WaitForExitAsync(cancellationToken);
203+
try
204+
{
205+
await process.WaitForExitAsync(cancellationToken);
206+
}
207+
catch (OperationCanceledException)
208+
{
209+
// Best effort — Kill may fail on an elevated child from a non-elevated parent.
210+
try { process.Kill(entireProcessTree: true); }
211+
catch { /* ignored */ }
212+
throw;
213+
}
198214

199215
var std = File.Exists(outputPath)
200216
? await File.ReadAllTextAsync(outputPath, cancellationToken)
@@ -293,6 +309,9 @@ public async Task<Dictionary<string, InstalledPackageInfo>> GetInstalledAsync(st
293309
status = PackageInstallState.InstalledUnknown;
294310
}
295311

312+
// Assumed vsrepo output columns: [Name] [Installed] [Latest] [Type] [Identifier]
313+
// Index from end: ^[^5] [^4] [^3] [^2] [^1]
314+
// So tokens[^1] = Identifier, tokens[^3] = Latest (displayed as installed version reference).
296315
var identifier = tokens[^1].Trim();
297316
var installedVersion = tokens[^3].Trim();
298317
if (string.IsNullOrWhiteSpace(identifier))
@@ -309,8 +328,17 @@ public async Task<Dictionary<string, InstalledPackageInfo>> GetInstalledAsync(st
309328
public VsPackageRoot LoadDefinitions(string definitionsPath)
310329
{
311330
var json = File.ReadAllText(definitionsPath, Encoding.UTF8);
312-
return JsonSerializer.Deserialize<VsPackageRoot>(json, JsonOptions)
313-
?? new VsPackageRoot();
331+
var root = JsonSerializer.Deserialize<VsPackageRoot>(json, JsonOptions)
332+
?? new VsPackageRoot();
333+
334+
// System.Text.Json does not enforce C# nullable annotations — explicit null in JSON overrides default.
335+
root.Packages ??= [];
336+
foreach (var pkg in root.Packages)
337+
{
338+
pkg.Releases ??= [];
339+
}
340+
341+
return root;
314342
}
315343

316344
public bool IsAdministrator()

0 commit comments

Comments
 (0)