Skip to content

fix: reparse point checks, safe enumeration, async void guard#388

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/symlinks-threading-enumeration
May 15, 2026
Merged

fix: reparse point checks, safe enumeration, async void guard#388
laurentiu021 merged 1 commit into
mainfrom
fix/symlinks-threading-enumeration

Conversation

@laurentiu021

@laurentiu021 laurentiu021 commented May 15, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes infinite loop risk on symlinks, crash on mid-enumeration file access errors, and async void crash risk.

Reparse Point Check (SVC-11, SVC-19)

  • DuplicateFileService and LargeFileScanner traversed directories without checking for reparse points (symlinks, junctions). Circular symlinks could cause infinite loops.
  • Fix: Added FileAttributes.ReparsePoint check before pushing directories onto the traversal stack, matching the existing pattern in DiskAnalyzerService.

Safe Enumeration (CR-02)

  • DeepCleanupService — EnumerateFiles() only caught exceptions at enumerator creation, not during MoveNext() iteration. Files becoming inaccessible mid-scan caused unhandled exceptions.
  • Fix: Wrapped iteration with explicit MoveNext() call inside try-catch for IOException and UnauthorizedAccessException.

Async Void Guard (SVC-47)

  • TrayIconService — OnTimerTick is async void (required by event handler signature). If UpdateTooltipAsync threw an unexpected exception type, it would crash the entire application.
  • Fix: Wrapped the entire call in try-catch(Exception) with logging.

Build

  • 0 errors, 16 warnings (all pre-existing)
  • Tests project: 0 errors

Summary by CodeRabbit

Bug Fixes

  • Enhanced stability during file scanning operations when files become inaccessible or permissions change
  • Fixed potential crashes from unhandled exceptions that could occur during system tray updates
  • Improved handling of special directory types including symbolic links and junction points during traversal

Review Change Stack

- DuplicateFileService: skip reparse points (symlinks/junctions) during
  directory traversal to prevent infinite loops
- LargeFileScanner: same reparse point check
- DeepCleanupService: catch IOException/UnauthorizedAccessException during
  MoveNext() iteration in EnumerateFiles (not just at creation)
- TrayIconService: wrap OnTimerTick async void in try-catch to prevent
  unhandled exceptions from crashing the application
@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR hardens four core services against filesystem and async operation failures. Two directory traversal services now skip reparse points to prevent infinite loops. File enumeration and async timer execution gain exception handling to gracefully degrade when filesystem access or exception propagation fails.

Changes

Stability Hardening for Filesystem Traversal and Async Operations

Layer / File(s) Summary
Reparse point filtering and attribute exception handling in directory traversal
SysManager/SysManager/Services/DuplicateFileService.cs, SysManager/SysManager/Services/LargeFileScanner.cs
DuplicateFileService and LargeFileScanner filter reparse-point directories before traversal to prevent infinite loops, with exception handling for attribute retrieval failures to continue scanning safely when access denied.
Exception handling in file enumeration iteration and async void timer execution
SysManager/SysManager/Services/DeepCleanupService.cs, SysManager/SysManager/Services/TrayIconService.cs
DeepCleanupService catches IO and access exceptions during file enumeration's MoveNext() calls to skip inaccessible entries. TrayIconService wraps async tooltip updates in try-catch to log warnings instead of crashing on unhandled exceptions in the timer callback.
Release notes for 0.48.19
CHANGELOG.md
Release notes document the four targeted stability fixes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 Reparse points beware, we skip you today,
No infinite loops shall come out to play!
Exceptions caught kindly, no crashes on sight,
Our services harden—robust and tight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title comprehensively summarizes the three main changes in the pull request: reparse point checks, safe enumeration handling, and async void exception guard.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/symlinks-threading-enumeration

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
SysManager/SysManager/Services/TrayIconService.cs (1)

131-135: ⚡ Quick win

Log the exception object to preserve stack trace context.

Catching here is correct, but logging only ex.Message loses diagnostics; prefer Serilog’s exception overload.

♻️ Proposed change
-            Log.Warning("TrayIcon timer tick failed: {Error}", ex.Message);
+            Log.Warning(ex, "TrayIcon timer tick failed");
🤖 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/TrayIconService.cs` around lines 131 - 135,
The catch block in TrayIconService currently logs only ex.Message which loses
stack trace info; update the catch in the timer tick handler (the catch in
TrayIconService's timer callback) to use Serilog's exception overload so the
full Exception object is logged (e.g., call Log.Warning(ex, "TrayIcon timer tick
failed") or equivalent) to preserve stack and exception context.
🤖 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.

Nitpick comments:
In `@SysManager/SysManager/Services/TrayIconService.cs`:
- Around line 131-135: The catch block in TrayIconService currently logs only
ex.Message which loses stack trace info; update the catch in the timer tick
handler (the catch in TrayIconService's timer callback) to use Serilog's
exception overload so the full Exception object is logged (e.g., call
Log.Warning(ex, "TrayIcon timer tick failed") or equivalent) to preserve stack
and exception context.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c8be5f81-04b9-4e8b-9084-47ce20dbba08

📥 Commits

Reviewing files that changed from the base of the PR and between 7580420 and 66c904e.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • SysManager/SysManager/Services/DeepCleanupService.cs
  • SysManager/SysManager/Services/DuplicateFileService.cs
  • SysManager/SysManager/Services/LargeFileScanner.cs
  • SysManager/SysManager/Services/TrayIconService.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 (4)
CHANGELOG.md (1)

9-21: LGTM!

SysManager/SysManager/Services/DuplicateFileService.cs (1)

110-119: LGTM!

SysManager/SysManager/Services/LargeFileScanner.cs (1)

119-128: LGTM!

SysManager/SysManager/Services/DeepCleanupService.cs (1)

383-395: LGTM!

Comment on lines +131 to +135
catch (Exception ex)
{
// async void must never throw — unhandled exceptions crash the app.
Log.Warning("TrayIcon timer tick failed: {Error}", ex.Message);
}
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 43.47826% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
SysManager/SysManager/Services/TrayIconService.cs 0.00% 5 Missing ⚠️
...anager/SysManager/Services/DuplicateFileService.cs 50.00% 2 Missing and 1 partial ⚠️
SysManager/SysManager/Services/LargeFileScanner.cs 50.00% 2 Missing and 1 partial ⚠️
...sManager/SysManager/Services/DeepCleanupService.cs 66.66% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@laurentiu021
laurentiu021 merged commit 0627074 into main May 15, 2026
5 checks passed
@laurentiu021
laurentiu021 deleted the fix/symlinks-threading-enumeration branch May 15, 2026 09:52
laurentiu021 added a commit that referenced this pull request May 22, 2026
- DuplicateFileService: skip reparse points (symlinks/junctions) during
  directory traversal to prevent infinite loops
- LargeFileScanner: same reparse point check
- DeepCleanupService: catch IOException/UnauthorizedAccessException during
  MoveNext() iteration in EnumerateFiles (not just at creation)
- TrayIconService: wrap OnTimerTick async void in try-catch to prevent
  unhandled exceptions from crashing the application

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants