fix: ping CTS leak, Event Log UI-thread block, AMD/Intel GPU name#819
Merged
Conversation
Round 3 (batch 2) — thread/resource safety: - PingMonitorService.Stop dropped the CTS reference (set _cts=null) without disposing it whenever the 1.5s loop wait timed out, leaking the CTS. It now captures the CTS locally and disposes it once the loop finishes — immediately if already complete, else via a ContinueWith. - EventLogService.ReadAsync ran the blocking EventLogReader.ReadEvent() COM call on the caller's UI thread (await Task.Yield only released it momentarily), so large logs froze the UI. Each read now runs on the thread pool via Task.Run. - DashboardViewModel.UpdateGpuUsage only queried NVIDIA via NvAPI, so AMD/Intel users saw no GPU at all. Added a Win32_VideoController (WMI) fallback for the adapter name (usage % stays NVIDIA-only — vendor-specific APIs). StartupViewModel's cross-thread mutation (also flagged in the audit) was already fixed in #585, verified. Test: PingMonitorServiceLifecycleTests covers start/stop/dispose/double-start.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // thread so enumerating large logs never blocks the UI thread the | ||
| // caller awaits on. (await Task.Yield() alone did not move the work | ||
| // off the UI thread — it only released it momentarily per 200 rows.) | ||
| EventRecord? rec; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 3 (batch 2) — thread/resource safety.
Ping monitor leaked its CancellationTokenSource
PingMonitorService.Stopset_cts = nullwithout disposing it whenever the 1.5s loop wait timed out (loop still winding down), leaking the CTS. It now captures the CTS locally and disposes it once the loop actually finishes — immediately if complete, otherwise via aContinueWith.System Logs froze the UI while reading the Event Log
EventLogService.ReadAsyncran the blockingEventLogReader.ReadEvent()COM call on the caller's (UI) thread;await Task.Yield()only released it momentarily per 200 rows, so large logs still froze the app. Each read now runs on the thread pool viaTask.Run.Dashboard GPU only worked for NVIDIA
UpdateGpuUsagequeried only NvAPI, so AMD/Intel users saw no GPU. Added aWin32_VideoController(WMI) fallback for the adapter name. Live usage % stays NVIDIA-only (vendor-specific APIs).Note
StartupViewModel's cross-thread mutation (also in the audit) was already fixed in #585 — verified, no change needed.
Tests & regression
PingMonitorServiceLifecycleTests: start/stop, stop-without-start, repeated cycles, dispose-after-start, double-start idempotency.