Skip to content

fix: drop async-void pipe listener and sync-over-async wrappers #616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laurentiu021 merged 1 commit into from
Jun 3, 2026
Merged

fix: drop async-void pipe listener and sync-over-async wrappers #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## [1.18.1] - 2026-06-03
## [1.18.2] - 2026-06-03

### Fixed
- **Pipe listener no longer fire-and-forgets via `async void`.** `App.StartPipeListener` was an async-void method, meaning any exception escaping the loop would crash the process via the AppDomain handler. Renamed to `StartPipeListenerAsync` returning `Task`; `OnStartup` calls it as `_ = StartPipeListenerAsync()` so a stray exception flows through `TaskScheduler.UnobservedTaskException` (logged) instead of terminating the app.
- **StartupService — removed sync wrapper over async.** `SetEnabled` (sync) was a thin wrapper around `SetEnabledAsync` using `.GetAwaiter().GetResult()`. The wrapper is gone; tests now call `SetEnabledAsync` directly via xUnit `Task` test methods.
- **Schtasks stderr read** — replaced `stderrTask.Wait(timeout) ? .GetAwaiter().GetResult() : ""` with `await stderrTask.WaitAsync(timeout)` so the read is fully async with a clean timeout fallback.

### Fixed
- **Privacy Toggles no longer write to the registry on every click.** Toggling a switch now updates local state only; the user must press **Apply** to write pending changes. A live counter shows how many changes are pending, and **Discard** reverts them without touching the registry. Prevents accidental system changes when scrolling through the toggle list.
Expand Down
22 changes: 11 additions & 11 deletions SysManager/SysManager.Tests/StartupToggleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
namespace SysManager.Tests;

/// <summary>
/// Tests for <see cref="StartupService.SetEnabled"/> covering registry-based
/// Tests for <see cref="StartupService.SetEnabledAsync"/> covering registry-based
/// and Task Scheduler entries, plus specific error messages (#159, #160).
/// </summary>
public class StartupToggleTests
{
[Fact]
public void SetEnabled_TaskScheduler_EmptyPath_ReturnsFalseWithMessage()
public async Task SetEnabledAsync_TaskScheduler_EmptyPath_ReturnsFalseWithMessage()
{
var entry = new StartupEntry
{
Expand All @@ -26,14 +26,14 @@ public void SetEnabled_TaskScheduler_EmptyPath_ReturnsFalseWithMessage()
StatusText = "Enabled"
};

var result = StartupService.SetEnabled(entry, false);
var result = await StartupService.SetEnabledAsync(entry, false);

Assert.False(result);
Assert.Contains("task path unknown", entry.StatusText, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void SetEnabled_TaskScheduler_NullPath_ReturnsFalseWithMessage()
public async Task SetEnabledAsync_TaskScheduler_NullPath_ReturnsFalseWithMessage()
{
var entry = new StartupEntry
{
Expand All @@ -45,14 +45,14 @@ public void SetEnabled_TaskScheduler_NullPath_ReturnsFalseWithMessage()
StatusText = "Enabled"
};

var result = StartupService.SetEnabled(entry, false);
var result = await StartupService.SetEnabledAsync(entry, false);

Assert.False(result);
Assert.Contains("task path unknown", entry.StatusText, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void SetEnabled_RegistryEntry_NeverShowsGenericAdminMessage()
public async Task SetEnabledAsync_RegistryEntry_NeverShowsGenericAdminMessage()
{
var entry = new StartupEntry
{
Expand All @@ -65,13 +65,13 @@ public void SetEnabled_RegistryEntry_NeverShowsGenericAdminMessage()
StatusText = "Enabled"
};

var result = StartupService.SetEnabled(entry, false);
var result = await StartupService.SetEnabledAsync(entry, false);

Assert.DoesNotContain("may need admin", entry.StatusText, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void SetEnabled_TaskScheduler_WithBogusPath_ReturnsFalseWithSpecificError()
public async Task SetEnabledAsync_TaskScheduler_WithBogusPath_ReturnsFalseWithSpecificError()
{
var entry = new StartupEntry
{
Expand All @@ -83,15 +83,15 @@ public void SetEnabled_TaskScheduler_WithBogusPath_ReturnsFalseWithSpecificError
StatusText = "Enabled"
};

var result = StartupService.SetEnabled(entry, false);
var result = await StartupService.SetEnabledAsync(entry, false);

Assert.False(result);
Assert.StartsWith("Error", entry.StatusText, StringComparison.OrdinalIgnoreCase);
Assert.DoesNotContain("may need admin", entry.StatusText, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void SetEnabled_PreservesEntryState_OnFailure()
public async Task SetEnabledAsync_PreservesEntryState_OnFailure()
{
var entry = new StartupEntry
{
Expand All @@ -103,7 +103,7 @@ public void SetEnabled_PreservesEntryState_OnFailure()
StatusText = "Enabled"
};

StartupService.SetEnabled(entry, false);
await StartupService.SetEnabledAsync(entry, false);

Assert.True(entry.IsEnabled);
}
Expand Down
12 changes: 8 additions & 4 deletions SysManager/SysManager/App.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,10 @@ protected override void OnStartup(StartupEventArgs e)

ThemeService.Instance.Initialize();

// Start listening for activation requests from subsequent instances
StartPipeListener();
// Start listening for activation requests from subsequent instances.
// Fire-and-forget is intentional — the listener loop runs for the app
// lifetime and is cancelled via _pipeCts on OnExit.
_ = StartPipeListenerAsync();
Comment on lines +81 to +84
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 3, 2026

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

🧩 Analysis chain

🌐 Web query:

In modern .NET / WPF, when does TaskScheduler.UnobservedTaskException fire for a faulted fire-and-forget Task, and is it guaranteed to run before process exit?

💡 Result:

The TaskScheduler.UnobservedTaskException event is triggered when a faulted Task containing an unobserved exception is garbage collected [1][2]. For a fire-and-forget task, this means the event will only fire after the task has completed with an exception, the task object has become unreachable, and the Garbage Collector (GC) has performed a collection that finalizes the task [1][2]. Crucially, this event is not guaranteed to run before the process exits [3]. Because it relies on the GC, if the application terminates before a garbage collection occurs, the event will never be raised [1][3]. Consequently, you cannot rely on this event as a robust mechanism for logging or handling exceptions in fire-and-forget tasks [1][2]. In modern.NET (including WPF applications), the recommended practice is to explicitly observe all tasks, even those intended as fire-and-forget, by attaching a continuation (e.g., ContinueWith) or using an asynchronous wrapper that handles exceptions, such as a try-catch block inside the task itself [2][4]. Relying on UnobservedTaskException is discouraged because its timing is nondeterministic and dependent on GC behavior [1][5].

Citations:

Don’t rely on TaskScheduler.UnobservedTaskException for pipe-listener failures (SysManager/SysManager/App.xaml.cs lines 81-84).

StartPipeListenerAsync() is started fire-and-forget (_ = ...), so any exception escaping the listener loop is surfaced only when the faulted Task is later garbage-collected; this timing is nondeterministic and may not occur before process exit. Catch/log inside StartPipeListenerAsync or attach an explicit fault-only continuation at the call site (instead of relying on UnobservedTaskException).

💡 One minimal fix 
-        _ = StartPipeListenerAsync();
+        _ = StartPipeListenerAsync().ContinueWith(
+            t =&gt; LogService.Logger?.Error(t.Exception, "Named-pipe listener failed"),
+            CancellationToken.None,
+            TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
+            TaskScheduler.Default);
📝 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
// Start listening for activation requests from subsequent instances.
// Fire-and-forget is intentional — the listener loop runs for the app
// lifetime and is cancelled via _pipeCts on OnExit.
_ = StartPipeListenerAsync();
// Start listening for activation requests from subsequent instances.
// Fire-and-forget is intentional — the listener loop runs for the app
// lifetime and is cancelled via _pipeCts on OnExit.
_ = StartPipeListenerAsync().ContinueWith(
t => LogService.Logger?.Error(t.Exception, "Named-pipe listener failed"),
CancellationToken.None,
TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
TaskScheduler.Default);
🤖 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 81 - 84,
StartPipeListenerAsync is started fire-and-forget, so unhandled exceptions may
never be observed; instead either ensure StartPipeListenerAsync catches and logs
all exceptions internally (wrap its listener loop in try/catch and log before
rethrowing or swallowing), or attach an explicit fault-only continuation at the
call site to log exceptions (replace the "_ = StartPipeListenerAsync();" call
with StartPipeListenerAsync().ContinueWith(...,
TaskContinuationOptions.OnlyOnFaulted) and log t.Exception), while preserving
cancellation via _pipeCts and the existing OnExit behavior.

}

protected override void OnExit(ExitEventArgs e)
Expand Down Expand Up @@ -127,9 +129,11 @@ private static void ActivateExistingInstance()

/// <summary>
/// Listens for named pipe connections from subsequent instances and
/// activates the main window when one connects.
/// activates the main window when one connects. Returns a Task so the
/// caller can fire-and-forget without using the async-void anti-pattern;
/// any exception escaping the loop is logged via OnTask (UnobservedTaskException).
/// </summary>
private async void StartPipeListener()
private async Task StartPipeListenerAsync()
{
_pipeCts = new CancellationTokenSource();
var ct = _pipeCts.Token;
Expand Down
19 changes: 9 additions & 10 deletions SysManager/SysManager/Services/StartupService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,13 +305,6 @@ private static void ApplyApprovedState(List<StartupEntry> entries)
catch (System.IO.IOException) { return null; /* I/O error reading key */ }
}

/// <summary>
/// Synchronous overload — delegates to <see cref="SetEnabledAsync"/> for
/// backward compatibility with existing callers and tests.
/// </summary>
public static bool SetEnabled(StartupEntry entry, bool enabled)
=> SetEnabledAsync(entry, enabled).GetAwaiter().GetResult();

/// <summary>
/// Toggle a startup entry on or off by writing to the StartupApproved
/// registry key. This is the same mechanism Task Manager uses.
Expand Down Expand Up @@ -444,9 +437,15 @@ private static async Task<bool> SetTaskSchedulerEnabledAsync(StartupEntry entry,
return false;
}

var stderr = stderrTask.Wait(TimeSpan.FromSeconds(5))
? stderrTask.GetAwaiter().GetResult().Trim()
: string.Empty;
string stderr;
try
{
stderr = (await stderrTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false)).Trim();
}
catch (TimeoutException)
{
stderr = string.Empty;
}

if (proc.ExitCode == 0)
{
Expand Down
6 changes: 3 additions & 3 deletions SysManager/SysManager/SysManager.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
<RootNamespace>SysManager</RootNamespace>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<NoWarn>NU1603;NU1701</NoWarn>
<Version>1.18.1</Version>
<FileVersion>1.18.1.0</FileVersion>
<AssemblyVersion>1.18.1.0</AssemblyVersion>
<Version>1.18.2</Version>
<FileVersion>1.18.2.0</FileVersion>
<AssemblyVersion>1.18.2.0</AssemblyVersion>
<Product>SysManager</Product>
<Description>SysManager — Windows system monitoring toolkit by laurentiu021. Network, updates, health, logs, safe deep cleanup.</Description>
<PackageProjectUrl>https://github.com/laurentiu021/SystemManager</PackageProjectUrl>
Expand Down
Loading