fix: functionality fixes — health analyzer, depth sort, history race, multi-disk#405
Merged
Merged
Conversation
… multi-disk - FUNC-M2: HealthAnalyzer no longer claims DNS is clean when DNS is bad - FUNC-M3: TuneUpService sorts directories by depth not string length - FUNC-M4: SpeedTestHistoryService serializes Save/Clear with SemaphoreSlim - FUNC-M5: FixedDriveService maps drive letters to physical disks via partition
📝 WalkthroughWalkthroughThis PR bundles five targeted fixes for System Manager v0.48.25: HealthAnalyzer now returns a ChangesSystem Manager v0.48.25 Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 `@SysManager/SysManager/Services/SpeedTestHistoryService.cs`:
- Around line 39-43: Public LoadAsync currently calls LoadCoreAsync without
acquiring _fileLock, allowing reads to race with SaveAsync/ClearAsync; change
LoadAsync to acquire the same _fileLock used by SaveAsync and ClearAsync, await
LoadCoreAsync while holding the lock, and release the lock in a finally block so
transient JsonException/IOException during concurrent writes don't cause a
temporary empty history to be returned; reference LoadAsync, LoadCoreAsync,
_fileLock, SaveAsync, ClearAsync, File.WriteAllTextAsync and File.Delete when
locating where to add the lock.
- Around line 31-34: The instance-level SemaphoreSlim _fileLock inside
SpeedTestHistoryService doesn't prevent races across multiple
SpeedTestHistoryService instances created by SpeedTestViewModel; change the lock
to a static SemaphoreSlim (e.g., make _fileLock static readonly) so all
instances share the same async mutex, or alternatively change registration so
SpeedTestHistoryService is a singleton in DI or introduce a shared file
coordinator used by SaveAsync/LoadAsync to serialize file access; update
references to _fileLock and ensure SaveAsync/LoadAsync await the shared lock
appropriately.
🪄 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: d4aed670-ced0-47f2-aecb-bfaa5a0f39e9
📒 Files selected for processing (7)
CHANGELOG.mdSysManager/SysManager.Tests/HealthAnalyzerEdgeCaseTests.csSysManager/SysManager.Tests/HealthAnalyzerTests.csSysManager/SysManager/Services/FixedDriveService.csSysManager/SysManager/Services/HealthAnalyzer.csSysManager/SysManager/Services/SpeedTestHistoryService.csSysManager/SysManager/Services/TuneUpService.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 (7)
CHANGELOG.md (1)
9-24: LGTM!SysManager/SysManager/Services/TuneUpService.cs (1)
185-190: LGTM!SysManager/SysManager/Services/HealthAnalyzer.cs (1)
93-109: LGTM!SysManager/SysManager.Tests/HealthAnalyzerEdgeCaseTests.cs (1)
136-147: LGTM!SysManager/SysManager.Tests/HealthAnalyzerTests.cs (1)
186-188: LGTM!SysManager/SysManager/Services/FixedDriveService.cs (1)
72-107: LGTM!SysManager/SysManager/Services/SpeedTestHistoryService.cs (1)
80-83: LGTM!Also applies to: 117-120, 128-138, 169-172
Comment on lines
+31
to
+34
| // FUNC-M4: Serialize all file operations to prevent concurrent SaveAsync | ||
| // calls from racing (load-modify-save is not atomic). A SemaphoreSlim(1,1) | ||
| // acts as an async-compatible mutex. | ||
| private readonly SemaphoreSlim _fileLock = new(1, 1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "DI registrations or manual construction:"
rg -n -C2 '\b(AddSingleton|AddScoped|AddTransient)\s*<\s*SpeedTestHistoryService\b|new\s+SpeedTestHistoryService\s*\('
echo
echo "All references to SpeedTestHistoryService:"
rg -n -C2 '\bSpeedTestHistoryService\b'Repository: laurentiu021/SystemManager
Length of output: 8857
The instance-level lock does not prevent cross-instance writes; make it static or use a shared coordinator.
The service is instantiated manually in SpeedTestViewModel (line 19: private readonly SpeedTestHistoryService _history = new();), not registered as a singleton. Each ViewModel instance therefore has its own _fileLock, so concurrent saves from different ViewModels still race. Consider a static lock, a shared file coordinator, or registering the service as singleton in DI.
🤖 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/SpeedTestHistoryService.cs` around lines 31 -
34, The instance-level SemaphoreSlim _fileLock inside SpeedTestHistoryService
doesn't prevent races across multiple SpeedTestHistoryService instances created
by SpeedTestViewModel; change the lock to a static SemaphoreSlim (e.g., make
_fileLock static readonly) so all instances share the same async mutex, or
alternatively change registration so SpeedTestHistoryService is a singleton in
DI or introduce a shared file coordinator used by SaveAsync/LoadAsync to
serialize file access; update references to _fileLock and ensure
SaveAsync/LoadAsync await the shared lock appropriately.
Comment on lines
39
to
+43
| public async Task<List<SpeedTestResult>> LoadAsync(CancellationToken ct = default) | ||
| => await LoadCoreAsync(ct).ConfigureAwait(false); | ||
|
|
||
| /// <summary>Internal load without locking — called from within locked sections.</summary> | ||
| private async Task<List<SpeedTestResult>> LoadCoreAsync(CancellationToken ct) |
There was a problem hiding this comment.
Lock LoadAsync too.
Line 39 still lets public reads bypass _fileLock, so a concurrent SaveAsync/ClearAsync can overlap with File.WriteAllTextAsync/File.Delete. In that window, LoadAsync can return an empty list from a transient JsonException/IOException, which makes history briefly disappear for callers even though the write succeeds.
Suggested fix
public async Task<List<SpeedTestResult>> LoadAsync(CancellationToken ct = default)
- => await LoadCoreAsync(ct).ConfigureAwait(false);
+{
+ await _fileLock.WaitAsync(ct).ConfigureAwait(false);
+ try
+ {
+ return await LoadCoreAsync(ct).ConfigureAwait(false);
+ }
+ finally
+ {
+ _fileLock.Release();
+ }
+}🤖 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/SpeedTestHistoryService.cs` around lines 39 -
43, Public LoadAsync currently calls LoadCoreAsync without acquiring _fileLock,
allowing reads to race with SaveAsync/ClearAsync; change LoadAsync to acquire
the same _fileLock used by SaveAsync and ClearAsync, await LoadCoreAsync while
holding the lock, and release the lock in a finally block so transient
JsonException/IOException during concurrent writes don't cause a temporary empty
history to be returned; reference LoadAsync, LoadCoreAsync, _fileLock,
SaveAsync, ClearAsync, File.WriteAllTextAsync and File.Delete when locating
where to add the lock.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…ssages (#427) ## Summary **Batch 7** of the QA bug fix series. ### Issue #405 — Registry modifications not logged Added Serilog audit logging to all PerformanceService registry modification methods: - \SetGameMode\ — logs AllowAutoGameMode value - \SetXboxGameBar\ — logs AppCaptureEnabled and GameDVR_Enabled values - \SetGpuMaxPerformance\ — logs DisableDynamicPstate value and subkey - \SetUiEffects\ — logs visual effects action ### Issue #407 — Generic error messages Replaced generic \Error: {message}\ with operation-specific context across 3 ViewModels: - **PerformanceViewModel** (26 replacements): Power plan change failed, GPU setting change failed, Game Mode change failed, etc. - **ServicesViewModel** (6 replacements): Start service failed, Stop service failed, etc. - **SystemHealthViewModel** (6 replacements): System health scan failed, Disk report failed, etc. ### Testing - Build: 0 errors Closes #405, Closes #407 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Adds CHANGELOG entries for all 9 releases from the QA bug fix session: - **v0.28.16** — Dispose lifecycle (#395, #410) - **v0.28.17** — CTS disposal + bare catch (#396, #413) - **v0.28.18** — Input validation + null checks (#397, #398) - **v0.28.19** — JSON error handling (#400) - **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401, #402, #403) - **v0.28.21** — Audit logging + error messages (#405, #407) - **v0.28.22** — SHA256 verification (#408, #409) - **v0.28.23** — Service timeout + snapshot persist + traceroute DNS (#414, #415, #416) - **v0.28.24** — Accessibility (#411) 18 bugs fixed in total. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
… multi-disk (#405) - FUNC-M2: HealthAnalyzer no longer claims DNS is clean when DNS is bad - FUNC-M3: TuneUpService sorts directories by depth not string length - FUNC-M4: SpeedTestHistoryService serializes Save/Clear with SemaphoreSlim - FUNC-M5: FixedDriveService maps drive letters to physical disks via partition 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
Addresses MEDIUM functionality/logic findings from the comprehensive code review.
Fixes
Tests updated
Files changed (7)
Services: HealthAnalyzer, TuneUpService, SpeedTestHistoryService, FixedDriveService
Tests: HealthAnalyzerTests, HealthAnalyzerEdgeCaseTests
Docs: CHANGELOG.md
Build
0 errors, 0 new warnings.
Summary by CodeRabbit
Bug Fixes
Tests