Skip to content

fix: thread safety, event marshaling, entry-point robustness, config alignment#396

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/service-thread-safety
May 15, 2026
Merged

fix: thread safety, event marshaling, entry-point robustness, config alignment#396
laurentiu021 merged 1 commit into
mainfrom
fix/service-thread-safety

Conversation

@laurentiu021

@laurentiu021 laurentiu021 commented May 15, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes all 13 remaining findings from the FINAL_COMPLETE_AUDIT.

Service Thread-Safety (SVC-01/02, SVC-21/23, SVC-27, SVC-40, SVC-42)

  • AppAlertService — events marshaled to UI via captured SynchronizationContext
  • NetworkRepairService — SemaphoreSlim serializes PowerShellRunner access
  • PerformanceService — same SemaphoreSlim serialization for all subscribe/unsubscribe
  • PowerShellRunner — LineReceived documented as thread-pool event
  • StartupService — catches RuntimeBinderException on dynamic COM invocation
  • StartupService — stderr task guarded with 5s timeout (prevents GetAwaiter race)

ViewModel & Entry-Point (VM-19, VM-21, VM-28, ENTRY-01, ENTRY-03)

  • AppAlertsViewModel — uses Application.Current.Dispatcher (not CurrentDispatcher)
  • NetworkSharedState — FlushPending bg-thread path documented as intentional
  • AboutViewModel — no longer auto-downloads updates without user consent
  • App.xaml.cs — named pipe listener for single-instance activation when minimized to tray
  • MainWindow.xaml.cs — hooks Application.Exit as safety net for ViewModel disposal

Config (CFG-01, CFG-02, CFG-03)

  • SysManager.csproj — version updated from 0.12.1 to 0.48.21
  • SysManager.Tests + IntegrationTests — xunit bumped 2.5.3 -> 2.9.3
  • dependabot.yml — added IntegrationTests directory entry

Build

0 errors, 0 new warnings.

Files changed (14)

Services: AppAlertService.cs, NetworkRepairService.cs, PerformanceService.cs, PowerShellRunner.cs, StartupService.cs
ViewModels: AboutViewModel.cs, AppAlertsViewModel.cs, NetworkSharedState.cs
Entry: App.xaml.cs, MainWindow.xaml.cs
Config: SysManager.csproj, SysManager.Tests.csproj, IntegrationTests.csproj, dependabot.yml
Docs: CHANGELOG.md

Summary by CodeRabbit

Release Notes

  • New Features

    • Named-pipe single-instance activation for improved window restoration in tray mode.
    • Update check displays available version with manual download option instead of auto-downloading.
  • Bug Fixes

    • Enhanced thread-safe event delivery and PowerShell operation serialization.
    • Improved error handling for COM shortcut resolution and stderr timeouts.
    • Fixed UI-thread disposal behavior during application shutdown.
  • Chores

    • Updated xUnit from 2.5.3 to 2.9.3.
    • Version bumped to 0.48.21.

Review Change Stack

…alignment

SVC-01/02: AppAlertService events marshaled to UI via SynchronizationContext
SVC-21/23: NetworkRepairService + PerformanceService serialize PowerShellRunner access
SVC-27: PowerShellRunner.LineReceived documented as thread-pool event
SVC-40: StartupService catches RuntimeBinderException on dynamic COM
SVC-42: StartupService stderr task guarded with 5s timeout
VM-19: NetworkSharedState FlushPending bg-thread path documented as intentional
VM-21: AppAlertsViewModel uses Application.Current.Dispatcher
VM-28: AboutViewModel no longer auto-downloads updates without consent
ENTRY-01: App.xaml.cs named pipe listener for tray-minimized activation
ENTRY-03: MainWindow.xaml.cs hooks Application.Exit for ViewModel disposal
CFG-01: SysManager.csproj version updated to 0.48.21
CFG-02: xunit 2.5.3 -> 2.9.3 in Tests + IntegrationTests
CFG-03: dependabot.yml adds IntegrationTests directory
@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR enhances SysManager v0.48.22 with threading safety, event marshaling, and reliability improvements. Key changes include single-instance activation via named pipes, serialized PowerShell execution across services, SynchronizationContext-aware event delivery, defensive error/timeout handling, and proper ViewModel disposal on app exit. Version metadata and xUnit dependencies are also updated.

Changes

Threading, Concurrency, and Reliability Improvements

Layer / File(s) Summary
Single-instance activation via named pipes
SysManager/SysManager/App.xaml.cs
App introduces PipeName, _pipeCts, and StartPipeListener() to enable subsequent instances to trigger window activation via named pipe even when the main window is hidden in tray mode. ActivateExistingInstance() now attempts pipe connection first, falling back to Win32 window-handle lookup on timeout/error. Startup begins the listener after DI/tray setup; exit cancels and disposes it before resource cleanup.
Event delivery and SynchronizationContext marshaling
SysManager/SysManager/Services/AppAlertService.cs, SysManager/SysManager/ViewModels/AppAlertsViewModel.cs, SysManager/SysManager/Services/PowerShellRunner.cs
AppAlertService captures SynchronizationContext at construction and routes NewAppDetected through a RaiseNewAppDetected helper that posts to the captured context (or invokes directly if unavailable), ensuring events fire on the subscription thread. AppAlertsViewModel dispatcher resolution falls back to Application.Current?.Dispatcher. PowerShellRunner documentation clarifies that LineReceived fires on thread-pool threads and requires dispatcher marshaling by UI subscribers.
Serialized PowerShell execution
SysManager/SysManager/Services/PerformanceService.cs, SysManager/SysManager/Services/NetworkRepairService.cs
PerformanceService and NetworkRepairService each introduce a SemaphoreSlim gate to serialize PowerShell runner access and LineReceived event subscriptions. GetActivePlanAsync, EnsureUltimatePerformancePlanAsync, FindPlanGuidByNameAsync, and ReadProcessorMinPercentAsync acquire the gate before subscribing and release it in finally after execution, preventing concurrent event handler interleaving.
Defensive error and timeout handling
SysManager/SysManager/Services/StartupService.cs
.lnk shortcut resolution now catches RuntimeBinderException with matching debug logging. SetTaskSchedulerEnabled bounds stderr-reading wait to 5 seconds, falling back to empty stderr on timeout instead of blocking indefinitely.
Disposal safety on application exit
SysManager/SysManager/MainWindow.xaml.cs
MainWindow constructor registers an Application.Current.Exit handler that disposes MainWindowViewModel even when OnClosed is not invoked during abrupt shutdown.
Headless dispatcher documentation
SysManager/SysManager/ViewModels/NetworkSharedState.cs
OnSample clarifies that when Dispatcher is null (headless/unit-test scenarios), ObservableCollection modifications occur immediately on the calling thread because collections are not UI-bound.
Update download behavior
SysManager/SysManager/ViewModels/AboutViewModel.cs
"Update available" status now shows an explicit "Click Download" prompt with version and publish date instead of auto-initiating download.
Version and dependency updates
SysManager/SysManager/SysManager.csproj, SysManager/SysManager.Tests/SysManager.Tests.csproj, SysManager/SysManager.IntegrationTests/SysManager.IntegrationTests.csproj, .github/dependabot.yml
SysManager.csproj version bumped to 0.48.21; xUnit updated from 2.5.3 to 2.9.3 in both test projects; Dependabot extended with weekly Monday NuGet updates for IntegrationTests with 3 open PR limit and dependencies/tests labels.
Release notes
CHANGELOG.md
Version 0.48.22 (2026-05-15) documents Fixed items for threading/marshalling, PowerShell serialization, defensive handling, pipe activation, and disposal safety; Changed items list version and Dependabot updates.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • laurentiu021/SystemManager#392: Adjusts mutex release exception handling on app exit, which complements this PR's named-pipe listener disposal logic and overall App.xaml.cs shutdown flow.
  • laurentiu021/SystemManager#291: Made PowerShellRunner transient to prevent LineReceived event sharing; this PR serializes event subscriptions in PerformanceService and NetworkRepairService to directly address the cross-talk issue.
  • laurentiu021/SystemManager#268: Earlier single-instance and tray-mode integration that modified App startup/shutdown; this PR extends that foundation with named-pipe activation signaling while maintaining tray service lifecycle coordination.

Poem

🐰 Threads now marshalled, no more cross-talk cries,
Pipes carry signals when the instance hides,
Gates protect the PowerShell's shared domain,
Timeouts prevent the endless waiting pain—
Disposal's safe, and App exits with grace! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: thread safety improvements, event marshaling fixes, entry-point robustness enhancements, and configuration alignment across the codebase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/service-thread-safety

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
SysManager/SysManager/Services/NetworkRepairService.cs (1)

25-39: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same semaphore-leak risk: subscription is outside try in all three methods.

After WaitAsync, handler attachment happens before try. Any exception during attach can leave _gate permanently held.

Suggested fix pattern (apply to FlushDnsAsync, ResetWinsockAsync, ResetTcpIpAsync)
 await _gate.WaitAsync(ct).ConfigureAwait(false);
 var output = new System.Collections.Concurrent.ConcurrentQueue<string>();
 void OnLine(PowerShellLine line) => output.Enqueue(line.Text);
-_ps.LineReceived += OnLine;
-try
+var subscribed = false;
+try
 {
+    _ps.LineReceived += OnLine;
+    subscribed = true;
     var exit = await _ps.RunProcessAsync("ipconfig.exe", "/flushdns", ct, PowerShellRunner.OemEncoding)
         .ConfigureAwait(false);
@@
-finally { _ps.LineReceived -= OnLine; _gate.Release(); }
+finally
+{
+    if (subscribed) _ps.LineReceived -= OnLine;
+    _gate.Release();
+}

Also applies to: 47-61, 69-83

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/Services/NetworkRepairService.cs` around lines 25 - 39,
The semaphore (_gate) is acquired before subscribing the handler (OnLine) to
_ps.LineReceived, so if handler attachment throws the semaphore can remain held;
update FlushDnsAsync, ResetWinsockAsync, and ResetTcpIpAsync to attach the
LineReceived handler inside the try block after WaitAsync succeeds (or otherwise
ensure attachment cannot throw outside try), and keep the existing finally to
always unsubscribe (_ps.LineReceived -= OnLine) and release (_gate.Release());
this guarantees the semaphore is always released even if subscription or
subsequent code throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@SysManager/SysManager/App.xaml.cs`:
- Around line 96-104: After successfully signaling the running instance via the
named pipe, return immediately so the method does not continue to the
process/window scan fallback; specifically, inside the try block that creates
the NamedPipeClientStream (using PipeName and PipeDirection.Out) and calls
client.Connect(...), add an early return right after the successful
client.Connect(...) call to ensure the Win32 fallback only runs when the pipe
path fails.

In `@SysManager/SysManager/MainWindow.xaml.cs`:
- Around line 21-29: The viewmodel may be disposed twice because both
OnApplicationExit (hooked to Application.Current.Exit) and MainWindow.OnClosed
call (DataContext as MainWindowViewModel)?.Dispose(); fix by making disposal
idempotent: add a private bool _disposed field and check/set it inside
MainWindowViewModel.Dispose() so subsequent calls return immediately, or
alternatively unsubscribe Application.Current.Exit in MainWindow.OnClosed
(Application.Current.Exit -= OnApplicationExit) before calling Dispose() to
guarantee a single invocation; reference MainWindowViewModel.Dispose(),
MainWindow.OnClosed, and OnApplicationExit/Application.Current.Exit when making
the change.

In `@SysManager/SysManager/Services/PerformanceService.cs`:
- Around line 167-172: The semaphore (_psGate) is acquired before the try and
the event handler (OnLine -> _ps.LineReceived += OnLine) is subscribed outside
the try, so if subscription throws the semaphore is never released; fix by
moving the handler subscription inside the try block immediately after await
_psGate.WaitAsync(ct) (i.e., acquire gate, then enter try where you subscribe
the handler), and ensure the finally block always removes the handler
(_ps.LineReceived -= OnLine) and releases the gate (_psGate.Release()); apply
the same pattern to the other methods that subscribe to _ps.LineReceived (the
blocks around RunProcessAsync and their corresponding OnLine handlers).

In `@SysManager/SysManager/ViewModels/AboutViewModel.cs`:
- Line 103: The UpdateStatus string currently always appends
LatestPublishedLabel in parentheses which yields empty "()" when
LatestPublishedLabel is empty; update the logic where UpdateStatus is set
(referencing UpdateStatus, LatestVersionLabel and LatestPublishedLabel) to
conditionally include the published label only when LatestPublishedLabel is
non-empty — e.g., build the suffix as either $" ({LatestPublishedLabel})" when
non-empty or "" when empty, then set UpdateStatus = $"Update available:
{LatestVersionLabel}{suffix}. Click Download to get it."; ensure
trimming/spacing remains correct.

---

Outside diff comments:
In `@SysManager/SysManager/Services/NetworkRepairService.cs`:
- Around line 25-39: The semaphore (_gate) is acquired before subscribing the
handler (OnLine) to _ps.LineReceived, so if handler attachment throws the
semaphore can remain held; update FlushDnsAsync, ResetWinsockAsync, and
ResetTcpIpAsync to attach the LineReceived handler inside the try block after
WaitAsync succeeds (or otherwise ensure attachment cannot throw outside try),
and keep the existing finally to always unsubscribe (_ps.LineReceived -= OnLine)
and release (_gate.Release()); this guarantees the semaphore is always released
even if subscription or subsequent code throws.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f0c4d9fb-ca21-458a-a439-cf1ede359a99

📥 Commits

Reviewing files that changed from the base of the PR and between e161f26 and aad70c6.

📒 Files selected for processing (15)
  • .github/dependabot.yml
  • CHANGELOG.md
  • SysManager/SysManager.IntegrationTests/SysManager.IntegrationTests.csproj
  • SysManager/SysManager.Tests/SysManager.Tests.csproj
  • SysManager/SysManager/App.xaml.cs
  • SysManager/SysManager/MainWindow.xaml.cs
  • SysManager/SysManager/Services/AppAlertService.cs
  • SysManager/SysManager/Services/NetworkRepairService.cs
  • SysManager/SysManager/Services/PerformanceService.cs
  • SysManager/SysManager/Services/PowerShellRunner.cs
  • SysManager/SysManager/Services/StartupService.cs
  • SysManager/SysManager/SysManager.csproj
  • SysManager/SysManager/ViewModels/AboutViewModel.cs
  • SysManager/SysManager/ViewModels/AppAlertsViewModel.cs
  • SysManager/SysManager/ViewModels/NetworkSharedState.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build & unit tests
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)

297-300: LGTM!

SysManager/SysManager/Services/StartupService.cs (2)

114-117: LGTM!


432-434: LGTM!

SysManager/SysManager/SysManager.csproj (1)

14-16: LGTM!

SysManager/SysManager.IntegrationTests/SysManager.IntegrationTests.csproj (1)

16-16: LGTM!

SysManager/SysManager.Tests/SysManager.Tests.csproj (1)

16-16: LGTM!

.github/dependabot.yml (1)

40-50: LGTM!

CHANGELOG.md (1)

9-45: LGTM!

SysManager/SysManager/Services/PerformanceService.cs (1)

30-30: 🏗️ Heavy lift

PowerShellRunner is Transient, not shared across services.

The DI configuration in ServiceRegistration.cs line 22 registers PowerShellRunner as Transient:

services.AddTransient<PowerShellRunner>();

The inline comment explicitly states: "PowerShellRunner is Transient — each consumer gets its own instance to avoid LineReceived event cross-talk between tabs."

Since each PerformanceService instance receives its own PowerShellRunner instance, the instance-level _psGate semaphore does not need to protect against cross-service access. The gate protects only concurrent calls within a single service instance, which is appropriate.

The semaphore usage is not uniform across all methods in PerformanceService—some methods (e.g., DuplicateSchemeAsync at line 217) properly await the gate before subscribing to events, while others (e.g., GetActivePlanAsync at line 167) subscribe first and only release the gate in the finally block without acquiring it. However, this inconsistency does not create a cross-service serialization gap because runners are not shared.

SysManager/SysManager/App.xaml.cs (1)

6-7: LGTM!

Also applies to: 19-23, 77-84, 123-153

SysManager/SysManager/Services/AppAlertService.cs (1)

23-39: LGTM!

Also applies to: 176-176, 190-208

SysManager/SysManager/ViewModels/AppAlertsViewModel.cs (1)

6-7: LGTM!

Also applies to: 32-35

SysManager/SysManager/Services/PowerShellRunner.cs (1)

24-29: LGTM!

Comment on lines +96 to +104
// Try named pipe first — works even when the window is hidden (tray mode)
try
{
using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out);
client.Connect(timeout: 2000);
// Connection alone signals the running instance to activate
}
catch (TimeoutException) { /* pipe not available, fall back to window activation */ }
catch (IOException) { /* pipe not available */ }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Return immediately after a successful pipe signal.

Lines 96-104 still fall through into the process/window scan after client.Connect(...) succeeds, so a normal second launch can trigger both activation paths instead of using the Win32 path only as fallback. Adding an early return; keeps the old logic as a true backup path.

Proposed fix
         try
         {
             using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out);
             client.Connect(timeout: 2000);
             // Connection alone signals the running instance to activate
+            return;
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Try named pipe first — works even when the window is hidden (tray mode)
try
{
using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out);
client.Connect(timeout: 2000);
// Connection alone signals the running instance to activate
}
catch (TimeoutException) { /* pipe not available, fall back to window activation */ }
catch (IOException) { /* pipe not available */ }
// Try named pipe first — works even when the window is hidden (tray mode)
try
{
using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out);
client.Connect(timeout: 2000);
// Connection alone signals the running instance to activate
return;
}
catch (TimeoutException) { /* pipe not available, fall back to window activation */ }
catch (IOException) { /* pipe not available */ }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/App.xaml.cs` around lines 96 - 104, After successfully
signaling the running instance via the named pipe, return immediately so the
method does not continue to the process/window scan fallback; specifically,
inside the try block that creates the NamedPipeClientStream (using PipeName and
PipeDirection.Out) and calls client.Connect(...), add an early return right
after the successful client.Connect(...) call to ensure the Win32 fallback only
runs when the pipe path fails.

Comment on lines +21 to 29
// Ensure ViewModel disposal even if OnClosed is not called (e.g. app shutdown)
if (Application.Current != null)
Application.Current.Exit += OnApplicationExit;
}

private void OnApplicationExit(object sender, ExitEventArgs e)
{
(DataContext as MainWindowViewModel)?.Dispose();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether MainWindowViewModel.Dispose() is idempotent and whether duplicate disposal is currently possible.

set -euo pipefail

echo "== MainWindow exit/dispose call sites =="
fd -i '^MainWindow\.xaml\.cs$' --exec rg -n -C3 --type=cs 'Exit \+=|OnApplicationExit|OnClosed|Dispose\s*\(' {}

echo
echo "== MainWindowViewModel dispose implementation =="
for f in $(fd -i '^MainWindowViewModel\.cs$'); do
  echo "-- $f --"
  rg -n -C5 --type=cs '\bclass\s+MainWindowViewModel\b|\bvoid\s+Dispose\s*\(|\bbool\s+_disposed\b|Interlocked|GC\.SuppressFinalize' "$f"
done

echo
echo "Expected verification result:"
echo "1) Dispose implementation has an idempotency guard (_disposed/Interlocked/etc.), OR"
echo "2) MainWindow is updated to ensure dispose is invoked exactly once."

Repository: laurentiu021/SystemManager

Length of output: 2067


Add an idempotency guard to MainWindowViewModel.Dispose() or ensure single invocation.

On normal shutdown, both OnApplicationExit (line 28) and OnClosed (line 94) call Dispose() on the same MainWindowViewModel instance. The Dispose() implementation has no guard against multiple calls and will attempt to dispose already-disposed items, risking ObjectDisposedException.

Either:

  1. Add a _disposed boolean guard in MainWindowViewModel.Dispose() to make it idempotent, or
  2. Unsubscribe from Application.Current.Exit in OnClosed to ensure disposal runs exactly once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/MainWindow.xaml.cs` around lines 21 - 29, The viewmodel
may be disposed twice because both OnApplicationExit (hooked to
Application.Current.Exit) and MainWindow.OnClosed call (DataContext as
MainWindowViewModel)?.Dispose(); fix by making disposal idempotent: add a
private bool _disposed field and check/set it inside
MainWindowViewModel.Dispose() so subsequent calls return immediately, or
alternatively unsubscribe Application.Current.Exit in MainWindow.OnClosed
(Application.Current.Exit -= OnApplicationExit) before calling Dispose() to
guarantee a single invocation; reference MainWindowViewModel.Dispose(),
MainWindow.OnClosed, and OnApplicationExit/Application.Current.Exit when making
the change.

Comment on lines +167 to +172
await _psGate.WaitAsync(ct).ConfigureAwait(false);
var lines = new List<string>();
void OnLine(PowerShellLine l) => lines.Add(l.Text);
_ps.LineReceived += OnLine;
try { await _ps.RunProcessAsync("powercfg.exe", "/getactivescheme", ct, PowerShellRunner.OemEncoding); }
finally { _ps.LineReceived -= OnLine; }
finally { _ps.LineReceived -= OnLine; _psGate.Release(); }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wrap handler subscription in the same try/finally as gate ownership.

Right now, WaitAsync succeeds before try, and handler subscription also happens before try. If subscription throws (e.g., disposal race), the semaphore is never released, blocking future calls.

Suggested pattern (apply to all four methods)
 await _psGate.WaitAsync(ct).ConfigureAwait(false);
 var lines = new List<string>();
 void OnLine(PowerShellLine l) => lines.Add(l.Text);
-_ps.LineReceived += OnLine;
-try { await _ps.RunProcessAsync("powercfg.exe", "/getactivescheme", ct, PowerShellRunner.OemEncoding); }
-finally { _ps.LineReceived -= OnLine; _psGate.Release(); }
+var subscribed = false;
+try
+{
+    _ps.LineReceived += OnLine;
+    subscribed = true;
+    await _ps.RunProcessAsync("powercfg.exe", "/getactivescheme", ct, PowerShellRunner.OemEncoding).ConfigureAwait(false);
+}
+finally
+{
+    if (subscribed) _ps.LineReceived -= OnLine;
+    _psGate.Release();
+}

Also applies to: 217-220, 238-243, 468-477

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/Services/PerformanceService.cs` around lines 167 - 172,
The semaphore (_psGate) is acquired before the try and the event handler (OnLine
-> _ps.LineReceived += OnLine) is subscribed outside the try, so if subscription
throws the semaphore is never released; fix by moving the handler subscription
inside the try block immediately after await _psGate.WaitAsync(ct) (i.e.,
acquire gate, then enter try where you subscribe the handler), and ensure the
finally block always removes the handler (_ps.LineReceived -= OnLine) and
releases the gate (_psGate.Release()); apply the same pattern to the other
methods that subscribe to _ps.LineReceived (the blocks around RunProcessAsync
and their corresponding OnLine handlers).

// gracefully if blocked.
if (!IsDownloading && DownloadedPath == null)
_ = DownloadAsync();
UpdateStatus = $"Update available: {LatestVersionLabel} ({LatestPublishedLabel}). Click Download to get it.";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid empty-date parentheses in update status text.

When LatestPublishedLabel is empty, this renders (...) in the UI. Build the status string conditionally to avoid awkward text.

Proposed fix
-                UpdateStatus = $"Update available: {LatestVersionLabel} ({LatestPublishedLabel}). Click Download to get it.";
+                var publishedSuffix = string.IsNullOrWhiteSpace(LatestPublishedLabel)
+                    ? string.Empty
+                    : $" ({LatestPublishedLabel})";
+                UpdateStatus = $"Update available: {LatestVersionLabel}{publishedSuffix}. Click Download to get it.";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
UpdateStatus = $"Update available: {LatestVersionLabel} ({LatestPublishedLabel}). Click Download to get it.";
var publishedSuffix = string.IsNullOrWhiteSpace(LatestPublishedLabel)
? string.Empty
: $" ({LatestPublishedLabel})";
UpdateStatus = $"Update available: {LatestVersionLabel}{publishedSuffix}. Click Download to get it.";
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/ViewModels/AboutViewModel.cs` at line 103, The
UpdateStatus string currently always appends LatestPublishedLabel in parentheses
which yields empty "()" when LatestPublishedLabel is empty; update the logic
where UpdateStatus is set (referencing UpdateStatus, LatestVersionLabel and
LatestPublishedLabel) to conditionally include the published label only when
LatestPublishedLabel is non-empty — e.g., build the suffix as either $"
({LatestPublishedLabel})" when non-empty or "" when empty, then set UpdateStatus
= $"Update available: {LatestVersionLabel}{suffix}. Click Download to get it.";
ensure trimming/spacing remains correct.

@laurentiu021
laurentiu021 merged commit 940e583 into main May 15, 2026
5 checks passed
@laurentiu021
laurentiu021 deleted the fix/service-thread-safety branch May 15, 2026 12:47
laurentiu021 added a commit that referenced this pull request May 22, 2026
…vice (#419)

## Summary

**Batch 2** of the QA bug fix series.

### Issue #396 — CancellationTokenSource not disposed
Added Dispose(bool) override to **8 ViewModels** that had CTS fields but
no cleanup on dispose:
- AppUpdatesViewModel
- DiskAnalyzerViewModel
- DriversViewModel
- DuplicateFileViewModel
- LogsViewModel
- SpeedTestViewModel
- TracerouteViewModel
- UninstallerViewModel

Each override disposes the CTS field and calls �ase.Dispose(disposing).
The existing VMs that already had Dispose (CleanupVM, DeepCleanupVM,
NetworkVM, SystemHealthVM, WindowsUpdateVM) were verified and left
unchanged.

### Issue #413 — Bare catch in UpdateService
Replaced bare \catch\ blocks with specific exception types + Serilog
logging:
- **GetRecentAsync**: catches \OperationCanceledException\,
\HttpRequestException\, \JsonException\
- **DownloadAsync**: catches \OperationCanceledException\,
\HttpRequestException\, \IOException\
- Cleanup catch blocks in DownloadAsync now catch \IOException\ and
\UnauthorizedAccessException\ instead of bare catch

### Testing
- Main project builds with 0 errors
- No behavior changes — only proper resource cleanup and specific
exception handling
- CI will validate full test suite

Closes #396, Closes #413

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021 added a commit that referenced this pull request May 22, 2026
Adds CHANGELOG entries for all 9 releases from the QA bug fix session:

- **v0.28.16** — Dispose lifecycle (#395, #410)
- **v0.28.17** — CTS disposal + bare catch (#396, #413)
- **v0.28.18** — Input validation + null checks (#397, #398)
- **v0.28.19** — JSON error handling (#400)
- **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401,
#402, #403)
- **v0.28.21** — Audit logging + error messages (#405, #407)
- **v0.28.22** — SHA256 verification (#408, #409)
- **v0.28.23** — Service timeout + snapshot persist + traceroute DNS
(#414, #415, #416)
- **v0.28.24** — Accessibility (#411)

18 bugs fixed in total.

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021 added a commit that referenced this pull request May 22, 2026
…alignment (#396)

SVC-01/02: AppAlertService events marshaled to UI via SynchronizationContext
SVC-21/23: NetworkRepairService + PerformanceService serialize PowerShellRunner access
SVC-27: PowerShellRunner.LineReceived documented as thread-pool event
SVC-40: StartupService catches RuntimeBinderException on dynamic COM
SVC-42: StartupService stderr task guarded with 5s timeout
VM-19: NetworkSharedState FlushPending bg-thread path documented as intentional
VM-21: AppAlertsViewModel uses Application.Current.Dispatcher
VM-28: AboutViewModel no longer auto-downloads updates without consent
ENTRY-01: App.xaml.cs named pipe listener for tray-minimized activation
ENTRY-03: MainWindow.xaml.cs hooks Application.Exit for ViewModel disposal
CFG-01: SysManager.csproj version updated to 0.48.21
CFG-02: xunit 2.5.3 -> 2.9.3 in Tests + IntegrationTests
CFG-03: dependabot.yml adds IntegrationTests directory

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants