fix: drop async-void pipe listener and sync-over-async wrappers#616
Conversation
- App.xaml.cs: rename StartPipeListener (async void) to StartPipeListenerAsync (Task); fire as _ = StartPipeListenerAsync() so escaped exceptions surface via UnobservedTaskException instead of crashing the AppDomain - StartupService: drop the SetEnabled(...) sync wrapper that called .GetAwaiter().GetResult() on SetEnabledAsync; tests migrated to call SetEnabledAsync directly - StartupService.SetTaskSchedulerEnabledAsync: replace stderrTask.Wait(timeout) + GetAwaiter().GetResult() with stderrTask.WaitAsync(timeout) for a clean async + TimeoutException fallback
📝 WalkthroughWalkthroughThis PR refactors async patterns across the SystemManager codebase by removing synchronous wrappers, converting ChangesAsync Pattern Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SysManager/SysManager.Tests/StartupToggleTests.cs (1)
55-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test still hits the success path.
SetEnabledAsyncdoes not consultentry.RegistryKeyhere; forRegistryCurrentUserit opensStartupApproved\Runand writesentry.ValueNamedirectly. With a fresh GUID value name, this call can succeed and setStatusTextto"Disabled", so the test never proves the registry-failure message contract implied by its name. Either assert the success behavior explicitly, or move the “no generic admin message on registry failure” check to a test that can force a real registry write failure.🤖 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.Tests/StartupToggleTests.cs` around lines 55 - 68, The test SetEnabledAsync_RegistryEntry_NeverShowsGenericAdminMessage currently succeeds because SetEnabledAsync (called on StartupService) ignores entry.RegistryKey for RegistryCurrentUser and writes to StartupApproved\Run using entry.ValueName, so a fresh GUID value can be created and StatusText becomes "Disabled"; to fix, either (A) change the assertion to explicitly assert the successful path (e.g., assert StatusText == "Disabled" and no admin message) when Source == StartupSource.RegistryCurrentUser, or (B) move this "no generic admin message on registry failure" check into a new test that forces a registry write failure (for example set Source to RegistryLocalMachine or use an invalid/non-writable registry hive, or mock the registry accessor used by StartupService to throw when SetEnabledAsync is called) and then assert that StatusText and the logged messages meet the failure contract; update the test to reference StartupEntry, ValueName, RegistryKey, StartupSource.RegistryCurrentUser, StartupApproved\\Run, SetEnabledAsync, and StatusText accordingly so it deterministically exercises the intended success or failure path.
🤖 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 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.
---
Outside diff comments:
In `@SysManager/SysManager.Tests/StartupToggleTests.cs`:
- Around line 55-68: The test
SetEnabledAsync_RegistryEntry_NeverShowsGenericAdminMessage currently succeeds
because SetEnabledAsync (called on StartupService) ignores entry.RegistryKey for
RegistryCurrentUser and writes to StartupApproved\Run using entry.ValueName, so
a fresh GUID value can be created and StatusText becomes "Disabled"; to fix,
either (A) change the assertion to explicitly assert the successful path (e.g.,
assert StatusText == "Disabled" and no admin message) when Source ==
StartupSource.RegistryCurrentUser, or (B) move this "no generic admin message on
registry failure" check into a new test that forces a registry write failure
(for example set Source to RegistryLocalMachine or use an invalid/non-writable
registry hive, or mock the registry accessor used by StartupService to throw
when SetEnabledAsync is called) and then assert that StatusText and the logged
messages meet the failure contract; update the test to reference StartupEntry,
ValueName, RegistryKey, StartupSource.RegistryCurrentUser, StartupApproved\\Run,
SetEnabledAsync, and StatusText accordingly so it deterministically exercises
the intended success or failure path.
🪄 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: 691a4f93-e20f-4bf1-a53d-471580f5094d
📒 Files selected for processing (5)
CHANGELOG.mdSysManager/SysManager.Tests/StartupToggleTests.csSysManager/SysManager/App.xaml.csSysManager/SysManager/Services/StartupService.csSysManager/SysManager/SysManager.csproj
📜 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 (3)
SysManager/SysManager/Services/StartupService.cs (1)
440-448: LGTM!SysManager/SysManager.Tests/StartupToggleTests.cs (1)
17-33: LGTM!Also applies to: 36-52, 74-109
SysManager/SysManager/SysManager.csproj (1)
13-15: LGTM!
| // 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(); |
There was a problem hiding this comment.
🧩 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:
- 1: https://devblogs.microsoft.com/dotnet/faq-taskscheduler-unobservedtaskexception-event-doesnt-work/
- 2: https://stackoverflow.com/questions/76528191/what-happens-with-unhandled-exceptions-for-fire-and-forget-tasks-for-c-restfu
- 3: https://stackoverflow.com/questions/3973315/taskscheduler-unobservedtaskexception-never-gets-called
- 4: https://devblogs.microsoft.com/dotnet/task-exception-handling-in-net-4-5/
- 5: https://stackoverflow.com/questions/12226268/what-is-the-recommended-way-to-handle-taskscheduler-unobservedtaskexception-in-g
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 => 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.
| // 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.
1 participant
Summary
Round 3 of the post-1.18.0 audit fixes. Tackles three high-priority async correctness issues left over after PR #614.
What's fixed
App.xaml.cs —
StartPipeListenerwasasync voidBefore:
private async void StartPipeListener(). Any exception escaping the loop (rare but possible: corrupted pipe state, OOM during cancellation) would propagate to the AppDomain unhandled-exception handler and crash the app instead of going through the normal task-exception path.After: renamed to
StartPipeListenerAsync()returningTask.OnStartupinvokes it as_ = StartPipeListenerAsync(). Escaped exceptions now surface viaTaskScheduler.UnobservedTaskException, whichApp.OnTaskalready logs cleanly. Cancellation behavior is unchanged (_pipeCtsis still disposed inOnExit).StartupService — dropped sync-over-async wrapper
Before:
public static bool SetEnabled(StartupEntry, bool) => SetEnabledAsync(...).GetAwaiter().GetResult();— only used by tests. The pattern is brittle: any blocking dependency inSetEnabledAsync(e.g., a future SemaphoreSlim) could deadlock when called from a sync context.After: wrapper deleted.
StartupToggleTestsnow usesasync Tasktest methods thatawait SetEnabledAsyncdirectly — the canonical xUnit async-test pattern.StartupService.SetTaskSchedulerEnabledAsync — async stderr read with timeout
Before:
Sync
Task.Wait(timeout)blocks the current thread;.GetAwaiter().GetResult()rethrows synchronously. Inside an already-async method, this is the wrong pattern.After:
Pure async, with a clean
TimeoutExceptionfallback.Files
App.xaml.cs— rename + signature change + caller updateServices/StartupService.cs— drop sync wrapper, switch toWaitAsyncSysManager.Tests/StartupToggleTests.cs— migrate 5 tests toasync TaskCHANGELOG.md—[1.18.2]entrySysManager.csproj— bump to 1.18.2Audit findings deliberately skipped (verified false alarms)
These were flagged by audit agents but checked by hand and rejected:
ManagementScopeusing: the type does not implementIDisposablein .NET 10 (verified by attemptingusing var scope = new ManagementScope(...)— producesCS1674). Nothing to dispose.ThemeService.Initializeasync: must run synchronously before the first window is shown to avoid a flash of default theme. The settings file is ~200 bytes; the read is sub-millisecond.ActivityLogServicector async: singleton uses lazy initialization;Load()runs on first access from a non-UI context. JSON file is small (max 20 entries). Not a UI freeze risk.LogService[GeneratedRegex]: the regex is built at runtime fromEnvironment.GetFolderPath(UserProfile)so a source generator cannot help. The fallback regex is rarely hit.ProcessManagerService.Thread.Sleep(100): invoked inside a syncSnapshot()method that callers wrap inTask.Run(...). Runs on a thread-pool thread, never on the UI thread.Task.Run(async () => ...): the wrapped lambda callsUpdateGpuUsage()(a sync NvAPI call); withoutTask.Run, the first synchronous portion would run on the UI context. The wrapper is required.dynamicCOM: working code touching the WUA COM API;Marshal.FinalReleaseComObjectis paired correctly. Thedynamicis intentional (no typed interop assembly available). Risk of breakage during refactor outweighs the maintainability win.Test plan
dotnet buildRelease: 0 warnings, 0 errors (main, Tests, UITests)Summary by CodeRabbit
Bug Fixes
Chores