fix: sync-over-async, lazy enumeration, specific catches (CQ-004, CQ-010, CQ-011)#372
Conversation
📝 WalkthroughWalkthroughThis PR addresses three quality issues across core system services: removing sync-over-async blocking in process enrichment, switching to lazy enumeration for memory efficiency in cleanup operations, and narrowing exception handling in network diagnostics. ChangesQuality Fixes v0.48.11
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/Services/DeepCleanupService.cs`:
- Around line 381-384: The EnumerateFiles/EnumerateDirectories usage currently
only catches exceptions thrown when creating the IEnumerable but not those
thrown during deferred iteration; move each foreach into its own try/catch so
iteration is protected: for the files loop (in DeepCleanupService) wrap "foreach
(var f in files) yield return f;" inside a try that catches IOException and
UnauthorizedAccessException and continues on exception, and for the dirs loop
wrap "foreach (var d in dirs) stack.Push(d);" inside a try that on
IOException/UnauthorizedAccessException sets dirs to empty or simply continues;
update the blocks around the variables `files`, `dirs`, and the stack push to
ensure iteration exceptions are handled the same way as enumeration
construction.
🪄 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: 6eafaac3-e1a6-4b8d-bc4d-7545c581e734
📒 Files selected for processing (5)
.gitignoreCHANGELOG.mdSysManager/SysManager/Services/DeepCleanupService.csSysManager/SysManager/Services/TracerouteService.csSysManager/SysManager/ViewModels/ProcessManagerViewModel.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: Analyze (csharp)
- GitHub Check: Build & unit tests
🔇 Additional comments (3)
CHANGELOG.md (1)
9-20: LGTM!SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs (1)
57-59: LGTM!SysManager/SysManager/Services/TracerouteService.cs (1)
57-59: LGTM!Also applies to: 106-106
| try { files = Directory.EnumerateFiles(cur); } catch (IOException) { continue; } catch (UnauthorizedAccessException) { continue; } | ||
| try { dirs = Directory.EnumerateDirectories(cur); } catch (IOException) { dirs = []; } catch (UnauthorizedAccessException) { dirs = []; } | ||
| foreach (var f in files) yield return f; | ||
| foreach (var d in dirs) stack.Push(d); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, can Directory.EnumerateFiles/Directory.EnumerateDirectories throw IOException or UnauthorizedAccessException during foreach iteration (MoveNext), not just when the enumerable is created?
💡 Result:
Yes, in.NET, Directory.EnumerateFiles and Directory.EnumerateDirectories can throw exceptions such as UnauthorizedAccessException and IOException during the foreach iteration (specifically when MoveNext is called), not just when the enumerable is initially created [1][2]. Because these methods use deferred execution—acting as wrappers around native file system enumeration APIs—they do not retrieve the entire list of files or directories upfront [3][4]. Instead, they fetch entries incrementally as you iterate [1][2]. Consequently, if the enumerator encounters a directory it cannot access, a file that has been locked, or other I/O-related issues while traversing the file system, the exception will be raised at the point of iteration [1][2]. To handle these scenarios robustly, you can use the EnumerationOptions class (available in modern.NET versions) to configure the behavior of the enumerator [5][6]. Specifically, setting the IgnoreInaccessible property to true allows the enumerator to skip files or directories that would otherwise trigger an UnauthorizedAccessException, enabling the iteration to continue [5][6]. For older versions of.NET or more complex error handling, you may need to implement custom logic to wrap the enumeration in a way that catches and handles these exceptions during the MoveNext process [7][1].
Citations:
- 1: https://stackoverflow.com/questions/13479304/can-iterating-over-directoryinfo-enumeratefiles-throw-an-exception
- 2: https://stackoverflow.com/questions/67573357/how-to-fix-random-filesystemenumerableiterator-movenext-error-the-handle-is-inv
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.io.directory.enumeratedirectories?view=net-9.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.io.directory.enumeratedirectories?view=net-10.0
- 5: https://stackoverflow.com/questions/13130052/directoryinfo-enumeratefiles-causes-unauthorizedaccessexception-and-other
- 6: https://learn.microsoft.com/en-za/answers/questions/1200623/unhandled-exception-system-unauthorizedaccessexcep
- 7: https://stackoverflow.com/questions/5098011/directory-enumeratefiles-unauthorizedaccessexception
🏁 Script executed:
find . -name "DeepCleanupService.cs" -type fRepository: laurentiu021/SystemManager
Length of output: 125
🏁 Script executed:
sed -n '370,410p' ./SysManager/SysManager/Services/DeepCleanupService.cs | cat -nRepository: laurentiu021/SystemManager
Length of output: 1961
Handle deferred Enumerate* exceptions inside iteration.
Directory.EnumerateFiles and Directory.EnumerateDirectories use deferred execution and can throw IOException or UnauthorizedAccessException during foreach iteration (via MoveNext), not just when the enumerable is created. The current exception handling only wraps the enumerable creation, leaving iteration unprotected.
Move the foreach loops inside the try blocks:
Proposed fix for EnumerateFiles
- IEnumerable<string> files;
- IEnumerable<string> dirs;
- try { files = Directory.EnumerateFiles(cur); } catch (IOException) { continue; } catch (UnauthorizedAccessException) { continue; }
- try { dirs = Directory.EnumerateDirectories(cur); } catch (IOException) { dirs = []; } catch (UnauthorizedAccessException) { dirs = []; }
- foreach (var f in files) yield return f;
- foreach (var d in dirs) stack.Push(d);
+ IEnumerable<string> dirs = [];
+ try
+ {
+ foreach (var f in Directory.EnumerateFiles(cur))
+ yield return f;
+ }
+ catch (IOException) { continue; }
+ catch (UnauthorizedAccessException) { continue; }
+
+ try { dirs = Directory.EnumerateDirectories(cur); }
+ catch (IOException) { dirs = []; }
+ catch (UnauthorizedAccessException) { dirs = []; }
+
+ foreach (var d in dirs) stack.Push(d);Proposed fix for EnumerateDirectoriesDepthFirst
- IEnumerable<string> dirs;
- try { dirs = Directory.EnumerateDirectories(cur); } catch (IOException) { continue; } catch (UnauthorizedAccessException) { continue; }
- foreach (var d in dirs) stack.Push(d);
+ try
+ {
+ foreach (var d in Directory.EnumerateDirectories(cur))
+ stack.Push(d);
+ }
+ catch (IOException) { continue; }
+ catch (UnauthorizedAccessException) { continue; }🤖 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/DeepCleanupService.cs` around lines 381 - 384,
The EnumerateFiles/EnumerateDirectories usage currently only catches exceptions
thrown when creating the IEnumerable but not those thrown during deferred
iteration; move each foreach into its own try/catch so iteration is protected:
for the files loop (in DeepCleanupService) wrap "foreach (var f in files) yield
return f;" inside a try that catches IOException and UnauthorizedAccessException
and continues on exception, and for the dirs loop wrap "foreach (var d in dirs)
stack.Push(d);" inside a try that on IOException/UnauthorizedAccessException
sets dirs to empty or simply continues; update the blocks around the variables
`files`, `dirs`, and the stack push to ensure iteration exceptions are handled
the same way as enumeration construction.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| try { ((Action<TracerouteHop>)h).Invoke(hop); } | ||
| catch { /* swallow subscriber errors */ } | ||
| catch (Exception) { /* swallow subscriber errors to protect iteration */ } |
Add CHANGELOG entry for v0.28.14 — CodeQL missed-using-statement fixes. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…010, CQ-011) (#372) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
3 participants
Summary
Addresses 3 MEDIUM findings from QA audit.
Changes
CQ-004: ProcessManagerViewModel (MEDIUM — thread safety)
CQ-010: DeepCleanupService (MEDIUM — performance)
CQ-011: TracerouteService (MEDIUM — error handling)
Housekeeping
Build
Summary by CodeRabbit
Bug Fixes