fix: WMI disposal, operation lock deadlock, separate CTS#384
Merged
Conversation
- SystemInfoService: add using(mo) + using collection in QueryMemory and QueryDisks (4 foreach loops — COM handle leak) - FixedDriveService: add using(mo) + using collection in MSFT_PhysicalDisk - DeepCleanupViewModel: extract ScanCoreAsync() without lock acquisition; CleanAsync calls ScanCoreAsync instead of ScanAsync (which re-acquires the same disk lock and always failed silently) - WindowsFeaturesViewModel: separate _scanCts and _toggleCts so toggle operations do not cancel a running feature scan - Add audit JSON files to .gitignore
📝 WalkthroughWalkthroughThis PR backports three targeted bug fixes for v0.48.18: WMI resource disposal in system query services, deadlock prevention in deep cleanup scan flow via method extraction, and independent cancellation control in Windows features to prevent toggling from interrupting scans. ChangesWMI Disposal and Concurrency Control Fixes
🎯 2 (Simple) | ⏱️ ~15 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: 1
🤖 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/ViewModels/WindowsFeaturesViewModel.cs`:
- Around line 22-23: The current split of _scanCts and _toggleCts allows Scan
and Toggle to run concurrently while both flip a single IsBusy flag, producing
transient incorrect idle states; change the busy tracking to an atomic ref-count
or separate busy flags and ensure they are mutated in try/finally. Specifically,
add an integer BusyCount (or two bools IsScanning/IsToggling) and replace direct
sets of IsBusy in the methods that use _scanCts and _toggleCts with
Interlocked.Increment/Decrement on BusyCount (or set/clear IsScanning and
compute IsBusy = IsScanning || IsToggling), and always decrement/clear in a
finally block so IsBusy remains true until all operations complete; update the
IsBusy getter to return BusyCount > 0 (or the OR of the two flags).
🪄 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: 1d7679dc-2bdd-4259-8f5d-dd12e9132b2c
📒 Files selected for processing (5)
CHANGELOG.mdSysManager/SysManager/Services/FixedDriveService.csSysManager/SysManager/Services/SystemInfoService.csSysManager/SysManager/ViewModels/DeepCleanupViewModel.csSysManager/SysManager/ViewModels/WindowsFeaturesViewModel.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 (9)
CHANGELOG.md (1)
9-21: LGTM!SysManager/SysManager/ViewModels/DeepCleanupViewModel.cs (1)
149-157: LGTM!Also applies to: 231-231
SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs (1)
150-154: LGTM!Also applies to: 160-164
SysManager/SysManager/Services/FixedDriveService.cs (1)
60-69: LGTM!SysManager/SysManager/Services/SystemInfoService.cs (5)
29-48: LGTM!
52-68: LGTM!
73-84: LGTM!Also applies to: 91-107
120-152: LGTM!
157-170: LGTM!
Comment on lines
+22
to
+23
| private CancellationTokenSource? _scanCts; | ||
| private CancellationTokenSource? _toggleCts; |
There was a problem hiding this comment.
Add runtime busy guards to prevent overlapping scan/toggle operations.
After splitting cancellation sources, scan and toggle can now overlap, but both methods independently flip a single IsBusy flag. This can transiently report idle while one operation is still running.
💡 Suggested fix
[RelayCommand]
private async Task ScanAsync()
{
+ if (IsBusy) return;
IsBusy = true;
+ ToggleFeatureCommand.NotifyCanExecuteChanged();
IsProgressIndeterminate = true;
StatusMessage = "Querying Windows optional features…";
@@
finally
{
IsBusy = false;
IsProgressIndeterminate = false;
+ ToggleFeatureCommand.NotifyCanExecuteChanged();
}
}
@@
[RelayCommand(CanExecute = nameof(CanToggle))]
private async Task ToggleFeatureAsync(WindowsFeature? feature)
{
- if (feature == null) return;
+ if (feature == null || IsBusy) return;Also applies to: 51-54, 102-104
🤖 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/WindowsFeaturesViewModel.cs` around lines 22
- 23, The current split of _scanCts and _toggleCts allows Scan and Toggle to run
concurrently while both flip a single IsBusy flag, producing transient incorrect
idle states; change the busy tracking to an atomic ref-count or separate busy
flags and ensure they are mutated in try/finally. Specifically, add an integer
BusyCount (or two bools IsScanning/IsToggling) and replace direct sets of IsBusy
in the methods that use _scanCts and _toggleCts with
Interlocked.Increment/Decrement on BusyCount (or set/clear IsScanning and
compute IsBusy = IsScanning || IsToggling), and always decrement/clear in a
finally block so IsBusy remains true until all operations complete; update the
IsBusy getter to return BusyCount > 0 (or the OR of the two flags).
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
- SystemInfoService: add using(mo) + using collection in QueryMemory and QueryDisks (4 foreach loops — COM handle leak) - FixedDriveService: add using(mo) + using collection in MSFT_PhysicalDisk - DeepCleanupViewModel: extract ScanCoreAsync() without lock acquisition; CleanAsync calls ScanCoreAsync instead of ScanAsync (which re-acquires the same disk lock and always failed silently) - WindowsFeaturesViewModel: separate _scanCts and _toggleCts so toggle operations do not cancel a running feature scan - Add audit JSON files to .gitignore Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
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
Fixes WMI COM handle leaks, operation lock deadlock, and shared CTS interference.
WMI Disposal (SVC-43-45, SVC-15)
Operation Lock Deadlock (VM-16)
Shared CTS Interference (VM-17)
Build
Summary by CodeRabbit