fix: WMI disposal, Ookla timeout, docs accuracy (CQ-001, CQ-003)#370
Merged
Conversation
📝 WalkthroughWalkthroughThis PR is a maintenance release (0.48.10) that fixes two service-level resource management issues: proper disposal of WMI ChangesRelease 0.48.10
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 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)
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: 2
🤖 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 `@SECURITY.md`:
- Around line 15-18: The support table in SECURITY.md lists 0.48.x and 0.47.x as
supported but the written policy line stating "fixes apply to the latest minor
only" is inconsistent; either update that sentence to state that fixes apply to
the latest two minors or change the table to mark only the latest minor as
supported. Locate the sentence containing "fixes apply to the latest minor only"
and either amend it to "latest two minor releases" (to match rows 0.48.x and
0.47.x) or adjust the table rows (0.48.x, 0.47.x, 0.46.x) so only the intended
supported minor remains checked—ensure the wording and the table entries
(0.48.x, 0.47.x, 0.46.x) are consistent.
In `@SysManager/SysManager/Services/SpeedTestService.cs`:
- Around line 201-210: The awaits using linked cancellation token around
proc.Start(), proc.StandardOutput.ReadToEndAsync(linked),
proc.StandardError.ReadToEndAsync(linked), and proc.WaitForExitAsync(linked) can
throw OperationCanceledException on timeout but currently do not terminate the
child process; wrap these awaits in try/catch/finally (or try/finally with a
catch for OperationCanceledException) and in the finally check the Process
instance (proc) and if !proc.HasExited call proc.Kill(entireProcessTree: true)
(or proc.Kill() if older framework) and await a safe WaitForExit to ensure the
speedtest.exe is terminated before disposing; reference the existing
variables/procedures proc, psi, timeoutCts/linked, Start,
StandardOutput.ReadToEndAsync, StandardError.ReadToEndAsync, and
WaitForExitAsync when making the change.
🪄 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: 778b6855-beae-4482-ac5c-c90f42557374
📒 Files selected for processing (5)
CHANGELOG.mdCONTRIBUTING.mdSECURITY.mdSysManager/SysManager/Services/DiskHealthService.csSysManager/SysManager/Services/SpeedTestService.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 (3)
SysManager/SysManager/Services/DiskHealthService.cs (1)
32-53: LGTM!Also applies to: 76-90
CHANGELOG.md (1)
9-22: LGTM!CONTRIBUTING.md (1)
42-42: LGTM!Also applies to: 78-79
Comment on lines
+15
to
+18
| | 0.48.x | :white_check_mark: | | ||
| | 0.47.x | :white_check_mark: | | ||
| | 0.46.x | :white_check_mark: | | ||
| | 0.45.x | :white_check_mark: | | ||
| | 0.44.x | :x: | | ||
| | < 0.44 | :x: | | ||
| | 0.46.x | :x: | | ||
| | < 0.46 | :x: | |
There was a problem hiding this comment.
Align the support table with the written support policy.
Lines 15-18 now show two supported minors, but Line 10 still says fixes apply to the latest minor only. Please make them consistent to avoid ambiguity in security support commitments.
Suggested doc fix
-Security fixes are applied to the latest minor release only. If you're on an
-older build, the first step is usually to update.
+Security fixes are applied to the latest two minor release lines. If you're on an
+older build, the first step is usually to update.🤖 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 `@SECURITY.md` around lines 15 - 18, The support table in SECURITY.md lists
0.48.x and 0.47.x as supported but the written policy line stating "fixes apply
to the latest minor only" is inconsistent; either update that sentence to state
that fixes apply to the latest two minors or change the table to mark only the
latest minor as supported. Locate the sentence containing "fixes apply to the
latest minor only" and either amend it to "latest two minor releases" (to match
rows 0.48.x and 0.47.x) or adjust the table rows (0.48.x, 0.47.x, 0.46.x) so
only the intended supported minor remains checked—ensure the wording and the
table entries (0.48.x, 0.47.x, 0.46.x) are consistent.
Comment on lines
+201
to
+210
| using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); | ||
| timeoutCts.CancelAfter(TimeSpan.FromMinutes(5)); | ||
| var linked = timeoutCts.Token; | ||
|
|
||
| using var proc = new Process(); | ||
| proc.StartInfo = psi; | ||
| await Task.Run(() => proc.Start(), ct).ConfigureAwait(false); | ||
| var stdout = await proc.StandardOutput.ReadToEndAsync(ct); | ||
| var stderr = await proc.StandardError.ReadToEndAsync(ct); | ||
| await proc.WaitForExitAsync(ct); | ||
| await Task.Run(() => proc.Start(), linked).ConfigureAwait(false); | ||
| var stdout = await proc.StandardOutput.ReadToEndAsync(linked); | ||
| var stderr = await proc.StandardError.ReadToEndAsync(linked); | ||
| await proc.WaitForExitAsync(linked); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether RunOoklaAsync has a cancellation path that kills the spawned process.
fd -i 'SpeedTestService.cs' | while read -r f; do
echo "== $f =="
rg -n -C4 'RunOoklaAsync|CreateLinkedTokenSource|WaitForExitAsync|OperationCanceledException|Kill\(' "$f"
doneRepository: laurentiu021/SystemManager
Length of output: 1315
🏁 Script executed:
#!/bin/bash
# Get the complete RunOoklaAsync method to verify exception handling
rg -n -A 60 'public async Task<SpeedTestResult> RunOoklaAsync' 'SysManager/SysManager/Services/SpeedTestService.cs' | head -100Repository: laurentiu021/SystemManager
Length of output: 2759
Kill the Ookla process on timeout/cancellation to prevent orphan speedtest.exe instances.
Lines 207–210 await with the timeout token, but have no exception handler. If the 5-minute timeout triggers, OperationCanceledException propagates uncaught, leaving speedtest.exe running. The using statement on proc only disposes handles—it doesn't terminate the child process.
Proposed fix
using var proc = new Process();
proc.StartInfo = psi;
-await Task.Run(() => proc.Start(), linked).ConfigureAwait(false);
-var stdout = await proc.StandardOutput.ReadToEndAsync(linked);
-var stderr = await proc.StandardError.ReadToEndAsync(linked);
-await proc.WaitForExitAsync(linked);
+try
+{
+ await Task.Run(() => proc.Start(), linked).ConfigureAwait(false);
+ var stdout = await proc.StandardOutput.ReadToEndAsync(linked);
+ var stderr = await proc.StandardError.ReadToEndAsync(linked);
+ await proc.WaitForExitAsync(linked);
+}
+catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !ct.IsCancellationRequested)
+{
+ try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); } catch { }
+ throw new TimeoutException("Ookla CLI timed out after 5 minutes.");
+}
+catch (OperationCanceledException)
+{
+ try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); } catch { }
+ throw;
+}📝 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
| using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); | |
| timeoutCts.CancelAfter(TimeSpan.FromMinutes(5)); | |
| var linked = timeoutCts.Token; | |
| using var proc = new Process(); | |
| proc.StartInfo = psi; | |
| await Task.Run(() => proc.Start(), ct).ConfigureAwait(false); | |
| var stdout = await proc.StandardOutput.ReadToEndAsync(ct); | |
| var stderr = await proc.StandardError.ReadToEndAsync(ct); | |
| await proc.WaitForExitAsync(ct); | |
| await Task.Run(() => proc.Start(), linked).ConfigureAwait(false); | |
| var stdout = await proc.StandardOutput.ReadToEndAsync(linked); | |
| var stderr = await proc.StandardError.ReadToEndAsync(linked); | |
| await proc.WaitForExitAsync(linked); | |
| using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); | |
| timeoutCts.CancelAfter(TimeSpan.FromMinutes(5)); | |
| var linked = timeoutCts.Token; | |
| using var proc = new Process(); | |
| proc.StartInfo = psi; | |
| try | |
| { | |
| await Task.Run(() => proc.Start(), linked).ConfigureAwait(false); | |
| var stdout = await proc.StandardOutput.ReadToEndAsync(linked); | |
| var stderr = await proc.StandardError.ReadToEndAsync(linked); | |
| await proc.WaitForExitAsync(linked); | |
| } | |
| catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !ct.IsCancellationRequested) | |
| { | |
| try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); } catch { } | |
| throw new TimeoutException("Ookla CLI timed out after 5 minutes."); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); } catch { } | |
| throw; | |
| } |
🤖 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/SpeedTestService.cs` around lines 201 - 210,
The awaits using linked cancellation token around proc.Start(),
proc.StandardOutput.ReadToEndAsync(linked),
proc.StandardError.ReadToEndAsync(linked), and proc.WaitForExitAsync(linked) can
throw OperationCanceledException on timeout but currently do not terminate the
child process; wrap these awaits in try/catch/finally (or try/finally with a
catch for OperationCanceledException) and in the finally check the Process
instance (proc) and if !proc.HasExited call proc.Kill(entireProcessTree: true)
(or proc.Kill() if older framework) and await a safe WaitForExit to ensure the
speedtest.exe is terminated before disposing; reference the existing
variables/procedures proc, psi, timeoutCts/linked, Start,
StandardOutput.ReadToEndAsync, StandardError.ReadToEndAsync, and
WaitForExitAsync when making the change.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced May 15, 2026
fix: PATH hijacking, orphan process kill, log privacy sanitization (SEC-007, SEC-008, PRIV-001)
#376
Merged
Merged
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…ceService (#370) ## Summary Fix 2 CodeQL `cs/missed-using-statement` alerts. ## Changes ### ServiceManagerService.cs - `ServiceController` objects from `GetServices()` now wrapped in `using` block instead of manual `try/finally Dispose()` ### PerformanceService.cs - `Process` objects from `GetProcesses()` in `TrimWorkingSets` now wrapped in `using` block instead of manual `try/finally Dispose()` ## Testing - Build: 0 errors - No behavioral changes — same disposal semantics, cleaner pattern Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
5 tasks
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.
Summary
Addresses 3 HIGH findings from QA audit + 2 documentation accuracy issues.
Code Quality Fixes
CQ-001: DiskHealthService WMI disposal (HIGH)
CQ-003: SpeedTestService Ookla timeout (HIGH)
Documentation Fixes
CONTRIBUTING.md
SECURITY.md
Build
Summary by CodeRabbit
Release Notes - v0.48.10
Bug Fixes
Documentation